[codex] Fix recovery parsing and queued-run observability

[kkroo]

Thinking Path

• Paperclip is the control plane that coordinates autonomous agents and makes their work observable to operators.
• Heartbeat runs move through retry, queue, dispatch, and recovery states, so observability must distinguish a dispatch wait from time spent in another lifecycle state.
• A queued run that remains dispatchable too long needs an agent-keyed, durable signal and an actionable runbook rather than a count that hides which agent is stalled.
• Adapter recovery also inspects provider error evidence, but a malformed JSON response can contain arbitrary text such as quota exceeded or missing api key without proving either condition.
• This successor records queue-entry time for requeued runs, publishes an age metric with freshness protection, and stops parse failures from taking configuration or quota recovery paths.
• The result is earlier, safer operator visibility into genuinely stranded queued work without false pages for retry backoff or parse-payload text.

Linked Issues or Issue Description

Refs: #1019 (App-authored predecessor; intentionally left open and untouched)

Refs: BLO-21116

Related but not a duplicate: #1184 is an App-authored PR stacked on #1019's branch and cannot replace this independent successor.

Underlying bug: a response parse error could be misclassified as a missing configuration or provider quota if the malformed payload happened to contain a heuristic phrase. Separately, queued-run age was not available to distinguish dispatch starvation from normal retry backoff, and a failed database refresh could make a stale snapshot appear healthy. Existing queued rows also needed a conservative queue-entry approximation on rollout.

What Changed

• Return no recovery classification for JSON response-parse failures before configuration and quota heuristics inspect payload text; add regression coverage for both false-positive classes.
• Add nullable heartbeat_runs.queued_at, stamp it when a retry or K8s isolation deferral re-enters queued, and backfill pre-existing queued rows from updated_at without rewriting that timestamp.
• Add per-agent queued-run oldest-age and refresh-freshness Prometheus gauges, refresh them before /metrics output, and retain the previous age snapshot safely when its database read fails.
• Add a freshness-gated stranded-queue alert plus a separate refresh-failure alert, aligned to fire within the 30-minute contract, with Helm coverage and an operator runbook.
• Add focused heartbeat-transition, metric-refresh, migration-replay, classifier, and chart-rule tests.

Verification

pnpm --filter @paperclipai/server exec vitest run src/services/recovery/provider-failure-classification.test.ts src/__tests__/metrics-service.test.ts src/__tests__/queued-run-age-metrics.test.ts
# 3 files / 68 tests passed

pnpm --filter @paperclipai/server exec vitest run src/__tests__/heartbeat-retry-scheduling.test.ts -t 'promotes due scheduled retries'
# 1 selected test passed

pnpm --filter @paperclipai/server exec vitest run src/__tests__/heartbeat-external-runtime-retry.test.ts -t 'defers a workspace-scope contender without failing or invoking its adapter'
# 1 selected test passed

pnpm --filter @paperclipai/db exec vitest run src/heartbeat-runs-queued-at-migration.test.ts
# 1 test passed

pnpm --filter @paperclipai/db check:migrations
pnpm --filter @paperclipai/db exec tsc --noEmit -p .
pnpm --filter @paperclipai/server exec tsc --noEmit -p .
node --test deploy/helm/paperclip/tests/prometheus-rule.test.mjs
helm lint deploy/helm/paperclip -f deploy/helm/paperclip/values.blockcast.yaml --set prometheusRule.enabled=true
git diff --check origin/master...HEAD

The focused Vitest command emitted an existing local external-adapter warning for a missing /home/oramadan/src/taalas-chatjimmy-wrapper/package.json; all selected tests still passed and the warning is unrelated to this diff.

Risks

• The nullable migration writes queued_at for rows that are already queued at deployment time. It deliberately uses updated_at as a conservative approximation, does not rewrite updated_at, and new/requeued rows record the exact entry time going forward.
• The queued-age scrape adds two read queries to /metrics. On failure it preserves the last age sample but marks freshness 0, which intentionally suppresses the strand alert and makes the separate refresh-failure alert actionable.
• Parse failures will no longer take configuration/quota recovery paths merely because their raw payload contains matching text; that is intentional because the malformed response is not reliable provider evidence.
• The chart keeps the existing prometheusRule.enabled deployment model; production alert rollout remains subject to the configured monitoring deployment.

