diff --git a/packages/realm-server/prerender/prerender-app.ts b/packages/realm-server/prerender/prerender-app.ts index 2bc829cafbb..69d1b3f26b5 100644 --- a/packages/realm-server/prerender/prerender-app.ts +++ b/packages/realm-server/prerender/prerender-app.ts @@ -187,6 +187,44 @@ export function shouldRerenderForStaleShell({ ); } +// 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. +// +// 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)) { + 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 [...types]; +} + 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 +314,29 @@ 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, + rowTypes: ('instance' | 'file')[], +): void { + if (rowTypes.length === 0) { + return; + } + response.meta = { + ...(response.meta ?? {}), + diagnostics: { + ...(response.meta?.diagnostics ?? {}), + staleShellFailure: rowTypes, + }, + }; +} + // A one-shot notification that shutdown has begun, which the holder releases // when it no longer needs it. type DrainSubscription = { @@ -1321,6 +1382,38 @@ 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. + let unattributable = unattributableRowTypes(response); + if ( + unattributable.length > 0 && + 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 %s row(s) unattributable to the card', + url, + warmedAtStart ?? 'none', + warmedAtCompletion ?? 'none', + shellAtCompletion, + unattributable.join(', '), + ); + stampStaleShellFailure(response, unattributable); + } } 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..44985fb6377 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,48 @@ 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('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, ['instance']); + assert.deepEqual(response.meta, { + requestId: 'abc', + 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 } }, + } 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-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

'); diff --git a/packages/realm-server/tests/prerender-server-test.ts b/packages/realm-server/tests/prerender-server-test.ts index f60e1ab59c7..8e8ca445679 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,156 @@ 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.deepEqual( + res.body.data.attributes.meta.diagnostics.staleShellFailure, + ['instance'], + 'marked, and scoped to the row that actually failed this way', + ); + }); + + 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 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 + // 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 b2fc3cfbb6a..c8125ef862a 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, @@ -1365,6 +1381,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 = + 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 @@ -1375,9 +1410,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( @@ -1385,8 +1427,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; @@ -1609,6 +1669,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(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 @@ -1636,7 +1708,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, // 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 c67928bcdec..e500ca600b7 100644 --- a/packages/runtime-common/index.ts +++ b/packages/runtime-common/index.ts @@ -854,6 +854,27 @@ 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. + // 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