feat(metrics): observe PR review queue wait (BLO-30623)

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• One of those agents, Ally, reviews GitHub pull requests; requests reach it through the webhook receiver, become heartbeat_runs keyed pr_review:<repo>:<number>, and wait for a concurrency slot
• The receiver is well instrumented, but every counter it emits stops at durably queued — there is nothing measuring how long a queued review then waits to start
• BLO-20491 found the consequence: 12 PRs received, correctly keyed, correctly queued, then starved 320–584 minutes for a slot, while execution time never moved off 6–16 minutes. Ingest counters read perfectly healthy the whole time, because received == queued + deferred with zero terminal loss is exactly what a healthy ingest feeding a saturated consumer looks like
• Detecting that required an org-wide GitHub sweep of 286 open PRs — the symptom was only visible from outside the system
• This pull request adds the missing service-latency signal: a histogram of queued-to-started for pr_review:* runs, plus a p95 alert
• The benefit is that consumer starvation becomes a page instead of a manual sweep, and it closes the last open instrumentation acceptance criterion on BLO-20491

Linked Issues or Issue Description

• Closes BLO-30623 — "Instrument Ally PR-review queue wait and alert on p95 saturation"
• Refs BLO-20491 — parent; this is its remaining "review-request service latency is observable, not just ingest" AC
• Related but not duplicated: BLO-20761 measures GitHub Actions queue wait. Different queue, different subject — that one is about CI runners, this one is about heartbeat_runs.
• Searched open and merged PRs for queue wait / pr review queue / BLO-30623. Nearest neighbours are fix(dispatch): bound worst-case queue wait for every priority tier (BLO-21792) #1022 (bound worst-case queue wait for every priority tier, merged — changes dispatch behaviour, adds no metric) and fix: yield slots during external waits #1195 (yield slots during external waits, open — also behavioural). Neither instruments queue wait, and no open PR touches paperclip_pr_review_queue_wait_seconds.

What Changed

• server/src/services/metrics.ts — adds paperclip_pr_review_queue_wait_seconds, a histogram with buckets [60, 300, 600, 900, 1800, 3600, 7200, 14400, 28800] seconds. Exposes computePrReviewQueueWaitSeconds (pure, testable) and recordPrReviewQueueWait (observes). Wired into ensureRegistry's null-check, return object, and __resetMetricsForTest alongside the neighbouring metrics.
• server/src/services/heartbeat.ts — calls recordPrReviewQueueWait at the guarded queued-to-running transition, immediately after the if (!claimed) return null guard. Placement is the whole design: it observes exactly once per run, and a run that never starts is excluded by construction rather than by a filter that could drift.
• Bounded labels. The histogram carries no labels at all. The pr_review: task key selects which runs are observed but is deliberately never promoted to a label — repo and PR number are exactly where cardinality would grow without bound. Per-request detail stays in durable heartbeat_runs.context_task_key and the logs.
• deploy/helm/paperclip/templates/prometheusrule.yaml — new paperclip-pr-review-queue group with PaperclipPrReviewQueueWaitSaturated: p95 > 60m over a rolling 6h window, held 10m, carrying runbook_url and bounded service/signal labels.
• deploy/helm/paperclip/values.yaml — parameterises window / hold / threshold / runbook URL rather than hardcoding them in the template.
• runbooks/pr-review-queue-wait.md — triage PromQL and what to inspect when it fires.

Verification

This commit was authored on a branch far behind master and I rebased it by hand, so I re-verified from scratch rather than trusting the pre-rebase result. That mattered: resolving the conflicts as a union spliced the new histogram inside an unrelated Gauge definition and dropped a function's closing brace in metrics.ts. Both were repaired before the commit.

# 84/84 pass
cd server && vitest run src/__tests__/metrics-service.test.ts

# 22/22 pass
cd deploy/helm/paperclip && node --test tests/prometheus-rule.test.mjs

• Server tests cover: exact-duration observation for a synthetic pr_review:* run (3900s), rejection of an issue_board: key, rejection of a never-started run and an unparseable timestamp, bucket emission at le="3600" = 0 / le="7200" = 1 / _count = 1, the exact bucket list, and an assertion that rendered output contains no repo name.
• Helm test asserts the rendered p95 expression, for: 10m, and the runbook URL.
• tsc --noEmit — the error list is identical to pristine master's, modulo a +1 line shift from the added import. Zero new type errors. Both runs report the same 142 pre-existing errors in my sandbox, caused by stale workspace dists there; they are unrelated to this change and reproduce without it.

