feat(plugin): claim floor + out-of-row job leases — the nextRenderTime index no longer degrades (v0.34.0)

[harper-joseph]

The claim scan degrades, measurably and permanently, and this is the fix for it.

Every completed render moves a key from the head of the nextRenderTime index into the future and
leaves a dead index entry at the seek point. Measured on harper 5.1.26 / rocksdb-js 2.4.1, the
claim scan went from 0.36 ms to 6.25 ms over 40,000 reschedules — linearly, at ~0.15 µs per
completed render — and it did not self-heal (6.17 ms after the churn stopped). The cost is
position-dependent, not volume- or depth-dependent: identical churn away from the seek point was
free (0.26 → 0.27 ms), and backlog depth was free too (2,083 → 133,333 overdue rows was flat at
0.18–0.35 ms). The sort clause was free.

The fix is a one-sided lower bound on the scan — nextRenderTime >= floor — so it no longer
seeks the absolute minimum of that index. The same 20 keys come back in 0.43 ms instead of
6.20 ms, with no schema change.

What had to move for that to mean anything

The lease used to be written into nextRenderTime — claim stored now + jobLeaseTime there to
mark a job taken. A lease written into the due-time column is a row moved forward, so a floor over
that column is meaningless while the lease lives there. Leases now live in a node-local shared buffer
(util/renderLease.js), which also halves queue writes: 2 per render down to 1, ~87 → ~44
MB/day/node of audit. And syncQueueState's second head-seeking scan (nextRenderTime <= now,
limit 1 — ~700 ms/min of synchronous native iteration on an aged node, on the worker that also serves
bot traffic) is deleted: empty/queued is now derived from the floor plus the last claim outcome at
zero database cost. test/queueStatusDerived.test.js pins its absence by installing a search that
throws.

The floor rule

floor_new = the due minute of the first due row the pass observed — granted, skipped as
already-leased, or refused for any other reason. If the pass observed no due row: nowMinute - guard.

The obvious alternative — "floor = the last row I granted" — is wrong in the worst possible way.
Simulated over 1,189 rows at a 15% renderer-failure rate it stranded 167 rows (14.0%, matching the
failure rate exactly): every lease that expired without a posted result sat behind the advanced
floor forever. The shipped rule stranded zero. test/renderQueueFloor.test.js drives the same
trace through both rules, because the rejected half is what makes the test explain itself.

A floor silently breaks any writer that files a row below it — that row is never read again, which
is the terminal render gap: a URL that stops rendering forever and reports nothing. Three things
guard it:

• One funnel. util/renderSchedule.js is the only module in src/ that writes RenderSchedule,
  so the due-time write and the floor lowering happen together. There were eight call sites across
  five files before; "remember to lower the floor" is not an invariant sixteen call sites can hold.
  test/queueFunnel.test.js is a source scan that fails the build if any other file writes the
  table.
• A guard band. The floor is clamped queue.claimFloor.guard behind the current minute on every
  read. RenderSchedule is residency-pinned, so ~75% of "render this URL now" writes on a four-node
  cluster come from a node that cannot lower the owner's floor — the guard makes those rows land above
  every node's floor by construction, with no signalling and no cross-node coordination.
• A periodic reset. The table is @exported and the operations API reaches it regardless, so
  PUT /RenderSchedule/<key> writes a due time with no plugin code in the path. Worker 0 zeroes the
  floor every queue.claimFloor.resetInterval, bounding that from permanent to one interval.

queue.claimFloor.enabled: false rolls the whole thing back to the old full seek and changes nothing
else.

The accepted cost, and how it is bounded

The floor cannot advance past the oldest due row, and the only thing that moves a row is its own
result — a lease expiring does not, because claiming writes nothing to the table. The
generic-failure branch (target exists → hold the lease, write no row, count no strike) is where every
renderer crash, navigation timeout and settle failure lands, so one permanently-failing URL would pin
the floor at its own minute forever while dead entries piled up above it: ~43 ms/pass after a day,
i.e. worse than the 6.25 ms this change exists to remove.

