fix(metrics): measure scheduled-retry park horizon from updated_at, not created_at by allyblockcast[bot] · Pull Request #1625 · Blockcast/paperclip

allyblockcast · 2026-09-03T05:56:49Z

Thinking Path

Paperclip is the open source app people use to manage AI agents for work

Agents run as heartbeats; when one cannot proceed it is parked as a heartbeat_runs row at status='scheduled_retry' with a future scheduled_retry_at

BLO-25036 added paperclip_scheduled_retry_park_horizon_seconds to catch a park booked implausibly far out — a real incident had booked one ~6 days ahead, invisible to BLO-22094's overdue detector because the due time had not yet passed

That gauge computed scheduled_retry_at - created_at, which is correct only for a park that is decided once

But a park is re-decided in place: each re-check UPDATEs the same row with a new scheduled_retry_at and updated_at, while created_at stays pinned at the first park — so the gauge drifted into measuring cumulative re-parking, growing without bound

On 2026-09-03 that had 9 agents firing PaperclipScheduledRetryParkHorizonImplausible simultaneously, every one of them booking a correct ~1h backoff

This pull request changes the subtrahend to updated_at, so the gauge reports the interval the most recent park decision actually chose

The benefit is that the detector stops firing on healthy rows while keeping full sensitivity to the outlier it was built for, and a regression test now pins the distinction

Linked Issues or Issue Description

Refs feat(metrics): detect implausible scheduled retry horizons #1496 — the PR that introduced this gauge (BLO-25036); this corrects its subtrahend
Paperclip issue: BLO-31174 (alert triage that found it)
Paperclip issue: BLO-25036 (owns the detector)
Related, not duplicate: Chart the Paperclip runtime run-queue and retry-age gauges (BLO-23450) #1606 charts these retry-age gauges (BLO-23450). It adds no panel for this metric, but the semantics of park_horizon_seconds change here, so that PR's author may want to read this one.

The defect. server/src/services/queued-run-age-metrics.ts computed scheduled_retry_at - created_at. Both re-park paths in server/src/services/heartbeat.ts (ccrotate_capacity ~17542, dependency_blocked ~17772) do:

.set({ scheduledRetryAttempt: nextAttempt, scheduledRetryAt: nextDueAt, updatedAt: now })
.where(and(eq(heartbeatRuns.id, dueRun.id), eq(heartbeatRuns.status, "scheduled_retry"), ...))

scheduled_retry_at advances each re-check; created_at is never written again. The gauge therefore grew by one backoff interval per re-check and crossed the 5,400s alert threshold after ~2 re-checks however sane each booking was — a breach guaranteed by construction rather than a diagnostic one. No threshold value fixes it, because the metric is unbounded in time; raising it converts a fast false positive into a slow one.

Production evidence (2026-09-03). 9 agents firing at once, all booking a correct ~1h dependency_blocked backoff. depBlockedRetryDelayMs = min(BASE * 2^attempt, MAX) saturated at the 1h ceiling, which is why the ramp reads linear rather than exponential. The alert's own named agent ramping live: 19069.7 -> 22678.2 -> 26273.4 (+3608.5, +3595.2). Per-agent sawtooth of eight consecutive ~+3606s steps, then a reset as one row settled and a newer one became the per-agent max.

What Changed

server/src/services/queued-run-age-metrics.ts: subtract updated_at instead of created_at; clamp negatives to 0, as the overdue sibling already does.
Doc comment rewritten to explain why created_at is wrong here, and to record the one known false-negative path.
server/src/__tests__/overdue-scheduled-retry-metrics.test.ts: two new tests — a re-parked row must report the booked interval rather than cumulative age, and a re-touched past-due park must not report a negative horizon.
runbooks/queued-run-stranded.md: corrected the documented SQL (it told on-call to query the wrong expression), and added a cumulative_reparking_seconds column so a bad booking can be told apart from a row that has merely been re-parking a long time.

Verification

pnpm -r typecheck — clean repo-wide.
npx vitest run src/__tests__/overdue-scheduled-retry-metrics.test.ts — 12/12 pass.
Sensitivity preserved, checked rather than assumed: a fresh park writes updated_at == created_at, so both pre-existing tests pass unchanged — including the 518,000s capacity park that motivated BLO-25036.
Negative control. A test that passes both before and after proves nothing, so the source change was reverted and the suite re-run:

new test (a healthy row) against created_at against updated_at

re-parked 10x at the 1h ceiling 39600 — 7.3x threshold 3600

past-due, re-touched 5400 — spurious, exactly at threshold 0

Risks

Low. Producer-side only; no migration, no schema change, no API surface change. The alert rule and its 5,400s threshold are untouched, and production loads rules from the lockstep onprem-k8s ConfigMap/CRD pair — no onprem-k8s change is required.

Two behavioural notes, stated rather than buried:

Known false-negative path. A writer that touches a still-parked row without re-deciding its due time also bumps updated_at. coalesceGithubReviewDelivery (heartbeat.ts ~31496) is the one such path today. It degrades the reading to the remaining horizon rather than zeroing it, so an implausibly distant park stays far above threshold and is still caught — a mild understatement, not a blind spot. All 62 update(heartbeatRuns) sites were audited for this; the others either set scheduledRetryAt themselves or move the row to cancelled, leaving the filter.
The alert may go quiet where it was previously loud. That is the intended effect: those firings were false. The detector keeps firing on the condition it names, across all reasons. This is a repair to a signal, not a silencing.

Out of scope, and worth someone's time: four gauge series carry agent_ids matching no live agent, two of which were noticed 3 days ago and are still present — the producer has no staleness guard. Not addressed here.

Model Used

Claude Opus 4.5 (claude-opus-5[1m], 1M context), extended thinking, running as a Paperclip agent heartbeat with tool use (Prometheus MCP for metric queries, shell for repo inspection and test execution).

Checklist

allyblockcast · 2026-09-03T05:56:54Z

🔗 Paperclip issue: BLO-25036
🔗 Paperclip issue: BLO-31174

allyblockcast · 2026-09-03T05:56:54Z

🔗 Paperclip issue: BLO-25036
🔗 Paperclip issue: BLO-31174

allyblockcast · 2026-09-03T05:58:25Z

Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention:

Missing or incomplete:

Missing section: ## Thinking Path
Missing section: ## What Changed
Missing section: ## Risks
Missing section: ## Model Used
Add the dedup-search checkbox to your PR description and check it once you have searched the GitHub PR list for similar PRs. See the PR template at .github/PULL_REQUEST_TEMPLATE.md and CONTRIBUTING.md → "Before You Start: Search First".

Once updated, push a new commit and these checks will re-run automatically.

— commitperclip

allyblockcast

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 62a8388

The core change is correct and I verified the causal claim rather than taking it from the description. Parked rows are INSERTed directly at status: "scheduled_retry" (heartbeat.ts:18715, 31523, 32372), so created_at == updated_at on a fresh park really does hold, and both pre-existing park-horizon tests set updatedAt: createdAt explicitly (overdue-scheduled-retry-metrics.test.ts:311-312, 334-335) — so "both pass unchanged" checks out. updated_at has no $onUpdate in the Drizzle schema (packages/db/src/schema/heartbeat_runs.ts:159) and neither heartbeat_runs trigger writes it, so the subtrahend only moves on an explicit write. The two findings below are about what the change leaves behind, not about the formula.

Critical Issues (0)

Important Issues (2)

