Add overdue-scheduled_retry age gauge and alert (BLO-22094) - #1184
Conversation
1 similar comment
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: 5f6d705
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- The overdue gauge uses
scheduled_retry_atonly after it is due, so normal backoff is not reported as a stranded retry. - Reset-then-set aggregation and explicit zero series allow the new alert to resolve after promotion or re-deferral.
- Tests cover due-time arithmetic, future retries, promotion, aggregation, clearing, and rendered alert shape.
Recommended Action
- No blocking changes requested.
5f6d705 to
abaa049
Compare
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: abaa049
Critical Issues (0)
Important Issues (1)
- [native-codex]
server/src/services/queued-run-age-metrics.ts:87— This new aggregate runs on every unauthenticated/metricsscrape butheartbeat_runshas no index forstatus = 'scheduled_retry'andscheduled_retry_at < now(). As history grows, PostgreSQL must scan/filter the full run table and then group it, adding unbounded database work to every Prometheus scrape. Add a migration with a partial index on overdue-retry lookup columns, e.g.(scheduled_retry_at, agent_id) WHERE status = 'scheduled_retry', and validate the query plan against realistic run history.
Suggestions (0)
Strengths
- The age derives from the missed due time, correctly excluding ordinary backoff.
- Reset-then-set behavior gives every known agent an explicit zero and allows alert resolution after promotion.
- The added tests cover due-time arithmetic, aggregation, promotion, and clearing.
Recommended Action
- Address the Important issue before merge.
…LO-22094) Ally review on #1184: refreshOverdueScheduledRetryAgeMetrics filters status='scheduled_retry' AND scheduled_retry_at < now() on every unauthenticated /metrics scrape with no supporting index, forcing a sequential scan over all of heartbeat_runs (measured ~219k rows / ~1.8 GB in migration 0208) instead of the tiny handful of rows actually parked at any moment. Add a partial index on (agent_id, scheduled_retry_at) WHERE status='scheduled_retry', following the same guarded online-precreation pattern migrations 0205/0208/0209 already use for this table. Co-Authored-By: Paperclip <noreply@paperclip.ing>
|
Addressed the Important finding from Ally's review (native-codex lens, Pushed |
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: 05e487a
Prior Findings Dispositioned (1)
- prior:abaa049 important 1 — fixed —
packages/db/src/migrations/0213_heartbeat_runs_overdue_scheduled_retry_index.sql:82— The new verified partial index onheartbeat_runseliminates the full historical-table scan previously caused by the scrape-time aggregate.
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- The gauge measures age after
scheduled_retry_atbecomes due, keeping ordinary retry backoff out of the alert. - The verified online-precreation migration follows the established hot-table migration pattern and prevents an unbounded historical scan during Prometheus scrapes.
- Tests cover due-time arithmetic, future retries, promotion, aggregation, reset-to-zero behavior, and rendered alert shape.
Recommended Action
- No blocking changes requested.
c5a3b49 to
a96b115
Compare
…LO-22094) Ally review on #1184: refreshOverdueScheduledRetryAgeMetrics filters status='scheduled_retry' AND scheduled_retry_at < now() on every unauthenticated /metrics scrape with no supporting index, forcing a sequential scan over all of heartbeat_runs (measured ~219k rows / ~1.8 GB in migration 0208) instead of the tiny handful of rows actually parked at any moment. Add a partial index on (agent_id, scheduled_retry_at) WHERE status='scheduled_retry', following the same guarded online-precreation pattern migrations 0205/0208/0209 already use for this table. Co-Authored-By: Paperclip <noreply@paperclip.ing>
05e487a to
f5a5c6f
Compare
…LO-22094) Ally review on #1184: refreshOverdueScheduledRetryAgeMetrics filters status='scheduled_retry' AND scheduled_retry_at < now() on every unauthenticated /metrics scrape with no supporting index, forcing a sequential scan over all of heartbeat_runs (measured ~219k rows / ~1.8 GB in migration 0208) instead of the tiny handful of rows actually parked at any moment. Add a partial index on (agent_id, scheduled_retry_at) WHERE status='scheduled_retry', following the same guarded online-precreation pattern migrations 0205/0208/0209 already use for this table. Co-Authored-By: Paperclip <noreply@paperclip.ing>
f5a5c6f to
6cd1cf1
Compare
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: 6cd1cf1
Critical Issues (0)
Important Issues (1)
-
[gstack/review + native-codex]
server/src/services/queued-run-age-metrics.ts:93— The new gauge has no freshness companion, so a failed refresh leaves it frozen and the alert cannot tell a stale value from a healthy one.The sibling in this same file (
refreshQueuedRunAgeMetrics, lines 35–69) wraps its work in try/catch and callssetQueuedRunAgeMetricsRefreshSuccess(false)on failure, with an explicit comment that it keeps the last snapshot rather than writing synthetic zeros because "the companion freshness gauge makes the stale data ineligible for the stranded-run alert."PaperclipQueuedRunStrandedconsumes that atdeploy/helm/paperclip/templates/prometheusrule.yaml:370viaand on(instance) (paperclip_queued_run_age_metrics_refresh_success == 1), andPaperclipQueuedRunAgeMetricsRefreshFailedpages on it directly.refreshOverdueScheduledRetryAgeMetricsinherits none of that.gauge.reset()runs only insidesetOverdueScheduledRetryAgeMetricson the success path, so when the aggregate throws, the previous per-agent values persist unchanged;server/src/app.ts:319swallows the rejection into alogger.warnand the scrape still returns 200.prometheusrule.yaml:423then evaluatesmax(...) by (agent_id) > 5400against that stale snapshot with no instance-level freshness filter. Two concrete failure modes:- Frozen at 0 — the overwhelmingly common healthy value — means the detector this PR exists to add is silently dead, and nothing anywhere indicates it. This is exactly the hazard the sibling alert's own description warns about ("do not interpret a stale zero as an empty queue").
- Frozen at a high value on one replica means a false page that
maxacross replicas cannot shed until that replica's refresh recovers.
The shared-database case is covered incidentally: if Postgres is unreachable both refreshes fail and
PaperclipQueuedRunAgeMetricsRefreshFailedpages. But nothing enforces that coupling — the new aggregate is a different query against a different index (migration 0222) with a different predicate, so a statement timeout, lock wait, or plan regression affecting only it fails only it, leavingpaperclip_queued_run_age_metrics_refresh_success == 1and the overdue gauge frozen with no signal at all.- Recommendation: give the new gauge the same contract — try/catch in
refreshOverdueScheduledRetryAgeMetricsdriving apaperclip_overdue_scheduled_retry_age_metrics_refresh_successgauge, gate the alert expr withand on(instance) (... == 1), and add the companion refresh-failed alert. Note that the helm assertion atdeploy/helm/paperclip/tests/prometheus-rule.test.mjs:587anchors the expr with/^max\(...\) by \(agent_id\) > (\d+)$/, so it needs loosening for a gated expr to render.
Suggestions (1)
- [pr-review-toolkit]
server/src/services/metrics.ts:2119—__resetMetricsForTestnulls every other gauge holder (queuedRunOldestAgeon this line) but notoverdueScheduledRetryOldestAge, declared at line 1008. This is harmless today: the reset setsregistry = nullfirst, and theensureRegistryguard at line 1070 short-circuits on!registry, so the whole registry — including this gauge — is rebuilt regardless. Worth adding for consistency anyway, since it is now the only holder left dangling and the omission becomes a live cross-test-contamination bug the moment that reset stops nullingregistrywholesale.
Strengths
- The gauge ages off
scheduled_retry_atrather thancreated_atand counts only rows already past due, so ordinary retry backoff contributes exactly 0 — the alert fires on promotion-sweep overshoot, not on designed backoff. The test atserver/src/__tests__/overdue-scheduled-retry-metrics.test.ts:83pins the future-due-time case explicitly. - The threshold derivation is unusually well evidenced: an n=5253 population with quantiles, an explicit rejection of the 5-sample BLO-16366 table as measuring a different quantity (due-to-execution-start vs due-to-promotion), and a stated reason for margining off the worst day's max rather than the aggregate p99 because
ccrotate_capacityhas its own p99 of 2528s. - The migration guard does not just require online precreation — it verifies the precreated index matches the definition (access method, key count, column order,
indoption, partial predicate) rather than accepting any index sharing the name, and all three rejection paths are covered by tests. - Reset-then-set with an explicit per-agent 0 means the alert can resolve after promotion, and the "promoted run keeps its stale
scheduledRetryAt" trap is called out in a comment and pinned by a test. - The runbook's Step 2 distinguishes a wedged sweep from a legitimate
issue_dependencies_blockedre-defer with a concrete diagnostic (re-run the query and watchupdated_at/scheduled_retry_attemptmove), which is the triage an on-call reader actually needs.
Recommended Action
- Address the Important issue this cycle — the freshness gate is what makes this detector trustworthy, and without it a refresh failure can silently disable the very signal being added.
- Consider the Suggestion opportunistically.
CI note: the five red checks on this head (Build, Typecheck + Release Registry, General tests (server 2/4), General tests (workspaces-b), Canary Dry Run) all terminate in the same runner shutdown at 07:18:06Z — "The runner has received a shutdown signal … The operation was canceled." Build reports conclusion cancelled. None is a code failure from this diff, and the server tsc build completed before the cancellation. Helm chart, policy, and workspaces-a passed. The new vitest suites had not reported before the cancellation, so this review does not claim they are green — a re-run is needed to establish that.
…(BLO-22094) PaperclipQueuedRunStranded (BLO-21116) deliberately excludes status='scheduled_retry' rows from its queued-age gauge, so a retry that parks and is never promoted was invisible to any detector, forever. Add paperclip_overdue_scheduled_retry_oldest_age_seconds (ages off scheduled_retry_at due time, not creation time; 0 for a row still backing off) and the PaperclipOverdueScheduledRetry alert on top of it, threshold derived from a 7-day park-to-promotion population rather than the 5-sample table in the issue. Extends the runbook to distinguish a wedged promotion sweep from a gate (issue_dependencies_blocked) legitimately re-deferring. Co-Authored-By: Paperclip <noreply@paperclip.ing>
…LO-22094) Ally review on #1184: refreshOverdueScheduledRetryAgeMetrics filters status='scheduled_retry' AND scheduled_retry_at < now() on every unauthenticated /metrics scrape with no supporting index, forcing a sequential scan over all of heartbeat_runs (measured ~219k rows / ~1.8 GB in migration 0208) instead of the tiny handful of rows actually parked at any moment. Add a partial index on (agent_id, scheduled_retry_at) WHERE status='scheduled_retry', following the same guarded online-precreation pattern migrations 0205/0208/0209 already use for this table. Co-Authored-By: Paperclip <noreply@paperclip.ing>
… freshness (BLO-22094) Ally's Important finding on #1184: refreshOverdueScheduledRetryAgeMetrics shipped without the try/catch, freshness gauge, and alert gate its sibling refreshQueuedRunAgeMetrics carries. That gap is not cosmetic here. The reset-then-set only runs on the success path, so a throw leaves the previous per-agent values frozen in the registry while /metrics still returns 200 (app.ts swallows the rejection into a logger.warn). The frozen value is almost always 0 -- the HEALTHY reading -- so an ungated alert sits silently green on top of a dead detector. That is precisely the invisible-failure class this whole issue exists to eliminate: without this, the fix for an unobservable failure is itself unobservable. - queued-run-age-metrics.ts: wrap the refresh in try/catch driving the new freshness gauge, rethrowing so the caller still logs. Do not zero the age gauge on failure -- synthetic zeros would read as "nothing overdue". - metrics.ts: add paperclip_overdue_scheduled_retry_age_metrics_refresh_success plus its setter. Deliberately a separate series from the sibling's: the two refreshes run different aggregates behind different indexes (0217 for status='queued', 0224 for the overdue-parked predicate), so one can fail alone and a shared signal would let a healthy sibling vouch for a dead one. - metrics.ts: null overdueScheduledRetryOldestAge in __resetMetricsForTest, which the original commit missed (Ally's Suggestion). - prometheusrule.yaml: gate the alert with `and on(instance) (... == 1)` inside the `max by (agent_id)` (the aggregation strips the instance label the join needs), and add the companion PaperclipOverdueScheduledRetryAgeMetricsRefreshFailed alert. - tests: assert the gated expr shape rather than the bare one, assert the companion alert renders, assert the two freshness signals move independently, and cover the stale-snapshot path server-side. - runbook: new "When the overdue refresh-failure alert fires" section reading it as a detector outage rather than an all-clear, and calling out that a healthy sibling refresh does not vouch for this one. Threshold unchanged at 5400s. Also renumbers the index migration 0222 -> 0224; master took 0222 and 0223 during reconciliation, and the in-SQL/in-test message strings move with it. Refs: https://paperclip.blockcast.net/BLO/issues/BLO-22094 Co-Authored-By: Paperclip <noreply@paperclip.ing>
6cd1cf1 to
3606299
Compare
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: 3606299
Prior Findings Dispositioned (1)
- prior:6cd1cf1 important 1 — fixed —
server/src/services/queued-run-age-metrics.ts:124— The refresh now marks the independent freshness gauge successful only after the indexed overdue aggregate and reset-then-set publication complete; failures set that gauge to 0 and rethrow, while the alert gates on the freshness signal.
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- The overdue gauge correctly measures time past
scheduled_retry_at, not ordinary retry backoff. - The partial index migration and verified online-precreation guard protect the scrape-time aggregate on the hot
heartbeat_runstable. - The independent freshness gauge, gated alert, companion failure alert, and focused tests close the previously identified stale-snapshot failure mode.
Recommended Action
- No blocking changes requested.
The runbooks/README entry for the overdue-scheduled-retry section still listed only the ungated trigger and omitted PaperclipOverdueScheduledRetryAgeMetricsRefreshFailed. Mirror the sibling entry above, which already names both its alerts and its stale-snapshot case. Refs: https://paperclip.blockcast.net/BLO/issues/BLO-22094 Co-Authored-By: Paperclip <noreply@paperclip.ing>
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: 50a9092
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- The overdue age is derived from
scheduled_retry_atand filtered to rows already past due, so ordinary retry backoff is excluded. - The verified partial-index migration removes the scrape-time historical-table scan while preserving an online precreation path for populated databases.
- The independent freshness gauge, gated alert, companion failure alert, and focused tests make refresh failures observable instead of treating stale values as a healthy zero.
Recommended Action
- No blocking changes requested.
Thinking Path
Linked Issues or Issue Description
Fixes: BLO-22094
Refs: BLO-21116
Unstacked — targets
masterdirectly. Originally stacked on #1019 (BLO-21116), which merged 2026-08-12. Reconciled onto current master by cherry-pick (see "Reconciliation notes").What Changed
server/src/services/queued-run-age-metrics.ts: newrefreshOverdueScheduledRetryAgeMetrics— same "query every agent id, reset-then-set" shape as the siblingrefreshQueuedRunAgeMetrics, aggregatingMIN(scheduled_retry_at)over rows wherestatus='scheduled_retry' AND scheduled_retry_at < now(). Wrapped in the same try/catch the sibling carries, driving its own freshness gauge and rethrowing.server/src/services/metrics.ts: newpaperclip_overdue_scheduled_retry_oldest_age_secondsgauge +setOverdueScheduledRetryAgeMetrics, reset-then-set per agent so an agent with no overdue parked row reads an explicit0. Pluspaperclip_overdue_scheduled_retry_age_metrics_refresh_successand its setter.server/src/app.ts: wire the new refresh into the/metricsscrape path alongside the existing queued-run refresh.deploy/helm/paperclip/templates/prometheusrule.yaml+values.yaml: newPaperclipOverdueScheduledRetryalert at> 5400(90m) for 5m, gated on the freshness gauge; plus the companionPaperclipOverdueScheduledRetryAgeMetricsRefreshFailed. Threshold derived from the observed park→promotion lag distribution over the 7 days preceding authorship (2026-07-31..2026-08-07, n=5253 promotions: p50=21.5s, p90=83.7s, p95=131.9s, p99=1594.8s, max=3567.5s — zero of 5253 exceeded 3600s); set to ~50% margin over the worst single day's max rather than the aggregate p99, becauseccrotate_capacity's own per-reason p99 (2528s) would sit too close to a p99-derived threshold on a contention day. Full derivation is in thevalues.yamlcomment. Threshold unchanged by the freshness work.packages/db/src/migrations/0224_heartbeat_runs_overdue_scheduled_retry_index.sql: partial index(agent_id, scheduled_retry_at) WHERE status = 'scheduled_retry', so the new scrape-time aggregate does not sequential-scanheartbeat_runs(~219k rows / ~1.8 GB per migration 0208). Added in response to Ally's earlier Important finding. Master's0217_heartbeat_runs_queued_age_idxisWHERE status = 'queued'and does not cover this predicate. Follows the guarded online-precreation pattern already established on this table (0205/0208/0209/0217).runbooks/queued-run-stranded.md: new## Overdue scheduled-retry (BLO-22094)section — the diagnostic split between a legitimately re-deferring gate (issue_dependencies_blocked) and a wedgedpromoteDueScheduledRetriessweep — plus aWhen the overdue refresh-failure alert firessubsection.runbooks/README.mdindex entry added.prometheus-rule.test.mjs), due-time arithmetic + explicit-0reset + stale-snapshot behaviour (overdue-scheduled-retry-metrics.test.ts), and the migration's precreation guard (heartbeat-runs-overdue-scheduled-retry-index-migration.test.ts).Addressing Ally's review (commit
3606299f)1 Important — the new refresh had no failure handling, no freshness gauge, and an ungated alert. Confirmed against the source and fixed. The sibling
refreshQueuedRunAgeMetricscarries all three; this one carried none:refreshQueuedRunAgeMetricssetQueuedRunAgeMetricsRefreshSuccesssetOverdueScheduledRetryAgeMetricsRefreshSuccessand on(instance) (… == 1)Why this was treated as blocking rather than a nit.
gauge.reset()runs only insidesetOverdueScheduledRetryAgeMetricson the success path, andapp.tsswallows the rejection intologger.warnwhile the scrape still returns 200. So a failed refresh leaves the previous per-agent values frozen — and the frozen value is almost always0, the healthy reading. An ungated alert would therefore sit silently green on top of a dead detector: the exact invisible-failure class this issue exists to eliminate. Shipping it ungated would make the fix for an unobservable failure itself unobservable.The freshness gauge is deliberately a separate series from the sibling's, not a shared one. The two refreshes run different aggregates against different indexes (
0217forstatus='queued',0224for the overdue-parked predicate), so a statement timeout or plan regression can hit one alone — leavingpaperclip_queued_run_age_metrics_refresh_successat1while this gauge is frozen. A test asserts the two move independently.The gate sits inside the
max by (agent_id)becauseon(instance)needs the instance label the aggregation strips — same shape as the sibling.1 Suggestion —
__resetMetricsForTestdid not nulloverdueScheduledRetryOldestAge. Taken; both it and the new freshness gauge are now reset.Reconciliation notes
This branch was reconciled onto master twice. The second pass (this one) cherry-picked the two BLO-22094 commits onto
a7201a36after the branch wentDIRTYat 33 behind. Two conflicts, both mechanical:tests/prometheus-rule.test.mjsmigrations/meta/_journal.json0222and02230224Migration renumbered
0222→0224(it had already moved0213→0222in the first reconciliation).0222_issues_parked_dispositionand0223_pipeline_stage_exit_hardeninglanded on master in between. The file, its_journal.jsonentry (idx: 224,whenbumped past0223), and all in-SQL/in-testmigration 0222message strings were updated together — the test asserts on those exact strings, so a partial rename would pass the numbering check and fail the test.Note the test regexes matching
alert: PaperclipOverdueScheduledRetryare now anchored with a trailing\n, since the new companion alert name is a prefix-superset of the original.Verification
Re-run in full against the reconciled + review-fixed branch:
Rendered alerts confirmed from the reconciled tree:
Risks
refreshQueuedRunAgeMetrics'sstatus='queued'filter orpromoteScheduledRetryRun'squeuedAtreset — both stay exactly as BLO-21116/onprem-k8s#2013 intentionally left them.deploy/helm/paperclip/templates/prometheusrule.yamldoes not deploy on Blockcast (prometheusRule.enabled: false); the rule that actually pages must be landed inBlockcast/onprem-k8s(lockstep files) and synced through themonitoring-rulesArgo app, same as BLO-21116's alert. Flagged in the runbook; not done in this PR since it's an onprem-k8s-repo change, same split as the original alert (onprem-k8s#2029).ccrotate_capacity/issue_dependencies_blockedbackoff shape changes materially, it should be re-derived rather than assumed permanent.CONCURRENTLYbefore the migration applies, and a mismatched precreated index is rejected with a repair hint.Model Used
Claude Opus 5 (
claude-opus-5[1m]), 1M context window, via Claude Code (Paperclipclaude_k8sadapter) for this reconciliation + review-fix pass. Original implementation was authored under Claude Sonnet 5 (claude-sonnet-5[1m]) on the same adapter.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template3606299fCo-Authored-By: Paperclip noreply@paperclip.ing