Skip to content

CS-11024: Defer prerender error status to afterRender#4642

Merged
habdelra merged 4 commits intomainfrom
cs-11024-defer-prerender-status-error-to-afterrender
May 5, 2026
Merged

CS-11024: Defer prerender error status to afterRender#4642
habdelra merged 4 commits intomainfrom
cs-11024-defer-prerender-status-error-to-afterrender

Conversation

@habdelra
Copy link
Copy Markdown
Contributor

@habdelra habdelra commented May 4, 2026

Why this matters

A user-visible "indexing error" surface that hides the real underlying card error has been recurring across multiple realms in production. Every occurrence is a confused user and a noisy support thread; the actual card error (most often a wrong-subpath import or similar module-eval throw) never reaches them.

Repros deterministically on staging for buck/buck/SprintTask/7d92636a-…json across 4+ separate from-scratch attempts on different days, plus across multiple realms with the same pattern (bn3/bn, lukemelia/testing-2, etc.). All of them surface as the literal worker log render.html returned an invalid error payload: with empty value after the colon.

Mechanism

The prerender server's capture wait condition (packages/realm-server/prerender/utils.ts:765-779) treats two signals as equivalent — wait is satisfied when data-prerender-status ∈ {ready, error, unusable} OR the <pre data-prerender-error> has non-empty text:

let status = element.dataset.prerenderStatus ?? '';
const errorHasText = errorText.length > 0;
const hasError = errorElement !== null && errorHasText;
if (!statuses.includes(status) && !hasError) { continue; }   // keep waiting

The host is supposed to use data-prerender-status on the outer [data-prerender] container as the canonical "DOM is settled, snapshot now" signal. But in #processRenderError (packages/host/app/routes/render.ts), the old #applyErrorMetadata wrote prerenderStatus='error' synchronously, before #transitionToErrorRoute triggered the Ember transition that renders the render.error template. Until the template renders, the inner <pre data-prerender-error> is either absent or has stale/empty content.

Race window:

  1. data-prerender-status='error' written ← prerender wait condition is satisfied here
  2. Server poll fires, takes capture against the still-empty <pre data-prerender-error>
  3. Ember transitions to render.error
  4. Template renders {{this.reason}} into the <pre>

When the server polls between (1) and (4), the captured value is empty. JSON.parse('') fails, and renderCaptureToError synthesizes "render.html returned an invalid error payload: ". The actual card error never reaches the user.

The intent — and the contract this PR preserves — is that data-prerender-status='error' means the DOM is fully settled in an error state, not the route has decided to enter an error state. Today's code raised the signal at decision time rather than at settlement time.

Fix

Defer data-prerender-status='error' to fire after the render.error template has rendered, via Ember's afterRender queue. The id/nonce datasets stay where they are — those are immutable for this render and the server doesn't gate on them. Only the status flip moves.

  • Split #applyErrorMetadata in packages/host/app/routes/render.ts into:
    • #applyErrorMetadataAttrs(context) — sets data-prerender-id and data-prerender-nonce on [data-prerender] and [data-prerender-error] synchronously. The server uses these for id/nonce parity checks; they're immutable for this render.
    • #applyErrorStatus(context) — sets data-prerender-status='error' on the outer container.
  • #processRenderError now calls #applyErrorMetadataAttrs synchronously, then #transitionToErrorRoute(transition), then schedule('afterRender', this, this.#applyErrorStatus, context). Glimmer flushes the transitioned-to template's DOM updates before afterRender fires, so by the time we lift the readiness signal the <pre>'s textContent is populated.

Acceptance criteria (from CS-11024)

  • ✅ After the fix, data-prerender-status='error' is set only after [data-prerender-error]'s textContent.trim().length > 0.
  • data-prerender-id / data-prerender-nonce are still set synchronously, so id/nonce-based wait-condition checks continue to work.
  • ✅ Window-error path (Path A) is untouched — it already orders the setError (textContent write) before the status flip via setStatusToUnusable in the wrapping handler.
  • ✅ The buck/buck/SprintTask/7d92636a-…json indexing run will no longer log render.html returned an invalid error payload:. Worker error will become the actual underlying error (e.g. the proxy ReferenceError for the wrong-subpath import).

Out of scope

  • Path A (window-error): already correct; orders textContent before status.
  • Improving the strict-namespace proxy's error message to suggest the correct subpath. Tracked in CS-11023; pairs with this ticket so the actionable error reaches the user and the message itself is more useful.
  • Worker-process crash investigation: separate concern, downstream of this fix — once the capture surfaces the real error reliably we can correlate which error classes correlate with worker exits.
  • Adding a non-DOM side-channel for the error payload: the DOM is the single source of truth; the bug is in when we signal readiness, not in the surface.

Test

Adds a realm-server prerendering regression test at packages/realm-server/tests/prerendering-test.ts that drives a card whose module throws synchronously during evaluation (eval-throw.gtsthrow new Error('module-eval-throw') at top level). Asserts:

  • captured error message contains the actual underlying throw (module-eval-throw),
  • captured error is not the synthesized "returned an invalid error payload" fallback,
  • the failure is not classified as a timeout.

The test exercises the route-error path: module evaluation rejects → model() rejects → route's error action fires with the active transition → #processRenderError lifts status='error' → server captures and parses.

Test plan

  • pnpm lint (host + realm-server)
  • Realm-server prerendering test runs locally
  • CI realm-server tests shard passes
  • No regression on existing route-error tests (card prerender surfaces errors thrown before the render model hook, card prerender hoists module transpile errors, card prerender surfaces actionable error for bad icon import, etc.)

🤖 Generated with Claude Code

The prerender server treats `data-prerender-status='error'` on the
`[data-prerender]` container as the canonical "DOM is settled, snapshot
now" signal. The route-error path was lifting it synchronously before
Ember had transitioned to `render.error` and Glimmer had populated the
`<pre data-prerender-error>`, so the server could poll between the two
writes and capture an empty payload — surfacing the synthesized "invalid
error payload" string instead of the actual underlying card error.

Split `#applyErrorMetadata` into `#applyErrorMetadataAttrs` (id + nonce,
still synchronous) and `#applyErrorStatus` (status flip), and schedule
the status flip on the `afterRender` queue from `#processRenderError`.
The id/nonce dataset writes that the server uses to correlate captures
keep their existing timing.

Adds a realm-server prerendering regression test that drives a card
whose module throws synchronously during evaluation and asserts the
surfaced error contains the underlying throw, not the "invalid error
payload" fallback.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 867430698c

ℹ️ 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".

Comment thread packages/host/app/routes/render.ts
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 4, 2026

Preview deployments

Host Test Results

    1 files  ±0      1 suites  ±0   1h 56m 55s ⏱️ - 2m 39s
2 563 tests +1  2 548 ✅ +1  15 💤 ±0  0 ❌ ±0 
2 582 runs  +1  2 567 ✅ +1  15 💤 ±0  0 ❌ ±0 

Results for commit 27375b8. ± Comparison against earlier commit 74b6e4a.

Realm Server Test Results

    1 files  ± 0      1 suites  ±0   18m 4s ⏱️ +55s
1 235 tests +22  1 235 ✅ +23  0 💤 ±0  0 ❌  - 1 
1 307 runs  +22  1 307 ✅ +23  0 💤 ±0  0 ❌  - 1 

Results for commit 27375b8. ± Comparison against earlier commit 74b6e4a.

Codex review caught a downgrade bug: #transitionToErrorRoute's
early-failure fallback (when renderBaseParams isn't set yet) writes
data-prerender-status='unusable' to force page eviction —
renderCaptureToError keys eviction on the 'unusable' literal. The
afterRender callback was unconditionally overwriting it to 'error',
which meant pages that should have been discarded could be kept and
reused.

Skip the 'error' write when the current status is already 'unusable'.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a prerendering race in the Host app’s render route error handling by deferring the data-prerender-status="error" readiness signal until Ember’s afterRender, ensuring the render.error template has populated the <pre data-prerender-error> payload before the prerender server snapshots it.

Changes:

  • Split error metadata writes so data-prerender-id / data-prerender-nonce are applied synchronously, while data-prerender-status="error" is scheduled on afterRender.
  • Add a realm-server prerender regression test that exercises a module top-level evaluation throw and asserts the captured error surfaces the underlying throw instead of the synthesized “invalid error payload” fallback.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
packages/host/app/routes/render.ts Defers raising the prerender “error settled” signal to afterRender to avoid capturing an empty error payload.
packages/realm-server/tests/prerendering-test.ts Adds a regression test covering module-evaluation throws to validate the corrected prerender capture ordering.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread packages/host/app/routes/render.ts Outdated
habdelra and others added 2 commits May 4, 2026 14:58
CI's existing 'errors thrown before the render model hook' test
showed the unusable→error downgrade Codex/Copilot flagged is in fact
reachable: when beforeModel() throws the runloop is still alive enough
for afterRender to fire, so #applyErrorStatus runs after
#transitionToErrorRoute's early-failure fallback synchronously wrote
data-prerender-status='unusable', overwriting it to 'error' and
breaking page eviction.

Skip the schedule when renderBaseParams isn't set (the precondition
the early-failure fallback gates on). On that path the fallback
already writes status='unusable' and the error textContent
synchronously, so there's nothing to defer.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@habdelra habdelra requested a review from a team May 4, 2026 23:00
@habdelra habdelra merged commit 1743863 into main May 5, 2026
74 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants