fix(op): sort standalone-test dependencies so the test rootfs is deterministic - #1180
Conversation
…rministic `StandaloneTest::dependencies` collected into a `HashSet<SandboxMapped>` fed by an unsorted `HashMap` iteration. The sandbox hardlinks these first-writer-wins, so when two dependencies install the same path the winner was decided by a per-process `RandomState` seed and varied between runs. Collect into a `Vec` sorted by spec hash, deduplicating by path and keeping the first occurrence, so a given test assembles the same rootfs every time. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 16 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
twitchyliquid64
left a comment
There was a problem hiding this comment.
Theres no need to sort and/or remove dupes if the upstream collection of paths into a rootfs is a BTreeSet - it will be deterministic order, and first write will win. A lot of this diff seems superfluous (tho s/HashSet/BTreeSet seems fine)
|
Done in bbab0ff — took it one level further per your note: |
Sandbox::new hardlinks Config::rootfs entries first-writer-wins by iterating the collection directly, so a HashSet let a per-process RandomState seed decide which entry provided a contended path. Make the collection itself ordered: SandboxMapped gets an Ord mirroring its manual Eq/Hash key (variant tag, path) and Config::rootfs becomes a BTreeSet. This fixes every rootfs producer at once; the op-layer sort/dedup this replaces was only treating one call site's symptom. The standalone-test path keeps the plain HashSet->BTreeSet swap. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
bbab0ff to
afdbc22
Compare
Summary
Same class of issue as #1178, on the standalone-test path.
StandaloneTest::dependenciescollects into aHashSet<SandboxMapped>, fed by an unsortedHashMapiteration. The sandbox hardlinks these first-writer-wins, so when twodependencies install the same path the winner is decided by iteration order, which comes
from a per-process
RandomStateseed. A test therefore ran against a different rootfs fromone invocation to the next.
Change
Collect into a
Vecsorted by spec hash, deduplicating by path and keeping the firstoccurrence. One file.
Why this one is worth doing
Unlike a build, a test that composes its rootfs differently can silently report the wrong
result rather than failing.
Concretely:
packages/gawk'smatchingtest hastest_deps = [base-bootstrap], and thatclosure contains
gawk-bootstrap5.3.2, which installsusr/bin/gawk,usr/bin/awkandusr/lib/gawk/*.so— the same paths gawk 5.4.0 installs.sandbox2also symlinks/bin → usr/bin, so command lookup resolves to whichever won. On an unlucky iterationminimal check gawkasserts against the 5.3.2 binary and reports a pass for 5.4.0; itwould report a pass even if 5.4.0 could not start.
packages/mtoolshas the same shapewith
bash.This PR makes that outcome consistent rather than random. It does not decide which
package ought to win — as with #1178, the overlapping paths are the underlying problem and
belong in the specs. Filing this separately so the variance is gone while that is sorted
out.
Note
The queue builder does not run standalone tests, so no published artifact depends on these
verdicts today.
Note
Sort standalone-test rootfs dependencies deterministically using
BTreeSetHashSet<SandboxMapped>withBTreeSet<SandboxMapped>in bothConfig.rootfsandStandaloneTest.dependenciesto give deterministic iteration order.PartialOrd/Ordimplementations forSandboxMapped, ordering by variant (File < Dir < TempDir < FileCopy) then by associated path.Config.rootfsnow sees a stable, sorted order instead of hash-determined order.Macroscope summarized afdbc22.