fix(rstest): write generated test modules atomically - #844
Conversation
🦋 Changeset detectedLatest commit: 0ec282f The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e2e1a72d8f
ℹ️ 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".
commit: |
Fixes #843
Why
agentBundleRstest()andagentBundleBrowserRstest()regenerate modules under.agent-bundle/teston every run. Each writer calledwriteFileon the target in place.writeFiletruncates the file and then writes it in 512 KiB chunks. A second Rstest process that loads the file in that window gets an empty or partial module. Parallel runs from agents and editors hit this. A singlepnpm run checkdoes not.Scope
src/rstest/generated-module.tsaddswriteGeneratedTestModule. It writes the source to a unique sibling withwx, then renames it over the target. A reader sees the previous module or the new one.writeTestMetaModule,writeRouteTestSetup, andwriteBrowserTestSetupnow call it. Their in-placemkdirpluswriteFilecode is deleted.routes/typegen.ts,eval/run-store.ts,install/receipt.ts) stay as they are, since each carries its own policy.agent-bundle.Verification
meta.mjsof 2,099,200 bytes while a separate process read it for 4 seconds. Before the fix, 16,752 reads were empty and hundreds were partial, against 5,863 complete reads. After the fix, all 7,758 reads were complete.tests/rstest-generated-module-write.test.tslands first in history. It runs two rewrite loops and three readers against each of the three writers, and asserts that every read equals the complete module and that no temp file is left behind.partial: 0 of 2099278 charsandpartial: 524288 of 2099278 chars.pnpm run checkpassed. Unit ran 4478 tests, route-unit 91, projection 197, and integration 1183, with 0 failures. Lint and typecheck passed.packages/workbench/tests/examples-real.e2e.test.ts. A route stayedrunningpast its timeout. That test does not touch the rstest writers. It passed 7 of 7 when run alone, and the full gate rerun passed.Principles
Review
Independent review, not the fixer. Verdict PASS+NOTES.
3a4adfc5d1167dd1561a5400cab330365b180917.git diff origin/main...3a4adfc5d1 | git patch-id --stablegivesc54a26f79e6994c3c3fe4fd22a82783c9b61a17d. Base wasorigin/mainat3b667c8019.tests/rstest-generated-module-write.test.tswas copied onto base. It failed 3 of 3 runs with all three writers failing, including reads likepartial: 0 of 2099278 chars. On head it passed 3 of 3 runs (measured).writeTestMetaModule(4,196,342 bytes) andwriteRouteTestSetup. On base each reader saw about 590 empty, 410 partial, and 290 complete reads for meta. For route it was about 340 empty, 300 partial, and 740 complete. On head every read was complete (272+287 and 591+587), with no temp files left (measured).renamerejects withEISDIR. The rejection propagates, and on head only the target entry remains, so thefinallyrmcleans up the temp file (measured).directoryatsrc/rstest/generated-module.ts:17-19, so the rename is atomic on POSIX (measured by read).writeGeneratedTestModule, and theirmkdirpluswriteFilebodies are deleted.config/rendered-skill.ts:110-116writes only into a private temp directory (measured by grep and read).pnpm run checkfailed 1 of 1187 integration tests,packages/workbench/tests/examples-real.e2e.test.tswith a Playwright assertion timeout. That is the same flake the author reported. It passed 2 of 2 alone on head and 2 of 2 alone on base. Lint and typecheck passed on their own. A full rerun ofpnpm run checkexited 0 with unit 4478, route-unit 91, projection 197, and integration 1183 passing, plus lint and typecheck (measured).Notes. None of these block the merge.
src/rstest/generated-module.ts:23. libuv'srenameusesMoveFileExWwithMOVEFILE_REPLACE_EXISTING, and Node and Rust readers open files withFILE_SHARE_DELETE, so replacing an existing target works. Two processes renaming onto the same target at once can still fail on Windows with a transientEPERMorEACCES, which is the case graceful-fs retries. Nothing here retries. The failure is a loud rejected write, never a partial read. The new test is not in the Windowshost-filesystemCI slice, so this path is unmeasured (inferred).writeFileandrenameleaves a.<name>.<pid>.<uuid>.tmpfile in.agent-bundle/test, becausefinallydoes not run on SIGKILL. The file is harmless and nothing imports it (inferred).src/rstest/browser.ts:130-132deletes and recompiles.agent-bundle/test/browser-app-buildin place on every browser-pool run, so the same concurrent-run race could hit it. This is not a module write, and this PR does not change it (inferred).origin/main.git merge-treeshows a clean merge, and the onlysrc/rstestorsrc/testchange on main since the merge base issrc/test/contract.ts. AGENTS.md asks for the gate on a branch that contains current main (measured).Landing gate (head
0ec282fd91, containsorigin/main184ff0235d)Follow-up in
7bae3927f0:writeGeneratedTestModulenow retries a replacingrenameon win32 when it fails withEACCES,EBUSY, orEPERM. It makes up to 10 attempts with linear backoff, about 450 ms in total. POSIX behavior is unchanged.tests/rstest-generated-module-win32-rename.test.tsinjects the rename failures. The win32 case fails withEPERMwhen the attempt count is set to 1 and passes with the retry. The POSIX case asserts a single attempt and that no temp file is left behind.Local gate, all exit 0:
pnpm build,pnpm typecheck,pnpm lintpnpm test:unit: 307 files, 4330 testspnpm test:route-unit(91) andpnpm test:projection(197), run concurrently against the same.agent-bundle/testmodulesrstest-meta-consumerandtest-browser-rstest: 7 testsscaffold-packed.e2eandrsc-runtime-optional-packagingviascripts/run-packed-tests.mjs: 5 testsReview: change-risk-reviewer on Claude Opus 5.5.
rminfinallycan mask the primary error. Left as is, to match the sibling private writers.The Codex changeset thread was fixed in
3a4adfcand is resolved.