Skip to content

Add overdue-scheduled_retry age gauge and alert (BLO-22094) - #1184

Merged
allyblockcast[bot] merged 4 commits into
masterfrom
sre/blo-22094-overdue-scheduled-retry-gauge
Aug 22, 2026
Merged

Add overdue-scheduled_retry age gauge and alert (BLO-22094)#1184
allyblockcast[bot] merged 4 commits into
masterfrom
sre/blo-22094-overdue-scheduled-retry-gauge

Conversation

@allyblockcast

@allyblockcast allyblockcast Bot commented Aug 9, 2026

Copy link
Copy Markdown

Thinking Path

  • Paperclip is the open source app people use to manage AI agents for work
  • The heartbeat scheduler's scheduled_retry state parks a run until a due time, then a periodic sweep (promoteDueScheduledRetries) promotes it back to queued
  • PaperclipQueuedRunStranded (fix(recovery,observability): stop misclassifying JSON-parse adapter failures, add queued-run-age gauge (BLO-21116) #1019, BLO-21116) deliberately excludes scheduled_retry rows from its queued-age gauge, on purpose (onprem-k8s#2013) — but the side effect is that a retry which parks and is never promoted is invisible to any detector, forever
  • Observed park→dispatch lag on a real retry chain (BLO-16366) ranged from +2m to +3h49m across five samples — luck, not observability, since nothing pages if the promotion sweep wedges
  • It needs a dedicated gauge and alert that only this state transition can supply, aged off scheduled_retry_at due time rather than creation time so backoff never counts against it
  • This pull request adds paperclip_overdue_scheduled_retry_oldest_age_seconds, the PaperclipOverdueScheduledRetry alert on top of it (threshold derived from a 7-day park→promotion population, not the 5-sample table), its own freshness gate so the detector cannot fail silently, and a runbook section distinguishing a wedged promotion sweep from a gate (issue_dependencies_blocked) legitimately re-deferring
  • The benefit is a dead/wedged scheduled-retry promotion path finally has a detector, closing the last blind spot in the BLO-21116 queued-run observability work

Linked Issues or Issue Description

Fixes: BLO-22094
Refs: BLO-21116

Unstacked — targets master directly. 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: new refreshOverdueScheduledRetryAgeMetrics — same "query every agent id, reset-then-set" shape as the sibling refreshQueuedRunAgeMetrics, aggregating MIN(scheduled_retry_at) over rows where status='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: new paperclip_overdue_scheduled_retry_oldest_age_seconds gauge + setOverdueScheduledRetryAgeMetrics, reset-then-set per agent so an agent with no overdue parked row reads an explicit 0. Plus paperclip_overdue_scheduled_retry_age_metrics_refresh_success and its setter.
  • server/src/app.ts: wire the new refresh into the /metrics scrape path alongside the existing queued-run refresh.
  • deploy/helm/paperclip/templates/prometheusrule.yaml + values.yaml: new PaperclipOverdueScheduledRetry alert at > 5400 (90m) for 5m, gated on the freshness gauge; plus the companion PaperclipOverdueScheduledRetryAgeMetricsRefreshFailed. 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, because ccrotate_capacity's own per-reason p99 (2528s) would sit too close to a p99-derived threshold on a contention day. Full derivation is in the values.yaml comment. 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-scan heartbeat_runs (~219k rows / ~1.8 GB per migration 0208). Added in response to Ally's earlier Important finding. Master's 0217_heartbeat_runs_queued_age_idx is WHERE 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 wedged promoteDueScheduledRetries sweep — plus a When the overdue refresh-failure alert fires subsection. runbooks/README.md index entry added.
  • Tests: rendered-alert assertions (prometheus-rule.test.mjs), due-time arithmetic + explicit-0 reset + 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 refreshQueuedRunAgeMetrics carries all three; this one carried none:

sibling refreshQueuedRunAgeMetrics before after
try/catch
freshness gauge setQueuedRunAgeMetricsRefreshSuccess setOverdueScheduledRetryAgeMetricsRefreshSuccess
alert gated and on(instance) (… == 1) ❌ bare ✅ gated
companion refresh-failed alert

Why this was treated as blocking rather than a nit. gauge.reset() runs only inside setOverdueScheduledRetryAgeMetrics on the success path, and app.ts swallows the rejection into logger.warn while the scrape still returns 200. So a failed refresh leaves the previous per-agent values frozen — and the frozen value is almost always 0, 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 (0217 for status='queued', 0224 for the overdue-parked predicate), so a statement timeout or plan regression can hit one alone — leaving paperclip_queued_run_age_metrics_refresh_success at 1 while this gauge is frozen. A test asserts the two move independently.

The gate sits inside the max by (agent_id) because on(instance) needs the instance label the aggregation strips — same shape as the sibling.

1 Suggestion — __resetMetricsForTest did not null overdueScheduledRetryOldestAge. 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 a7201a36 after the branch went DIRTY at 33 behind. Two conflicts, both mechanical:

File Conflict Resolution
tests/prometheus-rule.test.mjs master appended tests in the same region; HEAD side empty (pure append at EOF) keep both
migrations/meta/_journal.json master took 0222 and 0223 keep master's, append mine as 0224

Migration renumbered 02220224 (it had already moved 02130222 in the first reconciliation). 0222_issues_parked_disposition and 0223_pipeline_stage_exit_hardening landed on master in between. The file, its _journal.json entry (idx: 224, when bumped past 0223), and all in-SQL/in-test migration 0222 message 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: PaperclipOverdueScheduledRetry are 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:

node --test deploy/helm/paperclip/tests/prometheus-rule.test.mjs                    # 14/14 pass (mine + master's invariant tests)
pnpm --filter @paperclipai/server exec vitest run \
  src/__tests__/overdue-scheduled-retry-metrics.test.ts \
  src/__tests__/queued-run-age-metrics.test.ts \
  src/__tests__/metrics-service.test.ts                                             # 84/84 pass, no regression on the sibling gauge
pnpm --filter @paperclipai/db exec vitest run \
  src/heartbeat-runs-overdue-scheduled-retry-index-migration.test.ts                # 3/3 pass
pnpm --filter @paperclipai/db exec vitest run \
  src/check-migration-safety.test.ts src/client.test.ts                             # 39/39 pass (applies 0224 in sequence)
pnpm --filter @paperclipai/db exec tsx src/check-migration-numbering.ts             # clean
pnpm --filter @paperclipai/server exec tsc --noEmit -p .                            # clean

Rendered alerts confirmed from the reconciled tree:

alert: PaperclipOverdueScheduledRetry
expr: max by (agent_id) (paperclip_overdue_scheduled_retry_oldest_age_seconds and on(instance) (paperclip_overdue_scheduled_retry_age_metrics_refresh_success == 1)) > 5400
for: 5m

alert: PaperclipOverdueScheduledRetryAgeMetricsRefreshFailed
expr: paperclip_overdue_scheduled_retry_age_metrics_refresh_success == 0
for: 5m

Risks

  • Additive only: new gauges, new alerts, new index, new runbook section. Does not change refreshQueuedRunAgeMetrics's status='queued' filter or promoteScheduledRetryRun's queuedAt reset — both stay exactly as BLO-21116/onprem-k8s#2013 intentionally left them.
  • Same known limitation as the parent alert: deploy/helm/paperclip/templates/prometheusrule.yaml does not deploy on Blockcast (prometheusRule.enabled: false); the rule that actually pages must be landed in Blockcast/onprem-k8s (lockstep files) and synced through the monitoring-rules Argo 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).
  • Threshold is a wedge detector, not a saturation detector — deliberately. A later fleet census (2026-08-19, n=595 parked / 272 currently overdue) found lateness ceilinged at 42.3 min with 143 rows >30 min overdue; a 15-min threshold would fire today across ~9 agents. That is a real signal, but its cause is per-agent concurrency saturation, not a wedged sweep, and it is not actionable by this alert's runbook — so 5400s stays. The saturation view belongs in its own signal rather than by lowering this one into permanent-firing territory; noted on the issue.
  • Threshold (5400s) is a first cut from 7 days of real data — if the ccrotate_capacity/issue_dependencies_blocked backoff shape changes materially, it should be re-derived rather than assumed permanent.
  • Index migration takes the established guarded path: a populated database must precreate it CONCURRENTLY before 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 (Paperclip claude_k8s adapter) for this reconciliation + review-fix pass. Original implementation was authored under Claude Sonnet 5 (claude-sonnet-5[1m]) on the same adapter.

