Skip to content

feat(browser): decide suppression at navigation, not after the settle #117

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
harper-joseph wants to merge 1 commit into from
+361 −15
Closed

feat(browser): decide suppression at navigation, not after the settle #117

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions packages/browser/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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` / `<link rel=canonical>` off the _initial_
DOM rather than 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, 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
Expand Down
2 changes: 1 addition & 1 deletion packages/browser/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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": [
Expand Down
35 changes: 35 additions & 0 deletions packages/browser/src/config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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: {},
Expand Down
90 changes: 76 additions & 14 deletions packages/browser/src/renderer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 = () => {};

Expand Down Expand Up @@ -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 `<head>` 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;
}
}
Comment on lines +301 to +309

@gemini-code-assist gemini-code-assist Bot Aug 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The early non-indexable check is an optimization to avoid the overhead of the settle phase. However, calling readIndexVerdict (which performs a page.evaluate) immediately after navigation can be prone to transient Puppeteer errors such as Execution context was destroyed if the page performs rapid client-side redirects, reloads, or early history manipulation.

If an error is thrown during this early optimization check, it will propagate and fail the entire render job. Since the post-settle check remains as a robust backstop, we should wrap this early check in a try...catch block and let any errors fail-safe by falling through to the settle phase.

		if (config.suppression.earlyNonIndexable && status === 200 && !job.isFromSitemap) {
			try {
				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;
				}
			} catch (e) {
				// Early check failed (e.g. execution context destroyed). Fall back to the settle phase.
			}
		}

}

const settleStart = Date.now();

const networkIdle = () =>
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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;
Expand Down
Loading