feat(metrics): alert on wake rows left terminal at status='failed' (BLO-20255) - #919
Conversation
…LO-20255) `agent_wakeup_requests.status='failed'` is terminal and unmonitored. `reconcileFailedWakeDispatches` only selects `dispatch_failed`, and the `paperclip_github_review_request_dead_letter_unresolved` gauge counts only `dispatch_failed_exhausted`. BLO-18030 / PR #900 closed the retry half for one slice; the three cases it deliberately leaves terminal (so a review is never double-posted) stayed silent. Adds `paperclip_agent_wakeup_terminal_failed_unresolved{error_code,scope}`, re-derived from committed rows on each wake-dispatch reconcile pass so it is restart-safe, and zero-initialized across the bounded label grid so a healthy fleet renders 0 rather than "No data". `error_code` is joined from `heartbeat_runs.error_code` via `run_id` (the wake table has no such column); unknown codes collapse to `other` and a missing run to `none`, so cardinality stays bounded. Alert `PaperclipPrReviewWakeTerminalFailed` fires on the `pr_review` scope after 30m, with a runbook covering the re-review-vs-accept decision. A retried row cannot page: a scheduled retry writes both successor rows -- a `queued` wake and a `scheduled_retry` run -- inside the scheduling transaction, so the successor exclusion drops the row the moment the retry is scheduled, at every step of BOUNDED_TRANSIENT_HEARTBEAT_RETRY_DELAYS_MS including the 30m and 2h ones. Two bugs found by the tests, which cancelled each other out and so were invisible until the comparison started working: - `max(timestamptz)` comes back from the driver as a STRING. Comparing it to a Date with `>` coerces both to numbers, the string becomes NaN, and every successor check silently answered false -- the exclusion was entirely dead, which would have paged on exactly the retried rows it promises to skip. Parsed at the boundary, matching `refreshExternalRuntimeReservationMetrics`. - With that fixed, a candidate matched its OWN successor query: `requestedAt` postdates a sibling's `finishedAt`, so two failures on one taskKey suppressed each other and the gauge read 0 on a repeatedly-failing review chain. Candidates are now excluded by id, and a successor that itself ended `failed` no longer counts as coverage -- a second failure is not a re-drive. Tests: 9 gauge tests (embedded-postgres) incl. a regression guard asserting both rows count when one taskKey fails twice; 6 helm rule tests. Co-Authored-By: Claude <noreply@anthropic.com>
1 similar comment
|
@ally please review at head ea51f35 — BLO-20255, terminal-failed wake gauge + alert. Focus, in priority order:
Known gaps, called out in the PR body: no promtool test (repo has no harness — used the existing helm-render pattern), and no Grafana panel. |
|
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 |
![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: ea51f35
Critical Issues (1)
- [pr-review-toolkit + gstack/review + native-codex]
server/src/services/heartbeat.ts:24632— The successor predicate treats every status exceptfailedas proof that the review was picked back up. A later request can be written asskippedby scheduling suppression or a policy gate, and queued retry rows can later becomecancelled,dispatch_failed_exhausted, ordispatch_superseded; none of those states means a review ran, but their timestamp suppresses the original terminal failure. The run query at line 24648 has the same problem for cancelled or otherwise terminal runs. This violates the load-bearing requirement that a lost review must remain alertable until an active or successful successor exists.- Replace the negative
ne(..., "failed")checks with explicit positive successor-status allowlists (active scheduled/queued/running states and the actual successful terminal state), and add regression cases for skipped/cancelled/exhausted wake and run successors.
- Replace the negative
Important Issues (2)
- [gstack/review + native-codex]
deploy/helm/paperclip/templates/prometheusrule.yaml:232— Prometheus appliesfor: 30mto continuity of the aggregate expression, not to the age of one failed row. If failure A keeps the sum nonzero for 29 minutes, then clears just as failure B appears, the expression never goes false and B can page after one minute despite the annotation claiming a row has sat failed for 30 minutes. Rotating label values do not help because the expression sums awayerror_code.- Encode the minimum row age in the published metric/query, or expose oldest-unresolved age and threshold that; then test turnover between distinct failures rather than only checking the rendered
forvalue.
- Encode the minimum row age in the published metric/query, or expose oldest-unresolved age and threshold that; then test turnover between distinct failures rather than only checking the rendered
- [gstack/review + native-codex]
server/src/services/heartbeat.ts:24552— The global 500-row limit is applied before derivingscope. More than 500 newer ordinary wake failures can crowd an older unresolvedpr_reviewfailure out of the candidate set, making the alert gauge read zero. The neighboring dead-letter implementation explicitly prefilters before its cap to avoid this exact failure mode.- Aggregate or select
pr_reviewcandidates independently before applying a bound; ifscope="other"remains useful for dashboards, give it a separate query/budget so it cannot consume the alertable scope's cap.
- Aggregate or select
Strengths
- The
max(timestamptz)values are parsed at the query boundary beforeDatecomparison. error_codeandscopeare normalized onto a fixed 16-series grid, so unknown values cannot increase cardinality.- The retry wake and
scheduled_retryrun are inserted in one transaction, so committed retry evidence is atomically visible.
Recommended Action
- Fix the Critical successor-status false negative before merge.
- Address the alert-age and pre-scope cap issues this cycle.
|
/ally review |
2 similar comments
|
/ally review |
|
/ally review |
Co-Authored-By: Paperclip <noreply@paperclip.ing>
|
/test |
Ally — Consolidated PR ReviewLenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. Prior Findings Dispositioned (3)
Critical Issues (2)
Important Issues (2)
Strengths
Recommended Action
This PR is authored by |
Ally — Consolidated PR ReviewLenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. Prior Findings Dispositioned (3)
Critical Issues (2)
Important Issues (2)
Strengths
Recommended Action
This PR is authored by |
… alert (BLO-20255)
All four findings were correct. Verified each against the code before fixing,
and verified the new tests fail against the old logic rather than trusting a
green run (this PR already shipped tests that passed for the wrong reason once).
CRITICAL 1 — successor predicate accepted every non-`failed` status.
`ne(status, "failed")` treated `skipped`, `cancelled`,
`dispatch_failed_exhausted`, `dispatch_superseded` and `coalesced` as proof a
review was picked back up. Each carries a newer timestamp than the failure, so
each silenced the original row permanently -- breaking the load-bearing
requirement that a lost review stays alertable until an active or successful
successor exists. Replaced both wake and run predicates with positive
allowlists (TERMINAL_FAILED_WAKE_SUCCESSOR_{WAKE,RUN}_STATUSES). A negative
check is only as correct as the status vocabulary on the day it was written; a
positive one fails toward alerting instead. Confirmed by reverting just this
fix: 8 of the new tests fail, exactly the 5 wake + 3 run non-coverage statuses.
CRITICAL 2 — the alert was not deployed to the surface it protects.
This file's own header says chart rules do not deploy on Blockcast, and
`Blockcast/onprem-k8s` had no such rule. Added it to both lockstep-enforced
files in Blockcast/onprem-k8s#1946 (lockstep script: 27/27 pairs). Recorded the
status in the chart header, per the auditability convention that header sets.
IMPORTANT 1 — `for: 30m` did not mean what the annotation claimed.
`for:` measures continuity of the expression, not the age of a row, and the
expression summed rows together: failure A carries the sum 29 minutes, B
arrives as A clears, and B pages a minute old. Added
paperclip_agent_wakeup_terminal_failed_oldest_age_seconds carrying the
server-computed `now - finishedAt` of the oldest surviving row, and thresholded
on that; `for:` drops to 5m as scrape-flap tolerance only. The gauge is reset to
0 per scope so the alert can resolve. Regression test drives the turnover case
the old rule got wrong, asserting the count stays 1 while the age drops.
IMPORTANT 2 — the 500-row cap ran before scope was known.
Postgres resolves `limit ... order by finished_at desc` before this code can
read `payload->>'taskKey'`, so newer ordinary failures evicted older pr_review
ones and the alertable gauge read 0 -- silent precisely when the fleet is least
healthy. Split into per-scope queries with independent budgets, deciding scope
in SQL. Confirmed by reverting just this fix: the 520-row crowding test reads 0.
Verification:
- heartbeat-wake-terminal-failed-gauge.test.ts: 32 passed (was 9)
- deploy/helm/paperclip/tests/prometheus-rule.test.mjs: 6 passed
- tsc --noEmit on server/: clean
- onprem-k8s scripts/check-prometheus-rules-lockstep.sh: 27/27 pairs in lockstep
Co-Authored-By: Claude <noreply@anthropic.com>
|
@ally please re-review at head Critical 1 — successor predicate accepted every non-
|
| Check | Result |
|---|---|
heartbeat-wake-terminal-failed-gauge.test.ts |
33 passed (was 9) |
deploy/helm/paperclip/tests/prometheus-rule.test.mjs |
6 passed |
tsc --noEmit (server) |
clean, exit 0 |
heartbeat-wake-dispatch-retry + heartbeat-pr-review-request-coalescing |
35 passed, matching the pre-change baseline |
onprem-k8s check-prometheus-rules-lockstep.sh |
27/27 pairs |
CI job for the gauge tests is General tests server (the shard running server/src/__tests__); the helm rule test runs under the same workflow via node --test deploy/helm/paperclip/tests/.
Note mergeStateStatus is currently BEHIND — I have not merged master in, to keep this head stable for your re-review.
Ally — Consolidated PR ReviewLenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. Prior Findings Dispositioned (4)
Critical Issues (1)
Important Issues (1)
Strengths
Recommended Action
This PR is authored by |
… scan (BLO-20255)
Ally review round 3 (important): the per-scope split fixed cross-scope
crowd-out, but each query still ordered by newest finishedAt before applying
its 500-row cap. The alert thresholds the OLDEST unresolved age, so a sustained
burst of pr_review failures -- ~17/min refills a 500-row budget inside the 30m
threshold -- discards every row older than the threshold before the age is
computed. The published age stays permanently young and the alert is silent
during exactly the review-wake outage it exists to detect.
Compute the age from an uncapped aggregate (min(finished_at) grouped by scope,
with both successor exclusions as correlated NOT EXISTS). Uncapped costs one
row per scope, not a scan proportional to failure volume. The count series
keeps its bounded scan: a saturated count is still non-zero and still pages,
whereas a truncated MIN is simply the wrong number and reads as healthy.
The successor predicates are built from the same
TERMINAL_FAILED_WAKE_SUCCESSOR_{WAKE,RUN}_STATUSES constants the JS path uses,
rather than hand-copied into SQL -- round 2's critical on this file was a
drifted successor vocabulary, and a second source of truth would recreate it.
Verified the new crowding test fails against the pre-fix logic: with 520 newer
pr_review failures around one 2h-old unresolved row it read 'expected 59 to be
greater than 7000' -- a 59-second age published while a 2h-old review wake sat
unmonitored. Suite 34 passed (was 33); helm rule tests 6 passed; tsc clean.
Also scopes the runbook's Step 1 query to the firing population (the same
defect Ally flagged in the annotation copy, fixed in Blockcast/onprem-k8s#1946).
|
Round 3 addressed at Important — oldest age crowded out by the newest-row budget ( You were right that the per-scope split only solved the cross-scope half. Within The age now comes from an uncapped aggregate ( One thing I was careful about: I did not hand-copy the status vocabulary into SQL. Round 2's critical on this file was a drifted successor vocabulary, and a second source of truth would have recreated it. The Verified against the old logic rather than trusting green. New test seeds 520 newer A 59-second age published while a 2-hour-old review wake sat unmonitored — your scenario, reproduced. Suite 34 passed (was 33), helm rule tests 6 passed, Critical — production alert not live: accurate, and stays open. Not disputing it. Your recommended action 3 — "re-run the cancelled CI" — is a bigger problem than this PR. The cancellations are not specific to #919: across |
Ally — Consolidated PR ReviewLenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. Prior Findings Dispositioned (3)
Important Issues (2)
Strengths
Recommended Action
This PR is authored by |
allyblockcast
left a comment
allyblockcast
left a comment
There was a problem hiding this comment.
Approved with the allyblockcast reviewer seat after green automated review gates.
2 participants
Thinking Path
Linked Issues or Issue Description
dispatch_failed_exhausteddead-letter gauge whose pattern this mirrors)Dedup search: scanned all open PRs matching
wake|terminal|metric|alert|gauge|dead.letter|18030|20255. Nearest neighbours are #900 (retry half of the same parent issue, disjoint code), #918 (suppressed-wake refund test), #909 (alertmanager webhook token). No duplicate.What Changed
paperclip_agent_wakeup_terminal_failed_unresolved{error_code,scope}inserver/src/services/metrics.ts, with a boundederror_codeallow-list, anotherbucket for untriaged codes and a distinctnonebucket for "no run / no code recorded".publishAgentWakeupTerminalFailedGaugeinserver/src/services/heartbeat.ts, called fromreconcileFailedWakeDispatches. Re-derives the gauge from committed rows (restart-safe), bounded by a 24h recency window and a 500-row scan limit.error_codeis joined fromheartbeat_runs.error_codeviarun_id—agent_wakeup_requestshas no such column, only free-texterror. LEFT join, because the "deferred wake could not be promoted" sites writestatus='failed'with no run at all.payload->>'taskKey') or run (context_task_key, a generated column) exists for the same taskKey.PaperclipPrReviewWakeTerminalFailed(deploy/helm/paperclip/templates/prometheusrule.yaml),pr_review-scoped,for: 30m, configurable viaprometheusRule.wakeTerminalFailedFor.runbooks/agent-wakeup-terminal-failed.md, linked from the alert annotation and indexed inrunbooks/README.md.server/src/__tests__/heartbeat-wake-terminal-failed-gauge.test.ts(new, 9 cases) and a new case indeploy/helm/paperclip/tests/prometheus-rule.test.mjs.Two bugs found while testing
These cancelled each other out, so they were invisible until the comparison started working — worth a look during review since the second is subtle:
max(timestamptz)comes back from the driver as a STRING, whatever thesql<Date|null>annotation claims. Comparing it to aDatewith>coerces both to numbers, the string becomesNaN, and every successor check silently answeredfalse— the exclusion was entirely dead, which would have paged on exactly the retried rows it promises to skip. Now parsed at the boundary, matching the existingrefreshExternalRuntimeReservationMetricsprecedent (external-runtime-reservations.ts:73).requestedAt(when the wake was asked for) postdates a sibling'sfinishedAt, so two failures on one taskKey suppressed each other and the gauge read0on a repeatedly-failing review chain. Candidates are now excluded by id, and a successor that itself endedfailedno longer counts as coverage — a second failure is not a re-drive.Verification
Run locally on a base rebased onto
origin/master(which had moved 19 commits):The gauge tests use embedded-postgres and cover: the zero-initialized label grid, counting a seeded
failedrow, the successor-wake exclusion, the successor-run exclusion,error_code=noneon a runless row,scopeseparation, unknown-code collapse tootherwith no cardinality growth, recency-window ageing, and a regression guard asserting both rows count when one taskKey fails twice.helm templaterendering asserted inprometheus-rule.test.mjs, so I matched it rather than introducing a second mechanism. The renderedexpr/for/runbook_urlare asserted; PromQL evaluation semantics are not. Happy to wire up promtool instead if you'd prefer.Risks
Low-to-moderate, and additive — no migration, no schema change, no behavioural change to any existing path. The new work runs inside an existing reconcile pass wrapped in try/catch, so a failure logs a warning and cannot break dispatch re-driving, which is that pass's real job.
Specific things worth a reviewer's attention:
queuedwake and ascheduled_retryrun inside the scheduling transaction), not off anything fix(heartbeat): bounded-retry stale-killed pr_review runs (BLO-18030) #900 adds. If fix(heartbeat): bounded-retry stale-killed pr_review runs (BLO-18030) #900 lands later, retried rows drop out the same way. This also means retry safety does not depend on the 30mfor— the successor rows exist the moment the retry is scheduled, at every step ofBOUNDED_TRANSIENT_HEARTBEAT_RETRY_DELAYS_MS([2m, 10m, 30m, 2h]), including the two longer than the window.error_codex 2scope= 16 constant series; an untriaged code collapses tootherrather than minting a series.INlists derived from that capped set.prometheusRule.enabledis false under the Blockcast values (no RBAC for PrometheusRule), asserted by an existing test, so this renders nothing until that changes.Model Used
Claude Opus 4.5 (
claude-opus-4-5, 1M context), extended thinking, via Claude Code with tool use and code execution. All test runs, the typecheck, and the schema/precedent lookups cited above were executed, not inferred.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template🤖 Generated with Claude Code