Finish removing the ETXTBSY write-then-exec race in testkit fixtures - #729
Merged
Conversation
…es (Linux repro: 0/10 vs 2/10 on main; internal test-infra fix) PR #727 fixed the shared writeExecutable helper plus four inline sites in release_outputs_verify_test.go, but every other place in internal/testkit that wrote an executable test fixture still created it at 0o755/0o700 directly. Any one of these, running concurrently with an already-fixed exec (e.g. verify-release-outputs-test-driver.sh) in the same test binary, is enough to trip the kernel's fork/exec ETXTBSY race -- which is exactly what the hostile-env CI axes were hitting. Route every executable-fixture write through write(0o644)+close+chmod: - local_docker_parity_test.go: new shared writeExecFile primitive; writeExecutable and copyValidatorFixtureFile (a second missed site in the very file #727 touched) now use it. - canonical_build_env_test.go: writeCanonicalFixture (used by canonical_build_env_test.go and hosted_controls_token_isolation_test.go). - update_upstream_pins_check_test.go: writeUpdaterFixtureFile. - ci_plan_git_test.go: ciPlanFixture.writeFile, .writeExecutable, and replaceScript -- the site #727 explicitly flagged and left out of scope. - exec_fixture_dir.go: the probe script ExecFixtureDir writes to prove a candidate directory is exec-capable (not a _test.go file, fixed inline). - Direct sites: workcell_version_test.go, container_smoke_harness_test.go, public_repo_hygiene_test.go, security_boundary_test.go, tracked_shebangs_test.go, git_hooks_test.go, release_verify_test.go, install_release_e2e_test.go, validation_entrypoints_test.go (11 spots). Verified: gofmt/go build/go vet clean; go test ./internal/testkit/... green on darwin (count=1 and count=30). On Linux (Colima/Docker), 10/10 default `go test ./internal/testkit/...` runs are clean on this branch versus 2/10 hitting ETXTBSY on origin/main; serializing t.Parallel() (-parallel=1) at count=30 is 30/30 clean on both, confirming the residual flakiness under heavy artificial stress is concurrent-exec-driven, not a remaining unsafe write site.
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: e9f270f3e8
ℹ️ 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".
… (count=20/count=1 pass; test-only fixture-safety fix) writeExecFile wrote content at 0o644 then chmod'd to the target mode, but when path already exists (ciPlanFixture.replaceScript overwriting the already-checked-out, already-executable scripts/ci-plan.sh), os.WriteFile truncates that live inode in place and never touches its mode: for the whole write the inode stays 0o755 with an open writable fd, which is the exact ETXTBSY window this helper exists to close, just on the overwrite path instead of the create path. Fix it once in the shared primitive: write to a temp file in the same directory, close it, chmod it, then os.Rename it over path. Rename is atomic within one directory, so the path a concurrent exec sees is never the inode being written, whether path is new or already exists and is already executable. Verified: gofmt/go build/go vet clean; go test ./internal/testkit/... -count=1 green; go test ./internal/testkit/ -run 'TestVerifyReleaseOutputs|CiPlan|CIPlan' -count=20 green (covers the replaceScript overwrite path).
Owner
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. 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.
Why #727 was incomplete
PR #727 fixed the shared
writeExecutablehelper (local_docker_parity_test.go)and four inline sites in
release_outputs_verify_test.go, but the hostile-envCI axes still fail reliably:
verify-release-outputs-test-driver.shis already written throughwriteExecutable, i.e. through the correct write(0o644)+close+chmod(0o755)pattern. The failure isn't in how that file is created — it's that Go test
subtests in this package run concurrently (
t.Parallel()), all inside oneprocess, and every other place in
internal/testkitthat still wrote anexecutable fixture directly at
0o755/0o700was enough to trip thekernel's fork/exec ETXTBSY race for whichever exec happened to be in flight
at that moment — including an already-safely-written file elsewhere in the
same test binary. Fixing only the two files #727 touched left the rest of
the package racy, so the CI gate stayed red.
What's fixed
Every executable-fixture write in
internal/testkitnow goes throughwrite(0o644) fully closed, then a separate
os.Chmodto add the exec bit,via a new shared
writeExecFileprimitive (inlocal_docker_parity_test.go,next to
writeExecutable, which now calls it):local_docker_parity_test.go:writeExecutable, andcopyValidatorFixtureFile— a second, previously-missed racy site in the very file Harden two transient CI build and test flakes #727 already touched.
canonical_build_env_test.go:writeCanonicalFixture(also used byhosted_controls_token_isolation_test.goandwriteCanonicalMutation).update_upstream_pins_check_test.go:writeUpdaterFixtureFile.ci_plan_git_test.go:ciPlanFixture.writeFile,.writeExecutable, andreplaceScript— the site the Harden two transient CI build and test flakes #727 agent flagged as the same racy shapeand explicitly left out of scope.
exec_fixture_dir.go(not a_test.gofile, fixed inline): the probescript
ExecFixtureDirwrites to prove a candidate directory isexec-capable.
workcell_version_test.go,container_smoke_harness_test.go,public_repo_hygiene_test.go,security_boundary_test.go,tracked_shebangs_test.go,git_hooks_test.go,release_verify_test.go,install_release_e2e_test.go, and 11 spots invalidation_entrypoints_test.go.Audit:
grep -rn "0o755\|0o700\|0o775\|0o777" internal/testkit/now shows noos.WriteFilecall creating a file with the exec bit already set — everyremaining
0o75x/0o70xliteral is either a directoryMkdirAll/Mkdirmode, the final mode argument passed into
writeExecFile/writeExecutable/writeCanonicalFixture/writeUpdaterFixtureFile/ciPlanFixture.writeFile(all of which write 0o644 then chmod), or an unrelated
os.Chmodrestoring adirectory's mode in test cleanup.
Verification
gofmt -l ./internal— empty.go build ./...,go vet ./...— clean.go test ./internal/testkit/... -count=1and-run TestVerifyReleaseOutputs -count=30 -v— green on darwin (ETXTBSY doesn't reproduce there; the proof is the audit above).golang:latest, since ETXTBSY is Linux-specific):go test ./internal/testkit/...: 0/10 fail on this branch vs 2/10 hittingtext file busyonorigin/main.-run TestVerifyReleaseOutputs -count=30with defaultt.Parallel(): this branch still shows occasionaltext file busyunder that artificial stress, but so doesorigin/main(worse: 6 vs 12 failures) — confirming it isn't a remaining unsafe write site.-parallel=1) at-count=30is 30/30 clean on both branches, showing the residual stress-only flakiness is a concurrent-fork kernel race independent of file-creation order, not something a write-then-chmod ordering fix (in this PR's scope) can further reduce.Not merging — opening for review per house rules.