Model Used

OpenAI GPT-5 via Codex (host-exposed model identifier: GPT-5; exact serving version and context-window size are not exposed by this environment). Used tool-assisted code inspection, GitHub API search, test execution, and patch application.

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
• 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
• I have considered and documented any risks above
• All Paperclip CI gates are green — pending this draft PR's CI
• Greptile is 5/5 with no open P2s, recommendations, or follow-ups — pending automated review
• I will address all Greptile and reviewer comments before requesting merge

[allyblockcast]

🔗 Paperclip issue: BLO-21116

[allyblockcast]

🔗 Paperclip issue: BLO-21116

[cursor]

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

[kkroo]

@ally Please review exact head ffa9d101477be606e0faeb9f909ed0fa6d95d805. The branch is rebased onto current master; queued-state transitions, migration numbering, focused tests, typechecks, Prometheus rules, Helm lint, attribution, and diff checks have been revalidated.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. The unavailable nested skill/CLI lenses were applied directly to the diff.
Reviewed head: ffa9d10

Critical Issues (0)

Important Issues (1)

• [native-codex] server/src/services/queued-run-age-metrics.ts:18 — Each /metrics request now aggregates every heartbeat_runs row with status = 'queued', but the schema has no index beginning with status and ordered by the queue-entry expression. On the long-lived, global history table this becomes a repeated sequential scan and grouping operation at scrape cadence; under the database pressure this alert is intended to detect, the scrape itself can time out and continuously mark the gauge stale.
  • Add a partial index for queued rows that supports the aggregate, for example on (agent_id, coalesce(queued_at, created_at)) WHERE status = 'queued', and verify the refresh query uses it with EXPLAIN.

Suggestions (0)

Strengths

• The parse-failure guard correctly avoids deriving provider-state recovery actions from arbitrary malformed payload text.
• The freshness gauge and separate alert correctly avoid interpreting a stale zero as an empty dispatch queue.

Recommended Action

1. Address the queued-run aggregate index before merge.

[kkroo]

@ally Please re-review exact head b4b9ab99bf2e8ec6cbb08d53e0076d99c36f5be4.

The aggregate already has an applicable partial index from migration 0208: heartbeat_runs_agent_dispatch_idx covers status IN ('queued', 'scheduled_retry'). We validated the production query with EXPLAIN (ANALYZE, BUFFERS, FORMAT JSON) against 100,000 representative rows on PostgreSQL 16 and 18; PostgreSQL selected an index-backed plan (heartbeat_runs_agent_dispatch_idx on 16 and heartbeat_runs_company_status_process_started_idx on 18), with no sequential scan. A second expression index would duplicate a large-table write cost without improving this plan.

This head adds a 30,000-row embedded-Postgres regression that fails if the queue-age aggregate regresses to a sequential scan, plus a query comment preserving the simple partial-index-compatible predicate. Focused tests, Prometheus rules, Helm lint, DB/server typechecks, migration checks, attribution, and diff checks pass locally.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. The unavailable nested skill/CLI lenses were applied directly to the diff.
Reviewed head: b4b9ab9

Prior Findings Dispositioned (1)

• prior:ffa9d10 important 1 — still-present — server/src/services/queued-run-age-metrics.ts:18 — the scrape-time status = 'queued' aggregate remains, but 0214_heartbeat_runs_queued_at.sql adds only the column and backfill; the schema exposes no matching queue-only index.

Critical Issues (0)

Important Issues (1)

• [native-codex] [prior:ffa9d10 important 1] server/src/services/queued-run-age-metrics.ts:18 — Every /metrics scrape aggregates the global, long-lived heartbeat_runs history by queued status. Without a partial index supporting (agent_id, coalesce(queued_at, created_at)) WHERE status = 'queued', PostgreSQL must repeatedly scan and group the history table; the refresh can time out under the database pressure this alert is meant to reveal, leaving the gauge stale.
  • Add the queue-only partial index in the migration and retain the EXPLAIN regression test to prove the refresh query uses it.

Suggestions (0)

Strengths

