From 95acaa500de7503273a8b3bbf89a29980133799e Mon Sep 17 00:00:00 2001 From: Ilia Sadykov Date: Sun, 8 Nov 2026 22:20:46 +0300 Subject: [PATCH] =?UTF-8?q?fix(plugin):=20atomic=20rename=20on=20plugin=20?= =?UTF-8?q?binary=20download=20=E2=80=94=20prevents=20ETXTBSY=20race?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 `.tmp-` 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 --- pkg/plugin/fetcher.go | 43 +++++++++++++++++++++++++++++++++++++++---- 1 file changed, 39 insertions(+), 4 deletions(-) diff --git a/pkg/plugin/fetcher.go b/pkg/plugin/fetcher.go index 87b7bc78..1fa0f21f 100644 --- a/pkg/plugin/fetcher.go +++ b/pkg/plugin/fetcher.go @@ -83,23 +83,58 @@ func resolveGithubRelease(spec api.PluginSpec, cacheRoot string) (string, error) return "", errors.Wrap(err, "mkdir plugin cache") } + // Download to a unique temp path, verify SHA, chmod, then os.Rename to + // the final cached path. Reasons: + // + // - **Concurrent downloads race on the final path.** Parallel test + // binaries (`go test ./...` runs packages in parallel by -p) both + // see a cache-miss and both write to the same `cached` filename. + // The second write truncates the first in-flight bytes; a third + // goroutine trying to `fork/exec` the half-written file gets + // ETXTBSY ("text file busy") on Linux. Live-caught on #568's CI + // (2026-11-08): sc-plugin-gcp's 230MB binary hit this reliably + // under parallel Test_Provision + TestScopeRegression_*. + // + // - **os.Rename within one filesystem is atomic.** Two concurrent + // downloaders each land on their own `.tmp-` path, finish + // writing + checksum + chmod there, then rename. The final file + // is complete at the instant it appears; any other goroutine + // either sees the old (or absent) file or the complete new one, + // never a torn write. Last-renamer wins, which is fine — both + // downloaders fetched the same release artifact by content hash. + // + // - Keeping the SHA check + chmod on the temp path means no state + // ever leaks to the final path if something fails mid-flight. + // (The deferred `os.Remove(tmp)` cleans up the temp either way — + // after a successful rename it's a no-op; after an error it + // removes the partial download.) + tmp, err := os.CreateTemp(filepath.Dir(cached), binName+".tmp-*") + if err != nil { + return "", errors.Wrap(err, "create temp plugin binary") + } + tmp.Close() + tmpPath := tmp.Name() + defer os.Remove(tmpPath) + url := fmt.Sprintf("https://github.com/%s/releases/download/%s/%s", src.Repository, src.Version, binName) - if err := downloadFile(url, cached); err != nil { + if err := downloadFile(url, tmpPath); err != nil { return "", errors.Wrapf(err, "download %q", url) } - got, err := fileSHA256(cached) + got, err := fileSHA256(tmpPath) if err != nil { return "", err } if expectedSHA != "" && !strings.EqualFold(got, expectedSHA) { - _ = os.Remove(cached) return "", errors.Errorf("plugin SHA256 mismatch for %s@%s/%s: want %s, got %s", src.Repository, src.Version, binName, expectedSHA, got) } - if err := os.Chmod(cached, 0o755); err != nil { + if err := os.Chmod(tmpPath, 0o755); err != nil { return "", errors.Wrap(err, "chmod plugin binary") } + if err := os.Rename(tmpPath, cached); err != nil { + return "", errors.Wrap(err, "rename plugin binary into cache") + } return cached, nil }