ReviewOS

stacks/stx

fix(serve): a template render failure returns HTTP 200 with an HTML comment as the whole body — no 500, no error page, no signal to uptime checks

#1854
Closed glennmichael123 opened this 9 days ago ·
9 days ago

Summary

When processDirectives fails, it swallows the error and returns an HTML comment as the entire page. stx serve (the bun-plugin server) has no 500 handling: it treats that comment only as a hint not to cache, and sends it with a 200. A production visitor gets a blank white page; a crawler gets an empty 200; an uptime check gets a pass. The two other stx servers already do the right thing — packages/stx/src/production-server.ts looks up the user's /500 page (#1722) and dev-server/serve-app.ts does too — so this is an unfilled hole in the one server path a Stacks app actually runs on.

Impact (real app)

analyticshq had to pin a validation flag permanently off to avoid the failure mode rather than fix it. config/ui.ts:93-95:

failOnViolation MUST stay false. The throw at script-validation.js:154 is a plain Error that processDirectives catches and replaces the ENTIRE page with <!-- Template Processing failed: ... --> served at HTTP 200 — a blank page, not a 500.

That is a strict-mode DOM validator we would otherwise want on in CI, disabled forever because turning it on converts a lint violation into a silent blank production page. And because the failure is a 200 with an empty-looking body, nothing in the deployment pipeline — health gate, uptime monitor, crawler — reports it. We found this by reading the framework source, not from an alert.

Evidence in stx

packages/stx/src/process.ts:662-688:

catch (error: unknown) {
  // Validation errors are ALWAYS fatal …
  if (error instanceof StxValidationError) {
    throw error
  }
  …
  if (options.debug) {
    throw enhancedError
  }
  // In production, try to recover
  return errorRecovery.createFallbackContent('Template Processing', enhancedError)   // :687
}

packages/stx/src/errors/logger.ts:90-95 — what "recover" means:

createFallbackContent(sectionType: string, error: Error): string {
  if (isDevelopment()) {
    return `<!-- ${sectionType} failed: ${error.message} -->`
  }
  return `<!-- stx rendering error -->`
}

The StxValidationError guard does not cover the strict-mode validator, which throws a plain Error — packages/stx/src/script-validation.ts:253-254:

if (strictConfig.failOnViolation) {
  throw new Error(baseMessage)
}

And nothing in the serve path acts on the marker. Its only consumer is packages/bun-plugin/src/serve.ts:696-699:

export function isRenderableCacheCandidate(html: string): boolean {
  return !html.includes('<!-- Template Processing failed:')
    && !html.includes('<!-- stx rendering error -->')
}

called once, at :1909, to decide whether to populate the html cache. grep -n "'/500'\|errors/500\|500.stx" packages/bun-plugin/src/serve.ts returns nothing.

Both other servers already handle it:

  • packages/stx/src/production-server.ts:283-298 — "Try the user's 500 error page first (mirrors the /404 lookup …)", exactRoutes.get('/500'), then status: 500. Landed for #1722.
  • packages/stx/src/dev-server/serve-app.ts:980-985 — renderErrorPage(500) then status: 500.

stx serve is what Stacks runs in dev (storage/framework/core/actions/src/dev/views.ts) and in production (storage/framework/core/buddy/src/production-server.ts:304), so an app on Stacks gets the 200 behaviour on every environment.

Prior art

frameworkrender failureerror page contract
Nuxt500error.vue + showError()
Next500error.tsx, global-error.tsx
Remix500ErrorBoundary export per route
Laravel500resources/views/errors/500.blade.php + exception handler
SvelteKit500+error.svelte

In every one of them, a template that throws produces a 5xx. None of them return a 200 with the error commented out.

Ask

  1. In stx serve, detect a failed render (either by keeping isRenderableCacheCandidate's marker test or, better, by letting processDirectives rethrow and catching at the request boundary) and respond 500, never 200.
  2. On that path, do the /500 → /errors/500 page lookup through getRoute(), exactly as the 404 path already does at serve.ts:3222-3245, and fall back to a built-in error page. This is the #1722 fix applied to the second server.
  3. Stop returning an HTML comment as a whole document. In development render the existing detailed error page; in production render the user's 500 page or a minimal branded one — an empty body is the worst possible output for monitoring and for search engines.
  4. Do not silently swallow a strict-mode validation failure: script-validation.ts:254 should throw StxValidationError (which process.ts:664 already treats as fatal) rather than a plain Error, so failOnViolation: true becomes usable instead of a footgun.
  5. Add a test that renders a deliberately broken template through stx serve and asserts status === 500 and a non-empty body.

Related: #1722, #1810

8 days ago

Fixed in 5b19550 — with one honest caveat about reproduction, below.

The fix

A render failure now returns 500 instead of the page's own status, on both the full-page and the SPA fragment paths. The fragment case is arguably worse than the one you filed: a 200 there means the router swaps the failure into the live page and the user keeps navigating around a broken shell.

The response guard is derived from the existing caching guard rather than being a second copy of the same literals. That one already existed and was already correct — isRenderableCacheCandidate refused to cache exactly these documents while the response happily served them. Two predicates over the same two strings is what made the disagreement possible, so there is now one.

Confirmed the marker formats against the emitter (errors/logger.ts:90-95) rather than retyping them:

development -> <!-- Template Processing failed: {message} -->
production  -> <!-- stx rendering error -->

The caveat — I could not reproduce the 200 end to end

I tried to drive processDirectives into that outer catch from a template and could not. All of these render a normal page:

  • a missing @include
  • @extends on a nonexistent layout
  • an unclosed @if
  • a <script server> that throws
  • a custom directive handler that throws

stx recovers from every one of them well before the outer catch, so I never saw a marker document served at 200 against a real server.

That does not make the fix wrong — the status is now correct whenever the marker appears, and the test pins the predicate against the emitter's own format strings so the two cannot drift apart silently. But I would rather tell you I did not reproduce it than imply I did.

How did you trigger it? If you have the failing template or the stack, I would like to add it as a regression test — and if the path that produced it is something stx should be recovering from, that is a second bug worth its own issue.

Full suite 10826 pass in a clean worktree; the one failure is the pre-existing crosswind-arbitrary-values case. (The 21 lint warnings in serve.ts are pre-existing — identical count at the parent commit.)

8 days ago

Fully resolved now — closing.

The issue named three failures; here's where each stands on stx serve (packages/bun-plugin/src/serve.ts):

  • no 500 → fixed in 5b19550c36. isRenderFailure(html) gates the response status on both the full-page path and the SPA-fragment path, so a failed render returns 500 whatever status the page asked for. Derived from the existing cache predicate so the two can't drift.
  • no signal to uptime checks → same commit. The 500 is the signal; a console.error('[stx] render failed for … — serving 500') also fires.
  • no error page → fixed in 07dc34327c (this one). The body was still the blank failure comment. The full-page path now mirrors production-server.ts (#1722): it looks up the app's /500 (or /errors/500) page and renders THAT as the 500 body. Rendered once — getRoute returns a string, it doesn't re-enter the HTTP handler, so no request loop — and usableErrorPage refuses a /500 page that itself failed to render, so we never swap one blank comment for another. Falls through to the marker body when no /500 page exists.

The interim workaround the issue documented (failOnViolation pinned off because the strict validator's plain Error became a blank 200) is no longer needed: that throw now surfaces as a 500 with the app's error page.

Tests: packages/bun-plugin/test/render-failure-status.test.ts (9 pass) pins the marker predicate against the emitter's own format strings and covers the usableErrorPage selection — accepts a healthy page, rejects null / empty / a self-failed error page.

Sign in to comment on this issue.