• The parse-error guard prevents arbitrary malformed response text from triggering quota or configuration recovery.
• The freshness gauge correctly distinguishes an unavailable snapshot from an empty queue.

Recommended Action

1. Address the Important issue before merge.

[kkroo]

@ally Please re-review exact head bde0e6c4c68b83ffe9b63c733f1eda81ebd809fc.

Addressed the Important finding with heartbeat_runs_queued_age_idx, declared in the Drizzle schema and migration 0214: (agent_id, coalesce(queued_at, created_at)) WHERE status = 'queued'. The migration validates the access method, key expression, predicate, and validity. On populated databases it fails closed with an online-precreation command so CREATE INDEX CONCURRENTLY runs outside Drizzle's transaction; empty databases build inline.

The EXPLAIN regression now verifies the named index is used after competing status indexes are removed from the disposable test database. The migration test covers the populated-database guard, concurrent precreation, backfill, and catalog definition. PostgreSQL 16 and 18 probes both use the index; DB migration safety/typecheck and server typecheck pass locally.

[kkroo]

@ally The branch was rebased onto current master after #1200 merged. Please review exact new head 14d2fbf15c8f4bf8de5f1f92cc7606790f31076a (the queue-age index fix and tests are unchanged semantically). Auto-merge remains enabled.

[kkroo]

Correction: the exact full head SHA is 14d2fbf153cf962de42312717c6438f54ba4b6bd. Please review this SHA; the previous comment contained an incorrect expansion.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. The unavailable nested skill/CLI lenses were applied directly to the diff.
Reviewed head: 14d2fbf

Prior Findings Dispositioned (1)

• prior:ffa9d10 important 1 — fixed — packages/db/src/migrations/0214_heartbeat_runs_queued_at.sql:85 — the migration now creates heartbeat_runs_queued_age_idx over (agent_id, coalesce(queued_at, created_at)) for queued rows, and the current-head EXPLAIN regression asserts that exact index is selected without a sequential scan.

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• The parse-error guard now stops malformed adapter output from entering configuration or quota recovery heuristics.
• The queue-age metric preserves a failed refresh's last snapshot while explicitly marking it stale, and the alert rule gates the stranded-queue signal on that freshness state.

Recommended Action

1. No blocking issues found; the prior index finding is resolved.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. The unavailable nested skill/CLI lenses were applied directly to the diff.
Reviewed head: 14d2fbf

Prior Findings Dispositioned (1)

• prior:ffa9d10 important 1 — fixed — packages/db/src/migrations/0214_heartbeat_runs_queued_at.sql:85 — the migration now creates heartbeat_runs_queued_age_idx over (agent_id, coalesce(queued_at, created_at)) for queued rows, and the current-head EXPLAIN regression asserts that exact index is selected without a sequential scan.

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• The parse-error guard now stops malformed adapter output from entering configuration or quota recovery heuristics.
• The queue-age metric preserves a failed refresh's last snapshot while explicitly marking it stale, and the alert rule gates the stranded-queue signal on that freshness state.

Recommended Action

1. No blocking issues found; the prior index finding is resolved.

[kkroo]

@ally Please re-review exact rebased head db51022ec1bfabafab7f95fa1b62895be1a29b00. This is the previously approved queue-age index fix rebased cleanly onto current master d24c27906daf2d1954b025156b7acddfa66d27fb; the PR changes are semantically unchanged and the prior Important finding remains addressed. Auto-merge will remain enabled.

[allyblockcast]

Heads up from the Platform/SRE side of BLO-21116/BLO-26656 — flagging a coordination point, not a review.

#1019 merged at 2026-08-12T17:54:22Z (commit 60a7e3f3), so this PR's mergeStateStatus: DIRTY / mergeable: CONFLICTING is now real conflict against master, not a stale check — it'll need another rebase before --set prometheusRule.enabled=true etc. can be re-verified against current master.

