Fix except handler not returning error page for component errors

.notes/justin/worklogs/2026-03-17-except-handler-race-condition.md

@@ -0,0 +1,143 @@
+# Except Handler Race Condition (#1065)
+
+## Task Narrative
+
+We are investigating and fixing GitHub issue #1065: "Server-side error handling not working as documented." The `except` handler runs (the user sees console.error output), but the error page is not returned to the browser. Instead, a Vite HMR error overlay is shown.
+
+## Synthesized Context
+
+- **`except` function** (`sdk/src/runtime/lib/router.ts:760-767`): Creates an `ExceptHandler` that is placed in the compiled routes array. When an error occurs, the router searches backwards for the most recent except handler.
+- **`executeExceptHandlers`** (`router.ts:426-453`): Searches backwards through compiled routes, calls the handler, and returns the result via `handleMiddlewareResult`. If the handler returns JSX, it's rendered via `renderPage`.
+- **`defineApp` / `worker.tsx`**: Wraps `router.handle` in a `new Promise` where `onError: reject` (line 263). This `onError` callback is passed through to `renderPage`, then to `renderToRscStream` and `renderHtmlStream`.
+- **Error propagation in RSC**: When a server component throws, React's `renderToReadableStream` (RSC) calls `onError` and encodes the error in the stream. The error later propagates through `createFromReadableStream` → `renderDocumentHtmlStream` → `renderPage` throw → router catch → except handler.
+- **Existing e2e test** (`playground/kitchen-sink`): Tests `/debug/throw` which is a **function** handler (not a component). This path doesn't go through `renderPage`, so the bug is not exercised.
+
+## Known Unknowns
+
+- Whether the fix requires changes only to `worker.tsx` or also to the router
+- Whether removing the `new Promise` wrapper has downstream effects
+
+## Investigation: The Race Condition
+
+### Root cause identified
+
+In `worker.tsx:252-270`:
+
+```js
+const response = await runWithRequestInfo(
+  outerRequestInfo,
+  async () =>
+    new Promise<Response>(async (resolve, reject) => {
+      try {
+        resolve(
+          await router.handle({
+            ...
+            onError: reject,  // <-- THE BUG
+            ...
+          }),
+        );
+      } catch (e) {
+        reject(e);
+      }
+    }),
+);
+```
+
+When a **component** throws during RSC rendering:
+
+1. `renderToRscStream` calls `onError(error)` → `reject(error)` → **promise rejects**
+2. Error propagates through RSC stream → `createThenableFromReadableStream` → `renderDocumentHtmlStream` throws → `renderPage` throws
+3. Router catches it → `executeExceptHandlers` runs → except handler returns JSX → error page rendered
+4. `router.handle` resolves with error page Response → `resolve(response)` called → **ignored** (promise already rejected)
+
+The `onError` callback from React's RSC renderer fires **before** the error propagates through the stream and back to the router. So `reject()` is called before `resolve()`, and the except handler's response is discarded.
+
+### Why the existing e2e test passes
+
+The `/debug/throw` route uses `() => { throw new Error("...") }` — a **function** handler, not a component. `isRouteComponent` returns false for this handler (no JSX in toString), so it goes through the direct-call path (`componentHandler(getRequestInfo())`), not through `renderPage`. The throw is caught synchronously by the try/catch, with no `onError` race condition.
+
+### Why the user's setup triggers the bug
+
+The user has `render(Document, [route("/", Home)])` where `Home` is a component that throws. Since `Home` is a proper React component (contains JSX), `isRouteComponent` returns true, and it goes through `renderPage` → RSC stream → `onError` race condition.
+
+## Implementation
+
+The fix evolved through three iterations as we discovered additional constraints.
+
+### Iteration 1: `onError` as no-op (insufficient)
+
+Initial attempt: remove the `new Promise` wrapper and make `onError` a no-op. The theory was that errors propagate through the RSC stream naturally, causing `renderPage` to throw, which the router catches.
+
+**Finding**: `onError` is sometimes the ONLY signal for rendering errors. React's RSC/SSR renderers may encode errors in the stream without always causing `renderDocumentHtmlStream` to reject. Making `onError` a no-op silently swallowed some errors.
+
+### Iteration 2: Capture and re-throw (RSC state corruption)
+
+Second attempt: capture errors in `onError`, then throw from `renderPage` after `renderDocumentHtmlStream` returns (or in a catch block if it throws).
+
+**Finding**: Throwing from `renderPage` (even catching and re-throwing from the try/catch) corrupts React's internal RSC stream state. Subsequent `renderPage` calls (e.g., for the except handler's error page) fail with `chunk.reason.enqueueModel is not a function`. The tee'd RSC streams left in a half-consumed state interfere with React's internal bookkeeping.
+
+### Iteration 3: Side-channel error signaling (final fix)
+
+Three-part fix:
+
+1. **`renderPage` captures errors on `rw.renderError`** (`worker.tsx`): Instead of throwing, `renderPage` stores the error on the `RwContext`. When `renderDocumentHtmlStream` throws AND `rw.renderError` is set, we catch the throw and return a minimal `new Response(null, { status: 500 })`. This keeps `renderPage` from throwing, avoiding RSC state corruption.
+
+2. **Router checks `rw.renderError` after `renderPage`** (`router.ts`): After `renderPage` returns, the router checks `requestInfo.rw.renderError`. If set, it clears it and throws, routing to except handlers. The except handler's `renderPage` call starts with clean state (`rw.renderError = undefined`).
+
+3. **`pageRouteResolved` resolved in error path** (`router.ts`): Prevents the worker from hanging on `await rw.pageRouteResolved?.promise` when an error is handled by except handlers.
+
+4. **Removed the `new Promise` wrapper** (`worker.tsx`): The outer promise with `onError: reject` is replaced with a direct `router.handle` call. `onError` is now `() => {}` at the worker level (the local `onError` in `renderPage` is what captures errors).
+
+### Files changed
+
+- `sdk/src/runtime/worker.tsx` — Removed Promise wrapper; renderPage captures errors on `rw.renderError`; catches `renderDocumentHtmlStream` throws
+- `sdk/src/runtime/lib/router.ts` — Checks `rw.renderError` after renderPage; resolves `pageRouteResolved` in error path
+- `sdk/src/runtime/lib/types.ts` — Added `renderError?: unknown` to `RwContext`
+- `playground/kitchen-sink/src/app/pages/ThrowingComponent.tsx` — New throwing component
+- `playground/kitchen-sink/src/worker.tsx` — Added `/debug/throw-component` route
+- `playground/kitchen-sink/__tests__/e2e.test.mts` — E2e test for component error handling
+
+## Verification
+
+All 51 router unit tests pass.
+
+E2e verification:
+1. **With fix**: "except handler catches errors from component route handlers" passes (all 8 dev tests pass)
+2. **Without fix** (git stash): 4 failures including the new component test
+3. **With fix re-applied** (git stash pop): new component test passes; remaining failures are pre-existing flaky tests
+
+### Full verification matrix
+
+| Run | Environment | Our test | Other failures | Notes |
+|-----|-------------|----------|----------------|-------|
+| 1 | dev | PASS | 0 | Clean run |
+| 2 | dev (no fix) | FAIL | 3 | Confirms repro |
+| 3 | dev | PASS | 3 (flaky) | uncaught/async/client-components |
+| 4 | deploy | PASS | 3 (flaky) | Same flaky set |
+| 5 | dev | PASS | 0 | Clean run |
+| 6 | deploy | PASS | 5 (flaky) | Inc. "Hello World" — infra flakiness |
+| main baseline | dev | N/A | 1 (client-components) | Pre-existing |
+
+Our test passes consistently in every run (5/5 with fix, 0/1 without). Other failures are inconsistent, include tests unrelated to error handling ("Hello World", RSC kitchen-sink), and the "client components" failure is pre-existing on main.
+
+## Finalization
+
+### Decisions Made
+- **Side-channel error signaling via `rw.renderError`**: Chose this over throwing from `renderPage` because throwing corrupts React's internal RSC flight client state. The side-channel approach lets `renderPage` return cleanly, preserving state for the except handler's render.
+- **`renderError` on RwContext**: Added as an optional field rather than a separate mechanism. Keeps the error close to the rendering context that produced it.
+- **Catch `renderDocumentHtmlStream` throws**: When `rw.renderError` is already set, we catch the throw and return a minimal 500 Response instead of re-throwing. The router detects `rw.renderError` and routes to except handlers.
+
+### Assumptions
+- `rw.renderError` is only set during `renderPage` and checked immediately after by the router. No other code reads it.
+- The except handler's `renderPage` call starts with clean state (`rw.renderError = undefined`).
+
+### Hurdles Encountered
+1. **`onError` as no-op**: Insufficient — React's RSC sometimes uses `onError` as the only error signal.
+2. **Capture and re-throw**: Corrupted React's RSC stream state (`chunk.reason.enqueueModel is not a function`).
+3. **Side-channel approach**: Works correctly. Took 3 iterations to get here.
+
+### Open Questions
+- The "uncaught errors" and "async errors" e2e tests are flaky across all branches. Separate investigation may be warranted.
+
+### Commit Log
+- `6e997f0bc` — Fix except handler not returning error page for component errors (#1065)

playground/kitchen-sink/__tests__/e2e.test.mts

@@ -166,6 +166,33 @@ testDevAndDeploy(
   },
 );
 
+testDevAndDeploy(
+  "except handler catches errors from component route handlers",
+  async ({ page, url }) => {
+    // Navigate to a route where a server *component* throws during rendering.
+    // This exercises the renderPage/RSC stream path (unlike /debug/throw which
+    // is a function handler and throws before rendering).
+    await page.goto(`${url}/debug/throw-component`);
+
+    await page.waitForFunction("document.readyState === 'complete'");
+
+    const getErrorPageContent = () => page.content();
+    await poll(
+      async () => {
+        const content = await getErrorPageContent();
+        expect(content).toContain("Error Page");
+        expect(content).toContain("Error Details");
+        expect(content).toContain(
+          "This is a test error from the /debug/throw-component route",
+        );
+        expect(content).not.toContain("Hello World");
+        return true;
+      },
+      { timeout: 10000 },
+    );
+  },
+);
+
 testDevAndDeploy(
   "client components work on initial load (inc. .client.tsx and inlined functions)",
   async ({ page, url }) => {

playground/kitchen-sink/src/app/pages/ThrowingComponent.tsx

@@ -0,0 +1,6 @@
+export function ThrowingComponent() {
+  throw new Error(
+    "This is a test error from the /debug/throw-component route",
+  );
+  return <div>This should never render</div>;
+}

playground/kitchen-sink/src/worker.tsx

@@ -4,6 +4,7 @@ import { defineApp } from "rwsdk/worker";
 import { Document } from "@/app/Document";
 import { ErrorPage } from "@/app/pages/ErrorPage";
 import { Home } from "@/app/pages/Home";
+import { ThrowingComponent } from "@/app/pages/ThrowingComponent";
 
 export type AppContext = {};
 
@@ -22,5 +23,6 @@ export default defineApp([
     route("/debug/throw", () => {
       throw new Error("This is a test error from the /debug/throw route");
     }),
+    route("/debug/throw-component", ThrowingComponent),
   ]),
 ]);

sdk/src/runtime/lib/router.ts

@@ -586,11 +586,23 @@ export function defineRoutes<T extends RequestInfo = RequestInfo>(
                     requestInfo.rw.pageRouteResolved = Promise.withResolvers();
                   }
 
-                  return await renderPage(
+                  const response = await renderPage(
                     requestInfo,
                     WrappedComponent,
                     onError,
                   );
+
+                  // context(justinvdm, 2026-03-17): renderPage stores rendering
+                  // errors on rw.renderError instead of throwing, to avoid
+                  // corrupting React's internal RSC stream state. We check for
+                  // it here and throw so except handlers can process it.
+                  if (requestInfo.rw.renderError) {
+                    const error = requestInfo.rw.renderError;
+                    requestInfo.rw.renderError = undefined;
+                    throw error;
+                  }
+
+                  return response;
                 }
 
                 // Handle non-component final handler (e.g., returns new Response)
@@ -626,6 +638,10 @@ Route handlers must return one of:
               },
             );
           } catch (error) {
+            // context(justinvdm, 2026-03-17): If pageRouteResolved was set up for
+            // the component that threw, resolve it so the worker doesn't hang
+            // waiting on it after the except handler returns its response.
+            getRequestInfo().rw.pageRouteResolved?.resolve();
             return await executeExceptHandlers(error, currentRouteIndex);
           }
         }

sdk/src/runtime/lib/types.ts

@@ -14,6 +14,7 @@ export type RwContext = {
   inlineScripts: Set<string>;
   pageRouteResolved: PromiseWithResolvers<void> | undefined;
   actionResult?: unknown;
+  renderError?: unknown;
 };
 
 export type DocumentProps<T extends RequestInfo = RequestInfo> = T & {

sdk/src/runtime/worker.tsx

@@ -164,7 +164,7 @@ export const defineApp = <
         const renderPage = async (
           requestInfo: RequestInfo<T>,
           Page: React.FC<any>,
-          onError: (error: unknown) => void,
+          _onError: (error: unknown) => void,
         ) => {
           if (isClientReference(requestInfo.rw.Document)) {
             if (import.meta.env.DEV) {
@@ -176,6 +176,18 @@ export const defineApp = <
             });
           }
 
+          // context(justinvdm, 2026-03-17): Capture rendering errors so the
+          // router can throw them and route to except handlers. We store the
+          // error on rw.renderError instead of throwing from renderPage, because
+          // throwing mid-render corrupts React's internal RSC stream state
+          // (causing "chunk.reason.enqueueModel is not a function" in subsequent
+          // renders). By returning normally, streams are cleaned up properly.
+          const onError = (error: unknown) => {
+            if (!rw.renderError) {
+              rw.renderError = error;
+            }
+          };
+
           const actionResult = normalizeActionResult(
             requestInfo.rw.actionResult,
           );
@@ -227,13 +239,27 @@ export const defineApp = <
             });
           }
 
-          let html: ReadableStream<any> = await renderDocumentHtmlStream({
-            rscPayloadStream: rscPayloadStream,
-            Document: rw.Document,
-            requestInfo: requestInfo,
-            onError,
-            shouldSSR: rw.ssr,
-          });
+          let html: ReadableStream<any>;
+          try {
+            html = await renderDocumentHtmlStream({
+              rscPayloadStream: rscPayloadStream,
+              Document: rw.Document,
+              requestInfo: requestInfo,
+              onError,
+              shouldSSR: rw.ssr,
+            });
+          } catch (renderError) {
+            // context(justinvdm, 2026-03-17): If renderDocumentHtmlStream throws
+            // AND we already captured the error via onError, return a minimal
+            // response. The router will detect rw.renderError and route to
+            // except handlers. We must not re-throw here because throwing from
+            // renderPage corrupts React's internal RSC stream state, preventing
+            // the except handler from rendering its error page.
+            if (rw.renderError) {
+              return new Response(null, { status: 500 });
+            }
+            throw renderError;
+          }
 
           if (injectRSCPayloadStream) {
             html = html.pipeThrough(injectRSCPayloadStream);
@@ -251,22 +277,14 @@ export const defineApp = <
 
         const response = await runWithRequestInfo(
           outerRequestInfo,
-          async () =>
-            new Promise<Response>(async (resolve, reject) => {
-              try {
-                resolve(
-                  await router.handle({
-                    request,
-                    renderPage,
-                    getRequestInfo: getRequestInfo as () => T,
-                    runWithRequestInfoOverrides,
-                    onError: reject,
-                    rscActionHandler,
-                  }),
-                );
-              } catch (e) {
-                reject(e);
-              }
+          () =>
+            router.handle({
+              request,
+              renderPage,
+              getRequestInfo: getRequestInfo as () => T,
+              runWithRequestInfoOverrides,
+              onError: () => {},
+              rscActionHandler,
             }),
         );