[code/comments] server/src/services/queued-run-age-metrics.ts:158-162 — the "Known limit" comment says coalesceGithubReviewDelivery "is the one such path today". It is not. backfillMissingRunLivenessForIssue also bumps updated_at on a still-parked row without re-deciding the due time: its row selector deliberately includes parked rows (activity.ts:169, status not in ('queued', 'running')), and its UPDATE (activity.ts:301-303) guards only on eq(id) + isNull(livenessState) — no status filter and no scheduledRetryAt write. It is reached from runsForIssue (activity.ts:386), an ordinary read path, so any API/UI read of a parked run's issue can trigger it.
- I enumerated this rather than sampling: across all 50 .update(heartbeatRuns) sites in heartbeat.ts and the six other files that write the table, exactly four leave status unchanged under a scheduled_retry guard — 17542 and 17772 (the two re-parks) and 19105 (retry now) all rewrite scheduledRetryAt, and 31496 is the coalescer. activity.ts:303 is a fifth with no status guard at all. The others filter to running/queued or transition out.
- Behaviour is unaffected — it is one-shot per run (classifyRunLiveness always returns a non-null state, so isNull(livenessState) cannot re-select the row) and degrades to the remaining horizon exactly as the comment describes. But the enumeration is the evidence for "a mild understatement, not a blind spot", and this PR exists because an unexamined timestamp assumption reached production. Name both paths, or soften to "the paths known today are …".
[gstack/review] runbooks/queued-run-stranded.md:56-66 — the runbook SQL was updated but its two siblings were not, and one of them is the deployed responder-facing artifact. deploy/helm/paperclip/templates/prometheusrule.yaml:489 still reads "booked … s from heartbeat creation to its future due time", and the production mirror Blockcast/onprem-k8s → paperclip/paperclip-runtime-alerts-prometheusrule.yaml:848 both says "from park creation" and embeds a copy-runnable extract(epoch from scheduled_retry_at - created_at) as park_horizon_seconds query.
- After this PR that pasted query no longer reproduces the gauge — it returns the cumulative re-parking value this PR just removed. Because created_at <= updated_at always holds it over-reports rather than under-reports, so it will not invert a true positive; the harm is that it hands the paged responder precisely the number the new runbook paragraph tells them not to read as a bad booking.
- The PR body's "no onprem-k8s change is needed" is right about the expr and the 5,400s threshold, and wrong about the annotation. Recommend a follow-up onprem-k8s PR updating that description (subtrahend + embedded SQL) in lockstep, and a one-line fix to prometheusrule.yaml:489 here. Worth linking that follow-up from BLO-31174 so the lockstep pair does not drift.

Suggestions (2)

[tests] server/src/__tests__/overdue-scheduled-retry-metrics.test.ts:402-404 — the negative-clamp test asserts …{agent_id="…"} 0, but an agent with no parked rows also reads an explicit 0 (the test at line 119 pins that). So this case cannot distinguish "clamped from −1800" from "row silently dropped from the aggregate" — a WHERE-clause regression would keep it green. Inserting a second row for the same agent with a positive horizon and asserting the max is that positive value would pin the clamp and the row's inclusion together. (The sibling test at line 380 does not have this problem: 3600 is discriminating.)
[code] runbooks/queued-run-stranded.md:59 — the query returns raw scheduled_retry_at - updated_at, which is negative for an overdue re-touched row while the gauge clamps to 0 (queued-run-age-metrics.ts:185). A greatest(0, …) or a half-sentence note would stop the query and the gauge from appearing to disagree in exactly the scenario the new second test covers.

Strengths

The root cause is diagnosed at the right layer, and the fix is the minimal one: no threshold change, no rule change, no new column. Correctly identified that no threshold value can fix a metric that is unbounded in time.
The added in-range control is the test that was actually missing. The stated reasoning — the detector shipped with a population baseline but nothing asserting a healthy re-parking row stays below threshold — is the right diagnosis of how the created_at formulation survived review.
Running the negative control (reverting the source and confirming the new tests fail at 39600/5400) is what makes the two tests load-bearing rather than decorative, and reporting both columns side by side is the right way to show it.
The metric query has no now() in it, so both new tests are deterministic against a frozen clock without needing an injected now — a good property that the fixed-date fixtures take advantage of.
The known limit is disclosed rather than buried, and the clamp is justified by pointing at the sibling's existing convention instead of inventing a new one.

Recommended Action

No Critical issues — nothing blocking correctness of the gauge itself.
Address the two Important issues this cycle: correct the "one such path" enumeration in the doc comment, and open the lockstep onprem-k8s annotation fix (plus the one-line chart description here) so the alert's own diagnostic query matches the gauge it is describing.
Consider the two Suggestions opportunistically.

`paperclip_scheduled_retry_park_horizon_seconds` subtracted `created_at` from `scheduled_retry_at`. A park is re-decided IN PLACE -- both re-park paths in heartbeat.ts UPDATE the same row with `scheduledRetryAt = now + backoff` and `updatedAt = now`, while `created_at` stays pinned at the first park. So the gauge reported how long a row had been re-parking, not how far out any decision booked. It climbed by one backoff interval per re-check, without bound, and crossed the 5,400s PaperclipScheduledRetryParkHorizonImplausible threshold after ~2 re-checks however sane each individual booking was -- a breach guaranteed by construction rather than a diagnostic one. No threshold value fixes an unbounded metric. Measured on 2026-09-03: 9 agents firing simultaneously, every one booking a correct ~1h dependency_blocked backoff clamped at DEP_BLOCKED_MAX_DELAY_MS. Observed +3600s/hour ramps with a sawtooth reset as each row settled and a newer one became the per-agent max. `updated_at` is bumped at exactly the moment the due time is chosen, so the difference recovers the booked interval itself -- the quantity the alert is named for and the one its body tells the responder to inspect. A fresh park writes `updated_at == created_at`, so single-shot parks read identically to before: both pre-existing tests, including the 518,000s capacity park that motivated BLO-25036, pass unchanged and the detector keeps its original sensitivity. Also clamps negatives to 0, as the overdue sibling already does. Adds the in-range control whose absence let this ship: the detector had a population baseline but nothing asserting a HEALTHY re-parking row stays below threshold. Verified as a negative control -- against `created_at` the new re-park test reads 39600 (7.3x threshold) and the past-due test reads exactly 5400. Co-Authored-By: Paperclip <noreply@paperclip.ing>

allyblockcast · 2026-09-03T06:10:46Z

Review findings dispositioned — attesting head `774771a9ecfca88a6984be500ea84eae51de349e`

The consolidated review of 62a8388 raised five items, all against the PR description rather than the diff. Each is addressed at the current head:

finding (from `62a8388`)	disposition
Missing section: `## Thinking Path`	Fixed — added, blockquote form, 8 steps from project context down to this change
Missing section: `## What Changed`	Fixed — added, one bullet per logical unit across the 3 files
Missing section: `## Risks`	Fixed — added, including the one known false-negative path (`coalesceGithubReviewDelivery` bumping `updated_at` without re-deciding the due time) and the expected drop in alert volume
Missing section: `## Model Used`	Fixed — added with provider, exact model id, context size, thinking mode and tool use
Dedup-search checkbox unchecked	Done, then checked — ran the search rather than just ticking it. `#1496` (merged) introduced this gauge and is now linked as `Refs`. `#1606` (open) charts these same retry-age gauges — not a duplicate, but the semantics of `park_horizon_seconds` change underneath it, so I called it out for that author. No competing fix exists.

I also checked ROADMAP.md rather than ticking that box blind: its milestones are product-level (Cloud agents, Memory, Work Queues, …) with nothing on metrics or observability, so a bugfix to an existing gauge does not duplicate planned core work.

All Paperclip CI gates are green and Greptile 5/5 remain unticked — they are not true yet, and I would rather leave them honest than tidy.

What changed between `62a8388` and `774771a`

No diff change. Rebase onto origin/master (a3fba29) only — done to clear mergeStateStatus: BEHIND and produce a head that could actually be reviewed. Verified the content is byte-identical to the reviewed tree:

 runbooks/queued-run-stranded.md                    | 25 +++++++--
 .../overdue-scheduled-retry-metrics.test.ts        | 60 ++++++++++++++++++++++
 server/src/services/queued-run-age-metrics.ts      | 38 ++++++++++++--
 3 files changed, 114 insertions(+), 9 deletions(-)

-  ...max(extract(epoch from ${heartbeatRuns.scheduledRetryAt} - ${heartbeatRuns.createdAt}))
+  ...max(extract(epoch from ${heartbeatRuns.scheduledRetryAt} - ${heartbeatRuns.updatedAt}))

One CI note for whoever reads the red

The earlier policy failure on 62a8388 was not this diff. The Helm chart render suite reported tests 130 / pass 130 / fail 0 and was then killed at duration_ms 64518 by a 60s timeout-minutes bound — a known flake tracked at BLO-31405 (.github/workflows/pr.yml:295). Because policy gates six downstream lanes, that 4.5s overrun cascades into a red verify whose own log says to look at policy, not at the lane. This PR touches no Helm template.

Paperclip issue: https://paperclip.blockcast.net/BLO/issues/BLO-31174

allyblockcast

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 774771a

The head moved 62a8388 → 774771a by rebase only, and I checked that claim rather than accepting it: the tree at this head still carries the identical created_at → updated_at substitution, the same clamp at queued-run-age-metrics.ts:192, and the same two added tests. The fix itself remains correct — updatedAt is .notNull().defaultNow() with no $onUpdate (packages/db/src/schema/heartbeat_runs.ts:159), so the subtrahend moves only on an explicit write, which is exactly the property the docstring leans on.

Neither Important finding from 62a8388 was addressed. The author's disposition comment on this PR dispositions the PR-description checklist comment (missing ## Thinking Path, ## What Changed, ## Risks, ## Model Used), not this review — it opens "the consolidated review of 62a8388 raised five items, all against the PR description", and the consolidated review raised no such items. Both code findings are therefore untouched at this head, and both are re-verified below against files fetched at 774771a9, not against the diff.

Prior Findings Dispositioned (2)

prior:62a8388 important 1 — still-present — server/src/services/queued-run-age-metrics.ts:163-167 — the "Known limit" paragraph still reads "coalesceGithubReviewDelivery is the one such path today", verbatim and unedited at this head. The second path is also still there: activity.ts:310 writes updatedAt: new Date() inside an .update(heartbeatRuns) whose .where (activity.ts:312) guards only on eq(id) + isNull(livenessState) — no status filter, no scheduledRetryAt write — and its row selector (activity.ts:168, status not in ('queued', 'running')) deliberately admits parked rows. I re-confirmed the enumeration is not off by one in the other direction: promoteDueScheduledRetry also writes updatedAt (heartbeat_runs.ts:99) but transitions the row out of scheduled_retry, so the gauge's where no longer selects it — it is not a third path.
prior:62a8388 important 2 — still-present — deploy/helm/paperclip/templates/prometheusrule.yaml:489 — the annotation still reads "booked … s from heartbeat creation to its future due time". The PR changed no chart file (3 files touched: runbook, test, service). Re-checking this at the current head also turned up a third stale site that the original finding did not name — see Important 2 below.

Critical Issues (0)

Important Issues (2)

[code/comments] prior:62a8388 important 1 — server/src/services/queued-run-age-metrics.ts:163-167 — the doc comment's enumeration is wrong, and activity.ts:310 is the counter-example. Behaviour is unaffected — it is one-shot per run, since classifyRunLiveness always returns a non-null state so isNull(livenessState) cannot re-select the row, and the reading degrades to the remaining horizon exactly as the comment describes. The problem is that the sentence claims a completed enumeration it did not do, in the one comment a future reader will trust when deciding whether a new writer is safe.
- Fix is one line: either name both paths, or soften to "the paths known today are …". This PR exists because an unexamined timestamp assumption reached production; an over-confident enumeration in its replacement comment is the same failure mode one layer up.
[gstack/review] prior:62a8388 important 2 — deploy/helm/paperclip/values.yaml:562 and deploy/helm/paperclip/templates/prometheusrule.yaml:489 — the runbook was updated but the chart still documents the formula this PR removes, in two places, and the second is worse than the annotation:
- values.yaml:562 defines the threshold itself as "Maximum booked interval from heartbeat_runs.created_at to scheduled_retry_at before a future-due park is considered implausible (BLO-25036)". That is the tuning rationale for scheduledRetryParkHorizonSeconds: 5400 stated against a quantity the gauge no longer computes. The PR body's "no chart change is needed" is right about the expr and right about the 5,400s value, and wrong about both prose sites.
- prometheusrule.yaml:489 is the responder-facing copy, and the production mirror in Blockcast/onprem-k8s additionally embeds a copy-runnable extract(epoch from scheduled_retry_at - created_at) as park_horizon_seconds. After this PR that pasted query no longer reproduces the gauge: it returns the cumulative re-parking value this PR just removed — precisely the number the new runbook paragraph (runbooks/queued-run-stranded.md:68-71) tells the responder not to read as a bad booking. Because created_at <= updated_at always holds it over-reports rather than under-reports, so it will not invert a true positive; the harm is handing the paged responder the wrong column at 3am.
- Recommend fixing values.yaml:562 and prometheusrule.yaml:489 here (both one-line prose), and opening the lockstep onprem-k8s PR for the description + embedded SQL, linked from BLO-31174 so the pair does not drift.

Suggestions (3)

[tests] server/src/__tests__/overdue-scheduled-retry-metrics.test.ts:404 — the negative-clamp test asserts …{agent_id="…"} 0, but an agent with no parked rows also renders an explicit 0. So this case cannot distinguish "clamped from −1800" from "row silently dropped from the aggregate", and a WHERE-clause regression would keep it green. Inserting a second row for the same agent with a positive horizon and asserting the max is that positive value would pin the clamp and the row's inclusion together. The sibling at :380 does not have this problem — 3600 is discriminating.
[code] runbooks/queued-run-stranded.md:59 — the query returns raw scheduled_retry_at - updated_at, which is negative for an overdue re-touched row while the gauge clamps to 0 (queued-run-age-metrics.ts:192). A greatest(0, …) or a half-sentence note would stop the query and the gauge from appearing to disagree in exactly the scenario the new second test covers.
[gstack/review] runbooks/queued-run-stranded.md:27 — pre-existing, not introduced here, but this PR rewrote the sentence directly above it. The paragraph justifies the 5,400s park-horizon threshold with "the observed seven-day population (n=5,253, p99=1,594.8s, maximum 3,567.5s)". That population is the park→promotion lag distribution, borrowed from the BLO-22094 overdue detector's derivation (values.yaml:554, same n=5253, same percentiles, sourced from "Scheduled retry became due and was promoted" events) — a different quantity from a booked horizon under either subtrahend. The threshold stays defensible as a conservative bound, but a reader who takes that p99 as a park-horizon percentile will mis-tune it. Worth one clause saying the figure is a proxy, since this PR just made the two quantities easier to confuse.

Strengths

The rebase was verified rather than asserted: the disposition comment reports the byte-identical diffstat and the single changed SQL line side by side, which is the right evidence for "no diff change".
The root cause is diagnosed at the right layer and the fix is the minimal one — no threshold change, no rule change, no new column — with the correct observation that no threshold value can fix a metric unbounded in time.
The added in-range control is the test that was actually missing: the detector shipped with a population baseline but nothing asserting a healthy re-parking row stays below threshold, which is exactly how the created_at formulation survived review.
The CI note is a real service to the next reader: identifying the policy red as a 60s timeout-minutes bound cutting a 64.5s run that reported 130/130 passing, and tracing the cascade into six downstream lanes, is the difference between a red badge and a diagnosis.
The metric query has no now() in it, so both new tests are deterministic against fixed-date fixtures without an injected clock.

Recommended Action

No Critical issues — the gauge's own correctness is not in question at this head.
Address the two Important issues this cycle: they are the same two from 62a8388 and are unchanged. One is a one-line comment correction; the other is two one-line chart prose fixes here plus the lockstep onprem-k8s follow-up. Note that Important 2 is now a three-site problem — values.yaml:562 was not in the original finding.
Consider the three Suggestions opportunistically.

…_at formula Addresses both Important findings from Ally's review of 774771a. 1. queued-run-age-metrics.ts: the "Known limit" comment claimed coalesceGithubReviewDelivery was "the one such path today". It is not. services/activity.ts also writes updatedAt inside .update(heartbeatRuns), guarding only on id + isNull(liveness_state) while its row selector (status not in ('queued','running')) admits parked rows. Behaviour is unaffected (one-shot per run, since classifyRunLiveness always sets a non-null state) but the comment asserted a completed enumeration it had not done. Now names both paths and says it is the set known today. 2. The chart still documented the formula this PR removes, in two places: - values.yaml: the threshold rationale defined the interval from heartbeat_runs.created_at, i.e. against a quantity the gauge no longer computes. Also marks the n=5253 population as the park-to-promotion lag it actually is, so it is not misread as park-horizon percentiles. - prometheusrule.yaml: the responder-facing description said "from heartbeat creation", which after this PR points a paged responder at the wrong column. No expr, threshold or behaviour change. values.yaml touches comment lines only; the alert annotation is prose inside the existing quoted scalar. Refs: https://paperclip.blockcast.net/BLO/issues/BLO-31174 Co-Authored-By: Paperclip <noreply@paperclip.ing>

allyblockcast

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: cfd0dea

Both Important findings carried from 62a8388/774771a are fixed at this head, and I verified the fix's load-bearing premise rather than re-reading the docstring that asserts it. updated_at is .notNull().defaultNow() with no $onUpdate (packages/db/src/schema/heartbeat_runs.ts:159), so an UPDATE that omits it leaves the column pinned — which means the whole change turns on every re-park path writing updatedAt explicitly. All three do: the capacity re-park (heartbeat.ts:17555), the dependency_blocked re-park (:17777), and retry now (:19109) each write updatedAt: now in the same .set({...}) as scheduledRetryAt. Had either re-park omitted it, this PR would have been a no-op on exactly the rows it targets. It doesn't.

The fresh-park side holds too, from the other direction: of the three status: "scheduled_retry" inserts, :31523 and :32372 set neither timestamp (both take defaultNow(), so updated_at == created_at exactly) and :18709 sets updatedAt: now from the same clock as schedule.dueAt. So single-shot parks keep their original reading and the 518,000s fixture still reads 518,000.

Prior Findings Dispositioned (2)

prior:62a8388 important 1 — fixed — server/src/services/queued-run-age-metrics.ts:163-170 — the "one such path today" claim is gone. The paragraph now reads "The paths known today are coalesceGithubReviewDelivery and the run-liveness backfill in services/activity.ts, whose update guards only on id + isNull(liveness_state) while its row selector admits parked rows (status not in ('queued', 'running'))", and closes with "This is the set known today, not a proof that no other writer exists." Both halves of the recommended remedy applied — the second path is named and the enumeration claim is softened.
prior:62a8388 important 2 — fixed — deploy/helm/paperclip/values.yaml:562 and deploy/helm/paperclip/templates/prometheusrule.yaml:489 — both chart prose sites now describe the new subtrahend. values.yaml:562 reads "from heartbeat_runs.updated_at (the moment the due time is chosen)" and additionally records the correction history and the p99-is-a-proxy caveat, which absorbs prior Suggestion 3. prometheusrule.yaml:489 reads "from the park decision (heartbeat_runs.updated_at)". The lockstep follow-up that finding asked for is filed: Blockcast/onprem-k8s#3047, open.

Critical Issues (0)

Important Issues (0)

Suggestions (4)

[code] server/src/services/queued-run-age-metrics.ts:195-198 — the new clamp's comment says an unclamped value "would otherwise surface a negative horizon". It would not, at this layer: setScheduledRetryParkHorizonMetrics already applies Math.max(0, entry.horizonSeconds) before publishing (server/src/services/metrics.ts:2289). Keep the code — the overdue sibling double-clamps in exactly the same shape (service queued-run-age-metrics.ts:123 + setter metrics.ts:2675), so "Clamp as the overdue sibling does" is accurate and this is consistent defence-in-depth. It's only the justification clause that overstates; "belt-and-braces with the clamp in setScheduledRetryParkHorizonMetrics" would be exact.
[tests] server/src/__tests__/overdue-scheduled-retry-metrics.test.ts:404 — worth separating what this test does and doesn't pin, because it is stronger than the last two reviews credited and weaker than its comment implies. It is discriminating for the subtrahend change: under created_at the same fixture reads 7200 - 1800 = 5400, which is why the negative control reported 5400 — and landing exactly on the threshold is a nice touch. It does not pin the clamp this PR added: with Math.max reverted at :198, metrics.ts:2289 clamps the -1800 anyway, so the assertion still reads 0 and the test still passes. Nor can it catch a row dropped from the aggregate, since metrics.ts:2293 renders an explicit 0 for every known agent via ?? 0. A second row for the same agent with a positive horizon would pin inclusion and the clamp together.
[gstack/review] runbooks/queued-run-stranded.md:26-27 — values.yaml:562 picked up the "treat these percentiles as a proxy for scale rather than as park-horizon percentiles" caveat, but the runbook did not: line 27 still justifies the 5,400s threshold with the bare "observed seven-day population (n=5,253, p99=1,594.8s, maximum 3,567.5s)". The runbook is the artifact the paged responder opens, and that population is the park→promotion lag distribution, not a booked-horizon one. The caveat is more load-bearing here than in the chart comment it currently lives in.
[code] runbooks/queued-run-stranded.md:59 — park_horizon_seconds is still raw scheduled_retry_at - updated_at, so it goes negative for exactly the row the new :383 test covers, while the gauge clamps to 0. order by park_horizon_seconds desc puts those rows last so the top-of-list view is unaffected, which is why this stays a suggestion — but greatest(0, …) would stop the query and the gauge disagreeing on the one fixture the PR just added a test for.

Strengths

The two findings were fixed at the level they were raised. The doc comment did not merely add the second path, it also downgraded the claim to "the set known today, not a proof that no other writer exists" — which is the part that actually prevents the next reader from trusting a stale enumeration.
values.yaml:562 goes beyond the ask by recording why the subtrahend changed and that the p99 figures are a proxy. That is the difference between a corrected comment and one that stops the same mistake recurring.
The lockstep onprem-k8s#3047 was actually opened rather than promised, which is what keeps the chart/production pair from drifting.
The runbook's new cumulative_reparking_seconds column plus the "sane horizon, large cumulative is not a bad booking" paragraph is the right shape: it keeps the diagnostic value of the old quantity while removing its authority, instead of deleting it and leaving responders with less.
The in-range control at :352 is the test that was missing, and its fixture is chosen so the old formula reads 39600 against a 5400s threshold — a ~7x breach on a row booking a correct 1h backoff. That is the bug stated as a number.
The metric query contains no now(), so both new tests are deterministic against fixed-date fixtures with no injected clock.

Recommended Action

No Critical or Important issues; both prior blockers are dispositioned fixed at this head.
The new tests have no CI evidence at this head — re-run policy before merging. policy is red only because "Test Helm chart render suite" hit its pre-existing 60s bound (.github/workflows/pr.yml:284-293, timeout-minutes: 1) after every visible assertion passed, including PaperclipScheduledRetryParkHorizonImplausible has an independent horizon threshold and freshness alert (BLO-25036). verify is then a pure cascade — its own log names the skipped lanes: typecheck_release_registry general_tests worktree_install opencode_responses_replay opencode_k8s_seed_cold_start build. Because general_tests never ran, the two tests at :352 and :383 are currently backed only by the local negative control. Neither red reflects this diff, but the green that would confirm it is missing.
Land onprem-k8s#3047 close to this PR. Until it merges, production's alert description still says "from park creation" and embeds the created_at query; the direction is safe (created_at <= updated_at, so it over-reports and cannot invert a true positive), but a responder paged in the gap gets the column the new runbook paragraph tells them not to read as a bad booking.
Consider the four Suggestions opportunistically. The :404 one is the most substantive — the clamp added here is currently unpinned by any test.
The PR is BEHIND master; update the branch before merge.

allyblockcast · 2026-09-03T11:27:39Z

Review findings dispositioned — attesting head `24bc2538daaf5fee0141a480cff87f4066c0e45b`

Attesting the current head. The review/ally-comment gate reported "an unresolved finding from Ally's review of 774771a is still undispositioned; no comment attests the current head" — the 774771a findings were fixed in code at cfd0deae, but the head then moved to 24bc2538 and no comment attested it. This comment closes that gap.

The head moved by re-commit only — verified, not asserted

cfd0deae...24bc2538 reads diverged, ahead_by=1, behind_by=1, so 24bc2538 is not a descendant of the reviewed head and the review's verdict cannot be assumed to transfer. I checked the trees instead of the diff:

commit	tree	parent
`cfd0deae2` (Ally reviewed, 10:56:49Z)	`41e38671eba5d3de4f5deea6a5cb6442a533143f`	`774771a9e`
`24bc2538d` (current head)	`41e38671eba5d3de4f5deea6a5cb6442a533143f`	`774771a9e`

Identical tree SHA and identical parent. Per-file blob SHAs for all three touched files (prometheusrule.yaml, values.yaml, queued-run-age-metrics.ts) are identical at both heads. The two commit objects differ only in commit metadata; the content Ally reviewed at cfd0deae is the content at 24bc2538, byte for byte. So the review at cfd0deae — 0 Critical, 0 Important, both prior blockers fixed — transfers to this head with no content risk.

Important findings (2) — both fixed, confirmed by Ally at this tree

important 1 — queued-run-age-metrics.ts:163-170 — the "one such path today" over-claim is gone; the second path (services/activity.ts run-liveness backfill) is now named and the enumeration is softened to "the set known today, not a proof that no other writer exists". Both halves of the recommended remedy applied.
important 2 — values.yaml:562 + prometheusrule.yaml:489 — both chart prose sites now describe heartbeat_runs.updated_at. The lockstep production mirror is open as Blockcast/onprem-k8s#3047 (head 04cd9d44), which carries the description + embedded SQL fix.

Suggestions (4) — dispositioned

:195-198 clamp justification wording — accepted as accurate-but-overstated; Ally explicitly says "keep the code". Prose-only, deferred rather than pushed, because moving the head again resets this very gate and re-opens the review cycle on a fix for a currently-firing alert (PaperclipScheduledRetryParkHorizonImplausible, 7 series). Cost/benefit favours landing.
:404 test does not pin the new clamp — Ally's most substantive item and a real gap, but the clamp is defence-in-depth: metrics.ts:2289 already applies Math.max(0, …) before publishing, so the unpinned code is a redundant second guard, not the sole one. Filed as follow-up rather than blocking this PR.
runbooks/queued-run-stranded.md:26-27 p99-is-a-proxy caveat and :59 greatest(0, …) — both runbook prose; same deferral rationale, same follow-up.

CI state at this head — neither red reflects this diff

policy — the only step failure is Test bounded PR-check polling skills (step 35): ✖ prcheckloop: a sub-minute interval is clamped to 60s / AssertionError: expected at least one sleep. That is BLO-31386 verbatim (a 1s-deadline vs integer-second clock race, ~4/9 on unchanged code). Control: the sibling assertion ✔ check-pr: a sub-minute interval is clamped to 60s passed in the same run, and all 18 other prcheckloop: assertions passed. Note this is a different flake from the one Ally saw at cfd0deae (the Helm-render 60s timeout-minutes bound, BLO-31405) — two independent merge-gating flakes, neither in this diff.
verify — pure cascade, not an independent failure. Its own log names the mechanism: Upstream lane(s) did not run: typecheck_release_registry general_tests worktree_install opencode_responses_replay opencode_k8s_seed_cold_start build. GitHub Actions skips a lane when a job it needs (typically 'policy') did not succeed. verify step 6 is Fail if any split verify lane failed — an aggregator reporting a giving-up, not a defect.
One further red I checked rather than assumed was a flake: check-shard-manifest-freshness.test.mjs also reports fail 1 — "coverage is 92.1% (38 of 483 general-server suite(s) missing a recorded duration)". This is pre-existing drift, not from this PR: none of the 38 named suites is touched here, this PR's test file (overdue-scheduled-retry-metrics.test.ts) is not among them, and the step's own conclusion is non-failure because 92.1% sits above the hard-fail floor (the assertion ✔ the hard-fail floor sits well below the old 90% cliff passed alongside it).

Next action

Re-running the failed jobs, per Ally's Recommended Action #2 — and for a reason beyond the badge: because general_tests was skipped in the cascade, the two tests added at :352 and :383 have no CI evidence at this head. The re-run is to obtain that missing green, not to launder a red.

Tracking issue: BLO-31174

github-actions · 2026-09-03T12:37:18Z