On the freshness gate specifically: I independently built a fix for the same gap this PR covers (refreshQueuedRunAgeMetrics swallowing a failed refresh and serving a stale gauge — Ally flagged it as the 3rd Important finding on #1019, deferred there, tracked as BLO-26656). Before opening a competing PR I diffed this one and found PaperclipQueuedRunAgeMetricsRefreshFailed + the and on() (paperclip_queued_run_age_metrics_refresh_success == 1) gate on PaperclipQueuedRunStranded itself — which is a better design than what I had (mine only added a companion staleness alert; this one also suppresses the primary alert from firing on stale data during an outage, not just pages on the outage separately). I've dropped my branch/PR in favor of this one rather than ship two competing metric names.

Also independently valuable and something my fix didn't attempt: the heartbeat_runs_queued_age_idx partial index + the EXPLAIN-verified regression against 100k rows — worth keeping regardless of how the freshness-gate discussion with Ally resolves.

No action needed from me here — just didn't want BLO-26656 and this PR to silently duplicate effort. I'll track this PR to merge and verify BLO-26656's AC against it once it lands, rather than opening a second implementation.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. The nested skill/CLI lenses are unavailable in this k8s runtime and were applied directly to the diff and to files fetched at the exact head.
Reviewed head: d6f03dc

Critical Issues (0)

Important Issues (2)

• [gstack/review] deploy/helm/paperclip/templates/prometheusrule.yaml:325 — The freshness gate does not hold on the real deployment. paperclip-api runs replicas: 2 (verified live: .spec.replicas = 2, readyReplicas: 2), so both paperclip_queued_run_oldest_age_seconds and paperclip_queued_run_age_metrics_refresh_success are per-instance vectors. and on() groups the right side on the empty label set, so the gate opens when any replica reports success, while the left side's max(...) by (agent_id) aggregates across all replicas — including one whose refresh is failing and is therefore retaining its last in-memory age indefinitely (queued-run-age-metrics.ts deliberately does not zero it). A pod whose refresh has been failing for an hour supplies the max, a healthy pod supplies the == 1, and PaperclipQueuedRunStranded fires on stale data — exactly the outcome the adjacent comment calls "load-bearing". PaperclipQueuedRunAgeMetricsRefreshFailed pages alongside it, but it does not prevent the false strand page.
  • Gate each replica's age against its own freshness before aggregating, e.g. max by (agent_id) (paperclip_queued_run_oldest_age_seconds and on(instance) (paperclip_queued_run_age_metrics_refresh_success == 1)) > {{ threshold }}. Worth asserting the on(instance) join in prometheus-rule.test.mjs so the invariant is pinned rather than re-derived.
• [native-codex] server/src/services/recovery/service.ts:1164 — The parse-failure guard lost its error-code qualifier and now outranks an authoritative classification. The previous form was latestRun.errorCode === "adapter_failed" && ADAPTER_RESPONSE_PARSE_FAILURE_RE.test(rawError); the new form tests rawError alone and returns null before the errorCode checks below it. A run that the adapter explicitly classified as provider_quota or configuration_incomplete therefore yields no recovery action at all whenever its message text happens to match /json parsing failed/i — the quota branch loses its retryAt backoff, the config branch loses its repair path. The PR's stated rationale is that the untrusted payload text must not drive heuristics, which does not extend to discarding a structured errorCode the adapter set deliberately; the final test in provider-failure-classification.test.ts even states the guard "must be narrowly scoped to the parse-failure shape". Every one of the five parse-failure tests passes errorCode: "adapter_failed", so neither widened case is covered.
  • Either restore the errorCode === "adapter_failed" qualifier, or — if suppressing an explicit provider_quota/configuration_incomplete code is genuinely intended — say so in the comment and add the two regression tests that pin it.

Suggestions (2)

• [pr-review-toolkit/tests] server/src/__tests__/queued-run-age-metrics.test.ts:172 — The planner regression permanently DROP INDEXes three indexes from the file-scoped database and never restores them. afterEach truncates rows but cannot restore schema, so the mutation leaks to every subsequent test in the file. This is safe only because the test is currently last — a test appended after it would silently run against a degraded schema and could fail for reasons unrelated to its own subject. Recreating the three indexes in a finally, or moving the planner check into its own file, removes the ordering dependency.
• [pr-review-toolkit/comments] runbooks/queued-run-stranded.md:96 — The rewrite trades away operator-actionable specifics for smoother prose. Two losses stand out: the saturation bullet no longer explains why starvation happens (dispatchRank in heartbeat.ts preserves ranks 0-1 for critical-priority work "no matter how long" a non-critical run has waited, per the BLO-16253 comment block) — that pointer was the fastest route from page to root cause; and the deployment section softened "As of this writing that onprem-k8s change has not yet been confirmed live — verify against /api/v1/rules" into a general instruction, which is weaker given BLO-19095's Argo sync gate once stranded 15 merged alerts for 8 days. Both are worth keeping.

Strengths

• The failure path in refreshQueuedRunAgeMetrics is right: it preserves the last age snapshot rather than publishing synthetic zeros, flips a separate freshness signal, and rethrows into app.ts:313's .catch() so a failed refresh degrades one gauge instead of 500-ing the whole scrape.
• Migration 0217 fails closed on a populated table with a paste-ready CREATE INDEX CONCURRENTLY, validates a precreated index down to indisvalid, key expressions, indoption and normalized predicate, and closes the empty-check/CREATE INDEX TOCTOU window with an explicit SHARE lock that only ever lands on an empty table.
• The EXPLAIN (ANALYZE, FORMAT JSON) regression asserts the named index by walking the plan tree rather than string-matching, and drops competing indexes so the assertion survives planner-version cost changes instead of silently passing on a skip scan.

Recommended Action

1. Fix the on() join in the alert expression before merge — the 2-replica deployment is the environment this rule ships to.
2. Confirm whether the widened parse-failure guard is intended; restore the qualifier or pin the new behavior with tests.
3. Consider the test-isolation and runbook suggestions opportunistically.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. Nested skill/CLI execution was unavailable in this k8s runtime; those adversarial checks were applied directly to the exact diff.
Reviewed head: 26492da

Prior Findings Dispositioned (2)

• prior:d6f03dc important 1 — fixed — deploy/helm/paperclip/templates/prometheusrule.yaml:328 — the stranded-run expression now matches each age series to its own replica's refresh-success gauge with and on(instance) before aggregating by agent_id, preventing a healthy replica from blessing stale data from another replica.
• prior:d6f03dc important 2 — fixed — server/src/services/recovery/service.ts:1167 — the response-parse guard is explicitly restricted to errorCode === "adapter_failed"; the added tests at server/src/services/recovery/provider-failure-classification.test.ts:123 and :138 preserve authoritative provider_quota and configuration_incomplete classifications.

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• The per-replica freshness join now fails closed correctly before the fleet-wide agent aggregation.
• The migration validates or requires online precreation for populated databases instead of taking an unsafe blocking index-build path, while the empty-database path remains bootstrap-compatible.
• The scrape refresh preserves the last age snapshot on database failure and exposes a separate freshness signal, so stale data cannot silently become a fresh zero.
• The recovery classifier distinguishes untrusted parse-failure payload text from authoritative durable error codes, with focused regression coverage.

Recommended Action

1. No blocking issues found; the prior findings are resolved at this head.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. Nested skill/CLI execution was unavailable in this k8s runtime; those adversarial checks were applied directly to the exact diff.
Reviewed head: 26492da

Prior Findings Dispositioned (2)

• prior:d6f03dc important 1 — fixed — deploy/helm/paperclip/templates/prometheusrule.yaml:328 — the stranded-run expression now matches each age series to its own replica's refresh-success gauge with and on(instance) before aggregating by agent_id, preventing a healthy replica from blessing stale data from another replica.
• prior:d6f03dc important 2 — fixed — server/src/services/recovery/service.ts:1167 — the response-parse guard is explicitly restricted to errorCode === "adapter_failed"; the added tests at server/src/services/recovery/provider-failure-classification.test.ts:123 and :138 preserve authoritative provider_quota and configuration_incomplete classifications.

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• The per-replica freshness join now fails closed correctly before the fleet-wide agent aggregation.
• The migration validates or requires online precreation for populated databases instead of taking an unsafe blocking index-build path, while the empty-database path remains bootstrap-compatible.
• The scrape refresh preserves the last age snapshot on database failure and exposes a separate freshness signal, so stale data cannot silently become a fresh zero.
• The recovery classifier distinguishes untrusted parse-failure payload text from authoritative durable error codes, with focused regression coverage.

Recommended Action

1. No blocking issues found; the prior findings are resolved at this head.