The verdict cache remembers which switches the pass ran under - #382
Merged
Merged
Conversation
A marker recorded the container a pass was about and nothing about what that pass carried, so a DEVLAUNCH_NO_ZELLIJ=1 pass -- which probes provisioned, because the probe is about the tools -- wrote a marker the next full launch trusted. zellij was never installed, and no top-up could ever notice: they all read the same marker and skip the same trip. MarkerSwitches is two bools in a private struct rather than serde on the pub Switches, so the on-disk format of a cache file stays out of the frozen surface. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Reviewer's GuideThe verdict cache now records the tools/zellij switches used by each provisioning pass and only trusts markers for launches with the same switch configuration, preventing an opt-out pass from permanently suppressing a later required stage while safely invalidating legacy markers. Sequence diagram for switch-aware verdict cache validationsequenceDiagram
participant Launch
participant Provision
participant VerdictCache
participant Container
Launch->>Provision: provision(workspace, switches)
Provision->>VerdictCache: trusted(workspace, switches)
VerdictCache->>VerdictCache: read_marker()
alt marker switches match launch
VerdictCache->>Container: sole_workspace_result()
Container-->>VerdictCache: matching result mtime
VerdictCache-->>Provision: true
Provision-->>Launch: CachedProvisioned
else switches differ or marker is legacy
VerdictCache-->>Provision: false
Provision->>Container: run provisioning stages
Container-->>Provision: observed result
Provision->>VerdictCache: record(workspace, observed, switches)
end
Entity relationship diagram for switch-aware verdict markerserDiagram
VERDICT_CACHE ||--o| MARKER : stores
MARKER {
Verdict verdict
Stamp result_mtime
MarkerSwitches switches
}
MARKER_SWITCHES {
bool tools
bool zellij
}
MARKER }o--|| MARKER_SWITCHES : records
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
This was referenced Aug 24, 2026
Merged
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.
Found by a retrospective
wf-reviewof #328, which merged two minutes after opening on CI-green auto-merge. Sourcery left high-level feedback; nothing looked at the cache's correctness.The defect
Markerrecorded{verdict, result_mtime}— which container the pass was about, and nothing about what that pass carried.So:
DEVLAUNCH_NO_ZELLIJ=1 dl <ws>runs a setup pass with no zellij stage.dl <ws>with no variable set wants the zellij stage. It finds a trusted marker for a container that never stopped, returnsCachedProvisioned, and skips the trip.zellij is never installed. And it never will be: every subsequent top-up reads that same marker and skips that same trip, so the failure is permanent for the life of the container and completely silent. The container has to be recreated before anything notices — which is the same shape as the herdr feature withdrawn in #376, a feature whose majority case was "installs nothing" with nothing saying so.
The marker answered "was this container provisioned?" when the question the flow asks it is "was it provisioned the way this launch wants?" Those differ exactly when somebody used an opt-out, which is the only reason the opt-outs exist.
The fix
Markergainsswitches, andtrusted()takes the switches this launch wants. A marker written under different switches reads as no verdict.The comparison runs before the anchor check: it is cheaper, and it is the one that is wrong about a container that really is standing — the mtime can match perfectly while the marker describes a pass that skipped the stage being asked for.
MarkerSwitchesis two bools in a private struct rather than serde derives on thepub Switches. Deriving on the public type would put the on-disk format of a cache file into the frozen surface, where a field rename becomes a breaking change to something no consumer ever asked for.Existing markers
A marker written by 0.12.0 or earlier has no
switcheskey, so it fails to deserialize and reads as absent — one redundant round trip on the first launch after upgrading. That is the directionVerdict's own doc argues is the only harmless one, and it is why the enum-with-one-arm trick was chosen there in the first place. Treating a keyless marker as a full install would silently re-open the defect for every container that already exists.Verified
a_marker_written_by_an_opted_out_pass_is_not_trusted_by_a_full_launchfails with the switch comparison removed and passes with it. It asserts both directions: the pass that wrote the marker must still be answered by it, and a launch wanting the zellij stage must not be.a_marker_from_a_build_that_did_not_record_switches_is_not_trustedstrips the key from a real marker on disk and checks the reading.cargo test --workspace: 1,673 passed, 0 failed.cargo clippy --locked --all-targets -- -D warningsclean,cargo fmt --checkclean.MarkerandMarkerSwitchesare private, andtrusted/recordarepub(crate).From the same review, not fixed here
DEVLAUNCH_NO_TOOLS=1on a cached top-up returnsCachedProvisionedinstead ofDisabled, dropping theProvisioningDisabledevent and flippingtools_present()false→true. The cache is consulted above theToolsSwitch::Skiparm. Related, but a different ordering fix.metadata.rs:596,completion_cache.rs:165,verdict_cache.rs:243) that disagree on fsync, interrupt-time cleanup and error policy — the newest is the only one that swallows every error and never removes its staged temp. This is Sourcery's Two ways to stop paying for the setup pass #328 finding, confirmed and drifted in the three days since; it belongs under A standing rule for second copies of one fact #319.trustedanswers "is this the same container", which the flow reads as "are the tools still there" — tools removed from a container that never stopped are never noticed. Undocumented residual.Full report on #328.
🤖 Generated with Claude Code
Summary by Sourcery
Ensure cached provisioning is reused only when the cached pass matches the switches requested by the current launch.
Bug Fixes:
Enhancements:
Documentation:
Tests: