Skip to content

Fix selected outputs in the existing target eval cache - #57

Merged
jacobmichels merged 3 commits into
mainfrom
jacob/tecnix-cache-result-contract
Sep 15, 2026
Merged

jacobmichels merged 3 commits into
mainfrom
jacob/tecnix-cache-result-contract

Conversation

@jacobmichels

@jacobmichels jacobmichels commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

A warm Tecnix target-cache hit can return the wrong output. For instance, the cache can return dev even when the target selected out. The existing cache remembers only the .drv path—the build recipe—so importing it loses the original output selection.

Reproduced on seven real World target IDs at commit 5ea0946b556c, using Tecnix 9d05607fe on x86_64-linux. Representative examples:

Target Cold evaluation Warm cache hit
//system/scm/worldtree:rustBin out: executable binaries debug: debug symbols
//docs/world:book out: combined HTML/PDF directory html: HTML output only
//system/aquifer/secrets-bootstrap:incidentLibgit2 lib: runtime library dev: development files

Each cold/warm pair used the same World revision and .drv path, with an isolated cache and logs confirming the warm hit. The docs target’s Nix string dependency context also changed from out to html.

This PR fixes that existing behavior. It stores {drvPath, outputName} in a versioned result payload and reconstructs the selected output with its Nix string dependency information intact. Both fields are forced while source tracking is active, including a lazy output selector that does not change the recipe. Malformed payloads, unavailable recipes, and invalid output names fall back to evaluation.

The dependency-blob version advances from 1 to 2. Existing eval cache databases require no manual migration or deletion: old records become cache misses and are replaced as normal evaluation repopulates the cache. Users will see a cold cache that warms through use. This invalidation is necessary because older records never tracked output-selection inputs and cannot safely serve even dependency-only queries. The SQLite filename and schema, cache eligibility, evaluator-version handling, and packaging remain unchanged.

Checks:

  • Full build and the repository formatter passed.
  • Isolated Linux functional-test package: 215 passed, 0 failed, 10 skipped, with sandboxing enabled.
  • Tecnix builtins and gc functional suites plus nix-expr-tests: 3/3 passed.
  • Regressions cover selected outputs and string contexts, dependency-only warmup, deleted recipes, malformed payloads, lazy selector changes, and rejection of legacy dependency records.

Revision identities and build metadata for future remote-cache reuse are handled separately in #58. This PR depends on #59 for SQLite in the isolated test environment. #58 is the next layer above this selected-output fix. Native stack order: #59#57#58. This PR adds no remote transport or revision-stamping machinery.

@jacobmichels
jacobmichels force-pushed the jacob/tecnix-cache-result-contract branch from 71495c1 to b5a040c Compare September 5, 2026 04:21
@jacobmichels jacobmichels changed the title Preserve cached target outputs and isolate Tecnix revisions Fix cached target outputs and isolate evaluator revisions Sep 5, 2026
@jacobmichels
jacobmichels changed the base branch from main to jacob/tecnix-eval-cache-contract September 5, 2026 04:22
@jacobmichels
jacobmichels force-pushed the jacob/tecnix-cache-result-contract branch from b5a040c to c6edca7 Compare September 5, 2026 04:49
@jacobmichels
jacobmichels changed the base branch from jacob/tecnix-eval-cache-contract to main September 5, 2026 04:49
@jacobmichels jacobmichels changed the title Fix cached target outputs and isolate evaluator revisions Fix selected outputs in the existing target eval cache Sep 5, 2026
@jacobmichels
jacobmichels force-pushed the jacob/tecnix-cache-result-contract branch from c6edca7 to e4ccd76 Compare September 5, 2026 05:44
@jacobmichels
jacobmichels changed the base branch from main to jacob/tecnix-functional-tests-sqlite September 5, 2026 22:02
@jacobmichels
jacobmichels force-pushed the jacob/tecnix-cache-result-contract branch from 6a47eb0 to a845f0d Compare September 5, 2026 22:03
@jacobmichels
jacobmichels force-pushed the jacob/tecnix-cache-result-contract branch 2 times, most recently from 387e1f6 to d0a5026 Compare September 10, 2026 17:34
@jacobmichels
jacobmichels removed this pull request from stack #62 September 10, 2026 17:35
@jacobmichels
jacobmichels added this pull request to stack #65 September 10, 2026 17:35
@jacobmichels
jacobmichels removed this pull request from stack #65 September 10, 2026 20:35
@jacobmichels
jacobmichels force-pushed the jacob/tecnix-cache-result-contract branch from d0a5026 to 24c9577 Compare September 10, 2026 20:36
@jacobmichels
jacobmichels added this pull request to stack #66 September 10, 2026 20:36
@jacobmichels
jacobmichels force-pushed the jacob/tecnix-cache-result-contract branch from 24c9577 to effda45 Compare September 11, 2026 16:01
@jacobmichels
jacobmichels marked this pull request as ready for review September 14, 2026 18:46
Comment thread src/libexpr/primops/tecnix.cc Outdated

@lilyinstarlight lilyinstarlight left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perf testing shows no issues as well. I'm fine with this approach. I'll let you decide on the nit, I don't feel all that strongly on it

@lilyinstarlight lilyinstarlight left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually gonna leave one more comment, this does make one more cache miss/hit difference:

A cold target would have no outputSpecified value and a warm target would now have outputSpecified = true

The fix would probably be to make whether to add outputName to cache based on whether outputSpecified is present/true and fix why derivationToValue is returning a Value that has a default output that isn't the first output in outputs (at least according to PR description, I haven't personally validated this)

@jacobmichels
jacobmichels force-pushed the jacob/tecnix-cache-result-contract branch from effda45 to 28c73c6 Compare September 14, 2026 22:22
@jacobmichels

Copy link
Copy Markdown
Contributor Author

A cold target would have no outputSpecified value and a warm target would now have outputSpecified = true

@lilyinstarlight I don't think this is true in our case. afaict nixpkgs does this, but I don't think our caching logic does.

Base automatically changed from jacob/tecnix-functional-tests-sqlite to main September 15, 2026 02:21
Drop the redundant cachePayload switch; useCache already gates the cache
write, and the selected-output fields were already forced either way.

Check that cold and warm targets leave outputSpecified absent for both
lazy output selection and a direct drv.dev selection. Keep caching the
outputName because selecting an output does not add outputSpecified in
this fork, and importing the recipe chooses the alphabetically first
output rather than the original default.

Assisted-By: devx/be2f8951-dcdd-4edb-800e-b2b04251a210
@jacobmichels
jacobmichels force-pushed the jacob/tecnix-cache-result-contract branch from 28f9271 to f70e31d Compare September 15, 2026 02:21
@jacobmichels
jacobmichels merged commit 757a8c6 into main Sep 15, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants