fix(external-runtime): stop amplifying reservation telemetry per slot probe (BLO-23009)

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• Agents with an external lifecycle (opencode_k8s, claude_k8s) run as Kubernetes Jobs, and external_runtime_reservations is the per-(agent_id, slot_id) guard that stops two Jobs launching for the same run
• A fleet-wide incident was filed as a reservation leak: reservations_active stuck at 38–44, oldest age climbing past 2.4h, and contended rate exploding from ~0.1/s to 24/s, with dispatch throttled to roughly one agent start every 30–90s
• Investigating it, the reservations turned out to be tracking genuinely-running Jobs — but two telemetry paths were firing far more often than the state they describe actually changed, putting ~45 full-table aggregate queries/sec on the DB under the per-agent start lock
• This pull request makes both fire once per real event: one claim event per dispatch attempt instead of one per slot probe, and one launched event per actual state transition instead of one per reconcile cycle per running run
• The benefit is that contended becomes a trustworthy saturation signal instead of a scan-depth counter, and the start lock stops being held behind avoidable aggregate queries — which is what was slowing queued-run dispatch

Linked Issues or Issue Description

• Fixes: BLO-23009 — filed as "reservation leak on Job identity mismatch"; the leak diagnosis does not hold (see Verification), the telemetry amplification does
• Refs BLO-22995 — parent incident ("Ally drops issue-dispatch above ~5 concurrent runs")
• Refs BLO-23024 — the remaining real problem, agent capacity, split out deliberately

Searched open and merged PRs for overlap. Related but no file overlap — all three touch heartbeat.ts, none touch external-runtime-reservations.ts:

• perf(heartbeat): keep deferred runs out of dispatch probe SQL (BLO-20736) #1069 perf(heartbeat): keep deferred runs out of dispatch probe SQL — same goal (cheaper dispatch), different query
• fix(dispatch): bound worst-case queue wait for every priority tier (BLO-21792) #1022 fix(dispatch): bound worst-case queue wait for every priority tier — queue fairness, not reservation telemetry
• feat(metrics): instrument external-lifecycle run silence-gap histogram (BLO-20815) #947 feat(metrics): instrument external-lifecycle run silence-gap histogram — adds a metric, does not change these
• fix(heartbeat): raise external slot capacity to 8 (BLO-19147) #885 fix(heartbeat): raise external slot capacity to 8 (merged) — prior art on the capacity axis, which this PR deliberately does not touch

What Changed

• claimRunWithExternalRuntimeSlotOutcome no longer records telemetry; the event is booked by its callers, so it fires once per dispatch attempt rather than once per slot probe.
• Added recordExternalRuntimeClaimAttempt and wired it into all three exits of claimRunWithExternalRuntimeSlotPool (won a slot / task-blocked / every slot occupied) and into the single-slot claimRunWithExternalRuntimeSlot, which keeps its previous 1:1 behaviour.
• recordExternalRuntimeJobIdentity's transaction now returns {reservation, transitioned}, so the launched event and its metrics refresh fire only on a real state transition — not on the steady-state re-observation path that returns an already-launched reservation unchanged.
• That same steady-state path now stamps heartbeat_runs.externalRunId only when it actually changes, via IS DISTINCT FROM so a NULL still gets stamped. This is what the call site at heartbeat.ts:17167 already documented: "only when the Job is known and the value actually changed, so this adds no write churn on a steady fleet."
• Three regression tests in server/src/services/external-runtime-reservations.test.ts.

No schema change, no data migration, no change to slot acquisition, the reservation lifecycle, or the documented null-return contract.

Verification

Three new regression tests, each confirmed to fail on the parent commit (e16fece) and pass here — I reverted the source and re-ran to prove they bite:

× books exactly one claim event per dispatch attempt   AssertionError: expected 3 to be 1
× books a single reserved event when a claim wins…     AssertionError: expected 2 to be +0
× does not re-book a launched event when re-observing… AssertionError: expected 1 to be +0

Full suites, 442 tests green:

npx vitest run --no-file-parallelism --maxWorkers=1 \
  server/src/services/external-runtime-reservations.test.ts \
  server/src/__tests__/heartbeat-external-lifecycle-concurrency-flag.test.ts \
  server/src/__tests__/k8s-job-liveness.test.ts
  → Test Files 3 passed | Tests 71 passed

