Repository navigation
Conversation
…SY race Follow-up to #575. #575's timeout bump moved the failure window from "download takes too long" to "parallel downloaders race on the same cache file". Live-caught on #568's CI post-#575 (2026-11-08): fork/exec /home/runner/.sc/plugins/.../sc-plugin-gcp-linux-amd64: text file busy `go test ./...` runs packages in parallel (controlled by `-p`). Two parallel test binaries that both load a server.yaml with `plugins.gcp:` both see a cache-miss on the per-version plugin binary path, and both call `downloadFile` writing to the SAME `cached` path. Linux's ETXTBSY fires when a goroutine tries to fork/exec the file while another has it opened for write. ## Fix Download to `<cached>.tmp-<random>` via `os.CreateTemp`, verify SHA + chmod on the temp path, then `os.Rename` to the final `cached` path. - `os.Rename` within one filesystem is atomic. Any observer sees either the old/absent file or the complete new one — never a torn write. - Two parallel downloaders each land on their own temp, each finishes, each renames. Last-renamer wins by content — both downloaders fetched the same release artifact, so content is identical by content hash (the SHA check on the temp path gates this). - `defer os.Remove(tmpPath)` cleans up after a successful rename (no-op) or an error mid-flight (removes partial download). ## Why not a file lock - `flock()` works but adds a dependency and the semantics of "wait for the other downloader" are unnecessary — both downloaders fetch the same bytes. Letting them each finish in their own tmp and rename wins is simpler and equivalent. - A sync.Mutex only helps for a single process. `go test ./pkg/A/... ./pkg/B/...` runs two separate binaries; sync.Mutex wouldn't catch the cross-process race. ## Test plan - [x] `go build ./...` - [x] `go test ./pkg/plugin/... -count=1` — green (2.6s/0.07s). The fetcher's own tests use a mock server + path:-mode binaries, so they don't exercise the race directly; the real proof is #568's Run tests under parallel load, which this PR unblocks. - [x] gofumpt + gci applied ## Why this wasn't caught by #575's tests #575 was a timeout bump — fetcher tests use path:-mode (no actual download) or a mock HTTP server (deterministic timing). The race only fires against the real GitHub releases CDN under concurrent load, which only CI reproduces. Signed-off-by: Ilya Sadykov <smecsia@gmail.com>
smecsia
requested review from
Cre-eD,
Laboratory and
universe-ops
as code owners
November 8, 2026 19:21
Semgrep Scan ResultsRepository:
Scanned at 2026-11-08 20:06 UTC |
Security Scan ResultsRepository:
Scanned at 2026-11-08 20:07 UTC |
baasforger
approved these changes
Nov 8, 2026
universe-ops
approved these changes
Nov 8, 2026
universe-ops
enabled auto-merge (squash)
November 8, 2026 20:06
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
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.
Summary
Follow-up to #575. The timeout bump moved the failure window from
"download takes too long" to "parallel downloaders race on the same
cache file" — live-caught on #568's post-#575 CI with:
(Linux ETXTBSY — a process tried to fork/exec a file another
process had opened for write.)
What
Download to
<cached>.tmp-<random>viaos.CreateTemp, verify SHA +chmod on the temp path, then
os.Renameinto place. Rename within onefilesystem is atomic; any observer sees either the old/absent file or
the complete new one, never a torn write. Deferred
os.Remove(tmp)cleans up either way.
Two parallel downloaders both land on their own temp, finish, rename.
Last rename wins by content, which is fine — identical bytes by
content hash (the SHA check on the temp gates this).
Why not flock / sync.Mutex
flock()works but adds a dependency and the "wait for the otherdownloader" semantics are unnecessary — both downloaders fetch the
same bytes.
sync.Mutexonly helps within one process.go test ./pkg/A/... ./pkg/B/...runs two separate test binaries; a mutex would missthe cross-process race.
Why #575's tests didn't catch it
Fetcher unit tests use a mock HTTP server (deterministic) or
path:-mode (no download). The race fires only against the real
GitHub releases CDN under parallel
go test ./...load, which onlyCI reproduces.
Test plan
go build ./...go test ./pkg/plugin/... -count=1— green (2.6s + 0.07s)