fix: SPA/prerender build hangs on module-scope handles; stale server manifest under buildCache - #137
fix: SPA/prerender build hangs on module-scope handles; stale server manifest under buildCache#137ScriptedAlchemy wants to merge 4 commits into
Conversation
…fest - SPA-mode index.html and prerendering (classic and RSC) evaluate the server bundle in a worker thread that is terminated afterwards, so a module-scope handle in the app's server graph cannot keep rsbuild build alive (#135). - The node server-manifest module declares a file dependency on the captured manifest so Rspack's persistent cache invalidates it when the web build's asset names change (#136).
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. |
commit: |
Benchmark results
|
Merging this PR will not alter performance
Comparing 
Footnotes
|
…d template node_modules
ScriptedAlchemy
left a comment
ScriptedAlchemy
left a comment
There was a problem hiding this comment.
Review of 58c133a — two worker-lifecycle corrections before merge
The old #134 findings remain resolved on the unchanged base 8109c7d; this review is about the new #137 delta. Keep mandatory MF async startup and non-eager sharing. Nothing here calls for disabling either requirement, changing browser automatic public paths, or reopening the previous filename/split-chunk findings.
I checked the 13-file diff and relevant prerender callers, and verified Build Test run 34661224734: unit tests, package build, publint, ecosystem tests and example E2E all succeeded at this head. The worker isolation and declared cache dependency address the right boundaries; do not replace them with process-wide handle scanning, forced process.exit, or broad cache disabling.
The two inline findings are reproducible in the new transport:
- The Request reconstructed in the worker loses the caller's abort signal. Existing
createBuildRequestEffect/withBuildRequestfinalizers still abort the parent Request, but cleanup attached to the actual handler's Request no longer executes. A direct-handler control left zero outstanding abort-cleanup registrations after release; the worker path left one. An in-flight handler waiting for abort also remained pending after the parent aborted. - An exit with an empty pending map does not mark the worker terminal. A synthetic RSC handler returned a response and then called process.exit(0) on a timer; after the worker had exited, the next handler call remained pending instead of rejecting. A worker exit must be remembered independently of whether an RPC happens to be in flight.
Verification scope
These are actual Node worker-thread probes using copies of server-build-worker-client.ts and server-build-worker.ts whose Git blob hashes match this PR (7aaba4d273a641767b9225efbb372cbed175c3ac and d33d1c10017166ce75d1141f5d56048ae8b09d7b). Only TypeScript types were erased. A synthetic RSC fetch handler supplied the controlled behavior; the unused classic React Router/resolution imports were stubbed. This is not represented as a full Rsbuild/browser reproduction. Local repository/dependency downloads were blocked by DNS.
Keep the correction small
Preserve the request lifetime inside the worker, including abort-on-completion/error and cancellation of live requests. Make worker exit/close an unconditional terminal state and reject subsequent requests. Extend an actual worker-boundary fixture; the existing abort helper unit tests cannot observe the Request inside the worker. No retries, worker pool, generic RPC framework, or additional browser matrix is needed.
A non-blocking simplification: the worker already computes the classic build description before announcing readiness. Include it in the ready reply and retain it in the client, removing the separate describe request/response branch. This reduces protocol and lifecycle states rather than adding another abstraction.
For the manifest stamp, keep the explicit dependency. Avoid rewriting identical contents where practical, but that is an I/O/cache-efficiency cleanup, not a separately reproduced stale-cache defect.
Recommendation: address the two lifecycle findings before merging; retain the architecture and current behavioral regression coverage.
| const response = await handler( | ||
| new Request(message.url, { | ||
| method: message.method, | ||
| headers: message.headers, | ||
| body: message.body as BodyInit | undefined, | ||
| }) |
There was a problem hiding this comment.
P2 — Preserve abort-on-release for the Request the app actually receives
This constructs a new Request with an independent, never-aborted signal. createBuildRequestEffect in prerender-build.ts still aborts the caller's controller when rendering completes/fails, and tests/prerender.test.ts explicitly checks that contract, but that abort now stops at the worker boundary. A handler that registers request-scoped cleanup with request.signal.addEventListener('abort', cleanup) no longer receives it; because one worker serves the entire prerender batch, those resources can accumulate until the whole batch is terminated.
Reproduced with the exact type-erased worker/client source and a synthetic RSC handler: increment a counter on /resource, decrement it on that Request's abort, return a body, consume the parent Response and abort the parent controller, then query /state. Direct-handler control: counter 0. Worker path: counter 1. A handler waiting for abort also never completed when the parent aborted.
Give each worker-side request an AbortController and dispose it in finally after body consumption/error; relay cancellation for requests still in flight using the existing request IDs. Add a real worker-boundary regression for cleanup on success/failure rather than another parent-only signal assertion. Do not rely solely on terminating the worker after all routes.
There was a problem hiding this comment.
Fixed in 9ea6ee3. The worker now creates an AbortController per request and passes its signal to the Request the app receives; it is aborted when the parent releases the request (relayed as an abort message using the request id) or, as in the in-process path, once the response body has been consumed or the handler failed. Also took the simplification: the classic build description rides on the ready message, so the describe branch is gone.
Covered by a real-worker unit test against the built worker (tests/server-build-worker.test.ts: abort-on-release observed via the app's own request.signal listener, and an in-flight request that only resolves when the parent's abort reaches it) plus a real-build case in spa-build-process-test.ts where a root loader logs each aborted request path. Reintroducing the defect fails exactly those tests.
| worker.on('exit', code => { | ||
| if (pending.size > 0) { | ||
| failAll( | ||
| new Error( | ||
| `Server build worker exited with code ${code} while rendering` | ||
| ) | ||
| ); | ||
| } |
There was a problem hiding this comment.
P2 — Record every worker exit as terminal, even when no request is pending
When the worker exits between requests, this condition leaves failure unset. The next send() adds a pending entry and posts to a dead worker, so there is no responder or future exit event to settle that Promise. Prerendering does asynchronous body/output processing between handler calls, so an exit can occur in exactly this idle window.
Reproduced with the exact client and worker source: an RSC handler returns a response and schedules process.exit(0) shortly afterwards; consume the response, wait for exit, then call handler() again. The new request stays pending instead of rejecting. This can strand a programmatic build or let a CLI stop without normal completion; it should be a deterministic build error.
Set terminal state on every exit, reject existing pending work, and reject future sends immediately. Mark explicit close() terminal too, distinguishing expected termination from unexpected exit. The pending count should control only whether there are promises to reject, not whether the exit is remembered. No restart/retry machinery is needed.
There was a problem hiding this comment.
Fixed in 9ea6ee3. Every exit (and error) is now recorded as terminal regardless of the pending count, so outstanding requests reject and any later handler() call rejects immediately with Server build worker exited with code N; close() is terminal as well. No restart/retry.
Covered by tests/server-build-worker.test.ts: the fixture's fetch schedules process.exit(0) after replying, the test waits until nothing is pending, and the next two calls must reject. Reintroducing the pending.size > 0 guard makes that test fail. CI now builds before unit tests so the worker entry exists.
…exit as terminal - Each worker-side request gets its own AbortController; the Request the app receives is aborted when the parent releases it (relayed 'abort' message) or once its response has been consumed, matching the in-process contract. - Any worker exit is recorded as terminal regardless of pending requests, so a request sent to a worker that exited while idle rejects deterministically; close() is terminal too. - The classic build description rides on the ready message; the separate describe round-trip is gone. - Real-worker unit test (tests/server-build-worker.test.ts) against the built worker covers abort-on-release, in-flight abort relay, error mapping, idle exit, close(), and import failure; CI builds before unit tests.
- Type describeClassicBuild against ServerBuild; drop BuildRouteLike and the as-unknown cast. Type wire bodies as Uint8Array<ArrayBuffer> so no BodyInit or transfer-list casts remain. Drop the unread basename field. - Share headerEntries via the protocol module; reuse normalizeEffectError in the client; inline the one-liner in server-build-resolution so the worker bundle no longer pulls in the Effect runtime (361 KB -> 3 KB shared chunk). - Write the manifest stamp only when its content changed (no spurious cache misses or node rebuilds if the cache dir is watched); stamp the base manifest only; remove the unreachable non-classic write. - Terminal-failure check moved inside the request executor; null body by byteLength; request.body as the has-body condition. - Tests: rely on the harness default rsbuild.config (rsbuildConfig.basic gained buildCache), createEditor for the root edit, shared expectBuildSucceeded, one lifecycle fixture builder; drop the existsSync guard that setup.ts mocks. - Remove dead resolveServerBuildModule re-export and PrerenderServerBuild alias.
1 participant
Fixes #135 and #136 (both reported with clone-and-run repros; both re-run against this branch below).
#135 —
rsbuild buildnever exits withssr: falseSPA-mode
index.htmlgeneration and prerenderingimport()ed the freshly built server bundle into the build process. Any ref'd handle created at module scope in the app's server graph (the reporter'sBroadcastChannel) then kept the event loop alive afteronAfterBuildreturned, and Rsbuild'sbuildcommand onlyprocess.exits on the error path.Fix: the server bundle is now evaluated in a
worker_threadsWorker (dist/server-build-worker.js) that isterminate()d once rendering is done. The worker serves requests over a small message protocol and, for classic builds, returns a plain-data description of the build (route table, export presence,assets.routes[*].hasLoader,prerender) — everything the prerender code reads.IS_RR_BUILD_REQUEST=yesis set inside the worker only, so app-side guards keep working and the flag no longer leaks into the build process. Applies to classic SPA mode, classic prerender, and RSC prerender.#136 — warm
buildCachebuild rendersindex.htmlagainst the previous build's assetsThe node
virtual/react-router/server-manifestmodule has constant source; its real content is injected by a transform from the web compilation's emitted asset names. Nothing Rspack hashes for that module changes between builds, so the persistent cache legitimately restored the previous build's module — and the SPAindex.htmlimportedmanifest-<old>.js/root.<old>.jsthat no longer existed.Fix: the plugin writes the captured manifests to
<cachePath>/react-router/server-manifest.jsonwhenever they are captured, and the server-manifest transform declares that file as a dependency (addDependency/addMissingDependency). The cache now invalidates exactly when the manifest changes; dev mode is unaffected (dev/HMR suites green).Tests
tests/react-router-framework/integration/spa-build-process-test.ts(real builds):ssr: false, prerender, and RSC prerender each exit with status 0 while the root route's graph creates a module-scopeBroadcastChannel(build()helper gained atimeout+ SIGKILL so a hang fails instead of stalling CI).ssr: false+performance.buildCache: true: cold build → edit root → warm build; every/static/js/*.jsreferenced byindex.htmlexists in the warm output and differs from the cold set.Negative controls run locally against the pre-fix source: the hang test hangs (the process even ignores
timeout's SIGTERM), and the cache test fails with the warmindex.htmllisting the cold build's scripts.The RSC prerender unit test mocks the worker client (the worker ships in
dist/, which doesn't exist when unit tests run from source); the worker is exercised by the integration suite.Verification
pnpm test(typecheck + 676 unit tests) greenspa-build-process,spa-mode,prerender,rsc-client-version,build,route-entry-names— 99 passed;hmr-hdr,dev— 16 passedpnpm repro:hang→ "NOT REPRODUCED: the build exited on its own with code 0 after 5.7s";pnpm repro:stale-manifest→ "NOT REPRODUCED: every URL the warm index.html references exists in the warm output"IS_RR_BUILD_REQUESTset.Review follow-up (9ea6ee3)
AbortControllerper request and passes its signal to the app's Request; aborted when the parent releases the request (relayedabortmessage) or once the response is consumed / the handler fails — same contract as in-process rendering.exit/erroris recorded regardless of pending requests; laterhandler()calls reject immediately, andclose()is terminal.readymessage (nodescriberound-trip).tests/server-build-worker.test.tsdrives the built worker with real worker threads (abort-on-release, in-flight abort relay, error mapping, idle exit, close, import failure); CI builds before unit tests. Real-build cases added tospa-build-process-test.ts(loader-observed aborts; app exiting the worker mid-build fails deterministically). Reintroducing either defect fails the corresponding tests.Changeset: patch.