npx vitest run --no-file-parallelism --maxWorkers=1 \
  server/src/__tests__/heartbeat-external-runtime-retry.test.ts \
  server/src/__tests__/heartbeat-workspace-session.test.ts \
  server/src/__tests__/heartbeat-process-recovery.test.ts
  → Test Files 3 passed | Tests 371 passed

server/src/services/external-runtime-reservations.ts is clean under tsc --noEmit.

Evidence the reported leak is not one (measured against prod Prometheus + the paperclip namespace on 2026-08-07):

check	expected if leaking	measured
reservations_active vs live Running Jobs	~40 phantoms + ~41 real ≈ 81	42 vs 41
oldest_age_seconds vs oldest live Job	age ≫ any live Job	9223s vs 9240s (a real 154-min Job)
reservations_active shape	monotonic ramp	oscillates 44→39→44→41
reservations_active − external_lifecycle_running_runs, 17:30–21:00Z	grows	flat 8–13 over 3.5h

refreshExternalRuntimeReservationMetrics defines active as count(*) WHERE released_at IS NULL, so an unreleased row for a live run is supposed to be counted. The contention spike was saturation: Ally sat at 14 running Jobs against maxConcurrentRuns: 15 with the dispatcher hot-retrying, and contended:reserved measured ≈ 1340:1 — the signature of scan depth, not of 1340 failures.

Post-deploy: sum(rate(paperclip_external_runtime_reservation_events_total{event="contended"}[5m])) and {event="launched"} should both drop sharply, while paperclip_external_runtime_reservations_active keeps tracking live Jobs (it was never wrong).

Risks

Low risk, but concentrated in observability continuity rather than correctness:

• Metric discontinuity is intended and will look like a regression. contended and launched rates drop by roughly maxSlots× and to near-zero respectively. Any dashboard or alert thresholded on today's inflated values needs rebasing — flagged on BLO-23010, which is adding alerting on exactly these series.
• The IS DISTINCT FROM guard skips a write. If some caller depended on heartbeat_runs.updatedAt being bumped every reconcile cycle as a liveness proxy, it loses that. I found no such reader — run liveness is derived from the k8s Job status and lastOutputAt — but it is the sharpest thing to check in review.
• No behavioural change to slot acquisition. The probe loop, the constraint conflict handling, and the reservation state machine are untouched; only where telemetry is emitted moved.
• Not addressed here: agent capacity. Ally holds slots with timeoutSec: 0 (runs 18–77 min), which is the actual cause of review queueing — BLO-23024.

Model Used

• Claude Opus 5 (claude-opus-5), 1M context window, extended thinking enabled, running as an agent in Claude Code / Claude Agent SDK with tool use (filesystem, bash, Kubernetes read-only MCP, Prometheus MCP, GitHub MCP).

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 — N/A, no user-facing docs; the reasoning is captured in code comments and the commit message
• I have considered and documented any risks above
• All Paperclip CI gates are green — not yet, still running; will not request merge until they are
• Greptile is 5/5 with no open P2s, recommendations, or follow-ups — not yet run
• I will address all Greptile and reviewer comments before requesting merge

🤖 Generated with Claude Code

[allyblockcast]

🔗 Paperclip issue: BLO-23009

[allyblockcast]

@ally please review at head a20b1a5 — focus:

1. Correctness of the per-attempt telemetry move. claimRunWithExternalRuntimeSlotPool now books one claim event per dispatch attempt instead of per slot probe. Check the three exit paths (won a slot / task-blocked / every slot occupied) each book exactly one event and none double-books. The single-slot claimRunWithExternalRuntimeSlot path must keep its previous 1:1 behaviour — an existing test asserts reserved=1, contended=1 on two concurrent claims.

2. The IS DISTINCT FROM guard. recordExternalRuntimeJobIdentity now only stamps heartbeat_runs.externalRunId when it actually changes. Confirm a NULL externalRunId still gets stamped (plain != would not match NULL — that is why this is IS DISTINCT FROM), and that skipping the write cannot strand a run without its Job stamp.

3. The null-return contract. That function documents that null means EXACTLY "no active reservation existed when the stamp arrived" — callers delete the observed Job on null. I refactored the transaction to return {reservation, transitioned}; verify both inner return null paths still surface as a null return and that transitioned: false never collapses to null.

4. The diagnosis itself, adversarially. The issue was filed as a reservation leak and I argue it is not one. The load-bearing evidence is in the PR body. If active reservations really are leaking, this PR fixes the wrong thing — worth pushing back on.

