diff --git a/package-lock.json b/package-lock.json index db5dd0b9..2f204f2d 100644 --- a/package-lock.json +++ b/package-lock.json @@ -8570,7 +8570,7 @@ }, "packages/plugin": { "name": "@harperfast/prerender", - "version": "0.84.0", + "version": "0.85.0", "license": "Apache-2.0", "dependencies": { "fast-xml-parser": "^5.0.9", diff --git a/packages/plugin/package.json b/packages/plugin/package.json index 499d5bc5..cf082229 100644 --- a/packages/plugin/package.json +++ b/packages/plugin/package.json @@ -1,6 +1,6 @@ { "name": "@harperfast/prerender", - "version": "0.84.0", + "version": "0.85.0", "type": "module", "description": "Configurable Harper plugin for prerendering pages for bots and crawlers", "license": "Apache-2.0", diff --git a/packages/plugin/src/resources/RenderQueue.js b/packages/plugin/src/resources/RenderQueue.js index 49694ff1..6b9912da 100644 --- a/packages/plugin/src/resources/RenderQueue.js +++ b/packages/plugin/src/resources/RenderQueue.js @@ -705,11 +705,22 @@ export class RenderQueue extends Resource { // Suppress writes the URL row (its recheck) and drops every device's page; the verdict // SUPPRESSES the target rather than deleting it — see Target.suppress, which also grades // http-error verdicts by status (404/410 recheck less, die sooner). - const { deleted } = await Target.suppress(url, { reason: verdict.reason, statusCode: verdict.statusCode }); + const { deleted, absent } = await Target.suppress(url, { + reason: verdict.reason, + statusCode: verdict.statusCode, + }); // At maxStrikes the suppression DELETED the target, and `Target.delete` took the URL's default // device rows with it — so a folding row is already gone (a second delete would only write a // tombstone), while a non-default one-device row still needs retiring below. if (deleted) job.rowGone = job.fold; + // A gone verdict on a URL with no Target: nothing was minted (see Target.suppress), so this + // job's row is settled as any targetless result's is — already gone when a sibling job's + // retirement took it (the common case), dropped for a one-off, deferred for a recurring row + // whose target may simply not have replicated here yet. + else if (absent) { + logger.info(`Prerendered url ${url} is gone and has no Target on this node — not minting one`); + await settleTargetless(job, await readTargetlessRow(job)); + } await retireRowIfConverted(job, held); return; } diff --git a/packages/plugin/src/resources/Target.js b/packages/plugin/src/resources/Target.js index e49edcaa..48a30b54 100644 --- a/packages/plugin/src/resources/Target.js +++ b/packages/plugin/src/resources/Target.js @@ -198,8 +198,10 @@ export class Target extends TargetTable { * row is the only thing holding the next refresh off. See util/suppression.js. * * Creates the row when absent (a render-now one-off or a redirect destination can be - * proven non-indexable before anything targeted it). Deletes the cached pages either - * way — stale content of a page that said "don't index me" must not keep serving. + * proven non-indexable before anything targeted it) — EXCEPT on a gone verdict, which + * returns `{ absent: true }` and writes nothing: see the guard below. Deletes the cached + * pages either way — stale content of a page that said "don't index me" must not keep + * serving. */ static async suppress(url, { reason, statusCode } = {}) { const existing = await Target.get({ @@ -211,17 +213,33 @@ export class Target extends TargetTable { // The grading reads `sitemapUrl`, which this projection already carries: a gone verdict on a // target no sitemap lists has its own (lower) ceiling, because nothing re-creates it. See // util/suppression.js for why that asymmetry exists and why it is scoped to gone verdicts. - const { storedReason, recheckInterval, maxStrikes } = gradeSuppression({ + const { gone, storedReason, recheckInterval, maxStrikes } = gradeSuppression({ reason, statusCode, fromSitemap: !!existing?.sitemapUrl, }); - // `existing` is still required: with no row there is nothing to retire, and minting one is - // this method's documented job (a render-now one-off or a redirect destination can be proven - // non-indexable before anything targeted it). At an unlisted ceiling of 1 that row is created - // only to be retired on its first recheck — rare enough (those two paths only) to leave as - // the simpler behaviour rather than special-case a second deletion path here. + // A GONE VERDICT NEVER MINTS A ROW. A suppressed row exists to stop something re-creating the + // URL, and for a 404/410 nothing can: discovery mints only on a 200, and a URL a sitemap lists + // already has its row (attribution lives on it). So a minted gone row blocks nothing, costs a + // recheck render at `gone.recheckInterval`, and — because `handlePageScheduling` treats ANY row + // as "already known" — stops discovery re-creating the URL if the origin brings it back. + // + // This used to be assumed rare (a render-now one-off, a redirect destination). It is not: two + // jobs for one URL — its URL row and a pre-0.66.0 device row still waiting to fold — can be in + // flight together. The first 404 retires the target (unlisted, first verdict) and its + // `Target.delete` takes both rows; the second 404 arrives a moment later, finds no row, and + // minted it back as `http-gone` with a strike and a 14-day recheck. Measured on a four-node + // deployment: ~76,600 discovered URLs re-minted this way in the week after the unlisted + // ceiling shipped, i.e. the first-verdict retirement was undone for every URL that raced. + // + // The caller settles the job's own row (see RenderQueue.processDecodedJobResult). + if (!existing && gone) return { deleted: false, absent: true }; + + // With no row there is nothing to retire, and minting one is this method's documented job (a + // render-now one-off or a redirect destination can be proven non-indexable before anything + // targeted it) — for the verdicts whose page the origin still serves 200, which discovery + // WOULD otherwise re-mint. if (existing && Number.isFinite(maxStrikes) && maxStrikes > 0 && strikes >= maxStrikes) { logger.warn( `Prerender target ${url} non-indexable ${strikes} consecutive times (${storedReason ?? 'no reason'}) — deleting it` diff --git a/packages/plugin/test/renderQueueTargetless.test.js b/packages/plugin/test/renderQueueTargetless.test.js index 226f23d1..fd836453 100644 --- a/packages/plugin/test/renderQueueTargetless.test.js +++ b/packages/plugin/test/renderQueueTargetless.test.js @@ -289,6 +289,100 @@ test('the redirect path defers a recurring targetless row too', async () => { assert.ok(stores.renderSchedule.get(A)?.targetMissingSince, 'deferred, not dropped'); }); +// ───────────────────────────── a gone verdict never mints a row ───────────────────────────── +// +// A suppressed row exists to stop something re-creating the URL, and for a 404/410 nothing can: +// discovery mints only on a 200. So `Target.suppress` returns `absent` instead of minting, and the +// job's row is settled like any other targetless result's. The race it closes: a URL row and a +// pre-0.66.0 device row in flight together — the first 404 retires the target and takes both rows, +// and the second used to mint it straight back as a suppressed row with a 14-day recheck. + +const gone = (deviceType, statusCode = 404) => ({ + deviceType, + statusCode, + outcome: 'non-indexable', + isIndexable: false, + reason: 'http-error', + headers: {}, + renderTime: 100, +}); + +/** An unlisted target with its URL row AND a device row not yet folded — both due. */ +const seedUnfoldedPair = () => { + stores.target.set(A, { url: A, renderInterval: 4 * HOUR }); + stores.renderSchedule.set(A, { nextRenderTime: 1, fromSitemap: false, effectiveInterval: 4 * HOUR }); + stores.renderSchedule.set(key(A, 'mobile'), { nextRenderTime: 1, fromSitemap: false }); +}; + +const assertRetiredForGood = async () => { + assert.equal(stores.target.has(A), false, 'the late sibling must not mint the target back as suppressed'); + assert.equal(stores.renderSchedule.size, 0, 'no recheck row, and no device row left behind'); + assert.equal(stores.prerenderedPage.size, 0); + assert.deepEqual(await claim(), [], 'nothing left to render'); +}; + +test('a device-row 404 landing after its URL row retired the target does NOT re-mint it', async () => { + seedUnfoldedPair(); + const claimed = (await claim()).map((job) => job.id).sort(); + assert.deepEqual(claimed, [A, key(A, 'mobile')].sort(), 'precondition: both rows are in flight at once'); + + await postVariants(A, [gone('desktop'), gone('mobile')]); + assert.equal(stores.target.has(A), false, 'precondition: an unlisted 404 retires on the first verdict'); + assert.equal(stores.renderSchedule.has(key(A, 'mobile')), false, 'precondition: and its delete took the device row'); + + await post({ id: key(A, 'mobile'), url: A, ...gone('mobile') }); + + await assertRetiredForGood(); + assert.ok( + infos.some((m) => m.includes('not minting')), + 'the skipped mint is logged' + ); +}); + +test('the same race in the other order: a URL-row 404 after the device row retired the target', async () => { + seedUnfoldedPair(); + await claim(); + + await post({ id: key(A, 'mobile'), url: A, ...gone('mobile') }); + assert.equal(stores.target.has(A), false, 'precondition: the device-row verdict retired the target'); + + await postVariants(A, [gone('desktop'), gone('mobile')]); + + await assertRetiredForGood(); +}); + +test('a gone one-off with no target drops its row and mints nothing', async () => { + stores.renderSchedule.set(A, { nextRenderTime: 1, fromSitemap: false, effectiveInterval: null }); + await claim(); + await postVariants(A, [gone('desktop', 410), gone('mobile', 410)]); + + assert.equal(stores.target.has(A), false); + assert.equal(stores.renderSchedule.has(A), false, 'the one-off row is dropped'); + assert.equal(leased(A), false); +}); + +test('a gone verdict on a recurring row whose target is missing here defers the row and mints nothing', async () => { + // The target may exist elsewhere and not have replicated yet — minting here would REPLACE it with + // a row carrying `sitemapUrl: null` once it arrives. Deferred like any targetless recurring row. + seedRecurringRow(); + await claim(); + await postVariants(A, [gone('desktop'), gone('mobile')]); + + assert.equal(stores.target.has(A), false, 'a 404 must not mint a suppressed target'); + assert.ok(stores.renderSchedule.get(A)?.targetMissingSince, 'deferred, not dropped'); + assert.deepEqual(outcomes(), [['suppressed', 'http-error']], 'the verdict is still counted'); +}); + +test('a NON-gone verdict with no target still mints its suppressed row — the page 200s, so discovery would', async () => { + stores.renderSchedule.set(A, { nextRenderTime: 1, fromSitemap: false, effectiveInterval: null }); + await claim(); + const noindex = (deviceType) => ({ ...gone(deviceType, 200), reason: 'noindex' }); + await postVariants(A, [noindex('desktop'), noindex('mobile')]); + + assert.equal(stores.target.get(A)?.state, 'suppressed'); + assert.equal(stores.target.get(A)?.suppressedReason, 'noindex'); +}); + // ───────────────────────────── retirement leaves no page behind ───────────────────────────── test('retiring a target deletes its pages for EVERY supported device — a tablet one-off included', async () => { diff --git a/packages/plugin/test/suppressionStatus.test.js b/packages/plugin/test/suppressionStatus.test.js index d3ee47c4..3eb641a1 100644 --- a/packages/plugin/test/suppressionStatus.test.js +++ b/packages/plugin/test/suppressionStatus.test.js @@ -494,21 +494,37 @@ test('a client-side redirect landing on a 503 page keeps the source and seeds no assert.equal(stores.target.has(B), false, 'transient destination must not become a suppressed row'); }); -test('a client-side redirect landing on a 404 page retires the source and seeds an http-gone destination', async () => { - // The genuine-verdict path the guard must NOT swallow: gone destinations still classify. +const redirectedToGone = () => ({ + id: key(A), + url: A, + statusCode: 404, + outcome: 'redirected', + redirectedTo: B, + isIndexable: false, + reason: 'http-error', +}); + +test('a client-side redirect landing on a 404 page retires the source and mints no destination row', async () => { + // A suppressed row for a 404 destination would block nothing — discovery cannot mint a URL the + // origin 404s — and would cost a recheck render (see Target.suppress). seedSource(); - await postResult({ - id: key(A), - url: A, - statusCode: 404, - outcome: 'redirected', - redirectedTo: B, - isIndexable: false, - reason: 'http-error', - }); + await postResult(redirectedToGone()); assert.equal(stores.target.has(A), false, 'source retired — it leads to a page that is gone'); + assert.equal(stores.target.has(B), false, 'a gone destination nothing targets is not minted'); + assert.equal(stores.renderSchedule.has(B), false, 'and gets no recheck row'); +}); + +test('a client-side redirect landing on a 404 page still classifies a TARGETED destination as gone', async () => { + // The genuine-verdict path the auth/transient guard must NOT swallow: gone destinations still + // classify. Sitemap-listed, so the gone verdict suppresses rather than retiring on first sight. + seedSource(); + stores.target.set(B, { url: B, sitemapUrl: SITEMAP }); + await postResult(redirectedToGone()); + + assert.equal(stores.target.has(A), false, 'source retired'); assert.equal(stores.target.get(B)?.suppressedReason, 'http-gone', 'destination suppressed under the gone class'); + assert.equal(stores.target.get(B)?.sitemapUrl, SITEMAP, 'its sitemap attribution is kept'); }); test('a BigInt strikes value (Harper numeric surfacing) still counts toward the slow lane', async () => {