Three guest-rebuild tests fail under an inherited CARGO_TARGET_DIR — they clear CARGO_INCREMENTAL for exactly this reason and not this one #234
Labels
No labels
code-review
correctness
dos
performance
security
severity/high
severity/low
severity/medium
tech-debt
Kind/Breaking
Kind/Bug
Kind/Documentation
Kind/Enhancement
Kind/Feature
Kind/Security
Kind/Testing
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Reviewed
Confirmed
Reviewed
Duplicate
Reviewed
Invalid
Reviewed
Won't Fix
Status
Abandoned
Status
Blocked
Status
Need More Info
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
h-dv/code-index#234
Loading…
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Found while running the #220 lane's gates. Not a product defect — a test-harness environment dependency that makes three suites fail for a reason unrelated to what they grade, in the exact setup this project tells concurrent lanes to use.
The failure
guest_globals,ruby_guest_wireandtimeline_guest_rebuildshell out tocargo buildand then read the artifact back from:An inherited
CARGO_TARGET_DIRredirects the child build to that directory instead. The test then reads the hard-coded path, finds nothing, and fails withNotFoundin about 0.04s.All three pass once
CARGO_TARGET_DIRis unset.Why this is worth fixing rather than remembering
It is the documented workaround for lane contention. Concurrent lanes share
target/, so settingCARGO_TARGET_DIRis the natural way to isolate a build — and doing so silently breaks three suites in a way that looks like a code defect. Someone will spend an hour on it.The tests already know about this class of hazard and guard one instance of it. They clear
CARGO_INCREMENTALdeliberately, for precisely the reason that an inherited cargo env var changes what the child build produces.CARGO_TARGET_DIRis the same hazard, one variable over, unguarded.The failure mode is fast and wrong-looking. 0.04s and
NotFoundreads as "the artifact was never built" rather than "the artifact is somewhere else", which points the reader at the guest build script instead of at their own environment.The fix
Either:
CARGO_TARGET_DIRfor the child build alongsideCARGO_INCREMENTAL, so the hard-coded read path is correct by construction; or--message-format=jsonand readfilenamesoff thecompiler-artifactrecord) instead of assuming<guest dir>/target/..., which removes the assumption rather than defending it.The second is better — it makes the path a measurement rather than a constant that happens to be right — but the first is honest and small.
What a fix must prove
CARGO_TARGET_DIRset to a scratch directory. This must go RED against today's code; if it passes, the harness is not actually exporting the variable to the child.Note on scope
timeline_guest_rebuildis the leg that closed the in-image half ofde.h-dv.timeline's reproducibility claim, andruby_guest_wireis its long-standing model. Both are load-bearing, which is the argument for making their artifact path measured rather than assumed.Fixed in
f9ddfafby the package author — and the fix found the half of this issue I got wrong.I reported the loud failure. The dangerous one is silent.
I described a single mode: the child build is redirected, the hard-coded read finds nothing,
NotFoundin 0.04s. That is real, and it is only what happens on a clean tree.The measurement in the fix:
So under an inherited
CARGO_TARGET_DIRthe mutation goes GREEN on stale bytes. A rebuild test that compares against a leftover is not comparing anything: it is precisely the artifact-bound-to-a-constant defect this suite was written to remove, reintroduced by an environment variable.That is strictly worse than the failure I filed, and my write-up would have led someone to fix only the visible half — teach the paths to follow
CARGO_TARGET_DIR, and the silent mode survives untouched.Why
env_removerather than following the variableI suggested reading cargo's
--message-format=jsonoutput so the path becomes a measurement instead of a constant. The author took the other option, and gave the reason that makes it the right one:Following the variable makes the path honest while leaving the comparison free to run against whatever that path happens to contain. Clearing it forces the child to build where the test reads, so the artifact under comparison is necessarily the one just produced. The two variables are now cleared together with one stated reason — reproduce
build.sh's environment, not the ambient one — where beforeCARGO_INCREMENTALwas cleared andCARGO_TARGET_DIRwas not.The mutation was run in both worlds, which is what caught it
Removing
.env_remove("CARGO_TARGET_DIR")and running under an inherited one: RED on a clean tree, GREEN on a tree holding a previous build. Running that mutation in only the first state would have confirmed the fix and missed the reason for it.Closing. My original "what a fix must prove" list is superseded by that pair — the anti-vacuity arm I asked for was "break the guest source and confirm the comparison still goes RED", and the sharper version is "run the mutation on a tree that already holds an artifact."