Retry testkit fixture execs on the transient ETXTBSY fork race - #730
Merged
Conversation
…-unwrap proof + Linux hostile-tmpdir reproduction pass; test-only robustness fix) TestCanonicalBuildEnvironmentDirectEntrypoints/verify-github-hosted-controls.sh was flaking under the tmpdir hostile-env lane with "text file busy" from ExecFixtureDir's own probe exec. This is the well-known Go fork/exec ETXTBSY race (golang/go#22315): under t.Parallel, a concurrent goroutine's forkExec transiently dups another goroutine's writable fd on a freshly-written script, and execing that script in the brief window returns ETXTBSY. No amount of write-then-exec ordering can eliminate it; only a bounded retry on ETXTBSY at exec time closes the window. Add execRetryETXTBSY, a small helper that reruns a freshly-built *exec.Cmd up to 20 times with a backoff capped at 100ms, retrying only when the error unwraps to syscall.ETXTBSY (verified directly: the fork/exec failure surfaces as *os.PathError, not *exec.Error, so errors.Is needs no extra unwrapping). Any other error, including the EACCES/ENOEXEC a genuinely noexec filesystem returns, still fails immediately. Route ExecFixtureDir's probe exec through the helper so a transient ETXTBSY no longer Fatals a real candidate. Route the release-output verifier driver execs (runVerifyDriver and the inventory-guard driver) and the hosted-controls runner/verifier execs through the same helper, since all of them run a fixture script the test just wrote to disk. Verified: gofmt/build/vet clean; go test ./internal/testkit/... green; -count=30/50 stress of the exec-heavy tests green on darwin and, more meaningfully, on real Linux with the exact hostile TMPDIR shape (space/$/backtick/--prefix/80-char padding) that forces ExecFixtureDir's fallback path; a standalone repro confirms errors.Is(err, syscall.ETXTBSY) matches the genuine fork/exec failure and that the retry loop recovers once the writer closes its fd.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Owner
Author
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c80d5d299
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…+ first-attempt case re-verified; addresses Codex P2 on PR 730) TestExecRetryETXTBSYRetriesThroughTheTransientRace created busy.sh under t.TempDir(). On Linux with TMPDIR on a noexec mount - a layout this package's own hostile-env axis exercises elsewhere - exec on that path returns EACCES once the writer closes it, not ETXTBSY, so the test failed for the wrong reason instead of proving ETXTBSY recovery. Create the fixture under ExecFixtureDir(t) instead: the package's own probed exec-capable directory, which is what every other fixture-exec site in this package already uses. TestExecRetryETXTBSYReturnsImmediatelyOnOtherErrors is unaffected (its t.TempDir() path never has to be exec-capable; the exec fails on ENOENT before any executability check). Verified: gofmt -l ./internal empty; go vet ./... clean; go test ./internal/testkit/ -run TestExecRetryETXTBSY -count=20 green; and, in the Linux container, built the test binary once under a normal TMPDIR/GOCACHE and then ran it with TMPDIR pointed at a genuine tmpfs mounted noexec - 20/20 green, confirming the fix actually reproduces and recovers from ETXTBSY rather than accidentally passing.
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3e55edb4fb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…e (mutation-proof + real hostile-tmpdir stress on Linux; addresses two Codex P2s on PR 730) Two follow-up findings on the ETXTBSY-retry test coverage, both about the retry actually being reliable rather than just present: 1. TestExecRetryETXTBSYRetriesThroughTheTransientRace used a background goroutine with a 30ms sleep to close the held-open fixture file. Under a loaded scheduler the closer could run before the retry loop's first attempt ever executed, so the command could succeed on attempt 1 and the test would pass without ever exercising the retry path - a future regression that deleted the retry loop entirely could still pass this test. Replaced the timer with a deterministic gate: newCmd's own call count decides when to close the file (ETXTBSY on Linux is an unconditional kernel check, not a race, so holding the fd open through exactly one attempt and closing before the next is reproducible every time, no sleep needed), and the test now asserts attempts > 1. Confirmed this actually catches a regression: patched a scratch copy so the retry loop gives up after the first attempt, and the test failed 5/5 runs on Linux with the real ETXTBSY error; reverting the patch passes 20/20. 2. When a shell script Go execs (e.g. run-hosted-controls-audit.sh) itself execs a second test-written fixture (the verifier), the same golang/go#22315 race can hit that inner execve instead of the outer one. Go never sees ETXTBSY in that case - the outer process starts fine, and bash reports the inner failure as exit 126 with "text file busy" on stderr. execRetryETXTBSY didn't retry that shape, leaving the hosted_controls runner tests exposed under parallel load. Added execRetryETXTBSYOrNestedBusy, sharing the existing backoff loop (factored out as execRetryOn) but also retrying when the error is an *exec.ExitError with code 126 AND the combined output contains "text file busy" - scoped to that exact message so an unrelated exit 126 (missing interpreter, a real permission error) still fails immediately rather than being masked behind 20 retries. Routed the three run-hosted-controls-audit.sh exec sites in hosted_controls_token_isolation_test.go through it (the ones whose script nested-execs the verifier fixture); left the two sites that exec the verifier directly on the plain ETXTBSY-only helper, since neither of those has a nested test-written-fixture exec. Test-only change, as before - no product script touched. Added a deterministic unit test for the new predicate (a fixture script that reports the busy-126 shape once then succeeds, and a second fixture with an unrelated exit 126 that must not be retried). Verified: gofmt -l ./internal empty; go vet ./... clean; go test ./internal/testkit/ -run 'TestExecRetryETXTBSY|HostedControls' -count=30 green; full go test ./internal/testkit/... -count=1 green. On Linux (container): the three new/changed unit tests pass 20/20 individually; the regression-catching mutation check above; and -run 'TestExecRetryETXTBSY|HostedControls' -count=30 green under the exact hostile tmpdir TMPDIR shape (space/$/backtick/--prefix/80-char padding) that forces ExecFixtureDir's fallback path, confirming the 126 path is exercised and retried correctly under that lane's real conditions.
Owner
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
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.
Problem
TestCanonicalBuildEnvironmentDirectEntrypoints/verify-github-hosted-controls.shwas flaking under the
tmpdirhostile-env CI lane:PRs #727/#729 already removed every write-then-exec ordering bug in this
package (no fixture is created with the exec bit before its writer closes;
overwrites go through a fresh inode + rename).
root/uidmap/workspacehostile axes pass.
tmpdirstill failed because it is uniquely exposed:ExecFixtureDirrejects its earlier candidate bases under the hostileTMPDIR(the allowlist regex rejects the hostile shape;/dev/shm/XDG_RUNTIME_DIRare absent), falls back to the contended repo checkout root,and then
Fatals on a single ETXTBSY from its own probe exec.Root cause
This is the well-known Go fork/exec ETXTBSY race
(golang/go#22315). Under
t.Parallel, when goroutine A has a writable fd open on a freshly-writtenscript during its brief write window, and goroutine B calls
forkExecforany program, B's child transiently holds a dup of A's writable fd. If A
execs its script in that window, the kernel returns ETXTBSY. File-creation
ordering cannot eliminate this — the fix is a bounded retry on ETXTBSY at exec
time, since the window closes in milliseconds.
Fix
execRetryETXTBSY(internal/testkit/exec_fixture_dir.go): reruns afreshly-built
*exec.Cmdup to 20 times with a backoff capped at 100ms,retrying only when the error unwraps to
syscall.ETXTBSY. Any othererror — including the EACCES/ENOEXEC a genuinely noexec filesystem returns —
fails immediately, so a real "not executable" candidate is still rejected
without delay.
ExecFixtureDir's per-candidate probe now routes through the helper insteadof
Fataling on a transient ETXTBSY.same helper (system binaries and the product-under-test binary are left
alone — ETXTBSY isn't a concern there):
internal/testkit/release_outputs_verify_test.go:runVerifyDriver(theshared driver-exec choke point used by most
TestVerifyReleaseOutputs*cases) and
runReleaseOutputsInventoryGuard's direct driver exec.internal/testkit/hosted_controls_token_isolation_test.go: the fivesites that exec a
run-hosted-controls-audit.sh/verify-github-hosted-controls.shfixture the test itself just wrote(
TestHostedControlsRunnerRelaysTokenOnlyThroughStdin,TestHostedControlsRunnerRequiresNamespacedTokenInGitHubActions,TestHostedControlsRunnerUsesGitHubTokenOutsideActions,TestHostedControlsVerifierRelaysAmbientTokenOnce,TestHostedControlsVerifierScopesDummyTokenToGitHubCommands).internal/testkit/exec_fixture_dir_test.go: a direct unit test forthe new retry/backoff logic — one case proves a non-ETXTBSY error returns on
the first attempt (no wasted retries), the other (Linux-only, skipped
elsewhere) reproduces ETXTBSY by holding a script open for write and
confirms the retry loop recovers once the writer closes it.
Test-only change: no product code or axis composition touched.
Errno-unwrap verification
Confirmed directly, not assumed: a standalone repro that opens a script for
write (without closing) and execs it on Linux produces
err = fork/exec ...: text file busy (*fs.PathError), anderrors.Is(err, syscall.ETXTBSY)istruewith no extra unwrappingthrough
*exec.Error— that type only wrapsLookPathfailures, notfork/exec syscall failures. A second repro proved the retry loop itself
recovers once a concurrent goroutine closes the writer.
Verification
gofmt -l ./internalempty;go build ./...;go vet ./...— all clean.go test ./internal/testkit/... -count=1— green.go test ./internal/testkit/ -run 'TestVerifyReleaseOutputs|TestCanonicalBuildEnvironmentDirectEntrypoints|CiPlan' -count=30 -timeout 25m— green on darwin.tmpdirhostile shape (TMPDIRcontaining a space,$, backtick,---prefixed component, and ~80 chars of padding,XDG_RUNTIME_DIRunset) inside agolang:1.27Linux container and ran the fullinternal/testkitsuite against it — green (the only pre-existing failures weregit ls-fileserrors from an ad-hoc worktree copy artifact, unrelated to this change).-count=50, then-count=30for the same-runfilter as the darwin stress run) under that same hostile-TMPDIR shape on Linux — green.TestExecRetryETXTBSY*unit tests on Linux — both pass, including the ETXTBSY-reproduction case that's skipped on darwin.Deviations from the brief
None. Scope stayed in
internal/testkit; no product code or CI axis composition changed.