Checklist

  • I have included a thinking path that traces from project context to this change
  • I have specified the model used (with version and capability details)
  • I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work
  • I have searched GitHub for duplicate or related PRs and linked them above (none found for BLO-22094; fix(recovery,observability): stop misclassifying JSON-parse adapter failures, add queued-run-age gauge (BLO-21116) #1019 was the parent and is now merged)
  • I have either (a) linked existing issues with Fixes: # / Closes # / Refs # OR (b) described the issue in-PR following the relevant issue template
  • I have run tests locally and they pass
  • I have added or updated tests where applicable
  • If this change affects the UI, I have included before/after screenshots — N/A, no UI change
  • I have updated relevant documentation to reflect my changes (runbook + index)
  • I have considered and documented any risks above
  • All Paperclip CI gates are green — re-running against reconciled head 3606299f
  • I will address all Greptile and reviewer comments before requesting merge

Co-Authored-By: Paperclip noreply@paperclip.ing

@allyblockcast

allyblockcast Bot commented Aug 9, 2026

Copy link
Copy Markdown
Author

🔗 Paperclip issue: BLO-21116
🔗 Paperclip issue: BLO-22094
🔗 Paperclip issue: BLO-16366

1 similar comment
@allyblockcast

allyblockcast Bot commented Aug 9, 2026

Copy link
Copy Markdown
Author

🔗 Paperclip issue: BLO-21116
🔗 Paperclip issue: BLO-22094
🔗 Paperclip issue: BLO-16366

@allyblockcast allyblockcast Bot left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_at only 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

  1. No blocking changes requested.

@allyblockcast
allyblockcast Bot force-pushed the sre/blo-22094-overdue-scheduled-retry-gauge branch from 5f6d705 to abaa049 Compare August 9, 2026 01:19

@allyblockcast allyblockcast Bot left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 /metrics scrape but heartbeat_runs has no index for status = 'scheduled_retry' and scheduled_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

  1. Address the Important issue before merge.

allyblockcast Bot pushed a commit that referenced this pull request Aug 9, 2026
…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>
@allyblockcast

allyblockcast Bot commented Aug 9, 2026

Copy link
Copy Markdown
Author

Addressed the Important finding from Ally's review (native-codex lens, queued-run-age-metrics.ts:87): confirmed refreshOverdueScheduledRetryAgeMetrics had no supporting index — heartbeat_runs has no index pairing status with scheduled_retry_at, so the status='scheduled_retry' AND scheduled_retry_at < now() filter on every /metrics scrape fell back to a sequential scan over the whole table (migration 0208 measured ~219k rows / ~1.8 GB, growing).

Pushed 05e487a: migration 0213_heartbeat_runs_overdue_scheduled_retry_index.sql adds a partial index (agent_id, scheduled_retry_at) WHERE status = 'scheduled_retry' — small (only parked rows), lets the GROUP BY agent_id compute MIN(scheduled_retry_at) straight off the index instead of a table scan, and also serves the on-call runbook's per-agent overdue query. Followed the same guarded online-precreation pattern already established on this table by migrations 0205/0208/0209 (populated DBs must precreate the index CONCURRENTLY; the migration verifies the precreated index matches exactly rather than trusting the name). Added heartbeat-runs-overdue-scheduled-retry-index-migration.test.ts mirroring the sibling index migration tests (precreation required, invalid index rejected, wrong-definition same-name index rejected) — all passing against embedded Postgres, along with the full client.test.ts migration-application suite and the static check-migration-numbering/check-migration-safety checks.

@allyblockcast allyblockcast Bot left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 on heartbeat_runs eliminates 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_at becomes 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

  1. No blocking changes requested.

@kkroo
kkroo requested a review from allyblockcast August 11, 2026 21:56
@allyblockcast
allyblockcast Bot force-pushed the sre/blo-21116-queued-run-fixes branch 3 times, most recently from c5a3b49 to a96b115 Compare August 12, 2026 12:47
allyblockcast Bot pushed a commit that referenced this pull request Aug 20, 2026
…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>
@allyblockcast
allyblockcast Bot force-pushed the sre/blo-22094-overdue-scheduled-retry-gauge branch from 05e487a to f5a5c6f Compare August 20, 2026 07:01
@allyblockcast
allyblockcast Bot changed the base branch from sre/blo-21116-queued-run-fixes to master August 20, 2026 07:01
allyblockcast Bot pushed a commit that referenced this pull request Aug 20, 2026
…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>
@allyblockcast
allyblockcast Bot force-pushed the sre/blo-22094-overdue-scheduled-retry-gauge branch from f5a5c6f to 6cd1cf1 Compare August 20, 2026 07:09

@allyblockcast allyblockcast Bot left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 calls setQueuedRunAgeMetricsRefreshSuccess(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." PaperclipQueuedRunStranded consumes that at deploy/helm/paperclip/templates/prometheusrule.yaml:370 via and on(instance) (paperclip_queued_run_age_metrics_refresh_success == 1), and PaperclipQueuedRunAgeMetricsRefreshFailed pages on it directly.

    refreshOverdueScheduledRetryAgeMetrics inherits none of that. gauge.reset() runs only inside setOverdueScheduledRetryAgeMetrics on the success path, so when the aggregate throws, the previous per-agent values persist unchanged; server/src/app.ts:319 swallows the rejection into a logger.warn and the scrape still returns 200. prometheusrule.yaml:423 then evaluates max(...) by (agent_id) > 5400 against 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 max across replicas cannot shed until that replica's refresh recovers.

    The shared-database case is covered incidentally: if Postgres is unreachable both refreshes fail and PaperclipQueuedRunAgeMetricsRefreshFailed pages. 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, leaving paperclip_queued_run_age_metrics_refresh_success == 1 and the overdue gauge frozen with no signal at all.

    • Recommendation: give the new gauge the same contract — try/catch in refreshOverdueScheduledRetryAgeMetrics driving a paperclip_overdue_scheduled_retry_age_metrics_refresh_success gauge, gate the alert expr with and on(instance) (... == 1), and add the companion refresh-failed alert. Note that the helm assertion at deploy/helm/paperclip/tests/prometheus-rule.test.mjs:587 anchors 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__resetMetricsForTest nulls every other gauge holder (queuedRunOldestAge on this line) but not overdueScheduledRetryOldestAge, declared at line 1008. This is harmless today: the reset sets registry = null first, and the ensureRegistry guard 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 nulling registry wholesale.

Strengths

  • The gauge ages off scheduled_retry_at rather than created_at and 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 at server/src/__tests__/overdue-scheduled-retry-metrics.test.ts:83 pins 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_capacity has 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_blocked re-defer with a concrete diagnostic (re-run the query and watch updated_at/scheduled_retry_attempt move), which is the triage an on-call reader actually needs.

Recommended Action

  1. 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.
  2. 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.

PlatformSREEngineer and others added 3 commits August 22, 2026 09:45
…(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>
@allyblockcast
allyblockcast Bot force-pushed the sre/blo-22094-overdue-scheduled-retry-gauge branch from 6cd1cf1 to 3606299 Compare August 22, 2026 10:08

@allyblockcast allyblockcast Bot left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_runs table.
  • The independent freshness gauge, gated alert, companion failure alert, and focused tests close the previously identified stale-snapshot failure mode.

Recommended Action

  1. 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>

@allyblockcast allyblockcast Bot left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_at and 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

  1. No blocking changes requested.

@allyblockcast
allyblockcast Bot added this pull request to the merge queue Aug 22, 2026
Merged via the queue into master with commit 4ee58df Aug 22, 2026
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant