fix: contain request failures at the Start handler edge - #383
Open
everton-dgn wants to merge 1 commit into
Open
everton-dgn wants to merge 1 commit into
everton-dgn wants to merge 1 commit into
Conversation
Fixes solidjs#382. handleRequest now settles whatever escapes the start.middleware chain: a thrown Response or response envelope becomes the response, and in production builds any other failure is reported through the configured server error policy and answered with a bodyless 500 instead of rejecting to the host. A client-mode build fails when prerendering the shell answers a non-2xx status.
🦋 Changeset detectedLatest commit: efdaf56 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
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
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
Fixes #382.
start.middlewarechain at the handler edge instead of letting it reject to the host.Response(exceptResponse.error()), or theResponsea thrownrespond()envelope carries, becomes the response in dev and production.throw redirect('/login')from a middleware now answers the 302, as the server-function endpoint already does.start.setuporstart.renderModemodule failure, an uncaught in-process server-function call) is reported once to theconfigureServerErrorshook, or logged withconsole.errorwithout one, and answered with a bodyless 500. That 500 carries the headers and cookies written to the request event while the response head is still open.dist/client/index.html.Cause
handleRequestran the chain without a catch (src/ssr/index.tsat 52d93eb, L1433-L1453). Each host answered the rejection its own way: Nitro with a JSON 500 that drops the stub cookies,start.nodewith a plain-text 500, and the exampleserver.jsby echoinge.messageto the client. TheconfigureServerErrorshook never saw these failures, and a thrown controlResponsebecame a 500. In dev a thrownResponsereached Vite's error middleware, whoseprepareErrorreadserr.messageand throwsERR_INVALID_ARG_TYPE, so it answered 500 there too.Fix
handleRequestwraps the chain intry/catchinside the request scope and hands failures to a generatedcontainFailure(error, event).commitEventResponse(response, event)still folds strictly after the chain unwinds.isResponseEnvelope(error) ? error.response : error, theninstanceof Response && status !== 0. It runs in its owntry, so a hostile thrown value (a revoked Proxy) is treated as an ordinary failure instead of escaping the catch.reportServerError(error, { kind: 'render', handling: 'failed', event })fromsolid-js/internal, thenconsole.error(error)when no hook is registered. This mirrorsfailRenderin@solidjs/web. The answer isnew Response(null, { status: 500 }), the endpoint's plain-HTTP answer.handleRequest(request, { renderMode })validates the option before the chain runs. A host's bad option still rejects the call instead of being contained as a 500. A bad result from thestart.renderModemodule is a request failure and is contained.if (!response.ok) throw new Error('[@solidjs/vite-plugin] prerendering the client-mode shell failed: the handler answered <status>').Verification
The dev handler changed too, so I also ran the wider local gate on this branch: start-ssr all modes 666/666,
test/http-bridge.mjs10/10,test/components-warning.mjs11/11,test/webworker-warning.mjs12/12,test/dedupe.mjs8/8,test/host-dispatch.mjsok, start-client all modes 68/68, start-env 47/47, css-matrix 87/87.New checks:
src/middleware.ts, all thrown from the outermost middleware outside its error middleware:/mw-throw: an Error, after writing a stub cookie./mw-throw-late: a throw afternext()returned the page./mw-redirect: a thrownredirect()./mw-envelope: a thrownrespond()envelope with a 409 and a header./mw-response-error: a thrownResponse.error()./mw-direct-throw: an uncaught in-process server-function failure./setup-throw: astart.setupfailure.SSR_SERVER_ERRORS=1registers a recording hook, read back from/api/server-errors.Responseand envelope answer 302 and 409.Response.error()is.console.error.reportServerError.handleRequest(request, { renderMode: 'bogus' }).SOLID_SHELL_FAIL=1(astart.middlewarethat throws) exits non-zero with the new message and the original error in the log, and writes nodist/client/index.html.Regression proof:
src/ssr/index.ts: middleware 79/93, preview 35/47, node 75/82, start-client prod 25/26. Render-mode stays 121/121: the renderMode check passes on the old code because it never contained anything.Notes
reportServerErrorfromsolid-js/internal. It is the typed exportfailRenderitself calls, and it is present at the^2.0.0-rc.10peer floor and in rc.11. The issue suggested calling the hook through theSymbol.for('solid-js/server/errors')slot. Going through the runtime's once-per-error ledger instead avoids a second report for an uncaught in-process server-function call, which the runtime already reports asserver-function/thrownbefore rethrowing. The slot is still read, but only to decide theconsole.errorfallback, asfailRenderdoes. A public entry point for request failures in@solidjs/web, with its ownkindinServerErrorSite, would remove this coupling.{ kind: 'render', handling: 'failed' }plusevent. It is whatfailRenderreports and a validServerErrorSite, and the hook's return value is ignored forfailed.Response, and the README now recommendsthrow redirect(). Every other failure in dev is rethrown exactly as before.start.setupandstart.renderModedefault-export checks are module top-level throws: they fail at import, not per request, and are not contained.next()has returned a rendered page, the stub is committed with that page, and a later throw drops them (documented). The streamed body of that dropped page is not cancelled.errorBoundary: false), a render that fails asynchronously before the first chunk leaves the response pending forever. The chain isfailRender, thenabandon, then the pipe'sonFailurecallingend().createSSRResponse'send()returns early while there is no stream controller, so its promise never resolves.createSSRResponseis identical in@solidjs/webrc.10 and rc.11. Nothing is thrown, so this containment cannot catch it, and it needs a runtime fix. Probed on rc.10: pending after 3s without a boundary; a 500 withErrored+httpStatus(500); a 200 shell with onlyLoading.start.renderModemodule failure, which goes through the same catch as thestart.setupone.errorBoundarytoproductionErrorBoundaryin the new README paragraph;start.setup/start.renderModetoapp.*in the codegen comment and the changeset;examples/start-client/src/shell-failure.tswith theapp-clientrename;SOLID_SHELL_FAILknob in that example'svite.config.tsfromstart:toapp:.If refactor: rename Start mode to app mode #327 lands first, this PR makes the same changes in reverse.