From c46acd105a4aacca6871855d52945311f06de10d Mon Sep 17 00:00:00 2001 From: Joe Date: Fri, 21 Aug 2026 13:44:30 -0400 Subject: [PATCH] feat(browser): decide suppression at navigation, not after the settle; v1.18.0 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The settle phase IS a render's cost — measured on the reference deployment, the scroll-settle passes alone are ~78% of render time and the bulk of per-render CPU. A page that ends up non-indexable pays all of it, has its bytes discarded, and then pays it again on every `render.suppression.recheck` for as long as the plugin keeps the target. Two verdicts are already final right after the navigation, so they are taken there. 1. A NON-200. Behaviour-identical BY CONSTRUCTION: the status cannot change during the settle, and the post-settle branch does nothing with a non-200 except set isIndexable false / reason 'http-error' and return no content — for a sitemap-listed url too. So it needs no sitemap exemption, and turning it off buys nothing but latency. 2. A DOCUMENT THAT ALREADY DISOWNS ITSELF, when the plugin discovered it. Same `readIndexVerdict` the post-settle check uses, so it is the verdict that check would have reached — unless the page mutates its own during the settle. That is the entire scope of the difference, which is why this half has its own switch while the status half does not. A SITEMAP-LISTED URL IS NOT ELIGIBLE for (2), and the exemption is load-bearing rather than cautious: such a page is serialized even when non-indexable, so its settle is not wasted work and bailing would replace a served page with nothing. The plugin's suppression branch never sees those urls either — only urls it discovered are retirable that way. The post-settle check REMAINS THE BACKSTOP. A page that adds its noindex during the settle is still caught; it just pays for the settle to be caught. Moving the check forward must not narrow what gets suppressed, only when. Both checks now go through one `readIndexVerdict`, because they ask the identical question at two moments and the only difference that should exist between their answers is the page having changed. Two copies of that reasoning would add a second difference nobody would notice. 130 pass / 0 fail. `test/earlySuppression.test.ts` asserts on `timings.settle` being undefined — the only direct evidence the phase never started rather than merely ran fast — and covers the sitemap exemption, both kill switches reproducing the same verdict a phase later, the late-mutation backstop, and that a bail navigates exactly once. Co-Authored-By: Claude Opus 5 --- packages/browser/README.md | 29 +++ packages/browser/package.json | 2 +- packages/browser/src/config.ts | 35 +++ packages/browser/src/renderer.ts | 90 +++++-- .../browser/test/earlySuppression.test.ts | 220 ++++++++++++++++++ 5 files changed, 361 insertions(+), 15 deletions(-) create mode 100644 packages/browser/test/earlySuppression.test.ts diff --git a/packages/browser/README.md b/packages/browser/README.md index 5d86d86..5f35ec3 100644 --- a/packages/browser/README.md +++ b/packages/browser/README.md @@ -117,12 +117,41 @@ include what you change: }, "injectWebComponentsPolyfill": true, // force ShadyDOM/ShadyCSS so shadow-DOM CSS serializes "extraHeaders": {}, // extra request headers on the navigation request + // How much of a render to pay for before accepting the page will not be served. Both decide + // right after navigation instead of after the settle phase — see "Early suppression" below. + "suppression": { "earlyErrorStatus": true, "earlyNonIndexable": true }, } ``` Invalid config (missing viewport, `defaultDevice` not in `devices`, non-positive budgets) throws at `startWorker()`. +### Early suppression + +The settle phase is where a render's cost is — on the reference deployment the scroll-settle passes +alone are **~78% of render time** and the bulk of per-render CPU. A page that turns out to be +non-indexable pays all of it, has its bytes discarded, and then pays it again on every +`render.suppression.recheck` for as long as the plugin keeps the target. Two verdicts are already +final immediately after navigation, so `suppression` takes them there. + +The two switches are separate because their risk is not the same: + +- **`earlyErrorStatus`** is behaviour-identical _by construction_. An HTTP status cannot change during + the settle, and the post-settle branch does nothing with a non-200 but report it with no content — + for a sitemap-listed URL as well. There is no case where settling first produces a different result, + which is why it needs no sitemap exemption and why turning it off buys nothing but latency. +- **`earlyNonIndexable`** reads the page's own `noindex` / `` off the _initial_ + DOM rather than the settled one. Those agree unless the page mutates its own `` during the + settle — adding a canonical from client-side routing, or removing a server-rendered `noindex`. The + second direction is the one that costs something, so it gets its own switch. The post-settle check + remains the backstop either way: a page that adds its `noindex` late is still caught, it just pays + for the settle to be caught. + +**Sitemap-listed URLs are never eligible for `earlyNonIndexable`.** They are serialized even when +non-indexable, so their settle is not wasted work and bailing would replace a served page with +nothing. (`renderOnce` maps `captureNonIndexable` onto `job.isFromSitemap`, so an inspection render +settles by default.) + ## Custom renderer A renderer receives the Puppeteer `page` and the `RenderJob` and returns the serialized HTML (or diff --git a/packages/browser/package.json b/packages/browser/package.json index e078f4c..4410bb1 100644 --- a/packages/browser/package.json +++ b/packages/browser/package.json @@ -1,6 +1,6 @@ { "name": "@harperfast/prerender-browser", - "version": "1.17.0", + "version": "1.18.0", "type": "module", "description": "Headless-browser render library for Harper Prerender: claims render jobs from the @harperfast/prerender queue, renders pages in headless Chrome (Puppeteer), and posts the HTML back. Embedded by a render service and configured entirely via startWorker() options.", "keywords": [ diff --git a/packages/browser/src/config.ts b/packages/browser/src/config.ts index eaf8926..3bf33c9 100644 --- a/packages/browser/src/config.ts +++ b/packages/browser/src/config.ts @@ -218,6 +218,39 @@ export type WaitForRule = { * canonical comes back `A%20B`, or it simply serves what `A B` names — it form-decodes, and the * two spellings can never name different resources. */ +/** + * How much of a render to pay for before accepting that the page will not be served. + * + * The settle phase is a render's cost — measured on the reference deployment, the scroll-settle + * passes alone are ~78% of render time and the bulk of per-render CPU. Everything a non-indexable + * page spends there is discarded, and it is spent again on every `render.suppression.recheck` + * for as long as the plugin keeps the target. Both switches move the verdict to immediately after + * the navigation, where the answer is already available. + * + * They are separate because their risk is not the same: + * + * `earlyErrorStatus` is behaviour-identical BY CONSTRUCTION. An HTTP status cannot change during + * the settle, and the post-settle branch does nothing with a non-200 but report it with no + * content — for a sitemap-listed url as well. There is no case in which settling first would + * have produced a different result, which is why it needs no sitemap exemption and why turning + * it off buys nothing but latency. + * + * `earlyNonIndexable` reads the page's own `noindex`/canonical off the initial DOM instead of the + * settled one. Those agree unless the page MUTATES ITS OWN HEAD during the settle — adding a + * canonical from client-side routing, or removing a server-rendered `noindex`. The second + * direction is the one that costs something: the page would be suppressed on evidence it was + * about to withdraw. It is rare enough to default on and specific enough to be worth its own + * switch. Sitemap-listed urls are never eligible for it: they are serialized even when + * non-indexable, so their settle is not wasted and bailing would replace a served page with + * nothing. + */ +export type SuppressionConfig = { + /** Post the verdict right after navigation when the status is not 200. Default true. */ + earlyErrorStatus: boolean; + /** ...and when a non-sitemap page's initial DOM already disowns it. Default true. */ + earlyNonIndexable: boolean; +}; + export type CanonicalConfig = { /** Treat a re-spelled self-canonical as a duplicate cache key (non-indexable). Default false. */ strict: boolean; @@ -253,6 +286,7 @@ export type PrerenderConfig = { scroll: ScrollConfig; postProcess: PostProcessConfig; canonical: CanonicalConfig; + suppression: SuppressionConfig; cacheKey: CacheKeyConfig; /** * Optional declarative "wait for content" rules applied after scroll/settle and before the @@ -304,6 +338,7 @@ export const defaultConfig = (): PrerenderConfig => ({ resolveLazyImages: false, }, canonical: { strict: false }, + suppression: { earlyErrorStatus: true, earlyNonIndexable: true }, cacheKey: { plusIsSpace: false, trailingSlash: 'strip' }, injectWebComponentsPolyfill: true, extraHeaders: {}, diff --git a/packages/browser/src/renderer.ts b/packages/browser/src/renderer.ts index 4e3fbd3..d2f401a 100644 --- a/packages/browser/src/renderer.ts +++ b/packages/browser/src/renderer.ts @@ -5,6 +5,7 @@ import { CACHE_REPLAY_HEADER, getResourceCache } from './ResourceCache.js'; import type { PostProcessConfig } from './config.js'; import { canonicalizeUrl, canonicalVerdict } from './util/url.js'; import { markRenderPhase } from './util/renderPhase.js'; +import type { Page } from 'puppeteer'; const noop = () => {}; @@ -265,6 +266,49 @@ const renderer: Renderer = async (page, job) => { } } + // ── DECIDE WHAT IS ALREADY DECIDABLE, BEFORE PAYING FOR THE SETTLE ────────────────────────── + // + // The settle phase is where a render's cost is. Measured on this deployment the scroll-settle + // passes alone are ~78% of render time and the bulk of per-render CPU, and a page that ends up + // non-indexable pays all of it and then has its bytes discarded — and pays it again on every + // suppression recheck for as long as the target exists. Two verdicts are already final here: + // + // 1. A NON-200. The status cannot change during the settle, and the post-settle branch does + // nothing with a non-200 except set `isIndexable = false` / `reason: 'http-error'` and + // return no content — for a sitemap-listed url too. So this is behaviour-identical by + // construction rather than merely close, which is why it needs no sitemap exemption. + // 2. A DOCUMENT THAT ALREADY DISOWNS ITSELF. The signals come from the same + // `readIndexVerdict` the post-settle check uses, so this is the verdict that check would + // have reached — UNLESS the page mutates its own `` during the settle. That is the + // entire scope of the difference between the two, and the reason this half has its own + // switch while the status half does not. + // + // A SITEMAP-LISTED URL IS NOT ELIGIBLE for (2), and that exemption is load-bearing rather than + // cautious: such a page is serialized even when non-indexable (see the `job.isFromSitemap` + // branch below), so its settle is not wasted work and bailing would replace a served page with + // nothing at all. The plugin's own suppression branch never sees those urls either — only urls + // it discovered are retirable this way. + if (finalRes) { + const status = finalRes.status(); + + if (config.suppression.earlyErrorStatus && status !== 200) { + job.httpResponse = { statusCode: status, headers: finalRes.headers() }; + job.isIndexable = false; + job.reason = 'http-error'; + return; + } + + if (config.suppression.earlyNonIndexable && status === 200 && !job.isFromSitemap) { + const { indexable, reason } = await readIndexVerdict(page, config.canonical.strict); + if (!indexable) { + job.httpResponse = { statusCode: status, headers: finalRes.headers() }; + job.isIndexable = false; + job.reason = reason ?? undefined; + return; + } + } + } + const settleStart = Date.now(); const networkIdle = () => @@ -449,20 +493,9 @@ const renderer: Renderer = async (page, job) => { } if (statusCode === 200) { - const { canonicalHref, noindex } = await page.evaluate(extractIndexSignals); - // A canonical naming a DIFFERENT document always disowns the page — invariable, every - // site. A canonical naming this very document RE-SPELLED as another cache key - // ('variant') is only a duplicate if the site's origin cannot tell the two spellings - // apart, which is a property of its query parser, not of the URLs — so that half is - // config, defaulting to the historical lenient reading. See config.canonical.strict. - // Either way the reason slug is distinct, so duplicate spellings stay legible next to - // genuine mismatches. - const verdict = canonicalVerdict(canonicalHref, rawPageUrl); - const disowned = verdict === 'elsewhere' || (verdict === 'variant' && config.canonical.strict); - job.isIndexable = !noindex && !disowned; - if (!job.isIndexable) { - job.reason = noindex ? 'noindex' : verdict === 'variant' ? 'canonical-variant' : 'canonical-mismatch'; - } + const { indexable, reason } = await readIndexVerdict(page, config.canonical.strict); + job.isIndexable = indexable; + if (reason) job.reason = reason; if (job.isIndexable || job.isFromSitemap) { const ppStart = Date.now(); @@ -592,6 +625,35 @@ function countMatchingElements(selector: string): number { // lives in Node (util/url.ts) so it is unit-tested and can't drift from the redirect // normalizer. (That drift is exactly what marked self-canonical pages non-indexable: the // canonical's literal `:` never matched the request's `%3A`.) +/** + * The page's own statement about whether it should be indexed, read off the live DOM. + * + * ONE function for both the post-navigation check and the post-settle one, deliberately. They ask + * the identical question of a page at two different moments, and the only difference that is + * supposed to exist between their answers is the page having changed its own head in between. Two + * copies of this reasoning would add a second difference nobody would notice. + * + * A canonical naming a DIFFERENT document always disowns the page — invariable, every site. A + * canonical naming this very document RE-SPELLED as another cache key ('variant') is only a + * duplicate if the site's origin cannot tell the two spellings apart, which is a property of its + * query parser, not of the URLs — so that half is config, defaulting to the historical lenient + * reading. See config.canonical.strict. Either way the reason slug is distinct, so duplicate + * spellings stay legible next to genuine mismatches. + */ +async function readIndexVerdict( + page: Page, + canonicalStrict: boolean +): Promise<{ indexable: boolean; reason: string | null }> { + const { canonicalHref, noindex } = await page.evaluate(extractIndexSignals); + const verdict = canonicalVerdict(canonicalHref, page.url()); + const disowned = verdict === 'elsewhere' || (verdict === 'variant' && canonicalStrict); + if (!noindex && !disowned) return { indexable: true, reason: null }; + return { + indexable: false, + reason: noindex ? 'noindex' : verdict === 'variant' ? 'canonical-variant' : 'canonical-mismatch', + }; +} + function extractIndexSignals(): { canonicalHref: string | null; noindex: boolean } { let canonicalHref: string | null = null; let noindex = false; diff --git a/packages/browser/test/earlySuppression.test.ts b/packages/browser/test/earlySuppression.test.ts new file mode 100644 index 0000000..466999e --- /dev/null +++ b/packages/browser/test/earlySuppression.test.ts @@ -0,0 +1,220 @@ +import { test, before, after } from 'node:test'; +import assert from 'node:assert/strict'; +import http from 'node:http'; +import type { AddressInfo } from 'node:net'; +import { renderOnce } from '../dist/renderOnce.js'; + +/** + * What a render pays for a page it is going to throw away. + * + * The settle phase IS a render's cost — on the reference deployment the scroll-settle passes alone + * are ~78% of render time and the bulk of per-render CPU. A page that ends up non-indexable pays all + * of it, has its bytes discarded, and then pays it again on every suppression recheck for as long as + * the plugin keeps the target. Two verdicts are already final right after navigation, so they are + * taken there instead. + * + * `timings.settle` is the assertion that matters throughout: `undefined` means the settle phase never + * started, which is the only direct evidence the work was actually skipped rather than merely fast. + * The existing redirect bail asserts the same way (`test/redirect.test.ts`). + * + * NOTE ON `captureNonIndexable`. `renderOnce` maps it onto `job.isFromSitemap` — the harness marks + * jobs as sitemap-listed by default so non-indexable HTML stays inspectable. That makes it exactly + * the switch these tests need: `false` is the discovered-URL shape the early bail applies to, and the + * default is the sitemap-listed shape it must never apply to. + */ + +let origin: http.Server; +let base = ''; + +// Requests the origin saw, so a test can prove a bail did not re-fetch anything. +const seen: string[] = []; + +before(async () => { + origin = http.createServer((req, res) => { + const path = req.url ?? ''; + seen.push(path); + const html = (head: string, body = 'x') => + `t${head}${body}`; + + res.setHeader('content-type', 'text/html'); + + switch (path.split('?')[0]) { + case '/noindex': + return res.end(html('')); + case '/noindex-googlebot': + return res.end(html('')); + case '/canonical-elsewhere': + return res.end(html('')); + case '/gone': + res.writeHead(404, { 'content-type': 'text/html' }); + return res.end(html('', 'not found')); + case '/server-error': + res.writeHead(503, { 'content-type': 'text/html' }); + return res.end(html('', 'try later')); + // Clean at parse time; adds its own noindex a moment later. The early check must MISS this + // (it reads the initial DOM) and the post-settle check must still catch it — that boundary is + // the entire scope of the difference between the two. + case '/noindex-late': + return res.end( + html( + '', + `

