Repository navigation
[Flaky test fix] TestSandboxAssetPrewarmDownloads - atelet: make the sandbox prewarm worker joinable, fixing the race - #1940
Merged
Benjamin Elder (BenTheElder) merged 2 commits intoOct 6, 2026
Conversation
…race TestSandboxAssetPrewarmDownloads patches nodepath.StaticFilesDir and restores it in t.Cleanup, but returned as soon as the prewarmed asset appeared on disk — while the prewarm worker was still running its tail (the pause-image pull) and reading that global. On a loaded runner the overlap trips the race detector and fails the run-tests job; nothing let a caller wait for the worker to exit. startSandboxAssetPrewarm now returns the prewarmer, whose worker closes a done channel on exit and exposes wait(). The test joins the worker via t.Cleanup, which runs after t.Context() is canceled and before the earlier-registered cleanup restores the globals.
TestActorEgressHTTPSByHostnameMITM failed because the live upstream the egress e2e tests fetch (example.com/example.org) served 404s to the runner for ~30s; unrelated egress-HTTPS tests failed the same way on another branch minutes later. Nothing in this change touches the egress path.
Maya Wang (mayawang)
added
area/node
area/tests
Zoe Zhao (zoez7)
self-requested a review
September 30, 2026 16:38
Aditya Shantanu (aditya-shantanu)
changed the title
Benjamin Elder (BenTheElder)
approved these changes
Oct 5, 2026
Benjamin Elder (BenTheElder)
left a comment
Benjamin Elder (BenTheElder)
left a comment
Collaborator
There was a problem hiding this comment.
not going to block on this nit to pickup test fix but
Comment on lines
+388
to
+393
| // The worker reads the patched globals above. t.Context() is canceled | ||
| // before cleanups run, and cleanups run last-registered-first, so this | ||
| // joins the worker before the earlier cleanup restores the globals — | ||
| // without it the restore races the worker's tail (pause-image pull | ||
| // after the asset lands) and trips the race detector. | ||
| t.Cleanup(p.wait) |
Collaborator
There was a problem hiding this comment.
🤖 nit 🟢 – The pause-image pull runs concurrently with the asset download in prewarm, not after the asset lands. The comment could also be shorter, for example: "Join the worker before the earlier cleanup restores the globals it reads. t.Context() is canceled before cleanups run."
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
Fixes the flaky
TestSandboxAssetPrewarmDownloads(unit lane).Root cause: the test patches
nodepath.StaticFilesDir/prewarmMaxJitterand restores them int.Cleanup, but returns as soon as the prewarmed asset appears on disk. The prewarm worker goroutine is still running its tail at that point (pause-image pull) and readsnodepath.StaticFilesDirindownloadVerifiedwhile the cleanup writes it — a data race that-raceturns into a hard failure whenever the CI runner is slow enough for the two to overlap.Fix: the worker was not joinable — after context cancellation nothing could wait for it to exit.
startSandboxAssetPrewarmnow returns the prewarmer; its worker closes adonechannel on exit and exposeswait(). The test registerst.Cleanup(p.wait)after the globals-restore cleanup, so (LIFO, andt.Context()cancels first) the order becomes: cancel → join worker → restore globals.main.goignores the new return value.Closes #1939
Evidence
Cross-PR failures at ~1.5% of runs over the last week, including twice on
mainand once on the benchmarking-only #1934 — run links in #1939. The race trace pins write (test cleanup,sandbox_prewarm_test.go:360) against read (sandbox_assets.go:285inside the worker).Test plan
go test -race -run TestSandboxAssetPrewarm -count=200 ./cmd/atelet/— cleango test -race ./cmd/atelet/— full package clean&sandboxPrewarmer{...}test constructors never callrun(), so the nildonechannel is never closed there