Risks

• Low risk to running behaviour. The server change is one function call on the run-start path that computes a subtraction and observes a histogram. It has no database access, no I/O, and no failure mode that can reject a run.
• ⚠️ This PR alone does not put the alert in production, and should not be read as doing so. values.blockcast.yaml keeps prometheusRule.enabled: false — deliberately, because enabling it 403s the entire helm upgrade rather than just skipping the resource. Blockcast's authoritative PrometheusRules are applied from Blockcast/onprem-k8s. The metric ships here; the alert needs a companion onprem-k8s change before BLO-30623's deployment AC is met. The runbook states this at the point of use so a reader hitting it at 3am does not assume the alert is live.
• Bucket choice is a judgement call. The top bucket is 8h; the observed starvation was 5.3–9.7h, so the worst historical cases sit at or just past the top edge. p95 stays correct for the alert's purpose (it only needs to resolve the 60m crossing, which sits mid-range at the 3600 boundary), but a future incident materially worse than the one that motivated this would land in +Inf and be under-resolved.
• No migration, no schema change, no API change, no UI change.

Model Used

Claude Opus 4.5 (claude-opus-5[1m]), 1M context, extended thinking, with tool use and code execution via Claude Code.

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 surface
• I have updated relevant documentation to reflect my changes (new runbook)
• I have considered and documented any risks above
• All Paperclip CI gates are green — not yet; several checks still running at the time of writing
• Greptile is 5/5 with no open P2s, recommendations, or follow-ups — not yet reviewed
• I will address all Greptile and reviewer comments before requesting merge

🤖 Generated with Claude Code

[allyblockcast]

🔗 Paperclip issue: BLO-30623
🔗 Paperclip issue: BLO-20491

[allyblockcast]

🔗 Paperclip issue: BLO-30623
🔗 Paperclip issue: BLO-20491

[allyblockcast]

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 please review PR #1572 at head 640a1e3 (BLO-30623).

This commit was authored on a stale branch and rebased onto current master by hand — a union conflict resolution spliced the new histogram inside an unrelated Gauge definition and dropped a function's closing brace in server/src/services/metrics.ts. I repaired both, so please weight that file heavily:

1. metrics.ts — is prReviewQueueWait registered correctly, and are ensureRegistry's null-check, return-object, and __resetMetricsForTest entries all consistent with how the neighbouring metrics are wired?
2. heartbeat.ts — is recordPrReviewQueueWait at the right point? It must observe exactly once per run, after the claim guard, and never for a run that did not start.
3. Cardinality — confirm nothing repo/PR/agent-derived can reach a label. The pr_review: task key selects the run but is deliberately not promoted to a label.
4. The p95 alert expression and its 6h window / 10m hold / 3600s threshold parameterisation.

Context on the chart being a mirror only (prometheusRule.enabled: false in values.blockcast.yaml, authoritative rules live in Blockcast/onprem-k8s) is in the PR description.

[allyblockcast]

Ally — Consolidated PR Review

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

Critical Issues (0)

Important Issues (0)

Suggestions (1)

• [native-codex] server/src/services/heartbeat.ts:19514 — add a focused integration/regression test proving that a claimed PR-review run records queue wait using the persisted contextTaskKey and that the claim path observes exactly once under concurrent dispatch.

Strengths

• The metric is observed only after the guarded queued-to-running claim, avoiding measurements for never-started runs.
• Labels are intentionally bounded by omitting repository, PR, agent, and delivery identifiers.
• The disabled chart rule and the required authoritative onprem-k8s follow-up are documented clearly in the runbook and PR description.
• Tests cover valid and invalid timestamps, task-key filtering, bucket output, and label-cardinality protection.

Recommended Action

1. No Critical or Important issues found; merge may proceed subject to CI and the documented production alert follow-up.
2. Consider adding the claim-path integration coverage in a follow-up.

[kkroo]

Reran the failed lanes for exact head 2eca6397: General tests (workspaces-a) and verify both pass on rerun. All visible required checks are now green; the earlier failure was a flaky unrelated UI test (CompanyEnvironments.test.tsx:715), and this diff touches no UI files. Ally’s review reports 0 Critical/Important findings; the remaining integration-test suggestion is non-blocking. Good to merge when the repository’s normal merge gate permits.

[kkroo]

Approved after the exact-head failed-lane rerun: all required checks, including verify, are green. The sole transient failure was the unrelated CompanyEnvironments.test.tsx:715 workspace test; rerun passed. The PR is low-risk metrics/runbook work with no UI changes.