Skip to content

Fix env race flake in cache_env scrub self-tests - #1418

Merged
Mikola Lysenko (mikolalysenko) merged 1 commit into
mainfrom
ci-janitor/cache-env-scrub-snapshot
Oct 12, 2026
Merged

Mikola Lysenko (mikolalysenko) merged 1 commit into
mainfrom
ci-janitor/cache-env-scrub-snapshot

Conversation

@mikolalysenko

@mikolalysenko Mikola Lysenko (mikolalysenko) commented Oct 12, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

coverage (part of the ci-ok gate) failed on two PR runs in the last 24h with one cargo test target red and nothing else wrong:

  • run 38194961803 (agent/fix-ci-test-helper-copies-5): -p socket-patch-cli --test in_process_edge_cases failed; attempt 2 on the same SHA passed, so it was a flake.
  • run 38184196576 (agent/fix-go-selected-version-gate): -p socket-patch-cli --test vendor failed. That binary also links common/cache_env.rs and runs in-process tests. The failing test name isn't in the 5000-line log tail I could read, so I can't confirm this one is the same flake.

Running the in_process_edge_cases binary locally 60 times with --test-threads=16 failed 8 times. Every failure was one of two self-tests:

common::cache_env::cache_env_selftests::ambient_bun_scrub_removes_only_matching_parent_vars
  panicked at tests/common/cache_env.rs:392: SOCKET_OFFLINE is set in the parent env but was not scrubbed
common::cache_env::cache_env_selftests::ambient_vlt_scrub_removes_only_matching_parent_vars
  panicked at tests/common/cache_env.rs:451: SOCKET_OFFLINE kept

common/cache_env.rs is included through common/mod.rs in about 120 test binaries, so the flake can turn up in any of them that also runs in-process commands. In the merge queue, that evicts the entry.

Root cause

Each self-test reads std::env::vars_os() twice: once inside scrub_ambient_*_env and once to check that every matching parent var was removed. These self-tests are not #[serial], so they run at the same time as the binary's #[serial] in-process tests. Those tests call apply::run etc., and apply_env_toggles (src/args.rs) runs std::env::set_var(\"SOCKET_OFFLINE\", \"1\") for --offline. If the var appears between the two reads, the second read sees a matching var that the scrub never saw.

Fix

Read the parent env once (parent_var_names). Pass that one snapshot to both the scrub (scrub_parent_vars) and the self-test's assertions. The public scrub_ambient_bun_env / scrub_ambient_vlt_env behave exactly as before: they read the env once and remove the matching names. No assertion is weakened. The self-tests still check that the scrub removes exactly the matching names, now against a consistent snapshot.

Proof

  • Before: in_process_edge_cases failed 8 of 60 runs with --test-threads=16.
  • After: 0 of 200 runs failed with the same command.
  • cargo test -p socket-patch-cli --test vendor --test cli_parse_get cache_env passes.
  • rustfmt run on the touched file only. Clippy reports nothing new in cache_env.rs.

No tests were moved or removed.

🤖 Generated with Claude Code

https://claude.ai/code/session_018js6KeL55GXJTLwgF3bQWV


Generated by Claude Code

The cache_env self-tests ambient_{bun,vlt}_scrub_removes_only_matching_
parent_vars read the process env twice: once inside the scrub and once
to check that every matching parent var was removed. common/cache_env.rs
is compiled into ~120 test binaries, and in-process tests in the same
binary mirror --offline into the env (apply_env_toggles sets
SOCKET_OFFLINE), so a var appearing between the two reads failed the
self-test with "SOCKET_OFFLINE ... was not scrubbed" / "kept".

Snapshot the parent env once and pass that snapshot to both the scrub
and the assertions. The public scrubs behave exactly as before.

Locally in_process_edge_cases failed 8/60 runs before and 0/200 after.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018js6KeL55GXJTLwgF3bQWV
@mikolalysenko Mikola Lysenko (mikolalysenko) added the ci-janitor Opened by the CI janitor routine (flakes, redundant tests, CI perf) label Oct 12, 2026
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

bugbot run


Generated by Claude Code

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 8f29f99. Configure here.

@mikolalysenko Mikola Lysenko (mikolalysenko) added the Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review label Oct 12, 2026
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Ready for review (burn-down agent)

  • Head: 8f29f99
  • CI: 35/35 non-skipped checks green (19 skipped by path filters).
  • Bugbot: reviewed 8f29f99, no findings; no open review threads.
  • Mergeable, no conflicts; no CHANGELOG change.
  • Slack announcement: not sent this run (Slack send tool unavailable in this session); next run retries.

Generated by Claude Code

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Final review brief

What it does. Fixes a test-only flake in tests/common/cache_env.rs (linked into ~120 CLI test binaries). The ambient bun/vlt scrub self-tests read the parent env twice, and in-process tests in the same binary can set SOCKET_OFFLINE between the reads. The scrub and its self-test assertions now share one snapshot of the parent env names.

Risk: low. Test helper only. No product code changes, no assertions weakened, and the public scrub_ambient_bun_env / scrub_ambient_vlt_env still read the env once and remove the same names.

Look here

Verified. Read the full diff (one file). Checked the other self-tests in the file for the same double-read pattern; none of them read a var that in-process tests set. CHANGELOG.md is untouched. CI on 8f29f99: ci-ok and clippy green, 0 failed checks. Bugbot: no issues.

Changes I made. None.

Open questions. None.

Auto-merge is armed, so approving sends it straight to the merge queue.


Generated by Claude Code

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Another occurrence after this PR went up: run 38224049653 on #1421 (agent/fix-composer-xdg-home-order). coverage attempt 1 failed with only -p socket-patch-cli --test in_process_edge_cases red, and attempt 2 passed on the same SHA. That's 2 confirmed hits of this flake in ~6h on unrelated PRs, and coverage gates ci-ok, so the same flake would evict merge-queue entries. This PR is the top merge-reliability fix waiting on review.


Generated by Claude Code

Mikola Lysenko (mikolalysenko) pushed a commit that referenced this pull request Oct 12, 2026
The coverage job failed this PR's merge-queue run (2ea36d6). That job
flakes in in_process_edge_cases (#1416): the cache_env scrub
self-tests read the parent env twice while in-process tests set
SOCKET_OFFLINE. Port #1418's fix (one snapshot of the parent env) so
the queue run is not at the mercy of that race. It no-ops once #1418
lands.

Refs #1416

Assisted-by: Claude Code:claude-opus-5-5
Merged via the queue into main with commit 95e880e Oct 12, 2026
54 checks passed
@mikolalysenko
Mikola Lysenko (mikolalysenko) deleted the ci-janitor/cache-env-scrub-snapshot branch October 12, 2026 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci-janitor Opened by the CI janitor routine (flakes, redundant tests, CI perf) Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants