Merge pull request #45 from HarperFast/fix/align-device-jitter

Aligns a URL's device variants onto one render slot ahead of the sitemap pre-warm, plus rejects an empty cacheKey.delimiter (review finding).

Author: harper-joseph

package-lock.json

@@ -83,7 +83,11 @@ rest: true # required for the @export-ed table REST endpoints
 
   page:
     ttl: 86400000 # 24h — default cached-page TTL
-    minTtl: 21600000 # 6h  — floor for sitemap-derived TTLs
+    minTtl: 21600000 # 6h  — floor for sitemap-derived TTLs. Also floors the RENDER INTERVAL a
+    #                        sitemap's `changefreq` produces, and therefore the width of the
+    #                        initial-render jitter: `changefreq: hourly` becomes a 6h cadence
+    #                        spread over 6h, i.e. 4x the sustained render load of `daily`.
+    #                        Raise this to slow a fleet down; it trades page freshness for load.
     swrTtl: 10800000 # 3h  — stale-while-revalidate window
 
   render:
@@ -241,6 +245,33 @@ A forwarded-mode config that compiles to **zero** prerender routes is reported a
 (`/prerender_admin` surfaces it): nothing is prerendered in that state, and it is what a single
 typo produces, since invalid entries are dropped one at a time.
 
