Repository navigation
fix(server): dedupe server error hook reports per request, not per process - #4130
Merged
Merged
Conversation
…ocess The server error hook's once-per-error ledger (`verdicts`) was one process-global WeakMap keyed by the error object, so an error object reused across requests (a module-level constant, a cached rejection) was heard only by the first request's hook, and later requests were handed the first request's mapped wire value (#4125). The ledger is now keyed by scope, then error: the request a render started under (keyed by `event.request`, which the derived event of a direct SSR-time server-function call shares), else the render itself (a per-render object @solidjs/web puts on the render context as the internal `_errorScope`), else no dedup at all. Every road within one request still reads the same verdict, so a server-function failure that escapes into a <Loading> is reported once and carries one mapped value. Refs #4125 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018LY7F9czoRqdMEvk2QwPr1
🦋 Changeset detectedLatest commit: 267af39 The changes in this PR will be included in the next version bump. This PR includes changesets to release 14 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 |
Size (brotli, eager entry chunk)
|
Coverage Report for CI Build 39018278347Coverage remained the same at 77.564%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
…r process `throwers` was a process-global WeakMap from error object to the owner it was first thrown under. A reused error object (a module-level constant) kept the first request's owner, so later requests' hook reports and findings named the first request's `ownerPath`, and that request's owner tree stayed reachable for as long as the error lived. The thrower now lives on the error's verdict in the same scope the hook ledger uses (the request in scope, else the running render's `_errorScope`), so it is collected with that scope and a later request stamps and reads its own. Refs #4125 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018LY7F9czoRqdMEvk2QwPr1
…p-per-request # Conflicts: # packages/web/frames/src/frame-sink.ts
Merging this PR will improve performance by 8.87%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | createStore setter: delete + set one root key (#3044 overlay) |
1.9 ms | 1.8 ms | +8.87% |
Tip
Curious why performance improved? Comment @codspeedbot explain why performance improved on this PR, or directly use the CodSpeed MCP with your agent.
Comparing fix/server-error-dedup-per-request (267af39) with next (7010268)
Footnotes
-
5 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
… failure always reaches it A direct server-function call that throws during SSR was heard once, as `kind: "server-function"`; the <Errored> or <Loading> that then contained it was deduped away, so a render's `onError` could not tell that the render failed (#4125). The per-scope verdict now records the kinds it has been heard as: the render's sight is reported too, while the wire stays the first answer a hook gave. `kind: "request"` stays the catch-all, heard only when no other kind was. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018LY7F9czoRqdMEvk2QwPr1
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
3 participants
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
Before: the server error hook (
configureServerErrors, a render'sonError) kept one process-wide "already heard this error" ledger keyed by the error object. If two requests met the sameErrorobject (a module-level constant, a cached rejection, a memoized failing fetch), only the first request's hook heard about it. The second request got no report and was also handed the first request's mapped wire value. An unrelated request's failure was suppressed because a different request had failed earlier with the same object.After: the hook hears each error object once per kind per request, or per render when no request is in scope, and never once per process. A direct server-function call that throws during SSR inside an
<Errored>or<Loading>is heard twice: asserver-function(the call failed), then asrender(the render fell back). So a render'sonErrorseeingkind: "render"is enough to know that render failed, which is what #4125 asks for. Every road still carries one wire value, the first one a hook returned, and each request decides its own, so request B never gets request A's mapping.Refs #4125. With this, #4126's
hasErrorsflag isn't needed.When is there no request in scope?
The request scope is the
Symbol.for("solid.RequestContext")store (AsyncLocalStorage, installed by@solidjs/web/storage'sprovideRequestEventor by a framework). A render has no request when:renderToString/renderToStream/renderToFrameStreamis called withoutprovideRequestEventaround it. That covers static generation, prerender scripts, tests, and a bare Node script.@solidjs/web/storagewas never imported, so no store exists at all.AsyncLocalStorage(or only a sync polyfill) and the code runs after anawait. The store reads empty there even when the render started inside a request.In all of these cases the render is now the scope. Server functions always have a request: HTTP dispatch has its event, and a direct call throws "Cannot call server function outside of a request". A report with neither a request nor a render in hand (a bare
reportServerErrorfunnel, which has no call site that lacks both) is not deduplicated at all.How
verdictsinpackages/solid/src/server/signals.tsis nowWeakMap<scope, WeakMap<error, Verdict>>. The scope is picked in this order:_errorScopeon the render context, set byrenderToString/renderToStream: the request event the render started under, otherwise a fresh per-render object. It is inherited by the derived contexts (Object.create(ctx)) of<Loading>and frames.site.event.An event counts by its
request. The derived{...event}that a direct SSR-time server-function call runs under shares thatrequestwith the render that made it, which is what keeps that case at one verdict.Within a verdict,
heardrecords the kinds the hook has heard. A new kind is reported; a repeat of a kind is not. A hook's answer, once given, stays the wire (mapped), and a later kind's return is ignored. The default policy's value does not bind a later hook answer, as before.kind: "request"(reportRequestFailure) is the catch-all: it's heard only when no other kind was, so passing a render's own throw to it still adds nothing.reportServerError/ssrSanitizeErrortake an optional trailingscope. This is internal ABI only; there are no new exports or options. The boundary sites (<Errored>,<Loading>), the renderer's own sites (failRender, teardown, first-pass throw), the fragment and channel funnels, and the live-hole error chunks pass their render's scope. Server-function sites keep reading the request.Golf to fit the
renderToStringfloor: a verdict is always an object, so the!== undefinedguards are gone. The request-store and ambient-hook reads use?..Docs (RFC 10/12, the
ServerErrorHook/configureServerErrors/reportRequestFailuredoc comments) now say "once per error object per kind per request".throwers(where an error was thrown, used forownerPath) was also a process-globalWeakMap<error, Owner>. With a reused module-levelError, every later request reported the first request'sownerPath, and that request's owner tree stayed reachable for as long as the error lived. The thrower now lives on the error's verdict in the same scope: it is stamped under the request in scope, else the running render's_errorScope, and read back under the same scope. It is collected with its request or render, and each request stamps and reads its own. The separatethrowersmap is gone.How did you test this change?
New tests in
packages/web/test/server/server-error-hook.spec.tsx, under "once per request, never once per process (#4125)":Errorreused by two requests'<Loading>fragments. Both hooks fire, each with its own event, and each document carries its own request's mapping.renderToStreams with no request in scope share oneError. Each render'sonErrorhears it once, and each document carries only its own mapping.<Loading>(the Expose per-render failure state for buffered SSR consumers #4125 server-function repro): aserver-function/throwncall, then arender/clientcall. The hook returns a different value the second time, and the source's rejection and_frboth carry only the first, and the original never appears (this tier's default is fidelity, so a road that missed the verdict would leak it).<Errored>s in one render meet the sameError: one call, and both fallbacks render the mapping. The next render hears it again.Errorthrown by<First>in request 1 and by<Second>in request 2, and the same again in two renders with no request: each report'sownerPathnames its own thrower. This fails with the old globalthrowers.The existing direct-call tests (
<Errored>around a throwing call, and the two inconcurrent-render-error-policy.spec.tsx) now expect therendersight after theserver-functionone, with the function's mapping still on the wire. I also added areportRequestFailurecase (the same error object for two requests is heard twice) and updated the existing "once per error object" tests so the handler reports with the same event the render ran under. Tests 1, 2 and 4 and the newreportRequestFailurecase fail onnext.packages/solid:npx vitest run: 869 passed.cross-package-fieldsneedspackages/universalbuilt; it passes once that is built.packages/webclient: 1379 passed. Hydrate: 506 passed, 16 expected fail.packages/webserver (--config vite.config.server.mjs, includes the server-functions specs): 1715 passed, 1 failed. The failure is the known environmentalserver-functions-adapter-request(Solid 2: every POST server function fails with 400 "Malformed server function arguments" under Nitro/srvx, becausebodySizeLimitrebuilds the request #3311 on Node 22).tsc(build and test configs) is clean for solid-js and @solidjs/web, and prettier is clean.Size (
node scripts/size/size.mjsvsnext@ 7010268,gate.mjs)Only one scenario moved:
The gate passes with a WARN: 34 B over the brotli cap, but minified is 23 B under the size recorded with the cap.
check-floor-capsreports no cap raised, so no Size-Exception is needed.https://claude.ai/code/session_018LY7F9czoRqdMEvk2QwPr1
Generated by Claude Code