ok

` + ) + ); + default: + return res.end(html('', 'OK')); + } + }); + await new Promise((r) => origin.listen(0, '127.0.0.1', r)); + base = `http://127.0.0.1:${(origin.address() as AddressInfo).port}`; +}); + +after(async () => { + await new Promise((r) => origin.close(() => r())); +}); + +/** A discovered (non-sitemap) render — the shape the early non-indexable bail applies to. */ +const discovered = (path: string, overrides: Record = {}) => + renderOnce({ + url: `${base}${path}`, + captureNonIndexable: false, + config: { scroll: { enabled: false }, navigation: { domStableMs: 200 } }, + resourceCache: { enabled: false }, + ...overrides, + }); + +/** A sitemap-listed render — serialized even when non-indexable, so it must NOT bail early. */ +const submitted = (path: string) => + renderOnce({ + url: `${base}${path}`, + config: { scroll: { enabled: false }, navigation: { domStableMs: 200 } }, + resourceCache: { enabled: false }, + }); + +test('a noindex page the plugin discovered ends at navigation, before the settle phase', async () => { + const result = await discovered('/noindex'); + await result.close(); + + assert.equal(result.isIndexable, false); + assert.equal(result.reason, 'noindex'); + assert.equal(result.html, undefined, 'nothing to store — the plugin suppresses this target'); + assert.equal(result.timings.settle, undefined, 'the settle phase must never have started'); + assert.equal(result.statusCode, 200, 'the document itself answered 200; the page disowned itself'); +}); + +test('a `googlebot` meta counts, and the directive is case-insensitive', async () => { + const result = await discovered('/noindex-googlebot'); + await result.close(); + assert.equal(result.isIndexable, false); + assert.equal(result.reason, 'noindex'); + assert.equal(result.timings.settle, undefined); +}); + +test('a canonical naming another document ends at navigation too', async () => { + const result = await discovered('/canonical-elsewhere'); + await result.close(); + assert.equal(result.isIndexable, false); + assert.equal(result.reason, 'canonical-mismatch'); + assert.equal(result.timings.settle, undefined); +}); + +test('A SITEMAP-LISTED PAGE STILL SETTLES, because its bytes are still served', async () => { + // The exemption is load-bearing, not cautious: a sitemap-listed url is serialized even when + // non-indexable, so its settle is not wasted work and bailing would replace a served page with + // nothing at all. + const result = await submitted('/noindex'); + await result.close(); + + assert.equal(result.isIndexable, false, 'still reported non-indexable'); + assert.equal(result.reason, 'noindex'); + assert.notEqual(result.timings.settle, undefined, 'but it must have settled...'); + assert.ok(result.html, '...and produced content to serve'); +}); + +test('a non-200 ends at navigation for EVERY page, sitemap-listed included', async () => { + // Behaviour-identical by construction: a status cannot change during the settle, and the + // post-settle branch does nothing with a non-200 but report it with no content — for a + // sitemap-listed url too. So this one needs no exemption. + for (const [path, status] of [ + ['/gone', 404], + ['/server-error', 503], + ] as const) { + for (const render of [discovered, submitted]) { + const result = await render(path); + await result.close(); + assert.equal(result.statusCode, status, `${path} status`); + assert.equal(result.isIndexable, false, `${path} indexable`); + assert.equal(result.reason, 'http-error', `${path} reason`); + assert.equal(result.html, undefined, `${path} content`); + assert.equal(result.timings.settle, undefined, `${path} must not settle`); + } + } +}); + +test('a healthy page is untouched: it settles, and it serializes', async () => { + const result = await discovered('/fine'); + await result.close(); + + assert.equal(result.isIndexable, true); + assert.equal(result.reason, undefined); + assert.notEqual(result.timings.settle, undefined, 'the settle phase must still run'); + assert.match(result.html ?? '', /OK/); +}); + +test('a page that adds its noindex DURING the settle is still caught, by the post-settle check', async () => { + // The one difference between the two checks, made explicit. The early check reads the initial DOM, + // so it cannot see this — and the late check has to remain the backstop, or moving the check + // forward would silently narrow what gets suppressed at all. + const result = await discovered('/noindex-late'); + await result.close(); + + assert.equal(result.isIndexable, false, 'the late check still runs and still decides'); + assert.equal(result.reason, 'noindex'); + assert.notEqual(result.timings.settle, undefined, 'and it necessarily paid for the settle to see it'); +}); + +test('earlyNonIndexable: false restores settle-then-decide, with the same verdict', async () => { + // The kill switch has to be a revert of WHEN the decision is taken, not of the decision. + const result = await discovered('/noindex', { + config: { + scroll: { enabled: false }, + navigation: { domStableMs: 200 }, + suppression: { earlyNonIndexable: false }, + }, + }); + await result.close(); + + assert.equal(result.isIndexable, false, 'same verdict'); + assert.equal(result.reason, 'noindex', 'same reason'); + assert.notEqual(result.timings.settle, undefined, 'but it settled first'); +}); + +test('earlyErrorStatus: false likewise settles a 404 before reporting it', async () => { + const result = await discovered('/gone', { + config: { + scroll: { enabled: false }, + navigation: { domStableMs: 200 }, + suppression: { earlyErrorStatus: false }, + }, + }); + await result.close(); + + assert.equal(result.statusCode, 404); + assert.equal(result.reason, 'http-error'); + assert.notEqual(result.timings.settle, undefined); +}); + +test('a bail fetches the document exactly once — it does not re-navigate to decide', async () => { + seen.length = 0; + const result = await discovered('/noindex'); + await result.close(); + assert.equal( + seen.filter((p) => p === '/noindex').length, + 1, + 'one navigation; the verdict comes off the DOM already in front of us' + ); +});