test(perry): serialize archive cache environment reads

[proggeramlug]

Fixes #9083.

The archive-cache safety test recomputes keys using discovered tool paths, so sibling tests that temporarily replace the process-global PATH can turn its expected cache hit into a miss. Acquire the shared test environment lock across the miss/store/hit sequence so all three lookups see one stable tool environment.

Validation:

• cargo fmt --all -- --check
• focused archive-cache test
• RUST_MIN_STACK=16777216 cargo test -p perry --bins (1056 passed)

No version bump.

Summary by CodeRabbit

• Bug Fixes

  • Stabilized archive-cache fallback and cache-hit tests when running in parallel.
  • Prevented intermittent test failures caused by concurrent environment setting changes.

• Documentation

  • Added a changelog entry describing the archive-cache test reliability improvement.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 00996afa-ee6a-4818-a4c2-178c55698fe9

📥 Commits

Reviewing files that changed from the base of the PR and between f3f4052 and 030b862.

📒 Files selected for processing (2)

• changelog.d/9092-archive-cache-test-isolation.md
• crates/perry/src/commands/compile/link/archive_cache.rs

Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review.

---

📝 Walkthrough

Walkthrough

The archive-cache fallback and cache-hit test now holds the process-global environment lock during its full execution. A changelog entry documents the parallel test stabilization.

Changes

Archive cache test synchronization

Layer / File(s)	Summary
Synchronize archive cache test environment crates/perry/src/commands/compile/link/archive_cache.rs, changelog.d/9092-archive-cache-test-isolation.md	The test imports and acquires env_lock before its cache miss, store, and hit phases. The changelog documents the parallel-suite fix.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Merge Risk: ⚪ Minimal · up to 030b8

The change serializes archive-cache test environment reads to prevent parallel PATH changes from causing false cache misses; no actionable merge-blocking risk remains after normal checks.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 1 files. (1 skipped: 1…	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly identifies the archive-cache test change and the environment-read serialization used to address the flakiness.
Description check	✅ Passed	The description explains the failure cause, the locking fix, the linked issue, validation commands, test results, and version status. It omits the template headings and checklist, but the required cha…
Linked Issues check	✅ Passed	The PR addresses #9083 by acquiring the shared test environment lock across the archive-cache test's miss, store, and hit sequence. This prevents concurrent PATH changes from invalidating tool-path di…
Out of Scope Changes check	✅ Passed	The changes are limited to synchronizing the flaky archive-cache test and documenting the fix in the changelog. These changes support the linked issue and do not introduce unrelated scope.

Full details: Description check

Explanation

The description explains the failure cause, the locking fix, the linked issue, validation commands, test results, and version status. It omits the template headings and checklist, but the required change and validation details are sufficiently covered.

Full details: Linked Issues check

Explanation

The PR addresses #9083 by acquiring the shared test environment lock across the archive-cache test's miss, store, and hit sequence. This prevents concurrent PATH changes from invalidating tool-path discovery and cache-key consistency.

Full details: Docstring Coverage

Explanation

Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 1 files. (1 skipped: 1 unsupported.)

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[proggeramlug]

Merged. Fixes #9083, which I filed this afternoon after nearly misattributing it to #9070.

The diagnosis is right and matches the second of the two mechanisms I guessed: the cache key is recomputed from discovered tool paths, so every lookup reads process-global PATH, and six other modules in this crate take env_lock() precisely because they swap it. Holding the lock across the whole miss/store/hit sequence — rather than around each lookup — is the correct granularity, since the invariant under test is that all three see one environment.

Getting evidence for this took two attempts, and the first one was misleading. Ten runs of the full --bins suite on each arm gave 0/10 and 0/10 — no failures anywhere, which proves nothing about the fix because the baseline never failed either. When I originally hit this I had concurrent builds running, so I re-ran with six yes processes saturating the box (10 cores, load ~20–29):

	failures
main under load	10/12
this PR under load	0/12

So the flake is load-dependent, which is also why it presented as roughly coin-flip earlier rather than deterministic: it needs the scheduler to interleave this test with a sibling that has PATH swapped out. Anyone re-checking this should apply load, or they will measure 0/10 on a broken tree and conclude it's fine.

One note for whoever touches env_lock() next: it .expect()s on a poisoned lock, so a sibling panicking while holding the environment will now surface here as a poison panic rather than a cache miss. That is an improvement — the message names the real cause — but it does mean this test can fail for a reason that isn't its own.

Validation: 12 loaded runs clean as above, fmt clean, run_lint_gates.sh all 60 gates passed; 2 CI-only skipped.