From 54f6e69b9c8b936c2fa9ef39aa624e95ce638967 Mon Sep 17 00:00:00 2001 From: Buck Doyle Date: Thu, 10 Sep 2026 11:40:28 -0400 Subject: [PATCH 1/3] Withhold a render failure the environment caused, at the row that publishes it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The indexer stores a failed render as the card's content, so a render that resolves current realm source against a bundle predating an export turns a few minutes of deploy overlap into an error document served from cache to every anonymous reader until something reindexes the row. The decision has to be made where the row is written. A prerender server can delay a write but never prevent one — answering retryably only spends the client's retry budget and then the same row lands with a different message, and the manager prunes a server that answers 5xx, walking its registry as it goes. So the server states what it knows and the write site acts on it. The server is the only place holding both tokens at the moment of the render, so it reaches the conclusion: a missing export that survived a re-render on a pool that still cannot be shown to have been on the shell being served describes the environment, not the card. It marks the response and returns the failure unchanged. The write site tests for the mark's presence and nothing else. It does not re-derive the conclusion, so the rule has one implementation rather than two that drift, and absence means "no verdict" rather than "attributable" — nothing is withheld by default. When the mark is present and a prior published row exists, the row keeps the content the last good pass left: `pristine_doc` and its neighbours were already carried forward there, and this only declines to stamp `has_error` and `error_doc` over them. The published `search_doc` stays too, since the error's sparse one describes a render whose result is not being published. Two gates keep the suppression from hiding a real break. It requires a prior published row, because a brand-new card has no good content to protect and withholding its error would leave nothing at all. And it requires the missing export to *be* the failure rather than merely appear among the console errors merged onto it — the broader test is right for driving one more render, where being wrong costs a render, and wrong for withholding a row, where being wrong hides a genuine break. Tests state both directions, since the difference is the whole point: a pool that never reaches the current shell is marked, a genuine break on a recycled pool is not, and a timeout that merely mentions a missing export is not. Substituting the broad predicate for the narrow one fails that last case and only that case — the first draft of these tests did not distinguish them, which is why it is there. --- .../realm-server/prerender/prerender-app.ts | 70 ++++++++++ .../prerender-host-shell-recycle-test.ts | 28 ++++ .../tests/prerender-server-test.ts | 131 ++++++++++++++++++ packages/runtime-common/index-writer.ts | 54 +++++++- packages/runtime-common/index.ts | 13 ++ 5 files changed, 291 insertions(+), 5 deletions(-) diff --git a/packages/realm-server/prerender/prerender-app.ts b/packages/realm-server/prerender/prerender-app.ts index 2bc829cafbb..77a8197ebfd 100644 --- a/packages/realm-server/prerender/prerender-app.ts +++ b/packages/realm-server/prerender/prerender-app.ts @@ -187,6 +187,29 @@ export function shouldRerenderForStaleShell({ ); } +// Whether the missing export *is* the failure, rather than merely present +// among the console errors `RenderRunner` merged onto it. +// +// The broader test drives the re-render, where being wrong costs one extra +// render. This one gates suppressing a row, where being wrong hides a genuine +// break — so a render whose own failure was a timeout or a wedge, and whose +// console happened to carry a missing-export line, is not grounds to withhold +// the error it actually produced. +function missingExportIsTheFailure(response: RenderVisitResponse): boolean { + for (let candidate of [ + response.card?.error, + response.fileExtract?.error, + response.fileRender?.error, + response.pageUnusableError, + ]) { + let message = candidate?.error?.message; + if (typeof message === 'string' && isMissingExportMessage(message)) { + return true; + } + } + return false; +} + function hasMissingExportError(response: RenderVisitResponse): boolean { // Every sub-response that can carry a render failure, because every one of // them is persisted the same way: `prerender-html-visit` writes @@ -276,6 +299,23 @@ export function stampHostShellTokens( }; } +// Record that this failure is the environment's rather than the card's, so the +// write site can decline to publish it as the card's content. +// +// Stated by this server because it is the only place holding both the reported +// and warmed tokens at the moment of the render. The write site checks for the +// field's presence and nothing else — it does not re-derive the conclusion, so +// there is one implementation of the rule rather than two that can drift. +export function stampStaleShellFailure(response: RenderVisitResponse): void { + response.meta = { + ...(response.meta ?? {}), + diagnostics: { + ...(response.meta?.diagnostics ?? {}), + staleShellFailure: true, + }, + }; +} + // A one-shot notification that shutdown has begun, which the holder releases // when it no longer needs it. type DrainSubscription = { @@ -1321,6 +1361,36 @@ export function buildPrerenderApp(options: { discardedMs, shellAtStart, ); + // The re-render failed the same way and the pool still cannot be shown + // to have been on the shell being served, so this failure describes the + // environment and not the card. Say so on the response and return it + // unchanged: the write site declines to publish it as the card's + // content, which is the only place that decision can be made — a + // prerender server can delay a write but never prevent one. + // + // Narrower than the re-render's own test on purpose. That one accepts a + // missing export anywhere in the error, including the console errors + // merged onto an unrelated timeout, because being wrong there costs one + // render. Withholding a row wants the missing export to be the failure + // itself. + if ( + missingExportIsTheFailure(response) && + shouldRerenderForStaleShell({ + response, + warmedAtStart, + warmedAtCompletion, + reportedAtCompletion: shellAtCompletion, + }) + ) { + log.warn( + 'visit of %s failed to resolve a module on a pool warmed against %s -> %s while the current host shell is %s, after a re-render; marking the failure unattributable to the card', + url, + warmedAtStart ?? 'none', + warmedAtCompletion ?? 'none', + shellAtCompletion, + ); + stampStaleShellFailure(response); + } } let totalMs = Date.now() - start; let poolFlags = Object.entries({ diff --git a/packages/realm-server/tests/prerender-host-shell-recycle-test.ts b/packages/realm-server/tests/prerender-host-shell-recycle-test.ts index ac040fa6071..bef499c7a58 100644 --- a/packages/realm-server/tests/prerender-host-shell-recycle-test.ts +++ b/packages/realm-server/tests/prerender-host-shell-recycle-test.ts @@ -9,6 +9,7 @@ import { raceAgainstDrain, shouldRerenderForStaleShell, stampHostShellTokens, + stampStaleShellFailure, } from '../prerender/prerender-app.ts'; // Unit tests for the host-shell recycle decision a prerender server makes on @@ -390,6 +391,33 @@ module(basename(import.meta.filename), function () { }); }); + module('stampStaleShellFailure', function () { + // The write site tests only for this field's presence, so its encoding is + // the whole contract: absence has to mean "no verdict" rather than + // "attributable", or a response from anything that does not stamp it would + // read as a licence to withhold a row. + test('marks the failure and leaves the rest of diagnostics alone', function (assert) { + let response = { + meta: { requestId: 'abc', diagnostics: { renderMs: 12 } }, + } as unknown as RenderVisitResponse; + stampStaleShellFailure(response); + assert.deepEqual(response.meta, { + requestId: 'abc', + diagnostics: { renderMs: 12, staleShellFailure: true }, + } as unknown as typeof response.meta); + }); + + test('an unmarked response carries no verdict at all', function (assert) { + let response = { + meta: { diagnostics: { renderMs: 12 } }, + } as unknown as RenderVisitResponse; + assert.false( + 'staleShellFailure' in ((response.meta as any).diagnostics ?? {}), + 'absent rather than false — a reader must require presence', + ); + }); + }); + module('raceAgainstDrain', function () { // Stands in for the server's drain subscription, counting how many are // outstanding. The count is the whole point: a subscription that survives diff --git a/packages/realm-server/tests/prerender-server-test.ts b/packages/realm-server/tests/prerender-server-test.ts index f60e1ab59c7..6386d3f997f 100644 --- a/packages/realm-server/tests/prerender-server-test.ts +++ b/packages/realm-server/tests/prerender-server-test.ts @@ -1028,6 +1028,25 @@ module(basename(import.meta.filename), function () { }; } + // A render that failed for its own reasons and merely *mentions* a + // missing export among the console errors `RenderRunner` merged onto it. + function timeoutCarryingModuleError() { + return { + response: { + card: { + error: { + error: { + message: 'Render timed out after 30000ms', + additionalErrors: [{ message: MISSING_EXPORT }], + }, + }, + }, + }, + timings: timings(), + pool: poolMeta(), + }; + } + function rendered() { return { response: { card: { isolatedHTML: '
fresh
' } }, @@ -1247,6 +1266,118 @@ module(basename(import.meta.filename), function () { await built.prerenderer.stop(); }); + // The verdict the write site acts on, in both directions. Suppressing a + // row is only safe if the mark is absent whenever the failure might be + // the card's — so these two cases differ in nothing but whether the pool + // ever reached the shell being served. + test('a pool that never reaches the current shell marks the failure unattributable', async function (assert) { + let built = buildPrerenderApp({ + serverURL: 'http://127.0.0.1:4222', + getHostShellHash: () => 'b778fe76', + getWarmedHostShellHash: () => 'babf3612', + awaitHostShellRecycle: () => Promise.resolve(), + }); + let request: SuperTest = supertest(built.app.callback()); + + let calls = 0; + (built.prerenderer as any).prerenderVisit = async () => { + calls++; + return moduleFailure(); + }; + + let res = await visitRequest( + request, + `${realmURL.href}pool-never-current`, + authFor(), + ); + assert.strictEqual(calls, 2, 'one re-render, and no more'); + assert.strictEqual(res.status, 201, 'the failure is still returned'); + assert.true( + res.body.data.attributes.meta.diagnostics.staleShellFailure, + 'marked, so the write site can decline to publish it as content', + ); + }); + + test('a genuine break on a current pool is not marked', async function (assert) { + // The pool is behind for the first render and current after the + // recycle, so the retry runs on the shell being served and its failure + // is the card's. Nothing here may be withheld. + let warmed = 'babf3612'; + let built = buildPrerenderApp({ + serverURL: 'http://127.0.0.1:4222', + getHostShellHash: () => 'b778fe76', + getWarmedHostShellHash: () => warmed, + awaitHostShellRecycle: async () => { + warmed = 'b778fe76'; + }, + }); + let request: SuperTest = supertest(built.app.callback()); + + let calls = 0; + (built.prerenderer as any).prerenderVisit = async () => { + calls++; + return moduleFailure(); + }; + + let res = await visitRequest( + request, + `${realmURL.href}genuinely-broken-import`, + authFor(), + ); + assert.strictEqual(calls, 2, 'the re-render happened'); + assert.strictEqual(res.status, 201); + assert.notOk( + res.body.data.attributes.meta.diagnostics.staleShellFailure, + 'unmarked, so the error persists and the break stays visible', + ); + assert.strictEqual( + res.body.data.attributes.card.error.error.message, + MISSING_EXPORT, + "and it is the card's own failure that is returned", + ); + }); + + // The narrowing that separates driving a re-render from withholding a + // row. A timeout whose console happens to carry a missing-export line is + // worth one more render — the broad test allows that — but it is not + // grounds to withhold the timeout the render actually produced. Using the + // broad predicate for the mark makes this fail. + test('a failure that merely mentions a missing export is not marked', async function (assert) { + let built = buildPrerenderApp({ + serverURL: 'http://127.0.0.1:4222', + getHostShellHash: () => 'b778fe76', + getWarmedHostShellHash: () => 'babf3612', + awaitHostShellRecycle: () => Promise.resolve(), + }); + let request: SuperTest = supertest(built.app.callback()); + + let calls = 0; + (built.prerenderer as any).prerenderVisit = async () => { + calls++; + return timeoutCarryingModuleError(); + }; + + let res = await visitRequest( + request, + `${realmURL.href}timed-out-render`, + authFor(), + ); + assert.strictEqual( + calls, + 2, + 'the broad test still drove a re-render, which is cheap and fine', + ); + assert.notOk( + res.body.data.attributes.meta.diagnostics.staleShellFailure, + 'but the timeout is not withheld: the missing export was not the failure', + ); + assert.strictEqual( + res.body.data.attributes.card.error.error.message, + 'Render timed out after 30000ms', + 'and the real failure is what reaches the caller', + ); + }); + test('a rejecting re-render answers 500 so the visit is retried elsewhere', async function (assert) { let built = buildPrerenderApp({ serverURL: 'http://127.0.0.1:4222', diff --git a/packages/runtime-common/index-writer.ts b/packages/runtime-common/index-writer.ts index 6470da105e5..aa10271eaa4 100644 --- a/packages/runtime-common/index-writer.ts +++ b/packages/runtime-common/index-writer.ts @@ -1284,6 +1284,25 @@ export class Batch { let production: Record = (await this.getProductionVersion(url, baseTypeFromError(entry))) ?? {}; + // A failure the prerender server marked unattributable to the card is + // not published as the card's content, provided there is content to + // keep: the carried-forward `pristine_doc` and friends stay as the last + // good pass left them and `has_error` stays false, so readers keep + // seeing the card until a render that *can* be attributed replaces it. + // + // Presence of the mark is the whole test. The conclusion belongs to the + // prerender server, which is the only place holding the tokens it rests + // on, so this site does not re-derive it — one implementation of the + // rule rather than two that can drift. Absence means no verdict, never + // "attributable", so nothing is withheld by default. + // + // Gated on a prior published row. A brand-new card has no good content + // to protect, and withholding its error would leave nothing at all — + // the failure has to surface somewhere, and an error row is the right + // output there even when the environment caused it. + let withholdFailure = Boolean( + diagnostics?.staleShellFailure && production.pristine_doc, + ); entryPayload = { types: entry.types, // favor the last known good types over the types derived from the error state @@ -1294,9 +1313,16 @@ export class Batch { // the current searchData onto that doc keeps an instance's rich fields // when it degrades to a sparse error searchData, while a file / // dependency-error row (full searchData) wins outright. - search_doc: entry.searchData - ? { ...(production.search_doc ?? {}), ...entry.searchData } - : (production.search_doc ?? null), + // + // A withheld failure keeps the published doc untouched instead: the + // error's sparse searchData describes a render whose result is not + // being published, so overlaying it would degrade a row that is + // otherwise staying exactly as the last good pass left it. + search_doc: withholdFailure + ? (production.search_doc ?? null) + : entry.searchData + ? { ...(production.search_doc ?? {}), ...entry.searchData } + : (production.search_doc ?? null), // preserve last_known_good_deps through error cycles (may have been cleared // by getProductionVersion if it returned undefined, so we explicitly preserve it) last_known_good_deps: await this.getLastKnownGoodDeps( @@ -1304,8 +1330,26 @@ export class Batch { baseTypeFromError(entry), ), type: baseTypeFromError(entry), - error_doc: errorEntry?.error ?? entry.error, - has_error: true, + // A failure the prerender server marked unattributable to the card + // is not published as the card's content, provided there is content + // to keep. The row's carried-forward `pristine_doc` and friends stay + // exactly as the last good pass left them, and `has_error` is left + // false, so readers keep seeing the card until a render that can be + // attributed replaces it. + // + // Presence of the mark is the whole test — the conclusion is the + // prerender server's, which is the only place holding the tokens it + // rests on. Absence means no verdict, never "attributable", so + // nothing is withheld by default. + // + // Gated on there being a prior published row. A brand-new card has no + // good content to protect, and withholding its error would leave + // nothing at all: the failure has to surface somewhere, and an error + // row is the right output there even if the environment caused it. + error_doc: withholdFailure + ? null + : (errorEntry?.error ?? entry.error), + has_error: !withholdFailure, diagnostics: diagnostics, }; break; diff --git a/packages/runtime-common/index.ts b/packages/runtime-common/index.ts index abedc36d5ca..ad8d93eb0d3 100644 --- a/packages/runtime-common/index.ts +++ b/packages/runtime-common/index.ts @@ -802,6 +802,19 @@ export interface Diagnostics // card that is genuinely broken, so a reader must require presence. warmedHostShellHash?: string | null; warmedHostShellHashAtCompletion?: string | null; + // Set only when the prerender server has concluded that a module-resolution + // failure cannot be attributed to the card: the render failed on a missing + // export, a re-render on a recycled pool failed the same way, and the pool + // still could not be shown to have been on the shell being served. The + // server holding both tokens is the one that can decide this, so it states + // the conclusion rather than leaving every reader to re-derive it from the + // four tokens above — a predicate duplicated at the write site would be a + // second place for the presence rule to be got wrong. + // + // Absent means no verdict, never "attributable". A reader must require + // presence before suppressing anything, for the same reason the warmed + // tokens distinguish `null` from absence. + staleShellFailure?: true; // A row is produced by two prerender visits (index + prerender-html), // each its own HTTP request. `requestId` always carries the index visit's // id and this always carries the prerender-html visit's, whichever table From 31db0f615e63f659adf1420dc61479d7316ae0b5 Mon Sep 17 00:00:00 2001 From: Buck Doyle Date: Thu, 10 Sep 2026 17:13:23 -0400 Subject: [PATCH 2/3] Withhold on both channels, and only for the rows that earned it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two corrections, one of which meant the withholding did nothing in production. Suppressing `boxel_index.has_error` is not enough to stop the row reading as errored. `effectiveHasError()` is `COALESCE(i.has_error, FALSE) OR (ph.error_doc IS NOT NULL AND ph.generation >= i.generation)`, and `effectiveErrorDoc()` serves `ph.error_doc` in that case — so a current error on the prerendered-HTML channel publishes the failure whatever the index channel says. Postgres uses the split channel by default, so the previous version withheld nothing where it mattered. `writePrerenderedHtmlRow` now applies the same gate, keeping the preserved render instead of stamping an error over it, and gated on there being a preserved render for the same reason the other channel gates on `pristine_doc`. The verdict also has to say which rows it covers. One visit produces the instance and file rows independently, and they fail independently: a card render can hit the stale bundle while the file extraction beside it fails for a reason of its own. A response-level flag withheld both, hiding that second, genuine failure. So the mark is now the list of row types whose own failure was the missing export, and each write site requires its own type to be named. An empty list is not written at all, because absence has to keep meaning "no verdict" rather than becoming an empty array a reader might mis-test. A page that never became usable is the one case that covers both rows: neither render happened, so the failure belongs to both rather than to one. Tests follow the scoping. The route case that matters pairs a stale-bundle card failure with an unrelated file-extract failure and requires the verdict to name only `instance` — the shape that would have over-suppressed before. --- .../realm-server/prerender/prerender-app.ts | 65 +++++++++++++------ .../prerender-host-shell-recycle-test.ts | 21 +++++- .../tests/prerender-server-test.ts | 42 +++++++++++- packages/runtime-common/index-writer.ts | 48 ++++++++++++-- packages/runtime-common/index.ts | 10 ++- 5 files changed, 155 insertions(+), 31 deletions(-) diff --git a/packages/realm-server/prerender/prerender-app.ts b/packages/realm-server/prerender/prerender-app.ts index 77a8197ebfd..69d1b3f26b5 100644 --- a/packages/realm-server/prerender/prerender-app.ts +++ b/packages/realm-server/prerender/prerender-app.ts @@ -187,27 +187,42 @@ export function shouldRerenderForStaleShell({ ); } -// Whether the missing export *is* the failure, rather than merely present -// among the console errors `RenderRunner` merged onto it. +// The row types whose *own* failure is a missing export, rather than types +// that merely carry one among the console errors `RenderRunner` merged onto +// them. // -// The broader test drives the re-render, where being wrong costs one extra -// render. This one gates suppressing a row, where being wrong hides a genuine -// break — so a render whose own failure was a timeout or a wedge, and whose -// console happened to carry a missing-export line, is not grounds to withhold -// the error it actually produced. -function missingExportIsTheFailure(response: RenderVisitResponse): boolean { - for (let candidate of [ - response.card?.error, - response.fileExtract?.error, - response.fileRender?.error, - response.pageUnusableError, - ]) { +// Two narrowings, and both matter because of what the answer is used for. The +// broader `hasMissingExportError` drives the re-render, where being wrong costs +// one render; this gates withholding a row, where being wrong hides a genuine +// break. And it answers per row type rather than per response, because a single +// visit produces the instance and file rows independently — a card render that +// hit the stale bundle says nothing about a file extraction that failed beside +// it for its own reasons. +function unattributableRowTypes( + response: RenderVisitResponse, +): ('instance' | 'file')[] { + let types = new Set<'instance' | 'file'>(); + let consider = ( + candidate: { error?: { message?: unknown } } | undefined, + type: 'instance' | 'file', + ) => { let message = candidate?.error?.message; if (typeof message === 'string' && isMissingExportMessage(message)) { - return true; + types.add(type); } + }; + consider(response.card?.error, 'instance'); + consider(response.fileExtract?.error, 'file'); + consider(response.fileRender?.error, 'file'); + // A page that never became usable produced neither render, so the failure + // belongs to both rows rather than to one of them. + let pageUnusable = response.pageUnusableError; + let pageMessage = pageUnusable?.error?.message; + if (typeof pageMessage === 'string' && isMissingExportMessage(pageMessage)) { + types.add('instance'); + types.add('file'); } - return false; + return [...types]; } function hasMissingExportError(response: RenderVisitResponse): boolean { @@ -306,12 +321,18 @@ export function stampHostShellTokens( // and warmed tokens at the moment of the render. The write site checks for the // field's presence and nothing else — it does not re-derive the conclusion, so // there is one implementation of the rule rather than two that can drift. -export function stampStaleShellFailure(response: RenderVisitResponse): void { +export function stampStaleShellFailure( + response: RenderVisitResponse, + rowTypes: ('instance' | 'file')[], +): void { + if (rowTypes.length === 0) { + return; + } response.meta = { ...(response.meta ?? {}), diagnostics: { ...(response.meta?.diagnostics ?? {}), - staleShellFailure: true, + staleShellFailure: rowTypes, }, }; } @@ -1373,8 +1394,9 @@ export function buildPrerenderApp(options: { // merged onto an unrelated timeout, because being wrong there costs one // render. Withholding a row wants the missing export to be the failure // itself. + let unattributable = unattributableRowTypes(response); if ( - missingExportIsTheFailure(response) && + unattributable.length > 0 && shouldRerenderForStaleShell({ response, warmedAtStart, @@ -1383,13 +1405,14 @@ export function buildPrerenderApp(options: { }) ) { log.warn( - 'visit of %s failed to resolve a module on a pool warmed against %s -> %s while the current host shell is %s, after a re-render; marking the failure unattributable to the card', + 'visit of %s failed to resolve a module on a pool warmed against %s -> %s while the current host shell is %s, after a re-render; marking the %s row(s) unattributable to the card', url, warmedAtStart ?? 'none', warmedAtCompletion ?? 'none', shellAtCompletion, + unattributable.join(', '), ); - stampStaleShellFailure(response); + stampStaleShellFailure(response, unattributable); } } let totalMs = Date.now() - start; diff --git a/packages/realm-server/tests/prerender-host-shell-recycle-test.ts b/packages/realm-server/tests/prerender-host-shell-recycle-test.ts index bef499c7a58..44985fb6377 100644 --- a/packages/realm-server/tests/prerender-host-shell-recycle-test.ts +++ b/packages/realm-server/tests/prerender-host-shell-recycle-test.ts @@ -396,17 +396,32 @@ module(basename(import.meta.filename), function () { // the whole contract: absence has to mean "no verdict" rather than // "attributable", or a response from anything that does not stamp it would // read as a licence to withhold a row. - test('marks the failure and leaves the rest of diagnostics alone', function (assert) { + test('names the rows it covers and leaves the rest of diagnostics alone', function (assert) { let response = { meta: { requestId: 'abc', diagnostics: { renderMs: 12 } }, } as unknown as RenderVisitResponse; - stampStaleShellFailure(response); + stampStaleShellFailure(response, ['instance']); assert.deepEqual(response.meta, { requestId: 'abc', - diagnostics: { renderMs: 12, staleShellFailure: true }, + diagnostics: { renderMs: 12, staleShellFailure: ['instance'] }, } as unknown as typeof response.meta); }); + // A visit's rows fail independently, so a verdict that named the response + // rather than the rows would let one row's stale failure withhold + // another's genuine one. The write site tests membership, so an empty + // verdict must not be written at all. + test('an empty verdict stamps nothing', function (assert) { + let response = { + meta: { diagnostics: { renderMs: 12 } }, + } as unknown as RenderVisitResponse; + stampStaleShellFailure(response, []); + assert.false( + 'staleShellFailure' in ((response.meta as any).diagnostics ?? {}), + 'absent rather than an empty array a reader might mis-test', + ); + }); + test('an unmarked response carries no verdict at all', function (assert) { let response = { meta: { diagnostics: { renderMs: 12 } }, diff --git a/packages/realm-server/tests/prerender-server-test.ts b/packages/realm-server/tests/prerender-server-test.ts index 6386d3f997f..8e8ca445679 100644 --- a/packages/realm-server/tests/prerender-server-test.ts +++ b/packages/realm-server/tests/prerender-server-test.ts @@ -1292,9 +1292,10 @@ module(basename(import.meta.filename), function () { ); assert.strictEqual(calls, 2, 'one re-render, and no more'); assert.strictEqual(res.status, 201, 'the failure is still returned'); - assert.true( + assert.deepEqual( res.body.data.attributes.meta.diagnostics.staleShellFailure, - 'marked, so the write site can decline to publish it as content', + ['instance'], + 'marked, and scoped to the row that actually failed this way', ); }); @@ -1337,6 +1338,43 @@ module(basename(import.meta.filename), function () { ); }); + // The scoping the review asked for. One visit produces the instance and + // file rows independently: here the card render hit the stale bundle + // while the file extraction failed for a reason of its own. A verdict + // naming the response rather than the rows would withhold both, hiding + // the file's genuine failure. + test('a verdict names only the rows that failed on the stale bundle', async function (assert) { + let built = buildPrerenderApp({ + serverURL: 'http://127.0.0.1:4222', + getHostShellHash: () => 'b778fe76', + getWarmedHostShellHash: () => 'babf3612', + awaitHostShellRecycle: () => Promise.resolve(), + }); + let request: SuperTest = supertest(built.app.callback()); + + (built.prerenderer as any).prerenderVisit = async () => ({ + response: { + card: { error: { error: { message: MISSING_EXPORT } } }, + fileExtract: { + error: { error: { message: 'Unexpected end of JSON input' } }, + }, + }, + timings: timings(), + pool: poolMeta(), + }); + + let res = await visitRequest( + request, + `${realmURL.href}two-failures`, + authFor(), + ); + assert.deepEqual( + res.body.data.attributes.meta.diagnostics.staleShellFailure, + ['instance'], + "only the card's row is withheld; the file's own failure stays visible", + ); + }); + // The narrowing that separates driving a re-render from withholding a // row. A timeout whose console happens to carry a missing-export line is // worth one more render — the broad test allows that — but it is not diff --git a/packages/runtime-common/index-writer.ts b/packages/runtime-common/index-writer.ts index aa10271eaa4..49973ccbd24 100644 --- a/packages/runtime-common/index-writer.ts +++ b/packages/runtime-common/index-writer.ts @@ -269,6 +269,22 @@ function prerenderedHtmlEntryFrom( }; } +// Whether a verdict from the prerender server covers the row type about to be +// written. Presence *and* membership: the mark names which of a visit's rows it +// applies to, so a card render that hit a stale bundle cannot withhold a file +// row that failed for its own reasons. +// +// Absence means no verdict, never "attributable" — a response from anything +// that does not stamp this (a server predating the field, which a worker sees +// throughout a rolling deploy) must not read as licence to withhold a row. +function verdictCoversRow( + diagnostics: Diagnostics | undefined, + type: 'instance' | 'file', +): boolean { + let covered = diagnostics?.staleShellFailure; + return Array.isArray(covered) && covered.includes(type); +} + // Rows held in the write-behind buffer before a flush is forced (see // `Batch.bufferEntry`). Dependency reads flush earlier; this only bounds // memory across long runs of dependency-free files. Renders dwarf the writes, @@ -1300,9 +1316,9 @@ export class Batch { // to protect, and withholding its error would leave nothing at all — // the failure has to surface somewhere, and an error row is the right // output there even when the environment caused it. - let withholdFailure = Boolean( - diagnostics?.staleShellFailure && production.pristine_doc, - ); + let withholdFailure = + verdictCoversRow(diagnostics, baseTypeFromError(entry)) && + Boolean(production.pristine_doc); entryPayload = { types: entry.types, // favor the last known good types over the types derived from the error state @@ -1542,6 +1558,18 @@ export class Batch { }, url, ); + // Any preserved render is content worth keeping; `isolated_html` alone + // is not the test, since a FileDef family may carry only markdown. + let hasPriorRender = Boolean( + production?.isolated_html ?? + production?.embedded_html ?? + production?.fitted_html ?? + production?.atom_html ?? + production?.head_html ?? + production?.markdown, + ); + let withholdHtmlFailure = + verdictCoversRow(entry.diagnostics, type) && hasPriorRender; if (errorDoc.visitRequestFailure) { // Consecutive-failure bookkeeping for the reconcile sweep's // bounded retry lane: extend the prior row's run when it was also @@ -1569,7 +1597,19 @@ export class Batch { ...new Set([...(production?.deps ?? []), ...(errorDoc.deps ?? [])]), ], last_known_good_deps: production?.last_known_good_deps ?? null, - error_doc: errorDoc, + // The same withholding as the index channel, and it has to be here + // too: `effectiveHasError()` is + // `COALESCE(i.has_error, FALSE) OR (ph.error_doc IS NOT NULL AND + // ph.generation >= i.generation)`, so a current error on this channel + // makes the row read as errored whatever `boxel_index` says — and + // `effectiveErrorDoc()` then serves this column. Suppressing only the + // index channel would leave the transient failure published on the + // split path, which is the default on Postgres. + // + // Gated on prior HTML for the same reason the other channel gates on + // `pristine_doc`: with nothing to fall back to, the failure has to + // surface rather than leave the row blank. + error_doc: withholdHtmlFailure ? null : errorDoc, diagnostics: entry.diagnostics ?? null, // Like the HTML columns above: the manifest is a last-known-good // artifact — its objects still exist in the MediaCache and the diff --git a/packages/runtime-common/index.ts b/packages/runtime-common/index.ts index ad8d93eb0d3..4ae5bc60a10 100644 --- a/packages/runtime-common/index.ts +++ b/packages/runtime-common/index.ts @@ -814,7 +814,15 @@ export interface Diagnostics // Absent means no verdict, never "attributable". A reader must require // presence before suppressing anything, for the same reason the warmed // tokens distinguish `null` from absence. - staleShellFailure?: true; + // Which of this URL's row types the verdict covers, because one visit can + // produce several and they fail independently. A card render can hit the + // stale bundle while the file extraction beside it fails for reasons of its + // own, and withholding both on one response-level flag would hide that + // second, genuine failure. + // + // An empty array is not written: absence means no verdict, and a reader must + // find its own row type listed before withholding anything. + staleShellFailure?: ('instance' | 'file')[]; // A row is produced by two prerender visits (index + prerender-html), // each its own HTTP request. `requestId` always carries the index visit's // id and this always carries the prerender-html visit's, whichever table From bbc7cfb51e5b1f27c5d6ec5cab77f984c0e7b88d Mon Sep 17 00:00:00 2001 From: Buck Doyle Date: Fri, 11 Sep 2026 19:35:43 +0200 Subject: [PATCH 3/3] Cover the write side, where the withholding was found inert once already MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The gates were tested at the prerender end only: the verdict's content was pinned, and what the write sites did with it was not. That is how a version shipped that suppressed the index channel while the prerendered-HTML channel kept publishing the same failure — `effectiveHasError()` reads both, so the row stayed errored and the change did nothing on the path production takes. Four cases against a real database, in the split-batch module, which drives `IndexWriter` directly and needs no realm server: - a covered verdict keeps the published render and writes no error doc, which is the property `effectiveHasError()` actually reads; - a verdict naming the other row type does not withhold this one, so one row's stale failure cannot hide another's genuine one; - an unmarked failure publishes as before, because absence has to keep meaning "no verdict"; - a first render publishes its failure, since there is no good content to protect and withholding would leave the row blank. Removing the gate fails the first and only the first. --- .../tests/prerender-html-split-test.ts | 96 +++++++++++++++++++ 1 file changed, 96 insertions(+) diff --git a/packages/realm-server/tests/prerender-html-split-test.ts b/packages/realm-server/tests/prerender-html-split-test.ts index 25d4d27c132..bae4e026c19 100644 --- a/packages/realm-server/tests/prerender-html-split-test.ts +++ b/packages/realm-server/tests/prerender-html-split-test.ts @@ -656,6 +656,102 @@ module(basename(import.meta.filename), function () { }; } + // Withholding a stale-shell failure has to happen on THIS channel as well + // as on `boxel_index`, because `effectiveHasError()` is + // `COALESCE(i.has_error, FALSE) OR (ph.error_doc IS NOT NULL AND + // ph.generation >= i.generation)` — a current error doc here makes the row + // read as errored whatever the index channel says, and `effectiveErrorDoc()` + // then serves this column. Postgres uses the split channel by default, so a + // suppression that covered only the index channel would be inert in + // production. + module('withholding a stale-shell failure', function () { + async function writeError( + generation: number, + url: string, + opts: { diagnostics?: Diagnostics; message?: string } = {}, + ) { + let batch = await makeBatch(generation); + await batch.seedPrerenderedHtmlInvalidations([ + { url, operation: 'update' }, + ]); + await batch.updatePrerenderedHtmlEntry(new URL(url), { + type: 'instance-error', + error: { + message: opts.message ?? 'has no exported member', + status: 500, + additionalErrors: null, + }, + ...(opts.diagnostics ? { diagnostics: opts.diagnostics } : {}), + } as any); + await batch.done(); + } + + test('a covered verdict keeps the published render and writes no error', async function (assert) { + let url = `${testRealm}withheld.json`; + await writeInstance(1, url, '
good
'); + + await writeError(2, url, { + diagnostics: { staleShellFailure: ['instance'] } as Diagnostics, + }); + + let row = await productionRow(url); + assert.strictEqual( + row.error_doc, + null, + 'no error doc, so `effectiveHasError` does not see a current render error', + ); + assert.strictEqual( + row.isolated_html, + '
good
', + 'and the last good render is still what readers get', + ); + }); + + test('a verdict naming another row does not withhold this one', async function (assert) { + let url = `${testRealm}other-row.json`; + await writeInstance(1, url, '
good
'); + + // The card render hit the stale bundle; this instance row failed for + // its own reasons and must stay visible. + await writeError(2, url, { + diagnostics: { staleShellFailure: ['file'] } as Diagnostics, + }); + + let row = await productionRow(url); + assert.ok( + row.error_doc, + 'the error is published, because the verdict did not cover this row', + ); + }); + + test('an unmarked failure is published as before', async function (assert) { + let url = `${testRealm}unmarked.json`; + await writeInstance(1, url, '
good
'); + await writeError(2, url); + + let row = await productionRow(url); + assert.ok( + row.error_doc, + 'absence of a verdict means no verdict, never "withhold"', + ); + }); + + test('a first render has nothing to keep, so its failure surfaces', async function (assert) { + let url = `${testRealm}brand-new.json`; + // No prior good render at all — withholding here would leave the row + // blank rather than protecting anything. + await writeError(1, url, { + diagnostics: { staleShellFailure: ['instance'] } as Diagnostics, + }); + + let row = await productionRow(url); + assert.ok( + row.error_doc, + 'the failure has to surface somewhere when there is no good content', + ); + }); + }); + test('writes rendered rows and swaps them under the carried generation', async function (assert) { let url = `${testRealm}1.json`; await writeInstance(5, url, '

v5

');