Queue every detected change instead of deferring past a budget (v0.75.0) - #178
harper-joseph wants to merge 1 commit into
Conversation
… budget; v0.75.0
Deferring was the routine outcome of a busy pass, and it is a bad trade: the origin read
that proved a URL changed has already been paid for, and dropping the result throws that
away and re-buys it on the next pass — which in anchored mode is a DAY later. The render
queue is itself a backlog; a trigger files a row and the fleet drains it at whatever rate
it can. So bound how fast the writes LAND, not whether they land at all.
Three changes:
- `maxTriggersPerSweep` defaults to 0 = NO CEILING. It stays available as a cap on what
one pass may inject (sizing a new rule, say), but it is no longer the routine dropper.
- THE QUEUE OUTLIVES THE PASS. It is module-scoped and the pass no longer awaits
`drain()`. A pass that detected more change than the bounded drain can place would
otherwise be held open by it — and in anchored mode a pass still running at the next
anchor makes that anchor SKIP, turning a busy night into a missed one. An aborted pass
no longer stops the queue either: what is in it was genuinely detected and its baseline
is unwritten, so draining it is still right. Only disabling the probe clears it.
- `trigger.maxPending` 5,000 -> 50,000 and is now documented as what it actually is: the
ONLY remaining bound, and a MEMORY bound rather than a policy one (an entry carries the
observed signature, ~1.4 KB, so 50,000 is ~70 MB/node). A refusal is an overload alarm,
not a normal outcome.
CONSEQUENCE ON THE STATS, stated because it changes their meaning: `triggered`/`errors` are
cumulative counters read at pass end, not per-pass totals — a trigger this pass submitted
may settle during the next. `queued` is the honest per-pass number and the new
`triggerQueuePending` says how much had not landed when the pass ended.
Queue depth now also rides the sweep heartbeat, so the queue is visible WHILE a pass runs.
Previously the only reading came from a finished pass, which is useless for a drain whose
whole purpose is to outlive the pass — and that gap cost real time diagnosing a live fleet.
KNOWN TRADEOFF, not fixed here: filing everything due-now flattens priority, since claims
order by nextRenderTime and a large ready set at the same due time competes evenly. The
backlog drains, but lower-priority work starves while it does. Render priority lanes (#80)
are the real fix.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates @harperfast/prerender to version 0.75.0 and restructures the change probe sweep's trigger queue so that it outlives individual passes, defaulting maxTriggersPerSweep to 0 (unlimited) to avoid discarding paid-for origin reads. Feedback on these changes suggests ensuring that the queue is stopped and flushed when the change probe is disabled via live configuration to prevent data loss. Additionally, the reviewer recommends preserving per-pass delta metrics for triggered and errors instead of mixing cumulative queue stats with delta metrics, which complicates dashboard querying and aggregation.
| // An aborted pass does NOT stop the queue either: what is already in it was genuinely | ||
| // detected and its baseline is unwritten, so draining it is still the right thing. Only | ||
| // disabling the probe clears it. |
There was a problem hiding this comment.
When disabling the change probe via live configuration changes (e.g., when config.changeProbe.enabled becomes false in syncProbeTimers), ensure that we stop the timer and flush any remaining buffered data or counters in the queue one last time before clearing it. This prevents partial interval data or pending triggers from being silently lost.
References
- When disabling a periodic timer or stats collector via live configuration changes, ensure to stop the timer and flush any remaining buffered data or counters one last time to prevent partial interval data from being silently lost.
| stats.triggered = triggers.stats.triggered; | ||
| stats.errors = triggers.stats.errors; |
There was a problem hiding this comment.
Overwriting stats.triggered and stats.errors with the cumulative queue stats (triggers.stats.triggered and triggers.stats.errors) changes their meaning to cumulative counters. However, other metrics in the same probe_* series (like probe_probed, probe_changed, probe_deferred) remain per-pass deltas. Mixing cumulative and delta metrics in the same series makes dashboard querying and TSDB aggregation highly inconsistent and difficult. Consider preserving the per-pass delta meaning for triggered and errors by capturing the queue's start stats at the beginning of the pass and subtracting them at the end.
…ng; v0.77.0 `POST /prerender_admin/explain` exists to answer "why does this URL behave this way", and it could not answer it for the one property operators ask about most. It reported Target.renderInterval and stopped — but that is the CEILING, not the cadence. The value a row is actually scheduled from is resolveEffectiveInterval(url, target), which folds four inputs (route interval, stored interval, default, ladder rung) through two clamps (the route's demandFloor, the route's interval as a ceiling), and the answer is routinely none of the numbers an operator can see. Measured on a production cluster this week: a PDP route configured `renderInterval: 96h` was rendering every 48h. 95.7% of its targets carried a ladder rung, the route's `demandFloor: 48h` equalled the ladder's slowest rung, so `max(rung, floor)` clamped every rung to exactly 48h and the configured 96h ceiling never bound once. The knob named "floor" was the real cadence and the knob named "ceiling" was inert. Establishing that took fifteen explain calls plus reading Target.demandInterval out of the table by hand for 162 URLs and working the algebra backwards — because `demandInterval` was not in explain's select at all. So: select it, and add a `cadence` block reporting the whole chain — every input, which one supplied the base, and what clamped the result. `clampedBy` is the field to read first: 'floor' means the ladder wanted this page faster and the route refused (seeing it across a route means the ladder has no dynamic range there), 'ceiling' means a stored rung is slower than the route allows (what a rung outliving a lowered interval looks like), null means no rung. `explainCadence` lives in util/routeClass.js beside the resolver, not in the admin view that renders it, and derives from the same functions rather than recomputing the algebra. A second implementation would be a second thing to keep correct, and its failure mode is the worst available to a diagnostic: a view that confidently explains a cadence the scheduler is not using. A test cross-checks the two across the input matrix rather than trusting they stay in step. MERGE AFTER #181 (v0.76.0). Open PRs #165, #177 and #178 reserve v0.73.0, v0.74.0 and v0.75.0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Reviewing this as part of the combined release train (#165 → #177 → #178 → #181 → #182, plus kohls-pr#112 which lowers Verified: an aborted pass no longer stops the queue, and nothing else does eitherThe diff replaces if (stats.aborted) triggers.stop();
await triggers.drain();with a comment stating "An aborted pass does NOT stop the queue either… Only disabling the probe clears it." That path does not exist. Operationally that matters most during the incident this PR is for: Smallest fix: call Verified against the live cluster: this ships inert on kohlskohls holds a config-override row kohls-pr#112 prescribes an explicit deploy-then-delete order for exactly this hazard on Also worth knowing: kohls sets only Not independently verified — worth the author's eyesThese come from a deeper review pass. I have not traced them myself; treat as leads, not conclusions.
OrderingI'd ask that #178 and kohls-pr#112 not ship in the same deploy: #178's riskiest path is triggered by a canary trip, and #112 makes trips more likely. Happy to be wrong on the unverified items — flagging rather than blocking. 🤖 Generated with Claude Code |
…at each URL produces; v0.79.0 The browser now posts what its readiness contract said. Without something on this side reading it, contracts would reproduce the exact trap they exist to avoid: a gate that fails on every render and costs a timeout, with nothing anywhere saying so. Two consumers. 1. `render_readiness`, a new metric. `verdict` is the share of renders that finished COMPLETE — the only signal that separates a render missing a widget from a good one, since both are 200, non-empty and indexable (measured under CPU contention: 8 of 15 renders finished unsatisfied and all 15 reported outcome=ok). `unmet` names the CLAUSE, so a contract that has rotted against a template change reads as one clause failing across a page type rather than as an unexplained slowdown. `satisfied_ms` is what `timeoutMs` should be tuned from: a p95 approaching the timeout means the contract is being abandoned under load and the optimisation is quietly gone. 2. `RenderExpectation`, per-URL, node-local, written on the result path beside the probe claim. A contract bounds what it NAMES, and recommendation rails cannot usefully be named — they have no server-rendered placeholder, so "every rail that exists is filled" is true of a page that ended up with one rail instead of three. Measured: a render that satisfied its contract stored 610 product links where that URL normally stores 674. The page's own history is the only oracle that covers it. It converges rather than alarming forever, which is the whole difficulty: a rail removed site-wide must not fail that URL on every future render. A shortfall is a vote, not a verdict — three in a row and the expectation is re-learned. A SUSPECTED shortfall deliberately does not re-learn, because learning from a render we believe is short would ratchet the expectation down to whatever the page just failed to produce, which is how a real regression would erase its own evidence. The comparison runs where the data already is, on the result. Handing the expectation to the renderer at claim time would cost a cross-database point read per job or a denormalization onto RenderSchedule — residency-pinned and rewritten on every render, the two costs its own schema comments exist to avoid. The browser still supports being given expectations for the in-render path; nothing needs it for detection. Both are best-effort on the render path, like recordPageClaim: a regression signal must never cost a render. Version note: main is at 0.78.0 and open PR #178 bumps to 0.75.0, which would downgrade it. This takes 0.79.0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Why
Deferring was the routine outcome of a busy pass, and it's a bad trade. The origin read that proved a URL changed has already been paid for; dropping the result throws that away and re-buys it on the next pass — which in
anchoredmode is a day later.The render queue is itself a backlog. A trigger files a row; the fleet drains it at whatever rate it can. So the thing worth bounding is how fast the writes land, not whether they land at all.
So this change is not fixing a live loss. It stands on the principle — deferring discards an origin read already paid for, and a mass-change event (a real reprice, or a widened rule) is exactly when the old default would have dropped the most — plus the heartbeat queue-depth reading, which is worth having on its own. Treat it as a safety property, not an urgent fix, and size
maxPendingagainst a mass event rather than against steady state.What changed
maxTriggersPerSweepnow defaults to0— no ceiling. It remains available as a cap on what a single pass may inject (useful while sizing a new rule, where a mistake would otherwise queue the whole corpus), but it is no longer the routine dropper.The queue outlives the pass. It's module-scoped, and the pass no longer awaits
drain(). This matters beyond tidiness: a pass that detects more change than the bounded drain can place would otherwise be held open by it, and in anchored mode a pass still running at the next anchor makes that anchor skip — turning a busy night into a missed one. An aborted pass no longer stops the queue either; what's in it was genuinely detected and its baseline is unwritten, so draining it is still correct.trigger.maxPending5,000 → 50,000, and documented as what it actually is now: the only remaining bound, and a memory bound rather than a policy one. An entry carries the observed signature (~1.4 KB), so 50,000 pending is ~70 MB per node. A refusal is an overload alarm, not a normal outcome.Two things I want to be explicit about
The stats change meaning.
triggeredanderrorsare now cumulative counters read at pass end, not per-pass totals — a trigger submitted by this pass may settle during the next.queuedis the honest per-pass number, and the newtriggerQueuePendingreports how much hadn't landed when the pass ended.Filing everything due-now flattens priority. Claims order by
nextRenderTime, so a large ready set at the same due time competes evenly — the homepage against every changed PDP. The backlog drains, but lower-priority work starves while it does. Render priority lanes (#80) are the real fix; this is a knowing tradeoff, not an oversight.Also fixes an observability gap
Queue depth now rides the sweep heartbeat, so the queue is visible while a pass runs. Previously the only reading came from a finished pass — useless for a drain whose entire purpose is to outlive the pass, and it cost real time diagnosing a live fleet earlier today.
Tests
npm testinpackages/plugin: 1106 pass, 0 fail. Lint andformat:checkclean.New test pins the default: with
maxTriggers: 0, three changed URLs givedeferred: 0,queued: 3,triggered: 3, and three baselines written after their triggers. The existing "past the trigger budget a change DEFERS" test keeps its explicit finite cap, so the ceiling still works when set.Versioning — THIS PR CURRENTLY BUMPS BACKWARDS
It sets
packages/pluginto 0.75.0, but that reservation expired: #165 and #177 merged, the 0.73.0–0.76.0 numbers collapsed into a singleprerender-v0.77.0release, andmainis now at 0.79.0 (0.78.0 raw-cache TTL, 0.79.0 readiness contracts). Merging as-is is the downgrade-of-main that #59 caused.Renumber to the next free version at merge time — read
gh release listthen, rather than reusing a number written here. See #183.🤖 Generated with Claude Code