fix(queue-runner): don't parse buildproducts.path as a StorePath#2
Conversation
`buildproducts.path` is a full filesystem path, not a bare store path: Hydra's `$out/nix-support/hydra-build-products` lets a product point at a sub-path of an output (e.g. `doc manual $doc/share/doc/nix/manual index.html`), so the column legitimately contains a trailing `/...` after the `<hash>-<name>`. `OwnedBuildProduct::parse_paths` fed the whole string to `StoreDir::parse`, which died with `invalid store path / symbol at 50` on any such value. That wedged every cached-build fast path touching a product with a subpath — notably the `nix-manual-2.31.4/share/doc/nix/manual` products from `roots.nix` builds, leaving ~200 builds per eval stuck as 'unfinished' even though every output was already in the store. Even had the parse succeeded, `BuildProduct::from_db` was hard-coding `relative_path: \"\".into()`, so the sibling constructors (`from_grpc`, `from_shared`) and `from_db` disagreed on what the field meant. Fix by: - dropping the `StorePath` generic + `parse_paths` from `OwnedBuildProduct` (path stays as `Option<String>` — it's opaque to the db crate), and - routing `BuildProduct::from_db` through the same `RelativeStorePath::from_path` splitter `from_grpc`/`from_shared` already use. The db-side write path calls `RelativeStorePath::print`, so `from_path` is now its exact inverse. Adds unit tests covering the subpath product, a bare store path, and the print/from_path round-trip.
There was a problem hiding this comment.
Pull request overview
Fixes cached-build handling in hydra-queue-runner by treating buildproducts.path as a full filesystem path (potentially including a subpath) instead of parsing it as a bare store path, and aligns DB ↔ runtime conversion behavior.
Changes:
- Stop parsing
buildproducts.pathinto aStorePathin the DB crate; keep it asOption<String>. - Update queue-runner DB→runtime conversion to split base store path vs trailing subpath via
RelativeStorePath::from_path. - Add unit tests covering subpath products, bare store paths, and
print/from_pathround-tripping.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| subprojects/hydra-queue-runner/src/state/mod.rs | Adapts cached build output loading to new DB API and fallible product conversion. |
| subprojects/hydra-queue-runner/src/state/build.rs | Makes BuildProduct::from_db fallible and adds regression/round-trip unit tests for RelativeStorePath. |
| subprojects/crates/db/src/models.rs | Changes OwnedBuildProduct.path to opaque Option<String> and removes store-path parsing helper. |
| subprojects/crates/db/src/connection.rs | Simplifies get_build_products_for_build_id to return raw DB rows without store-path parsing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| res.products = db | ||
| .get_build_products_for_build_id(build_id, self.store.store_dir()) | ||
| .get_build_products_for_build_id(build_id) | ||
| .await? | ||
| .into_iter() | ||
| .map(build::BuildProduct::from_db) | ||
| .collect(); | ||
| .map(|p| build::BuildProduct::from_db(self.store.store_dir(), p)) | ||
| .collect::<anyhow::Result<_>>()?; |
There was a problem hiding this comment.
BuildProduct::from_db now returns a Result, but this code uses ? inside the cached-build fast path. If a single buildproduct row has an unexpected/non-store path, get_build_output_cached will now return an error instead of skipping the cached DB record and falling back to BuildOutput::new (similar to the existing try_into() handling above). Consider treating product-parse failures as a cache-miss here (e.g., log + continue), so bad DB data doesn't abort cached-build handling entirely.
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
|
|
||
| fn test_store_dir() -> nix_utils::StoreDir { | ||
| nix_utils::StoreDir::new("/nix/store").unwrap() | ||
| } |
There was a problem hiding this comment.
This test module uses unwrap()/expect() (e.g. StoreDir::new(...).unwrap() and .expect(...)), but the crate has #![deny(clippy::unwrap_used, clippy::expect_used)] at the crate level. Add an appropriate #![allow(clippy::unwrap_used, clippy::expect_used)] inside this #[cfg(test)] mod tests, or rewrite these calls to avoid unwrap/expect, otherwise CI clippy runs will fail.
|
This supposedly is a regression from 883b9c9 (NixOS#1670) |
angerman
merged commit fd3543d
into
angerman:fix/builder-grpc-keepalive
buildproducts.pathis a full filesystem path, not a bare store path: Hydra's$out/nix-support/hydra-build-productslets a product point at a sub-path of an output (e.g.doc manual $doc/share/doc/nix/manual index.html), so the column legitimately contains a trailing/...after the<hash>-<name>.OwnedBuildProduct::parse_pathsfed the whole string toStoreDir::parse, which died withinvalid store path / symbol at 50on any such value. That wedged every cached-build fast path touching a product with a subpath — notably thenix-manual-2.31.4/share/doc/nix/manualproducts fromroots.nixbuilds, leaving ~200 builds per eval stuck as 'unfinished' even though every output was already in the store.Even had the parse succeeded,
BuildProduct::from_dbwas hard-codingrelative_path: \"\".into(), so the sibling constructors (from_grpc,from_shared) andfrom_dbdisagreed on what the field meant.Fix by:
StorePathgeneric +parse_pathsfromOwnedBuildProduct(path stays asOption<String>— it's opaque to the db crate), andBuildProduct::from_dbthrough the sameRelativeStorePath::from_pathsplitterfrom_grpc/from_sharedalready use. The db-side write path callsRelativeStorePath::print, sofrom_pathis now its exact inverse.Adds unit tests covering the subpath product, a bare store path, and the print/from_path round-trip.