From 6d4c78c367296dab81319d9ed91e47bdaa2014e7 Mon Sep 17 00:00:00 2001 From: Joe Date: Wed, 26 Aug 2026 14:17:22 -0400 Subject: [PATCH 1/3] fix(plugin): scope the demand ladder's visit signal to bots and to targets it can act on; v0.61.0 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The visit filter behind `render.demand` recorded every prerender-class request from every bot, including URLs that own no Target and never will. Both are silent-failure paths: the Bloom ring's false-positive rate is `fill^k`, so a saturated ring does not error — it answers "visited" for everything and the ladder promotes the whole corpus to its floor, which looks exactly like a working ladder from every metric except `demand_fill`. Measured on a production deployment: terminal fill 0.986, i.e. ~91% false positives, against a `bitsPerSlice` sized for ~100k distinct URLs per slice. Two gates, both mirroring precedent that already exists for discovery: - `render.demand.bots`, the same shape and matching rules as `ingress.discoveryBots`. Cadence is render budget, so whoever counts decides where that budget goes; a scraper walking the corpus breadth-first promotes pages no search engine asked for. - A rotation gate. `recordVisit` moves behind `resolveResource` so it can read `cacheStatus`: any status that FOUND a page row counts (a page row exists only where a Target does, so demoting `stale`/`invalidated`/`blob-missing` would be a false negative), a `miss` counts only when it is a cacheable 200 on a route that mints targets, and `bypass`/`skip` never looked so they cannot claim one. Also corrects the reading guidance for `demand_fill` in both the metric catalog and METRICS.md: it is a sawtooth that resets each slice, so the documented "average it" reports the middle of the ramp — a mean of 0.75 (13% false) and a peak of 0.986 (91%) are the same slice, and only the peak is the answer. --- package-lock.json | 2 +- packages/plugin/METRICS.md | 14 +- packages/plugin/package.json | 2 +- packages/plugin/src/configSchema.js | 29 ++- .../plugin/src/http_handlers/bot_request.js | 55 +++-- packages/plugin/src/metrics.js | 12 +- packages/plugin/src/util/userAgent.js | 48 ++++- packages/plugin/test/demandSignal.test.js | 192 ++++++++++++++++++ packages/plugin/test/userAgent.test.js | 35 +++- 9 files changed, 358 insertions(+), 31 deletions(-) create mode 100644 packages/plugin/test/demandSignal.test.js diff --git a/package-lock.json b/package-lock.json index f18df952..d8db69c9 100644 --- a/package-lock.json +++ b/package-lock.json @@ -8570,7 +8570,7 @@ }, "packages/plugin": { "name": "@harperfast/prerender", - "version": "0.60.0", + "version": "0.61.0", "license": "Apache-2.0", "dependencies": { "fast-xml-parser": "^5.0.9", diff --git a/packages/plugin/METRICS.md b/packages/plugin/METRICS.md index 230820f1..97510980 100644 --- a/packages/plugin/METRICS.md +++ b/packages/plugin/METRICS.md @@ -139,8 +139,18 @@ Notes that bite: `analytics.recordUnmatched` for requests whose UA yields no name). No rows ≠ no traffic. - **`queue_health`'s snapshot series and `prerender_ops`' `demand_*` are gauges on a slow cadence**, one set per node (`management.backlogSnapshotInterval`, `render.demand.statsInterval`). Chart the latest value; - never sum a gauge over time. `overdue` sums across nodes and `fill` averages. The ladder's - decision counters are NOT gauges — they are per-interval counts and must be summed. + never sum a gauge over time. `overdue` sums across nodes; `fill` takes its **peak** (below). The + ladder's decision counters are NOT gauges — they are per-interval counts and must be summed. +- **`demand_fill` is a sawtooth — read max/p95, never the mean.** It is the set-bit fraction of the + newest visit-filter slice, which resets to ~0 at each `sliceMs` rollover and climbs until the next + one, so an average reports the middle of the ramp while the ladder's decisions land all along it, + including at the top. The false-positive rate is `fill^k` (k = `render.demand.hashes`, 7 by + default), and that exponent is what makes the distinction matter rather than nitpicking: a mean of + 0.75 implies 13% false positives, the 0.986 peak of the same slice implies **91%**. A saturated ring + reads "visited" for everything, so the ladder promotes the whole corpus to its floor — cadence + stops being demand-driven and no other metric says so. Fix it by cutting what goes in + (`render.demand.bots`, and the rotation gate that keeps URLs owning no Target out) before raising + `bitsPerSlice`, whose row replicates on every flush. - **`queue_health.overdue` includes in-flight renders** (a leased row keeps its past due time), so its healthy floor is the in-flight count, not zero — and it is not comparable with numbers from before v0.34.0. The scan is capped by `management.scanCap`: a backlog past the cap reports the cap. diff --git a/packages/plugin/package.json b/packages/plugin/package.json index 8736a15a..2888c7af 100644 --- a/packages/plugin/package.json +++ b/packages/plugin/package.json @@ -1,6 +1,6 @@ { "name": "@harperfast/prerender", - "version": "0.60.0", + "version": "0.61.0", "type": "module", "description": "Configurable Harper plugin for prerendering pages for bots and crawlers", "license": "Apache-2.0", diff --git a/packages/plugin/src/configSchema.js b/packages/plugin/src/configSchema.js index b46e8d8e..7bbc220a 100644 --- a/packages/plugin/src/configSchema.js +++ b/packages/plugin/src/configSchema.js @@ -1195,6 +1195,18 @@ export const configSchema = group('Prerender plugin configuration.', { 'nothing.', { min: 0, max: 1 } ), + bots: option( + ['*'], + 'Bots whose visits count as demand, by the bot name the analytics registry resolves ' + + "(analytics.bots / derived names / the literal 'other'), compared case-insensitively. " + + "['*'] (default) counts every bot; [] counts none, so every target rests at its route " + + 'cadence; a list counts exactly those. Same shape and matching rules as ' + + '`ingress.discoveryBots`, and worth setting for a related reason: cadence is render ' + + 'budget, so whoever this counts decides where that budget goes, and a third-party ' + + 'crawler walking the corpus breadth-first promotes pages no search engine asked for. ' + + 'It is also the first lever on ring saturation — see `bitsPerSlice`.', + { itemType: 'string' } + ), sliceMs: option( 6 * HOUR, 'Time resolution of the visit ring. Cannot be coarser than the fastest rung or that rung ' + @@ -1211,8 +1223,21 @@ export const configSchema = group('Prerender plugin configuration.', { 1 << 20, 'Bloom filter bits per ring slice, rounded UP to a power of two at use (byte sizing and ' + 'probe spread both require it). ~1M bits holds ~100k distinct URLs per slice at ~1% ' + - 'false positives. False positives promote a page nobody asked for — wasted renders, ' + - 'never staleness — and there are no false negatives.', + 'false positives. There are no false negatives, so a visited page is never demoted for ' + + 'lack of evidence.\n\n' + + 'SIZE THIS AGAINST THE DISTINCT-URL RATE, and treat overshoot as a correctness problem ' + + 'rather than a cost one. A slice holding n distinct URLs fills to `1 - e^(-kn/m)`, and ' + + 'the false-positive rate is `fill^k` — so it degrades not gradually but off a cliff: at ' + + 'the default k=7 and m=1M, 100k URLs fills to 0.49 (~0.7% false), 320k to 0.88 (~41%), ' + + '640k to 0.986 (~91%). Past that the ring answers "visited" for essentially everything, ' + + 'the ladder promotes the whole corpus to its floor, and nothing about the failure is ' + + 'loud — the cadence just stops being demand-driven. Watch `demand_fill` at its PEAK, ' + + 'not its mean (it is a sawtooth that resets each slice).\n\n' + + 'Raising this is the last lever, not the first: the row is `bitsPerSlice / 8` bytes and ' + + 'REPLICATES on every flush, which is how the per-worker version of this write produced ' + + 'a transaction log two orders of magnitude larger than the state it carried. Cut what ' + + 'goes in first — `bots` above, and the rotation gate in `recordDemand` — since a URL ' + + 'the ladder can never act on is pure fill.', { min: 1024 } ), hashes: option(7, 'Bloom hash count (k).', { min: 1, max: 32 }), diff --git a/packages/plugin/src/http_handlers/bot_request.js b/packages/plugin/src/http_handlers/bot_request.js index df69f1e7..593742ae 100644 --- a/packages/plugin/src/http_handlers/bot_request.js +++ b/packages/plugin/src/http_handlers/bot_request.js @@ -1,6 +1,6 @@ import { setTimeout as sleep } from 'node:timers/promises'; import { CacheKey } from '../util/cacheKey.js'; -import { getBotName, botMayDiscover } from '../util/userAgent.js'; +import { getBotName, botMayDiscover, botCountsAsDemand } from '../util/userAgent.js'; import { isPrerenderCandidate } from '../util/indexSignals.js'; import { canonicalizeUrl } from '../util/url.js'; import { config } from '../config.js'; @@ -49,17 +49,6 @@ export async function handleBotRequest(request) { recordCrawl(request.botName, cacheUrl); } - // Demand signal for the render ladder. Deliberately OUTSIDE the `recordBots` gate: that - // gate is about analytics volume, whereas this feeds scheduling — a deployment that turns - // analytics down must not silently demote its whole corpus for lack of observed traffic. - // Keyed on the device-free URL, since cadence resolves per URL and dropping the device - // split halves the distinct count the filter carries. No-op unless render.demand.enabled. - // Prerender-class only: those are the only keys the ladder ever probes (proxied and - // unclassified paths own no Target), and recording the plentiful junk URLs the CDN - // over-forwards would only raise the filter's fill factor — at high fill the - // false-positive rate explodes and the ladder degenerates into promoting everything. - if (routeClass === PRERENDER) recordVisit(cacheUrl); - // Debug/observability info surfaced as x-harper-* response headers (only when the // debug header is present). `route` is the matched route entry, if any; `routeClass` // decides whether this request is cached and scheduled at all. @@ -67,6 +56,7 @@ export async function handleBotRequest(request) { const resource = await resolveResource({ request, url, cacheUrl, deviceType, routeClass, info }); maybeSchedule(resource, routeClass, route, request.botName); + recordDemand({ resource, routeClass, route, cacheUrl, botName: request.botName, cacheStatus: info.cacheStatus }); // DEMAND-DRIVEN HEAL, default off and a no-op unless an invalidation is what cost this request // its cache serve (`info.invalidatedBy` is set only when the epoch was consulted, which happens // only when the page would otherwise have been served). Detached inside, like maybeSchedule — @@ -374,6 +364,47 @@ function maybeSchedule(resource, routeClass, route, botName) { setImmediate(handlePageScheduling, resource); } +// Cache statuses that never looked for a page row, so they can neither prove nor disprove that a +// Target exists: a non-GET (`bypass`) and a deliberate cache skip (`skip`, i.e. render-now). +// Excluding them is right on the second count too — a forced render is an operator action, not +// crawler demand, and should not buy the page a faster rung. +const NO_PAGE_LOOKUP = new Set(['bypass', 'skip']); + +// Demand signal for the render ladder (util/visitFilter.js -> util/demandLadder.js). Keyed on +// the device-free URL, since cadence resolves per URL and dropping the device split halves the +// distinct count the filter carries. No-op unless `render.demand.enabled`. +// +// DELIBERATELY OUTSIDE the `recordBots` analytics gate: that gate is about analytics volume, +// whereas this feeds scheduling — turning analytics down must not silently demote the corpus for +// lack of observed traffic. +// +// BOTH GATES BELOW EXIST TO HOLD THE RING'S FILL FACTOR DOWN, and that is a correctness concern, +// not a tidiness one. Fill sets the false-positive rate (~fill^k), and a saturated ring does not +// fail loudly — it answers "visited" for everything, so the ladder promotes the whole corpus to +// its floor and the visit signal stops being a signal. `bitsPerSlice` is sized for the URLs the +// ladder can actually act on, so anything else recorded here is spent budget. +// +// ROTATION. The ladder only ever probes URLs that own a Target. `cacheStatus` is the sharper +// test than `resource.miss`: 'stale', 'invalidated', 'blob-missing' and 'blob-timeout' all +// served from the origin but FOUND a page row, and a page row exists only where a Target does, +// so treating those as unvisited would demote pages that are merely late. 'miss' is the one +// status that proves nothing was found, and it still counts when THIS request is what puts the +// URL into the rotation — a cacheable 200 on a route that mints targets. It does not when the +// route is discovery-gated (combinatorial facet URLs, which own no Target and never will) or +// the origin returned no page (404s for URLs that predate the deployment). +// +// BOT. `render.demand.bots`, mirroring `ingress.discoveryBots`. Cadence is render budget, so a +// deployment usually wants it allocated by the engines it serves rather than by every crawler +// that walks the corpus. +export function recordDemand({ resource, routeClass, route, cacheUrl, botName, cacheStatus }) { + if (routeClass !== PRERENDER) return; + const owned = cacheStatus !== 'miss' && !NO_PAGE_LOOKUP.has(cacheStatus); + const minting = cacheStatus === 'miss' && resource.statusCode === 200 && route?.discoverTargets !== false; + if (!owned && !minting) return; + if (!botCountsAsDemand(botName)) return; + recordVisit(cacheUrl); +} + // On-demand render: force an immediate one-off render and wait for the fresh result, // bypassing both the cache and the origin proxy. Returns { resource, renderNowStatus } // where renderNowStatus is 'hit' (fresh render served) or 'timeout' (fell back). diff --git a/packages/plugin/src/metrics.js b/packages/plugin/src/metrics.js index 7a76beb8..f8241dc0 100644 --- a/packages/plugin/src/metrics.js +++ b/packages/plugin/src/metrics.js @@ -465,9 +465,15 @@ export const METRICS = Object.freeze({ '(promoted/demoted/held/skipped_cold/single_rung/promoted_fast/fast/graded) are per-interval/per-run counts whose `total` is the meaningful ' + 'sum (`count` is flushes/runs); serve_error, page_age_negative, invalidation_error, ' + 'invalidation_reenqueue, probe_canary_trip, probe_invalidated and discovery_gated are counters; config_warnings is a slow gauge (latest value); ' + - 'demand_fill is a per-worker gauge — average it, never sum ' + - '(fill = set-bit fraction of the newest visit-filter slot; a k=7 probe false-positives at ~fill^7, ' + - 'and false positives promote pages nobody visited — watch it before trusting the histogram). ' + + 'demand_fill is a per-worker gauge — never sum it, and READ ITS PEAK, NOT ITS MEAN. It is the ' + + 'set-bit fraction of the newest visit-filter slot, which resets to ~0 at every slice rollover ' + + 'and climbs until the next one, so it is a sawtooth: averaging over a window reports the middle ' + + 'of the ramp while the decisions that matter are made at the top of it. A k=7 probe ' + + 'false-positives at ~fill^7, so a mean of 0.75 (13%) and a peak of 0.986 (91%) are the same slice ' + + 'and only the second one is the answer — take max/p95 across buckets. False positives promote ' + + 'pages nobody visited, and a saturated ring promotes the whole corpus to its floor without ' + + 'raising any other alarm: watch this before trusting the histogram, and see ' + + '`render.demand.bitsPerSlice` for what to do when it is high. ' + 'unrouted’s bucket slot is bounded by ingress.report.maxBuckets per class. The per-level ladder ' + 'histogram exists only in the demand-ladder log line.', dimensions: { diff --git a/packages/plugin/src/util/userAgent.js b/packages/plugin/src/util/userAgent.js index e003187d..afb9544f 100644 --- a/packages/plugin/src/util/userAgent.js +++ b/packages/plugin/src/util/userAgent.js @@ -179,17 +179,21 @@ export function deriveBotName(ua) { return null; } -// ---- discovery allowlist ------------------------------------------------------------------- +// ---- bot allowlists ------------------------------------------------------------------------ -let discoverySet = null; // lowercase Set, or null meaning "every bot" ('*' present) -let discoveryFrom; // the config array the current set was built from - -const compileDiscovery = (bots) => { +// Shared by the two allowlists below: `['*']` compiles to null, meaning "every bot", and +// anything else to a lowercase Set. Both cache on the IDENTITY of the config array they were +// built from — applyOptions replaces config with a fresh object on every change, so an identity +// check is enough to notice an edit and cheap enough for the request path. +const compileAllowlist = (bots) => { const names = (Array.isArray(bots) ? bots : []).filter((name) => typeof name === 'string' && name !== ''); if (names.some((name) => name === '*')) return null; return new Set(names.map((name) => name.toLowerCase())); }; +let discoverySet = null; // lowercase Set, or null meaning "every bot" ('*' present) +let discoveryFrom; // the config array the current set was built from + /** * May a visit labeled `botName` create a NEW target (traffic discovery)? * @@ -200,15 +204,41 @@ const compileDiscovery = (bots) => { * so a config spelling like 'googlebot' still matches the registry's 'Googlebot'. * * This gates CREATION only. Serving is untouched (an unminted URL still proxies to the - * origin), and so is everything keyed on targets that already exist: the demand ladder's - * visit signal, invalidation reenqueue, and the sitemap pipeline (its targets are declared, - * not discovered). + * origin), and so are invalidation reenqueue and the sitemap pipeline (its targets are + * declared, not discovered). The demand ladder's visit signal has its OWN allowlist — + * `botCountsAsDemand` below — because the two questions are different: minting corpus is + * permanent, while counting demand only reallocates cadence within a budget that already + * exists, so a deployment can reasonably answer them differently. */ export const botMayDiscover = (botName) => { if (config.ingress.discoveryBots !== discoveryFrom) { - discoverySet = compileDiscovery(config.ingress.discoveryBots); + discoverySet = compileAllowlist(config.ingress.discoveryBots); discoveryFrom = config.ingress.discoveryBots; } if (discoverySet === null) return true; return typeof botName === 'string' && discoverySet.has(botName.toLowerCase()); }; + +let demandSet = null; // lowercase Set, or null meaning "every bot" ('*' present) +let demandFrom; // the config array the current set was built from + +/** + * Does a visit labeled `botName` count as demand for the render ladder? + * + * `render.demand.bots` has the same shape and matching rules as `ingress.discoveryBots`: + * `['*']` (default) counts every bot, `[]` counts none (every target rests at its route + * cadence), a list counts exactly those, compared case-insensitively. + * + * Cadence is render budget, so whoever this counts decides where that budget goes: a scraper + * walking the corpus breadth-first promotes pages no search engine asked for. It also bounds + * what reaches the Bloom ring, whose false-positive rate rises with fill — see `recordDemand` + * in http_handlers/bot_request.js for why that is a correctness concern and not just a cost. + */ +export const botCountsAsDemand = (botName) => { + if (config.render.demand.bots !== demandFrom) { + demandSet = compileAllowlist(config.render.demand.bots); + demandFrom = config.render.demand.bots; + } + if (demandSet === null) return true; + return typeof botName === 'string' && demandSet.has(botName.toLowerCase()); +}; diff --git a/packages/plugin/test/demandSignal.test.js b/packages/plugin/test/demandSignal.test.js new file mode 100644 index 00000000..a7cb202d --- /dev/null +++ b/packages/plugin/test/demandSignal.test.js @@ -0,0 +1,192 @@ +import { test, before, beforeEach } from 'node:test'; +import assert from 'node:assert/strict'; + +/** + * recordDemand — which requests are allowed to become demand for the render ladder. + * + * The ladder spends render budget on whatever this records, and it can only ever act on URLs + * that own a Target, so both gates here are load-bearing rather than tidying. They also hold + * down the Bloom ring's fill factor, and that failure mode is silent: the false-positive rate + * is `fill^k`, so a saturated ring answers "visited" for everything and the ladder promotes the + * whole corpus to its floor with no other metric moving. + * + * The properties pinned here: + * - a cache-served request is demand, and so is every status that FOUND a page row but served + * from the origin anyway (swr, stale, invalidated, blob-missing) — a page row exists only + * where a Target does, so demoting those for lack of evidence would be a false negative; + * - a `miss` counts only when this request is what puts the URL into the rotation: a + * cacheable 200 on a route that mints targets; + * - a miss on a discovery-gated route does NOT count (it owns no Target and never will), and + * neither does a non-200 (an origin 404 is not a page); + * - non-prerender classes never count, since they own no Target at all; + * - `render.demand.bots` gates on the resolved bot name. + * + * The assertions go through the real visitFilter rather than a stub, so a wiring change that + * records the wrong URL — the cache key instead of the device-free URL, say — fails here. + */ + +const rows = new Map(); +const sabs = new Map(); + +let applyOptions, recordDemand; +let flushSlices, refreshMerged, visitedWithin, resetVisitFilter; + +const H = 60 * 60 * 1000; +const URL_A = 'https://example.com/product/prd-1/a.jsp'; + +before(async () => { + globalThis.Resource = class {}; + globalThis.server = { hostname: 'node-a', workerIndex: 1, nodes: [], config: { http: { port: 9926 } } }; + globalThis.logger = { debug() {}, info() {}, warn() {}, error() {} }; + globalThis.databases = { + coordination: { + SharedBuffer: { + primaryStore: { + getUserSharedBuffer(key, initial) { + let buf = sabs.get(key); + if (!buf) sabs.set(key, (buf = initial)); + return buf; + }, + tryLock: () => true, + unlock() {}, + }, + }, + }, + crawl_stats: { + VisitFilter: { + async get(id) { + const row = rows.get(id); + return row ? { ...row } : null; + }, + async put(id, data) { + rows.set(id, { id, ...data }); + }, + async delete(id) { + rows.delete(id); + }, + async search({ conditions = [] } = {}) { + const out = []; + for (const row of rows.values()) { + const ok = conditions.every((c) => + c.comparator === 'less_than' + ? row.slot < c.value + : c.comparator === 'greater_than' + ? row.slot > c.value + : row.slot >= c.value + ); + if (ok) out.push({ ...row }); + } + return out; + }, + }, + }, + render_service: { Target: class {}, QueueControl: class {} }, + render_schedule: { RenderSchedule: class {} }, + page_cache: { PrerenderedPage: class {} }, + }; + + ({ applyOptions } = await import('../src/config.js')); + ({ recordDemand } = await import('../src/http_handlers/bot_request.js')); + ({ flushSlices, refreshMerged, visitedWithin, resetVisitFilter } = await import('../src/util/visitFilter.js')); +}); + +// `applyOptions` resolves each group against the schema, so a partial `demand` block resets its +// siblings to their defaults — `enabled` back to false, which silently no-ops `recordVisit` and +// makes every assertion below pass for the wrong reason. Always send the whole group. +const setDemand = (overrides = {}) => + applyOptions({ + ingress: { routes: [] }, + render: { + demand: { enabled: true, sliceMs: H, slices: 16, bitsPerSlice: 1 << 20, hashes: 7, bots: ['*'], ...overrides }, + }, + }); + +beforeEach(() => { + rows.clear(); + sabs.clear(); + resetVisitFilter(); + setDemand(); +}); + +/** Record one request, then round-trip the ring so `visitedWithin` can see it. */ +async function wasRecorded(args, url = URL_A) { + recordDemand({ + resource: { statusCode: 200 }, + routeClass: 'prerender', + route: {}, + cacheUrl: url, + botName: 'Googlebot', + ...args, + }); + await flushSlices(); + await refreshMerged(); + return visitedWithin(url, 2 * H); +} + +test('a cache serve is demand', async () => { + assert.equal(await wasRecorded({ cacheStatus: 'hit' }), true); +}); + +test('every status that FOUND a page row is demand, even when the body came from the origin', async (t) => { + // A page row exists only where a Target does, so these must not be treated as unvisited — + // that is the false-negative direction, and it demotes pages that are merely late. + for (const cacheStatus of ['swr', 'stale', 'invalidated', 'blob-missing', 'blob-timeout', 'peer-rescue']) { + await t.test(cacheStatus, async () => { + resetVisitFilter(); + rows.clear(); + sabs.clear(); + assert.equal(await wasRecorded({ cacheStatus, resource: { statusCode: 200 } }), true); + }); + } +}); + +test('a miss counts when THIS request is what mints the target: a cacheable 200 on a minting route', async () => { + assert.equal(await wasRecorded({ cacheStatus: 'miss', resource: { statusCode: 200 }, route: {} }), true); +}); + +test('a miss on a discovery-gated route is NOT demand — it owns no target and never will', async () => { + assert.equal( + await wasRecorded({ cacheStatus: 'miss', resource: { statusCode: 200 }, route: { discoverTargets: false } }), + false + ); +}); + +test('a gated route still counts once the URL has a page row', async () => { + // The gate stops target CREATION, not cadence: a sitemap-declared URL on a gated route is a + // real target and must keep earning its rung. + assert.equal(await wasRecorded({ cacheStatus: 'hit', route: { discoverTargets: false } }), true); +}); + +test('a missed non-200 is NOT demand — an origin 404 is not a page', async () => { + assert.equal(await wasRecorded({ cacheStatus: 'miss', resource: { statusCode: 404 } }), false); + assert.equal(await wasRecorded({ cacheStatus: 'miss', resource: { statusCode: 301 } }), false); +}); + +test('statuses that never looked for a page row cannot claim one', async () => { + // 'bypass' is a non-GET; 'skip' is render-now, which is an operator action rather than + // crawler demand and must not buy the page a faster rung. + assert.equal(await wasRecorded({ cacheStatus: 'bypass', resource: { statusCode: 200 } }), false); + assert.equal(await wasRecorded({ cacheStatus: 'skip', resource: { statusCode: 200 } }), false); +}); + +test('a non-prerender class is never demand', async () => { + assert.equal(await wasRecorded({ routeClass: 'passthrough', cacheStatus: 'hit' }), false); + assert.equal(await wasRecorded({ routeClass: 'unclassified', cacheStatus: 'hit' }), false); +}); + +test('render.demand.bots gates the signal on the resolved bot name', async () => { + setDemand({ bots: ['Googlebot'] }); + assert.equal( + await wasRecorded({ cacheStatus: 'hit', botName: 'AhrefsBot' }, 'https://example.com/product/prd-2/a.jsp'), + false + ); + assert.equal(await wasRecorded({ cacheStatus: 'hit', botName: 'Googlebot' }), true); +}); + +test('the device-free URL is what gets recorded, not the cache key', async () => { + // Cadence resolves per URL; recording per device would double the distinct count the ring + // carries for no added resolution. + await wasRecorded({ cacheStatus: 'hit' }, URL_A); + assert.equal(visitedWithin(URL_A, 2 * H), true); + assert.equal(visitedWithin(`${URL_A}|desktop`, 2 * H), false); +}); diff --git a/packages/plugin/test/userAgent.test.js b/packages/plugin/test/userAgent.test.js index 9bb5e7a5..0786e134 100644 --- a/packages/plugin/test/userAgent.test.js +++ b/packages/plugin/test/userAgent.test.js @@ -1,7 +1,7 @@ import { test, beforeEach } from 'node:test'; import assert from 'node:assert/strict'; import { applyOptions } from '../src/config.js'; -import { getBotName, botMayDiscover } from '../src/util/userAgent.js'; +import { getBotName, botMayDiscover, botCountsAsDemand } from '../src/util/userAgent.js'; // Minimal stand-in for a WHATWG Headers object. const headers = (map) => ({ get: (k) => map[k.toLowerCase()] ?? null }); @@ -151,3 +151,36 @@ test('botMayDiscover: an empty list turns traffic discovery off for every bot', assert.equal(botMayDiscover('Googlebot'), false); assert.equal(botMayDiscover('other'), false); }); + +test('botCountsAsDemand: the default counts every bot', () => { + assert.equal(botCountsAsDemand('Googlebot'), true); + assert.equal(botCountsAsDemand('other'), true); + assert.equal(botCountsAsDemand('Anything At All'), true); +}); + +test('botCountsAsDemand: a list gates case-insensitively and follows a live config change', () => { + applyOptions({ render: { demand: { bots: ['googlebot', 'Bingbot'] } } }); + assert.equal(botCountsAsDemand('Googlebot'), true); + assert.equal(botCountsAsDemand('BINGBOT'), true); + assert.equal(botCountsAsDemand('AhrefsBot'), false); + assert.equal(botCountsAsDemand('other'), false); + assert.equal(botCountsAsDemand(undefined), false); + applyOptions({ render: { demand: { bots: ['*'] } } }); + assert.equal(botCountsAsDemand('AhrefsBot'), true); +}); + +test('botCountsAsDemand: an empty list stops the ladder seeing any demand at all', () => { + applyOptions({ render: { demand: { bots: [] } } }); + assert.equal(botCountsAsDemand('Googlebot'), false); + assert.equal(botCountsAsDemand('other'), false); +}); + +// The two allowlists share a compile helper but must not share STATE: editing one cannot be +// allowed to answer for the other, which is exactly what a single cached set would do. +test('the discovery and demand allowlists are independent', () => { + applyOptions({ ingress: { discoveryBots: ['Googlebot'] }, render: { demand: { bots: ['Bingbot'] } } }); + assert.equal(botMayDiscover('Googlebot'), true); + assert.equal(botMayDiscover('Bingbot'), false); + assert.equal(botCountsAsDemand('Googlebot'), false); + assert.equal(botCountsAsDemand('Bingbot'), true); +}); From d665621252cb4959d7694dd42d08486ceb248820 Mon Sep 17 00:00:00 2001 From: Joe Date: Wed, 26 Aug 2026 14:19:09 -0400 Subject: [PATCH 2/3] docs(plugin): name the deliberate divergence between demand_fill and crawl breadth --- packages/plugin/METRICS.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/packages/plugin/METRICS.md b/packages/plugin/METRICS.md index 97510980..69e8b919 100644 --- a/packages/plugin/METRICS.md +++ b/packages/plugin/METRICS.md @@ -151,6 +151,15 @@ Notes that bite: stops being demand-driven and no other metric says so. Fix it by cutting what goes in (`render.demand.bots`, and the rotation gate that keeps URLs owning no Target out) before raising `bitsPerSlice`, whose row replicates on every flush. +- **`demand_fill` and crawl breadth measure different populations on purpose, and are expected to + diverge.** Crawl breadth (`GET crawl-breadth`) answers _what did crawlers ask for_ — every bot, + every route class, gated only on `analytics.enabled`. The visit ring answers _what can the ladder + act on_ — prerender-class URLs that own a Target (or are being minted one), from bots in + `render.demand.bots`. From v0.61.0 the second is a strict subset of the first, so `demand_fill` + falling while breadth holds flat is the gates working, not the filter breaking. Neither number + distinguishes a real crawler from a spoofed User-Agent: bot names come from the UA string alone, + with no reverse-DNS or IP verification, so cross-check a search engine's own reporting before + treating a per-bot breadth figure as that engine's crawl rate. - **`queue_health.overdue` includes in-flight renders** (a leased row keeps its past due time), so its healthy floor is the in-flight count, not zero — and it is not comparable with numbers from before v0.34.0. The scan is capped by `management.scanCap`: a backlog past the cap reports the cap. From cfdfa65192078b25bf43a1f50c234af4cdf3923b Mon Sep 17 00:00:00 2001 From: Joe Date: Wed, 26 Aug 2026 16:06:51 -0400 Subject: [PATCH 3/3] =?UTF-8?q?fix(plugin):=20correct=20the=20stated=20rul?= =?UTF-8?q?e=20for=20skip/bypass=20=E2=80=94=20'never=20looked',=20not=20'?= =?UTF-8?q?not=20real=20demand'?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- packages/plugin/src/http_handlers/bot_request.js | 8 +++++--- packages/plugin/test/demandSignal.test.js | 11 +++++++++-- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/packages/plugin/src/http_handlers/bot_request.js b/packages/plugin/src/http_handlers/bot_request.js index 593742ae..503eb4a7 100644 --- a/packages/plugin/src/http_handlers/bot_request.js +++ b/packages/plugin/src/http_handlers/bot_request.js @@ -365,9 +365,11 @@ function maybeSchedule(resource, routeClass, route, botName) { } // Cache statuses that never looked for a page row, so they can neither prove nor disprove that a -// Target exists: a non-GET (`bypass`) and a deliberate cache skip (`skip`, i.e. render-now). -// Excluding them is right on the second count too — a forced render is an operator action, not -// crawler demand, and should not buy the page a faster rung. +// Target exists: a non-GET (`bypass`) and a deliberate cache skip (`skip`). They are not `miss` +// either — a miss LOOKED and found nothing — so they take neither branch below and record no +// demand. That is the conservative reading of an ambiguous request, not a claim that on-demand +// traffic is illegitimate: a render-now MISS still records, because it looked, found nothing, and +// is about to mint the Target that makes the URL real. const NO_PAGE_LOOKUP = new Set(['bypass', 'skip']); // Demand signal for the render ladder (util/visitFilter.js -> util/demandLadder.js). Keyed on diff --git a/packages/plugin/test/demandSignal.test.js b/packages/plugin/test/demandSignal.test.js index a7cb202d..9967f00b 100644 --- a/packages/plugin/test/demandSignal.test.js +++ b/packages/plugin/test/demandSignal.test.js @@ -163,12 +163,19 @@ test('a missed non-200 is NOT demand — an origin 404 is not a page', async () }); test('statuses that never looked for a page row cannot claim one', async () => { - // 'bypass' is a non-GET; 'skip' is render-now, which is an operator action rather than - // crawler demand and must not buy the page a faster rung. + // 'bypass' is a non-GET, 'skip' a deliberate cache bypass. Neither is `miss` either — a miss + // LOOKED and found nothing — so they take neither branch and record nothing. assert.equal(await wasRecorded({ cacheStatus: 'bypass', resource: { statusCode: 200 } }), false); assert.equal(await wasRecorded({ cacheStatus: 'skip', resource: { statusCode: 200 } }), false); }); +test('a render-now MISS still records: it looked, found nothing, and mints the target', async () => { + // `resolveResource` stamps cacheStatus BEFORE the render-now branch, so an on-demand render + // of an unknown URL arrives here as a plain 'miss'. Excluding it would drop genuine crawler + // demand wherever a deployment serves misses by rendering rather than proxying. + assert.equal(await wasRecorded({ cacheStatus: 'miss', resource: { statusCode: 200 } }), true); +}); + test('a non-prerender class is never demand', async () => { assert.equal(await wasRecorded({ routeClass: 'passthrough', cacheStatus: 'hit' }), false); assert.equal(await wasRecorded({ routeClass: 'unclassified', cacheStatus: 'hit' }), false);