feat(browser): decide suppression at navigation, not after the settle - #117
feat(browser): decide suppression at navigation, not after the settle#117harper-joseph wants to merge 1 commit into
Conversation
…; v1.18.0
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 <head> 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 <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an "early suppression" optimization to bypass the expensive settle phase for pages that return a non-200 status or are immediately identified as non-indexable. It adds configuration options, refactors the indexability checks into a shared helper, and includes comprehensive tests. The reviewer recommended wrapping the early non-indexable check in a try-catch block to gracefully handle transient Puppeteer evaluation errors and fall back to the settle phase instead of failing the job.
| 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; | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
}
}1 participant
What
The settle phase is a render's cost. The Kohl's render-service config records it directly: "the scroll-settle passes are ~78% of render time and the bulk of per-render CPU." A page that ends up non-indexable pays all of that, has its bytes discarded, and then pays it again on every
render.suppression.recheckfor as long as the plugin keeps the target.Two verdicts are already final immediately after navigation, so this takes them there instead of ~25 s later.
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 but set
isIndexable = false/reason: 'http-error'and return no content, for a sitemap-listed URL as well. There is no case where settling first yields a different result, which is why it needs no sitemap exemption and why turning it off buys nothing but latency. This is the free half, and 404s are a large share of a crawled corpus.2. A document that already disowns itself, when the plugin discovered it. Same
readIndexVerdictthe post-settle check uses, so it reaches 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.The exemption that is load-bearing
A sitemap-listed URL is never eligible for (2). Such a page is serialized even when non-indexable, so its settle is not wasted work — bailing would replace a served page with nothing at all. The plugin's suppression branch never sees those URLs either; only URLs it discovered are retirable that way.
util/url.ts's own comment oncanonicalVerdictalready states this contract, and the test asserts it both ways.The post-settle check remains the backstop
A page that adds its
noindexduring the settle is still caught — it just pays for the settle to be caught. Moving a check forward must not narrow what gets suppressed, only when, and there's a test with a page that injects its<meta robots>on a timer to pin that.Both checks now go through one
readIndexVerdict, because they ask the identical question of a page at two moments. The only difference that should ever exist between their answers is the page having changed in between; two copies of that reasoning would add a second difference nobody would notice.Config
Separate switches because the risk differs, as above. Both default on.
Testing
130 pass / 0 fail (120 pre-existing, untouched).
test/earlySuppression.test.tsasserts ontimings.settlebeingundefinedthroughout — the only direct evidence the phase never started, rather than merely ran fast. Same waytest/redirect.test.tspins the existing redirect bail. It covers: noindex andgooglebot-meta and canonical-elsewhere all bailing; the sitemap-listed page still settling and producing content; a non-200 bailing for sitemap-listed and discovered alike; a healthy page untouched; the late-mutation backstop; both kill switches reproducing the same verdict a phase later; and a bail navigating exactly once.Observed in the run: a bailed render is ~120–360 ms against ~700 ms for the same page settled — on a trivial test document. Real pages settle for seconds.
How to measure the win in production
Already instrumented, no new metric needed:
rendertime_msis split by candidacy, so compare thenon-candidatedistribution before and after.renderoutcome=suppressedwith its reason detail is also what says how much of the corpus this can reach — if the suppression reasons are mostlycanonical-*rather thannoindex, that is where the volume is, and both are covered here.Not in this PR
The plugin-side half — sniffing the first bytes of an origin-miss response for
noindexbefore a Target is ever created, so a suppressed URL never enters the corpus and never accrues a 7-day recheck. That one touches the hot serve path (the origin body is a stream relayed to the crawler, so it needs a bounded pass-through tee) and is worth its own review. #116'scoldlane already stops existing suppression rechecks from competing with pages that are actually served, which takes the urgency out of it.🤖 Generated with Claude Code