Repository navigation
Conversation
The tar format makes every output offset computable before writing a single byte, given a manifest of (GraphId, size) pairs. ExtractLayout is that pure computation; write_extract_from_source streams the archive into any Write in a single pass (no Seek, no temp files), and write_extract drives it from a source TarballTileProvider. The canonical archive order is ascending by (level, tile index); note that GraphId's Ord deliberately orders levels in reverse, so the writer sorts its own manifest and never relies on caller order. Output is byte-deterministic: fixed ustar header metadata (mtime 0, uid/gid 0, mode 644) and spec-compliant entry paths. Also adds the minimal read accessors this needs: tile sizes from the tarball provider's index, and offset/size/graph ID getters on TileIndexBinEntry. Also add the extract module, which this writer is the first part of. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
MmapTilePointer::as_tile_bytes makes a slice from an index offset and size without a bounds check. Before this change, TarballTileProvider did not check that these ranges were inside the file, so a corrupt or truncated index.bin could make safe calls read outside the memory map. - TileIndexBinEntry::validated now also checks that offset + size fits in the archive (with checked_add, so a huge offset cannot wrap). A failure is the new InvalidIndexBinError::OutOfBounds. - The provider reads the archive length before it reads the index, so all entry checks run in one pass and an invalid index fails before the file is mapped. After the map is created, the provider makes sure that the map length is the length that it checked. - New tests: check_range at the exact edge and on overflow, validated() at the edge, a real tarball whose index points past the end of the file, and a proptest which makes sure that the read path accepts every entry that the write path creates. - tempfile is a new dev-dependency for the test tarball. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- ExtractLayout::from_manifest returns the new ExtractLayoutError
(EmptyManifest, DuplicateTile, EmptyTile, and InvalidIndexBin).
ExtractWriteError wraps it in the Layout variant. A tile with a size
of zero is now an error, because a graph tile always has a header.
- A tile that is not in the source extract is now
ExtractWriteError::MissingTile(GraphId), and an error from the byte
source is Source { graph_id, source }. Before this change, both were
a Provider error without the tile ID.
- TileBytesSource has an associated Error type, so a source that is
not a tarball does not have to put its errors in
GraphTileProviderError. ExtractWriteError<E> has a default type
parameter, so the tarball path keeps the same signature.
- The SAFETY comment in with_tile_bytes now refers to the bounds check
that the provider does when it opens the tarball.
- The round-trip test uses tempfile, so the file is removed even if an
assertion fails.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
main.rs now contains only the clap entry point, the tracing set-up, and the dispatch. Each subcommand is a module in src/commands with its clap arguments and a run function, so a new subcommand does not make main.rs larger. This is a refactor only: get-edge moves into commands/get_edge.rs, and the help output is the same as before. The only code change is that parse_valhalla_data_paths takes a &Path (clippy: ptr_arg). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
valinor-cli extract <SOURCE> <OUTPUT> --tile LEVEL/TILE_INDEX copies the given tiles of a source extract tarball into a new, spec-compliant extract with write_extract. --tile can be repeated or given a comma-separated list. The command prints a one-line summary on stderr, and removes a partial output file if the write fails. The extract subcommand takes a tarball directly, not a Valhalla JSON config. So the config path moves from a global argument into the get-edge arguments (valinor-cli get-edge <CONFIG> <GRAPH_ID>). The VALHALLA_CONFIG environment variable still works. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The tar writer and the directory provider both build the path of a tile file from a graph ID. Before this change, the writer turned the PathBuf from GraphId::file_path back into a string with `/` separators. Now GraphId::relative_path builds the `/`-separated string directly, and file_path uses it. relative_path also returns InvalidGraphIdError::Level for levels 4 to 7. Before this change, file_path panicked on an index out of bounds for these levels. GraphId now implements FromStr for a packed u64 value or the level/tile_id/index form, and GraphId::parse_tile parses the level/tile_id form of a whole tile. Both use the same component parser and return the new ParseGraphIdError. A later commit uses them in place of the separate parsers in the get-edge and extract subcommands. InvalidGraphIdError and ParseGraphIdError are now re-exported at the crate root, so callers can name them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Before this change, 22 tests built each fixture path with
PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("fixtures").join(...).
The new test_fixtures module gives each fixture a function with a short
description of what the fixture contains, so a test can find a fixture
by name. This is a test-only change.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The tarball provider and the extract writer each defined the tar block size, and each used the "index.bin" entry name. Both constants are now defined once, next to the index.bin format in the tarball provider, and the writer imports them. The test of TileIndexBinEntry::validated now calculates the end of the first fixture entry from the parsed entry (offset plus size), not from numbers copied into the test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
write_extract now takes any TileBytesSource and a list of tile IDs. The source gives the size and the bytes of each tile, so a caller never gives tile sizes by hand. TarballTileProvider (for all MUT and CACHE values) and DirectoryGraphTileProvider implement the trait, so an extract can be written from an extract tarball or from a tile directory. - TileBytesSource moves from the extract module to tile_provider, next to GraphTileProvider, and gets a tile_size method. GraphTileProvider cannot be used for this: a GraphTileView does not keep the bytes that it was decoded from. - The directory provider reads the size from the file metadata and the bytes from the file, under the per-tile lock. It does not use its tile cache, because a copy reads each tile once. A tile file larger than u32::MAX bytes is the new GraphTileProviderError::TileTooLarge. - write_extract_from_source and ExtractLayout are no longer public. The inherent TarballTileProvider::tile_size is replaced by the trait method. - The layout code names its cursor next_entry_offset and calculates the length of each entry with entry_len, in place of inline offset arithmetic. The writer counts the bytes that it writes, checks each payload offset against the layout in debug builds, and returns the real count. - The tests express the expected offsets in blocks, and compare the index with the entry offsets that the tar crate finds. A new proptest does the same for random tile sets. A new test makes sure that the directory and tarball fixtures give byte-identical extracts. - write_extract tests now cover an empty tile list, the same tile with two feature indexes, a missing tile (with no bytes written), a size mismatch, and a source error. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- valinor-cli extract now accepts a tile directory as the source, as well as an extract tarball, because write_extract takes any tile source. - The command writes the extract to a temporary file in the output directory, syncs it, and then renames it to the output path. So the output path is either the complete extract or what it was before the command. Before this change, the command truncated the output first, and only tried to remove a partial output after an error. The temporary file gets the permissions that File::create gives (0o666 minus the umask), not the 0o600 default of tempfile. tempfile moves from the dev-dependencies to the dependencies. - --tile uses GraphId::parse_tile, and get-edge parses its graph ID with the FromStr implementation of GraphId. The two local parsers are removed. - ExtractWriteError::MissingTile now says "not in the source", because the source can be a directory. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A search for other duplicates in the files of this PR found two more: - The "gph" extension was a literal in the directory provider and a private constant in the extract writer. It is now TILE_FILE_EXTENSION in tile_provider. - The writer calculated size_of::<TileIndexBinEntry>() again, although tarball.rs already defines INDEX_ENTRY_SIZE. That constant is now pub(crate), and the writer uses it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Remove the comments that restate the code, tell the design story, or explain a decision that belongs in the PR. Keep the contracts, the `# Errors` sections, the guards, and the facts that the code does not show (for example, the block offsets in test_layout_offsets). The help text of `valinor-cli extract` is shorter. It no longer says why the command takes a path in place of a Valhalla config. This commit changes only comments and docstrings. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Archdoog
force-pushed
the
feat/extract-integration-tests
branch
from
October 21, 2026 23:46
79edca5 to
30ee1af
Compare
Before this change, 22 tests built each fixture path with
PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("fixtures").join(...).
The new test_fixtures module gives each fixture a function. Each
function has a short description of its fixture, so a test can find a
fixture by name. This is a test-only change.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
MmapTilePointer::as_tile_bytes makes a slice from an index offset and size without a bounds check. Before this change, TarballTileProvider did not check that these ranges were inside the file. So a corrupt or truncated index.bin could make safe calls read outside the memory map. - TileIndexBinEntry::validated now also checks that offset + size fits in the archive. It uses checked_add, so a huge offset cannot wrap. A failure is the new InvalidIndexBinError::OutOfBounds. - The provider reads the archive length before it reads the index. So all entry checks run in one pass, and an invalid index fails before the provider maps the file. After it maps the file, the provider makes sure that the map length is the length that it checked. - New tests examine check_range at the exact edge and on overflow, and validated() at the edge. A test opens a real tarball whose index points past the end of the file. A proptest makes sure that the read path accepts every entry that the write path creates. - TileIndexBinEntry::graph_id, offset, and size are now public, so that tests and other readers can examine a parsed entry. - tempfile is a new dev-dependency for the test tarball. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Code that copies tiles verbatim, such as an extract writer, must know the size of each tile before it reads any tile data. It must also get the raw bytes of each tile. Before this change, GraphTileProvider gave only decoded tiles. - GraphTileHandle now has tile_bytes, which returns the bytes that the handle decoded. OwnedGraphTileHandle, MmapGraphTileHandle, and Arc<T> implement the trait. The handles do not copy the bytes. - GraphTileProvider::TileHandle must now implement GraphTileHandle. - GraphTileProvider has a new tile_size method. TarballTileProvider gets the size from its index.bin. DirectoryGraphTileProvider gets the size from the file metadata, so it does not read the file or change its tile cache. - GraphTileProviderError has a new TileTooLarge variant for a tile file that has more than u32::MAX bytes. - The tarball test that compares tiles with the directory provider now also compares the handle bytes and the sizes of all three providers. New tests examine a missing tile, a directory at a tile path, and a sparse file that is too large. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Extract tarballs and tile directories use the same relative path for a tile file, with `/` separators. GraphId::file_path returns a PathBuf with the separators of the platform, so it does not give the tar entry name on Windows. GraphId::relative_path builds the `/`-separated string directly, and file_path now uses it. relative_path also returns InvalidGraphIdError::Level for levels 4 to 7. Before this change, file_path panicked on an index out of bounds for these levels. GraphId now implements FromStr for a packed u64 value or the level/tile_id/index form. GraphId::parse_tile parses the level/tile_id form of a whole tile. Both use the same component parser and return the new ParseGraphIdError. InvalidGraphIdError and ParseGraphIdError are now re-exported at the crate root, so callers can name them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
# Conflicts: # valhalla-graphtile/src/tile_provider/directory.rs # valhalla-graphtile/src/tile_provider/mod.rs
This pull request now contains only the extract writer. The split of valinor-cli into subcommand modules moves to a separate pull request, which builds on this one. The extract subcommand moves there too. valinor-cli is the same as on main again. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
main.rs now contains only the clap entry point, the tracing set-up, and the dispatch. Each subcommand is a module in src/commands with its clap arguments and a run function, so a new subcommand does not make main.rs larger. This is a refactor only: get-edge moves into commands/get_edge.rs, and the help output is the same as before. The only code change is that parse_valhalla_data_paths takes a &Path (clippy: ptr_arg). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The writer now reads tiles through GraphTileProvider, so the separate TileBytesSource trait is gone. It gets each tile size from GraphTileProvider::tile_size before the write, and the tile bytes from GraphTileHandle::tile_bytes during the write. The handles decode each tile, so a corrupt tile is a decoding error. - write_extract takes any iterator of graph IDs. It reduces each ID to its base tile, sorts the tiles in archive order, and removes duplicates in one place. - ExtractLayoutError is gone. ExtractWriteError has the new NoTiles and InvalidIndexBin variants, and Provider replaces Source. The DuplicateTile and EmptyTile errors are gone: duplicate IDs give one tile, and an empty tile file fails to decode. - CountingWriter is gone. The layout already has every offset and the total length, and the tests check the offsets with the tar crate. - INDEX_BIN_ENTRY_NAME is now INDEX_BIN_FILE_NAME, and END_OF_ARCHIVE_LEN is now END_OF_ARCHIVE_MARKER_LEN. - The tests use the Andorra tiles through a test provider that can report wrong sizes and fail reads. New tests cover a tile file that changes after the directory provider caches it, and an empty tile file. Every assert has a message. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
valinor-cli extract <SOURCE> <OUTPUT> --tile LEVEL/TILE_ID copies the given tiles of a source into a new extract tarball with write_extract. The source can be an extract tarball or a tile directory. You can give --tile more than once, or give it a comma-separated list. It uses GraphId::parse_tile. The command prints a one-line summary on stderr, which counts each tile once. - The command writes the extract to a temporary file in the output directory, syncs it, and then renames it to the output path. So the output path is either the complete extract or what it was before the command. The temporary file gets the permissions that File::create gives, which are 0o666 minus the umask. The default of tempfile is 0o600. tempfile is a new dependency of valinor-cli. - The extract subcommand takes a source path directly, not a Valhalla JSON config. So the config path moves from a global argument into the get-edge arguments (valinor-cli get-edge <CONFIG> <GRAPH_ID>). The VALHALLA_CONFIG environment variable still works. - get-edge parses its graph ID with the FromStr implementation of GraphId, in place of its own parser. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The integration tests run with one command:
uv run --project scripts/integration valinor-integration
scripts/integration is a uv project (valinor-integration). It uses
docker-py to start valhalla_service, so it needs only uv and Docker
(and cargo, if VALINOR_CLI is not set).
- `run` (the default) writes a sub-extract of the Andorra fixture with
valinor-cli extract. It serves the full extract and the sub-extract
at the same time on free ports, sends each request to both, and
compares the responses. It makes sure that the sub-extract is a
proper subset of the full extract, so the comparison can never be
empty. It also makes sure that both logs have no [ERROR] lines.
- The checks compare the route shape, the maneuvers, and the summary
length and time of each /route response, and the edges of each
/locate response.
- `serve` serves both extracts on fixed ports (8002 and 8003) until
Ctrl-C or SIGTERM, for requests by hand.
- The requests are a Bruno collection in the OpenCollection YAML format
(scripts/integration/bruno). The harness reads the same files, so a
request that you try in Bruno is the request that CI sends. The
harness refuses Bruno features that it does not run, such as scripts
and assertions.
- The tiles of the sub-extract are in sub-extract-tiles.txt: every
fixture tile except the north-east corner (2/763927), which no
request uses.
- The image's own valhalla_build_config makes the Valhalla config. The
harness sets mjolnir.shortcut_caching to false, because Valhalla
asserts at start that all level 0 and level 1 tiles exist when it is
true.
- The Valhalla version comes from VALHALLA_VERSION, a readable tag.
CI pins it to 3.8.2 in integration.yml. If it is not set, the harness
uses `latest` and logs a warning.
- The harness copies the extract and the config into the container in
place of a bind mount, so a work directory outside the home directory
(such as /tmp) works with colima and Docker Desktop.
- If DOCKER_HOST is not set and the default socket fails, the harness
uses the endpoint of the current Docker CLI context (for colima).
- The harness has 37 unit tests (pytest) and a ruff configuration. CI
runs both before the integration test.
.github/workflows/integration.yml runs the harness on pull requests
that change the extract code, the tile providers, valinor-cli, or the
harness.
Verified locally: PASS against 3.8.2 and against latest (3.9.1), with
VALINOR_CLI set and with cargo. With 2/763926 removed from the tile
list, the harness fails as expected (4 routes return HTTP 400, and
/locate and the fifth route differ). In serve mode, both ports answer,
SIGTERM removes both containers, and a second start removes the
containers that a killed run left.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Archdoog
force-pushed
the
feat/extract-integration-tests
branch
from
November 3, 2026 00:35
30ee1af to
2d468ac
Compare
Main updated the workspace dependencies. Keep the versions from main, and keep the tempfile dev-dependency that this branch adds. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
On Linux, fs::copy uses the copy_file_range system call, and Miri does not support it. Two writer tests copied a tile with fs::copy, so they failed in the Miri jobs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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
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.
Summary
This PR adds integration tests that make sure a sub-extract from
valinor-cli extract(#13) works in stock C++ Valhalla the same way as the full extract.uv run --project scripts/integration valinor-integrationruns the full test.scripts/integrationis a uv project that uses docker-py, so it needs only uv and Docker (and cargo, ifVALINOR_CLIis not set).run(the default). The harness writes a sub-extract of the Andorra fixture withvalinor-cli extract. Then it serves the full fixture and the sub-extract with a stock Valhalla Docker image at the same time, sends each request to both, and compares the responses: the route shape, the maneuvers, the summary length and time, and the/locateedges. It also requires clean logs, and it makes sure that the sub-extract is a proper subset of the full extract, so the comparison can never be empty.serve.valinor-integration servekeeps both extracts up on ports 8002 and 8003 until Ctrl-C, for requests by hand.scripts/integration/bruno). The harness reads the same files, so a request that you try in Bruno is the request that CI sends. CI does not need Node or the Bruno CLI. The harness refuses Bruno scripts and assertions, because it does not run them..github/workflows/integration.ymlruns the harness's unit tests and ruff, and then the harness, on pull requests that change the extract code, the tile providers,valinor-cli, or the harness.Points for review:
VALHALLA_VERSION: "3.8.2"inintegration.yml. Without the variable, the harness useslatestand logs a warning.mjolnir.shortcut_cachingtofalse. When it istrue, Valhalla asserts at start that every level 0 and level 1 tile exists, and a partial extract then fails to start./tmp) works with colima.scripts/integration/uv.lock. The harness does not add a Rust dependency.Manual testing
Happy path
uv run --project scripts/integration valinor-integrationfrom the repository root.Extracted 6 tiles (2834944 bytes), sixok: ... is identical for both extractslines, andintegration: PASS.Key behaviors
uv run --project scripts/integration valinor-integration serve. Openscripts/integration/brunoin Bruno, select thesubenvironment, and send a request. Push Ctrl-C, and expectdocker ps -ato show novalinor-integration-*containers.target/integration. Expectresponses/full/,responses/sub/,valhalla.json, and the two server logs.Edge cases
--tilesset to a copy ofsub-extract-tiles.txtwithout2/763926. Four routes go through that tile, so expect a failure. I did this test: four routes returned HTTP 400,/locateand the fifth route differed, and the harness exited with status 1.VALHALLA_VERSIONunset. Expect a warning and a run againstlatest. I did this test against 3.9.1, and it passed.serverun with SIGKILL, and then start it again. Expect the second start to remove the containers that the first run left.Verification
The repo has no
just pre-prrecipe, so I ran the checks fromAGENTS.md. The Rust checks for the commits of #13 are in #13.Check output
This PR does not close an issue.
🤖 Generated with Claude Code