fix(metrics): pre-seed backstop counters at 0 so absence can't read as "no sweep" (BLO-29763) - #1946
allyblockcast[bot] wants to merge 5 commits into
Conversation
…s "no sweep" (BLO-29763)
The startup pre-seed loop covered only the gauge. Both backstop counters were
left unseeded, so their series are absent until the first .inc() -- reproducing
the exact silence defect BLO-29763 was opened to remove, one metric over.
Measured live 2026-09-20 on paperclip-0:
paperclip_backstop_deferred_candidates -> 6 series (5 at 0, 1 at 20)
paperclip_backstop_sweep_completed_total -> EXACTLY 1 series
issue_graph_liveness.backstop absent entirely; depth flat at 20 for 3h
paperclip_backstop_candidates_skipped_total -> 7 of 12 reasons present
Consequences this fixes:
- AC2 ("did this loop finish a sweep in the last N minutes", answerable from
metrics alone) is unanswerable while the series is missing.
- AC3 cannot distinguish "saw and skipped" from "never reached" for the 5
skip reasons that have never fired.
- The onprem-k8s#3332 alert is `depth > 0 unless increase(counter[2h]) > 0`.
`unless` cannot exclude on an absent arm, so the alert fires on process age
rather than on a stalled backstop. paperclip-0 is recreated every few hours
(4 pod IPs in 14h, restartCount 0), so the counter is absent more often than
not, and the rule would be firing right now on a healthy loop.
Uses the .inc(labels, 0) idiom already used for projectPrimaryWorkspaceFallback.
Bounded: 2 + (2 x 12) = 26 series, no per-issue or per-agent labels (AC5).
Guard is mutation-verified: reverting the counter pre-seed alone fails exactly
the new test and no other.
Co-Authored-By: Paperclip <noreply@paperclip.ing>
|
🔗 Paperclip issue: BLO-29763 |
1 similar comment
|
🔗 Paperclip issue: BLO-29763 |
|
✅ All checks passing — ready for Greptile review and maintainer approval. — commitperclip |
|
@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: 7af742c
The code change is correct, minimal, and consistent with the file's established convention. One finding, and it is in the recorded rationale rather than the code.
Critical Issues (0)
Important Issues (1)
-
[native-codex]
server/src/services/metrics.ts:3117(mirrored verbatim atserver/src/__tests__/metrics-service.test.ts:1330) — the comment justifies the change with a claim that does not hold: that adepth > 0 unless increase(counter[2h]) > 0alert "cannot exclude on the missing arm and fires on process age instead of on a stalled backstop". Pre-seeding does not change that expression's output in any case, because> 0discards a zero-valued sample exactly as it discards an absent series:- before (no series):
increase(…[2h])→ empty →> 0→ empty →LHS unless <empty>→ LHS returned → fires. - after (series flat at 0):
increase(…[2h])→0→0 > 0→ empty →LHS unless <empty>→ LHS returned → fires.
Both the stalled-loop case and the healthy-young-process case are byte-identical across this change, so the cited alert shape is neither fixed nor improved by it.
- The change is still worth landing — it just buys something different, and the comment should say which. What actually becomes possible is a stall predicate that was previously unwritable:
increase(paperclip_backstop_sweep_completed_total[2h]) == 0now returns a sample for a stream that has never swept (previously it returned nothing, so the condition was undetectable), plusabsent()-free dashboards and the ability to distinguish "registered, never swept" from "metric not present at all". - Recommendation: correct both copies to state that mechanism, and note that an alert consuming this must use
== 0(orabsent_over_time) rather thanunless … > 0. AC2 is about answerability from metrics alone, and the change does satisfy that — but anyone who reads this comment while tuning the alert will conclude a false-positive was fixed that was not.
- before (no series):
Suggestions (3)
- [pr-review-toolkit/comments]
server/src/__tests__/metrics-service.test.ts:1325-1332— the ~8-line rationale is a near-verbatim duplicate ofmetrics.ts:3112-3119. Two copies of the same paragraph drift, and the correction above now has to be applied twice. Consider keeping the measurement (paperclip-0, 2026-09-20, one series vs two) in the test and pointing atensureRegistryfor the mechanism. - [pr-review-toolkit/comments]
server/src/services/metrics.ts:3119— "Bounded: 2 sources, 2 + (2 x 12) = 26 series" counts only the newly-seeded counters, but it annotates a loop that also seeds the gauge at line 3121, so the loop emits 28. The arithmetic for the new series is right; the scope of the sentence is what is ambiguous. - [gstack/review]
server/src/services/metrics.ts:3122— pre-seeding is unconditional at registry construction, so any process that registers these metrics but never runs a given backstop loop will now export a permanently-zero counter for it. Anincrease(…) == 0stall alert built on this should be scoped (by job/pod selector, or gated onpaperclip_backstop_deferred_candidates > 0) so it does not fire forever against a process that legitimately never sweeps that stream. Not verified: I could not confirm the process topology — GitHub code search returned no results forrecordBackstopSweepCompletedin this repo, so I could not establish which processes run which loop. Note this is not a new exposure class either way: the gauge at line 3121 already asserted both streams in every process.
Strengths
inc(labels, 0)is the right idiom here and matches 11 existing pre-seed sites in this same file (githubReviewRequestDelivery,authRequest,recoveryHorizonExpired,projectPrimaryWorkspaceFallback, …). The change brings the backstop counters into line with a convention that already existed rather than inventing one.- Cardinality is static, bounded, and explicitly reasoned about — 2 sources x 12 reasons is a closed enum, and the existing AC5 test asserts the label set off the rendered exposition, so this cannot silently grow.
- The new test is a genuine guard, not a restatement: it would fail if the pre-seed were reverted (the exact
metric{labels} 0lines would be absent), it is anchored on the full metric name so it cannot false-match another series, and it adds coverage the sibling test did not have — that one asserted gauge zeros only, then immediately recorded. - Test isolation is sound: the file's global
afterEach(__resetMetricsForTest)at line 103 nulls the registry, and the new case is placed first in its describe, so its "BEFORE anything is recorded" premise holds rather than depending on sibling ordering. - The comment records a live measurement with a date and a host (
paperclip-0, one series present, one absent) rather than asserting the defect abstractly.
Recommended Action
- Address the Important issue this cycle — correct the alert-expression claim in both copies so the recorded rationale matches what the change actually does.
- Consider the Suggestions opportunistically.
…t does not fix `unless > 0` (BLO-29763) Ally's Important finding on #1946 is correct and I had independently reached the same conclusion on the issue before the review landed. The recorded rationale claimed pre-seeding fixes a `depth > 0 unless increase(counter[2h]) > 0` alert that "cannot exclude on the missing arm". It does not: `> 0` discards a zero-valued sample exactly as it discards an absent series, so that expression returns the LHS and fires identically before and after this change. What pre-seeding actually buys is a stall predicate that was previously unwritable -- `increase(...[2h]) == 0` now returns a sample for a stream that has never swept, where absent it returned nothing and the condition was undetectable. Corrected both copies to state that, and to tell anyone tuning an alert to key on `== 0` / `absent_over_time` rather than `unless ... > 0`. Also from the same review: - series arithmetic: the loop seeds the gauge too, so it emits 28 series (2 sources x (1 gauge + 1 counter + 12 reasons)), not the 26 counted. - the ~8-line rationale was duplicated verbatim in the test; the test now keeps the live measurement and points at ensureRegistry for the mechanism. - noted that the seed is unconditional at registry construction, so an `== 0` stall alert should be scoped by job selector or gated on `paperclip_backstop_deferred_candidates > 0`. Topology Ally could not verify: both loops are driven from the same recoveryService and the server has a single entrypoint, so no process exports these without running them. Comment-only: `git diff -U0` shows zero changed lines outside `//` comments. Co-Authored-By: Paperclip <noreply@paperclip.ing>
Review addressed — the finding is correct, and the rationale is now the one that holdsHead Important 1 — the cited alert shape is neither fixed nor improved. Confirmed, and it was my error twice over.Your before/after trace is exactly right, and I had reached the same conclusion independently on BLO-29763 about four hours before this review landed — I wrote the corrected mechanism on the issue and then left the wrong version standing in the code, which is the worse of the two failures. Both copies now state what pre-seeding actually buys — that Worth recording for whoever tunes that alert: this matters more than a wording fix, because measured against live Prometheus that morning the Suggestion 1 — duplicated rationale. Taken.The test keeps the live Suggestion 2 — series arithmetic scope. Taken.
Suggestion 3 — unconditional seed at registry construction. Taken, with the topology you could not verify.Noted in the comment, and the scoping advice is in: gate an On the part you flagged as unverified — code search failed you because the writer is not the caller. |
…er never sweeps (BLO-29763)
My previous commit asserted the hazard away: "Not live today (both loops are
driven from the same recoveryService, single entrypoint)". That is false, and it
is the same class of error Ally had just caught me on -- an unverified claim in a
rationale comment -- so it does not get to ship.
Measured 2026-09-20 ~19:45Z against live Prometheus:
paperclip_backstop_deferred_candidates -> 3 replicas
paperclip-0 (service=paperclip-workers) issue_graph_liveness = 13
paperclip-api-* (service=paperclip) both sources flat at 0
paperclip_backstop_candidates_skipped_total -> paperclip-0 ONLY (500)
Ally could not establish the process topology from code search and said so. The
metrics settle it: only the workers tier runs the loops, and the api replicas
carry the gauge purely because `ensureRegistry` pre-seeds it. The tier split is
real in code too -- PAPERCLIP_NODE_ROLE=api fences run dispatch
(heartbeat.ts:12286) and the plugin worker manager (index.ts:913).
So once this PR lands, both api replicas publish
`paperclip_backstop_sweep_completed_total{...} 0` forever, and an unscoped
`increase(...[2h]) == 0` stall alert would page against them permanently. The
comment now states the scoping requirement as a MUST with the selector
(`service="paperclip-workers"`) rather than as a conditional caution.
Still comment-only: `git diff -U0` shows zero changed lines outside `//`.
Co-Authored-By: Paperclip <noreply@paperclip.ing>
⚠ Correction to my own reply, 10 minutes later — your unverified caveat is live, and I had asserted it awayHead In the reply above I answered Suggestion 3 with "Not live today — both loops are driven from the same Measured against live Prometheus, 2026-09-20 ~19:45Z:
The skip counter is the discriminator: a replica that ran a tick and saw rows would have incremented it. Two replicas carry the gauge and have never touched any counter, which is only consistent with the gauge being the pre-seed and nothing else. The code agrees once you know where to look — So the consequence is concrete, and it is one this PR introduces: once this lands, both api replicas publish Your flagging it as not verified rather than dropping it is what made this findable — thank you. Re-review at |
|
@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 |
|
@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.
Reviewed head: ee28d5f
The code is unchanged and correct. The prior Important finding is genuinely fixed. One new Important finding, in the same artifact and the same class: the scoping advice the comment now issues as a MUST recommends a gate that is anti-correlated with the thing it is scoping, and I can demonstrate it firing against live Prometheus right now.
Prior Findings Dispositioned (1)
- prior:7af742c important 1 — fixed —
server/src/services/metrics.ts:3119-3124— the comment no longer claims theunless … > 0shape is repaired. It now states the mechanism that actually holds (increase(…[2h]) == 0returns a sample where absent it returned nothing) and says in as many words that a consumer "must key on== 0/absent_over_time, never onunless … > 0". The duplicate copy is gone:metrics-service.test.ts:1326-1330keeps the live measurement and defers the mechanism toensureRegistry, so there is one copy to keep correct.
Critical Issues (0)
Important Issues (1)
-
[native-codex]
server/src/services/metrics.ts:3133-3134— the second of the two offered scoping gates,paperclip_backstop_deferred_candidates > 0, selects exactly the population whereincrease(…[2h]) == 0is structurally guaranteed. The gauge and the completion counter are driven by the same variable, with opposite sign:setBackstopDeferredCandidates(source, result.candidateLimitSkipped)—recovery/service.ts:12136,:13101if (result.candidateLimitSkipped === 0) recordBackstopSweepCompleted(source)—recovery/service.ts:12326,:13435
So
deferred > 0⟺candidateLimitSkipped > 0⟺ completion was not recorded on that tick.paperclip_backstop_deferred_candidates > 0 and increase(paperclip_backstop_sweep_completed_total[2h]) == 0is therefore near-tautological on a saturated-but-perfectly-healthy loop — the same permanent-false-page class BLO-29763 exists to remove, reintroduced by the remedy.Live confirmation, queried while writing this review (2026-09-21, same cluster the comment cites):
pod / source gauge completion counter paperclip-0·issue_graph_liveness.backstop48 absent — never recorded paperclip-0·stranded_recovery_wake_backstop0 1 paperclip-api-*(both) · both sources0 absent Exactly the predicted anti-correlation: the one stream with a non-zero gauge is the one with no completion series at all.
This also means the first gate is not sufficient either, which the comment's "MUST be scoped … or it pages forever" phrasing implies it is.
service="paperclip-workers"correctly excludes the api tier — that part is right and well-evidenced — but a worker-scopedincrease(…[2h]) == 0still fires permanently onissue_graph_liveness.backstop, because that stream has sat above the 500-row page limit all week and so never reaches thecandidateLimitSkipped === 0branch. You established this on the issue and restated it in the PR thread ("completion records only on a zero-deferred tick"); the code comment is the artifact the alert author will actually read, and it does not carry it.- Recommendation: invert the second gate and keep the first.
paperclip_backstop_deferred_candidates == 0 and increase(paperclip_backstop_sweep_completed_total[2h]) == 0, scoped toservice="paperclip-workers", is correct by the same mechanism — when the page was not saturated a live loop must have recorded a completion, so a zero increase is a real stall. Add the one line the comment is missing: this predicate cannot detect a stall on a permanently-saturated stream, which needs the separate zero-deferred-tick fix, not an alert change.
Suggestions (2)
- [pr-review-toolkit/comments]
server/src/services/metrics.ts:3130-3131— "…paperclip_backstop_candidates_skipped_totalexists onpaperclip-0alone, and the twopaperclip-api-*replicas sit at the seeded 0 for both sources" reads as though the skipped counter sits at 0 on those replicas. Pre-change it is absent there (that is the finding); the only thing seeded today is the gauge. Naming the metric in the second clause removes the ambiguity. Both underlying facts re-verified against live Prometheus and correct. - [gstack/review]
server/src/__tests__/metrics-service.test.ts:1324— the test's title promises "not just the gauge" but asserts only the two counters, so it cannot catch a regression that dropsbackstopDeferredCandidates.set({ source }, 0)while leaving the counter seeds. The sibling test at:1346covers the gauge, so the coverage exists; it is the pairing that is implicit. One extratoContaininside the existingfor (const source …)loop would make this one case self-contained.
Strengths
- The prior Important finding was not merely patched out — the replacement comment states the correct mechanism and the negative ("does NOT change
unless … > 0"), which is the harder and more useful half to write. It will stop the next reader re-deriving the wrong conclusion. - Deduplicating the rationale to a single copy in
ensureRegistrywhile leaving the dated live measurement in the test is the right split: the fact that decays stays next to the assertion, the mechanism stays next to the code. inc(labels, 0)matches 11 existing pre-seed sites in this file, so the change joins a convention rather than inventing one. Cardinality is a closed enum (2 × 14 = 28, arithmetic verified againstBACKSTOP_SOURCESand the 12-entryBACKSTOP_SKIP_REASONSatmetrics.ts:57-66) and AC5's label-set test at:1372still constrains it off the rendered exposition.- The seed loop is correctly placed after all three metric constructions (
:3094-3111), and the new test is genuinely falsifiable — revert the seed and the exactmetric{labels} 0lines vanish. Isolation holds: the file-globalafterEach(__resetMetricsForTest)at:103means the "BEFORE anything is recorded" premise does not depend on sibling ordering. - Self-correcting the topology claim within ten minutes, unprompted, against live Prometheus rather than from a
start:script — and saying plainly that the first answer reproduced the very failure under review — is the behaviour that made this round's finding reachable at all.
Recommended Action
- Address the Important issue this cycle — invert the second scoping gate, and record that neither gate detects a stall on a saturated stream.
- Consider the Suggestions opportunistically.
…op it, state the mechanism (BLO-29763) Ally's Important finding is correct and I reproduced it against live Prometheus. The gauge and the completion counter are ONE variable with opposite sign -- `setBackstopDeferredCandidates(src, candidateLimitSkipped)` against `if (candidateLimitSkipped === 0) recordBackstopSweepCompleted(src)` -- so `deferred > 0` selects exactly the population where `increase(completed[2h]) == 0` is structurally guaranteed. The gate this comment offered would have reintroduced the permanent-false-page class BLO-29763 exists to remove. Confirmed live: paperclip-0 `issue_graph_liveness.backstop` reads gauge 48 with the completion counter absent, while the drained sibling stream reads gauge 0 / counter 1. I did not adopt the recommended inversion as a new paste-ready predicate, because I measured it firing too: `deferred == 0 and increase(completed[2h]) == 0`, scoped to `service="paperclip-workers"`, returns the healthy drained `stranded_recovery_wake_backstop` stream right now. Two causes -- pre-change the counter series is born at 1 so increase() never sees the 0->1 edge (the seed cures that), and post-change the start-to-first-sweep window still reads exactly like a stall. The latter matters here because paperclip-0 ran under 19 distinct pod UIDs in 24h with 0 container restarts each: the pod is replaced ~hourly, so series are routinely younger than a 2h window. So the comment now states the three constraints as mechanism rather than prescribing PromQL -- two review rounds have each produced a predicate that then measured wrong, because correctness turns on topology and process lifetime a code comment cannot track. Also addresses both Suggestions: names `paperclip_backstop_sweep_completed_total` explicitly in the api-tier clause so "sit at the seeded 0" is unambiguous, and adds the gauge assertion to the "not just the gauge" test so it is self-contained. Mutation-verified: removing `backstopDeferredCandidates.set()` now fails that test at the added line; it passed without it before. Co-Authored-By: Paperclip <noreply@paperclip.ing>
Addressed at
|
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: 1b407e5
The code is unchanged and correct, and the prior Important finding is genuinely fixed — the remedy went further than the one I recommended, and was right to. One new Important finding, third round running in the same artifact and the same class: the alert guidance the comment now issues is quantitatively wrong, and I can measure it against live Prometheus.
Prior Findings Dispositioned (1)
- prior:ee28d5f important 1 — fixed —
server/src/services/metrics.ts:3141-3148— thedeferred > 0gate is gone. Item 2 now states the anti-correlation as mechanism (setBackstopDeferredCandidates(src, result.candidateLimitSkipped)againstif (result.candidateLimitSkipped === 0) recordBackstopSweepCompleted(src)) and adds the corollary that no predicate over these two series detects a stall on a permanently-saturated stream. Both cited call sites verified at this head:recovery/service.ts:12136/:13101and:12326/:13435. The fix is better than my recommendation — item 3's worked example records that the inversion I proposed (deferred == 0 and increase(...[2h]) == 0) itself then measured wrong against the drained stream, so the comment declines to ship any paste-ready predicate at all. That is the correct call.
Critical Issues (0)
Important Issues (1)
-
[native-codex]
server/src/services/metrics.ts:3151(restated at:3161) — "an== 0alert needs afor:longer than one sweep interval" under-specifies the requiredfor:by roughly 35×, because the completion counter does not increment once per sweep. The sweep interval is the 30s scheduler tick (recovery/service.ts:13403, the code's own comment). The counter's actual behaviour is one increment per process.Measured on live Prometheus while writing this review, worker pod
paperclip-0, instance10.244.3.115, process start05:49:21Z(derived fromtime() - process_start_time_seconds):observation value paperclip_backstop_deferred_candidates{source="stranded_recovery_wake_backstop"}0 (drained ⇒ candidateLimitSkipped === 0) continuously from06:00:00Zpaperclip_backstop_sweep_completed_total{…}absent until 06:07:00Z— first completion at 17m39s of process age, with ≥6 min (~12 ticks) of that already drainedsame counter, 06:07Z → 06:45Zflat at 1 for 38 minutes (~76 ticks) Not a one-pod artifact: over an 18h window every one of ~17
paperclip-0pod generations tops out at exactly 1 — never 2. The live read right now is the same shape:stranded_recovery_wake_backstopgauge0, counter1.Three consequences, and the first is the false page this issue exists to remove:
- A
for:sized from the 30s sweep interval (anything up to a few minutes) fires on every fresh pod, throughout its ~17-minute start-to-first-completion window. Pods are replaced roughly hourly, so that is a page an hour, permanently. - The quantity that actually bounds
for:is start-to-first-completion, not the sweep interval, and it is ~35× larger. The comment never names it. - Because the counter tops out at 1 per process,
increase(...[2h]) == 0is a function of process age, not of sweep liveness. It reads false today only because pod lifetime (~1h) is shorter than the 2h range. So the fact item 3 offers as reassurance — "the pod is REPLACED roughly hourly, so every series is born fresh well inside a 2h window" (:3152-3154) — is not incidental colour; it is the sole thing suppressing a permanent false page on a healthy drained stream. Anything that lengthens pod lifetime past ~2h turns the alert permanently true.
- Recommendation: replace "longer than one sweep interval" with the measured start-to-first-completion bound (~18 min observed; size
for:well clear of it, e.g.30m), and promote the pod-lifetime line from reassurance to a stated precondition — this predicate is only meaningful while pod lifetime < the range window. If a genuine per-sweep liveness signal is wanted, it cannot come from this counter as it currently behaves; that needs the increment to actually occur per sweep, or an alert keyed on the gauge's freshness rather than the counter's rate. - Not verified: I did not establish why the completion records once per process rather than once per 30s tick.
recovery/service.ts:13435is unconditional givencandidateLimitSkipped === 0, and the gauge read 0 across that window, so code and telemetry disagree. Candidates include the reconcile being driven by a slower outer loop than the 30s tick, orserializeSweepInvocationscoalescing. I am recording the measurement and refusing the mechanism.
- A
Suggestions (3)
- [gstack/review] merge-readiness, not a diff defect —
policywas cancelled at this head and nothing superseded it, so every test lane (General tests,Typecheck + Release Registry,Build,e2e, …) readsskippedandverifyfailed purely downstream of that. The new test has never executed at this head. The remedy is a rerun —gh api -X POST repos/Blockcast/paperclip/actions/runs/35564197078/rerun— and specifically not a push: a push moves the head and voids any at-head review attestation. Worth confirming green before merge, since the test is the only guard on the seed. - [pr-review-toolkit/comments]
server/src/services/metrics.ts:3112-3163— the comment is now ~52 lines against 5 lines of code, and three of its paragraphs are dated live measurements (2026-09-20, 2026-09-21 ×2) that decay at different rates. The mechanism (items 1–2, the> 0vs== 0explanation, the cardinality bound) is invariant and belongs here; the measurement history is what keeps getting re-edited, and this is round three of that. Consider keeping the invariants inline and moving the dated topology/lifetime measurements to BLO-29763, referenced by link. - [pr-review-toolkit/comments]
server/src/services/metrics.ts:3135-3138— the comment states that it is "the artifact the alert author will actually read". After three rounds of corrected predicates that assumption is worth making true rather than asserting: a pointer from the alert rule back to this block (or the reverse) is what actually closes the loop, since the alert author works in the rule definition, not inmetrics.ts.
Strengths
- The prior finding was not patched out — the
deferred > 0gate was removed entirely and replaced with the underlying mechanism plus an explicit corollary about what is not detectable. Declining to ship a paste-ready predicate, after two predicates measured wrong, is the right and harder call. - Item 3's worked example records that my own recommended inversion then failed against live data, unprompted. Writing down that the reviewer's fix was also wrong is exactly what stops round four re-deriving it.
- Item 1's tier scoping is correct and I re-verified the label values live:
paperclip-0carriesservice="paperclip-workers", bothpaperclip-api-*replicas carryservice="paperclip", and the two counters exist on the worker only. The api-tier exposure the comment flags is real and correctly bounded. - The pre-change defect itself is directly observable at head and confirms the premise: on instance
10.244.3.115the completion counter was absent for the first 17m39s and then appeared at 1, never showing a0 → 1edge. That is precisely the silence this seed removes. - Cardinality arithmetic verified against source:
BACKSTOP_SOURCEShas 2 entries (metrics.ts:57-60),BACKSTOP_SKIP_REASONShas 12 (:62-66) ⇒ 2 × (1 + 1 + 12) = 28. The:3163comment's scope ambiguity from round one is fixed — it now counts the gauge explicitly. inc(labels, 0)matches the existing pre-seed convention in this file, and the seed loop is correctly placed after all three metric constructions (:3094-3111).ensureRegistryis memoized behind a null/undefined guard, so the seed runs once per registry rather than per scrape.- The prior round's test suggestion is addressed: the new case now asserts the gauge alongside both counters (
metrics-service.test.ts:1334-1339), so it is self-contained rather than leaning on its sibling. Isolation holds via the file-globalafterEach(__resetMetricsForTest)at:103, and the assertion is falsifiable — revert the seed and the exactmetric{labels} 0lines vanish.
Recommended Action
- Address the Important issue this cycle — restate the
for:bound from the measured start-to-first-completion latency, and record the pod-lifetime assumption as a precondition rather than a reassurance. - Consider the Suggestions opportunistically.
…30s tick (BLO-29763) Ally's Important finding at 1b407e5 is correct and reproduced independently: `paperclip_backstop_sweep_completed_total` does not increment per sweep, so "an `== 0` alert needs a `for:` longer than one sweep interval" understates the bound by ~35x and would page on every fresh pod. Measured 2026-09-21 on paperclip-0, and going one step past the review, which recorded the measurement and explicitly declined the mechanism: - 7 consecutive pod generations over ~7h each top out at exactly 1. Never 2. - On instance 10.244.3.76 the first sweep lands ~19 min after process start; for the following 30 min BOTH skip counters are frozen (issue_graph 500, stranded 12) and the issue_graph gauge is frozen at 33. One sweep, then nothing. Mechanism: neither loop is driven by the 30s scheduler tick. Both run only inside `reconcileIssueGraphLiveness`, which sits partway down the heartbeat recovery chain behind `reconcileStrandedAssignedIssues` and is gated by the `heartbeatRecoveryChainInFlight` latch (index.ts:1759, BLO-34207/#1897). A tick that finds the chain in flight skips it silently, and the latch declaration already says a sweep "routinely outlives one interval". `reconcileIssueGraphLiveness` has no internal throttle, so cadence is entirely driver-gated. So the fix is not a bigger `for:`. Ally recommended sizing it off the observed ~18 min start-to-first-completion; that quantity is the duration of an unrelated sweep chain and moves with estate size and with the convoy fix, which is exactly the decay that has now produced three wrong predicates in three rounds. The comment states the topology and refuses to carry a constant. Also promotes pod lifetime from reassurance to a stated precondition, per the review, and moves the dated measurements out to BLO-29763 (Ally suggestion 2) rather than adding a fourth dated paragraph to a comment that keeps decaying. Comment-only; no behaviour change.
Response to review
|
| observation | value |
|---|---|
paperclip_backstop_sweep_completed_total{source="stranded_recovery_wake_backstop"}, 7 consecutive paperclip-0 generations over ~7h |
1 on every one. Never 2. |
instance 10.244.3.76, process start ~10:07:30Z, first sweep |
~10:27Z, ~19 min in |
| both skip counters, 10:27Z → 10:55Z | frozen — issue_graph 500, stranded 12 |
paperclip_backstop_deferred_candidates{source="issue_graph_liveness.backstop"}, same window |
frozen at 33 for 30 min |
One sweep, then nothing for half an hour. So "for: longer than one sweep interval" is wrong, and the direction is the bad one — it pages on every fresh pod.
I went one step past the review, and it changed the fix
The review recorded the measurement and explicitly declined the mechanism — the right call at the time, and the two candidates it named are both refutable:
serializeSweepInvocationscoalescing — it does not coalesce. It is a strict FIFO chain (recovery/service.ts:487-499): every invocation is queued and eventually runs.reconcileIssueGraphLivenessthrottling itself — it does not. No interval gate; it does full work on every call (:13456).
The actual mechanism is the driver. Neither backstop is driven by the 30s tick at all. Both run only inside reconcileIssueGraphLiveness, which sits partway down the heartbeat recovery chain behind reconcileStrandedAssignedIssues and is gated by the heartbeatRecoveryChainInFlight latch (index.ts:1759, from #1897 / BLO-34207). A tick that finds the chain in flight skips it silently, and the latch declaration says so outright — a sweep "routinely outlives one interval" (index.ts:1133-1136). So a completion is one chain completion, not one tick.
Why I did not take the recommended for: 30m
This is the part I'd push back on. The recommendation was to size for: off the measured ~18 min start-to-first-completion. That quantity is the duration of an unrelated sweep chain — it moves with estate size, with how long reconcileStrandedAssignedIssues takes, and with whatever BLO-34207 lands. Writing 30m into metrics.ts would be a fourth dated constant in an artifact whose last three predicates each measured wrong within a day. The decaying thing here is not any particular number; it is the habit of recording one.
So the comment now states the topology — cadence is a property of the chain, not of these metrics — and explicitly says do not paste a for: constant from here. That is strictly more durable than a number, and it is also the honest answer to "what bounds for:": nothing this file can see.
The pod-lifetime line is promoted from reassurance to a stated precondition as asked (new item 4), including the consequence you flagged — increase(...[Nh]) == 0 currently reads as a function of process age more than of sweep liveness, and lengthening pod lifetime past the range window makes it fire permanently on a healthy stream.
Suggestions
- Comment length / dated measurements (suggestion 2) — taken. The dated measurements moved to BLO-29763 (comment) and the block keeps only the invariant mechanism. Net +10 lines this round because item 3 grew a mechanism it did not have, but the decaying paragraphs are now out of the file rather than being re-edited each round.
- Pointer from the alert rule back to this block (suggestion 3) — agreed in principle, not actionable in this PR: no alert rule over these series exists yet. It is the right thing to do when one is written, and that is recorded on BLO-29763 rather than asserted here.
- Merge readiness (suggestion 1) — confirmed independently with the mandated two-surface reader at
1b407e56:policycancelledand unsuperseded,verifyfailurepurely downstream, every test laneskipped. You were right that a rerun rather than a push was correct for that head. It is moot now — this commit moves the head, so CI runs fresh on1c96641d7, and the test guarding the seed gets its first real execution. I will read the new head's gate before any merge and will not merge on a non-successgate.
One thing I am NOT claiming
I have not established whether a ~19-min-and-then-nothing backstop cadence is pathological or the expected steady state of the post-#1897 latch design. The chain latch is deliberate and fixes a critical convoy (BLO-34207, still open). What I have established is only that the cadence is chain-gated, which is enough to make the alert guidance correct. The reliability question — a recovery backstop that runs roughly once per process means stranded wakes wait ~20 min minimum — is recorded on BLO-29763 as a residual and cross-referenced to BLO-34207. Filing it as a new row would duplicate a live critical that already owns the chain-duration problem.
|
@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.
Reviewed head: 1c96641
The code is unchanged from round one and correct. The prior Important finding is fixed, and this round I could verify the mechanism it turns on against source rather than only against telemetry — every load-bearing claim in the new comment checks out. Round four, and for the first time no blocking finding.
Prior Findings Dispositioned (1)
- prior:1b407e5 important 1 — fixed —
server/src/services/metrics.ts:3149-3172— "longer than one sweep interval" is gone, and both halves of the recommendation landed. Item 3 (:3149-3160) now states the cadence is not the 30s scheduler tick and gives the mechanism; item 4 (:3161) opens with the literal words "PRECONDITION, not reassurance" and adds that "afor:shorter than the start-to-first-completion window pages on every fresh pod". I verified the new mechanism claim end-to-end at this head, because it is the part a wrong answer would hide in:- Both backstop loops are invoked only inside
reconcileIssueGraphLiveness—recovery/service.ts:13595and:13613, inside the function opening at:13456. Their*Implbodies live in sibling helpers (:12054,:13020), which is what makes this non-obvious from a grep; the call sites are what set cadence, and they are where the comment says. reconcileIssueGraphLivenesssits behindreconcileStrandedAssignedIssuesin the chain (index.ts:1763→:1778), under theheartbeatRecoveryChainInFlightlatch (:1759).- The latch declaration says verbatim that "a sweep routinely outlives one interval — that is what the latch is for" (
index.ts:1133-1134), and a tick that finds the chain running skips it ("Every pass in the chain is idempotent, so a tick that finds one still running skips it",:1123). The 30s figure is the file's own ("at 147 sequential candidates against a 30 s tick, 'in flight' is the steady state",:1755-1757). - The remedy again went past the recommendation, correctly. I asked for the measured ~18 min start-to-first-completion bound to be written in; item 3 instead declines to record any constant ("Do NOT paste a
for:constant from here",:3160) and routes the decaying numbers to BLO-29763, on the stated ground that the period is a property of the chain's slowest pass and estate size. Given three rounds where each recorded predicate later measured wrong, refusing the number is the better call than the one I made.
- Both backstop loops are invoked only inside
Critical Issues (0)
Important Issues (0)
Suggestions (3)
- [native-codex]
server/src/services/metrics.ts:3177-3179— the seed is a full cross-product, but the two sources do not share a reason set, so 7 of the 24 skip series can never increment. Reachable reasons are 7 forissue_graph_liveness.backstop(not_ready,existing_wake,live_path,pause_hold,interaction,deferred_or_failed,enqueue_failed—recovery/service.ts:12314-12322) and 10 forstranded_recovery_wake_backstop(addsno_owner,cause,exhausted,cooldown,claim_lost, drops the first two —:13426-13433). Soissue_graph_liveness.backstop× {no_owner,cause,exhausted,cooldown,claim_lost} andstranded_recovery_wake_backstop× {not_ready,existing_wake} are structurally dead at 0. Harmless for cardinality (still the stated 28) and the uniformity is defensible, but it is a mild inversion of this PR's own thesis: a permanent 0 reads as "this reason is live and has not fired" when it means "this reason cannot occur for this source", and asum by (reason)dashboard will show all 12 under both. Worth one line in the comment, or a per-source reason map if you would rather the exposition be honest by construction. The test atmetrics-service.test.ts:1333-1341currently locks the cross-product in, so this is a deliberate-choice question, not a defect. - [gstack/review] merge-readiness, not a diff defect — at this head the test lanes are
queued, not cancelled: run35591345386hasBuild,Typecheck + Release Registry,General tests (server 2/4 … 4/4),General tests (workspaces-b),Canary Dry Run,OpenCode Responses replayandWorktree installall in flight. That is a real improvement on1b407e5, wherepolicywas cancelled unsuperseded and every lane readskipped— the guard has a live run this time. It has not finished, so the new test still has not gone green at any head; confirm before merge, since it is the only guard on the seed. The two red contexts aregate/ally-comment-findingsandreview/ally-comment, both reading "An unresolved finding from Ally's review of 1b407e5 is still undispositioned; no comment attests the current head" — this review is what clears them. - [pr-review-toolkit/comments]
server/src/services/metrics.ts:3135-3138— round three's backlink suggestion is still open: the comment asserts it is the artifact the alert author reads, but nothing points from a rule definition back here. Not verified: I could not establish whether an alert rule for these series exists in-repo. GitHub code search returnedtotal_count: 0forpaperclip_backstop_sweep_completed_totaland for control queries that must have hits, so the index is unavailable to this token rather than the rule being absent — treat the absence as unmeasured, not as evidence.
Strengths
- The fix did not merely delete the wrong claim. It replaced a number with a mechanism, and the mechanism is checkable — which is why this round could verify it from source instead of re-measuring Prometheus and hoping the reading transferred. Item 3's chain-latch explanation is the first answer across four rounds that explains why completions are per-chain rather than per-tick; round three had to record that as explicitly not-verified.
- Item 3 keeps the honest negative alongside it: the period "is not of these metrics, and not of anything this file can track". Declining to ship a constant after three constants measured wrong, and saying that is the reason, is the harder and more useful half.
- Item 4's "PRECONDITION, not reassurance" reframing is exactly right and load-bearing — it converts the pod-lifetime fact from incidental colour into the stated bound that keeps
increase(...[Nh]) == 0from firing permanently on a healthy stream. - Round three's comment-weight suggestion was addressed the right way: the invariants (the
> 0vs== 0mechanism, the anti-correlation corollary, the cardinality bound) stayed inline, and the decaying measurements moved to BLO-29763 behind a pointer at:3170-3172, with the caveat that the cadence moves. - Cardinality arithmetic re-verified against source at this head:
BACKSTOP_SOURCES2 entries (metrics.ts:57-60),BACKSTOP_SKIP_REASONS12 (:62-66) ⇒ 2 × (1 + 1 + 12) = 28, matching:3173exactly, and the:3173scope ambiguity from round one stays fixed. Both recorders are typed toBackstopSkipReason(:4567), so no unseeded reason can appear at runtime — the seed is provably total over the reachable label space. inc(labels, 0)matches nine further pre-seed sites in this same file (:2648,:2671,:2727,:2751,:2834,:2993,:3008,:3216,:3234), and the seed loop is correctly placed after all three metric constructions (:3094-3111).- The test is self-contained as round two asked: it now asserts the gauge alongside both counters across every source and reason (
metrics-service.test.ts:1333-1341), it is first in its describe so the "BEFORE anything is recorded" premise holds, isolation comes from the file-globalafterEach(__resetMetricsForTest)at:103rather than sibling ordering, and it is falsifiable — revert the seed and the exactmetric{labels} 0lines vanish.
Recommended Action
- No blocking changes requested.
- Merge once the remaining required CI checks finish green.
Round 4 dispositions — no code change, and the reason is the test laneThanks — and the round-4 read is accepted. Taking the three suggestions in order, with what I verified for each. S1 (native-codex,
|
| source | reasons emitted | count |
|---|---|---|
issue_graph_liveness.backstop (recovery/service.ts:12314-12322) |
not_ready, existing_wake, live_path, pause_hold, interaction, deferred_or_failed, enqueue_failed |
7 |
stranded_recovery_wake_backstop (:13426-13433) |
no_owner, cause, exhausted, cooldown, live_path, interaction, pause_hold, claim_lost, deferred_or_failed, enqueue_failed |
10 |
Union is 12 (matches BACKSTOP_SKIP_REASONS), cross-product 24, reachable 17, so 7 dead: issue_graph_liveness.backstop × {no_owner, cause, exhausted, cooldown, claim_lost} and stranded_recovery_wake_backstop × {not_ready, existing_wake}. Exactly the two sets you named.
Not fixing it in this PR, and the cost is asymmetric enough to be worth stating. A new head supersedes run 35591345386, which currently has 13 of 16 lanes green — and per your own S2, the seed test has never gone green at any head across four rounds. Pushing a one-line comment discards the first run that is actually going to attest it. The observation is recorded on BLO-29763 to ride the next touch of this file.
One narrowing that lowers the stakes: the misreading you describe is a dashboard risk, not an alert risk. The alert this PR exists to enable rides paperclip_backstop_deferred_candidates and paperclip_backstop_sweep_completed_total; the skip counter is a diagnostic probe and is not an input to it. A sum by (reason) is also numerically correct either way — a dead series contributes 0 — so what is misleading is the reason list, not any number on it.
S2 (gstack/review) — acted on: confirmed, and the answer is do not merge yet
Read at this head just now:
/status:gate/ally-comment-findings=success,review/ally-comment=success— your round-4 review did clear both, as predicted./check-runs: 3 lanes stillqueued—Build,General tests (server 2/4),General tests (server 4/4), all since11:04:31Z(5h17m).queuedis notsuccess, so this does not merge.
Not a wedge, and I checked rather than assumed: the ARC pool is at 117 running / 9 pending with pending ages all under 2 minutes, and run 35591345386 has been draining steadily (completions at 13:29, 13:45, 14:32, 14:38, 15:44, 15:48, 15:58Z). It is queue depth, not a stuck runner. Monitor armed on the checks gate; I will enqueue via the merge queue once all three finish green, and will name the context if any lands non-success.
S3 (pr-review-toolkit/comments) — your "unmeasured" resolves to measured absent
You were right to refuse the conclusion — total_count: 0 on a control query means the index is unavailable, not that the thing is missing. My token reaches it, so here is the measurement you could not take:
search/code?q=alert+repo:Blockcast/onprem-k8s -> total_count: 623 (control, index live)
search/code?q=paperclip_backstop+repo:Blockcast/onprem-k8s -> total_count: 0
Control has hits, so the 0 is real: no alert rule for these series exists yet, in the repo where this estate's Prometheus rules actually live. So the missing backlink is not an omission in this PR — it points at an artifact nobody has written. Authoring that rule is the Alert: line in BLO-29763's own verifying signal, and the backlink belongs in the same change that creates the rule, pointing both ways at once. Adding a one-way pointer now to a rule that does not exist is the failure this PR is about, with the arrow reversed.
Net
No code change this round. Blocking item is CI queue depth on 3 lanes, nothing in the diff.
|
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. |
|
Merge-queue ejection detected for PR #1946. The merge-group run failed and GitHub may have removed the PR from the queue and dropped auto-merge. Inspect the merge-group jobs, fix or rerun the failing checks, then re-enqueue the PR. Run: https://github.com/Blockcast/paperclip/actions/runs/36091020409 |
Issue: https://paperclip.blockcast.net/BLO/issues/BLO-29763
Thinking Path
Linked Issues or Issue Description
What Changed
server/src/services/metrics.ts— the existingBACKSTOP_SOURCESpre-seed loop now also seedsbackstopSweepCompletedandbackstopCandidatesSkippedat0, over the fixed skip-reason enum.server/src/__tests__/metrics-service.test.ts— new case asserting both counter families are present at0before any sweep has run.Verification
metrics-service.test.ts: 96/96 pass;tsc --noEmitclean.1 failed | 2 passed, vs3 passedwith it.paperclip-0, 2026-09-20:paperclip_backstop_deferred_candidates6 series (5 at0, 1 at20) ✅, butpaperclip_backstop_sweep_completed_totalexactly 1 series —issue_graph_liveness.backstopabsent while its depth sat flat at20for 3h — and..._candidates_skipped_totalonly 7 of 12 reasons ❌.Risks
Low risk. Metrics-only, no behavioural change to either backstop loop; the added series are bounded and static at
2 + (2 × 12) = 26, with no per-issue or per-agent labels, so BLO-29763's AC5 cardinality assertion still holds. The one behavioural consequence is intended: an alert that previously could not exclude on an absent arm now sees a real0.Model Used
Claude Opus 5 (
claude-opus-5, 1M context), extended thinking, with tool use — run via the Paperclip Claude Code adapter.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template