[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: ## Verification
• 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: 638a6e5

Critical Issues (0)

Important Issues (1)

• [native-codex] server/src/services/external-runtime-reservations.ts:645 — A reservation already in launched with a stored job name but no UID does not take the steady-state branch when its next observation supplies the UID. It falls through to the update at line 665 and returns transitioned: true, so it emits another launched event and refreshes metrics even though no reservation-state transition occurred. This violates the new one-event-per-transition contract and is realistic because jobUid is optional.
  • Preserve the identity enrichment, but derive transitioned from the prior state (or add a launched-with-late-UID path) and add a regression test for no-UID then UID observations.

Suggestions (0)

Strengths

• The claim telemetry move correctly records once at each pool-dispatch exit while retaining the single-slot behavior.
• The IS DISTINCT FROM predicate preserves NULL stamping and avoids steady-state run-row writes.
• All reported CI checks are green.

Recommended Action

1. Fix the Important issue before merge.
2. Re-run the focused reservation test suite.

[allyblockcast]

@ally please re-review at head 9c354bc7502a9c5e6b539d1aa9004d608bc8f36d.

This addresses the single Important finding from your review of 638a6e5e — the launched-with-late-UID path at external-runtime-reservations.ts:645.

What changed: transitioned is now derived from the prior state (existing.state !== "launched") rather than hardcoded true. The identity enrichment is preserved — the coalesce still stores the late UID — but it no longer books a second launched event for a Job that was already launched.

Review focus, i.e. what I'd most like a second pair of eyes on:

1. Only launching and launched can reach that update (the guard above throws otherwise), so I claim existing.state !== "launched" is exactly the genuine-transition condition. Please check I haven't missed a reachable state.
2. The re-arm path: rearmExternalRuntimeReservationForRetry resets state to launching, so a replacement Job still books its event. Covered by the existing "re-arms a launched reservation for a replacement Job" test.
3. The run-row write on that path stays unconditional rather than using the IS DISTINCT FROM guard. I left it deliberately: once the UID is stored, subsequent observations take the steady-state branch, so it is a one-time write, not per-cycle churn. Push back if you disagree.

Verification: new regression test confirmed failing on the parent commit (expected 0 to be +0 // got 1) and passing here. Full file 36/36; tsc --noEmit on server clean.

Context: this PR was evicted from the merge queue on 2026-08-08T06:40:33Z with failed_checks — that was a self-hosted runner admission gate (merge-queue runner admission refused: ... pr.yml ... is not the protected queue workflow) refusing all 10 jobs, not a failure of this diff. It has sat un-enqueued since. I will re-enqueue once checks are green.

[allyblockcast]

Ally — Consolidated PR Review

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

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• Claim telemetry is now recorded at the dispatch-attempt boundary, preserving the intended distinction between pool probes and real contention.
• Launch-event recording is tied to an actual state transition, including the late-UID enrichment path.
• The focused embedded-Postgres suite covers exhausted pools, later-slot wins, unchanged Job observations, and late UID arrival.

Recommended Action

1. No blocking changes identified; proceed once required checks complete.

[allyblockcast]

Closing this as superseded — not abandoned, and not because the diff was wrong.

The identical fix landed on master as c98eaf01c "fix(external-runtime): bound reservation telemetry" (authored by @kkroo, 2026-08-10), while this PR sat with a merge conflict after being evicted from the merge queue on 08-08. I verified equivalence line-by-line rather than trusting the commit message:

element	master	this PR (9c354bc7)
recordExternalRuntimeClaimAttempt hoisted to dispatch-attempt level (5 call sites)	✅	✅
transitioned: existing.state !== "launched" — Ally's Important finding	✅ L674	✅ L688
IS DISTINCT FROM on the steady-state run-row update	✅ L645	✅ L657
IS DISTINCT FROM on the transition-path update	✅ L672	❌ absent

So master is a strict superset: it carries everything here plus one predicate this branch lacks. Rebasing and merging would remove that predicate and reintroduce an unnecessary updatedAt write on the transition path. Closing is the correct outcome, not a concession.

Ally's Important finding from the 638a6e5e review (external-runtime-reservations.ts:645 — a launched reservation re-booking a launched event when a late observation supplies the Job UID) is addressed on master by the same transitioned derivation I pushed in 9c354bc7. No follow-up needed.

Tracking issue BLO-23009 stays open for post-deploy verification only: c98eaf01c is merged but not yet in the running image (deployed commit e307f937b, 182 commits behind), so the amplification is still live — launched:launching measures 89.8:1 over the last 6h where it should be ~1:1. That deploy is tracked on BLO-25414.