Uh oh!
There was an error while loading. Please reload this page.
fix(browser): read the canonical against the cache key, not the document (v1.17.0) - #90
fix(browser): read the canonical against the cache key, not the document (v1.17.0)#90harper-joseph wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a new 'canonical-variant' verdict to identify and suppress duplicate cache keys generated by re-spelled canonical URLs (such as differences between '%20' and '+'). It replaces the 'canonicalAllowsIndex' helper with 'canonicalVerdict' to categorize canonical URLs as 'self', 'variant', or 'elsewhere'. Feedback suggests a minor optimization in 'packages/browser/src/util/url.ts' to access 'current.searchParams' directly instead of redundantly instantiating a new 'URLSearchParams' object.
| return normalizeCanonicalUrl(new URL(canonicalHref, currentUrl)) === normalizeCanonicalUrl(currentUrl); | ||
| const current = new URL(currentUrl); | ||
| const canonical = new URL(canonicalHref, current); | ||
| const allowlist = [...new URLSearchParams(current.search).keys()]; |
There was a problem hiding this comment.
Since current is already a URL instance, you can directly access its searchParams property instead of redundantly instantiating a new URLSearchParams object from current.search. This avoids unnecessary parsing and object allocation.
| constallowlist=[...newURLSearchParams(current.search).keys()]; | |
| constallowlist=[...current.searchParams.keys()]; |
There was a problem hiding this comment.
Applied — current is already a URL, so that re-parsed a query string parsed a line earlier. Now current.searchParams.keys(). This code has moved to #94, which now carries the whole URL/normalization change end to end.
…ent; v1.17.0 A faceted origin resolves `+`, `%2B` and `%20` between facet values to one page and canonicalizes all of them to the `+` spelling — but each spelling is its own cache key. The self-canonical test ran on `normalizeCanonicalUrl`, whose form-encoding round-trip collapses `%20` and `+`, so a `%20`-for-`+` re-spelling read as self-canonical and became a second recurring target rendering the same bytes ~2x/day forever. Every other re-spelling in that family was already caught; this one could not be, by construction. Take the verdict on `canonicalizeUrl` instead. The consequence of a canonical verdict is `Target.suppress`, and a Target IS a cache key, so "same key" is the question worth asking, and asking it through the key-building function means the answer cannot drift from what the plugin stores. `canonicalAllowsIndex` becomes `canonicalVerdict` returning self | variant | elsewhere; 'variant' posts the new reason `canonical-variant` so a wave of duplicate spellings stays legible next to genuine mismatches. The rendered url's own param names are the allowlist for both sides, so a param the route drops can never manufacture a mismatch. NOT solved in the cache key on purpose: `+` and `%2B` are interchangeable only in separator position. In value position they are different values (measured against a live faceted origin: `Brand:ACME%2BCO` returns the brand's 52 products, `Brand:ACME+CO` returns an empty page canonicalizing elsewhere), so folding them would serve one facet's HTML under another facet's url — and re-keying the corpus orphans every cached page. Blast radius, verified in code and pinned by a new test: a sitemap-listed url is serialized even when non-indexable, so its result posts with content and `rendered` wins the outcome — the declared corpus cannot be retired by any canonical verdict, only discovered urls can. Measured on a live faceted origin: 103/103 real sitemap locs (60 catalog facet urls from 3 sitemaps, 30 PDPs, the homepage, 12 re-probed with a mobile UA) are self-canonical under the strict comparison, i.e. zero would-be suppressions. Plugin change is comment-only (the reason string flows through as data) — no release needed. Refs #84
… config
This plugin serves websites of all types, so a rule that depends on how one origin
parses its query cannot be hardcoded. Split the verdict along that line:
- A canonical naming a DIFFERENT document disowns the page. Invariable, every site,
not configurable.
- A canonical naming this very document RE-SPELLED as another cache key ('variant')
is a duplicate only if that origin's parser cannot tell the spellings apart —
a form-decoding origin cannot, an RFC-3986 one can. So: `canonical.strict`,
default false, which reproduces the historical lenient reading byte-for-byte.
The doc comment carries the one-request discriminator that settles it for a given
parameter across every URL (ask for `?f=A%2BB`, then `?f=A+B`; if the origin resolves
the second as a space it form-decodes), so an operator can decide without a crawl.
Tests pin both directions plus the invariable half under either setting.c9e9be3 to
9424b15Compare…s query From review on #90 — `current` is already a URL, so `new URLSearchParams(current.search)` re-parsed a query string that was parsed a line earlier.
harper-joseph
commented
Aug 13, 2026
Superseded by #94, which now carries the whole URL/normalization change end to end — this commit is merged into that branch unchanged, plus |
Closes the one duplicate-target class in #84 that nothing upstream could catch.
What #84's encoding variants actually do, measured
Probed a live faceted origin directly (bypass token, 100+ requests). Every re-spelling of one
catalog facet URL returns the same page — same product set, same title — and declares a
<link rel="canonical">pointing at the sitemap spelling:canonical-mismatchcanonical-mismatch%2Bcanonical-mismatch%252B(double-encoded)canonical-mismatch%20So four of the five classes were already self-limiting: suppressed on first render, rechecked
at
render.suppression.recheckInterval(7d), deleted aftermaxStrikes(4) ≈ 28 days. That is~0.18 renders/day per junk URL, not the "~2×/day forever" #84 assumes — I'll correct the issue
body separately.
The
%20case could not be caught:normalizeCanonicalUrldoes aURLSearchParamsround-tripthat collapses
%20and+on both sides, so the page read as self-canonical whilecanonicalizeUrl(correctly) kept the two spellings as different keys. That variant became afull-privilege recurring target holding bytes identical to its twin, forever.
The fix
Split along what is invariable vs what is a property of the site. This plugin serves websites
of all types, so the half that depends on how one origin parses its query is config, not a
hardcoded assumption:
canonical-mismatch)canonical-variant)canonical.strict, defaultfalsestrict: falsereproduces today's lenient reading byte-for-byte, so upgrading changes nothinguntil a deployment opts in. Whether two spellings are one resource is decided by the origin's
query parser — a form-decoding origin cannot tell
+from%20, an RFC-3986 one can — and thatis settled by one request per allowlisted parameter, for every URL: ask for a value with a
literal plus (
?f=A%2BB), then the same value with a raw+(?f=A+B); if the origin resolvesthe second as a space, it form-decodes and the two spellings can never name different resources.
The discriminator is in the
CanonicalConfigdoc comment so an operator can decide without a crawl.Take the verdict on
canonicalizeUrl— the function the cache key is actually built with.The consequence of a canonical verdict is
Target.suppress, and a Target is a cache key, so"does this name the same key" is the question worth asking, not "does this name the same
document".
canonicalAllowsIndex→canonicalVerdictreturningself | variant | elsewhere:variant— same document re-spelled as a different key → new reasoncanonical-variant,so a wave of duplicate spellings reads differently from an origin disowning its own pages.
elsewhere— unchanged behaviour, stillcanonical-mismatch.drops can never manufacture a mismatch.
Why this is not fixed in the cache key
+and%2Bare interchangeable only in separator position. In value position they aredifferent values — measured on the live origin,
Brand:ACME%2BCOreturns that brand's 52products while
Brand:ACME+COreturns an empty page canonicalizing elsewhere. Folding them incanonicalizeUrlwould serve one facet's HTML under another facet's URL (the canonical half isalso the origin-fetch/navigation URL), and re-keying orphans every cached page in the corpus.
Safety
The failure mode worth worrying about is an origin whose canonicals are spelled differently
from its own sitemap (writing
+for spaces is common): every page would be flagged, andTarget.suppressdeletes the cached page immediately and the target after 4 strikes.That cannot happen to a declared corpus, on any deployment. A sitemap-listed URL is
serialized even when non-indexable, so its result posts with content and
renderedwins inRenderJob.outcome— the plugin's suppression branch is never reached. Only URLs the plugindiscovered are retirable by a canonical verdict, which is exactly #84's subject. A new test
pins that guarantee so a refactor can't quietly remove it. For such an origin the only visible
effect on the declared corpus is
isIndexable: falseon the stored page (a debug header and anadmin column — it does not gate serving).
Deployment-specific evidence: 103/103 real sitemap locs are self-canonical under the strict
comparison — 60 catalog facet URLs sampled across 3 catalog sitemaps, 30 PDPs across 2 product
sitemaps, the homepage, and 12 re-probed with a mobile UA (no per-device canonical divergence).
Zero would-be suppressions.
Tests
npm run lint,format:check, browser 117 pass, plugin 642 pass. New: the%20re-spellingverdict, the
%2B-in-a-value regression guard (must stay indexable), route-dropped paramscan't mismatch, plus two renderer-level integration tests (
canonical-variantreason; thesitemap blast-radius guarantee).
The plugin change is comment-only — the reason string flows through as data — so no plugin
release is needed.
Refs #84
Measured in production
The existing mechanism is live and busy. Suppression reasons from the four nodes'
hdb.log,24h to 2026-08-12T22:52Z (two lines per URL — desktop + mobile):
canonical-mismatchnoindexhttp-errorSo ~6.5k distinct URLs/day already get a canonical verdict. On one node, 85% of the distinct
canonical-mismatchURLs are/product/prd-N/product.jsp— a placeholder slug, the class thatwas already caught.
The class this PR adds, sized. A 30-minute
read_audit_logwindow onrender_service.Target(615 distinct discovery-created URLs, 93% catalog — consistent with the ~27k/day in #84) contains
27 URLs carrying the space-written-as-
+signature: a multi-word facet value spelledSilhouette:Bath+Rugswhere the canonical spellsBath%20Rugs. Probing all 27 against the origin:elsewhere)That extrapolates to ~1,300 such URLs/day, ~900/day of them newly retired. Each was a recurring
target on two device keys at the 24h floor — ~2 renders/day, forever, for bytes the canonical
spelling already describes.
Note the direction: production's real shape is
+written where the canonical has%20, themirror of the fixture in
url.test.ts. Same equivalence class, same collapse — the fixture isjust written the other way round.
End-to-end confirmation on one pair, via
POST /prerender_admin/explain: the discovered+spelling is a live recurring target (
scheduled: true, suppressed: false); its%20canonicaltwin has no target at all.
Which is worth stating plainly: suppression does not hand coverage to the twin. Only 5 of the 25
differing canonicals are in a sitemap, so for most of them neither spelling ends up prerendered
and bots get the origin proxy for that page. That is the correct outcome — the origin disowns the
+spelling and Google folds it into the canonical regardless — but "adopt the canonical as anew target" is deliberately not part of this PR: the origin fabricates junk canonicals from
junk input (see #84), so adoption amplifies rather than heals.
All production access for this was read-only: log greps over SSH, one
read_audit_log, andexplain/overviewreads. The origin probes carry the bypass token, so they go straight tothe origin and never reach the plugin — no targets were created by measuring.