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' + ); +});