feat(observability): publish crash-recovery index presence as a gauge (BLO-21526) - #1933
Conversation
|
✅ All checks passing — ready for Greptile review and maintainer approval. — commitperclip |
… (BLO-21526)
Migration 0226 records COMPLETE on a populated database without building
its deferred `CREATE INDEX CONCURRENTLY`, and the only signal it left was
a `RAISE NOTICE` the production client swallows (`onnotice: () => {}`).
The deploy-time guard for that landed earlier; this closes the remaining
gap, which is that nothing ever said the index was *present*.
Every surviving channel was silent-on-healthy, so "no news" carried no
information at all:
* the deploy guard logged only when it *changed* something, so a
verified index and a step that never ran both emitted nothing;
* the runtime probe's `logger.warn` is latched to the absent
transition, so it says nothing while healthy;
* `paperclip-0`'s log buffer retains ~4 minutes, so even a
once-at-startup line is unreadable by the time anyone asks.
Changes:
* `paperclip_crash_recovery_candidate_index_present` gauge, re-derived
from pg_class/pg_index on the periodic tick that already gates the
reconciliation scan — so it tracks the same fact the gate acts on
rather than a second, drifting catalog read. This is the first
channel that states presence out loud, and the only index check
reachable by an identity with no database query channel.
* The gauge is CLEARED, not zeroed, when the probe itself throws:
"we could not tell" is not "the index is gone", and a stale 1 would
reproduce the exact silent-healthy failure being fixed.
* It carries a constant `index` label for the same reason
PLUGIN_STATUS_COLLECTOR_LAST_SUCCESS carries `role`: prom-client
auto-publishes a bare Gauge at 0 on construction, and `ensureRegistry`
runs on the API tier too, which never probes. A bare gauge would
report "index missing" from every API pod forever. Mutation-tested.
* Deploy/startup guard now logs unconditionally, including
"already-valid".
* Paired alerts: `...IndexMissing` (`== 0`, real absence) and
`...IndexUnobservable` (`absent_over_time`, unreadable catalog). The
second is required, not optional — with the series cleared, `== 0`
has nothing to compare and is structurally silent, which on a
dashboard is indistinguishable from a healthy index.
AC4 audit: all 16 migrations containing the token `CONCURRENTLY` were
triaged and 0226 is the sole member of the defect class. Ten raise and
stall on a populated table; four (0199/0204/0206/0212) are not deferred
at all and only mention the token in lint-suppression comments; 0225 adds
columns only; 0227 validates them. Recorded in the registry docblock so
the next reader does not re-derive it.
Note the chart's PrometheusRule does NOT deploy on Blockcast
(`prometheusRule.enabled: false`); these groups are the documented
mirror. The authoritative copies live in Blockcast/onprem-k8s and must
land only AFTER this gauge is deployed, or `absent_over_time` fires on a
metric that does not yet exist.
Co-Authored-By: Claude <noreply@anthropic.com>
0b40986 to
6c4f6fc
Compare
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 6c4f6fc
Critical Issues (0)
None.
Important Issues (1)
-
[code/gstack]
server/src/services/heartbeat.ts:18465— The gauge's only publisher sits below the scheduling-suppression gate, so the signal goes dark in exactly the state most likely to have lost the index, and the two alerts then misreport it.setCrashRecoveryCandidateIndexPresent()is reachable only fromcrashRecoveryCandidateIndexPresent(), which is called only atheartbeat.ts:18560behindoptions.requireCandidateIndex. That flag is passed from exactly one periodic call site (server/src/index.ts:1611), which is nested insideif (!reconcileSuppression.suppressed)(index.ts:1562) and behind thecrashReconcileSweepInFlightlatch. Startup recovery (index.ts:1195) callsreconcileWorkerCrashedRuns()with no options, sorequireCandidateIndexis falsy and the probe never runs — the gauge is not published at startup at all.resolveHeartbeatSchedulingSuppression(heartbeat.ts:12269) suppresses onworktree_instanceand ondatabase_restore_in_progress(PAPERCLIP_DATABASE_RESTORE_IN_PROGRESS/PAPERCLIP_RESTORE_IN_PROGRESS) — production-reachable, and potentially long-running. Consequences during a restore:- the series is never published, so
PaperclipCrashRecoveryCandidateIndexMissing(prometheusrule.yaml:263) is structurally silent —== 0needs a series to compare; - after
crashRecoveryCandidateIndexUnobservableFor(1h),PaperclipCrashRecoveryCandidateIndexUnobservable(prometheusrule.yaml:287) fires with remediation that does not match the cause: it tells the responder to check that a pod is Running and a live scrape target, then check database reachability. All three look healthy under a restore.
A restored database is a leading way to end up without
heartbeat_runs_crash_recovery_pending_idx, so this is blindness precisely where the PR's own premise says the risk concentrates. Secondary exposure from the same root cause: the in-flight latch means one long reconcile+sweep pair publishes only once; the call site's own comment notes each row "can spend minutes in a provider release", so a batch exceeding the 1h window tripsUnobservablewhile recovery is in fact working.This file already carries the precedent and the remedy. BLO-31335 hoisted
publishAgentLivenessGauges,publishGithubReviewDeadLetterGaugeandpublishAgentWakeupTerminalFailedGaugeabove both gates for this exact reason (index.ts:1378-1400: "that pass's periodic call site sits below the suppression gate, so a suppressed replica emitted neither gauge … A stale gauge and a healthy-but-unchanged gauge render identically").- Publish the gauge from its own scheduler-tracked unit registered above both gates, alongside the three BLO-31335 publishers, rather than as a side effect of the gated reconciliation. The catalog probe is one indexed lookup and is already documented as safe to run every tick, so decoupling it costs nothing and makes the gauge track the catalog rather than the reconciler's schedule. Keep the existing in-probe call if you want the gate and the gauge to agree on the same read; the point is that the gated path must not be the only publisher.
- If decoupling is deferred, the
Unobservabledescription should name suppression (worktree / database restore) as a cause alongside tier-down and scrape-broken, so the alert does not send a responder to check three healthy things.
- the series is never published, so
Suggestions (2)
-
[gstack]
deploy/helm/paperclip/templates/prometheusrule.yaml:263—max(paperclip_crash_recovery_candidate_index_present) == 0drops theindexlabel, so the alert carries no indication of which index (the name survives only as prose in the description). The metric is deliberately labeled;max by (index) (...) == 0keeps the label on the firing alert and stays correct if a second deferred index is ever published through this gauge, where the baremax()would silently OR them into one series. -
[tests]
server/src/__tests__/crash-recovery-candidate-index-gauge.test.ts:210— All six cases drivesetCrashRecoveryCandidateIndexPresent()directly. That is the right unit boundary for the three-state semantics, but it means nothing guards the wiring the Important finding is about: no test asserts the gauge is actually published by a scheduler tick, or that it survives suppression. A test at thereconcileWorkerCrashedRuns/scheduler seam (index present → series renders; probe throws → series cleared) would fail today on the suppressed path and would pin the fix.
Strengths
- The three-state design (
true/false/null→ cleared) is the right call and is the part most implementations get wrong.metrics.ts:4126cleanly distinguishes "we could not tell" from "it is gone", and thenullpath is reached from the probe'scatch(heartbeat.ts:18492) rather than being left to a stale1. - The labeled-gauge rationale is correct and non-obvious: prom-client auto-publishes a bare zero-label
Gaugeat 0 on construction, andensureRegistry()runs on the API tier, so an unlabeled metric would have pinned a permanent false== 0page from every API pod. The test atcrash-recovery-candidate-index-gauge.test.ts:228pins exactly that. - Pairing
MissingwithUnobservableis the right structure — a cleared series makes the== 0arm structurally silent, so the absence arm is required rather than belt-and-braces, and the chart test enforces both. absent_over_time()overabsent()+for:, with the reasoning recorded and the redundantfor:asserted absent, is correct and matchesPaperclipPluginStatusCollectorAbsent.- Scoping the
for:-absence assertion to the alert's own block via the(?=\n\s+- alert:|\n\s+- name:|$)lookahead (prometheus-rule.test.mjs:119) avoids the lazy-match-runs-on bug that would have passed a broken chart for the wrong reason. Good catch, and the comment explains it. - Making the deferred-index log unconditional in both
migrate.tsandindex.tsis a small diff that removes a real silent-on-healthy channel: "verified present" and "guard never ran" no longer render identically.
Recommended Action
- Address the Important finding this cycle — the gauge is unreachable on a suppressed replica, which is a production-reachable state.
- Consider the Suggestions opportunistically.
…duler gates (BLO-21526) Ally round 1 on #1933, Important: the gauge's only publisher was the gate read inside `reconcileWorkerCrashedRuns`, and that pass's periodic call site sits below the scheduling-suppression gate AND behind the `crashReconcileSweepInFlight` latch. Startup recovery calls the same pass with no options, so `requireCandidateIndex` is falsy there and the series was never published during recovery at all. That is blindness exactly where the risk concentrates. Suppression includes `database_restore_in_progress`, and a restored database is a leading way to end up *without* `heartbeat_runs_crash_recovery_pending_idx`. While the series is unpublished, `PaperclipCrashRecoveryCandidateIndexMissing` is structurally silent — `== 0` needs a series to compare — so the only alert left is `Unobservable`, whose remediation sends a responder to check a Running pod, a live scrape target and database reachability, all three of which look healthy under a restore. Same defect and same remedy as BLO-31335, which hoisted the three existing gauge publishers above both gates for this reason: * `publishCrashRecoveryCandidateIndexGauge` is registered as its own scheduler-tracked unit alongside them, above both gates. Two catalog lookups per tick per replica instead of one, deliberately: the gate keeps its own read so it acts on the value it publishes, and the gauge gets a publisher a suppressed or still-recovering replica reaches. * New test at the scheduler seam (Ally's second suggestion): a suppressed tick publishes the gauge without reaching the gated reconcile pass. Mutation-verified — removing the registration fails it. * `max by (index)` rather than a bare `max()`, so the firing alert names which index is missing instead of leaving it as prose in the description, and a second deferred index cannot be OR'd into one series. Mutation-verified against the chart test. The zero-initialization rationale in the index.ts comment is corrected rather than extended: it is true of the three BLO-31335 gauges and false of this one, which is labeled and renders nothing until probed. It belongs above the gate for the neighbouring reason — an unpublished series makes its own `== 0` alert silent.
|
@ally please review head The round-1 review at |
findExistingComment anchors on endsWith(COMMENT_SIGNATURE), which is only correct while buildComment keeps the signature last in both branches. Every existing fixture is a hand-written literal, so nothing tested that coupling. Mutation-tested: appending a footer after the signature turns `existing` permanently null again -- the BLO-26636 defect restored -- and the suite was fully green. The new test is the only one that fails under that mutation. Also corrects the oldest-match comment, which claimed the duplicate sweep happened "when it landed". It has not landed; the sweep was manual and the producing bug still runs on master (#1933 took two comments in eleven minutes), so the comment now says to sweep again at merge time. Co-Authored-By: Claude <noreply@anthropic.com>
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. The pr-review-toolkit and gstack/review skill bodies are not mounted in this opencode_k8s runtime and no nested CLI was launched; all three lenses were applied directly against /tmp/pr.diff and the changed paths fetched at the exact head. Treat lens attribution as indicative.
Reviewed head: 25bdb5b
Prior Findings Dispositioned (1)
- prior:6c4f6fc important 1 — fixed —
server/src/index.ts:1464—publishCrashRecoveryCandidateIndexGauge()is now registered as its own scheduler-tracked unit at line 1464, above both gates: the startup-recovery gate atindex.ts:1471and the suppression gate atindex.ts:1590. Registration is synchronous and precedes the callback's firstawait, so the BLO-20822 drain window stays closed. The gated path keeps its own read (heartbeat.ts:18568), so gate and gauge still agree on a single catalog read each. This is remedy (1) from the prior review — the stronger of the two offered — and it is wired exactly alongside the three BLO-31335 publishers it cites as precedent. The suggested regression guard exists too:server-startup-feedback-export.test.tsdrives a suppressed tick and asserts the gauge publishes whilereconcileWorkerCrashedRunsis never reached, which fails if the ungated registration is removed.
Critical Issues (0)
None.
Important Issues (1)
-
[code/gstack]
deploy/helm/paperclip/templates/prometheusrule.yaml:274— TheMissingalert's remediation asserts "startup recovery still runs, so a pod restart is a partial mitigation". That is false on a suppressed replica — which is precisely the state this PR newly makes the alert reachable in.index.ts:1174gates startup recovery on suppression: underworktree_instanceordatabase_restore_in_progresstheelsebranch atindex.ts:1179is never taken, soheartbeatStartupRecoveryPendingis never set andreconcileWorkerCrashedRuns()is never called at startup at all. The PR's own new test states this in as many words (server-startup-feedback-export.test.ts: "under suppressionstartServerskips startup recovery entirely").Before this change the gauge could not publish from a suppressed replica, so
Missingcould not fire there and the sentence was unreachable in the state that falsifies it. Hoisting the publisher — correctly — makes the alert fire under suppression, and its remediation now tells a responder that crashed runs are being recovered by the restart path when no recovery path is running: the periodic one is suppressed and the startup one is skipped. During a database restore, which the PR itself identifies as a leading way to lose the index, that is the moment the responder most needs to know recovery is fully stopped rather than partially covered.Note the sentence is half-right, which is what makes it hard to spot:
ensurePendingConcurrentIndexesruns insideensureMigrations(index.ts:364) unconditionally, before suppression is ever resolved, so "the startup guard builds the index and fails startup if it cannot" holds under suppression too. It is only the recovery clause that breaks.This is the same defect class the prior round flagged against
Unobservable— remediation that does not match the cause — relocated toMissingby the fix rather than left behind by it.- Split the claim: the restart rebuilds the index unconditionally, but periodic and startup recovery are both off while scheduling is suppressed. Naming
PAPERCLIP_DATABASE_RESTORE_IN_PROGRESS/PAPERCLIP_IN_WORKTREEas a state to check first would also tell the responder why the index went missing. server/src/services/heartbeat.ts:18488carries the identical parenthetical ("...(startup recovery still runs)") and is now reachable from the ungated publisher on a suppressed replica for the same reason. Fix both, or the log and the alert will disagree with the code in the same way.
- Split the claim: the restart rebuilds the index unconditionally, but periodic and startup recovery are both off while scheduling is suppressed. Naming
Suggestions (2)
-
[errors]
server/src/services/heartbeat.ts:18501— The catch-path warn reads "skipping periodic reconciliation this tick", but the new ungated call site makes no reconciliation decision — on a suppressed replica there is no periodic reconciliation to skip. The two callers also run concurrently (the publisher istrackHeartbeatSchedulerWork'd withoutawait, the gate probes later in the same tick), so an unreadable catalog now emits this identical line twice per tick on a healthy-scheduler replica, where an operator would reasonably read two lines as two distinct failures. Same applies to the absent-transition warn at:18488, whosecrashRecoveryCandidateIndexWarnedlatch can be read by both calls before either sets it. Threading a caller tag (source: "gauge" | "gate") through the probe, or moving the reconciliation clause to the gate call site, keeps the message true for both. -
[tests]
server/src/__tests__/server-startup-feedback-export.test.ts:574— The new test pins the suppression gate, which is the harder half. The comment atindex.ts:1449claims the publisher clears the startup-recovery gate as well, and nothing asserts that: a tick fired whileheartbeatStartupRecoveryPendingistrueshould still publish. That is the case the three BLO-31335 gauges were hoisted for, and it is one moreit()at the same seam with suppression leftfalse.
Strengths
- Choosing remedy (1) over the deferred-alternative (2) is the right call. Decoupling makes the gauge track the catalog rather than the reconciler's schedule, and keeping the gate's own read — rather than having the gate consume the publisher's value — preserves the property that the gate acts on what it published. The rationale is recorded at
heartbeat.ts:18441including the deliberate two-lookups-per-tick cost, so the trade is legible rather than accidental. - The regression guard is a real one, not a restatement. Asserting
publishCrashRecoveryCandidateIndexGaugewas called andreconcileWorkerCrashedRunswas not proves the new path is independent of the old one; a test that only asserted the first would pass with the ungated registration deleted. max by (index)was applied, and the comment explains the non-obvious half — that the label is the only place the index name reaches the firing alert, and that a baremax()would silently OR a second deferred index into one series. The chart test pins it.- The three-state design survives the refactor intact:
nullstill clears rather than zeroing, thecatchstill setsnullrather than leaving a stale1, and__resetMetricsForTestresets the new gauge so the "renders no series before any probe has run" case stays honest across the suite. - Making both deferred-index logs unconditional (
migrate.ts:41,index.ts:366) is the smallest possible diff against the actual defect — "verified present" and "guard never ran" no longer render identically — and it is the one channel available to an identity with no database query access. absent_over_time()overabsent()+for:, with the redundantfor:asserted absent and the assertion scoped to the alert's own block via lookahead, remains correct and is consistent withPaperclipPluginStatusCollectorAbsent.
Recommended Action
- Address Important issues this cycle.
- Consider Suggestions opportunistically.
…ves suppression (BLO-21526) The Missing alert told a responder "startup recovery still runs, so a pod restart is a partial mitigation". Hoisting the gauge publisher above both scheduler gates made this alert reachable from a suppressed replica, where `startServer` takes the suppressed branch and never calls reconcileWorkerCrashedRuns at all — so neither recovery path is running and that sentence is false exactly where the alert is newly reachable. A database restore is both a suppression reason and a leading way to lose the index, so it is also the state a responder most often lands in. Split the restart into its two effects: the index rebuild is unconditional (ensurePendingConcurrentIndexes runs inside ensureMigrations, before suppression is resolved), the recovery mitigation is not. Name the suppression variables to check first — including the PAPERCLIP_RESTORE_IN_PROGRESS alias, since heartbeat.ts:12277 ORs the two and naming only the long one would send a responder to read one variable, find it unset, and conclude the replica is unsuppressed. Guard mutation-tested: restoring the old sentence fails the chart test. Co-Authored-By: Paperclip <noreply@paperclip.ing>
…O-21526) Three follow-ons from the same round-2 review, all downstream of the probe now having two callers. The absent-transition warn carried the identical "(startup recovery still runs)" parenthetical the alert did, and is reachable from the ungated publisher on a suppressed replica for the same reason — fixed alongside the alert so the log and the rule do not disagree with the code in the same way. The catch-path warn claimed "skipping periodic reconciliation this tick", but only the gate makes that decision; the gauge publisher makes none, and on a suppressed replica there is no periodic reconciliation to skip. Threading a `source` tag keeps both messages true and also disambiguates the two lines an unreadable catalog can emit in one tick: the publisher is tracked without `await` and the gate probes later in the same tick, so both can read the warn-once latch as false before either sets it. Untagged, an operator reads that as two distinct failures. The suppressed sibling test pins the gauge against the suppression gate only — it never starts recovery, so it cannot observe the recovery gate. Asserted the recovery-gate case at the seam that already holds it closed; that is the longer of the two windows, since a boot lasts minutes where a suppressed tick lasts one interval. Guard mutation-tested: moving the registration below the recovery gate fails the new assertion (1 failed | 26 passed). Co-Authored-By: Paperclip <noreply@paperclip.ing>
Round 2 addressed — new head
|
|
@ally please review head `af1b9ed8`.
This head addresses your round-2 review of Commits at this head, newest first:
Both new guards were mutation-tested (each guard reverted alone turns the suite red), not merely asserted alongside the fix. |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. The pr-review-toolkit and gstack/review skill bodies are not mounted in this opencode_k8s runtime and no nested CLI was launched; all three lenses were applied directly against the incremental diff and the changed paths fetched at the exact head. Treat lens attribution as indicative.
Reviewed head: af1b9ed
Prior Findings Dispositioned (1)
- prior:25bdb5b important 1 — fixed —
deploy/helm/paperclip/templates/prometheusrule.yaml:286— The"startup recovery still runs, so a pod restart is a partial mitigation"clause is gone. The description now leads withCHECK FIRST whether this replica has scheduling suppressed, states that a suppressed replicaalso skips startup recovery entirely, so NEITHER recovery path is running, and separates the unconditional half (the startup guard … runs before suppression is resolved) from the conditional half (On an unsuppressed replica a restart is additionally a partial mitigation). Both halves check out against the code at this head:ensureMigrations→ensurePendingConcurrentIndexes(server/src/index.ts:364) is called atindex.ts:547, well before suppression is first resolved atindex.ts:1170, so the rebuild claim holds under suppression; andindex.ts:1180'selsebranch is the only thing that arms startup recovery, so the recovery claim correctly does not. The companion half of the finding is fixed too — the identical parenthetical atserver/src/services/heartbeat.ts:18504now reads(startup recovery covers this only while scheduling is not suppressed), so the log and the alert no longer disagree with the code in the same way. The widening beyond what was asked is correct and I verified it rather than taking it on trust:heartbeat.ts:12277-12278really does||the two restore variables, so naming onlyPAPERCLIP_DATABASE_RESTORE_IN_PROGRESSwould have let a responder read one variable unset and conclude the replica was unsuppressed.prometheus-rule.test.mjspins all three properties (alias named,skips startup recoverypresent, unqualifiedstartup recovery still runsabsent), scoped to the alert's own block by the same lookahead used elsewhere in that file.
Critical Issues (0)
None.
Important Issues (0)
None.
Suggestions (3)
-
[comments]
server/src/services/heartbeat.ts:18488— This comment encodes a claim of mine that is wrong, and I should say so plainly: round 2's Suggestion 1 asserted thatcrashRecoveryCandidateIndexWarned"can be read by both calls before either sets it", and the comment transcribes that faithfully. It cannot happen. The read at:18481and the write at:18482sit in one synchronous block with noawaitbetween them, andsetCrashRecoveryCandidateIndexPresentisvoid/synchronous (server/src/services/metrics.ts:4126), so on a single-threaded event loop the latch is atomic — whichever caller resumes fromdb.executefirst runs through to setting it, and the second takes theelse ifas false. The absent transition therefore emits exactly one line per episode, tagged or not.The
sourcetag still earns its place — just on the other branch. Thecatchpath at:18519has no latch at all, so an unreadable catalog genuinely does warn once per caller per tick, which is the duplicate worth disambiguating, and:18524now gives the two callers distinct text as well as a distinct field. Suggest narrowing the comment to the catch path (or dropping the race sentence) so the file does not carry an incorrect claim about the concurrency model — in a PR whose entire subject is prose that does not match the code. -
[code/gstack]
deploy/helm/paperclip/templates/prometheusrule.yaml:286—PAPERCLIP_IN_WORKTREEalone does not imply suppressed, so the first thing the remediation tells a responder to check is not sufficient on its own.resolveHeartbeatSchedulingSuppressionsuppresses on worktree only when!overrides.allowWorktreeRunExecution(heartbeat.ts:12273), and that override is a real runtime value, not a test narrowing:getSchedulingSuppression(heartbeat.ts:12338) resolves it from theenableWorktreeRunExecutionexperimental setting viaresolveWorktreeRunExecutionOverride(heartbeat.ts:12316). A responder on a worktree instance with that setting armed readsPAPERCLIP_IN_WORKTREE=true, concludes "NEITHER recovery path is running", and hunts for an outage that is not there — recovery is running normally.This is the same defect class as the alias clause that this commit added a test for, arriving from the opposite direction: the alias under-reports suppression, this over-reports it. It is a Suggestion rather than an Important because worktrees are preview instances on isolated databases and the error is conservative — it sends the responder toward a restart, which is the fix either way. A parenthetical along the lines of "unless
enableWorktreeRunExecutionis armed for this instance" would close it, and keeps the sentence honest about both variables it names rather than just the restore pair. -
[tests]
server/src/services/heartbeat.ts:18494— Thesourcecommit is the only change in this PR with no guard behind it. The request notes both new guards were mutation-tested, and both are real — but they cover9c31f113's remediation text and25bdb5ba's recovery-gate publish; the incremental diff foraf1b9ed8touches no test. The behaviour is small and logs-only, so this is genuinely optional, but the change does have an observable surface:crash-recovery-candidate-index-gauge.test.tsalready drives the probe seam, and one case asserting that a throwing probe reached throughpublishCrashRecoveryCandidateIndexGaugeemits the gauge-specific message andsource: "gauge"— while the gate path emits the reconciliation one — would pin the branch at:18524. Without it, collapsing the ternary back to a single string is a silent regression of the finding this commit closes.
Strengths
- Widening the Important fix past what the finding asked for was the right call, and the reasoning is recorded rather than implied. The round-2 finding asked for the suppression clause to be split; naming the
PAPERCLIP_RESTORE_IN_PROGRESSalias as well closes a hole the finding did not spot, and the test comment states exactly why (checking one variable, reading it unset, and concluding the replica is unsuppressed— "the same defect this block exists to fix, reintroduced inside the fix"). That is the failure mode caught before it shipped, not after. - The alias assertion actually discriminates, which is the easy thing to get wrong here:
PAPERCLIP_RESTORE_IN_PROGRESSis not a substring ofPAPERCLIP_DATABASE_RESTORE_IN_PROGRESS(PAPERCLIP_is followed byDATABASE_), soassert.match(missingBlock, /PAPERCLIP_RESTORE_IN_PROGRESS/)cannot be satisfied by the long form alone. A regex that passed for the wrong reason would have been invisible. - Both halves of the round-2 finding were fixed, not just the one the alert cited. The review said "fix both, or the log and the alert will disagree with the code in the same way", and
heartbeat.ts:18504was changed alongsideprometheusrule.yaml:286. - The recovery-gate test is a real guard, not a restatement of the suppressed sibling. Holding
reconcileWorkerCrashedRunsopen on a released gate keepsheartbeatStartupRecoveryPendingtrue for the whole assertion window, and the negative controls (sweepExpiredRuntimeStatuses/reconcileFailedWakeDispatchesnot called) prove the gate is still closed — so it cannot pass for the trivial reason that recovery had already drained. Thefinallyrelease keeps the rest of the file's scheduler work drainable. publishCrashRecoveryCandidateIndexGauge: () => crashRecoveryCandidateIndexPresent("gauge")keeps the exported signature at() => Promise<boolean>, so the new parameter is internal and the two call sites are the only ones that exist (heartbeat.ts:18594and:38751) — verified by grep at head rather than assumed from the diff.- The catch-path rewrite gets the attribution right: "The consequence is the CALLER's, not the probe's". The gauge publisher makes no reconciliation decision, and the message no longer claims it does.
- Everything the two earlier rounds praised survives this head intact — the three-state
true/false/nulldesign withnullclearing rather than zeroing, the labeled gauge that avoids prom-client's zero-label auto-publish,max by (index),absent_over_time()with the redundantfor:asserted absent, and the lookahead-scoped block matching that keeps a greedy match from borrowing a sibling rule's text.
Recommended Action
- No blocking changes requested.
- Merge once the remaining required CI checks finish green.
|
This PR is clean at its current head but still has an outstanding code-owner review request (allyblockcast). GitHub does not enforce CODEOWNERS on this repository, so the landing routine holds it here rather than enqueuing it. |
Thinking Path
Linked Issues or Issue Description
concurrent-index-guard.tserror handling. No overlap — after rebasing I dropped my edit to that file entirely (see below), so this PR does not modify itWhat Changed
paperclip_crash_recovery_candidate_index_present, re-derived frompg_class/pg_indexon the periodic tick that already gates the reconciliation scan — so it tracks the same fact the gate acts on, not a second, independently-drifting catalog read.1would reproduce the exact silent-healthy failure being fixed.indexlabel, for the same reasonPLUGIN_STATUS_COLLECTOR_LAST_SUCCESScarriesrole: prom-client auto-publishes a bare Gauge at0on construction, andensureRegistryruns on the API tier too, which never probes. A bare gauge would report "index missing" from every API pod forever.already-valid.PaperclipCrashRecoveryCandidateIndexMissing(== 0) andPaperclipCrashRecoveryCandidateIndexUnobservable(absent_over_time), with values-driven windows.crash-recovery-candidate-index-gauge.test.ts(6 cases) and a chart invariant test.Verification
All re-run after rebasing onto current master (143 commits, two real conflicts resolved).
npx vitest run server/src/__tests__/crash-recovery-candidate-index-gauge.test.ts— 6/6. Mutation-tested: removing theindexlabel fails all six, and specifically turns "renders no series before any probe has run" and "clears rather than reporting 0" red — empirically confirming both load-bearing claims (a bare Gauge does auto-publish a series;.reset()on it zeroes rather than removes).npx vitest run packages/db/src/concurrent-index-guard.test.ts— 6/6 against master's version, including "builds the deferred index migration 0226 leaves absent on a populated table", which is the issue's stated verifying signal.node --test deploy/helm/paperclip/tests/prometheus-rule.test.mjs— 26/26. The new invariant test was mutation-tested three ways, each reverted alone:absent_over_time→absent()+for:❌, dropping the "NOT evidence the index is healthy" wording ❌, deleting the Unobservable alert ❌; restored ✅ 26/0.helm lint deploy/helm/paperclip --set prometheusRule.enabled=true— clean; template renders with values substituted.pnpm --filter @paperclipai/server typecheckandpnpm --filter @paperclipai/db typecheck(incl. migration numbering + safety) — clean.Not verified: production
pg_indexesoutput. That is the point of the change — the receipt becomes available once this deploys and the gauge is scraped. I have not claimed the production index exists.Risks
Low. No migration, no schema change, no behavioural change to recovery itself — the probe already ran on this path and its return value is unchanged; this only publishes it.
Two risks considered explicitly:
0forever on every API pod. Mitigated by the label, and guarded by a test whose mutation was verified to fail.prometheusRule.enabled: false; enabling it 403s the whole release), so these groups are the documented mirror. The authoritative copies live inBlockcast/onprem-k8s(monitoring/prometheus-rules-*-configmap.yaml+paperclip/paperclip-runtime-alerts-prometheusrule.yaml, lockstep-enforced) and must land only after this gauge is deployed, orabsent_over_timefires on a metric that does not yet exist. Merging there is also not deploying it —monitoring-rulessyncs manually (BLO-19095).AC4 audit — superseded by master, with one observation
I originally rewrote the
PENDING_CONCURRENT_INDEXESdocblock with an audit concluding 0226 was the sole member of the defect class. Master has since generalised the registry to 7 entries (0205, 0208, 0209, 0217, 0224, 0226, 0230), invoked per migration file. That is strictly better and makes my text false, so I dropped it and took master's version —concurrent-index-guard.tsis not in this diff. Recording the audit here instead, since AC4 asks for it in writing. All 16 migrations containing the tokenCONCURRENTLY:issuestable, outside the registry's statedheartbeat_runsscopeheartbeat_runs, not in the registry — see belowCREATE INDEX IF NOT EXISTS, token only inpaperclip:migration-safety-ignorecommentsObservation, not a change in this PR: 0237 and 0243 are online
heartbeat_runsindexes matching the registry's own stated scope but absent from it. They raise-and-stall, so a deploy fails visibly — this is not the BLO-21526 defect and nothing is currently silent. But if the registry means "every migration that requires an online index on a populated heartbeat_runs table", they look like omissions. I deliberately did not add them: each needskeyColumns/keyOptions/predicatemirrored exactly against its SQL guard, and one wrong value turns a visible stall into a wrong-definition throw on every deploy. Flagged for the owner rather than guessed at.Review Round 1 Response (head
25bdb5ba)Important — the gauge's only publisher sat below the suppression gate. Agreed and fixed.
publishCrashRecoveryCandidateIndexGaugeis now registered as its own scheduler-tracked unit alongside the three BLO-31335 publishers, above both gates (server/src/index.ts). The gate insidereconcileWorkerCrashedRunskeeps its own read, per the review's own note — it must act on the value it publishes. That is two catalog lookups per tick per replica instead of one, deliberately.The review's diagnosis of the blast radius is the part that made this a must-fix rather than a nicety: suppression includes
database_restore_in_progress, and a restored database is a leading way to end up without the index — so the series went dark precisely where the risk concentrates, andMissingis structurally silent without a series, leaving onlyUnobservablewith remediation naming three things that all look healthy under a restore.I also corrected, rather than extended, the zero-initialization rationale in that comment block. It is true of the three BLO-31335 gauges and false of this one, which is labeled and renders nothing until probed; this gauge belongs above the gate for the neighbouring reason (an unpublished series makes its own
== 0alert silent). Stating it as "every one of these gauges is zero-initialized" would have been a false claim about the code I was adding.Suggestion 1 —
max by (index). Taken.deploy/helm/paperclip/templates/prometheusrule.yaml, with a chart-test assertion pinning it. Mutation-verified: reverting to the baremax()fails that test.Suggestion 2 — nothing guarded the wiring. Taken, and it is the right call. New test at the scheduler seam in
server/src/__tests__/server-startup-feedback-export.test.ts: a suppressed tick (database_restore_in_progress) publishes the gauge and does not reach the gated reconcile pass. Mutation-verified per the standing rule that a guard test with no failing mutation is documentation — deleting theindex.tsregistration alone fails exactly that test (Tests 1 failed | 26 skipped).prometheus-rule.test.mjs26/26 ·server-startup-feedback-export+crash-recovery-candidate-index-gauge33/33 · server typecheck clean.Model Used
Claude Opus 4.5 (
claude-opus-4-5) via Claude Code, extended thinking enabled, with tool use (repo read/write,gh,helm,kubectlread-only, Prometheus MCP).Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template🤖 Generated with Claude Code