Merge pull request #39 from HarperFast/fix/reconcile-query

fix(plugin): the reconcile walk was rejected on its first page; v0.10.1

Author: harper-joseph

package-lock.json

@@ -87,8 +87,9 @@ rest: true # required for the @export-ed table REST endpoints
       interval: 21600000 # 6h — how often each node sweeps its own slice of the keyspace
       startDelay: 300000 # 5m — grace after boot before the first sweep
       startJitter: 300000 # 5m — per-node spread, so a rolling restart doesn't sync the sweeps
-      batchSize: 500 # targets per page (bounds how long each read transaction stays open)
-      maxRestores: 5000 # ceiling on rows RESTORED per sweep; a truncated sweep logs that it was
+      maxRestores:
+        5000 # ceiling on rows RESTORED per sweep; the scan always completes, so a
+        # truncated sweep still reports the full size of the gap
 
   sitemap:
     refreshTime: '12:00' # local time-of-day for the daily sitemap refresh
@@ -291,9 +292,16 @@ The state is therefore terminal _and_ silent: the cached page expires, every lat
 falls through to the origin, and there is no error and no metric to notice it by. The only
 symptom is a page whose `lastCached` keeps receding.
 
-`render.reconcile` is the repair. Each node walks the target registry in primary-key pages and,
-**for the keys it owns**, checks node-locally whether the schedule row exists and restores it if
-not. Owner-scoped is a safety requirement, not an optimization: a point read of a
+`render.reconcile` is the repair. Each node makes **one pass** over the target registry and, **for
+the keys it owns**, checks node-locally whether the schedule row exists, collecting the gaps and
+restoring them once the scan has finished.
+
+The pass is deliberately cursor-free, so nothing depends on the order rows arrive in. Paging by
+primary key and resuming from the last key seen would make correctness rest on the storage engine
+returning rows in key order — and if that ever stopped holding, the cursor would skip rows
+silently, which is the worst failure mode available to a repair tool. Restoring only after the
+scan closes also makes the transaction rule structural rather than a convention: no write is ever
+issued while the scan's cursor is open. Owner-scoped is a safety requirement, not an optimization: a point read of a
 residency-pinned row this node does not own takes Harper's untimed replication fetch, so a
 single such read could hang the sweep forever. Every node sweeping its own slice covers the
 whole keyspace with no coordination and no cross-node reads.

packages/plugin/README.md

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

packages/plugin/package.json

@@ -208,10 +208,10 @@ const defaultConfig = () => ({
 			interval: 6 * HOUR, // how often each node sweeps its own slice of the keyspace
 			startDelay: 5 * MINUTE, // grace after boot before the first sweep
 			startJitter: 5 * MINUTE, // per-node spread, so a rolling restart doesn't sync the sweeps
-			batchSize: 500, // targets per page; bounds how long each read transaction stays open
-			// Ceiling on rows RESTORED per sweep (not rows examined). A membership change can
-			// strand a large slice of the keyspace at once, and rewriting millions of rows in
-			// a single pass would be its own outage. A truncated sweep says so in the log.
+			// Ceiling on rows RESTORED per sweep. The scan always runs to completion, so a
+			// truncated sweep still reports the true size of the gap — the cap bounds only how
+			// much is repaired at once, since a membership change can strand a large slice of the
+			// keyspace and rewriting millions of rows in one pass would be its own outage.
 			maxRestores: 5000,
 		},
 	},

packages/plugin/src/config.js

@@ -36,122 +36,115 @@ import { fnv1a32 } from './hash.js';
 import { getResidencyByUrl } from './residency.js';
 import { getInitialRenderTime } from './time.js';
 
+// Rows scanned between event-loop yields, so a sweep over a large registry stays background
+// work rather than monopolizing the thread.
+const YIELD_EVERY = 200;
+
 /**
- * Walk targets in primary-key order and restore any missing schedule row for the keys this
- * node owns. All I/O is injected so the traversal, the ownership filter and the caps are
- * testable without a live database.
+ * ONE pass over the targets: find the keys this node owns whose schedule row is missing, then
+ * restore them after the scan has finished. All I/O is injected, so the traversal, the
+ * ownership filter and the cap are testable without a live database.
+ *
+ * Deliberately CURSOR-FREE, and therefore indifferent to the order rows arrive in. An earlier
+ * version paged by primary key and resumed from the last key seen, which quietly made
+ * correctness depend on the storage engine returning rows in key order: if that ever stopped
+ * holding, the cursor would skip rows silently — the worst possible failure mode for a repair
+ * tool, in the one place nobody would look. A single pass needs no such guarantee.
+ *
+ * The two phases also make the transaction rule structural rather than a convention to
+ * remember: no write is issued while the scan's cursor is open, because every write happens
+ * after it closes. (Interleaving them would hold the read transaction across the writes and
+ * pin the log against reclamation — the same reason `claim` drains before leasing.)
  *
- * `searchTargets({ cursor, limit })` must return an ARRAY (a drained batch), not a live
- * iterator: writes must never be issued while a search cursor is still open, since that keeps
- * the read transaction open across them and pins the log against reclamation (the same
- * reason `claim` drains before leasing). Paging by primary key is what keeps each read
- * transaction short — a single walk across a million targets would sit open long enough for
- * Harper to complain about it and commit it out from under us.
+ * The cap bounds WRITES, not scanning: the scan always runs to completion, so `missing` is the
+ * true size of the gap even when only `maxRestores` of it was repaired this pass. That also
+ * means the loop never breaks early, so the iterator is always fully consumed and its read
+ * transaction always released.
  */
 export const reconcileSchedules = async ({
-	searchTargets,
+	streamTargets,
 	getSchedule,
 	putSchedule,
 	ownerOf,
 	hostname,
-	batchSize,
 	maxRestores,
-	onBatch = () => {},
+	onYield = () => {},
 } = {}) => {
-	const stats = { examined: 0, owned: 0, restored: 0, truncated: false, lastKey: null };
-	let cursor = null;
-
-	for (;;) {
-		const batch = await searchTargets({ cursor, limit: batchSize });
-		if (!batch.length) break;
-
-		for (const target of batch) {
-			const cacheKey = target.cacheKey;
-			// Advance the cursor for every row, including ones we skip, so a capped or
-			// interrupted run always makes forward progress instead of re-reading its prefix.
-			cursor = cacheKey;
-			stats.examined++;
-			stats.lastKey = cacheKey;
-
-			// Residency is keyed off the URL-half exactly as RenderSchedule's own
-			// `setResidencyById` computes it, so this agrees with where the row actually lives.
-			if (ownerOf(CacheKey.extractUrl(cacheKey)) !== hostname) continue;
-			stats.owned++;
-
-			if (await getSchedule(cacheKey)) continue;
-
-			// A cap on WRITES, not on rows examined: the pathological case is a membership
-			// change stranding a large slice of the keyspace at once, and restoring millions of
-			// rows in a single pass would be its own outage. Report the truncation so a short
-			// count is never mistaken for "all clear".
-			if (stats.restored >= maxRestores) {
-				stats.truncated = true;
-				return stats;
-			}
-
-			await putSchedule(cacheKey, {
-				// The jittered initial time, NOT "now": a repair pass can restore a great many
-				// rows at once, and scheduling them all immediately would replace a silent
-				// outage with a render herd. This is the same value the original
-				// `RenderTarget.put` would have written, so a repaired target rejoins the
-				// rotation exactly where it belonged.
-				//
-				// `Long` columns can arrive as BigInt, which `Number.isFinite` rejects outright, so
-				// coerce before handing it over. No range check is needed here: the callee guards
-				// `Number.isFinite(interval) && interval > 0`, so a NON-POSITIVE value falls back
-				// to the default too — `Number(null)` is 0, which that guard rejects.
-				nextRenderTime: getInitialRenderTime(cacheKey, Number(target.renderInterval)),
-				fromSitemap: !!target.sitemapUrl,
-			});
-			stats.restored++;
-		}
+	const stats = { examined: 0, owned: 0, missing: 0, restored: 0, truncated: false };
+	const toRestore = [];
+
+	// Phase 1 — read only.
+	for await (const target of streamTargets()) {
+		stats.examined++;
+		if (stats.examined % YIELD_EVERY === 0) await onYield();
+
+		const cacheKey = target.cacheKey;
 
-		await onBatch(stats);
+		// Residency is keyed off the URL-half exactly as RenderSchedule's own
+		// `setResidencyById` computes it, so this agrees with where the row actually lives.
+		if (ownerOf(CacheKey.extractUrl(cacheKey)) !== hostname) continue;
+		stats.owned++;
 
-		// A short page means the index walk is done.
-		if (batch.length < batchSize) break;
+		if (await getSchedule(cacheKey)) continue;
+		stats.missing++;
+
+		// Past the cap we keep counting but stop collecting, so the gap is measured in full
+		// while the repair stays bounded. A membership change can strand a large slice of the
+		// keyspace at once, and rewriting millions of rows in one pass would be its own outage.
+		if (toRestore.length < maxRestores) toRestore.push(target);
+	}
+
+	// Phase 2 — writes, with the scan's cursor now closed.
+	for (const target of toRestore) {
+		await putSchedule(target.cacheKey, {
+			// The jittered initial time, NOT "now": a repair pass can restore a great many rows at
+			// once, and scheduling them all immediately would replace a silent outage with a
+			// render herd. This is the same value the original `RenderTarget.put` would have
+			// written, so a repaired target rejoins the rotation exactly where it belonged.
+			//
+			// `Long` columns can arrive as BigInt, which `Number.isFinite` rejects outright, so
+			// coerce before handing it over. No range check is needed here: the callee guards
+			// `Number.isFinite(interval) && interval > 0`, so a NON-POSITIVE value falls back to
+			// the default too — `Number(null)` is 0, which that guard rejects.
+			nextRenderTime: getInitialRenderTime(target.cacheKey, Number(target.renderInterval)),
+			fromSitemap: !!target.sitemapUrl,
+		});
+		stats.restored++;
 	}
 
+	stats.truncated = stats.missing > stats.restored;
+
 	return stats;
 };
 
 /** `reconcileSchedules` bound to the live tables. */
-export const reconcileScheduleGaps = async ({
-	batchSize = config.render.reconcile.batchSize,
-	maxRestores = config.render.reconcile.maxRestores,
-} = {}) => {
+export const reconcileScheduleGaps = async ({ maxRestores = config.render.reconcile.maxRestores } = {}) => {
 	const {
 		render_service: { RenderTarget },
 		render_schedule: { RenderSchedule },
 	} = databases;
 
 	return reconcileSchedules({
-		searchTargets: ({ cursor, limit }) =>
-			Array.fromAsync(
-				RenderTarget.search({
-					// Paging by primary key is an ordinary index walk: Harper injects this exact
-					// condition shape (`greater_than` on the primary key) for any unconstrained
-					// scan, so a cursor is just that scan resumed. The first page passes no
-					// condition and lets Harper inject it.
-					...(cursor === null
-						? {}
-						: { conditions: [{ attribute: 'cacheKey', comparator: 'greater_than', value: cursor }] }),
-					sort: { attribute: 'cacheKey' },
-					select: ['cacheKey', 'renderInterval', 'sitemapUrl'],
-					limit,
-				})
-			),
+		// One unconstrained scan, streamed. No conditions, no `sort`, no `limit`:
+		//   - no `sort`, because asking Harper to sort by the primary key is what broke v0.10.0
+		//     (the primary key is not flagged `indexed`, so a sort on it is rejected unless a
+		//     condition accompanies it — and Harper injects its own scan condition only AFTER
+		//     that check, so the first page threw before scanning anything);
+		//   - no conditions, because with none Harper injects that full-scan condition itself,
+		//     which is exactly what this wants;
+		//   - no `limit`, because the caller streams and never resumes, so it needs no paging.
+		//
+		// Nothing here depends on the order rows arrive in — see `reconcileSchedules`.
+		streamTargets: () => RenderTarget.search({ select: ['cacheKey', 'renderInterval', 'sitemapUrl'] }),
 		// Node-local by construction — see the module comment. Existence is all that matters.
 		getSchedule: (cacheKey) => RenderSchedule.get({ id: cacheKey, select: ['cacheKey'] }, { replicateFrom: false }),
 		// Writes route by residency, so this reaches the owning node even though the read above
 		// deliberately does not.
 		putSchedule: (cacheKey, row) => RenderSchedule.put(cacheKey, row),
 		ownerOf: getResidencyByUrl,
 		hostname: server.hostname,
-		batchSize,
 		maxRestores,
-		// Yield between pages so a sweep over a large registry stays background work.
-		onBatch: () => setImmediate(),
+		onYield: () => setImmediate(),
 	});
 };
 
@@ -182,9 +175,9 @@ export const runReconcileOnce = async (options) => {
 		// warning rather than an info line. A clean pass says so quietly.
 		if (stats.restored || stats.truncated) {
 			logger.warn(
-				`[prerender] schedule reconcile: restored ${stats.restored} missing schedule row(s) across ${stats.owned} owned target(s) (${stats.examined} examined)` +
+				`[prerender] schedule reconcile: restored ${stats.restored} of ${stats.missing} missing schedule row(s) across ${stats.owned} owned target(s) (${stats.examined} examined)` +
 					(stats.truncated
-						? ` — stopped at the ${config.render.reconcile.maxRestores}-restore cap, more may remain`
+						? ` — ${stats.missing - stats.restored} left for the next sweep by the ${config.render.reconcile.maxRestores}-restore cap`
 						: '')
 			);
 		} else {