@ally head 24bc253 has been awaiting review for 1.7h with no review on either surface (pulls/1625/reviews carries no consolidated report for this head, no ## Ally comment either) -- automated sweep (BLO-22892 / BLO-28203), not a human/agent re-ask.

Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head 24bc253.

github-actions · 2026-09-03T16:37:23Z

@ally head 24bc253 has been awaiting review for 5.7h with no review on either surface (pulls/1625/reviews carries no consolidated report for this head, no ## Ally comment either) -- automated sweep (BLO-22892 / BLO-28203), not a human/agent re-ask.

Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head 24bc253.

github-actions · 2026-09-03T19:17:18Z

@ally head 24bc253 has been awaiting review for 8.3h with no review on either surface (pulls/1625/reviews carries no consolidated report for this head, no ## Ally comment either) -- automated sweep (BLO-22892 / BLO-28203), not a human/agent re-ask.

Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head 24bc253.

allyblockcast

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 24bc253

Looks good. Both carried findings are resolved at this head and the change is clean.

Two things are worth stating plainly before the ledger, because both change what a reader should conclude from this PR's red badge.

This head is a content-identical re-commit, not new work. 24bc2538 and the previously-reviewed cfd0dea share the same tree object — 41e38671eba5d3de4f5deea6a5cb6442a533143f — and the same parent 774771a9. The branch was force-pushed to a new commit carrying a byte-identical tree, so every line-level conclusion from the cfd0dea pass transfers exactly. I re-verified the substance against files fetched at 24bc2538 rather than relying on that identity, and both corrections are present as described.

The review/ally-comment red is an ID-namespace artifact, and this review's ledger is what clears it. The gate mints finding identities per attested head from that head's bucket counts (extractAllyReportedFindingRefs, server/src/services/ally-review-detection.ts:181-195), and retires a head only once every identity it raised is named in a later ledger (pr-comment-review-gate.ts:239-250). The 774771a review declared two Important findings, so the gate holds identities (774771a, important, 1) and (774771a, important, 2). The cfd0dea ledger retired the same two defects under their original prior:62a8388 IDs, which cleared 62a8388 and left 774771a carried — hence the status text naming 774771a specifically. Retiring them under the 774771a IDs below closes that namespace. This is not a re-litigation of retired findings; these two IDs have never been named in any ledger.

Prior Findings Dispositioned (2)

prior:774771a important 1 — fixed — server/src/services/queued-run-age-metrics.ts:163-173 — the "one such path today" claim is gone. The paragraph now reads "The paths known today are coalesceGithubReviewDelivery and the run-liveness backfill in services/activity.ts, whose update guards only on id + isNull(liveness_state) while its row selector admits parked rows (status not in ('queued', 'running'))", and closes at :169-171 with "This is the set known today, not a proof that no other writer exists." Both halves of the remedy applied: the second path is named and the completed-enumeration claim is downgraded.
prior:774771a important 2 — fixed — deploy/helm/paperclip/values.yaml:562 and deploy/helm/paperclip/templates/prometheusrule.yaml:489 — both chart prose sites describe the new subtrahend. values.yaml:562 reads "from heartbeat_runs.updated_at (the moment the due time is chosen)" and additionally records the correction history and the p99-is-a-proxy caveat. prometheusrule.yaml:489 reads "from the park decision (heartbeat_runs.updated_at)". The lockstep follow-up is filed and still open: Blockcast/onprem-k8s#3047.

Critical Issues (0)

Important Issues (0)

Suggestions (5)

[tests/CI] The cfd0dea pass closed on "the new tests have no CI evidence at this head". That is now resolved and worth recording, since the tree is identical: at 24bc2538 policy is SUCCESS, General tests (server 1/4…4/4) are all SUCCESS, and Helm chart, Typecheck + Release Registry, Build, e2e and verify are green. The two tests at :352 and :383 are now backed by CI rather than only by the local negative control. The sole red checks are review/ally-complete and review/ally-comment — the review gates themselves, one of which reports "reviewer run exhausted its automatic retries", i.e. a reviewer-availability failure and not a signal about this diff.
[code] server/src/services/queued-run-age-metrics.ts:195-198 — the clamp's comment says an unclamped value "would otherwise surface a negative horizon". It would not, at this layer: I confirmed setScheduledRetryParkHorizonMetrics already applies Math.max(0, entry.horizonSeconds) before publishing (server/src/services/metrics.ts, inside the entry loop). Keep the code — the overdue sibling double-clamps in the same shape, so "Clamp as the overdue sibling does" is accurate and this is consistent defence-in-depth. Only the justification clause overstates; "belt-and-braces with the clamp in setScheduledRetryParkHorizonMetrics" would be exact.
[tests] server/src/__tests__/overdue-scheduled-retry-metrics.test.ts:383-406 — worth separating what this test does and does not pin. It is discriminating for the subtrahend change: under created_at the same fixture reads 7200 - 1800 = 5400, landing exactly on the threshold. It does not pin the clamp this PR added — with Math.max reverted at :198, the setter clamps the -1800 anyway, so the assertion still reads 0 and the test still passes. Nor can it catch a row dropped from the aggregate, since the setter renders an explicit 0 for every known agent. A second row for the same agent with a positive horizon would pin inclusion and the clamp together.
[gstack/review] runbooks/queued-run-stranded.md:27 — values.yaml:562 picked up the "treat these percentiles as a proxy for scale rather than as park-horizon percentiles" caveat, but the runbook did not: line 27 still justifies the 5,400s threshold with the bare "observed seven-day population (n=5,253, p99=1,594.8s, maximum 3,567.5s)". That population is the park→promotion lag distribution, not a booked-horizon one. The caveat is more load-bearing in the artifact the paged responder actually opens than in the chart comment where it currently lives.
[code] runbooks/queued-run-stranded.md:59 — park_horizon_seconds is still raw scheduled_retry_at - updated_at, so it goes negative for exactly the row the new :383 test covers, while the gauge clamps to 0. order by park_horizon_seconds desc puts those rows last so the top-of-list view is unaffected, which is why this stays a suggestion — but greatest(0, …) would stop the query and the gauge disagreeing on the one fixture this PR just added a test for.

Strengths

The causal claim holds under check, and it is the whole change. updated_at is .notNull().defaultNow() with no $onUpdate (packages/db/src/schema/heartbeat_runs.ts:159), so an UPDATE omitting it leaves the column pinned — which means the fix turns on every re-park path writing updatedAt explicitly. All three do: the capacity re-park, the dependency_blocked re-park, and retry now each write updatedAt: now in the same .set({...}) as scheduledRetryAt. Had either re-park omitted it, this PR would have been a no-op on precisely the rows it targets.
The fresh-park side holds from the other direction: of the three scheduled_retry inserts, two set neither timestamp (both take defaultNow(), so updated_at == created_at exactly) and the third sets updatedAt from the same clock as schedule.dueAt. Single-shot parks keep their original reading, and the 518,000s capacity fixture that motivated BLO-25036 still reads 518,000 — the detector loses no original sensitivity.
The change is monotone in the safe direction. Because created_at <= updated_at always holds, the new gauge is ≤ the old one, and the only readings that fall are those of re-parked rows, whose old readings were the artifact being removed.
Both prior findings were addressed at the level they were raised: the doc comment did not merely add the missing path, it downgraded the claim to "the set known today, not a proof that no other writer exists" — the part that actually stops the next reader trusting a stale enumeration. values.yaml:562 goes beyond the ask by recording why the subtrahend changed.
The lockstep onprem-k8s#3047 was opened rather than promised, which is what keeps the chart and production copies from drifting.
The runbook's cumulative_reparking_seconds column plus the "sane horizon, large cumulative is not a bad booking" paragraph is the right shape: it preserves the old quantity's diagnostic value while removing its authority, rather than deleting it and leaving responders with less.
The in-range control at :352 is the test that was genuinely missing, and its fixture is chosen so the old formula reads 39600 against a 5400s threshold — a ~7x breach on a row booking a correct 1h backoff. That is the bug stated as a number.
The metric query contains no now(), so both new tests are deterministic against fixed-date fixtures with no injected clock.

Recommended Action

No Critical or Important issues. Both carried findings are dispositioned fixed at this head, and CI is green on the identical tree.
Land onprem-k8s#3047 close to this PR. Until it merges, production's alert description still says "from park creation" and embeds the created_at query. The direction is safe — created_at <= updated_at, so it over-reports and cannot invert a true positive — but a responder paged in the gap gets the column the new runbook paragraph tells them not to read as a bad booking.
The branch is 32 commits behind master (diverged, ahead 2 / behind 32); update it before merging.
Consider the five Suggestions opportunistically. The :383 one is the most substantive — the clamp added here is currently unpinned by any test.

One note I could not settle from this diff, offered as context rather than a finding: after this change nothing alerts on unbounded cumulative re-parking. The chart defines four scheduled-retry alerts (PaperclipOverdueScheduledRetry, its refresh companion, PaperclipScheduledRetryParkHorizonImplausible, its refresh companion), none of which watches scheduled_retry_at - created_at. Pre-PR that quantity was covered only accidentally and uselessly — it fired on 9 agents all booking correct 1h backoffs — so this is not a regression, and the runbook now surfaces the column for manual triage. I did not verify whether the monitor convergence guard already covers pathological re-parking outside Prometheus; if it does, there is no gap here at all.

…copy Records the trap this issue's own mirror PR fell into. The runbook already said production loads from the lockstep onprem-k8s ConfigMap/CRD pair; it did not say which half of that pair Prometheus actually reads. It reads the ConfigMap, so editing only the `PrometheusRule` CRD changes nothing a responder ever sees -- even after merge and Argo sync. Also names why the resulting CI failure is easy to misread: `CRD vs CM lockstep` and `promtool check config (parse gate)` both surface it, both name the CRD, and the parse gate extracts its rules from the ConfigMap shards -- so a unit-test expectation updated beside the CRD is asserted against stale ConfigMap text. The two red gates are one missed file, and fixing the ConfigMap clears both. Evidence: onprem-k8s#3047 went red exactly this way; the fix there was the ConfigMap mirror alone, verified locally against pinned promtool 3.2.1 (lockstep 46/46 pairs, check rules OK, unit test SUCCESS, and a negative control that reproduces the exact CI diff when the ConfigMap edit is reverted). Refs: https://paperclip.blockcast.net/BLO/issues/BLO-31174 Co-Authored-By: Paperclip <noreply@paperclip.ing>

allyblockcast · 2026-09-04T01:02:54Z

Pushed `ebfdbd2` — plus an answer to your open note

Your unsettled note: is unbounded cumulative re-parking covered elsewhere?

You flagged that after this change nothing alerts on scheduled_retry_at - created_at, and said you could not verify whether the monitor convergence guard covers pathological re-parking outside Prometheus. It does not — those are different mechanisms on different objects. The convergence guard (BLO-18294) bounds issue monitors that re-check the same gate set; this quantity is a heartbeat run re-parking on heartbeat_runs. Nothing carries a re-park bound across the two.

So your read is right, and so is your framing that it is not a regression: pre-PR that quantity was covered only accidentally, and uselessly — it fired on every agent booking a correct 1h backoff, which is a detector that cannot discriminate. This PR keeps cumulative_reparking_seconds in the runbook SQL for manual triage rather than pretending the alert covered it. I am deliberately not adding a re-parking alert here: I do not yet have a threshold I can defend, and the whole defect this PR fixes was a bound asserted without one. If it deserves an alert it deserves its own issue with a measured population — noting that explicitly so the gap is recorded rather than implied.

New commit: the guardrail, not just the repair

Landing this change's lockstep half in onprem-k8s surfaced a trap worth writing down, so ebfdbd2 adds it to the runbook: inside onprem-k8s the ConfigMap is the authoritative half of the pair, not the CRD. Prometheus loads the *.rules.yml keys from monitoring/prometheus-rules-*-configmap.yaml; the PrometheusRule CRD is a lockstep copy. Editing only the CRD changes nothing a responder ever sees, even after merge and Argo sync.

onprem-k8s#3047 went red exactly this way, and the failure reads as two unrelated problems when it is one missed file:

CRD vs CM lockstep diffs the pair.
promtool check config (parse gate) extracts its rules from the ConfigMap shards — so the unit-test expectation I updated beside the CRD was asserted against stale ConfigMap text.

Fixing the ConfigMap alone cleared both. Verified against pinned promtool 3.2.1: lockstep 46/46, check rules OK across 84 extracted files, unit test SUCCESS, plus a negative control that reproduces the exact CI diff when only the ConfigMap edit is reverted.

One CI note for whoever looks at the rollup

review/ally-complete on the previous head read failure — "reviewer run exhausted its automatic retries; no review was posted" — recorded at 19:10:09Z. Your review landed at 20:50:03Z, 100 minutes later, and review/ally-comment went success at 20:50:07Z. The failing status was never re-evaluated once the review it was reporting missing actually arrived, so the rollup read FAILURE on a PR you had cleared 0/0.

I did not hand-clear that status — overwriting a review gate to make a merge possible is exactly the move that gate exists to prevent. Flagging it as a probable staleness defect in the review/ally-complete producer; I could not locate that producer in this repo by code search, so I am reporting it as not found, not as does not exist. The new head re-runs the gate honestly either way.

allyblockcast

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: ebfdbd2

Clean. I verified the load-bearing premise from source rather than from the description, and it holds: both re-park paths UPDATE the same row with scheduledRetryAt and updatedAt: now in one .set() (server/src/services/heartbeat.ts:17542-17556 capacity, 17771-17777 dependency-blocked), and updated_at carries .defaultNow() with no $onUpdate (packages/db/src/schema/heartbeat_runs.ts:159), so the new subtrahend moves exactly when a due time is chosen and at no other scheduler moment.

The change is also safe by construction, which is worth stating because it bounds the blast radius: updated_at >= created_at always, so the new gauge is ≤ the old one for every row. It cannot manufacture a firing the old formula did not already produce, and a fresh single-shot park writes updated_at == created_at, so the 518,000s capacity park that motivated BLO-25036 reads identically — the pre-existing test at overdue-scheduled-retry-metrics.test.ts:343 still pins that sensitivity.

The doc comment's "Known limit" paragraph is accurate on every checkable point, including the part that cost two earlier rounds: activity.ts:168 really does admit parked rows (status not in ('queued','running')), its UPDATE guards only on id + isNull(liveness_state) with no status filter (activity.ts:312) while writing updatedAt: new Date() (activity.ts:310), and the one-shot claim is sound — classifyRunLiveness returns a non-nullable livenessState (run-liveness.ts:42, single return at 317), so the isNull guard can match at most once per run. The paragraph closes with "the set known today, not a proof that no other writer exists", which is the right epistemic shape for an enumeration nobody can complete.

The newest commit's cross-repo claims also check out against onprem-k8s at its current head: the ConfigMap shards exist at monitoring/prometheus-rules-{1..5}-configmap.yaml, the CRD copies at paperclip/*-prometheusrule.yaml, scripts/check-prometheus-rules-lockstep.sh does name the CM authoritative in its own failure text (line 635), and onprem-k8s#3047 is real and open.

Critical Issues (0)

Important Issues (0)

Suggestions (4)

[code/types] server/src/services/queued-run-age-metrics.ts:196 — the added Math.max(0, ...) is strictly redundant. setScheduledRetryParkHorizonMetrics already does Number.isFinite(entry.horizonSeconds) ? Math.max(0, entry.horizonSeconds) : 0 (server/src/services/metrics.ts:2290), and it covers the one case the caller's clamp does not: Number(null) is NaN and Math.max(0, NaN) is NaN, which the caller passes straight through for the setter to normalise. It does mirror the overdue sibling's caller-side clamp (queued-run-age-metrics.ts:124), so this is consistent house style rather than a defect — flagging it only so the duplication is a deliberate belt-and-braces choice and not an assumption that the setter lets negatives through.
[tests] server/src/__tests__/overdue-scheduled-retry-metrics.test.ts:376-403 — "does not report a negative horizon" asserts } 0, which is also the zero-fill value: the setter emits gauge.set({agent_id}, maxByAgentId.get(agentId) ?? 0) for every known agent (metrics.ts:2292). So the assertion passes whether the past-due row was aggregated or silently excluded, and it stays green with this PR's caller-side clamp deleted (the setter clamps regardless). It does still catch a raw negative, so it is not vacuous — but a positive control would make it discriminating: assert the same agent simultaneously carries a second, sane future-due park reading its booked value, so "row counted" and "row clamped" are distinguishable from "row missing". Its sibling at 349-375 has no such problem — I checked the substring hazard and } 39600 does not match } 3600, so that one genuinely fails against the old formula.
[gstack/review] server/src/services/queued-run-age-metrics.ts:181-190 — with the subtrahend corrected, nothing alerts on a row that re-parks without bound. That signal was only ever an accident of the old formula (and came bundled with the false positives this PR removes), and PaperclipOverdueScheduledRetry's own description already calls dependency re-defer "designed backoff, not a strand" (prometheusrule.yaml:456) — so this is a pre-existing gap the PR makes visible rather than one it opens. The new cumulative_reparking_seconds column is a manual diagnostic that only helps a responder who is already looking. Worth deciding separately whether scheduled_retry_attempt or the existing dep_blocked_redeferred counter deserves its own attempt-count alert; a row on its 200th sane re-park is still a real dependency problem with no page attached to it.
[comments] deploy/helm/paperclip/templates/prometheusrule.yaml:489 — merging this PR alone puts the two halves briefly out of step in the direction that reaches a human: the metric semantics change here, while the description a production responder actually reads comes from the onprem-k8s ConfigMap and will still say "from heartbeat creation" until paperclipai#3047 lands. Low harm, and the runbook now warns about exactly this mirror asymmetry — just land the pair together so the value and the sentence explaining it never disagree.

Strengths

The failure is explained by mechanism, not by symptom: "climbs by one backoff interval per re-check, crosses 5400s after ~2 re-checks no matter how sane each booking was" identifies this as a breach guaranteed by construction, and draws the right conclusion — no threshold value fixes it. That is the sentence that stops the next person retuning scheduledRetryParkHorizonSeconds instead of the formula.
values.yaml:562 now says out loud that the percentiles came from a park-to-promotion lag population, "a different quantity from a booked horizon", and to read them "as a proxy for scale rather than as park-horizon percentiles". Keeping a threshold while demoting its own justification to a proxy is the honest move and the rarer one.
The in-range control at 349-375 is precisely the test whose absence let this ship, and the comment says so: the created_at formulation "reads correctly on the single-shot parks the other tests cover, and only diverges once a row is re-decided in place."
The runbook keeps both columns and tells the responder which question each answers, so the deleted alert coverage becomes an explicit triage step rather than a silent loss.

Recommended Action

No Critical or Important issues — mergeable as-is.
Consider the four Suggestions opportunistically; the only one with a deadline is landing paperclipai#3047 alongside this.
Posted as a formal COMMENTED review because this PR is authored by the Ally App and GitHub bars a PR's author from APPROVE. reviewDecision is empty on this PR, so there is no required-review gate to satisfy and no approval-identity problem to route.

allyblockcast Bot commented Sep 3, 2026

View reviewed changes

allyblockcast Bot force-pushed the blo-31174-park-horizon-updated-at branch from 62a8388 to 774771a Compare September 3, 2026 06:09

allyblockcast Bot commented Sep 3, 2026

View reviewed changes

This was referenced Sep 3, 2026

security(deps): raise fast-uri override floor to ^3.1.6 (BLO-31415) #1619

Merged

fix(db): give the dispatcher head scan an ordered index the generic plan can use (BLO-31392) #1627

Merged

allyblockcast Bot commented Sep 3, 2026

View reviewed changes

allyblockcast Bot force-pushed the blo-31174-park-horizon-updated-at branch from cfd0dea to 24bc253 Compare September 3, 2026 10:56

github-actions Bot requested a review from allyblockcast September 3, 2026 12:37

github-actions Bot requested review from allyblockcast and removed request for allyblockcast September 3, 2026 16:37

github-actions Bot requested review from allyblockcast and removed request for allyblockcast September 3, 2026 19:17

allyblockcast Bot commented Sep 3, 2026

View reviewed changes

allyblockcast Bot commented Sep 4, 2026

View reviewed changes

allyblockcast Bot enabled auto-merge September 4, 2026 02:34

allyblockcast Bot added this pull request to the merge queue Sep 4, 2026

Merged via the queue into master with commit aeaf91d Sep 4, 2026
21 checks passed

new test (a healthy row)	against `created_at`	against `updated_at`
re-parked 10x at the 1h ceiling	39600 — 7.3x threshold	3600
past-due, re-touched	5400 — spurious, exactly at threshold	0

Uh oh!

Conversation

allyblockcast Bot commented Sep 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Thinking Path

Linked Issues or Issue Description

What Changed

Verification

Risks

Model Used

Checklist

Uh oh!

allyblockcast Bot commented Sep 3, 2026

Uh oh!

allyblockcast Bot commented Sep 3, 2026

Uh oh!

allyblockcast Bot commented Sep 3, 2026

Uh oh!

allyblockcast Bot left a comment

Choose a reason for hiding this comment

Ally — Consolidated PR Review

Critical Issues (0)

Important Issues (2)

Suggestions (2)

Strengths

Recommended Action

Uh oh!

allyblockcast Bot commented Sep 3, 2026

Review findings dispositioned — attesting head 774771a9ecfca88a6984be500ea84eae51de349e

What changed between 62a8388 and 774771a

One CI note for whoever reads the red

Uh oh!

allyblockcast Bot left a comment

Choose a reason for hiding this comment

Ally — Consolidated PR Review

Prior Findings Dispositioned (2)

Critical Issues (0)

Important Issues (2)

Suggestions (3)

Strengths

Recommended Action

Uh oh!

allyblockcast Bot left a comment

Choose a reason for hiding this comment

Ally — Consolidated PR Review

Prior Findings Dispositioned (2)

Critical Issues (0)

Important Issues (0)

Suggestions (4)

Strengths

Recommended Action

Uh oh!

allyblockcast Bot commented Sep 3, 2026

Review findings dispositioned — attesting head 24bc2538daaf5fee0141a480cff87f4066c0e45b

The head moved by re-commit only — verified, not asserted

Important findings (2) — both fixed, confirmed by Ally at this tree

Suggestions (4) — dispositioned

CI state at this head — neither red reflects this diff

Next action

Uh oh!

github-actions Bot commented Sep 3, 2026

Uh oh!

github-actions Bot commented Sep 3, 2026

Uh oh!

github-actions Bot commented Sep 3, 2026

Uh oh!

allyblockcast Bot left a comment

Choose a reason for hiding this comment

Ally — Consolidated PR Review

Prior Findings Dispositioned (2)

Critical Issues (0)

Important Issues (0)

Suggestions (5)

Strengths

Recommended Action

Uh oh!

allyblockcast Bot commented Sep 4, 2026

Pushed ebfdbd2 — plus an answer to your open note

Your unsettled note: is unbounded cumulative re-parking covered elsewhere?

New commit: the guardrail, not just the repair

allyblockcast Bot commented Sep 3, 2026 •

edited

Loading

Review findings dispositioned — attesting head `774771a9ecfca88a6984be500ea84eae51de349e`

What changed between `62a8388` and `774771a`

Review findings dispositioned — attesting head `24bc2538daaf5fee0141a480cff87f4066c0e45b`

Pushed `ebfdbd2` — plus an answer to your open note