test(credentials): stub creation and child spawning stop overlapping - #176
Merged
Conversation
The credentials suite failed CI on #174 and had been flaking for weeks. Amy's call was to fix it before the cosmetic pre-release items, on the argument that a merge gate failing a fifth of the time is one we will start ignoring. So the first decision was to measure rather than assert. The prior note called this a rare race; it is not — 6 of 20 parallel runs on the branch, 3 of 15 on unmodified `main`, 0 of 10 serial. The race is `ETXTBSY`. Writing a stub holds a write fd on it, and `fork` hands every open fd to the child; a sibling test forking in that window gives its child a duplicate, held until the child reaches `exec`. An `exec` of that stub meanwhile is "Text file busy". `O_CLOEXEC` does not save it — the fd closes *at* exec, and the whole window is before it. Nothing to do with the credential code; the harness owns this one. Second decision: fix the race, not the symptom. Retrying on `ETXTBSY` would go green while leaving a real fork/exec race in the tree, and a loop around a race teaches the next reader that the race is acceptable. So the fix removes the overlap instead. Third, a read/write lock rather than a mutex, because the two operations are not symmetric. Creation takes the exclusive side; spawning takes the shared side, so resolves still run concurrently and `a_blocking_key_resolve_does_not_stall_a_sibling_task` keeps testing the concurrency property it was written for rather than being quietly serialized into a tautology. Every resolve in the file goes through the helper, including the ones whose command is not a stub — a spawn that fails at `exec` has already forked, so it is in the race too. Poisoning is stepped over on purpose: otherwise one test's panicking assertion fails every later test with a lock error instead of its own message. Measured after: 0 of 60 parallel runs fail. The control that makes that number mean something — remove only the two lock lines and keep everything else — puts it back to 11 of 20. No changelog entry: the harness is not a user-facing surface, and the git log is the record for an internal fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
The credentials suite failed CI on #174 and had been flaking for weeks. Amy's call was to
fix it ahead of the cosmetic pre-release items — "a merge gate that fails a fifth of the
time is one we will start ignoring" — so this goes first in the pre-release queue.
Measure before asserting
Our own note called this a rare race. It is not:
main@f741a84, unmodifiedA fifth to a third of runs. That number is what moved this from "known flake, re-run it"
to "fix it now."
What it actually is
ETXTBSY. Writing a stub holds a write fd on it, andforkhands every open fd to thechild; a sibling test forking in that window gives its child a duplicate, which it holds
until it reaches
exec. Anexecof that stub in the meantime is "Text file busy".O_CLOEXECdoes not save it — the fd closes at exec, and the entire window is beforeit. Nothing to do with the credential code: the harness owns this one.
Three decisions
Fix the race, not the symptom. Retrying on
ETXTBSYwould go green while leaving areal fork/exec race in the tree, and a retry loop around a race teaches the next reader
that the race is acceptable. This removes the overlap instead.
A read/write lock, not a mutex — the two operations are not symmetric. Creation takes
the exclusive side (brief, and it must exclude every spawn); spawning takes the shared
side, so resolves still run concurrently with each other. That matters for one test
specifically:
a_blocking_key_resolve_does_not_stall_a_sibling_taskexists to pin aconcurrency property, and a coarse mutex would have quietly serialized it into a
tautology that passes without testing anything.
Every resolve goes through the helper, including the ones whose command is not a stub
— a spawn that fails at
exechas already forked, so it is in the race too.Poisoning is stepped over deliberately: otherwise one test's panicking assertion fails
every later test with a lock error instead of its own message.
The number, and the control that makes it mean something
0 of 60 parallel runs fail with the fix.
A zero is only worth as much as its control, so: remove only the two lock-acquisition
lines, keep the helper and every other change, and it goes back to 11 of 20 failing.
The lock is doing the work, and the instrument still detects the race it claims to have
closed.
Full suite 1328 passed / 0 failed.
cargo clippy --all-targetsclean.No changelog entry
The test harness is not a user-facing surface, and the house rule is that the git log is
the record for an internal fix.
🤖 Generated with Claude Code