fix: resolve #749 -- osm.modified.timestamp is milliseconds, and decode Feature.id in error messages - #750
Merged
Merged
Conversation
Caught live during the first real smoke test (2026-08-21): #747's `server["datacenter"]["location"]["name"]` was wrong -- confirmed against hcloud-go's actual schema/server.go, `location` sits directly on the server object, not nested under `datacenter`. Worse than the wrong field name itself: only the two `describe` calls were wrapped in try/except, not the field access that followed, so the KeyError raised straight out of `cmd_destroy" before it ran a single deletion command -- exactly the failure mode this function's own docstring said it was designed to avoid. The VM had to be torn down by hand via plain `hcloud` commands while this was diagnosed. Fix: the whole body of teardown_cost_note is now one try/except (subprocess.CalledProcessError, KeyError, TypeError, ValueError) -- covering both the describe calls and everything read from their result -- so a similarly wrong assumption about some other field can never again block deletion, regardless of which field turns out to be wrong next time. Also fixes the field name itself and adds a regression test reproducing the actual incident (a response shape that's missing the expected key), plus corrects the two existing tests' fixtures, which had encoded the same wrong assumption the code made and so didn't catch this before it shipped. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…#749) Two error messages in conflate/writer.rs printed the raw Feature.id-encoded value (raw_osm_id * 10 + {1,2,3}) instead of the actual OSM (type, id) -- confusing and actively misleading when debugging: the encoded number doesn't resolve to anything on openstreetmap.org, and following it there cost real time while investigating #749 (the value in that crash, 6870158061, decodes to node/687015806 -- a real, ordinary Swiss supermarket node with a perfectly normal edit history; the encoded form looked like garbage data by comparison). Reuses decode_member_id, already used a few lines below for the same purpose on osm_relation_members. Adds a regression test using the exact real numbers from the #749 incident. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Root cause of #749's crash, fully re-derived and empirically verified against the real europe/switzerland extract (downloaded locally, no cloud spend) after an earlier, incomplete diagnosis: osm_pbf_iter's Info.timestamp is already date_granularity-scaled to milliseconds for a DenseNodes-encoded node (nearly every node in a real extract) -- that's what DenseInfoParser has always done, per the OSM PBF format spec's own description of date_granularity ("normally... milliseconds since the 1970 epoch"). This project's writer.rs treated every Feature.timestamp as whole seconds unconditionally -- correct by pure coincidence for ways/relations (osm_pbf_iter's OLD Info::parse, used for those, returned the raw unscaled wire value, which happens to equal true seconds when date_granularity is the near-universal default of 1000), but wrong for every dense-encoded node, which is almost certainly why the pipeline crashed on the very first OSM-node-matched row it tried to write, not some rare corrupted record. Fix: UtcTimestamp::from_unix_timestamp_millis (new, mirroring the existing unix_timestamp_millis the Serialize impl already used the inverse of) replaces the direct time::UtcDateTime::from_unix_timestamp call in conflate/writer.rs. feature.proto's timestamp field doc comment corrected from "seconds" to "milliseconds", with the why. This is coupled to a matching fix landing upstream in osm_pbf_iter (github.com/astro/rust-osm-pbf-iter, PR pending review): that crate's Info::parse (used for ways, relations, and any non-dense-encoded node) currently returns the raw, un-scaled wire value -- silently wrong (not crashing) once this project's writer.rs starts treating every Feature.timestamp as milliseconds, unless osm_pbf_iter is *also* fixed to consistently scale by date_granularity everywhere DenseInfoParser already does. Verified this precise interaction locally via a temporary [patch.crates-io] path override during development, ahead of the real release -- and again just now against the actual published osm_pbf_iter 0.2.3 (astro/rust-osm-pbf-iter#96, released by @brawer) via a plain `cargo build`/`cargo test`: full test suite (248 lib + 4 integration tests) passes; way/relation timestamps come out correct too, not just node timestamps. Cargo.toml's osm_pbf_iter requirement is ">=0.2.3", not the usual ^0.2.2 -- this project's timestamp handling only makes sense paired with that release. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
brawer
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves #749. Two commits, both discovered while tracking that issue down:
1. The actual root cause:
osm.modified.timestampis milliseconds, not secondsFully re-derived and empirically verified against the real
europe/switzerlandextract (downloaded locally, no cloud spend), after an earlier, incomplete diagnosis posted to the issue.osm_pbf_iter'sInfo.timestampis alreadydate_granularity-scaled to milliseconds for aDenseNodes-encoded node (nearly every node in a real extract) -- that's whatDenseInfoParserhas always correctly done. This project'swriter.rstreated everyFeature.timestampas whole seconds unconditionally -- correct by pure coincidence for ways/relations, wrong for every dense-encoded node, which is almost certainly why the pipeline crashed on the very first OSM-node-matched row it tried to write, not some rare corrupted record.Fix: new
UtcTimestamp::from_unix_timestamp_millisreplaces the directtime::UtcDateTime::from_unix_timestampcall inconflate/writer.rs.feature.proto'stimestampfield doc comment corrected from "seconds" to "milliseconds".This was coupled to a matching fix upstream in
osm_pbf_iter-- without it, way/relation timestamps would go from "accidentally correct" to "silently wrong" the moment this project starts treatingFeature.timestampas milliseconds. That fix has since been released asosm_pbf_iter0.2.3 (astro/rust-osm-pbf-iter#96) --Cargo.toml's requirement is bumped to">=0.2.3", and this is now verified against the real published release (not a local patch): full test suite (248 lib + 4 integration tests) passes, way/relation timestamps come out correct too, not just node timestamps.2.
Feature.id-encoded values in error messages, decoded back tonode/way/relationidsTwo error messages in
conflate/writer.rsprinted the rawFeature.id-encoded value instead of the actual OSM(type, id)-- actively misleading when debugging: the encoded number doesn't resolve to anything on openstreetmap.org, and following it there cost real time while investigating #749 (the value in that crash,6870158061, decodes tonode/687015806-- a real, entirely ordinary Swiss supermarket node).Testing
cargo fmt/cargo clippy --all-targets -- -D warnings/cargo testall clean. New regression tests: one confirming the real incident's exact timestamp value now parses to the correct real edit date, one confirming a genuinely out-of-range value still fails with the decodednode/687015806id in its message (not the raw encoded value).🤖 Generated with Claude Code