fix(renderer): publish cycle artifacts atomically - #3996
Conversation
Concurrent top-level module transforms can share a graph-content-addressed cycle artifact even when they use one shared module cache, because that cache stores only completed paths. Direct writes exposed partial bytes to the sibling transaction during manifest sealing. Stage each immutable cycle artifact under a unique sibling path, serialize publication by final path, and atomically rename only complete bytes. Later identical writers reuse the durable artifact, while conflicting bytes fail closed. Keep ordinary module persistence unchanged. Expand the race to eight shared-cache callers, add a deterministic publication regression, retain corruption guards, and pass 200 consecutive stress runs. Fixes veryfront/veryfront-issue-inbox#613
|
Warning Review limit reached
Next review available in: 5 minutes Limit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
📦 Client bundle boundary
A server module in a client graph aborts hydration in the browser. New leaks fail CI; known leaks are tracked in |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 98f44a2942
ℹ️ 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".
Publishing a cycle artifact stages complete bytes under a unique sibling path, renames that file into place, and removes the staging file on every exit path. Nothing covered the removal or the conflict guard, so both could be deleted without a test noticing. Assert that a finished publish and a rejected publish each leave no `.pending-` file behind, and cover the branch that refuses to publish when the artifact path already holds different bytes. Both assertions fail on the base commit: the base writes straight to the artifact path and overwrites conflicting bytes instead of rejecting.
|
@codex review |
|
@coderabbitai review |
|
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
2 participants
Summary
Reachability analysis
A shared
moduleCachedoes not prevent the race. It maps cache keys to completed path strings only. It has no in-flight entry. Two concurrentloadModulecalls can both miss, enter separate top-level cycle-manifest transactions, and write one graph-content-addressed root path.The existing per-source in-flight sharing is scoped to one
CycleManifestTransaction, so it cannot coordinate those independent top-level graphs.Request-wide transform coalescing is not the right fix. It would couple independent cancellation signals and could merge source generations. The shared mutable boundary is artifact publication. Content-addressed graph paths make complete-byte atomic publication the smaller and safer invariant.
Failure mechanism
persistTransformedModulewrote transformed code directly to the final cycle artifact path. A sibling transaction could read that path while the write was incomplete. Its commit then raised either:Cycle manifest root evidence is missing, orCycle manifest changed after its root was sealed.Each writer now writes a complete unique staging file. Publication for a final path is serialized. The first writer renames its staged file atomically; later identical writers reuse it. The manifest commit can observe an old complete file or the new complete file, never partial bytes.
Red-green TDD
Red before the implementation:
That deterministic test runs eight concurrent writers and records every write and rename. The unchanged source wrote all eight directly to the final path and performed no rename.
The original race now also runs eight simultaneous top-level transforms with one shared
moduleCache. All callers still receive the same path, module namespace, cycle namespace, andimport.metaidentity.The two corruption guards remain independently reachable and tested:
Cycle manifest root evidence is missing;Cycle manifest changed after its root was sealed.Verification
module-persistence.test.ts: 20 steps passedcycle-manifest.test.ts: 10 steps passedindex.test.ts: 69 steps passedsrc/rendering/orchestrator/module-loader: 13 passed / 165 stepssrc/rendering/orchestrator: 33 passed / 503 stepsdeno task build:npm: passeddeno fmt --check: passeddeno lint: passeddeno check src/index.ts: passeddeno task generate:manifests:check: passedgit diff --check: passedFixes veryfront/veryfront-issue-inbox#613