Skip to content

fix(rstest): write generated test modules atomically - #844

Merged
ScriptedAlchemy merged 6 commits into
mainfrom
fix/atomic-rstest-writes
Sep 25, 2026
Merged

ScriptedAlchemy merged 6 commits into
mainfrom
fix/atomic-rstest-writes

Conversation

@ScriptedAlchemy

@ScriptedAlchemy ScriptedAlchemy commented Sep 25, 2026 •

Copy link
Copy Markdown
Owner

Fixes #843

Why

agentBundleRstest() and agentBundleBrowserRstest() regenerate modules under .agent-bundle/test on every run. Each writer called writeFile on the target in place. writeFile truncates 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 single pnpm run check does not.

Scope

  • src/rstest/generated-module.ts adds writeGeneratedTestModule. It writes the source to a unique sibling with wx, then renames it over the target. A reader sees the previous module or the new one.
  • writeTestMetaModule, writeRouteTestSetup, and writeBrowserTestSetup now call it. Their in-place mkdir plus writeFile code is deleted.
  • The package has no shared atomic string writer to reuse. The existing temp-and-rename sites (routes/typegen.ts, eval/run-store.ts, install/receipt.ts) stay as they are, since each carries its own policy.
  • The helper does not skip unchanged content. Rename already makes the write safe, and nothing measured the extra rewrite as a cost.
  • One patch changeset for agent-bundle.

Verification

  • A cross-process repro ran 20 writer processes against one meta.mjs of 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.ts lands 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.
  • Before the fix the test failed 6 of 6 runs, all three writers. Reads came back as partial: 0 of 2099278 chars and partial: 524288 of 2099278 chars.
  • After the fix it passed 6 of 6 runs.
  • pnpm run check passed. Unit ran 4478 tests, route-unit 91, projection 197, and integration 1183, with 0 failures. Lint and typecheck passed.
  • The first gate run had one failure in packages/workbench/tests/examples-real.e2e.test.ts. A route stayed running past 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

  • Outcome-Oriented Execution. The three writers converge on one helper in a single change. No flag or fallback keeps the in-place write.
  • Fix Root Causes. The fix targets the truncate-then-write, not a retry in the reader.
  • Test Behavior, Not Implementation. The test drives the public writers and asserts what a concurrent reader observes.

Review

Independent review, not the fixer. Verdict PASS+NOTES.

  • Head 3a4adfc5d1167dd1561a5400cab330365b180917. git diff origin/main...3a4adfc5d1 | git patch-id --stable gives c54a26f79e6994c3c3fe4fd22a82783c9b61a17d. Base was origin/main at 3b667c8019.
  • The PR test tests/rstest-generated-module-write.test.ts was copied onto base. It failed 3 of 3 runs with all three writers failing, including reads like partial: 0 of 2099278 chars. On head it passed 3 of 3 runs (measured).
  • A separate cross-process probe ran 12 writer processes (40 rewrites each) and 2 reader processes for 4s, against the real writeTestMetaModule (4,196,342 bytes) and writeRouteTestSetup. 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).
  • The failure path was checked by making the target a directory so rename rejects with EISDIR. The rejection propagates, and on head only the target entry remains, so the finally rm cleans up the temp file (measured).
  • The rename stays in the same directory. The temp file and the target are both joined from one directory at src/rstest/generated-module.ts:17-19, so the rename is atomic on POSIX (measured by read).
  • No shim, dual path, or leftover in-place write. All three writers call writeGeneratedTestModule, and their mkdir plus writeFile bodies are deleted. config/rendered-skill.ts:110-116 writes only into a private temp directory (measured by grep and read).
  • Gate on head. The first pnpm run check failed 1 of 1187 integration tests, packages/workbench/tests/examples-real.e2e.test.ts with 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 of pnpm run check exited 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.

  • Windows, at src/rstest/generated-module.ts:23. libuv's rename uses MoveFileExW with MOVEFILE_REPLACE_EXISTING, and Node and Rust readers open files with FILE_SHARE_DELETE, so replacing an existing target works. Two processes renaming onto the same target at once can still fail on Windows with a transient EPERM or EACCES, 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 Windows host-filesystem CI slice, so this path is unmeasured (inferred).
  • A process killed between writeFile and rename leaves a .<name>.<pid>.<uuid>.tmp file in .agent-bundle/test, because finally does not run on SIGKILL. The file is harmless and nothing imports it (inferred).
  • A related hazard is out of scope. src/rstest/browser.ts:130-132 deletes and recompiles .agent-bundle/test/browser-app-build in 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).
  • Head is 6 commits behind origin/main. git merge-tree shows a clean merge, and the only src/rstest or src/test change on main since the merge base is src/test/contract.ts. AGENTS.md asks for the gate on a branch that contains current main (measured).

Landing gate (head 0ec282fd91, contains origin/main 184ff0235d)

Follow-up in 7bae3927f0: writeGeneratedTestModule now retries a replacing rename on win32 when it fails with EACCES, EBUSY, or EPERM. 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.ts injects the rename failures. The win32 case fails with EPERM when 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 lint
  • pnpm test:unit: 307 files, 4330 tests
  • pnpm test:route-unit (91) and pnpm test:projection (197), run concurrently against the same .agent-bundle/test modules
  • Integration rstest-meta-consumer and test-browser-rstest: 7 tests
  • Packed scaffold-packed.e2e and rsc-runtime-optional-packaging via scripts/run-packed-tests.mjs: 5 tests

Review: change-risk-reviewer on Claude Opus 5.5.

  • Should-fix: concurrent replacing renames on Windows can fail with transient errors. Fixed above.
  • Nit: the rm in finally can mask the primary error. Left as is, to match the sibling private writers.
  • The follow-up review found no material issues.
  • The concurrency test was not added to the Windows host-filesystem slice. Its tight reader loops keep the target open almost continuously, which Rstest's brief module loads don't do.

The Codex changeset thread was fixed in 3a4adfc and is resolved.

@changeset-bot

changeset-bot Bot commented Sep 25, 2026 •

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 0ec282f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
agent-bundle Patch
create-agent-bundle Patch

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

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 25, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-09-25T02:28:19.758198Z e2e1a72 PR opened
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread .changeset/atomic-rstest-generated-modules.md Outdated
@pkg-pr-new

pkg-pr-new Bot commented Sep 25, 2026 •

Copy link
Copy Markdown
npm i https://pkg.pr.new/ScriptedAlchemy/agent-bundle@844
npm i https://pkg.pr.new/ScriptedAlchemy/agent-bundle/create-agent-bundle@844
npm i https://pkg.pr.new/ScriptedAlchemy/agent-bundle/rsc-markdown-stream@844
npm i https://pkg.pr.new/ScriptedAlchemy/agent-bundle/@agent-bundle/runtime@844

commit: 3a4adfc

@ScriptedAlchemy
ScriptedAlchemy merged commit cf82ffe into main Sep 25, 2026
5 checks passed
@github-actions github-actions Bot mentioned this pull request Sep 25, 2026
@ScriptedAlchemy
ScriptedAlchemy deleted the fix/atomic-rstest-writes branch September 25, 2026 20:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

writeTestMetaModule and the rstest setup writers rewrite generated modules non-atomically

1 participant