queue.claimFloor.unpinAfter (1h) bounds it — the claim path writes that one row forward by a
render interval and names it in a warning. It touches strikes nowhere: that is the target's one
shared counter that suppression and redirect verdicts delete targets on, and routing the
highest-volume failure path through it would walk the corpus toward deletion during a broad origin
outage. Self-limiting at one write per interval per node, because unpinning one row promotes the next,
which must then hold for a full interval of its own.

Review history — three rounds, and what each caught

Each round found real defects in the previous one, and every regression originated in the lease
accounting. Recorded here because the pattern is the useful part:

• Round 1 → 07d0e12 (15 findings). The blocker: a throw released the lease while the row stayed
  overdue, and the throw aborted the ambient transaction so the cached-page write rolled back too —
  an unpaced re-render loop at claim frequency. A throw now holds the lease.
• Round 2/3 → 2fbeb28 (this round's 4):
  • The occupancy gauge drifted up without bound. A lease that merely expires has nobody to
    decrement it, and nothing walked the slots on a timer. Since the gauge sizes the claim scan, it
    reached 820 against 20 truly in flight by pass 40, crossed a 1,000-row cap by pass 49 and
    stayed — after which every claim drains the full cap under the claim mutex. Minutes, not hours,
    during a broad outage.
  • release could truncate a recycled slot's fresh lease to the 5s grace, because it read the
    expiry after the ownership check. The key was then re-granted with its first render still running.
  • The wedged-row warning could not fire in its own motivating scenario — it hung off
    scanTruncated, which one wedged row on a healthy node never sets.
  • The unbounded pin above (deferred by round 2, on purpose, and correctly).
• Round 4 (Gemini, on this PR) → 7d360b3. Four findings, one root cause: Number(null) is 0
  and 0 is finite, so a bare finite check accepted a MISSING column as the epoch. Worst at
  lowerFloorFor, where a null due time drove the floor to 0 — which means no floor — silently
  restoring the degraded seek this PR exists to remove. Three fixed and consolidated into one shared
  numberOf guard; the fourth dismissed, because it argued Number.isFinite is unsafe on a
  possibly-BigInt Long and a Long in a Harper schema is a 52-bit integer that always arrives as a
  JS number. A real 0 still unbounds the floor deliberately — that is the documented
  nextRenderTime = 1 priority trick — so only absence is rejected.

Testing

485 tests pass; lint and Prettier clean. The new tests in this branch's last two rounds were each
verified to fail with their fix backed out, so none of them passes for the wrong reason.

One gap, stated deliberately: the release reordering has no unit test. Reaching the
interleaving requires a grant to run between two adjacent instructions of release, which no
sequential test can arrange. It is argued from grant's publish order (payload before hashLo) in
the module comment instead of pinned by a test that would pass either way.

Measurement scripts for every number above were run against a throwaway harper 5.1.26 + rocksdb-js
2.4.1 instance, matching the deployed stack.

Sequencing

Version 0.34.0, on top of main at 0.33.0. #70 (which held 0.34.0) and #71 are closed unmerged,
so the number is free and was never released — the newest release tag is prerender-v0.33.0. This
release took it in b8645c2, renumbered from 0.35.0 across package.json and the ten prose references
that name the version as a behavioural boundary (what enabled: false rolls back to, what the backlog
snapshot is no longer comparable with). The three earlier commit subjects still read v0.35.0; that is
left alone deliberately, because rewriting them means a force-push that would strip the anchors off
the review comments already here.

Nothing blocks this and nothing conflicts with it — it is the only open plugin PR and it is based
on the current main tip.

Nothing here depends on a browser release: the wire format between the queue and the render fleet is
unchanged, so the current fleet (browser 1.16.0) claims and posts against this exactly as it does
today.

🤖 Generated with Claude Code

[gemini-code-assist]

Code Review

This pull request introduces a node-local render-lease table and a claim floor mechanism to optimize the claim scan performance of the prerender queue, moving lease state out of the database rows and into a node-local shared buffer to halve write volume. The feedback identifies critical issues regarding numeric coercion and validation: specifically, coercing nextRenderTime values directly to a Number without a truthiness check can cause null to evaluate to 0, which would trigger false alarms or incorrectly lower the claim floor. Additionally, using Number.isFinite directly on potential BigInt/Long database values will return false, so they must be coerced to a Number first.

[gemini-code-assist]

Coercing row.nextRenderTime directly to a Number without checking for null or undefined first will cause null to evaluate to 0. Since 0 is finite and less than floorMs, this will incorrectly increment belowFloor and set oldestBelowFloorMs to 0 (epoch 1970), triggering false "Below claim floor" alarms on the dashboard. Performing a truthiness check first ensures that missing or null due times are safely skipped. Since this coerce-before-finite check is used across multiple sites, we should consolidate it into a shared helper.

Suggested change

	const at = Number(row.nextRenderTime);
	if (!Number.isFinite(at)) continue;
	const at = parseFiniteNumber(row.nextRenderTime);
	if (Number.isNaN(at)) continue;

References

1. Always perform a truthiness check (ensuring the value is not null or undefined) before coercing date/time values to prevent null from evaluating to 0, which can corrupt metrics or calculations.
2. Consolidate this coerce-before-finite check into a shared helper if used across multiple sites.

[gemini-code-assist]

Coercing row.nextRenderTime directly to a Number without checking for null or undefined first will cause null to evaluate to 0. Since 0 is finite and less than floor.floorMinute, this will incorrectly mark the row as belowClaimFloor in the admin view, triggering false alarms for individual keys. Performing a truthiness check first ensures that missing or null due times are safely handled. Since this coerce-before-finite check is used across multiple sites, we should consolidate it into a shared helper.

Suggested change

	const describeScheduleRow = (row, now) => {
	const at = Number(row.nextRenderTime);
	const describeScheduleRow = (row, now) => {
	const at = parseFiniteNumber(row.nextRenderTime);

References

1. Always perform a truthiness check (ensuring the value is not null or undefined) before coercing date/time values to prevent null from evaluating to 0, which can corrupt metrics or calculations.
2. Consolidate this coerce-before-finite check into a shared helper if used across multiple sites.

[gemini-code-assist]

Coercing nextRenderTime directly to a Number without checking for null or undefined first will cause null to evaluate to 0. Since 0 is finite, this will silently lower the floor to 0 (unbounding the scan) instead of logging a warning and returning early. Performing a truthiness check first ensures that missing or null due times are safely handled. Since this coerce-before-finite check is used across multiple sites, we should consolidate it into a shared helper.

Suggested change

	const lowerFloorFor = (nextRenderTime) => {
	const at = Number(nextRenderTime);
	if (!Number.isFinite(at)) {
	const lowerFloorFor = (nextRenderTime) => {
	const at = parseFiniteNumber(nextRenderTime);
	if (Number.isNaN(at)) {

References

1. Always perform a truthiness check (ensuring the value is not null or undefined) before coercing date/time values to prevent null from evaluating to 0, which can corrupt metrics or calculations.
2. Consolidate this coerce-before-finite check into a shared helper if used across multiple sites.

[gemini-code-assist]

Using Number.isFinite(nextRenderTime) directly on a value that may be a BigInt/Long from the database will return false for BigInts, causing them to be ignored and overwritten with the initial render time. Coercing the value to a Number first, after performing a truthiness check, ensures that BigInt values are correctly validated and handled. Since this coerce-before-finite check is used across multiple sites, we should consolidate it into a shared helper.

		const at = parseFiniteNumber(nextRenderTime);
		const resolvedNextRenderTime = !Number.isNaN(at) && at > 0 ? at : null;
		await writeSchedules(
			cacheKeysOf(url).map((cacheKey) => ({
				cacheKey,
				nextRenderTime: resolvedNextRenderTime ?? getInitialRenderTime(cacheKey, interval),
				fromSitemap,
			}))
		);

References

1. Always coerce database column values (which may be BigInt/Long) to a Number before validating with Number.isFinite, as Number.isFinite returns false for BigInts. Consolidate this coerce-before-finite check into a shared helper if used across multiple sites.

[harper-joseph]

Thanks — three of the four are real and are fixed in 7d360b3. They share one root cause, so they are now one shared guard (numberOf in util/time.js) rather than four call-site checks, as suggested.

Fixed — Number(null) === 0 is the live trap:

• renderSchedule.js lowerFloorFor — the serious one, and worse than described. A null due time became 0, and lowerFloorTo(0) means no floor, so one missing value silently put the claim scan back to seeking the absolute index minimum: the degraded 6.25 ms seek this release exists to remove, with no warning, because 0 passed the finite check. writeSchedules is worse still — one null row won the batch minimum and unbounded the floor for an entire device fan-out.
• backlogSnapshot.js — agreed, and the consequence is specifically corrosive: it is a false alarm on the one metric that reports rows filed where no claim will ever look again.
• PrerenderAdmin.js describeScheduleRow — agreed. Derived fields now report null instead of an answer they do not have, rather than a named URL appearing overdue-since-1970 and below-floor in the view an operator uses to decide whether to repair or delete it.

One behaviour deliberately preserved: a real 0 still unbounds the floor. A due time at or before the epoch minute is a documented value in this system — the nextRenderTime = 1 priority trick, or a junk PUT — so only absence is rejected, not zero. The new test pins both halves and was verified to fail with the old coercion restored.

Dismissed — Target.js: the BigInt premise does not hold here. A Long in a Harper schema is a 52-bit integer, so it always arrives as a JS number; Number.isFinite on one is safe by itself. Number.isFinite also does not coerce, so the existing Number.isFinite(x) && x > 0 already rejects null correctly — the suggested change is behaviour-neutral at that site.

Acting on it would have been actively harmful in one respect: it would have enshrined a wrong justification in the code. Comments elsewhere in this package that cited BigInt as the reason for a coercion have been corrected in the same commit — the reason is null, and always was.

[harper-joseph]

One more fix, 88de22b — found from outside this branch, which is why it is worth calling out.

maybeUnpinFloor filed now + render.defaultInterval. Every other schedule writer files completionMinute + resolveRenderInterval(...), so this was the one writer that ignored the row's own cadence — wrong in both directions: a 1h-cadence homepage parked for a full day, a 48h-cadence page back due long before its cadence.

The subtler half is that nextRenderTime - interval is how a completion is read back out of a row, precisely because every other writer files it that way. A flat defaultInterval manufactured a completion that never happened: on the deployed 48h route the row read as "rendered 24h ago", on a 1h route as "rendered in the future". Nothing on this branch performs that arithmetic yet — which is exactly the argument for fixing it here rather than later. The row is the only record, it outlives the pass that wrote it, and a later reader has no way to tell that this one value was synthetic.

It isn't hypothetical. A reviewer on the bulk-invalidation branch (which stacks on this one) found it because a guard there infers a completion from the row: the synthetic value waved through the never-completing row this hatch had just moved, re-arming it on every crawl and re-pinning the floor the hatch exists to release — which is the only bound on that pin.

Scoped deliberately: route > default, where processJobResult resolves route > the target's stored interval > default. Closing that last gap needs a Target read on the claim path plus an import cycle (resources/Target.js imports the funnel), so the residual is written down at the site instead of half-fixed.

486 tests pass, lint and Prettier clean. The new test asserts both the filed value and that filed - resolveRenderInterval(url, stored) === now, i.e. that the row reads as a completion NOW rather than a fake past, and it was verified to fail with the fix backed out.