+### How bulk sitemap population is staggered
+
+Populating a large sitemap must not queue every URL at once. A new target's first render is
+therefore `now + (hash(url) % renderInterval)`, floored to the minute — a uniform spread over the
+interval, so the fleet sees a flat stream rather than a herd. Because `processJobResult`
+reschedules from **render completion** (`currentMinuteMs() + interval`) rather than a fixed
+time-of-day, that spread is preserved cycle over cycle and self-paces to fleet throughput.
+
+Three properties are worth knowing before a bulk upload:
+
+- **The stagger window is the target's own `renderInterval`, not a fixed 24h.** For a sitemap
+  target that comes from `changefreq`, floored at `page.minTtl` — so `always`/`hourly` spread over
+  `minTtl` (6h by default) and re-render that often, i.e. 4× the sustained load of `daily`. A
+  sitemap's `changefreq` is the single biggest determinant of steady-state render load, and it
+  comes from the sitemap XML, not from this config. Check it before uploading.
+- **A URL's device variants share one slot.** The offset is seeded off the URL half of the cache
+  key, so `…|desktop` and `…|mobile` come due together, sort adjacently in claim order, and get
+  rendered back-to-back by one worker off a warm origin. It also keeps the cached copies of one
+  page the same age — seeded off the full key they drifted up to a whole interval apart, so a
+  content change could appear on one device and not the other for hours.
+- **`revalidate: true` bypasses the stagger entirely**, setting every entry due in the same
+  minute. That is its purpose (forcing a backfill), but it is not how to warm a large sitemap for
+  the first time — omit it and let the jitter place the URLs.
+
+There is no separate "warm-up" pacing knob, and none is needed: the initial spread is exactly the
+steady-state cadence, so a fleet that can sustain the ongoing load can absorb the warm.
+
 ### Staging passthrough
 
 To verify an origin against a staging edge (e.g. a CDN's staging network) _through_ the
@@ -336,7 +367,11 @@ this plugin's resources all set `loadAsInstance = false`.
 - **Overview** — per-node queue status with staleness, table counts, the due-now backlog, and
   a next-24h histogram of `nextRenderTime`. That histogram is the quickest way to tell a
   healthy jittered spread from a render herd: a flat distribution means the initial-render
-  jitter is working, a single tall bar means everything comes due at once.
+  jitter is working, a single tall bar means everything comes due at once. Note the histogram
+  is capped at `management.scanCap` rows and reports `truncated` — at a large registry read the
+  shape, not the counts. The due-now backlog is the capacity signal: jitter flattens the
+  arrival curve but cannot lower it, so a backlog that climbs and never returns to zero means
+  sustained demand (`Σ targets ÷ renderInterval`) exceeds fleet throughput.
 - **URL explainer** — paste a URL and see the ingress route that matched, the query allowlist
   it selected, the canonical URL, the resulting cache key, and the live
   `RenderTarget`/`RenderSchedule`/`PrerenderedPage`/`NonIndexable` rows under it. It also

packages/plugin/README.md

@@ -1,6 +1,6 @@
 {
 	"name": "@harperfast/prerender",
-	"version": "0.13.0",
+	"version": "0.14.0",
 	"type": "module",
 	"description": "Configurable Harper plugin for prerendering pages for bots and crawlers",
 	"license": "Apache-2.0",

packages/plugin/package.json

@@ -410,6 +410,25 @@ const mergeInto = (target, source, path = 'prerender') => {
 	}
 };
 
+/**
+ * Reject overrides that pass `mergeInto`'s type check but cannot be honored at all, restoring
+ * the default. This is distinct from `collectConfigWarnings`, which reports settings that are
+ * merely risky — these are values that would silently corrupt behavior, so they are dropped.
+ *
+ * `cacheKey.delimiter` is structural: an empty string is a string, so the type check accepts
+ * it, but then `toCacheKey` concatenates the attributes with no separator (making the keys of
+ * two URLs where one is a prefix of the other collide) and `parse` splits on `''`, i.e. into
+ * individual CHARACTERS — a cache key parses to `{ url: 'h', deviceType: 't' }`. It would also
+ * collapse the whole render schedule onto a single minute, since `indexOf('')` is 0 and every
+ * jitter seed becomes the empty string. Nothing downstream can work, so the default wins.
+ */
+const rejectUnusableOverrides = (fresh) => {
+	if (fresh.cacheKey.delimiter === '') {
+		getLogger().warn?.('[prerender] Ignoring prerender.cacheKey.delimiter: must not be empty — keeping the default');
+		fresh.cacheKey.delimiter = defaultConfig().cacheKey.delimiter;
+	}
+};
+
 /**
  * Apply host-provided options onto the live `config`, with validation. Safe to
  * call repeatedly (e.g. on every options `change`). Resets to defaults first so
@@ -418,6 +437,7 @@ const mergeInto = (target, source, path = 'prerender') => {
 export const applyOptions = (options) => {
 	const fresh = defaultConfig();
 	if (isPlainObject(options)) mergeInto(fresh, options);
+	rejectUnusableOverrides(fresh);
 
 	// Replace the contents of the live object in place to preserve the reference.
 	for (const key of Object.keys(config)) delete config[key];

packages/plugin/src/config.js

@@ -34,10 +34,11 @@ export class RenderTarget extends databases.render_service.RenderTarget {
 		// silently. `util/reconcile.js` is what actually repairs it.
 		const result = await super.put({ ...CacheKey.parse(cacheKey), ...data }, target);
 
-		// Absent a valid explicit time, jitter the first render across the interval (keyed
-		// off the cacheKey) so bulk-created targets don't all come due at once. RenderTarget
-		// is API-exposed, so validate the numbers (reject negatives / NaN / non-numbers)
-		// rather than trust the payload.
+		// Absent a valid explicit time, jitter the first render across the interval (keyed off
+		// the URL half of the cacheKey, so a URL's device variants share one slot) so
+		// bulk-created targets don't all come due at once. RenderTarget is API-exposed, so
+		// validate the numbers (reject negatives / NaN / non-numbers) rather than trust the
+		// payload.
 		const interval =
 			Number.isFinite(data.renderInterval) && data.renderInterval > 0
 				? data.renderInterval

packages/plugin/src/resources/RenderTarget.js

@@ -46,19 +46,43 @@ export const getNextTimeOfDay = (timeStr, timezone) => {
 };
 
 /**
- * Deterministic first-render time: `now` plus a per-key offset in `[0, interval)`,
+ * The jitter offset is seeded off the URL half of a cache key, NOT the whole key, so every
+ * device-type variant of one URL lands on the SAME minute. Seeded off the full key, `desktop`
+ * and `mobile` hash to unrelated offsets and drift up to a whole interval apart, which means
+ * the two copies of a page can differ in age by up to 24h — a content change shows on one
+ * device and not the other, and every render pays a cold origin/CDN fetch. Aligned, the pair
+ * sorts adjacently in `RenderQueue.claim`'s nextRenderTime order and is rendered back-to-back
+ * by one worker off a warm origin. Residency already groups them this way (`schedulerNode`
+ * comes from the URL alone), so the seed now agrees with the routing.
+ *
+ * Alignment persists cycle over cycle because `processJobResult` reschedules from
+ * `currentMinuteMs() + interval` — both variants completing within the same minute get an
+ * identical next time, so the pair stays locked instead of drifting.
+ */
+const jitterSeed = (key) => {
+	const str = String(key ?? '');
+	const at = str.indexOf(config.cacheKey.delimiter);
+	// No delimiter → not a cache key. Seed off the whole string rather than the empty prefix
+	// `CacheKey.extractUrl` would return, which would collapse every such key onto a single
+	// minute — precisely the herd this jitter exists to prevent.
+	return at === -1 ? str : str.slice(0, at);
+};
+
+/**
+ * Deterministic first-render time: `now` plus a per-URL offset in `[0, interval)`,
  * floored to the minute. Spreads the initial render of freshly-scheduled targets
  * across the render interval instead of firing them all at once (the thundering
- * herd on bulk sitemap population / crawl spikes). The offset is keyed off the
- * cacheKey so it's stable and reproducible. Recurring re-renders are scheduled
- * relative to render completion (see RenderQueue.processJobResult), so this initial
- * spread is preserved cycle over cycle rather than realigning to a fixed instant.
+ * herd on bulk sitemap population / crawl spikes). The offset is keyed off the URL
+ * half of the cacheKey (see `jitterSeed`) so it's stable, reproducible, and shared
+ * by a URL's device variants. Recurring re-renders are scheduled relative to render
+ * completion (see RenderQueue.processJobResult), so this initial spread is preserved
+ * cycle over cycle rather than realigning to a fixed instant.
  */
 export const getInitialRenderTime = (key, interval) => {
 	// Guard against a zero/negative/NaN interval (which would make the modulo NaN and
 	// schedule an invalid time) so the helper is safe to call with unvalidated inputs.
 	const safeInterval = Number.isFinite(interval) && interval > 0 ? interval : config.render.defaultInterval;
-	return currentMinuteMs(Date.now() + (fnv1a32(key) % safeInterval));
+	return currentMinuteMs(Date.now() + (fnv1a32(jitterSeed(key)) % safeInterval));
 };
 
 export const getNextSitemapRefreshTime = () => getNextTimeOfDay(config.sitemap.refreshTime, config.sitemap.timezone);

packages/plugin/src/util/time.js

@@ -145,3 +145,19 @@ test('applyOptions keeps the literal token when valueEnv is unset or missing', (
 	applyOptions({ securityToken: { value: 'literal', valueEnv: '__MISSING_ENV__' } });
 	assert.equal(config.securityToken.value, 'literal');
 });
+
+/*
+ * An empty cacheKey.delimiter passes mergeInto's typeof check (it IS a string) but cannot be
+ * honored: `toCacheKey` would concatenate with no separator, `parse` would split on '' into
+ * individual characters, and every jitter seed would become '' — collapsing the whole render
+ * schedule onto one minute. It is rejected outright rather than merely warned about.
+ */
+test('applyOptions rejects an empty cacheKey.delimiter and keeps the default', () => {
+	applyOptions({ cacheKey: { delimiter: '' } });
+	assert.equal(config.cacheKey.delimiter, '|');
+});
+
+test('applyOptions still honors a non-empty cacheKey.delimiter override', () => {
+	applyOptions({ cacheKey: { delimiter: '::' } });
+	assert.equal(config.cacheKey.delimiter, '::');
+});

packages/plugin/test/config.test.js

@@ -43,3 +43,43 @@ test('getInitialRenderTime is stable per key and spreads across keys', () => {
 	const values = new Set(Array.from({ length: 100 }, (_, i) => getInitialRenderTime(`key-${i}`, interval)));
 	assert.ok(values.size > 1, 'distinct keys spread across times');
 });
+
+/*
+ * The jitter is seeded off the URL half of the cache key, so a URL's device variants come due
+ * together: the pair sorts adjacently in claim order and renders back-to-back off a warm
+ * origin, and the two cached copies of a page never differ in age by up to a whole interval.
+ * Seeded off the full cacheKey (as it was), `|desktop` and `|mobile` hashed to unrelated
+ * offsets.
+ */
+test('getInitialRenderTime aligns a URL device variants on one slot', () => {
+	const interval = 24 * 60 * MINUTE;
+	const url = 'https://x.test/catalog/shoes';
+	const desktop = getInitialRenderTime(`${url}|desktop`, interval);
+	const mobile = getInitialRenderTime(`${url}|mobile`, interval);
+	const tablet = getInitialRenderTime(`${url}|tablet`, interval);
+
+	// Allow a 1-minute window: the calls can straddle a wall-clock minute boundary.
+	assert.ok(Math.abs(desktop - mobile) <= MINUTE, 'desktop and mobile share a slot');
+	assert.ok(Math.abs(desktop - tablet) <= MINUTE, 'desktop and tablet share a slot');
+});
+
+test('getInitialRenderTime still spreads distinct URLs that share a device type', () => {
+	const interval = 24 * 60 * MINUTE;
+	const values = new Set(
+		Array.from({ length: 200 }, (_, i) => getInitialRenderTime(`https://x.test/p/prd-${i}|desktop`, interval))
+	);
+	// Aligning device variants must not collapse the URL spread. 200 URLs over 1440 minute
+	// buckets: a handful of hash collisions is expected, a near-total collapse is the bug.
+	assert.ok(values.size > 100, `distinct URLs stay spread (got ${values.size} slots)`);
+});
+
+/*
+ * A key with no delimiter is not a cache key. It must fall back to hashing the whole string:
+ * `CacheKey.extractUrl` returns '' for such a key, and seeding off that would put EVERY
+ * delimiter-less key on the same minute — the exact herd this jitter prevents.
+ */
+test('getInitialRenderTime does not collapse keys that lack the delimiter', () => {
+	const interval = 24 * 60 * MINUTE;
+	const values = new Set(Array.from({ length: 100 }, (_, i) => getInitialRenderTime(`no-delimiter-${i}`, interval)));
+	assert.ok(values.size > 50, `delimiter-less keys stay spread (got ${values.size} slots)`);
+});