From 6c07deebf014f01d74e6939eec0b146e3a391352 Mon Sep 17 00:00:00 2001 From: Jory Irving Date: Wed, 14 Oct 2026 08:38:24 -0600 Subject: [PATCH 1/5] feat(recovery): fence capacity-exhaustion handoffs (#1256) --- .env.example | 6 + AGENTS.md | 7 + docs/capacity-handoff.md | 207 ++++++++ docs/worker-execution-contract.md | 23 + .../migration.sql | 32 ++ prisma/schema.prisma | 56 +++ .../[agentName]/next-task/route.test.ts | 197 +++++++- .../api/agents/[agentName]/next-task/route.ts | 65 ++- .../[agentName]/tasks/report/route.test.ts | 28 ++ .../agents/[agentName]/tasks/report/route.ts | 50 +- .../api/issues/capacity-handoff/route.test.ts | 453 ++++++++++++++++++ src/app/api/issues/capacity-handoff/route.ts | 114 +++++ src/app/api/issues/claim/route.test.ts | 13 + src/app/api/issues/claim/route.ts | 12 +- src/lib/agent-task.ts | 8 + src/lib/capacity-handoff.test.ts | 173 +++++++ src/lib/capacity-handoff.ts | 189 ++++++++ src/lib/issue-capacity-handoff.test.ts | 190 ++++++++ src/lib/issue-capacity-handoff.ts | 446 +++++++++++++++++ src/lib/lease.test.ts | 81 ++++ src/lib/lease.ts | 11 + src/lib/mc-client.ts | 6 + src/lib/next-action.ts | 2 + src/lib/pr-fix-queue.test.ts | 353 +++++++++++++- src/lib/pr-fix-queue.ts | 255 +++++++++- src/lib/prisma.ts | 1 + 26 files changed, 2942 insertions(+), 36 deletions(-) create mode 100644 docs/capacity-handoff.md create mode 100644 prisma/migrations/20261014000000_capacity_handoff_recovery/migration.sql create mode 100644 src/app/api/issues/capacity-handoff/route.test.ts create mode 100644 src/app/api/issues/capacity-handoff/route.ts create mode 100644 src/lib/capacity-handoff.test.ts create mode 100644 src/lib/capacity-handoff.ts create mode 100644 src/lib/issue-capacity-handoff.test.ts create mode 100644 src/lib/issue-capacity-handoff.ts diff --git a/.env.example b/.env.example index 6b38f34b..ee7fef7f 100644 --- a/.env.example +++ b/.env.example @@ -328,6 +328,12 @@ DISPATCH_AGENT_TOKEN="your_agent_token_here" # worker-side readiness check. Bounded to 60 000–604 800 000; default 21 600 000 (6h). # PR_FIX_EXTERNAL_WAIT_COOLDOWN_MS=21600000 +# Bounded cooldown (ms) applied to an explicit capacity-exhaustion handoff +# (#1256). Scoped to the exhausted capacity on the handed-off item only — the +# work stays dispatchable to unrelated capacities. Bounded to 60 000–21 600 000; +# default 900 000 (15 min). See docs/capacity-handoff.md. +# DISPATCH_CAPACITY_COOLDOWN_MS=900000 + # --- Legacy aliases (do not set) --- # MISSION_CONTROL_URL and MISSION_CONTROL_AGENT_TOKEN were renamed to diff --git a/AGENTS.md b/AGENTS.md index 21031f80..b6729886 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -49,6 +49,7 @@ npm run db:deploy # Deploy migrations (prod) | `DISPATCH_GROOMER_INTERVAL_MS` | No | Groomer run interval (default 600000 = 10m; `0` disables) | | `DISPATCH_PR_FOLLOWUP_INTERVAL_MS` | No | PR-followup sync interval (default 900000 = 15m; `0` disables) | | `DISPATCH_PRUNE_CLOSED_INTERVAL_MS` | No | Closed-issue prune interval (default 86400000 = 24h; `0` disables) | +| `DISPATCH_CAPACITY_COOLDOWN_MS` | No | Bounded cooldown for explicit capacity-exhaustion handoffs (default 900000 = 15m, clamped 1m–6h). Scoped to the exhausted capacity on the handed item only; unrelated capacities keep dispatch. See [docs/capacity-handoff.md](./docs/capacity-handoff.md) | | `DISPATCH_QUEUE_ADMISSION_MODE` | No | Grooming freshness admission for worker pickup: `off` (default, unchanged), `audit` (annotate queue items, filter nothing) or `enforce` (withhold ready issues without a current applied grooming decision or operator override). See [docs/hosted-groomer.md](./docs/hosted-groomer.md#worker-admission). | Resolution order: `DATABASE_URL` > `DISPATCH_DATABASE_URL`. `DISPATCH_AGENT_TOKEN` for agent API bearer auth. @@ -210,6 +211,7 @@ Cross-cutting concerns and where they live. Read these first when working in tha | Agent claim / unclaim (lease + status) | `app/api/issues/claim/route.ts`, `app/api/issues/unclaim/route.ts`, `lib/lease.ts` | | Assign / unassign (label-only, no lease) | `app/api/issues/actions/route.ts`, `app/api/issues/unassign/route.ts`, `lib/assignment-conflicts.ts` | | AgentWork lifecycle | `app/api/agent-work/route.ts`, `app/api/agent-work/{start,checkpoint,finish}/route.ts`, `lib/agent-work.ts` | +| Fenced capacity handoff (#1256) | `app/api/issues/capacity-handoff/route.ts`, `lib/issue-capacity-handoff.ts`, `lib/capacity-handoff.ts`, `lib/pr-fix-queue.ts` (report resolver), `lib/lease.ts` (claim token), [docs/capacity-handoff.md](./docs/capacity-handoff.md) | | Auth & audit actor identity | `lib/auth.ts` (`authorizeRequest`, `getAuthorizedActor`) | | Board UI component tree | `app/board/page.tsx` → `kanban-board-client.tsx` → `components/kanban-board.tsx` → `components/issue-card.tsx` | | App shell / header / nav | `app/layout.tsx` | @@ -315,9 +317,14 @@ The `tasks/report` endpoint accepts these outcomes: | `no_changes_needed` | No action was required | | `already_addressed` | A PR-fix attempt whose feedback was already handled — settles the queue item with an optional evidence string (commit SHAs/paths) and no new push (#1121). Still gated on PR merge state; a repeated same-evidence disagreement with an unchanged head reopens via #940 and bounds to BLOCKED at `PR_FIX_MAX_ATTEMPTS`. | | `handed-off` | The worker accepted the task but handed it to another process; records a neutral terminal run status and preserves supplied issue/PR identity without settling the PR-fix queue. Independent pr-followup sync owns ingestion. | +| `capacity-exhausted` | Explicit recognition that the worker's execution capacity cannot run the handed PR-fix attempt (#1256). Requires the issued `prFixItem` token (including its per-hand-out `handoutToken` receipt) and a `capacity` identifier; releases exactly the issued hand-out without spending repair budget and records a capacity-scoped cooldown. For claimed issue work use `POST /api/issues/capacity-handoff` instead. | Queue-backed `followup-pr` reports must echo the `prFixItem.{id, generation}` token issued by `next-task` on the task. Reports without it still record the AgentRun, but never settle PR-fix queue state — the item stays queued and the report is a no-op against the queue (#1074). +Workers may report `outcome: "capacity-exhausted"` only when a recognized execution capacity is genuinely unavailable, with a non-empty `capacity` identifier (an opaque worker-chosen name, for example a provider/model or runner pool) and the issued `prFixItem: { id, generation, handoutToken }` token. Dispatch fences the settlement to the **exact issued hand-out**: the `handoutToken` is a per-hand-out receipt next-task mints (rotated on every issuance), and the release removes only the reporting agent's receipt/hand-out/ack — other agents' live hand-outs at that generation are preserved, and the hand-out stamps clear only when the last live hand-out is consumed. While the item is still `QUEUED` the attempt token is consumed but the item **stays `QUEUED` at the same generation with the same `fixAttempts` count, branch, and PR identity** — no repair budget is spent and there is no global wait. The suppression is **capacity-scoped**: Dispatch records a bounded cooldown (`DISPATCH_CAPACITY_COOLDOWN_MS`, default 15m, clamped to 1m–6h) for that capacity on that item only. `next-task` requests may declare `?capacity=`; a request whose capacity matches the exhausted one skips the item until the cooldown expires, an unrelated capacity is never suppressed, and a request that declares no capacity conservatively skips cooled items. The capacity is caller-declared trusted *scoping*, never identity: settlement authority comes only from the issued receipt token, the cooldown deliberately does not filter by agent (the same worker with a different, unrelated capacity is legitimately allowed), and lying about a capacity grants nothing beyond re-acquiring work for the capacity one claims to have exhausted. A `capacity_handoff` history row and a `CapacityHandoff` row record the transition. A changed generation, an already-settled status, or a human hold (BLOCKED/PARKED/NEEDS_HUMAN) is never overwritten — the report settles as a no-op. Do not use this outcome for worker-local failures, ordinary errors, or an unrecognized capacity. + +For **claimed issue work** the fenced counterpart is `POST /api/issues/capacity-handoff` (bearer-auth; bound workers only for their own agent). The worker presents the `claimToken` that `POST /api/issues/claim` issued on the claim (also surfaced by `active-work` resume context and the MCP `claim_issue` result). Dispatch fences the release to that exact claim (agent + lease row + token, fence verified against the live GitHub labels and re-verified after the lease CAS — a force-claim that lands mid-flight narrows the release to ownership-only, never a status write) and refuses human holds (closed/done, any live `status/blocked`, a cached block carrying a `blockedReason`) and changed ownership (a different or missing `agent/*` label) with 409 and zero writes. On success the `agent/*` label is removed, the resting status follows the shared unclaim policy (in-progress → `status/ready`, immediately discoverable to another compatible worker), the lease is consumed, active AgentWork rows are retired with history, an audit row is written, and the same capacity-scoped cooldown applies to issue pickup. Branch/PR identity on the lease and work history is retained; nothing is re-pushed and no repair budget exists for issue work. See [docs/capacity-handoff.md](./docs/capacity-handoff.md) for the lease/label effects and the GitHub-vs-database consistency boundary. + The report body also accepts an optional `startedAt` — an extended ISO 8601 timestamp with timezone (`Z` or `±HH:MM`), for example `2026-10-03T04:20:58Z` or `2026-10-03T04:20:58.123456+00:00`, marking when the worker began the task. When it parses, is not in the future, and is no more than 24 hours before the report, it is stored as the run's start time so the run records a real duration; missing, malformed, future, or too-old values silently fall back to the report time (never a 400). The raw body value participates in the idempotency payload identity, so an identical-body retry always replays as a duplicate. An accepted `startedAt` is normalized to UTC (`Z`) form when stored and echoed back in the response. The echo reflects re-validation of the current request, so on an idempotent duplicate replay the echoed `startedAt` may be absent even though the original accepted run stored it — for example when an identical-body retry lands more than 24 hours after the reported start (the stored AgentRun is unaffected). #### Idempotent reporting (optional) diff --git a/docs/capacity-handoff.md b/docs/capacity-handoff.md new file mode 100644 index 00000000..edc688e6 --- /dev/null +++ b/docs/capacity-handoff.md @@ -0,0 +1,207 @@ +# Fenced Capacity-Exhaustion Handoff (#1256) + +Dispatch-native recovery for the case "the worker holding a handed attempt +cannot actually run it because its execution capacity is exhausted". The work +must return to circulation **for other compatible workers** without letting a +stale worker stomp a newer claim, a human hold, or anyone's retry budget. + +This is a generic Dispatch capability. The `capacity` identifier is an opaque, +worker-chosen string (for example `provider/model-a` or `runner-pool-gpu`); +Dispatch never interprets it and carries no coupling to any particular +executor, provider, or source product. + +## The two surfaces + +| Work form | Operation | Fencing token (issued by) | +|---|---|---| +| PR-fix queue attempt | `POST /api/agents/{agent}/tasks/report` with `outcome: "capacity-exhausted"`, `capacity`, and the issued `prFixItem: { id, generation, handoutToken }` | `next-task` hand-out (#1074 token + #1256 per-hand-out receipt token) | +| Claimed issue | `POST /api/issues/capacity-handoff` with `issueId`, `repoFullName`, `issueNumber`, `agentName`, `claimToken`, `capacity` | `POST /api/issues/claim` response `claimToken` (also in the `active-work` resume context) | + +Both require bearer auth; a bound worker credential (`DISPATCH_WORKER_TOKENS`) +may only hand off its **own** agent's work, and the unbound legacy worker token +is refused (#1129). A handoff must be an *explicit* recognition: a non-empty, +bounded (≤200 chars) `capacity` and a valid issued token are hard 400 gates. + +## Attempt tokens and fencing + +* **PR-fix**: settlement is a compare-and-set on `(item id, status QUEUED, + generation)` — a stale generation (a fresh attempt was re-issued meanwhile), + an already-settled status, or a human hold (BLOCKED / PARKED / NEEDS_HUMAN) + settles as a no-op; the report never overwrites newer state. For + `capacity-exhausted` the CAS is bound further, to the **exact source + attempt**: next-task mints a fresh opaque `handoutToken` per hand-out and + persists it as a receipt (`PrFixQueueItem.handoutReceipts`, one entry per + live hand-out, format `token@agent@generation`). The report must present + that token, and the write — pinned on the full receipt/hand-out/ack lists — + removes **only the reporting agent's** receipt, hand-out record, and own + ack. An unrelated agent that merely knows `(id, generation)` has no + authority, and neither does the *same* agent presenting a token from a + **previous** hand-out of the same generation (release keeps the generation, + so generation alone cannot distinguish hand-outs). A receipt exists exactly + while its `agentHandouts` entry exists — issuance pushes both in one + conditional write and every clear site clears both — so a released, + reclaimed, or generation-superseded hand-out can never be settled with its + old token. Pre-receipt (legacy) hand-outs cannot settle this outcome; every + other outcome ignores the token and settles unchanged. +* **Issue**: the claim token lives on the `Lease` row (`Lease.claimToken`, + unique, minted at lease creation). Renewals deliberately keep the token (a + heartbeating worker keeps its receipt), but a released or expired lease that + is re-claimed is a **new row with a new token** — an old worker's token is + permanently inert against a newer claim. The release itself is a CAS + `deleteMany` on `(lease id, agentName, issueId, expiredAt > now)`: exactly + one racing release wins; losers get 409 with zero writes. + +## Human-hold and ownership safeguards (issue surface) + +Checked **before** any write; every refusal is 409/400 with zero mutation: + +* closed issue, cached or **live** `status/done`; +* deliberate block: cached `status/blocked` carrying a `blockedReason`, or + **any live `status/blocked`** — fail-closed: a block applied on GitHub that + has not reached the cache is still a hold, and releasing it is an explicit + operator action, never a side effect of a capacity handoff; +* ownership changed: the **live GitHub** label set (not the cache — #1037 + rule) carries a *different* `agent/*` label, or *no* agent label at all + (already released or human-cleared). +* GitHub unreachable → 503 fail-closed: no lease delete, no label write. + +**Post-CAS re-verification**: a force-claim (which does not delete the +previous agent's lease row) or a human change can land between the pre-CAS +read and the lease delete. After the CAS the live labels are re-read; if +ownership moved **or the re-read itself fails**, the release narrows to +**ownership-only** — our own `agent/*` label is removed idempotently and the +cache mirrors the freshest known set minus that label — and no status label +is ever touched, so a newer owner's transition survives. A re-read failure +must not take the full-release path: ownership is unknown, and a status +transition on a stale snapshot could clobber it. Ownership-only cleanup is +safe under total ignorance (one targeted write, our own label, narrowing +only), and the lease is already consumed, so halting would strand the claim +with no owner. + +## What success does + +**Issue handoff** (`issue_capacity_handoff` audit action): + +1. Lease row CAS-deleted — this is the atomic ownership transition. +2. Claim released through the shared `releaseIssueClaim` policy, based on the + live labels: `agent/` removed; `status/in-progress` → `status/ready` + (immediately discoverable to another compatible worker); `status/in-review` + is retained with a status note. +3. Active `AgentWork` rows (CLAIMED/IN_PROGRESS/BLOCKED) for that agent+issue + are marked `RELEASED` with a `capacity_handoff` history entry. +4. A `CapacityHandoff` cooldown row is written (`kind: "issue"`). +5. Audit row with before/after labels, capacity, cooldown deadline. + +**PR-fix handoff** (`capacity_handoff` PrFixHistory action): + +1. Exact-owner CAS (see “Attempt tokens and fencing”): removes the reporting + agent's receipt, hand-out record, and own ack — and nothing else. Other + agents' live receipts, hand-out records, and acknowledgements at the same + generation are preserved, and the dispatch stamps + (`dispatchedGeneration/At`) plus `postDispatchEvidenceKeys` survive while + any *other* hand-out at that generation is still live (clearing them would + reclassify the surviving workers' mid-run evidence as pre-dispatch, #1119); + only the report that consumes the last live hand-out resets them, making + the generation freshly dispatchable. The item **stays `QUEUED`** at the + same `generation` and `fixAttempts`, with `branch`, `url`, and PR state + untouched — the attempt is returned, not burned, and no repair budget is + spent. It is explicitly *not* a `WAITING_EXTERNAL` park: nothing hides the + item from workers whose capacity is unrelated. +2. A `CapacityHandoff` row is written (`kind: "pr_fix"`, recording the + settled `generation`). +3. Idempotency is layered: a repeat report of the **same capacity at the + same generation** while the cooldown is active replays as `deferred` + without minting a second window; concurrent duplicates are fenced by the + consumed receipt (only one write can remove the exact token) and by the + CAS pinning the issued hand-out stamp (`dispatchedGeneration = + generation`) and the full receipt/hand-out/ack lists, so exactly one + duplicate can win and losers settle as no-ops with zero mutation. A + *different* capacity exhausting the same live generation records its own + scoped cooldown (it was legitimately dispatched alongside or after the + first hand-off, with its own receipt); an older generation's window never + absorbs a fresh attempt. + +## Capacity-scoped cooldown + +**Trust boundary.** The `capacity` identifier is **caller-declared and +trusted for scoping only** — it is a shaping hint, not an identity claim. +Settlement authority comes exclusively from the issued receipt/claim token, +never from the capacity string, and the cooldown deliberately does **not** +filter by agent: the same worker legitimately declares different capacities +(a different model or runner pool behind one agent name), and an unrelated +capacity must keep receiving the work. A worker that lies about its capacity +can only re-acquire work for a capacity it claims to have exhausted — it +consumes no extra repair budget, and its own hand-out is fenced from anyone +else. Given the active +(`cooldownUntil > now`) handoff rows: + +* requester's capacity **equals** the exhausted capacity (matched + case-insensitively — the identifier is opaque and `Model-A` re-declared is + the same capacity) → the cooled item is skipped until the deadline; +* requester declares an **unrelated** capacity → the item dispatches normally + (this is the entire point of scoping the cooldown); +* requester declares **no capacity** → conservatively skips *any* cooled item + (Dispatch cannot prove it is not the exhausted capacity). Legacy workers are + thus protected until expiry without globalizing the wait for declared, + unrelated capacities. + +Cooldown bound: `DISPATCH_CAPACITY_COOLDOWN_MS` (default 15 minutes, clamped +to [1 minute, 6 hours]). Expired rows are pruned lazily on read, throttled to +once per minute per process so the prune never becomes a per-poll write on the +dispatch hot path. When every remaining candidate is cooled, `next-task` says +so in the idle reason instead of a bare "No work available". A cooldown read +failure **fails open** (dispatch proceeds unfiltered): handing cooled work to +the exhausted capacity is a bounded nuisance the worker reports again; +blocking the whole lane is not. Symmetrically, a cooldown **write** failure +after the release already committed does not fail the handoff — the response +returns `cooldownUntil: null` (audited), because the ownership release is the +operation the worker asked for and a false 500 would invite a retry that can +only 409. + +The exhausted capacity's own re-report is also fenced: the PR-fix attempt +token is consumed and the issue lease is gone, so the same worker cannot +double-settle; at worst it re-receives the item after the cooldown, which is +the intended bounded retry for that capacity. + +## Label / lease effects and consistency boundaries + +**GitHub is the source of truth for the issue's labels and status. The +database (Issue cache, Lease, AgentWork, CapacityHandoff, PrFixQueueItem) is +Dispatch's coordination layer — never write it as if GitHub were behind.** + +* Ownership is *verified* against live GitHub labels before any mutation; the + Prisma cache is used only for non-label facts (`blockedReason`, + `linkedPrNumber`, closed state) and the post-write mirror. +* Label writes are targeted (`removeIssueLabel` for the agent label, + `transitionIssueStatus` for the status swap), never a full label-set + replacement, so concurrent human label changes on GitHub are not reverted. +* The **lease CAS delete is the commit point** for issue ownership. If a + label write fails *after* it, the worker has definitively lost ownership and + the residue (a stale `agent/*` label) is owned by the existing stale-claim + recovery / sync paths — the failure is audited (`success: false`, pointer in + `errorMessage`) and never resurrects the claim or blocks rediscovery. +* `CapacityHandoff` rows are Dispatch-local dispatch shaping. They are not + mirrored to GitHub, carry no label, and self-expire; deleting them is always + safe. +* The PR-fix cooldown deliberately does not touch PR state on GitHub at all: + no label, no comment, no push. Queue-row changes are cache-side work + bookkeeping; the PR/branch identity is retained exactly. +* Lease expiry alone never clears labels — expiry is a *staleness* signal + owned by stale-claim recovery; capacity handoff is an *intentional* release + owned by the worker holding the token. + +## Failure-mode summary + +| Situation | Result | +|---|---| +| Missing/blank capacity, attempt token, or receipt token (PR-fix) | 400 | +| PR-fix receipt token not owned by the reporting agent (unknown, rotated, or another agent's) | report `skipped`, queue untouched | +| Wrong agent's token (bound worker) | 403 | +| GitHub labels unreadable | 503, zero writes | +| Stale claim token (re-claimed lease) | 409 `stale_claim_token` | +| Expired lease | 409 `claim_expired` (stale recovery owns release) | +| Different/missing live `agent/*` label | 409 `ownership_changed` | +| Deliberate block / done / closed | 409/400 human hold | +| Lease CAS lost (concurrent release) | 409 `handoff_race_lost`, zero writes | +| PR-fix generation moved / status settled | report `skipped`, queue untouched | +| Repeat PR-fix report | `deferred`, one cooldown window only | diff --git a/docs/worker-execution-contract.md b/docs/worker-execution-contract.md index bd75adfa..4549b7d1 100644 --- a/docs/worker-execution-contract.md +++ b/docs/worker-execution-contract.md @@ -316,3 +316,26 @@ Behavioral notes: `status: "FIXED"` + `alreadyAddressed: true` + optional `evidence` (bearer marks still require a generation; the QUEUED/IGNORED maintainer-only gate is unchanged). + +## Fenced capacity-exhaustion handoff (#1256) + +A worker whose execution capacity cannot run a handed attempt returns it with +an explicit, token-fenced handoff instead of failing or silently stalling: + +- PR-fix attempt: `tasks/report` `outcome: "capacity-exhausted"` + issued + `prFixItem` token (including its per-hand-out `handoutToken` receipt) + + `capacity`. The release is fenced to the exact issued hand-out: only that + hand-out is removed, other agents' live hand-outs at the generation survive, + and the item stays QUEUED at the same generation and fixAttempts; the + cooldown is scoped to the exhausted capacity only (trusted caller-declared + scoping — never identity, never agent-filtered). +- Claimed issue: `POST /api/issues/capacity-handoff` with the `claimToken` + issued by the claim. Release is fenced to the exact claim and refuses human + holds and changed ownership. + +Full contract, lease/label effects, and the GitHub-vs-database consistency +boundary: [capacity-handoff.md](./capacity-handoff.md). + +## History +- **2026-10-14** — Review follow-up (#1256): per-hand-out `handoutToken` receipts bind capacity-exhausted settlement to the exact issued source attempt (unrelated agents and rotated same-agent tokens are refused); the release preserves other agents' live receipts/acks and the dispatch stamps while any hand-out is live; a failed post-CAS live-label re-read now takes the ownership-only path (never a status transition). +- **2026-10-14** — Added fenced capacity-exhaustion handoff (#1256): capacity-scoped cooldowns for PR-fix attempts and claim-token-fenced issue recovery. diff --git a/prisma/migrations/20261014000000_capacity_handoff_recovery/migration.sql b/prisma/migrations/20261014000000_capacity_handoff_recovery/migration.sql new file mode 100644 index 00000000..1d4ba8e7 --- /dev/null +++ b/prisma/migrations/20261014000000_capacity_handoff_recovery/migration.sql @@ -0,0 +1,32 @@ +-- Fenced capacity-exhaustion handoff (#1256) + +-- Claim/attempt fencing token on the issue lease. +ALTER TABLE "Lease" ADD COLUMN "claimToken" TEXT; + +CREATE UNIQUE INDEX "Lease_claimToken_key" ON "Lease"("claimToken"); + +-- Capacity-scoped bounded cooldowns for explicit capacity exhaustion. +CREATE TABLE "CapacityHandoff" ( + "id" TEXT NOT NULL, + "capacity" TEXT NOT NULL, + "kind" TEXT NOT NULL, + "repoFullName" TEXT NOT NULL, + "issueNumber" INTEGER, + "prNumber" INTEGER, + "agentName" TEXT NOT NULL, + "issueId" TEXT, + "generation" INTEGER, + "cooldownUntil" TIMESTAMP(3) NOT NULL, + "note" TEXT, + "createdAt" TIMESTAMP(3) NOT NULL DEFAULT CURRENT_TIMESTAMP, + + CONSTRAINT "CapacityHandoff_pkey" PRIMARY KEY ("id") +); + +CREATE INDEX "CapacityHandoff_capacity_cooldownUntil_idx" ON "CapacityHandoff"("capacity", "cooldownUntil"); +CREATE INDEX "CapacityHandoff_kind_repoFullName_issueNumber_cooldownUntil_idx" ON "CapacityHandoff"("kind", "repoFullName", "issueNumber", "cooldownUntil"); +CREATE INDEX "CapacityHandoff_kind_repoFullName_prNumber_cooldownUntil_idx" ON "CapacityHandoff"("kind", "repoFullName", "prNumber", "cooldownUntil"); +CREATE INDEX "CapacityHandoff_cooldownUntil_idx" ON "CapacityHandoff"("cooldownUntil"); + +-- Per-hand-out receipts for exact source-attempt fencing (#1256 review). +ALTER TABLE "PrFixQueueItem" ADD COLUMN "handoutReceipts" TEXT[] NOT NULL DEFAULT ARRAY[]::TEXT[]; diff --git a/prisma/schema.prisma b/prisma/schema.prisma index 721d9b8d..2caca7dc 100644 --- a/prisma/schema.prisma +++ b/prisma/schema.prisma @@ -186,6 +186,19 @@ model PrFixQueueItem { // wall-clock of that hand-out. dispatchedGeneration Int? dispatchedAt DateTime? + // #1256: authoritative per-hand-out receipts. One entry per live hand-out + // issued by next-task, format "@@" (token is + // an opaque random value containing no "@"; the agent name may contain "@", + // so the token boundary is the FIRST "@" and the generation the LAST). + // Settlement of a capacity-exhausted report requires the reporting agent to + // present the exact token it was issued, and the write releases only that + // agent's receipt — other agents' live hand-outs and acknowledgements at + // the same generation survive. Invariant: an entry exists exactly while the + // matching `agentHandouts` entry exists (issuance pushes both atomically; + // every clear site clears both), so a hand-out that was released, reclaimed, + // or superseded by a generation bump can never be settled with its old + // token, and each new hand-out rotates in a fresh token. + handoutReceipts String[] @default([]) // New (previously-unrecorded) evidence arrived AFTER the current generation // was handed out (#1119): the in-flight worker cannot see it, so settlement // must reopen a fresh attempt instead of absorbing it. Records the evidence @@ -805,6 +818,11 @@ model Lease { prUrl String? // associated PR URL if one exists expiredAt DateTime // absolute expiry; default TTL is set at creation time renewedAt DateTime? + // #1256: opaque attempt/claim token issued to the worker on claim. It fences + // a capacity-handoff release to the EXACT claim: renewals keep the token, + // but a released/expired lease that is re-claimed gets a fresh row and a + // fresh token, so a stale worker's token can never release a newer claim. + claimToken String? @unique createdAt DateTime @default(now()) @@unique([agentName, issueId]) @@ -812,3 +830,41 @@ model Lease { @@index([expiredAt]) @@index([issueId]) } + +// --------------------------------------------------------------------------- +// Capacity handoffs (#1256) — bounded, capacity-SCOPED cooldowns. +// +// When a worker explicitly recognizes that its execution capacity (an opaque +// provider/model/runner identifier chosen by the worker) cannot run a handed +// attempt, it settles the attempt with a fenced handoff. Dispatch keeps the +// work item itself discoverable to other compatible workers; this table only +// records "capacity C should not be re-handed until T". Workers +// that declare no capacity conservatively skip cooled items (fail-closed). +// Expired rows are pruned lazily on read; they carry no retry budget. +// --------------------------------------------------------------------------- + +model CapacityHandoff { + id String @id @default(cuid()) + capacity String // opaque worker-declared capacity identifier + // Which subsystem owns the handed-off work: "issue" (claim/lease release) + // or "pr_fix" (PR-fix queue attempt). + kind String + repoFullName String + issueNumber Int? + prNumber Int? + // The worker that handed the work off, for audit/history. + agentName String + issueId String? // issue-form handoffs: the local Issue row + // The attempt generation the handoff settles (pr_fix rows). The replay + // check is keyed on it so a fresh attempt is never absorbed by an older + // generation's cooldown window. + generation Int? + cooldownUntil DateTime // handoff suppresses re-hand to `capacity` before this time + note String? @db.Text + createdAt DateTime @default(now()) + + @@index([capacity, cooldownUntil]) + @@index([kind, repoFullName, issueNumber, cooldownUntil]) + @@index([kind, repoFullName, prNumber, cooldownUntil]) + @@index([cooldownUntil]) +} diff --git a/src/app/api/agents/[agentName]/next-task/route.test.ts b/src/app/api/agents/[agentName]/next-task/route.test.ts index 5939c9ff..0a21cf00 100644 --- a/src/app/api/agents/[agentName]/next-task/route.test.ts +++ b/src/app/api/agents/[agentName]/next-task/route.test.ts @@ -18,6 +18,8 @@ const { mocks } = vi.hoisted(() => ({ linkedRows: [] as any[], fetchPullRequestLabels: vi.fn().mockResolvedValue([]), fetchPullRequestHeadSha: vi.fn().mockResolvedValue(null), + capacityHandoffFindMany: vi.fn().mockResolvedValue([]), + capacityHandoffDeleteMany: vi.fn().mockResolvedValue({ count: 0 }), }, })); @@ -38,6 +40,10 @@ vi.mock("@/lib/prisma", () => ({ updateMany: mocks.prFixUpdateMany, }, prFixHistory: { create: mocks.prFixHistoryCreate }, + capacityHandoff: { + findMany: mocks.capacityHandoffFindMany, + deleteMany: mocks.capacityHandoffDeleteMany, + }, $transaction: async (fn: any) => fn({ prFixQueueItem: { create: mocks.prFixCreate, @@ -426,7 +432,7 @@ describe("GET /api/agents/[agentName]/next-task", () => { const body = await res.json(); expect(body.type).toBe("followup-pr"); - expect(body.prFixItem).toEqual({ id: "prfix-1", generation: 3 }); + expect(body.prFixItem).toMatchObject({ id: "prfix-1", generation: 3 }); expect(body.prFixExclusions).toEqual({ version: 1, skipped: [] }); }); @@ -597,7 +603,7 @@ describe("GET /api/agents/[agentName]/next-task", () => { expect(body.pullRequest.repoFullName).toBe("org/repo"); expect(body.pullRequest.number).toBe(12); expect(body.pullRequest.url).toBe("https://github.com/org/repo/pull/12"); - expect(body.prFixItem).toEqual({ + expect(body.prFixItem).toMatchObject({ id: "prfix-1", generation: 1, }); @@ -632,7 +638,7 @@ describe("GET /api/agents/[agentName]/next-task", () => { { params: Promise.resolve({ agentName: "example-agent" }) }, ); const firstBody = await first.json(); - expect(firstBody.prFixItem).toEqual({ id: "prfix-1", generation: 1 }); + expect(firstBody.prFixItem).toMatchObject({ id: "prfix-1", generation: 1 }); // Same queue row after an explicit requeue — same id, bumped generation. item.generation = 2; @@ -642,7 +648,7 @@ describe("GET /api/agents/[agentName]/next-task", () => { ); const secondBody = await second.json(); expect(secondBody.type).toBe("followup-pr"); - expect(secondBody.prFixItem).toEqual({ id: "prfix-1", generation: 2 }); + expect(secondBody.prFixItem).toMatchObject({ id: "prfix-1", generation: 2 }); expect(secondBody.prFixItem.id).toBe(firstBody.prFixItem.id); }); @@ -701,7 +707,7 @@ describe("GET /api/agents/[agentName]/next-task", () => { const due = await poll(); const body = await due.json(); expect(body.type).toBe("followup-pr"); - expect(body.prFixItem).toEqual({ id: waiting.id, generation: issuedGeneration + 1 }); + expect(body.prFixItem).toMatchObject({ id: waiting.id, generation: issuedGeneration + 1 }); expect(waiting.status).toBe("QUEUED"); expect(waiting.generation).toBeGreaterThan(issuedGeneration); expect(waiting.fixAttempts).toBeUndefined(); @@ -748,7 +754,7 @@ describe("GET /api/agents/[agentName]/next-task", () => { ); const secondBody = await second.json(); expect(secondBody.type).toBe("followup-pr"); - expect(secondBody.prFixItem).toEqual({ id: itemId, generation: 2 }); + expect(secondBody.prFixItem).toMatchObject({ id: itemId, generation: 2 }); }); it("stamps the hand-out with a generation-conditional update (#1119)", async () => { @@ -848,6 +854,10 @@ describe("GET /api/agents/[agentName]/next-task", () => { expect(row.dispatchedAt).toBeInstanceOf(Date); expect(row.postDispatchEvidenceKeys).toEqual([]); expect(row.agentHandouts).toEqual(["example-agent@2"]); + // #1256 review: the hand-out pushed a per-hand-out receipt alongside the + // record, and the task carries its token back to the worker. + const firstIssued = /^(hnd_[0-9a-f]+)@example-agent@2$/.exec(row.handoutReceipts[0] ?? ""); + expect(firstIssued).not.toBeNull(); const stampedAt = row.dispatchedAt; // (b) New evidence enqueues mid-run, after the dispatch. @@ -862,7 +872,12 @@ describe("GET /api/agents/[agentName]/next-task", () => { ); const secondBody = await second.json(); expect(secondBody.type).toBe("followup-pr"); - expect(secondBody.prFixItem).toEqual({ id: "prfix-1", generation: 2 }); + expect(secondBody.prFixItem).toMatchObject({ id: "prfix-1", generation: 2 }); + expect(typeof secondBody.prFixItem.handoutToken).toBe("string"); + // The token rotates per hand-out: B's receipt is a distinct entry from A's. + expect(row.handoutReceipts).toHaveLength(2); + expect(row.handoutReceipts[1]).toBe(`${secondBody.prFixItem.handoutToken}@other-agent@2`); + expect(row.handoutReceipts[0]).not.toBe(row.handoutReceipts[1]); expect(row.postDispatchEvidenceKeys).toEqual(["review:o/r#7:r2@H1"]); expect(row.dispatchedGeneration).toBe(2); expect(row.dispatchedAt).toBe(stampedAt); @@ -962,7 +977,7 @@ describe("GET /api/agents/[agentName]/next-task", () => { const body = await res.json(); // The already-handed item is skipped; the NEXT pr-fix item ships. expect(body.type).toBe("followup-pr"); - expect(body.prFixItem).toEqual({ id: "prfix-2", generation: 1 }); + expect(body.prFixItem).toMatchObject({ id: "prfix-2", generation: 1 }); expect(handed.agentHandouts).toEqual(["example-agent@2"]); expect(fresh.agentHandouts).toEqual(["example-agent@1"]); }); @@ -1118,7 +1133,7 @@ describe("GET /api/agents/[agentName]/next-task", () => { request("/api/agents/example-agent/next-task?lane=local"), { params: Promise.resolve({ agentName: "example-agent" }) }, ); - expect((await first.json()).prFixItem).toEqual({ id: "prfix-1", generation: 2 }); + expect((await first.json()).prFixItem).toMatchObject({ id: "prfix-1", generation: 2 }); expect(row.agentHandouts).toEqual(["example-agent@2"]); // The item is re-issued as a fresh attempt (requeue/reopen): generation @@ -1133,7 +1148,7 @@ describe("GET /api/agents/[agentName]/next-task", () => { ); const secondBody = await second.json(); expect(secondBody.type).toBe("followup-pr"); - expect(secondBody.prFixItem).toEqual({ id: "prfix-1", generation: 3 }); + expect(secondBody.prFixItem).toMatchObject({ id: "prfix-1", generation: 3 }); expect(row.agentHandouts).toEqual(["example-agent@3"]); }); @@ -1182,7 +1197,7 @@ describe("GET /api/agents/[agentName]/next-task", () => { const body = await res.json(); expect(body.type).toBe("followup-pr"); - expect(body.prFixItem).toEqual({ id: "prfix-1", generation: 2 }); + expect(body.prFixItem).toMatchObject({ id: "prfix-1", generation: 2 }); expect(row.agentHandouts).toEqual(["some-other-agent@2", "example-agent@2"]); }); @@ -1301,7 +1316,7 @@ describe("GET /api/agents/[agentName]/next-task", () => { expect(stamps[1].generation).toBe(3); // The handed-out token is the NEW generation, so the worker's settle // report matches the live row instead of being rejected as a mismatch. - expect(body.prFixItem).toEqual({ id: "prfix-1", generation: 3 }); + expect(body.prFixItem).toMatchObject({ id: "prfix-1", generation: 3 }); // The payload reasons come from the latest read, not the queue snapshot. expect(body.reasons).toContain("CI failure on main (re-issued)"); expect(body.reasons).toContain("fix lint"); @@ -1490,7 +1505,7 @@ describe("GET /api/agents/[agentName]/next-task", () => { expect(body.type).toBe("followup-pr"); // Best-effort: the task is served with the pre-stamp snapshot's // generation and reasons. - expect(body.prFixItem).toEqual({ id: "prfix-1", generation: 2 }); + expect(body.prFixItem).toMatchObject({ id: "prfix-1", generation: 2 }); expect(body.reasons).toContain("CI failure on main"); expect(body.reasons).toContain("update tests"); }); @@ -1530,7 +1545,7 @@ describe("GET /api/agents/[agentName]/next-task", () => { expect(res.status).toBe(200); const body = await res.json(); expect(body.type).toBe("followup-pr"); - expect(body.prFixItem).toEqual({ id: "prfix-1", generation: 2 }); + expect(body.prFixItem).toMatchObject({ id: "prfix-1", generation: 2 }); expect(body.reasons).toContain("CI failure on main"); expect(body.reasons).toContain("update tests"); // Only present in the re-read row — proves the payload reasons come from @@ -1785,7 +1800,7 @@ describe("GET /api/agents/[agentName]/next-task", () => { const body = await res.json(); expect(body.type).toBe("followup-pr"); expect(body.shouldRun).toBe(true); - expect(body.prFixItem).toEqual({ id: "linked-prfix", generation: 1 }); + expect(body.prFixItem).toMatchObject({ id: "linked-prfix", generation: 1 }); expect(mocks.prFixCreate).toHaveBeenCalledTimes(1); expect(mocks.prFixHistoryCreate).toHaveBeenCalledTimes(1); }); @@ -1820,7 +1835,7 @@ describe("GET /api/agents/[agentName]/next-task", () => { const body = await res.json(); expect(body.type).toBe("followup-pr"); - expect(body.prFixItem).toEqual({ id: "existing-prfix", generation: 3 }); + expect(body.prFixItem).toMatchObject({ id: "existing-prfix", generation: 3 }); expect(mocks.prFixCreate).not.toHaveBeenCalled(); expect(mocks.fetchPullRequestLabels).not.toHaveBeenCalled(); }); @@ -1834,7 +1849,7 @@ describe("GET /api/agents/[agentName]/next-task", () => { ); const firstBody = await first.json(); expect(firstBody.type).toBe("followup-pr"); - expect(firstBody.prFixItem).toEqual({ id: "linked-prfix", generation: 1 }); + expect(firstBody.prFixItem).toMatchObject({ id: "linked-prfix", generation: 1 }); expect(mocks.prFixHistoryCreate).toHaveBeenCalledTimes(1); const second = await GET( @@ -1851,7 +1866,12 @@ describe("GET /api/agents/[agentName]/next-task", () => { id: "linked-prfix", generation: 1, status: "QUEUED", NOT: { agentHandouts: { has: "example-agent@1" } }, }), - data: { agentHandouts: { push: ["example-agent@1"] } }, + // #1256 review: the hand-out record and its per-hand-out receipt are + // pushed in the same conditional write (token rotates per issuance). + data: expect.objectContaining({ + agentHandouts: { push: ["example-agent@1"] }, + handoutReceipts: { push: [expect.stringMatching(/^hnd_[0-9a-f]+@example-agent@1$/)] }, + }), })); }); @@ -1990,7 +2010,7 @@ describe("GET /api/agents/[agentName]/next-task", () => { const body = await res.json(); expect(body.type).toBe("followup-pr"); expect(body.pullRequest.number).toBe(16); - expect(body.prFixItem).toEqual({ id: "linked-prfix-2", generation: 1 }); + expect(body.prFixItem).toMatchObject({ id: "linked-prfix-2", generation: 1 }); }); it("materializes linked-only work for a lane without a PR-fix equivalent, without handing it to that lane", async () => { @@ -2663,3 +2683,142 @@ describe("GET /api/agents/[agentName]/next-task — bound worker scope (#1129)", expect(res.status).toBe(403); }); }); + +describe("GET /api/agents/[agentName]/next-task — capacity-scoped handoff cooldowns (#1256)", () => { + beforeEach(() => { + delete process.env.DISPATCH_AUTH_MODE; + resetAuthCaches(); + vi.clearAllMocks(); + mocks.prFixFindMany.mockResolvedValue([]); + mocks.issueFindMany.mockResolvedValue([]); + mocks.findLeasedIssueIds.mockResolvedValue([]); + mocks.prFixUpdateMany.mockResolvedValue({ count: 1 }); + mocks.prFixFindUnique.mockResolvedValue(null); + mocks.prFixFindUniqueByRepo.mockResolvedValue(null); + mocks.capacityHandoffFindMany.mockResolvedValue([]); + mocks.capacityHandoffDeleteMany.mockResolvedValue({ count: 0 }); + }); + + function cooledPrRow(): any { + return { + id: "prfix-cooled", + repo: "org/repo", + pr: 70, + issue: null, + branch: "work/70", + url: "https://github.com/org/repo/pull/70", + lane: "NORMAL", + status: "QUEUED", + reason: "review changes requested", + feedback: [], + generation: 1, + queuedAt: new Date("2026-01-01T00:00:00Z"), + updatedAt: new Date("2026-01-01T00:00:00Z"), + dispatchedAt: null, + dispatchedGeneration: null, + postDispatchEvidenceKeys: [], + agentHandouts: [], + }; + } + + it("rejects a malformed capacity query", async () => { + const res = await GET( + request(`/api/agents/example-agent/next-task?capacity=${"x".repeat(250)}`), + { params: Promise.resolve({ agentName: "example-agent" }) }, + ); + expect(res.status).toBe(400); + }); + + it("does not re-hand a cooled PR-fix attempt to the exhausted capacity, but hands it to an unrelated capacity", async () => { + const row = cooledPrRow(); + mocks.prFixFindMany.mockResolvedValue([row]); + mocks.prFixUpdateMany.mockImplementation(async ({ where, data }: any) => { + if (row.id !== where.id) return { count: 0 }; + if (where.generation !== undefined && row.generation !== where.generation) return { count: 0 }; + applyUpdate(row, data); + return { count: 1 }; + }); + mocks.prFixFindUnique.mockImplementation(async () => ({ + reason: row.reason, feedback: row.feedback, generation: row.generation, + status: row.status, lane: row.lane, agentHandouts: row.agentHandouts, + })); + mocks.capacityHandoffFindMany.mockResolvedValue([ + { kind: "pr_fix", capacity: "model-a", repoFullName: "org/repo", issueNumber: null, prNumber: 70 }, + ]); + + // The exhausted capacity is suppressed; with no other work it goes idle. + const cooled = await GET( + request("/api/agents/example-agent/next-task?lane=local&capacity=model-a"), + { params: Promise.resolve({ agentName: "example-agent" }) }, + ); + expect((await cooled.json()).type).toBe("idle"); + + // An unrelated capacity still gets the work — the cooldown is scoped. + const unrelated = await GET( + request("/api/agents/example-agent/next-task?lane=local&capacity=model-b"), + { params: Promise.resolve({ agentName: "example-agent" }) }, + ); + const body = await unrelated.json(); + expect(body.type).toBe("followup-pr"); + expect(body.pullRequest.number).toBe(70); + expect(body.prFixItem).toMatchObject({ id: row.id, generation: 1 }); + }); + + it("suppresses a cooled issue only for the exhausted capacity; undeclared workers skip conservatively", async () => { + mocks.issueFindMany.mockResolvedValue([independentIssue(101)]); + mocks.capacityHandoffFindMany.mockResolvedValue([ + { kind: "issue", capacity: "model-a", repoFullName: "org/repo", issueNumber: 101, prNumber: null }, + ]); + + const exhausted = await GET( + request("/api/agents/example-agent/next-task?lane=local&capacity=model-a"), + { params: Promise.resolve({ agentName: "example-agent" }) }, + ); + expect((await exhausted.json()).type).toBe("idle"); + + // Fail-closed for workers that declare no capacity: Dispatch cannot prove + // they are not the exhausted capacity. + const undeclared = await GET( + request("/api/agents/example-agent/next-task?lane=local"), + { params: Promise.resolve({ agentName: "example-agent" }) }, + ); + expect((await undeclared.json()).type).toBe("idle"); + + const unrelated = await GET( + request("/api/agents/example-agent/next-task?lane=local&capacity=model-b"), + { params: Promise.resolve({ agentName: "example-agent" }) }, + ); + const body = await unrelated.json(); + expect(body.type).toBe("implement"); + expect(body.issue.number).toBe(101); + }); + + it("resumes normal dispatch once no active cooldown exists (expiry is lazy)", async () => { + mocks.issueFindMany.mockResolvedValue([independentIssue(101)]); + // The route filters by `cooldownUntil > now`; expiry simply means the row + // no longer comes back from the active read. + mocks.capacityHandoffFindMany.mockResolvedValue([]); + + const res = await GET( + request("/api/agents/example-agent/next-task?lane=local&capacity=model-a"), + { params: Promise.resolve({ agentName: "example-agent" }) }, + ); + const body = await res.json(); + expect(body.type).toBe("implement"); + expect(body.issue.number).toBe(101); + }); + + it("dispatches without capacity suppression when the cooldown read fails (availability over strictness)", async () => { + mocks.issueFindMany.mockResolvedValue([independentIssue(101)]); + mocks.capacityHandoffFindMany.mockRejectedValue(new Error("db down")); + + const res = await GET( + request("/api/agents/example-agent/next-task?lane=local&capacity=model-a"), + { params: Promise.resolve({ agentName: "example-agent" }) }, + ); + expect(res.status).toBe(200); + const body = await res.json(); + expect(body.type).toBe("implement"); + expect(body.issue.number).toBe(101); + }); +}); diff --git a/src/app/api/agents/[agentName]/next-task/route.ts b/src/app/api/agents/[agentName]/next-task/route.ts index 16fb8759..f69a4889 100644 --- a/src/app/api/agents/[agentName]/next-task/route.ts +++ b/src/app/api/agents/[agentName]/next-task/route.ts @@ -10,9 +10,10 @@ import { } from "@/lib/agent-task"; import { isBacklogLane, prFixLaneForRequest } from "@/lib/lane-config"; import { fetchAgentQueueData } from "@/lib/agent-queue-fetch"; -import { agentAlreadyHanded, agentHandoutToken, createLinkedPrFixItem, normalizeQueueRepo } from "@/lib/pr-fix-queue"; +import { agentAlreadyHanded, agentHandoutToken, createLinkedPrFixItem, handoutReceipt, newHandoutToken, normalizeQueueRepo } from "@/lib/pr-fix-queue"; import { fetchPullRequestLabels, fetchPullRequestHeadSha } from "@/lib/github"; import { NEEDS_HUMAN_LABEL } from "@/lib/pr-fix-surfacing"; +import { buildCapacitySkip, loadActiveCapacityHandoffs, normalizeCapacity } from "@/lib/capacity-handoff"; /** A queued PR-fix item as the route consumes it (see toAgentQueuePrFixItem). */ type PrFixCandidate = { @@ -148,6 +149,17 @@ export async function GET( const { searchParams } = new URL(request.url); const lane = searchParams.get("lane"); + // #1256: optional worker-declared execution capacity. When set, active + // capacity handoffs only suppress re-hand of the matching capacity's work; + // when absent, a worker conservatively skips ANY cooled item (Dispatch + // cannot prove it is not the exhausted capacity). + const capacityParam = searchParams.get("capacity"); + const requestedCapacity = capacityParam === null || capacityParam.trim() === "" + ? undefined + : normalizeCapacity(capacityParam) ?? undefined; + if (capacityParam !== null && capacityParam.trim() !== "" && requestedCapacity === undefined) { + return errorResponse("capacity must be a non-empty string of at most 200 characters", 400); + } const excludeDecomposed = searchParams.get("exclude_decomposed"); const includeClaimed = searchParams.get("includeClaimed") === "true"; const includeRenovate = searchParams.get("includeRenovate") === "true"; @@ -205,6 +217,20 @@ export async function GET( if (scopeError) return scopeError; try { + // Active capacity-scoped cooldowns (#1256). Best-effort: a read failure + // must not 500 the poll (dispatching cooled work to the exhausted capacity + // is a bounded nuisance; blocking the whole lane is not), so fail open + // with no skips and log. + let skipIssueForCapacity = (_repo: string | null | undefined, _issueNumber: number) => false; + let skipPrForCapacity = (_repo: string, _prNumber: number) => false; + try { + const handoffs = await loadActiveCapacityHandoffs(prisma as any); + const skip = buildCapacitySkip(handoffs, requestedCapacity); + skipIssueForCapacity = skip.skipIssue; + skipPrForCapacity = skip.skipPr; + } catch (error) { + console.error("next-task capacity-handoff read failed; dispatching without capacity suppression:", error); + } const { laneValid, resolvedLane, rankedQueue, fullQueue, withheldQueue, admissionMode, prFixItems, availableLanes } = await fetchAgentQueueData({ agentName, @@ -233,16 +259,31 @@ export async function GET( if (excluded(candidate.repo, candidate.pr, candidate.generation)) { return null; } + // #1256: the exhausted capacity must not be re-handed the attempt it + // gave up (and an undeclared capacity is conservatively treated the + // same); unrelated capacities dispatch it normally. + if (skipPrForCapacity(candidate.repo, candidate.pr)) return null; if (agentAlreadyHanded(candidate, agentName)) return null; const confirmed = await confirmPrFixHandOut(candidate, agentName, excluded); if (!confirmed) return null; if (excluded(candidate.repo, candidate.pr, confirmed.generation)) return null; + // Mint the per-hand-out receipt token (#1256 review): every issuance + // rotates in a fresh opaque token, pushed atomically with the hand-out + // record (one conditional write — the hand-out/receipt bijection never + // drifts). The task ships it in `prFixItem.handoutToken`; only a + // capacity-exhausted report must present it back, and settlement then + // releases exactly THIS hand-out — not a newer one for the same agent + // at the same generation, and never another agent's. + const handoutToken = newHandoutToken(); try { // Conditional append fences concurrent polls for the same agent/generation. const claimed = await prisma.prFixQueueItem.updateMany({ where: { id: candidate.id, generation: confirmed.generation, status: "QUEUED", NOT: { agentHandouts: { has: agentHandoutToken(agentName, confirmed.generation) } } }, - data: { agentHandouts: { push: [agentHandoutToken(agentName, confirmed.generation)] } }, + data: { + agentHandouts: { push: [agentHandoutToken(agentName, confirmed.generation)] }, + handoutReceipts: { push: [handoutReceipt(handoutToken, agentName, confirmed.generation)] }, + }, }); if (claimed.count !== 1) return null; } catch (error) { @@ -262,7 +303,7 @@ export async function GET( issue: candidate.issue ? { repoFullName: candidate.repo, number: candidate.issue } : undefined, - prFixItem: { id: candidate.id, generation: confirmed.generation }, + prFixItem: { id: candidate.id, generation: confirmed.generation, handoutToken }, reasons, }); }; @@ -409,7 +450,7 @@ export async function GET( // instead of re-serving the deferred one. const isDeferred = (item: { repoFullName?: string | null; number: number }) => deferredIssues.has(issueKey(item.repoFullName, item.number)); - const first = rankedQueue.find((item) => !isDeferred(item)); + const first = rankedQueue.find((item) => !isDeferred(item) && !skipIssueForCapacity(item.repoFullName, item.number)); if (first) { if (admissionMode === "audit" && first.admission && !first.admission.admitted) { console.warn( @@ -440,15 +481,25 @@ export async function GET( const excludedNote = skipped.size > 0 ? `${skipped.size} PR-fix generations skipped by client exclusion` : null; + // #1256: surface capacity suppression separately from queue ownership, + // so an operator can tell "everything left is cooled for this capacity" + // from an empty lane. + const capacityDeferredCount = + prFixItems.filter((candidate) => skipPrForCapacity(candidate.repo, candidate.pr)).length + + rankedQueue.filter((item) => !isDeferred(item) && skipIssueForCapacity(item.repoFullName, item.number)).length; + const capacityNote = capacityDeferredCount > 0 + ? `${capacityDeferredCount} item${capacityDeferredCount === 1 ? "" : "s"} cooled for ${requestedCapacity ?? "any capacity (undeclared request)"} by a capacity handoff` + : null; + if (withheldQueue.length > 0) { const withheldNote = `${withheldQueue.length} ready issue${withheldQueue.length === 1 ? "" : "s"} withheld by grooming admission`; return respond( - createIdleTask(`No work available (${[withheldNote, deferredNote, excludedNote].filter(Boolean).join("; ")})`), + createIdleTask(`No work available (${[withheldNote, deferredNote, excludedNote, capacityNote].filter(Boolean).join("; ")})`), ); } - if (deferredNote || excludedNote) { - return respond(createIdleTask(`No work available (${[deferredNote, excludedNote].filter(Boolean).join("; ")})`)); + if (deferredNote || excludedNote || capacityNote) { + return respond(createIdleTask(`No work available (${[deferredNote, excludedNote, capacityNote].filter(Boolean).join("; ")})`)); } return respond(createIdleTask("No work available")); diff --git a/src/app/api/agents/[agentName]/tasks/report/route.test.ts b/src/app/api/agents/[agentName]/tasks/report/route.test.ts index e424e81e..f8effb1a 100644 --- a/src/app/api/agents/[agentName]/tasks/report/route.test.ts +++ b/src/app/api/agents/[agentName]/tasks/report/route.test.ts @@ -1593,6 +1593,34 @@ describe("POST /api/agents/[agentName]/tasks/report — idempotencyKey", () => { }); }); +describe("POST /api/agents/[agentName]/tasks/report — capacity exhaustion handoff", () => { + const validReport = { taskType: "followup-pr", outcome: "pr_updated", repoFullName: "org/repo", pullRequestNumber: 10 }; + + it("requires a recognized capacity identifier and issued PR-fix attempt token", async () => { + const missingCapacity = await postRequest({ ...validReport, taskType: "followup-pr", outcome: "capacity-exhausted", prFixItem: { id: "item-1", generation: 1, handoutToken: "hnd_deadbeef00" } }); + expect(missingCapacity.status).toBe(400); + const missingAttempt = await postRequest({ ...validReport, taskType: "followup-pr", outcome: "capacity-exhausted", capacity: "provider/model" }); + expect(missingAttempt.status).toBe(400); + // #1256 review: the per-hand-out receipt token is a hard gate for this + // outcome — (id, generation) alone cannot identify the exact hand-out. + const missingReceipt = await postRequest({ ...validReport, taskType: "followup-pr", outcome: "capacity-exhausted", capacity: "provider/model", prFixItem: { id: "item-1", generation: 1 } }); + expect(missingReceipt.status).toBe(400); + expect((await missingReceipt.json()).error).toMatch(/handoutToken/); + }); + + it("passes the normalized capacity and issued attempt token to the resolver", async () => { + const response = await postRequest({ + ...validReport, taskType: "followup-pr", outcome: "capacity-exhausted", + capacity: " provider/model ", prFixItem: { id: "item-1", generation: 2, handoutToken: " hnd_deadbeef00 " }, + }); + expect(response.status).toBe(200); + expect(prFixResolveMock).toHaveBeenCalledWith(expect.objectContaining({ + outcome: "capacity-exhausted", capacity: "provider/model", + attempt: { itemId: "item-1", generation: 2, handoutToken: "hnd_deadbeef00" }, + })); + }); +}); + describe("POST /api/agents/[agentName]/tasks/report — prFixItem attempt token (#1074)", () => { beforeEach(() => { delete process.env.DISPATCH_AUTH_MODE; diff --git a/src/app/api/agents/[agentName]/tasks/report/route.ts b/src/app/api/agents/[agentName]/tasks/report/route.ts index ee4d8fc2..f765283a 100644 --- a/src/app/api/agents/[agentName]/tasks/report/route.ts +++ b/src/app/api/agents/[agentName]/tasks/report/route.ts @@ -32,6 +32,7 @@ const VALID_OUTCOMES = [ "already_done", "handed-off", "waiting-external", + "capacity-exhausted", ] as const; type ValidOutcome = (typeof VALID_OUTCOMES)[number]; @@ -67,7 +68,8 @@ export interface TaskReportBody { // The (id, generation) attempt token next-task issued on this followup-pr // task, echoed back by the worker (#1074). Required for PR-fix queue // settlement; part of the report's payload identity (idempotency hash). - prFixItem?: { id: string; generation: number } | null; + prFixItem?: { id: string; generation: number; handoutToken?: string } | null; + capacity?: string; } function deriveStatus(outcome: ValidOutcome): string { @@ -75,6 +77,9 @@ function deriveStatus(outcome: ValidOutcome): string { if (outcome === "failed") return "failed"; if (outcome === "blocked") return "blocked"; if (outcome === "waiting-external") return "waiting-external"; + // #1256: neutral terminal status — the worker's capacity gave the attempt + // back; nothing succeeded or failed. + if (outcome === "capacity-exhausted") return "capacity-exhausted"; return "completed"; } @@ -276,6 +281,28 @@ export async function POST( return errorResponse(`Invalid outcome. Must be one of: ${VALID_OUTCOMES.join(", ")}`, 400); } + if (outcome === "capacity-exhausted") { + if (typeof raw.capacity !== "string" || raw.capacity.trim().length === 0 || raw.capacity.trim().length > 200) { + return errorResponse("capacity must be a non-empty recognized capacity identifier of at most 200 characters", 400); + } + if (!raw.prFixItem || typeof raw.prFixItem !== "object" || Array.isArray(raw.prFixItem)) { + return errorResponse("capacity-exhausted requires the issued prFixItem { id, generation } token", 400); + } + // #1256 review: the release is fenced to the EXACT issued hand-out, so + // the per-hand-out receipt token is a hard gate for this outcome (other + // outcomes never required it and are unchanged). + const rawCapacityAttempt = raw.prFixItem as { handoutToken?: unknown }; + if (typeof rawCapacityAttempt.handoutToken !== "string" || rawCapacityAttempt.handoutToken.trim().length === 0) { + return errorResponse("capacity-exhausted requires the issued prFixItem.handoutToken receipt token", 400); + } + // The outcome's entire effect is the queue settlement; a report without + // the PR coordinates would record an AgentRun and silently settle nothing. + // Claimed-issue exhaustion uses POST /api/issues/capacity-handoff instead. + if (typeof raw.repoFullName !== "string" || raw.repoFullName.trim() === "" || typeof raw.pullRequestNumber !== "number") { + return errorResponse("capacity-exhausted requires repoFullName and pullRequestNumber (issue work uses POST /api/issues/capacity-handoff)", 400); + } + } + if (raw.issueNumber !== undefined && (typeof raw.issueNumber !== "number" || !Number.isInteger(raw.issueNumber))) { return errorResponse("issueNumber must be an integer", 400); } @@ -284,7 +311,7 @@ export async function POST( return errorResponse("pullRequestNumber must be an integer", 400); } - const stringFields: readonly string[] = ["repoFullName", "pullRequestUrl", "summary", "error", "evidence"]; + const stringFields: readonly string[] = ["repoFullName", "pullRequestUrl", "summary", "error", "evidence", "capacity"]; for (const field of stringFields) { if (raw[field] !== undefined && typeof raw[field] !== "string") { return errorResponse(`${field} must be a string`, 400); @@ -423,6 +450,14 @@ export async function POST( if (typeof prFixItem.generation !== "number" || !Number.isInteger(prFixItem.generation) || prFixItem.generation < 1) { return errorResponse("prFixItem.generation must be an integer >= 1", 400); } + // #1256: optional per-hand-out receipt token. Optional here so legacy + // (pre-receipt) reports keep settling every other outcome; the resolver + // enforces its presence for `capacity-exhausted`. + if (prFixItem.handoutToken !== undefined && prFixItem.handoutToken !== null) { + if (typeof prFixItem.handoutToken !== "string" || prFixItem.handoutToken.trim().length === 0 || prFixItem.handoutToken.trim().length > 100) { + return errorResponse("prFixItem.handoutToken must be a non-empty string of at most 100 characters", 400); + } + } } // Optional worker-chosen opaque key. Present → the report becomes @@ -468,6 +503,10 @@ export async function POST( ? { id: (raw.prFixItem as { id: string }).id.trim(), generation: (raw.prFixItem as { generation: number }).generation, + handoutToken: + typeof (raw.prFixItem as { handoutToken?: unknown }).handoutToken === "string" + ? (raw.prFixItem as { handoutToken: string }).handoutToken.trim() + : undefined, } : undefined; @@ -493,6 +532,7 @@ export async function POST( // #1120: normalized worker start time; undefined → report time (now). startedAt: resolveReportedStartedAt(raw.startedAt, now), prFixItem, + capacity: typeof raw.capacity === "string" ? raw.capacity.trim() : undefined, }; try { @@ -598,9 +638,13 @@ export async function POST( // the resolver ignores both for any other outcome. blockReason: report.blockReason, decision: report.decision, + capacity: report.capacity, + agentName, // #1074: settlement is authorized by the attempt token the worker was // issued; a report without one never mutates the queue. - attempt: prFixItem ? { itemId: prFixItem.id, generation: prFixItem.generation } : null, + attempt: prFixItem + ? { itemId: prFixItem.id, generation: prFixItem.generation, handoutToken: prFixItem.handoutToken } + : null, }); if (typeof idempotencyKey === "string") { // Persist the resolution before responding so a retry replays the real diff --git a/src/app/api/issues/capacity-handoff/route.test.ts b/src/app/api/issues/capacity-handoff/route.test.ts new file mode 100644 index 00000000..c773bfb5 --- /dev/null +++ b/src/app/api/issues/capacity-handoff/route.test.ts @@ -0,0 +1,453 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { + authedRequest, + makeDispatchEnvMock, + TEST_AGENT_TOKEN as mockToken, +} from "@/test/route-helpers"; + +process.env.DISPATCH_AGENT_TOKEN = mockToken; + +// dispatch-env: maintainer token (full rights), a worker token bound to +// "alpha", and an unbound legacy worker token (#1129 refusals). +vi.mock("@/lib/dispatch-env", () => + makeDispatchEnvMock(mockToken, { "unbound-worker-token": "worker" }, { "bound-alpha-token": "alpha" }), +); + +const { mocks, db } = vi.hoisted(() => { + const mocks = { + getLiveIssueLabels: vi.fn<(repo: string, num: number) => Promise>(async () => []), + addIssueLabel: vi.fn(async () => {}), + removeIssueLabel: vi.fn(async () => {}), + fetchPullRequestState: vi.fn(async () => ({ state: null as string | null, mergedAt: null as string | null })), + }; + // Mutable in-memory database, reset per test via makeState(). + const db: Record = { + issue: null, + lease: null, + agentWork: [] as any[], + agentWorkUpdates: [] as any[], + agentWorkHistory: [] as any[], + handoffs: [] as any[], + audit: [] as any[], + leaseDeletes: 0, + }; + return { mocks, db }; +}); + +vi.mock("@/lib/prisma", () => ({ + prisma: { + issue: { + findUnique: async ({ where }: any) => + db.issue && db.issue.id === where.id ? db.issue : null, + update: async ({ where, data }: any) => { + if (db.issue && db.issue.id === where.id) db.issue = { ...db.issue, ...data }; + return db.issue; + }, + }, + lease: { + findUnique: async ({ where }: any) => { + const key = where.agentName_issueId; + return db.lease && db.lease.agentName === key.agentName && db.lease.issueId === key.issueId + ? db.lease + : null; + }, + deleteMany: async ({ where }: any) => { + if ( + db.lease && + db.lease.id === where.id && + db.lease.agentName === where.agentName && + db.lease.issueId === where.issueId && + where.expiredAt?.gt != null && + new Date(db.lease.expiredAt).getTime() > new Date(where.expiredAt.gt).getTime() + ) { + db.lease = null; + db.leaseDeletes += 1; + return { count: 1 }; + } + return { count: 0 }; + }, + }, + agentWork: { + findMany: async () => db.agentWork, + update: async ({ where, data }: any) => { + db.agentWorkUpdates.push({ where, data }); + return null; + }, + }, + agentWorkHistory: { + create: async ({ data }: any) => { + db.agentWorkHistory.push(data); + return data; + }, + }, + capacityHandoff: { + create: async ({ data }: any) => { + if (db.failHandoffCreate) throw new Error("cooldown write failed"); + db.handoffs.push(data); + return data; + }, + findMany: async () => db.handoffs, + deleteMany: async () => ({ count: 0 }), + }, + auditLog: { + create: async ({ data }: any) => { + db.audit.push(data); + return data; + }, + }, + }, +})); + +vi.mock("@/lib/claim-gate", () => ({ + getLiveIssueLabels: mocks.getLiveIssueLabels, +})); + +vi.mock("@/lib/github", () => ({ + addIssueLabel: mocks.addIssueLabel, + removeIssueLabel: mocks.removeIssueLabel, +})); + +vi.mock("@/lib/github-prs", () => ({ + fetchPullRequestState: mocks.fetchPullRequestState, +})); + +import { POST } from "./route"; +import { resetAuthCaches } from "@/lib/auth"; + +const CLAIM_TOKEN = "claim_test-token-1"; + +function makeState(overrides: Record = {}) { + db.issue = { + id: "issue-1", + number: 42, + state: "open", + blockedReason: null, + linkedPrNumber: null, + labels: ["agent/alpha", "status/in-progress"], + repository: { fullName: "org/repo" }, + ...overrides, + }; + db.lease = { + id: "lease-1", + agentName: "alpha", + issueId: "issue-1", + claimToken: CLAIM_TOKEN, + expiredAt: new Date(Date.now() + 10 * 60 * 1000), + ...(overrides.lease as object | undefined), + }; + if (overrides.lease === null) db.lease = null; + db.agentWork = [{ id: "work-1" }]; + db.agentWorkUpdates = []; + db.agentWorkHistory = []; + db.handoffs = []; + db.audit = []; + db.leaseDeletes = 0; + db.failHandoffCreate = false; + mocks.getLiveIssueLabels.mockResolvedValue( + (overrides.liveLabels as string[] | undefined) ?? ["agent/alpha", "status/in-progress"], + ); + mocks.addIssueLabel.mockResolvedValue(undefined); + mocks.removeIssueLabel.mockResolvedValue(undefined); +} + +function postRequest(body: Record = {}, token?: string) { + return POST( + authedRequest("http://localhost/api/issues/capacity-handoff", { + method: "POST", + token, + body: { + issueId: "issue-1", + repoFullName: "org/repo", + issueNumber: 42, + agentName: "alpha", + claimToken: CLAIM_TOKEN, + capacity: "provider/model-a", + ...body, + }, + }), + ); +} + +describe("POST /api/issues/capacity-handoff (#1256)", () => { + beforeEach(() => { + vi.clearAllMocks(); + resetAuthCaches(); + delete process.env.DISPATCH_AUTH_MODE; + delete process.env.DISPATCH_CAPACITY_COOLDOWN_MS; + makeState(); + }); + + afterEach(() => { + delete process.env.DISPATCH_CAPACITY_COOLDOWN_MS; + }); + + // ── auth ────────────────────────────────────────────────────────────────── + + it("requires bearer authentication", async () => { + const response = await POST( + new Request("http://localhost/api/issues/capacity-handoff", { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({}), + }), + ); + expect(response.status).toBe(401); + }); + + it("refuses a bound worker token handing off ANOTHER agent's claim", async () => { + const response = await postRequest({ agentName: "bravo" }, "bound-alpha-token"); + expect(response.status).toBe(403); + expect(db.leaseDeletes).toBe(0); + }); + + it("refuses the unbound legacy worker token (#1129)", async () => { + const response = await postRequest({}, "unbound-worker-token"); + expect(response.status).toBe(403); + expect(db.leaseDeletes).toBe(0); + }); + + // ── validation ───────────────────────────────────────────────────────────── + + it("rejects a missing or blank capacity (explicit recognition required)", async () => { + expect((await postRequest({ capacity: undefined })).status).toBe(400); + expect((await postRequest({ capacity: " " })).status).toBe(400); + expect((await postRequest({ capacity: "x".repeat(201) })).status).toBe(400); + expect(db.leaseDeletes).toBe(0); + }); + + it("rejects a missing or oversized claim token", async () => { + expect((await postRequest({ claimToken: undefined })).status).toBe(400); + expect((await postRequest({ claimToken: "" })).status).toBe(400); + expect((await postRequest({ claimToken: "t".repeat(201) })).status).toBe(400); + }); + + // ── happy path ──────────────────────────────────────────────────────────── + + it("releases the exact claim, returns the issue to ready, and records a scoped cooldown", async () => { + const response = await postRequest(); + expect(response.status).toBe(200); + const json = await response.json(); + expect(json.success).toBe(true); + expect(json.labels).not.toContain("agent/alpha"); + expect(json.labels).toContain("status/ready"); + expect(json.status).toBe("status/ready"); + expect(new Date(json.cooldownUntil).getTime()).toBeGreaterThan(Date.now()); + + // Ownership token consumed, claim label removed, in-progress -> ready. + expect(db.lease).toBeNull(); + expect(mocks.removeIssueLabel).toHaveBeenCalledWith("org/repo", 42, "agent/alpha"); + expect(mocks.removeIssueLabel).toHaveBeenCalledWith("org/repo", 42, "status/in-progress"); + expect(mocks.addIssueLabel).toHaveBeenCalledWith("org/repo", 42, "status/ready"); + expect(db.issue.labels).toContain("status/ready"); + expect(db.issue.labels).not.toContain("agent/alpha"); + + // Capacity-scoped cooldown row. + expect(db.handoffs).toHaveLength(1); + expect(db.handoffs[0]).toMatchObject({ + kind: "issue", + capacity: "provider/model-a", + repoFullName: "org/repo", + issueNumber: 42, + agentName: "alpha", + issueId: "issue-1", + }); + + // AgentWork retired with history. + expect(db.agentWorkUpdates[0]).toMatchObject({ + where: { id: "work-1" }, + data: { state: "RELEASED" }, + }); + expect(db.agentWorkHistory[0]).toMatchObject({ + workId: "work-1", + action: "capacity_handoff", + state: "RELEASED", + }); + + // Audit trail. + expect(db.audit).toHaveLength(1); + expect(db.audit[0]).toMatchObject({ + action: "issue_capacity_handoff", + success: true, + issueNumber: 42, + }); + expect(db.audit[0].notes).toContain("capacity=provider/model-a"); + }); + + it("honors DISPATCH_CAPACITY_COOLDOWN_MS for the bounded cooldown", async () => { + process.env.DISPATCH_CAPACITY_COOLDOWN_MS = "120000"; + const response = await postRequest(); + const json = await response.json(); + const delta = new Date(json.cooldownUntil).getTime() - Date.now(); + expect(delta).toBeGreaterThan(100_000); + expect(delta).toBeLessThan(130_000); + }); + + // ── fencing: stale owners, races, changed ownership ─────────────────────── + + it("refuses a stale claim token after the claim was re-issued (exact-claim fence)", async () => { + // The lease was released and re-claimed: new row, new token. + db.lease = { ...db.lease, id: "lease-2", claimToken: "claim_newer-token" }; + const response = await postRequest(); // old CLAIM_TOKEN + expect(response.status).toBe(409); + expect((await response.json()).error).toMatch(/does not match the current claim/i); + // The NEW claim is untouched. + expect(db.lease).not.toBeNull(); + expect(db.lease.claimToken).toBe("claim_newer-token"); + expect(mocks.removeIssueLabel).not.toHaveBeenCalled(); + expect(db.handoffs).toHaveLength(0); + }); + + it("refuses an expired claim (stale recovery owns release)", async () => { + db.lease = { ...db.lease, expiredAt: new Date(Date.now() - 1000) }; + const response = await postRequest(); + expect(response.status).toBe(409); + expect((await response.json()).error).toMatch(/expired/i); + expect(mocks.removeIssueLabel).not.toHaveBeenCalled(); + }); + + it("refuses when no claim exists", async () => { + db.lease = null; + const response = await postRequest(); + expect(response.status).toBe(409); + expect((await response.json()).error).toMatch(/no active tokened claim/i); + }); + + it("refuses when GitHub shows a DIFFERENT agent owns the claim (concurrent new owner)", async () => { + mocks.getLiveIssueLabels.mockResolvedValue(["agent/bravo", "status/in-progress"]); + const response = await postRequest(); + expect(response.status).toBe(409); + expect((await response.json()).error).toMatch(/belongs to agent\/bravo/i); + // No writes at all — bravo's claim is untouched. + expect(db.lease).not.toBeNull(); + expect(mocks.removeIssueLabel).not.toHaveBeenCalled(); + expect(mocks.addIssueLabel).not.toHaveBeenCalled(); + expect(db.handoffs).toHaveLength(0); + }); + + it("refuses when the agent label is gone from GitHub (ownership changed / already released)", async () => { + mocks.getLiveIssueLabels.mockResolvedValue(["status/ready"]); + const response = await postRequest(); + expect(response.status).toBe(409); + expect((await response.json()).error).toMatch(/no agent claim/i); + expect(db.lease).not.toBeNull(); + }); + + it("refuses a lost lease race (CAS miss) with zero writes", async () => { + // Another handoff released the lease between the read and the CAS. + const { prisma } = await import("@/lib/prisma"); + const leaseModel = (prisma as any).lease; + const original = leaseModel.deleteMany; + leaseModel.deleteMany = async () => ({ count: 0 }); + try { + const response = await postRequest(); + expect(response.status).toBe(409); + expect((await response.json()).error).toMatch(/changed concurrently/i); + expect(mocks.removeIssueLabel).not.toHaveBeenCalled(); + expect(db.handoffs).toHaveLength(0); + expect(db.audit).toHaveLength(0); + } finally { + leaseModel.deleteMany = original; + } + }); + + // ── human holds ──────────────────────────────────────────────────────────── + + it("refuses a deliberate block (human hold with blockedReason)", async () => { + db.issue = { ...db.issue, labels: ["agent/alpha", "status/blocked"], blockedReason: "waiting on vendor API" }; + mocks.getLiveIssueLabels.mockResolvedValue(["agent/alpha", "status/blocked"]); + const response = await postRequest(); + expect(response.status).toBe(409); + expect((await response.json()).error).toMatch(/human hold/i); + expect(db.lease).not.toBeNull(); + expect(mocks.removeIssueLabel).not.toHaveBeenCalled(); + }); + + it("refuses a LIVE status/blocked even when the cache has not caught up (fail-closed hold)", async () => { + // Cache still says in-progress with no blockedReason; GitHub was blocked + // out-of-band. The live set wins — the block is never reversed. + mocks.getLiveIssueLabels.mockResolvedValue(["agent/alpha", "status/blocked"]); + const response = await postRequest(); + expect(response.status).toBe(409); + expect((await response.json()).error).toMatch(/status\/blocked/i); + expect(db.lease).not.toBeNull(); + expect(db.leaseDeletes).toBe(0); + expect(mocks.removeIssueLabel).not.toHaveBeenCalled(); + }); + + it("narrows to an ownership-only release when ownership changed after the CAS", async () => { + // First (pre-CAS) read shows our claim; the post-CAS re-read shows a + // force-claim moved the issue to bravo. Only OUR label may be removed — + // bravo's status transition must survive. + mocks.getLiveIssueLabels + .mockResolvedValueOnce(["agent/alpha", "status/in-progress"]) + .mockResolvedValueOnce(["agent/bravo", "status/in-progress"]); + const response = await postRequest(); + expect(response.status).toBe(200); + const json = await response.json(); + // Ownership-only: our agent label removed, no status churn. + expect(mocks.removeIssueLabel).toHaveBeenCalledWith("org/repo", 42, "agent/alpha"); + expect(mocks.removeIssueLabel).not.toHaveBeenCalledWith("org/repo", 42, "status/in-progress"); + expect(mocks.addIssueLabel).not.toHaveBeenCalledWith("org/repo", 42, "status/ready"); + expect(json.labels).toContain("status/in-progress"); + expect(json.labels).not.toContain("agent/alpha"); + expect(db.audit[0].notes).toMatch(/ownership changed post-CAS/i); + }); + + it("still succeeds when the cooldown write fails after the release committed", async () => { + db.failHandoffCreate = true; + const response = await postRequest(); + expect(response.status).toBe(200); + const json = await response.json(); + expect(json.success).toBe(true); + expect(json.cooldownUntil).toBeNull(); + expect(db.lease).toBeNull(); + expect(mocks.removeIssueLabel).toHaveBeenCalledWith("org/repo", 42, "agent/alpha"); + expect(db.audit[0].notes).toMatch(/none \(cooldown write failed\)/i); + }); + + it("refuses a done issue (live status/done wins over the cache)", async () => { + mocks.getLiveIssueLabels.mockResolvedValue(["agent/alpha", "status/done"]); + const response = await postRequest(); + expect(response.status).toBe(409); + expect((await response.json()).error).toMatch(/done/i); + expect(db.lease).not.toBeNull(); + }); + + it("refuses a closed issue before touching the lease", async () => { + db.issue = { ...db.issue, state: "closed" }; + const response = await postRequest(); + expect(response.status).toBe(400); + expect(db.leaseDeletes).toBe(0); + expect(mocks.getLiveIssueLabels).not.toHaveBeenCalled(); + }); + + // ── fail-closed GitHub behavior ─────────────────────────────────────────── + + it("fails closed when live GitHub labels cannot be verified", async () => { + mocks.getLiveIssueLabels.mockRejectedValue(new Error("GitHub 503")); + const response = await postRequest(); + expect(response.status).toBe(503); + expect((await response.json()).error).toMatch(/could not verify live github labels/i); + expect(db.lease).not.toBeNull(); + expect(db.handoffs).toHaveLength(0); + }); + + it("rejects a cross-referenced issue identity before any write (#1129)", async () => { + const response = await postRequest({ issueNumber: 43 }); + expect(response.status).toBe(400); + expect(db.leaseDeletes).toBe(0); + }); + + it("audits a partial failure where the lease released but the label release failed", async () => { + mocks.removeIssueLabel.mockRejectedValueOnce(new Error("GitHub write failed")); + const response = await postRequest(); + expect(response.status).toBe(500); + // Lease consumed (ownership definitively released); failure audited. + expect(db.lease).toBeNull(); + expect(db.audit).toHaveLength(1); + expect(db.audit[0]).toMatchObject({ action: "issue_capacity_handoff", success: false }); + expect(db.audit[0].errorMessage).toMatch(/stale-claim recovery/i); + // No cooldown row is minted for a failed handoff. + expect(db.handoffs).toHaveLength(0); + }); +}); diff --git a/src/app/api/issues/capacity-handoff/route.ts b/src/app/api/issues/capacity-handoff/route.ts new file mode 100644 index 00000000..6862ddf0 --- /dev/null +++ b/src/app/api/issues/capacity-handoff/route.ts @@ -0,0 +1,114 @@ +// POST /api/issues/capacity-handoff (#1256) +// +// Worker-facing fenced recovery for explicit capacity exhaustion on a claimed +// issue. The caller presents the claim token Dispatch issued at claim time; +// the release is fenced to that exact claim and refuses human holds and +// changed ownership (see src/lib/issue-capacity-handoff.ts). On success the +// issue returns to circulation for other compatible workers and the exhausted +// capacity gets a bounded, capacity-SCOPED cooldown — unrelated capacities are +// never suppressed. +// +// Auth follows the claim/unclaim contract: bearer-authenticated; a bound +// worker credential may only hand off its own agent's claim; the unbound +// legacy worker token is refused (#1129). +import { NextResponse } from "next/server"; +import { errorResponse, handleApiError } from "@/lib/api-errors"; +import { prisma } from "@/lib/prisma"; +import { authorizeRequest, authErrorResponse, getAuthorizedActor } from "@/lib/auth"; +import { enforceWorkerAgentScope } from "@/lib/worker-identity"; +import { recoverIssueCapacityHandoff } from "@/lib/issue-capacity-handoff"; +import { normalizeCapacity } from "@/lib/capacity-handoff"; +import { enforceRateLimit } from "@/lib/rate-limit"; + +const RATE_LIMIT = { limit: 30, windowMs: 10_000 }; +const MAX_NOTE_LENGTH = 500; +const MAX_TOKEN_LENGTH = 200; + +export async function POST(request: Request) { + const auth = await authorizeRequest(request); + if (!auth.authorized) { + return authErrorResponse(auth); + } + + const limited = enforceRateLimit(`capacity-handoff:${auth.actor}`, RATE_LIMIT); + if (limited) return limited; + + try { + let body: unknown; + try { + body = await request.json(); + } catch { + return errorResponse("Invalid JSON body", 400); + } + if (typeof body !== "object" || body === null) { + return errorResponse("Invalid JSON body", 400); + } + + const { + issueId, + repoFullName, + issueNumber, + agentName, + claimToken, + capacity, + note, + } = body as Record; + + if ( + typeof issueId !== "string" || issueId.trim() === "" || + typeof repoFullName !== "string" || repoFullName.trim() === "" || + typeof issueNumber !== "number" || !Number.isInteger(issueNumber) || + typeof agentName !== "string" || agentName.trim() === "" + ) { + return errorResponse("Missing required fields: issueId, repoFullName, issueNumber, agentName", 400); + } + if (typeof claimToken !== "string" || claimToken.trim() === "" || claimToken.length > MAX_TOKEN_LENGTH) { + return errorResponse(`claimToken must be a non-empty string of at most ${MAX_TOKEN_LENGTH} characters (issued with the claim)`, 400); + } + // Explicit recognition of capacity exhaustion: a non-empty, bounded, + // opaque capacity identifier. Silently exhausting nothing is not allowed. + const normalizedCapacity = normalizeCapacity(capacity); + if (normalizedCapacity === null) { + return errorResponse("capacity must be a non-empty recognized capacity identifier of at most 200 characters", 400); + } + if (note !== undefined && (typeof note !== "string" || note.length > MAX_NOTE_LENGTH)) { + return errorResponse(`note must be a string of at most ${MAX_NOTE_LENGTH} characters`, 400); + } + + // A bound worker credential may only hand off its own agent's claim; an + // unbound legacy worker token is refused here (#1129). + const scopeError = await enforceWorkerAgentScope(auth, agentName); + if (scopeError) return scopeError; + + const actor = getAuthorizedActor(auth, request, agentName); + + const result = await recoverIssueCapacityHandoff({ + client: prisma as any, + issueId: issueId.trim(), + repoFullName: repoFullName.trim(), + issueNumber, + agentName: agentName.trim(), + claimToken: claimToken.trim(), + capacity: normalizedCapacity, + note: typeof note === "string" && note.trim() !== "" ? note.trim() : null, + actor, + }); + + if (!result.ok) { + return errorResponse(result.message, result.httpStatus); + } + + return NextResponse.json({ + success: true, + labels: result.labels, + status: result.status, + statusNote: result.statusNote, + capacity: normalizedCapacity, + // null only when the (post-release, best-effort) cooldown write failed; + // the ownership release itself succeeded. + cooldownUntil: result.cooldownUntil ? result.cooldownUntil.toISOString() : null, + }); + } catch (error) { + return handleApiError("capacity handoff", error); + } +} diff --git a/src/app/api/issues/claim/route.test.ts b/src/app/api/issues/claim/route.test.ts index 5321558d..45ee1085 100644 --- a/src/app/api/issues/claim/route.test.ts +++ b/src/app/api/issues/claim/route.test.ts @@ -149,6 +149,19 @@ describe("POST /api/issues/claim — business logic", () => { expect(mocks.createAuditLog).toHaveBeenCalledWith({ data: expect.objectContaining({ action: "claim_issue", success: true, actor: "test-agent" }) }); }); + it("echoes the issued claim token that fences a later capacity handoff (#1256)", async () => { + // Mirror the real client: create persists what the code minted. + mocks.leaseCreate.mockImplementation(async ({ data }: any) => ({ id: "l-1", ...data })); + const res = await POST(makeRequest()); + expect(res.status).toBe(200); + const body = await res.json(); + expect(typeof body.claimToken).toBe("string"); + expect(body.claimToken).toMatch(/^claim_/); + // The minted token is persisted on the lease row, not just returned. + const created = (mocks.leaseCreate.mock.calls[0] as any)[0].data; + expect(created.claimToken).toBe(body.claimToken); + }); + it("refuses closed issues", async () => { mocks.findUnique.mockResolvedValue({ id: "issue-1", state: "closed", number: 42, repository: { fullName: "org/repo" }, labels: [] as string[] }); const res = await POST(makeRequest()); diff --git a/src/app/api/issues/claim/route.ts b/src/app/api/issues/claim/route.ts index e68f1155..213f5fce 100644 --- a/src/app/api/issues/claim/route.ts +++ b/src/app/api/issues/claim/route.ts @@ -216,8 +216,10 @@ export async function POST(request: Request) { data: { labels: updatedLabels, lastSyncedAt: new Date() }, }); - // Create or renew a lease for this agent (issue #166) - await upsertLease({ + // Create or renew a lease for this agent (issue #166). + // #1256: the lease carries the opaque claim token that fences a later + // capacity-handoff release to THIS exact claim; echo it to the worker. + const lease = await upsertLease({ agentName: agentName as string, issueId: issueId as string, checkpoint: "issue_claimed", @@ -254,7 +256,11 @@ export async function POST(request: Request) { }, }); - return NextResponse.json({ success: true, labels: updatedLabels }); + return NextResponse.json({ + success: true, + labels: updatedLabels, + claimToken: lease.lease.claimToken ?? null, + }); } catch (error) { const errorMessage = error instanceof Error ? error.message : "Unknown error"; diff --git a/src/lib/agent-task.ts b/src/lib/agent-task.ts index 88bf5867..65d971c9 100644 --- a/src/lib/agent-task.ts +++ b/src/lib/agent-task.ts @@ -29,6 +29,14 @@ export interface PullRequestRef { export interface PrFixItemIdentity { id: string; generation: number; + /** + * #1256: per-hand-out receipt token minted for THIS hand-out (rotates on + * every issuance). Workers echo it back on reports; a `capacity-exhausted` + * settlement is only authorized with it, releasing exactly the hand-out it + * was issued with. Other outcomes ignore it, so pre-token workers are + * unaffected. + */ + handoutToken?: string; } export interface IdleTask { diff --git a/src/lib/capacity-handoff.test.ts b/src/lib/capacity-handoff.test.ts new file mode 100644 index 00000000..09577b11 --- /dev/null +++ b/src/lib/capacity-handoff.test.ts @@ -0,0 +1,173 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +import { + buildCapacitySkip, + capacityCooldownMs, + loadActiveCapacityHandoffs, + newClaimToken, + normalizeCapacity, + recordCapacityHandoff, + MAX_CAPACITY_LENGTH, +} from "./capacity-handoff"; + +describe("capacityCooldownMs (#1256)", () => { + const prev = process.env.DISPATCH_CAPACITY_COOLDOWN_MS; + afterEach(() => { + if (prev === undefined) delete process.env.DISPATCH_CAPACITY_COOLDOWN_MS; + else process.env.DISPATCH_CAPACITY_COOLDOWN_MS = prev; + }); + + it("defaults to 15 minutes", () => { + delete process.env.DISPATCH_CAPACITY_COOLDOWN_MS; + expect(capacityCooldownMs()).toBe(15 * 60 * 1000); + }); + + it("honors a configured override", () => { + process.env.DISPATCH_CAPACITY_COOLDOWN_MS = "120000"; + expect(capacityCooldownMs()).toBe(120_000); + }); + + it("clamps the bound: never below 1 minute, never above 6 hours", () => { + process.env.DISPATCH_CAPACITY_COOLDOWN_MS = "1"; + expect(capacityCooldownMs()).toBe(60_000); + process.env.DISPATCH_CAPACITY_COOLDOWN_MS = String(99 * 60 * 60 * 1000); + expect(capacityCooldownMs()).toBe(6 * 60 * 60 * 1000); + }); + + it("falls back to the default on garbage values", () => { + process.env.DISPATCH_CAPACITY_COOLDOWN_MS = "not-a-number"; + expect(capacityCooldownMs()).toBe(15 * 60 * 1000); + process.env.DISPATCH_CAPACITY_COOLDOWN_MS = "-5"; + expect(capacityCooldownMs()).toBe(15 * 60 * 1000); + process.env.DISPATCH_CAPACITY_COOLDOWN_MS = ""; + expect(capacityCooldownMs()).toBe(15 * 60 * 1000); + }); +}); + +describe("normalizeCapacity (#1256)", () => { + it("trims and accepts a bounded non-empty identifier", () => { + expect(normalizeCapacity(" provider/model ")).toBe("provider/model"); + expect(normalizeCapacity("x".repeat(MAX_CAPACITY_LENGTH))).toBe("x".repeat(MAX_CAPACITY_LENGTH)); + }); + + it("rejects blank, oversized, and non-string values", () => { + expect(normalizeCapacity("")).toBeNull(); + expect(normalizeCapacity(" ")).toBeNull(); + expect(normalizeCapacity("x".repeat(MAX_CAPACITY_LENGTH + 1))).toBeNull(); + expect(normalizeCapacity(undefined)).toBeNull(); + expect(normalizeCapacity(42)).toBeNull(); + }); +}); + +describe("newClaimToken (#1256)", () => { + it("mints distinct opaque tokens", () => { + const a = newClaimToken(); + const b = newClaimToken(); + expect(a).toMatch(/^claim_/); + expect(a).not.toBe(b); + }); +}); + +function makeHandoffClient() { + const rows: any[] = []; + return { + rows, + capacityHandoff: { + create: vi.fn(async ({ data }: any) => { + const row = { id: `h-${rows.length + 1}`, createdAt: new Date(), ...data }; + rows.push(row); + return row; + }), + findMany: vi.fn(async ({ where }: any) => + rows.filter( + (r) => + where?.cooldownUntil?.gt == null || + new Date(r.cooldownUntil).getTime() > new Date(where.cooldownUntil.gt).getTime(), + ), + ), + deleteMany: vi.fn(async ({ where }: any) => { + let count = 0; + for (let i = rows.length - 1; i >= 0; i--) { + if ( + where?.cooldownUntil?.lte != null && + new Date(rows[i].cooldownUntil).getTime() <= new Date(where.cooldownUntil.lte).getTime() + ) { + rows.splice(i, 1); + count++; + } + } + return { count }; + }), + }, + }; +} + +describe("recordCapacityHandoff / loadActiveCapacityHandoffs (#1256)", () => { + beforeEach(() => { + delete process.env.DISPATCH_CAPACITY_COOLDOWN_MS; + }); + + it("persists a bounded cooldown and only loads active rows", async () => { + const client = makeHandoffClient(); + const { cooldownUntil } = await recordCapacityHandoff(client, { + kind: "issue", + capacity: "provider/model-a", + repoFullName: "Org/Repo", + issueNumber: 7, + agentName: "alpha", + issueId: "issue-7", + }); + expect(cooldownUntil.getTime()).toBeGreaterThan(Date.now()); + expect(client.rows[0]).toMatchObject({ + kind: "issue", + capacity: "provider/model-a", + repoFullName: "Org/Repo", + issueNumber: 7, + agentName: "alpha", + }); + + // Expire it manually; the active load must prune and skip it. + client.rows[0].cooldownUntil = new Date(Date.now() - 1000); + const active = await loadActiveCapacityHandoffs(client); + expect(active).toHaveLength(0); + expect(client.rows).toHaveLength(0); // lazily pruned + }); +}); + +const HANDOFFS = [ + { kind: "issue", capacity: "provider/model-a", repoFullName: "org/repo", issueNumber: 10, prNumber: null }, + { kind: "pr_fix", capacity: "provider/model-a", repoFullName: "Org/Repo", issueNumber: null, prNumber: 55 }, +]; + +describe("buildCapacitySkip (#1256) — cooldown is scoped to the exhausted capacity", () => { + it("suppresses ONLY the exhausted capacity on the handed item", () => { + const { skipIssue, skipPr } = buildCapacitySkip(HANDOFFS, "provider/model-a"); + expect(skipIssue("org/repo", 10)).toBe(true); + expect(skipPr("org/repo", 55)).toBe(true); + // Other items are untouched even for the exhausted capacity. + expect(skipIssue("org/repo", 11)).toBe(false); + expect(skipPr("org/repo", 56)).toBe(false); + }); + + it("never suppresses an unrelated capacity — the work stays discoverable", () => { + const { skipIssue, skipPr } = buildCapacitySkip(HANDOFFS, "provider/model-b"); + expect(skipIssue("org/repo", 10)).toBe(false); + expect(skipPr("org/repo", 55)).toBe(false); + }); + + it("conservatively suppresses ANY cooled item for a worker that declares no capacity", () => { + const { skipIssue, skipPr } = buildCapacitySkip(HANDOFFS, undefined); + expect(skipIssue("org/repo", 10)).toBe(true); + expect(skipPr("org/repo", 55)).toBe(true); + expect(skipIssue("org/repo", 12)).toBe(false); + }); + + it("folds repo casing and never leaks a handoff into the wrong kind", () => { + const { skipIssue, skipPr } = buildCapacitySkip(HANDOFFS, "provider/model-a"); + expect(skipIssue("ORG/repo", 10)).toBe(true); + expect(skipPr("ORG/REPO", 55)).toBe(true); + // The issue handoff must not suppress a PR-fix item with the same number. + expect(skipPr("org/repo", 10)).toBe(false); + expect(skipIssue("org/repo", 55)).toBe(false); + }); +}); diff --git a/src/lib/capacity-handoff.ts b/src/lib/capacity-handoff.ts new file mode 100644 index 00000000..4df8ff1b --- /dev/null +++ b/src/lib/capacity-handoff.ts @@ -0,0 +1,189 @@ +// ─── Capacity-scoped handoff cooldowns (#1256) ─────────────────────────────── +// +// When a worker explicitly recognizes that its execution capacity cannot run a +// handed attempt, it settles the attempt with a fenced capacity handoff. The +// work item itself stays dispatchable to other compatible workers; this module +// only persists the bounded, capacity-SCOPED suppression: +// +// "capacity C should not be re-handed until T". +// +// A handoff is NOT a global wait: it never parks the item for every worker, +// never consumes task-repair budget (fixAttempts / generation are untouched), +// and never changes branch/PR identity. Workers whose declared capacity is +// unrelated keep receiving the work immediately. +// +// The capacity identifier is an opaque, worker-chosen string (for example a +// provider/model or runner-pool name). Dispatch is the authoritative owner of +// the handoff record; it does not interpret the capacity value and carries no +// coupling to any particular executor or source product. +// +// Consistency boundary: handoff rows are Dispatch-local dispatch state (like +// the lease and the queue cache), NOT mirrored to GitHub. GitHub remains the +// source of truth for labels/status; cooldown rows only shape WHO a Dispatch +// poll hands work to, and expire on their own. + +import { randomUUID } from "node:crypto"; +import { prisma } from "@/lib/prisma"; + +export type CapacityHandoffKind = "issue" | "pr_fix"; + +/** Maximum accepted length for a capacity identifier. */ +export const MAX_CAPACITY_LENGTH = 200; + +const DEFAULT_COOLDOWN_MS = 15 * 60 * 1000; // 15 minutes +const MIN_COOLDOWN_MS = 60 * 1000; // 1 minute +const MAX_COOLDOWN_MS = 6 * 60 * 60 * 1000; // 6 hours + +/** + * Bounded cooldown applied to a capacity handoff. `DISPATCH_CAPACITY_COOLDOWN_MS` + * overrides the default; the result is always clamped into [1m, 6h] so a + * misconfigured value can never park work indefinitely or churn instantly. + */ +export function capacityCooldownMs(now = Date.now()): number { + const raw = process.env.DISPATCH_CAPACITY_COOLDOWN_MS; + const parsed = raw !== undefined && raw.trim() !== "" ? Number(raw) : NaN; + const value = Number.isFinite(parsed) && parsed > 0 ? parsed : DEFAULT_COOLDOWN_MS; + return Math.min(Math.max(value, MIN_COOLDOWN_MS), MAX_COOLDOWN_MS); +} + +/** + * Validate + normalize a capacity identifier from a request. Returns the + * trimmed value, or null when absent/malformed. Routes validate before use; + * libs trust the result. + */ +export function normalizeCapacity(raw: unknown): string | null { + if (typeof raw !== "string") return null; + const trimmed = raw.trim(); + if (trimmed.length === 0 || trimmed.length > MAX_CAPACITY_LENGTH) return null; + return trimmed; +} + +/** Fresh opaque token fencing one claim (issued on lease creation, #1256). */ +export function newClaimToken(): string { + return `claim_${randomUUID()}`; +} + +/** Minimal client surface for handoff persistence/reads (test-injectable). */ +export interface CapacityHandoffClient { + capacityHandoff: { + create: (args: any) => Promise; + findMany: (args: any) => Promise; + deleteMany?: (args: any) => Promise<{ count: number }>; + }; +} + +export interface CapacityHandoffRecord { + kind: CapacityHandoffKind; + capacity: string; + repoFullName: string; + issueNumber?: number | null; + prNumber?: number | null; + agentName: string; + issueId?: string | null; + /** Attempt generation settled (pr_fix rows). */ + generation?: number | null; + note?: string | null; +} + +/** + * Capacity matching is case-folded: the identifier is opaque, and a worker + * that re-declares `Model-A` after handing off `model-a` IS the same + * capacity. Everything else about the value stays opaque to Dispatch. + */ +export function capacityEquals(a: string, b: string): boolean { + return a.toLowerCase() === b.toLowerCase(); +} + +/** + * Persist a handoff with its bounded cooldown. Returns the row (including + * `cooldownUntil`) so callers can echo the deadline to the worker. + */ +export async function recordCapacityHandoff( + client: CapacityHandoffClient, + record: CapacityHandoffRecord, + now: Date = new Date(), +): Promise<{ cooldownUntil: Date }> { + const cooldownUntil = new Date(now.getTime() + capacityCooldownMs(now.getTime())); + await client.capacityHandoff.create({ + data: { + kind: record.kind, + capacity: record.capacity, + repoFullName: record.repoFullName, + issueNumber: record.issueNumber ?? null, + prNumber: record.prNumber ?? null, + agentName: record.agentName, + issueId: record.issueId ?? null, + generation: record.generation ?? null, + cooldownUntil, + note: record.note ?? null, + }, + }); + return { cooldownUntil }; +} + +// Lazy-prune throttle: the prune is a write statement and next-task reads run +// on every worker poll, so at most one prune per minute per process. The +// active-row read still filters by cooldownUntil, so a skipped prune only +// delays dead-row removal; it never changes dispatch decisions. +let lastPruneAt = 0; +const PRUNE_INTERVAL_MS = 60_000; + +/** Active (not-yet-expired) handoffs, with throttled lazy pruning. */ +export async function loadActiveCapacityHandoffs( + client: CapacityHandoffClient, + now: Date = new Date(), +): Promise { + if (client.capacityHandoff.deleteMany && now.getTime() - lastPruneAt >= PRUNE_INTERVAL_MS) { + lastPruneAt = now.getTime(); + // Best-effort — a prune failure never blocks dispatch reads. + try { + await client.capacityHandoff.deleteMany({ where: { cooldownUntil: { lte: now } } }); + } catch { + // ignore + } + } + return client.capacityHandoff.findMany({ where: { cooldownUntil: { gt: now } } }); +} + +/** + * Build the skip predicates a dispatcher (next-task) applies to a request that + * declared `capacity` (undefined = the worker declared no capacity). + * + * Rule (fail-closed for undisclosed workers): + * - a handoff for an item suppresses re-hand when the requester's declared + * capacity EQUALS the exhausted capacity; + * - a requester that declared NO capacity is conservatively suppressed by ANY + * active handoff on the item, because Dispatch cannot prove it is not the + * exhausted capacity; + * - an unrelated declared capacity is never suppressed — this is the whole + * point of scoping the cooldown to the exhausted capacity. + * + * Repo comparison is case-folded, matching queue repo normalization. + */ +export function buildCapacitySkip( + handoffs: ReadonlyArray<{ + kind: string; + capacity: string; + repoFullName: string; + issueNumber?: number | null; + prNumber?: number | null; + }>, + requestedCapacity: string | undefined, +): { skipIssue: (repo: string | null | undefined, issueNumber: number) => boolean; skipPr: (repo: string, prNumber: number) => boolean } { + const fold = (repo: string | null | undefined) => (repo ?? "").toLowerCase(); + const issueKeys = new Set(); + const prKeys = new Set(); + for (const h of handoffs) { + const relevant = requestedCapacity === undefined || capacityEquals(h.capacity, requestedCapacity); + if (!relevant) continue; + if (h.kind === "issue" && h.issueNumber != null) { + issueKeys.add(`${fold(h.repoFullName)}#${h.issueNumber}`); + } else if (h.kind === "pr_fix" && h.prNumber != null) { + prKeys.add(`${fold(h.repoFullName)}#${h.prNumber}`); + } + } + return { + skipIssue: (repo, issueNumber) => issueKeys.has(`${fold(repo)}#${issueNumber}`), + skipPr: (repo, prNumber) => prKeys.has(`${fold(repo)}#${prNumber}`), + }; +} diff --git a/src/lib/issue-capacity-handoff.test.ts b/src/lib/issue-capacity-handoff.test.ts new file mode 100644 index 00000000..3d67152a --- /dev/null +++ b/src/lib/issue-capacity-handoff.test.ts @@ -0,0 +1,190 @@ +import { describe, expect, it, beforeEach, vi } from "vitest"; + +// The GitHub-facing surfaces are driven by mocks: the live-label re-reads, +// the targeted label removal, and the shared claim-release policy. Everything +// else (fence evaluation, CAS, cooldown, audit) runs against a fake client. +const { githubMocks, claimGateMocks, claimMocks } = vi.hoisted(() => ({ + githubMocks: { + removeIssueLabel: vi.fn().mockResolvedValue(undefined), + }, + claimGateMocks: { + getLiveIssueLabels: vi.fn(), + }, + claimMocks: { + releaseIssueClaim: vi.fn(), + }, +})); + +vi.mock("./github", () => ({ removeIssueLabel: githubMocks.removeIssueLabel })); +vi.mock("./claim-gate", () => ({ getLiveIssueLabels: claimGateMocks.getLiveIssueLabels })); +vi.mock("./issue-claim", () => ({ releaseIssueClaim: claimMocks.releaseIssueClaim })); + +import { recoverIssueCapacityHandoff, IssueCapacityHandoffClient } from "./issue-capacity-handoff"; + +const REPO = "org/repo"; +const NUMBER = 42; +const ISSUE_ID = "issue-42"; +const AGENT = "alpha"; +const TOKEN = "claim_token_abc"; + +function makeClient(overrides: Record = {}) { + const audits: any[] = []; + const mirrors: any[] = []; + const handoffs: any[] = []; + const client: any = { + audits, + mirrors, + handoffs, + issue: { + findUnique: async () => ({ + id: ISSUE_ID, + number: NUMBER, + state: "open", + labels: ["agent/alpha", "status/in-progress"], + blockedReason: null, + repository: { fullName: REPO }, + }), + update: async ({ where, data }: any) => { + mirrors.push({ where, data }); + }, + }, + lease: { + findUnique: async () => ({ + id: "lease-1", + agentName: AGENT, + issueId: ISSUE_ID, + claimToken: TOKEN, + expiredAt: new Date(Date.now() + 600_000), + }), + deleteMany: async () => ({ count: 1 }), + }, + agentWork: { + findMany: async () => [], + update: async () => ({}), + }, + agentWorkHistory: { create: async () => ({}) }, + auditLog: { + create: async ({ data }: any) => { + audits.push(data); + }, + }, + capacityHandoff: { + create: async ({ data }: any) => { + const row = { id: `handoff-${handoffs.length + 1}`, ...data }; + handoffs.push(row); + return row; + }, + }, + ...overrides, + }; + return client as IssueCapacityHandoffClient & { + audits: any[]; + mirrors: any[]; + handoffs: any[]; + }; +} + +function call(client: any, extra: Record = {}) { + return recoverIssueCapacityHandoff({ + client, + issueId: ISSUE_ID, + repoFullName: REPO, + issueNumber: NUMBER, + agentName: AGENT, + claimToken: TOKEN, + capacity: "provider/model-a", + actor: AGENT, + ...extra, + }); +} + +describe("recoverIssueCapacityHandoff (#1256)", () => { + beforeEach(() => { + vi.clearAllMocks(); + githubMocks.removeIssueLabel.mockResolvedValue(undefined); + claimGateMocks.getLiveIssueLabels.mockResolvedValue(["agent/alpha", "status/in-progress"]); + claimMocks.releaseIssueClaim.mockResolvedValue({ + labels: ["status/ready"], + status: "status/ready", + statusNote: null, + }); + }); + + it("happy path: same live owner post-CAS takes the full release policy", async () => { + const client = makeClient(); + const result = await call(client); + expect(result.ok).toBe(true); + expect(claimMocks.releaseIssueClaim).toHaveBeenCalledTimes(1); + // The full policy owns the label writes; the handoff itself adds none. + expect(githubMocks.removeIssueLabel).not.toHaveBeenCalled(); + expect(client.handoffs[0]).toMatchObject({ kind: "issue", capacity: "provider/model-a" }); + expect(client.audits.at(-1)).toMatchObject({ success: true }); + expect(client.audits.at(-1).notes).not.toMatch(/ownership-only/); + }); + + it("post-CAS re-read shows a new owner: ownership-only release, never a status transition (#1256 review)", async () => { + claimGateMocks.getLiveIssueLabels + .mockResolvedValueOnce(["agent/alpha", "status/in-progress"]) + .mockResolvedValueOnce(["agent/omega", "status/in-progress"]); + const client = makeClient(); + const result = await call(client); + expect(result.ok).toBe(true); + // The shared release policy (which flips in-progress → ready) is skipped. + expect(claimMocks.releaseIssueClaim).not.toHaveBeenCalled(); + // Only our own label is removed; the new owner's status survives. + expect(githubMocks.removeIssueLabel).toHaveBeenCalledTimes(1); + expect(githubMocks.removeIssueLabel).toHaveBeenCalledWith(REPO, NUMBER, "agent/alpha"); + if (!result.ok) throw new Error("unreachable"); + expect(result.status).toBe("status/in-progress"); + expect(result.statusNote).toMatch(/claim moved to agent\/omega/); + expect(result.statusNote).toMatch(/no status change/); + // The cache mirrors the LIVE set minus our label. + expect(client.mirrors[0].data.labels).toEqual(["agent/omega", "status/in-progress"]); + expect(client.audits.at(-1).notes).toMatch(/ownership changed post-CAS/); + }); + + it("post-CAS re-read FAILS: ownership-only cleanup, never a status transition (#1256 review)", async () => { + claimGateMocks.getLiveIssueLabels + .mockResolvedValueOnce(["agent/alpha", "status/in-progress"]) + .mockRejectedValueOnce(new Error("github unavailable")); + const client = makeClient(); + const result = await call(client); + // The lease is already consumed — halting would strand the claim — but + // ownership is UNKNOWN, so the release must never touch status. + expect(result.ok).toBe(true); + expect(claimMocks.releaseIssueClaim).not.toHaveBeenCalled(); + expect(githubMocks.removeIssueLabel).toHaveBeenCalledTimes(1); + expect(githubMocks.removeIssueLabel).toHaveBeenCalledWith(REPO, NUMBER, "agent/alpha"); + if (!result.ok) throw new Error("unreachable"); + // The resting status stays in-progress (mirrored), NOT flipped to ready: + // a stale snapshot must not clobber a possible new owner's transition. + expect(result.status).toBe("status/in-progress"); + expect(result.statusNote).toMatch(/re-read failed/); + expect(result.statusNote).toMatch(/no status change/); + expect(client.mirrors[0].data.labels).toEqual(["status/in-progress"]); + expect(client.audits.at(-1).notes).toMatch(/re-read failed/); + expect(client.audits.at(-1).notes).toMatch(/ownership-only/); + // The handoff itself (lease gone, cooldown recorded) still completes. + expect(client.handoffs[0]).toMatchObject({ kind: "issue", capacity: "provider/model-a" }); + }); + + it("no tokened claim: refuses without pointing the worker at an unscoped unclaim", async () => { + const client = makeClient({ + lease: { + findUnique: async () => ({ id: "lease-1", agentName: AGENT, issueId: ISSUE_ID, claimToken: null, expiredAt: new Date(Date.now() + 600_000) }), + deleteMany: async () => ({ count: 1 }), + }, + }); + const result = await call(client); + expect(result.ok).toBe(false); + if (result.ok) return; + expect(result.code).toBe("no_active_claim"); + expect(result.message).toMatch(/re-claim/); + // The legacy unclaim path is exactly the unfenced release this endpoint + // replaces; the refusal must not recommend it. + expect(result.message).not.toMatch(/^use unclaim|use unclaim for a legacy claim/); + expect(result.message).toMatch(/do not force an unclaim/); + expect(client.audits).toHaveLength(0); + expect(client.handoffs).toHaveLength(0); + }); +}); diff --git a/src/lib/issue-capacity-handoff.ts b/src/lib/issue-capacity-handoff.ts new file mode 100644 index 00000000..42aa6d1b --- /dev/null +++ b/src/lib/issue-capacity-handoff.ts @@ -0,0 +1,446 @@ +// ─── Fenced capacity-exhaustion recovery for issues (#1256) ────────────────── +// +// A worker that has CLAIMED an issue but discovers its execution capacity +// cannot run the task needs a way to hand the work back that is safe under +// races. Plain unclaim accepts only (issue identity, agent name): a stale +// worker can release a claim it no longer holds the moment another agent (or +// a human reassignment) moves the label, and lease expiry alone never clears +// the `agent/*` label or guarantees rediscovery. +// +// This module implements the fenced counterpart: +// +// * The caller presents the claim token Dispatch issued on the claim (the +// authoritative attempt receipt — see `Lease.claimToken`). The release is +// fenced to the EXACT claim: exact agent, exact lease row, still active, +// token equal. A re-claimed issue has a new lease row with a new token, so +// a stale worker's token is inert. +// * Ownership is verified against the LIVE GitHub label set before any write +// (#1037 rule: the claim decision lives on GitHub, not the cache). A +// different `agent/*` label, a missing own label, or a live `status/done` +// means the claim is no longer this worker's — refuse, mutate nothing. +// * Human holds are refused: closed/done issues and a deliberate block +// (`status/blocked` carrying a `blockedReason`) are human decisions this +// operation must never overwrite. +// * The ownership gate itself is a compare-and-delete on the lease row +// (`id + agentName + issueId + expiredAt > now`): exactly one racing +// handoff can take count=1; losers get 409 and write nothing. +// * On success the claim is released with the shared resting-status policy +// (`releaseIssueClaim`: in-progress → ready, so the issue is immediately +// discoverable to another compatible worker), a capacity-SCOPED cooldown +// is recorded, active AgentWork rows are retired with history, and an +// audit row closes the loop. +// +// Consistency boundary (documented in docs/capacity-handoff.md): the lease +// delete is the atomic ownership transition; GitHub label writes and the +// cache mirror happen after it. If a label write fails post-delete, the lease +// is already released (the worker has definitively lost ownership) and the +// residue — a stale `agent/*` label — is repaired by the existing stale-claim +// recovery/sync paths. The failure is audited; it can never resurrect the +// claim or block the item from rediscovery. + +import { AGENT_PREFIX, getAgentFromLabels } from "@/types"; +import { removeIssueLabel } from "./github"; +import { getLiveIssueLabels } from "./claim-gate"; +import { releaseIssueClaim } from "./issue-claim"; +import { recordCapacityHandoff, type CapacityHandoffClient } from "./capacity-handoff"; + +export interface IssueCapacityHandoffClient extends CapacityHandoffClient { + issue: { + findUnique: (args: any) => Promise; + update: (args: any) => Promise; + }; + lease: { + findUnique: (args: any) => Promise; + deleteMany: (args: any) => Promise<{ count: number }>; + }; + agentWork: { + findMany: (args: any) => Promise; + update: (args: any) => Promise; + }; + agentWorkHistory: { + create: (args: any) => Promise; + }; + auditLog: { + create: (args: any) => Promise; + }; +} + +export type IssueCapacityHandoffInput = { + client: IssueCapacityHandoffClient; + issueId: string; + repoFullName: string; + issueNumber: number; + agentName: string; + claimToken: string; + capacity: string; + note?: string | null; + /** Audit actor identity (token-derived for bound workers). */ + actor: string; + now?: Date; +}; + +export type IssueCapacityHandoffResult = + | { + ok: true; + labels: string[]; + status: string | null; + statusNote: string | null; + /** null only when the best-effort post-release cooldown write failed. */ + cooldownUntil: Date | null; + } + | { + ok: false; + /** HTTP status to surface. */ + httpStatus: number; + /** Machine-readable refusal class. */ + code: + | "issue_not_found" + | "identity_mismatch" + | "issue_closed" + | "issue_done" + | "human_hold" + | "no_active_claim" + | "stale_claim_token" + | "claim_expired" + | "ownership_changed" + | "handoff_race_lost" + | "github_unavailable" + | "release_failed"; + message: string; + }; + +/** + * Fenced capacity-exhaustion recovery for a claimed issue. See file header for + * the ownership/human-hold semantics. Never consumes task-repair budget and + * never touches PR/branch identity (branch/PR metadata stays on the lease + * history and AgentWork rows; the issue itself only loses its claim). + */ +export async function recoverIssueCapacityHandoff( + input: IssueCapacityHandoffInput, +): Promise { + const { client, issueId, repoFullName, issueNumber, agentName, claimToken, capacity } = input; + const now = input.now ?? new Date(); + const agentLabel = `${AGENT_PREFIX}${agentName}`; + + // ── 1. Identity + non-negotiable issue state (cache is fine here) ──────── + const issue = await client.issue.findUnique({ + where: { id: issueId }, + include: { repository: true }, + }); + if (!issue) { + return { ok: false, httpStatus: 404, code: "issue_not_found", message: "Issue not found in local cache" }; + } + // #1129 identity invariant: canonical repo/number come from the DB row; any + // cross-reference mix-up is refused before a single write. + if (issue.repository?.fullName !== repoFullName || issue.number !== issueNumber) { + return { + ok: false, + httpStatus: 400, + code: "identity_mismatch", + message: `issueId does not match repoFullName/issueNumber; expected ${issue.repository?.fullName}#${issue.number}`, + }; + } + if (issue.state === "closed") { + return { ok: false, httpStatus: 400, code: "issue_closed", message: "Cannot hand off a closed issue" }; + } + const cachedStatus = issue.labels.find((l: string) => l.startsWith("status/")); + if (cachedStatus === "status/done") { + return { ok: false, httpStatus: 400, code: "issue_done", message: "Cannot hand off a done issue" }; + } + // Human hold: a deliberate block carries a blockedReason; releasing the + // claim here would drag a human-owned decision back into circulation. + if (cachedStatus === "status/blocked" && (issue.blockedReason ?? null) != null) { + return { + ok: false, + httpStatus: 409, + code: "human_hold", + message: "Issue is deliberately blocked (blockedReason is set); a capacity handoff never overrides a human hold", + }; + } + + // ── 2. Fence to the exact claim (agent + lease row + issued token) ─────── + const lease = await client.lease.findUnique({ + where: { agentName_issueId: { agentName, issueId } }, + }); + if (!lease || !lease.claimToken) { + return { + ok: false, + httpStatus: 409, + code: "no_active_claim", + message: + "No active tokened claim exists for this agent+issue; this claim was already released, superseded, or predates claim tokens — re-claim to obtain a fresh fenced token (do not force an unclaim)", + }; + } + if (lease.claimToken !== claimToken) { + // A re-claim produced a new lease row with a new token. This token belongs + // to a previous (stale) claim and is inert. + return { + ok: false, + httpStatus: 409, + code: "stale_claim_token", + message: "claimToken does not match the current claim; a re-issued claim invalidates prior tokens", + }; + } + if (new Date(lease.expiredAt).getTime() <= now.getTime()) { + return { + ok: false, + httpStatus: 409, + code: "claim_expired", + message: "Claim has expired; stale-claim recovery owns release from here", + }; + } + + // ── 3. Ownership against LIVE GitHub labels (#1037), fail closed ───────── + let liveLabels: string[]; + try { + liveLabels = await getLiveIssueLabels(repoFullName, issueNumber); + } catch (error) { + const message = error instanceof Error ? error.message : "Unknown error"; + return { + ok: false, + httpStatus: 503, + code: "github_unavailable", + message: `Could not verify live GitHub labels: ${message}`, + }; + } + const refusal = classifyLiveLabels(liveLabels, agentLabel); + if (refusal) return refusal; + + // ── 4. Atomic ownership gate: CAS delete of the exact lease row ────────── + // Exactly one racing release can take count=1. A lease that expired (or was + // otherwise replaced) between the read and here yields count=0 and a 409 + // with zero writes — the new owner is never disturbed. + const { count } = await client.lease.deleteMany({ + where: { id: lease.id, agentName, issueId, expiredAt: { gt: now } }, + }); + if (count !== 1) { + return { + ok: false, + httpStatus: 409, + code: "handoff_race_lost", + message: "Claim changed concurrently (expired or released); no state was modified", + }; + } + + // ── 5. Release the claim with the shared resting-status policy ─────────── + // Re-verify live ownership AFTER the commit point. A force-claim (which + // does not delete the previous agent's lease row) or a human label change + // can land between the pre-CAS read and here; acting on the stale snapshot + // would strip a newer owner's status transition. On a changed/missing + // owner — OR WHEN THE RE-READ ITSELF FAILS — we narrow to an + // ownership-ONLY release: remove our own agent label (idempotent — a + // force-claim usually removed it already) and mirror the freshest known + // label set minus our label; touch no status label. A re-read failure must + // never take the full-release path: ownership is unknown, so a status + // transition (e.g. in-progress → ready) could clobber a newer owner's or a + // human's status. Ownership-only cleanup is safe under total ignorance — + // it writes exactly one label, our own, which only ever narrows state. + let labels: string[]; + let status: string | null; + let statusNote: string | null; + let ownershipChangedPostCas = false; + let recheckFailedPostCas = false; + let releaseLabels = liveLabels; + try { + const recheck = await getLiveIssueLabels(repoFullName, issueNumber); + if (getAgentFromLabels(recheck) !== agentLabel) { + ownershipChangedPostCas = true; + releaseLabels = recheck; + } + } catch (error) { + // Ownership unverifiable: assume it may have changed. The lease is + // already consumed, so halting would strand the claim with no owner; + // proceed with ownership-ONLY cleanup on the pre-CAS snapshot and never + // a status transition. + recheckFailedPostCas = true; + console.error(`capacity handoff: post-CAS live-label re-read failed for ${repoFullName}#${issueNumber}; releasing ownership-only:`, error); + } + + if (ownershipChangedPostCas || recheckFailedPostCas) { + const newOwner = getAgentFromLabels(releaseLabels); + try { + // Targeted, idempotent: only our own label. Never a status write — + // the new owner's transition stands. + await removeIssueLabel(repoFullName, issueNumber, agentLabel); + } catch (error) { + console.error(`capacity handoff: post-CAS label cleanup failed for ${repoFullName}#${issueNumber}:`, error); + } + labels = releaseLabels.filter((l: string) => l !== agentLabel); + status = labels.find((l: string) => l.startsWith("status/")) ?? null; + statusNote = recheckFailedPostCas + ? "live label re-read failed after the lease CAS; ownership unverifiable, so only our own agent label was removed — no status change" + : newOwner + ? `claim moved to ${newOwner} after the lease CAS; only our own agent label was removed — no status change` + : "no agent claim present after the lease CAS; mirrored GitHub minus our label — no status change"; + try { + await client.issue.update({ + where: { id: issueId }, + data: { labels, lastSyncedAt: now }, + }); + } catch (error) { + console.error(`capacity handoff: cache mirror failed for ${repoFullName}#${issueNumber}:`, error); + } + } else try { + const released = await releaseIssueClaim({ + prisma: client, + issue: { ...issue, labels: releaseLabels }, + repoFullName, + issueNumber, + agentName, + }); + labels = released.labels; + status = released.status; + statusNote = released.statusNote; + } catch (error) { + const message = error instanceof Error ? error.message : "Unknown error"; + // The lease is already deleted: the worker has definitively lost + // ownership, and any residue (a stale agent/* label on GitHub) is owned by + // stale-claim recovery. Audit the partial state explicitly. + try { + await client.auditLog.create({ + data: { + actor: input.actor, + action: "issue_capacity_handoff", + repoFullName, + issueNumber, + issueId, + beforeLabels: liveLabels, + afterLabels: [], + success: false, + errorMessage: `Lease released but claim release failed: ${message}; stale-claim recovery owns the residual agent label`, + }, + }); + } catch { + // Audit failure must not mask the refusal. + } + return { + ok: false, + httpStatus: 500, + code: "release_failed", + message: `Claim lease released but label release failed: ${message}`, + }; + } + + // ── 6. Retire active AgentWork rows with history (audit trail) ─────────── + try { + const activeWork = await client.agentWork.findMany({ + where: { agentName, issueId, state: { in: ["CLAIMED", "IN_PROGRESS", "BLOCKED"] } }, + select: { id: true }, + }); + for (const work of activeWork) { + await client.agentWork.update({ + where: { id: work.id }, + data: { state: "RELEASED", leaseExpiresAt: now }, + }); + await client.agentWorkHistory.create({ + data: { + workId: work.id, + action: "capacity_handoff", + state: "RELEASED", + summary: `Capacity exhausted (${capacity}); claim released by fenced handoff`, + }, + }); + } + } catch (error) { + // AgentWork bookkeeping is visibility, not ownership: the claim is + // already released. Log and continue — never fail the handoff over it. + console.error(`capacity handoff: AgentWork release failed for ${repoFullName}#${issueNumber}:`, error); + } + + // ── 7. Capacity-scoped cooldown + audit ────────────────────────────────── + // The release itself already committed. A cooldown-write failure must not + // turn a successful handoff into a 500 (the lease is gone; a retry would + // read 409 and the worker would believe nothing happened). Degrade to "no + // cooldown": worst case the exhausted capacity may be re-handed and report + // exhaustion again — bounded, and strictly better than a false failure. + let cooldownUntil: Date | null = null; + try { + cooldownUntil = (await recordCapacityHandoff( + client, + { + kind: "issue", + capacity, + repoFullName, + issueNumber, + agentName, + issueId, + note: input.note ?? null, + }, + now, + )).cooldownUntil; + } catch (error) { + console.error(`capacity handoff: cooldown write failed for ${repoFullName}#${issueNumber}; releasing without cooldown:`, error); + } + + try { + await client.auditLog.create({ + data: { + actor: input.actor, + action: "issue_capacity_handoff", + repoFullName, + issueNumber, + issueId, + beforeLabels: liveLabels, + afterLabels: labels, + success: true, + notes: + `capacity=${capacity}; cooldownUntil=${cooldownUntil ? cooldownUntil.toISOString() : "none (cooldown write failed)"}; ` + + `lease ${lease.id} released by claim-token fence${ + recheckFailedPostCas + ? "; post-CAS live-label re-read failed: ownership-only release of our label" + : ownershipChangedPostCas + ? "; ownership changed post-CAS: ownership-only release of our label" + : "" + }${input.note ? `; ${input.note}` : ""}`, + }, + }); + } catch { + // Audit failure is best-effort; the release already happened. + } + + return { ok: true, labels, status, statusNote, cooldownUntil }; +} + +/** + * Live-label refusals applied both before the lease CAS. Status checks read + * the LIVE set (not the cache): a `status/blocked` that exists on GitHub but + * has not reached the cache is still a hold the handoff must not reverse, so + * any live blocked label is refused fail-closed — the stale-recovery/operator + * paths own releasing one. + */ +type HandoffRefusal = Extract; + +function classifyLiveLabels( + liveLabels: string[], + agentLabel: string, +): HandoffRefusal | null { + if (liveLabels.includes("status/done")) { + return { ok: false, httpStatus: 409, code: "issue_done", message: "Issue is done on GitHub; nothing to hand off" }; + } + if (liveLabels.includes("status/blocked")) { + return { + ok: false, + httpStatus: 409, + code: "human_hold", + message: "Issue carries status/blocked on GitHub; a capacity handoff never reverses a block — release the block explicitly first", + }; + } + const liveAgent = getAgentFromLabels(liveLabels); + if (liveAgent && liveAgent !== agentLabel) { + return { + ok: false, + httpStatus: 409, + code: "ownership_changed", + message: `Issue claim on GitHub belongs to ${liveAgent}; refusing to release another owner's claim`, + }; + } + if (!liveAgent) { + return { + ok: false, + httpStatus: 409, + code: "ownership_changed", + message: "No agent claim is present on GitHub; this handoff's claim is already released or was changed by a human", + }; + } + return null; +} diff --git a/src/lib/lease.test.ts b/src/lib/lease.test.ts index 7bec8a95..5e141741 100644 --- a/src/lib/lease.test.ts +++ b/src/lib/lease.test.ts @@ -408,3 +408,84 @@ describe("findLeasedIssueIds", () => { expect(result).toEqual([]); }); }); + +describe("claim token fencing (#1256)", () => { + it("mints an opaque claim token on lease creation", async () => { + mocks.leaseFindUnique.mockResolvedValue(null); + mocks.leaseCreate.mockImplementation((args: any) => Promise.resolve({ id: "l-1", ...args.data })); + + const result = await upsertLease({ agentName: "saffron", issueId: "i-1" }); + + const token = result.lease.claimToken; + expect(typeof token).toBe("string"); + expect(token.length).toBeGreaterThan(10); + expect(token).toMatch(/^claim_/); + }); + + it("issues a distinct token for every new claim (re-claims are fenced)", async () => { + mocks.leaseFindUnique.mockResolvedValue(null); + mocks.leaseCreate.mockImplementation((args: any) => Promise.resolve({ id: "l-1", ...args.data })); + + await upsertLease({ agentName: "saffron", issueId: "i-1" }); + await upsertLease({ agentName: "saffron", issueId: "i-1" }); + + const [first, second] = mocks.leaseCreate.mock.calls.map((c: any) => c[0].data.claimToken); + expect(first).not.toBe(second); + }); + + it("keeps the claim token stable across renewals (heartbeats do not invalidate it)", async () => { + mocks.leaseFindUnique.mockResolvedValue(makeLease({ id: "l-1", claimToken: "claim_stable" })); + mocks.leaseUpdate.mockImplementation((args: any) => + Promise.resolve({ ...makeLease(args.where), claimToken: "claim_stable", ...args.data }), + ); + + await upsertLease({ agentName: "saffron", issueId: "i-1", checkpoint: "branch_created" }); + + const updateData = (mocks.leaseUpdate.mock.calls[0] as any)[0].data; + // Renewal must NOT mint or rewrite a token — the issued receipt stays valid. + expect(updateData.claimToken).toBeUndefined(); + }); + + it("exposes the claim token on the resume context so a resuming worker keeps its receipt", async () => { + mocks.leaseFindFirst.mockResolvedValue({ + id: "l-1", + agentName: "saffron", + issueId: "i-1", + checkpoint: "issue_claimed", + branch: null, + prUrl: null, + claimToken: "claim_resume-me", + expiredAt: new Date(Date.now() + 60_000), + renewedAt: makeNow(), + createdAt: makeNow(), + issue: { number: 42, repository: { fullName: "misospace/dispatch" } }, + }); + mocks.issueFindUnique.mockResolvedValue({ id: "i-1" }); + + const result = await resolveActiveWork("saffron"); + + expect(result).not.toBeNull(); + expect(result!.claimToken).toBe("claim_resume-me"); + }); + + it("leaves claimToken undefined for legacy token-less leases", async () => { + mocks.leaseFindFirst.mockResolvedValue({ + id: "l-1", + agentName: "saffron", + issueId: "i-1", + checkpoint: "issue_claimed", + branch: null, + prUrl: null, + claimToken: null, + expiredAt: new Date(Date.now() + 60_000), + renewedAt: makeNow(), + createdAt: makeNow(), + issue: { number: 42, repository: { fullName: "misospace/dispatch" } }, + }); + mocks.issueFindUnique.mockResolvedValue({ id: "i-1" }); + + const result = await resolveActiveWork("saffron"); + + expect(result!.claimToken).toBeUndefined(); + }); +}); diff --git a/src/lib/lease.ts b/src/lib/lease.ts index 006b1343..4ceeb4ed 100644 --- a/src/lib/lease.ts +++ b/src/lib/lease.ts @@ -8,6 +8,7 @@ import { Prisma } from "@prisma/client"; import { prisma } from "@/lib/prisma"; +import { newClaimToken } from "@/lib/capacity-handoff"; import type { CheckpointValue, ResumeContext, ResumeContextWithLease } from "./next-action"; import { buildResumeContext, isValidCheckpoint } from "./next-action"; @@ -65,6 +66,12 @@ export async function upsertLease(params: { // @@unique([agentName, issueId]) constraint. The loser hits a P2002 // unique violation — re-read and return the lease the winner created // instead of surfacing a 500 to the caller. + // + // #1256: creation mints the opaque claim token that fences later + // capacity-handoff releases to THIS exact claim. Renewals deliberately keep + // the token (a heartbeating worker must not lose its receipt); a released or + // expired lease that is re-claimed is a NEW row with a NEW token, so old + // tokens are inert against new claims. try { const lease = await prisma.lease.create({ data: { @@ -74,6 +81,7 @@ export async function upsertLease(params: { checkpoint, branch: params.branch, prUrl: params.prUrl, + claimToken: newClaimToken(), }, }); return { created: true, lease }; @@ -214,6 +222,9 @@ export async function resolveActiveWork(agentName: string): Promise): Record void) | null; @@ -75,10 +76,42 @@ function makeClient(): PrFixQueueClient & { } { const items: any[] = []; const history: any[] = []; + // #1256: capacity-scoped cooldown rows. + const handoffs: any[] = []; let seq = 0; const client: any = { items, history, + handoffs, + capacityHandoff: { + create: async ({ data }: any) => { + const row = { id: `handoff-${handoffs.length + 1}`, createdAt: new Date(), ...data }; + handoffs.push(row); + return row; + }, + findMany: async ({ where }: any) => + handoffs.filter( + (h) => + (where?.kind === undefined || h.kind === where.kind) && + (where?.repoFullName === undefined || String(h.repoFullName).toLowerCase() === String(where.repoFullName).toLowerCase()) && + (where?.prNumber === undefined || h.prNumber === where.prNumber) && + (where?.cooldownUntil === undefined || + (where.cooldownUntil.gt != null && new Date(h.cooldownUntil).getTime() > new Date(where.cooldownUntil.gt).getTime())), + ), + deleteMany: async ({ where }: any) => { + const before = handoffs.length; + for (let i = handoffs.length - 1; i >= 0; i--) { + const h = handoffs[i]; + if ( + where?.cooldownUntil?.lte != null && + new Date(h.cooldownUntil).getTime() <= new Date(where.cooldownUntil.lte).getTime() + ) { + handoffs.splice(i, 1); + } + } + return { count: before - handoffs.length }; + }, + }, // #1124: true while a $transaction callback is executing. Tests use it to // prove no network call (e.g. the post-dispatch head fetch) runs inside a // transaction. @@ -127,6 +160,9 @@ function makeClient(): PrFixQueueClient & { // #1211 hand-out ack/reclaim columns: default to their schema values // so a freshly created row reads back as "no ack, never reclaimed". handoutAcks: [], // mirrors the column's @default([]) + agentHandouts: [], // mirrors the column's @default([]) + // #1256 review: per-hand-out receipts (bijection with agentHandouts). + handoutReceipts: [], // mirrors the column's @default([]) handoutReclaims: 0, // mirrors the column's @default(0) retryAt: null, // #1218 outcome classification columns: default to null so a freshly @@ -172,13 +208,21 @@ function makeClient(): PrFixQueueClient & { (where.id === undefined || i.id === where.id) && (where.generation === undefined || i.generation === where.generation) && (where.status === undefined || i.status === where.status) && + (where.dispatchedGeneration === undefined || i.dispatchedGeneration === where.dispatchedGeneration) && (where.retryAt === undefined || (where.retryAt.lte && i.retryAt != null && new Date(i.retryAt).getTime() <= new Date(where.retryAt.lte).getTime()) || i.retryAt === where.retryAt) && (where.postDispatchEvidenceKeys === undefined || JSON.stringify(i.postDispatchEvidenceKeys ?? []) === JSON.stringify(where.postDispatchEvidenceKeys.equals ?? [])) && (where.handoutAcks === undefined || JSON.stringify(i.handoutAcks ?? []) === - JSON.stringify(where.handoutAcks.equals ?? [])), + JSON.stringify(where.handoutAcks.equals ?? [])) && + // #1256 review: receipt/hand-out list pins (exact-owner CAS). + (where.handoutReceipts === undefined || + JSON.stringify(i.handoutReceipts ?? []) === + JSON.stringify(where.handoutReceipts.equals ?? [])) && + (where.agentHandouts === undefined || + JSON.stringify(i.agentHandouts ?? []) === + JSON.stringify(where.agentHandouts.equals ?? [])), ); for (const match of matches) { const idx = items.findIndex((i) => i.id === match.id); @@ -4151,6 +4195,313 @@ describe("PR-fix outcome separation (#1218)", () => { githubPrsMocks.fetchPullRequestMergeState.mockResolvedValue({ mergeableState: null, mergeable: null }); }); + // #1256 review: a valid hand-out is seeded as the live triple the route + // produces — agentHandouts entry + handoutReceipts receipt + stamp. + function seedHandout(row: any, agent: string, generation: number, token: string) { + row.agentHandouts = [...(row.agentHandouts ?? []), `${agent}@${generation}`]; + row.handoutReceipts = [...(row.handoutReceipts ?? []), `${token}@${agent}@${generation}`]; + row.dispatchedGeneration = generation; + row.dispatchedAt = new Date(); + return token; + } + + it("capacity handoff releases only the issued generation without using retry budget or losing PR metadata (#1256)", async () => { + await enqueuePrFixItem(client, { + repo: "org/repo", pr: 300, lane: "NORMAL", reason: "r", feedback: "f", evidenceKey: "k300", + }); + const row = client.items[0]; + row.branch = "work/300"; + row.url = "https://github.com/org/repo/pull/300"; + seedHandout(row, "worker-a", 1, "hnd_aaaaaaaaaaaaaaaa"); + row.postDispatchEvidenceKeys = ["review:o/r#7:r1@H1"]; + const result = await resolvePrFixFromAgentReport({ + client: client as PrFixQueueClient, repoFullName: "org/repo", pullRequestNumber: 300, + outcome: "capacity-exhausted", capacity: "provider/model", agentName: "worker-a", + attempt: { itemId: row.id, generation: 1, handoutToken: "hnd_aaaaaaaaaaaaaaaa" }, + }); + expect(result.action).toBe("deferred"); + const released = client.items[0]; + // The item STAYS QUEUED at the same generation: no global park, no retry + // budget spent, PR/branch identity retained. + expect(released.status).toBe("QUEUED"); + expect(released.retryAt).toBeNull(); + expect(released.fixAttempts).toBe(1); + expect(released.generation).toBe(1); + expect(released.branch).toBe("work/300"); + expect(released.url).toBe("https://github.com/org/repo/pull/300"); + // The consumed hand-out is cleared so any compatible worker can take it: + // the last live receipt resets the stamps AND the post-dispatch list. + expect(released.agentHandouts).toEqual([]); + expect(released.handoutReceipts).toEqual([]); + expect(released.dispatchedGeneration).toBeNull(); + expect(released.postDispatchEvidenceKeys).toEqual([]); + expect(client.history.at(-1)).toMatchObject({ action: "capacity_handoff", status: "QUEUED", reason: "provider/model" }); + // The suppression is capacity-SCOPED, not a global wait. + expect(client.handoffs).toHaveLength(1); + expect(client.handoffs[0]).toMatchObject({ + kind: "pr_fix", capacity: "provider/model", repoFullName: "org/repo", prNumber: 300, + agentName: "worker-a", + }); + expect(client.handoffs[0].cooldownUntil).toBeInstanceOf(Date); + expect(client.handoffs[0].cooldownUntil.getTime()).toBeGreaterThan(Date.now()); + }); + + it("repeated capacity-exhausted report replays idempotently without minting a second cooldown (#1256)", async () => { + await enqueuePrFixItem(client, { repo: "org/repo", pr: 301, lane: "NORMAL", reason: "r", feedback: "f", evidenceKey: "k301" }); + const row = client.items[0]; + seedHandout(row, "worker-a", 1, "hnd_aaaaaaaaaaaaaaaa"); + const first = await resolvePrFixFromAgentReport({ + client: client as PrFixQueueClient, repoFullName: "org/repo", pullRequestNumber: 301, + outcome: "capacity-exhausted", capacity: "provider/model", agentName: "worker-a", + attempt: { itemId: row.id, generation: 1, handoutToken: "hnd_aaaaaaaaaaaaaaaa" }, + }); + expect(first.action).toBe("deferred"); + const repeat = await resolvePrFixFromAgentReport({ + client: client as PrFixQueueClient, repoFullName: "org/repo", pullRequestNumber: 301, + outcome: "capacity-exhausted", capacity: "provider/model", agentName: "worker-a", + attempt: { itemId: row.id, generation: 1, handoutToken: "hnd_aaaaaaaaaaaaaaaa" }, + }); + expect(repeat.action).toBe("deferred"); + expect(client.handoffs).toHaveLength(1); + expect(client.history.filter((h: any) => h.action === "capacity_handoff")).toHaveLength(1); + }); + + it("a different capacity exhausting the same live generation records its own scoped cooldown (#1256)", async () => { + await enqueuePrFixItem(client, { repo: "org/repo", pr: 305, lane: "NORMAL", reason: "r", feedback: "f", evidenceKey: "k305" }); + const row = client.items[0]; + // Capacity A already handed this generation off (its receipt was consumed); + // capacity B was then dispatched at the same generation with its own + // receipt and exhausts too. Capacity is caller-declared trusted scoping: + // settlement is authorized by the RECEIPT, never by agent identity matching. + seedHandout(row, "worker-b", 1, "hnd_bbbbbbbbbbbbbbbb"); + client.handoffs.push({ + id: "handoff-a", kind: "pr_fix", capacity: "model-a", repoFullName: "org/repo", + prNumber: 305, generation: 1, cooldownUntil: new Date(Date.now() + 600_000), + }); + const result = await resolvePrFixFromAgentReport({ + client: client as PrFixQueueClient, repoFullName: "org/repo", pullRequestNumber: 305, + outcome: "capacity-exhausted", capacity: "model-b", agentName: "worker-b", + attempt: { itemId: row.id, generation: 1, handoutToken: "hnd_bbbbbbbbbbbbbbbb" }, + }); + expect(result.action).toBe("deferred"); + expect(client.handoffs).toHaveLength(2); + expect(client.handoffs[1]).toMatchObject({ capacity: "model-b", generation: 1 }); + }); + + it("a duplicate concurrent report loses the hand-out-stamp CAS (#1256)", async () => { + await enqueuePrFixItem(client, { repo: "org/repo", pr: 306, lane: "NORMAL", reason: "r", feedback: "f", evidenceKey: "k306" }); + const row = client.items[0]; + // The duplicate that loses arrives after the winner consumed the issued + // hand-out stamp (dispatchedGeneration cleared) but before the winner's + // cooldown row became visible: the stamp-pinned CAS is the second fence. + seedHandout(row, "worker-a", 1, "hnd_aaaaaaaaaaaaaaaa"); + row.dispatchedGeneration = null; + row.dispatchedAt = null; + const result = await resolvePrFixFromAgentReport({ + client: client as PrFixQueueClient, repoFullName: "org/repo", pullRequestNumber: 306, + outcome: "capacity-exhausted", capacity: "model-a", agentName: "worker-a", + attempt: { itemId: row.id, generation: 1, handoutToken: "hnd_aaaaaaaaaaaaaaaa" }, + }); + expect(result.action).toBe("skipped"); + expect(client.handoffs).toHaveLength(0); + expect(client.history.filter((h: any) => h.action === "capacity_handoff")).toHaveLength(0); + }); + + it("capacity handoff loses a generation race with zero mutation (#1256)", async () => { + await enqueuePrFixItem(client, { repo: "org/repo", pr: 302, lane: "NORMAL", reason: "r", feedback: "f", evidenceKey: "k302" }); + const row = client.items[0]; + seedHandout(row, "worker-a", 1, "hnd_aaaaaaaaaaaaaaaa"); + // The generation moves inside the read→write gap (fresh attempt re-issued + // concurrently): the CAS must miss and write nothing. + client.hooks.beforeUpdateMany = () => { + const idx = client.items.findIndex((i: any) => i.id === row.id); + client.items[idx] = { ...client.items[idx], generation: 2 }; + }; + const result = await resolvePrFixFromAgentReport({ + client: client as PrFixQueueClient, repoFullName: "org/repo", pullRequestNumber: 302, + outcome: "capacity-exhausted", capacity: "provider/model", agentName: "worker-a", + attempt: { itemId: row.id, generation: 1, handoutToken: "hnd_aaaaaaaaaaaaaaaa" }, + }); + expect(result.action).toBe("skipped"); + expect(client.handoffs).toHaveLength(0); + expect(client.history.filter((h: any) => h.action === "capacity_handoff")).toHaveLength(0); + // The newer generation's dispatch tracking is untouched. + expect(client.items[0].agentHandouts).toEqual(["worker-a@1"]); + expect(client.items[0].handoutReceipts).toEqual(["hnd_aaaaaaaaaaaaaaaa@worker-a@1"]); + expect(client.items[0].dispatchedGeneration).toBe(1); + }); + + it("an unrelated agent cannot release the current hand-out even with the live (id, generation) (#1256 review)", async () => { + await enqueuePrFixItem(client, { repo: "org/repo", pr: 310, lane: "NORMAL", reason: "r", feedback: "f", evidenceKey: "k310" }); + const row = client.items[0]; + seedHandout(row, "worker-b", 1, "hnd_bbbbbbbbbbbbbbbb"); + row.handoutAcks = ["worker-b@1"]; + const before = { ...client.items[0] }; + // A snooping agent knows the (id, generation) token from the queue but + // holds no receipt: the exact-owner CAS refuses with zero mutation. + const result = await resolvePrFixFromAgentReport({ + client: client as PrFixQueueClient, repoFullName: "org/repo", pullRequestNumber: 310, + outcome: "capacity-exhausted", capacity: "model-x", agentName: "worker-snoop", + attempt: { itemId: row.id, generation: 1, handoutToken: "hnd_ffffffffffffffff" }, + }); + expect(result.action).toBe("skipped"); + expect(result.reason).toMatch(/no live hand-out receipt/); + expect(client.items[0]).toEqual(before); + expect(client.handoffs).toHaveLength(0); + expect(client.history.filter((h: any) => h.action === "capacity_handoff")).toHaveLength(0); + }); + + it("a stale token from a PREVIOUS same-agent hand-out of the SAME generation cannot release the live one (#1256 review)", async () => { + await enqueuePrFixItem(client, { repo: "org/repo", pr: 311, lane: "NORMAL", reason: "r", feedback: "f", evidenceKey: "k311" }); + const row = client.items[0]; + // worker-a's first hand-out was released (its receipt consumed with it, + // per the receipt↔hand-out bijection) and worker-a was re-handed the SAME + // generation with a rotated receipt. The late report still carries the + // old token. + seedHandout(row, "worker-a", 1, "hnd_111111111111111a"); + const before = { ...client.items[0] }; + const result = await resolvePrFixFromAgentReport({ + client: client as PrFixQueueClient, repoFullName: "org/repo", pullRequestNumber: 311, + outcome: "capacity-exhausted", capacity: "model-a", agentName: "worker-a", + attempt: { itemId: row.id, generation: 1, handoutToken: "hnd_000000000000000a" }, + }); + expect(result.action).toBe("skipped"); + // The live rotated receipt and the dispatch stamps survive. + expect(client.items[0]).toEqual(before); + expect(client.handoffs).toHaveLength(0); + }); + + it("preserves other agents' live receipts, acks, and the dispatch stamps (#1256 review)", async () => { + await enqueuePrFixItem(client, { repo: "org/repo", pr: 312, lane: "NORMAL", reason: "r", feedback: "f", evidenceKey: "k312" }); + const row = client.items[0]; + seedHandout(row, "worker-a", 1, "hnd_aaaaaaaaaaaaaaaa"); + row.agentHandouts.push("worker-b@1"); + row.handoutReceipts.push("hnd_bbbbbbbbbbbbbbbb@worker-b@1"); + row.handoutAcks = ["worker-b@1"]; + row.postDispatchEvidenceKeys = ["review:o/r#7:r1@H1"]; + const result = await resolvePrFixFromAgentReport({ + client: client as PrFixQueueClient, repoFullName: "org/repo", pullRequestNumber: 312, + outcome: "capacity-exhausted", capacity: "model-a", agentName: "worker-a", + attempt: { itemId: row.id, generation: 1, handoutToken: "hnd_aaaaaaaaaaaaaaaa" }, + }); + expect(result.action).toBe("deferred"); + const after = client.items[0]; + // Only worker-a's receipt/hand-out/ack were removed; worker-b's live + // attempt survives intact. + expect(after.agentHandouts).toEqual(["worker-b@1"]); + expect(after.handoutReceipts).toEqual(["hnd_bbbbbbbbbbbbbbbb@worker-b@1"]); + expect(after.handoutAcks).toEqual(["worker-b@1"]); + // Another hand-out is still live at this generation: the stamps and the + // post-dispatch evidence list MUST survive (clearing them would + // reclassify worker-b's mid-run evidence as pre-dispatch, #1119). + expect(after.dispatchedGeneration).toBe(1); + expect(after.postDispatchEvidenceKeys).toEqual(["review:o/r#7:r1@H1"]); + expect(client.handoffs).toHaveLength(1); + }); + + it("a hand-out racing into the read→write gap pins the CAS to a no-op (#1256 review)", async () => { + await enqueuePrFixItem(client, { repo: "org/repo", pr: 313, lane: "NORMAL", reason: "r", feedback: "f", evidenceKey: "k313" }); + const row = client.items[0]; + seedHandout(row, "worker-a", 1, "hnd_aaaaaaaaaaaaaaaa"); + // A concurrent next-task hand-out to worker-c lands right before the + // settlement write: the pinned receipt/hand-out lists miss. + client.hooks.beforeUpdateMany = () => { + const idx = client.items.findIndex((i: any) => i.id === row.id); + client.items[idx] = { + ...client.items[idx], + agentHandouts: [...client.items[idx].agentHandouts, "worker-c@1"], + handoutReceipts: [...client.items[idx].handoutReceipts, "hnd_cccccccccccccccc@worker-c@1"], + }; + }; + const result = await resolvePrFixFromAgentReport({ + client: client as PrFixQueueClient, repoFullName: "org/repo", pullRequestNumber: 313, + outcome: "capacity-exhausted", capacity: "model-a", agentName: "worker-a", + attempt: { itemId: row.id, generation: 1, handoutToken: "hnd_aaaaaaaaaaaaaaaa" }, + }); + expect(result.action).toBe("skipped"); + const after = client.items[0]; + expect(after.agentHandouts).toEqual(["worker-a@1", "worker-c@1"]); + expect(after.handoutReceipts).toEqual([ + "hnd_aaaaaaaaaaaaaaaa@worker-a@1", + "hnd_cccccccccccccccc@worker-c@1", + ]); + expect(after.dispatchedGeneration).toBe(1); + expect(client.handoffs).toHaveLength(0); + }); + + it("a pre-receipt (legacy) hand-out cannot settle capacity-exhausted (#1256 review)", async () => { + await enqueuePrFixItem(client, { repo: "org/repo", pr: 314, lane: "NORMAL", reason: "r", feedback: "f", evidenceKey: "k314" }); + const row = client.items[0]; + // A hand-out issued before receipts existed: record + stamp, no receipt. + row.agentHandouts = ["worker-a@1"]; + row.dispatchedGeneration = 1; + row.dispatchedAt = new Date(); + const result = await resolvePrFixFromAgentReport({ + client: client as PrFixQueueClient, repoFullName: "org/repo", pullRequestNumber: 314, + outcome: "capacity-exhausted", capacity: "model-a", agentName: "worker-a", + attempt: { itemId: row.id, generation: 1, handoutToken: "hnd_aaaaaaaaaaaaaaaa" }, + }); + expect(result.action).toBe("skipped"); + expect(client.items[0].agentHandouts).toEqual(["worker-a@1"]); + expect(client.items[0].dispatchedGeneration).toBe(1); + expect(client.handoffs).toHaveLength(0); + }); + + it("capacity handoff refuses a stale generation before any write (#1256)", async () => { + await enqueuePrFixItem(client, { repo: "org/repo", pr: 303, lane: "NORMAL", reason: "r", feedback: "f", evidenceKey: "k303" }); + const row = client.items[0]; + row.generation = 2; // a fresh attempt already exists + const result = await resolvePrFixFromAgentReport({ + client: client as PrFixQueueClient, repoFullName: "org/repo", pullRequestNumber: 303, + outcome: "capacity-exhausted", capacity: "provider/model", + attempt: { itemId: row.id, generation: 1 }, + }); + expect(result.action).toBe("skipped"); + expect(client.handoffs).toHaveLength(0); + }); + + it("does not overwrite a human hold on capacity handoff (#1256)", async () => { + await enqueuePrFixItem(client, { repo: "org/repo", pr: 299, lane: "NORMAL", reason: "r", feedback: "f", evidenceKey: "k299" }); + const row = client.items[0]; + row.status = "BLOCKED"; + row.lane = "NEEDS_HUMAN"; + const result = await resolvePrFixFromAgentReport({ + client: client as PrFixQueueClient, repoFullName: "org/repo", pullRequestNumber: 299, + outcome: "capacity-exhausted", capacity: "provider/model", + attempt: { itemId: row.id, generation: 1 }, + }); + expect(result.action).toBe("skipped"); + expect(row.status).toBe("BLOCKED"); + expect(row.lane).toBe("NEEDS_HUMAN"); + expect(client.handoffs).toHaveLength(0); + expect(client.history).toHaveLength(1); + }); + + it("capacity handoff requires an explicit capacity and an issued attempt token (#1256)", async () => { + await enqueuePrFixItem(client, { repo: "org/repo", pr: 304, lane: "NORMAL", reason: "r", feedback: "f", evidenceKey: "k304" }); + const row = client.items[0]; + const noCapacity = await resolvePrFixFromAgentReport({ + client: client as PrFixQueueClient, repoFullName: "org/repo", pullRequestNumber: 304, + outcome: "capacity-exhausted", attempt: { itemId: row.id, generation: 1 }, + }); + expect(noCapacity.action).toBe("skipped"); + const legacy = await resolvePrFixFromAgentReport({ + client: client as PrFixQueueClient, repoFullName: "org/repo", pullRequestNumber: 304, + outcome: "capacity-exhausted", capacity: "provider/model", attempt: null, + }); + expect(legacy.action).toBe("skipped"); + // #1256 review: the receipt token itself is required — (id, generation) + // alone identifies the item, not the exact hand-out. + const noReceiptToken = await resolvePrFixFromAgentReport({ + client: client as PrFixQueueClient, repoFullName: "org/repo", pullRequestNumber: 304, + outcome: "capacity-exhausted", capacity: "provider/model", agentName: "worker-a", + attempt: { itemId: row.id, generation: 1 }, + }); + expect(noReceiptToken.action).toBe("skipped"); + expect(client.handoffs).toHaveLength(0); + expect(client.items[0].status).toBe("QUEUED"); + }); + it("parks a failed report past the fix-attempt cap and never labels it human", async () => { const prev = process.env.PR_FIX_MAX_ATTEMPTS; process.env.PR_FIX_MAX_ATTEMPTS = "1"; diff --git a/src/lib/pr-fix-queue.ts b/src/lib/pr-fix-queue.ts index e35a0715..1c9a8de8 100644 --- a/src/lib/pr-fix-queue.ts +++ b/src/lib/pr-fix-queue.ts @@ -1,7 +1,9 @@ +import { randomBytes } from "node:crypto"; import { normalizePrFixLane, normalizePrFixStatus, normalizePrFixType, normalizePrFixOutcome, PrFixLane, PrFixOutcome, PrFixStatus, PrFixType, PrFixDecisionContext, PR_FIX_TYPE_PRIORITY } from "@/types"; import { surfacePrFixBlocked, surfacePrFixParked, surfacePrFixRequeued, surfacePrFixUnblocked, extractUrlsFromText, type PrFixUnblockOutcome } from "./pr-fix-surfacing"; import { prisma } from "@/lib/prisma"; import { fetchPullRequestMergeState, fetchPullRequestHeadSha, fetchPullRequestState } from "./github-prs"; +import { capacityCooldownMs, capacityEquals } from "./capacity-handoff"; import { fetchRepositoryMetadata } from "./github-code-search"; import { sanitizeForStorage } from "./groomer/sanitize"; @@ -20,6 +22,13 @@ export type PrFixQueueClient = { create: (args: any) => Promise; findMany?: (args: any) => Promise; }; + // #1256: capacity-scoped cooldown rows. Optional so legacy/test clients + // without the table still settle other outcomes; capacity-exhausted + // settlement records the cooldown whenever the surface exists. + capacityHandoff?: { + create: (args: any) => Promise; + findMany?: (args: any) => Promise; + }; $transaction: (fn: (tx: PrFixQueueClient) => Promise) => Promise; }; @@ -562,7 +571,7 @@ async function reactivateDueExternalWaits(client: PrFixQueueClient, now: Date): await client.$transaction(async (tx) => { const { count } = await tx.prFixQueueItem.updateMany({ where: { id: row.id, status: "WAITING_EXTERNAL", generation: row.generation, retryAt: row.retryAt }, - data: { status: "QUEUED", ...freshAttemptGeneration(), retryAt: null, dispatchedGeneration: null, dispatchedAt: null, postDispatchEvidenceKeys: [], attemptHeadSha: row.headSha ?? row.attemptHeadSha ?? null, outcome: null, decisionDetail: null, agentHandouts: [], handoutAcks: [] }, + data: { status: "QUEUED", ...freshAttemptGeneration(), retryAt: null, dispatchedGeneration: null, dispatchedAt: null, postDispatchEvidenceKeys: [], attemptHeadSha: row.headSha ?? row.attemptHeadSha ?? null, outcome: null, decisionDetail: null, agentHandouts: [], handoutAcks: [], handoutReceipts: [] }, }); if (count === 1) await tx.prFixHistory.create({ data: { itemId: row.id, action: "mark", status: "QUEUED", lane: row.lane, note: "External-wait cooldown elapsed; retrying prerequisite check." } }); }); @@ -912,6 +921,7 @@ export async function reclaimStalePrFixHandouts( status: "STALE", agentHandouts: [], handoutAcks: [], + handoutReceipts: [], dispatchedGeneration: null, dispatchedAt: null, }, @@ -958,6 +968,7 @@ export async function reclaimStalePrFixHandouts( decisionDetail: null, agentHandouts: [], handoutAcks: [], + handoutReceipts: [], dispatchedGeneration: null, dispatchedAt: null, }, @@ -1026,6 +1037,7 @@ export async function reclaimStalePrFixHandouts( handoutReclaims: { increment: 1 }, agentHandouts: [], handoutAcks: [], + handoutReceipts: [], postDispatchEvidenceKeys: [], dispatchedGeneration: null, dispatchedAt: null, @@ -1240,7 +1252,7 @@ export async function enqueuePrFixItem(client: PrFixQueueClient, rawInput: Enque // way, and the skip check must match only the current identity // (#1119, #1133, #1211). ...(isFreshAttempt - ? { postDispatchEvidenceKeys: [], agentHandouts: [], handoutAcks: [] } + ? { postDispatchEvidenceKeys: [], agentHandouts: [], handoutAcks: [], handoutReceipts: [] } : {}), // Structured outcome (#1218): recorded on the same write as the // status. Only BLOCKED/PARKED carry one; decisionDetail is persisted @@ -1618,6 +1630,7 @@ export async function markPrFixItem( data.dispatchedGeneration = null; data.dispatchedAt = null; data.agentHandouts = []; + data.handoutReceipts = []; data.handoutAcks = []; } @@ -1688,6 +1701,7 @@ export async function markPrFixItem( postDispatchEvidenceKeys: [], agentHandouts: [], handoutAcks: [], + handoutReceipts: [], } : { status: "QUEUED", @@ -1703,6 +1717,7 @@ export async function markPrFixItem( // prior generation's acknowledgements are dead (#1211). agentHandouts: [], handoutAcks: [], + handoutReceipts: [], }; const { count } = await tx.prFixQueueItem.updateMany({ where: { @@ -1806,6 +1821,7 @@ export async function markPrFixItem( // match must not survive the identity change (#1133). data.postDispatchEvidenceKeys = []; data.agentHandouts = []; + data.handoutReceipts = []; data.handoutAcks = []; } // QUEUED → QUEUED: leave the key list untouched — the entries still refer @@ -1851,6 +1867,7 @@ export async function markPrFixItem( postDispatchEvidenceKeys: [], agentHandouts: [], handoutAcks: [], + handoutReceipts: [], } : { status: "QUEUED", @@ -1867,6 +1884,7 @@ export async function markPrFixItem( // acknowledgements (#1133, #1211). agentHandouts: [], handoutAcks: [], + handoutReceipts: [], }; const refusal = await client.$transaction(async (tx) => { const { count } = await tx.prFixQueueItem.updateMany({ @@ -2134,6 +2152,7 @@ export async function markPrFixItem( postDispatchEvidenceKeys: [], agentHandouts: [], handoutAcks: [], + handoutReceipts: [], } : { status: "QUEUED", @@ -2149,6 +2168,7 @@ export async function markPrFixItem( // prior generation's acknowledgements are dead (#1211). agentHandouts: [], handoutAcks: [], + handoutReceipts: [], }; const forced = await client.$transaction(async (tx) => { const { count } = await tx.prFixQueueItem.updateMany({ @@ -2351,6 +2371,78 @@ export function parseAgentHandoutToken(entry: unknown): { agentName: string; gen return { agentName: entry.slice(0, idx), generation }; } +// ── Per-hand-out receipts (#1256 review) ─────────────────────────────────── +// +// `agentHandouts` answers "was this generation already handed to agent X", +// which is all ordinary dispatch needs. Capacity-exhausted settlement needs +// more: it must authorize the EXACT hand-out attempt a worker was issued, so +// that (a) an unrelated agent that merely knows the (id, generation) token can +// never release the current hand-out, and (b) a stale token from a PREVIOUS +// hand-out of the SAME agent at the SAME generation (release/reclaim keep the +// generation) cannot release the live one. Generation alone cannot express +// either — the receipt carries a fresh opaque token per hand-out. +// +// Receipt entry format: `@@`. The token is +// Dispatch-minted hex (never contains "@"), so the token boundary is the +// FIRST "@"; the agent name may contain "@", so its boundary with the +// generation stays the LAST "@" (same rule as `agentHandoutToken`). +// +// Invariant (enforced at every write site below): a receipt exists exactly +// while the matching `agentHandouts` entry exists — issuance pushes both in +// one conditional write, every site that clears `agentHandouts` clears +// `handoutReceipts` in the same data object. A released/reclaimed/ +// superseded hand-out therefore has no live receipt, and every NEW hand-out +// rotates in a fresh token. + +/** Fresh opaque per-hand-out token. Opaque to everyone but the matcher. */ +export function newHandoutToken(): string { + return `hnd_${randomBytes(12).toString("hex")}`; +} + +/** Encoding of a persisted hand-out owner receipt. */ +export function handoutReceipt(token: string, agentName: string, generation: number): string { + return `${token}@${agentName}@${generation}`; +} + +export function parseHandoutReceipt( + entry: unknown, +): { token: string; agentName: string; generation: number } | null { + if (typeof entry !== "string") return null; + const first = entry.indexOf("@"); + const last = entry.lastIndexOf("@"); + // Token and agent name must both be non-empty; token shape is Dispatch's. + if (first <= 0 || last <= first) return null; + const token = entry.slice(0, first); + if (!/^hnd_[0-9a-f]{8,}$/.test(token)) return null; + const generation = Number(entry.slice(last + 1)); + if (!Number.isInteger(generation) || generation < 1) return null; + return { token, agentName: entry.slice(first + 1, last), generation }; +} + +/** + * The receipt in `entries` matching the exact (agent, generation, token) + * triple, or null. All three must match: the agent binds the source owner, + * the token binds the exact hand-out issuance, the generation is defense in + * depth against a cleared-then-reissued entry. + */ +export function findHandoutReceipt( + entries: readonly unknown[], + owner: { agentName: string; generation: number; token: string }, +): string | null { + for (const entry of entries) { + const parsed = parseHandoutReceipt(entry); + if ( + parsed && + parsed.agentName === owner.agentName && + parsed.generation === owner.generation && + parsed.token === owner.token + ) { + return entry as string; + } + } + return null; +} + /** * Whether this item was already handed to `agentName` at its CURRENT * generation (#1133). Entries for older generations and malformed entries @@ -2461,6 +2553,7 @@ export async function requeuePrFixItem(client: PrFixQueueClient, rawInput: Reque postDispatchEvidenceKeys: [], agentHandouts: [], handoutAcks: [], + handoutReceipts: [], handoutReclaims: 0, }, }); @@ -2517,7 +2610,8 @@ export type AgentReportOutcome = | "waiting-external" // #1143: an issue close, not a PR-fix settlement; never touches the queue. | "already_done" - | "handed-off"; + | "handed-off" + | "capacity-exhausted"; /** * The (id, generation) attempt token `next-task` issues on a followup-pr @@ -2526,6 +2620,13 @@ export type AgentReportOutcome = export interface PrFixReportAttempt { itemId: string; generation: number; + /** + * #1256: the per-hand-out receipt token next-task issued with this + * hand-out. REQUIRED by `capacity-exhausted` settlement (exact source- + * attempt fencing); ignored by every other outcome, so legacy reports and + * token-less hand-outs settle exactly as before. + */ + handoutToken?: string | null; } export interface ResolvePrFixFromAgentReportInput { @@ -2542,6 +2643,13 @@ export interface ResolvePrFixFromAgentReportInput { // give-up) marks PARKED — a bare give-up is NOT a human decision. blockReason?: string | null; decision?: PrFixDecisionContext | null; + // #1256: opaque worker-declared capacity identifier, required (with an + // attempt token) by the `capacity-exhausted` outcome. + capacity?: string | null; + // #1256: reporting agent — recorded on the capacity-handoff history row and, + // for `capacity-exhausted`, part of the exact-owner CAS (the presented + // handoutToken must belong to THIS agent's live receipt). + agentName?: string | null; client?: PrFixQueueClient; // #1074: required for queue settlement. When present, the report settles // exactly the item + generation the token identifies; when absent (legacy @@ -2713,6 +2821,145 @@ export async function resolvePrFixFromAgentReport( const expectedGeneration = attempt?.generation ?? undefined; + // #1256: explicit capacity exhaustion on a handed PR-fix attempt. + // + // Semantics (deliberately NOT a WAITING_EXTERNAL park): the item STAYS + // QUEUED at the same generation with the same fixAttempts count, branch, and + // PR identity — the attempt token is consumed (hand-out stamps cleared so + // any compatible worker can take it) but no repair budget is spent, and no + // global wait hides the item from workers whose capacity is unrelated. The + // suppression is scoped to the exhausted capacity via a CapacityHandoff + // cooldown row that next-task applies when the requester declares a + // capacity (workers declaring none skip conservatively until expiry). + // + // Fencing (#1256 review): the release is authorized by the EXACT source + // attempt — the reporting agent plus the per-hand-out receipt token + // next-task issued with the hand-out (`prFixItem.handoutToken`). An + // unrelated agent that merely knows (id, generation), and the same agent + // presenting a token from a PREVIOUS hand-out of the same generation + // (release keeps the generation), are both refused with zero mutation. The + // write pins the full hand-out state (receipts/hand-outs/acks) and removes + // ONLY the reporting agent's receipt, hand-out entry, and own ack; other + // agents' live hand-outs and acknowledgements at the same generation are + // preserved, and the dispatch stamps survive while any other hand-out is + // still live. A generation race (fresh attempt re-issued meanwhile) or a + // human hold (BLOCKED/PARKED/NEEDS_HUMAN — the non-QUEUED gate above) + // lands as count=0 → skipped with zero mutation. + if (input.outcome === "capacity-exhausted") { + const capacity = input.capacity?.trim(); + const handoutToken = attempt?.handoutToken?.trim(); + const reportingAgent = input.agentName?.trim(); + if (expectedGeneration === undefined || !capacity) { + return { matched: true, action: "skipped", itemId: existing.id ?? null, reason: "capacity-exhausted requires a capacity identifier and prFixItem attempt token" }; + } + if (!handoutToken || !reportingAgent) { + return { matched: true, action: "skipped", itemId: existing.id ?? null, reason: "capacity-exhausted requires the issued prFixItem handoutToken receipt (exact source-attempt fencing)" }; + } + // Idempotent replay: an active cooldown recorded for THIS capacity at + // THIS generation means the report was already processed. The check is + // keyed on both so a different capacity exhausting the same live + // generation (possible: hand-outs are cleared, another worker can be + // dispatched at the same generation) still records its own scoped + // cooldown, and an older generation's window never absorbs a fresh one. + const activeHandoffs = client.capacityHandoff?.findMany + ? await client.capacityHandoff.findMany({ + where: { kind: "pr_fix", repoFullName: repo, prNumber: pr, cooldownUntil: { gt: new Date() } }, + }) + : []; + if ( + activeHandoffs.some( + (h: any) => + h.prNumber === pr && + String(h.repoFullName).toLowerCase() === repo && + h.generation === expectedGeneration && + typeof h.capacity === "string" && + capacityEquals(h.capacity, capacity), + ) + ) { + return { matched: true, action: "deferred", itemId: existing.id ?? null, reason: `capacity handoff already recorded for capacity ${capacity} at generation ${expectedGeneration}`, attemptGeneration: expectedGeneration }; + } + const cooldownUntil = new Date(Date.now() + capacityCooldownMs()); + const queueClient = client as PrFixQueueClient; + const result = await queueClient.$transaction(async (tx) => { + // Read the live hand-out set and pin it below. The CAS still pins the + // ISSUED hand-out stamp (dispatchedGeneration = the generation + // next-task stamped): exactly one concurrent report can consume the + // stamps, so two racing duplicates cannot both mint a cooldown. + const live = await tx.prFixQueueItem.findUnique({ where: { id: existing.id } }); + if ( + !live || + live.status !== "QUEUED" || + live.generation !== expectedGeneration || + live.dispatchedGeneration !== expectedGeneration + ) { + return "stale" as const; + } + const receipts: string[] = Array.isArray(live.handoutReceipts) ? live.handoutReceipts : []; + const handouts: string[] = Array.isArray(live.agentHandouts) ? live.agentHandouts : []; + const acks: string[] = Array.isArray(live.handoutAcks) ? live.handoutAcks : []; + // Exact source-owner authorization: agent + generation + issued token + // must all match a live receipt. An unknown or rotated token (the + // hand-out was already released, reclaimed, or re-issued to this same + // agent at this same generation) has no authority over the current one. + const owned = findHandoutReceipt(receipts, { + agentName: reportingAgent, + generation: expectedGeneration, + token: handoutToken, + }); + if (!owned) return "unauthorized" as const; + const remainingReceipts = receipts.filter((r) => r !== owned); + const ownEntry = agentHandoutToken(reportingAgent, expectedGeneration); + const remainingHandouts = handouts.filter((h) => h !== ownEntry); + const remainingAcks = acks.filter((a) => a !== ownEntry); + // The dispatch stamps survive while ANY other agent's hand-out at this + // generation is still live (#1119: clearing them would reclassify + // mid-run evidence as pre-dispatch for the surviving workers). Only the + // report that consumes the last live hand-out resets the stamps and the + // post-dispatch evidence list, making the generation freshly + // dispatchable. + const lastHandout = remainingReceipts.length === 0; + const { count } = await tx.prFixQueueItem.updateMany({ + where: { + id: live.id, + status: "QUEUED", + generation: expectedGeneration, + dispatchedGeneration: expectedGeneration, + // Receipt/hand-out/ack pins: a concurrent hand-out, ack, or reclaim + // landing in the read→write gap makes this a no-op. + handoutReceipts: { equals: receipts }, + agentHandouts: { equals: handouts }, + handoutAcks: { equals: acks }, + }, + data: { + handoutReceipts: remainingReceipts, + agentHandouts: remainingHandouts, + handoutAcks: remainingAcks, + ...(lastHandout + ? { postDispatchEvidenceKeys: [], dispatchedGeneration: null, dispatchedAt: null } + : {}), + }, + }); + if (count !== 1) return "stale" as const; + await tx.prFixHistory.create({ data: { itemId: existing.id, action: "capacity_handoff", status: "QUEUED", lane: existing.lane, reason: capacity, note: `Capacity exhausted: ${capacity}; attempt generation ${expectedGeneration} released by ${reportingAgent} without consuming retry budget; item stays QUEUED and discoverable to compatible capacities.${remainingReceipts.length > 0 ? ` ${remainingReceipts.length} other live hand-out(s) at this generation preserved (dispatch stamps retained).` : ""}` } }); + if (tx.capacityHandoff) { + await tx.capacityHandoff.create({ data: { kind: "pr_fix", capacity, repoFullName: repo, prNumber: pr, issueNumber: existing.issue ?? null, agentName: reportingAgent, generation: expectedGeneration, cooldownUntil, note: `PR-fix attempt generation ${expectedGeneration} handed off by capacity report` } }); + } + return true; + }); + return { + matched: true, + action: result === true ? "deferred" : "skipped", + itemId: existing.id ?? null, + reason: + result === true + ? `capacity ${capacity} exhausted; attempt released with a capacity-scoped cooldown until ${cooldownUntil.toISOString()}` + : result === "unauthorized" + ? `capacity handoff refused: no live hand-out receipt for ${reportingAgent} at generation ${expectedGeneration} matches the presented handoutToken (already released, reclaimed, or rotated by a newer hand-out); the queue was not modified` + : "capacity handoff lost a status/generation race", + attemptGeneration: expectedGeneration, + }; + } + if (input.outcome === "waiting-external") { if (expectedGeneration === undefined) { return { matched: true, action: "skipped", itemId: existing.id ?? null, reason: "waiting-external requires a prFixItem attempt token" }; @@ -2809,6 +3056,7 @@ export async function resolvePrFixFromAgentReport( postDispatchEvidenceKeys: [], agentHandouts: [], handoutAcks: [], + handoutReceipts: [], } : { status: "QUEUED", @@ -2824,6 +3072,7 @@ export async function resolvePrFixFromAgentReport( // acknowledgements are dead (#1211). agentHandouts: [], handoutAcks: [], + handoutReceipts: [], }; // The head-snapshot pin guards the fresh-attempt branch's baseline on // EVERY pass; the capped BLOCKED branch carries no baseline, so it diff --git a/src/lib/prisma.ts b/src/lib/prisma.ts index be7f143d..647594fd 100644 --- a/src/lib/prisma.ts +++ b/src/lib/prisma.ts @@ -136,6 +136,7 @@ export function asPrFixQueueClient(client: PrismaClient): PrFixQueueClient { issue: client.issue, prFixQueueItem: client.prFixQueueItem, prFixHistory: client.prFixHistory, + capacityHandoff: client.capacityHandoff, $transaction: (fn: (tx: PrFixQueueClient) => Promise): Promise => client.$transaction(fn as (tx: Prisma.TransactionClient) => Promise), }; From 0322ca346bcd525ede264e79415939dc56dba6d3 Mon Sep 17 00:00:00 2001 From: Jory Irving Date: Wed, 14 Oct 2026 10:20:00 -0600 Subject: [PATCH 2/5] fix(recovery): serialize claim cleanup and settle exact handouts (#1256) --- AGENTS.md | 2 +- docs/capacity-handoff.md | 61 ++++-- .../migration.sql | 13 ++ prisma/schema.prisma | 14 ++ .../[agentName]/queue-admission.test.ts | 4 +- .../api/issues/capacity-handoff/route.test.ts | 32 +-- src/app/api/issues/claim/route.test.ts | 6 + src/app/api/issues/claim/route.ts | 12 ++ src/lib/issue-capacity-handoff.test.ts | 202 ++++++------------ src/lib/issue-capacity-handoff.ts | 198 ++++++++--------- src/lib/issue-mutation-guard.test.ts | 100 +++++++++ src/lib/issue-mutation-guard.ts | 61 ++++++ src/lib/lease.test.ts | 7 + src/lib/lease.ts | 10 + src/lib/pr-fix-queue.test.ts | 47 ++++ src/lib/pr-fix-queue.ts | 45 ++-- 16 files changed, 499 insertions(+), 315 deletions(-) create mode 100644 prisma/migrations/20261015000000_issue_mutation_guard/migration.sql create mode 100644 src/lib/issue-mutation-guard.test.ts create mode 100644 src/lib/issue-mutation-guard.ts diff --git a/AGENTS.md b/AGENTS.md index b6729886..7bf03527 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -323,7 +323,7 @@ Queue-backed `followup-pr` reports must echo the `prFixItem.{id, generation}` to Workers may report `outcome: "capacity-exhausted"` only when a recognized execution capacity is genuinely unavailable, with a non-empty `capacity` identifier (an opaque worker-chosen name, for example a provider/model or runner pool) and the issued `prFixItem: { id, generation, handoutToken }` token. Dispatch fences the settlement to the **exact issued hand-out**: the `handoutToken` is a per-hand-out receipt next-task mints (rotated on every issuance), and the release removes only the reporting agent's receipt/hand-out/ack — other agents' live hand-outs at that generation are preserved, and the hand-out stamps clear only when the last live hand-out is consumed. While the item is still `QUEUED` the attempt token is consumed but the item **stays `QUEUED` at the same generation with the same `fixAttempts` count, branch, and PR identity** — no repair budget is spent and there is no global wait. The suppression is **capacity-scoped**: Dispatch records a bounded cooldown (`DISPATCH_CAPACITY_COOLDOWN_MS`, default 15m, clamped to 1m–6h) for that capacity on that item only. `next-task` requests may declare `?capacity=`; a request whose capacity matches the exhausted one skips the item until the cooldown expires, an unrelated capacity is never suppressed, and a request that declares no capacity conservatively skips cooled items. The capacity is caller-declared trusted *scoping*, never identity: settlement authority comes only from the issued receipt token, the cooldown deliberately does not filter by agent (the same worker with a different, unrelated capacity is legitimately allowed), and lying about a capacity grants nothing beyond re-acquiring work for the capacity one claims to have exhausted. A `capacity_handoff` history row and a `CapacityHandoff` row record the transition. A changed generation, an already-settled status, or a human hold (BLOCKED/PARKED/NEEDS_HUMAN) is never overwritten — the report settles as a no-op. Do not use this outcome for worker-local failures, ordinary errors, or an unrecognized capacity. -For **claimed issue work** the fenced counterpart is `POST /api/issues/capacity-handoff` (bearer-auth; bound workers only for their own agent). The worker presents the `claimToken` that `POST /api/issues/claim` issued on the claim (also surfaced by `active-work` resume context and the MCP `claim_issue` result). Dispatch fences the release to that exact claim (agent + lease row + token, fence verified against the live GitHub labels and re-verified after the lease CAS — a force-claim that lands mid-flight narrows the release to ownership-only, never a status write) and refuses human holds (closed/done, any live `status/blocked`, a cached block carrying a `blockedReason`) and changed ownership (a different or missing `agent/*` label) with 409 and zero writes. On success the `agent/*` label is removed, the resting status follows the shared unclaim policy (in-progress → `status/ready`, immediately discoverable to another compatible worker), the lease is consumed, active AgentWork rows are retired with history, an audit row is written, and the same capacity-scoped cooldown applies to issue pickup. Branch/PR identity on the lease and work history is retained; nothing is re-pushed and no repair budget exists for issue work. See [docs/capacity-handoff.md](./docs/capacity-handoff.md) for the lease/label effects and the GitHub-vs-database consistency boundary. +For **claimed issue work** the fenced counterpart is `POST /api/issues/capacity-handoff` (bearer-auth; bound workers only for their own agent). The worker presents the `claimToken` that `POST /api/issues/claim` issued on the claim (also surfaced by `active-work` resume context and the MCP `claim_issue` result). A persistent per-issue `IssueMutationGuard` serializes cooperating claim, `upsertLease`, and release mutations under a PostgreSQL advisory lock. Dispatch fences release to that exact claim (agent + lease row + token, verified against live GitHub labels); if ownership changed during verification, cleanup removes only this worker's agent label and never changes status. Human holds (closed/done, any live `status/blocked`, a cached block carrying a `blockedReason`) and changed/missing ownership are refused. The lease remains until remote label cleanup succeeds; then it is consumed, active AgentWork rows are retired with history, an audit row is written, and the same capacity-scoped cooldown applies to issue pickup. Failed cleanup retains the lease and guard so an exact-token retry can resume. Guards are persistent and have no automatic timeout: after a process crash, an operator must confirm the process stopped and reconcile live GitHub state before removing one. Branch/PR identity on the lease and work history is retained; nothing is re-pushed and no repair budget exists for issue work. See [docs/capacity-handoff.md](./docs/capacity-handoff.md) for the lease/label effects and the GitHub-vs-database consistency boundary. The report body also accepts an optional `startedAt` — an extended ISO 8601 timestamp with timezone (`Z` or `±HH:MM`), for example `2026-10-03T04:20:58Z` or `2026-10-03T04:20:58.123456+00:00`, marking when the worker began the task. When it parses, is not in the future, and is no more than 24 hours before the report, it is stored as the run's start time so the run records a real duration; missing, malformed, future, or too-old values silently fall back to the report time (never a 400). The raw body value participates in the idempotency payload identity, so an identical-body retry always replays as a duplicate. An accepted `startedAt` is normalized to UTC (`Z`) form when stored and echoed back in the response. The echo reflects re-validation of the current request, so on an idempotent duplicate replay the echoed `startedAt` may be absent even though the original accepted run stored it — for example when an identical-body retry lands more than 24 hours after the reported start (the stored AgentRun is unaffected). diff --git a/docs/capacity-handoff.md b/docs/capacity-handoff.md index edc688e6..01a4b5d2 100644 --- a/docs/capacity-handoff.md +++ b/docs/capacity-handoff.md @@ -47,9 +47,12 @@ bounded (≤200 chars) `capacity` and a valid issued token are hard 400 gates. unique, minted at lease creation). Renewals deliberately keep the token (a heartbeating worker keeps its receipt), but a released or expired lease that is re-claimed is a **new row with a new token** — an old worker's token is - permanently inert against a newer claim. The release itself is a CAS - `deleteMany` on `(lease id, agentName, issueId, expiredAt > now)`: exactly - one racing release wins; losers get 409 with zero writes. + permanently inert against a newer claim. A durable `IssueMutationGuard` + serializes cooperating claim, lease-upsert, and capacity-release mutations + per issue. The release retains the lease while it performs remote cleanup; + only after GitHub cleanup succeeds does it CAS-delete + `(lease id, agentName, issueId, expiredAt > now)`. A failed CAS is an + invariant/bypass race, not the normal inter-writer coordination mechanism. ## Human-hold and ownership safeguards (issue surface) @@ -65,10 +68,9 @@ Checked **before** any write; every refusal is 409/400 with zero mutation: (already released or human-cleared). * GitHub unreachable → 503 fail-closed: no lease delete, no label write. -**Post-CAS re-verification**: a force-claim (which does not delete the -previous agent's lease row) or a human change can land between the pre-CAS -read and the lease delete. After the CAS the live labels are re-read; if -ownership moved **or the re-read itself fails**, the release narrows to +**Post-read ownership re-verification**: a force-claim (which does not delete +the previous agent's lease row) or a human change can land during label +verification. If ownership moved, the release narrows to **ownership-only** — our own `agent/*` label is removed idempotently and the cache mirrors the freshest known set minus that label — and no status label is ever touched, so a newer owner's transition survives. A re-read failure @@ -82,15 +84,19 @@ with no owner. **Issue handoff** (`issue_capacity_handoff` audit action): -1. Lease row CAS-deleted — this is the atomic ownership transition. -2. Claim released through the shared `releaseIssueClaim` policy, based on the - live labels: `agent/` removed; `status/in-progress` → `status/ready` +1. The operation creates a committed per-issue durable mutation guard with + `executing: true`. Cooperating claim and `upsertLease` writers cannot enter + during release. Only an exact agent+token retry can atomically resume a + retained `capacity-release` guard with `executing: false`; unrelated + operations and retries while executing are refused as busy. +2. Claim cleanup uses the shared `releaseIssueClaim` policy, based on live + labels: `agent/` removed; `status/in-progress` → `status/ready` (immediately discoverable to another compatible worker); `status/in-review` - is retained with a status note. -3. Active `AgentWork` rows (CLAIMED/IN_PROGRESS/BLOCKED) for that agent+issue - are marked `RELEASED` with a `capacity_handoff` history entry. -4. A `CapacityHandoff` cooldown row is written (`kind: "issue"`). -5. Audit row with before/after labels, capacity, cooldown deadline. + is retained with a status note. If ownership changed during verification, + cleanup removes only our own agent label and never changes status. +3. Only after remote label cleanup succeeds is the exact lease CAS-deleted. + Active `AgentWork` rows are then retired with history, followed by the + `CapacityHandoff` cooldown (`kind: "issue"`) and audit record. **PR-fix handoff** (`capacity_handoff` PrFixHistory action): @@ -175,11 +181,21 @@ Dispatch's coordination layer — never write it as if GitHub were behind.** * Label writes are targeted (`removeIssueLabel` for the agent label, `transitionIssueStatus` for the status swap), never a full label-set replacement, so concurrent human label changes on GitHub are not reverted. -* The **lease CAS delete is the commit point** for issue ownership. If a - label write fails *after* it, the worker has definitively lost ownership and - the residue (a stale `agent/*` label) is owned by the existing stale-claim - recovery / sync paths — the failure is audited (`success: false`, pointer in - `errorMessage`) and never resurrects the claim or blocks rediscovery. +* A durable `IssueMutationGuard` serializes cooperating claim, + `upsertLease`, and capacity-release writers for an issue. It is a persistent + singleton row per issue, not a lease: there is no automatic timeout or + stale-guard clearing. The `executing` flag is a durable, nonexpiring + execution fence: callback work runs outside database transactions, and only + the matching owner can reopen a completed-but-incomplete attempt. A crash + leaves `executing: true`; an operator must confirm process termination + before resetting it to false. +* Capacity release retains its lease until GitHub label cleanup succeeds; the + exact lease CAS delete follows remote cleanup. A cleanup failure retains both + lease and guard for an exact-token retry. Failed GitHub reads/removals must + not be represented as a successful cache mirror or lease release. After a + crash, an operator must confirm the prior process stopped and reconcile live + GitHub state before removing the guard. There is no timeout-based automatic + recovery that could overlap an in-flight remote mutation. * `CapacityHandoff` rows are Dispatch-local dispatch shaping. They are not mirrored to GitHub, carry no label, and self-expire; deleting them is always safe. @@ -202,6 +218,9 @@ Dispatch's coordination layer — never write it as if GitHub were behind.** | Expired lease | 409 `claim_expired` (stale recovery owns release) | | Different/missing live `agent/*` label | 409 `ownership_changed` | | Deliberate block / done / closed | 409/400 human hold | -| Lease CAS lost (concurrent release) | 409 `handoff_race_lost`, zero writes | +| Another cooperating issue mutation is active / guard is not the exact resumable release | 409 busy response; no remote mutation | +| Cleanup fails after guard acquisition | Lease and durable guard retained; exact-token retry can resume | +| Lease CAS unexpectedly misses after remote cleanup (bypassing writer) | 409 `handoff_race_lost`; no cooldown is recorded; reconcile GitHub and lease state | +| Guard remains after process crash | Operator confirms prior process stopped and reconciles GitHub before removal; no automatic timeout | | PR-fix generation moved / status settled | report `skipped`, queue untouched | | Repeat PR-fix report | `deferred`, one cooldown window only | diff --git a/prisma/migrations/20261015000000_issue_mutation_guard/migration.sql b/prisma/migrations/20261015000000_issue_mutation_guard/migration.sql new file mode 100644 index 00000000..d7414f8c --- /dev/null +++ b/prisma/migrations/20261015000000_issue_mutation_guard/migration.sql @@ -0,0 +1,13 @@ +CREATE TABLE "IssueMutationGuard" ( + "issueId" TEXT NOT NULL, + "operation" TEXT NOT NULL, + "ownerToken" TEXT NOT NULL, + "agentName" TEXT, + "claimToken" TEXT, + "executing" BOOLEAN NOT NULL DEFAULT true, + "createdAt" TIMESTAMP(3) NOT NULL DEFAULT CURRENT_TIMESTAMP, + CONSTRAINT "IssueMutationGuard_pkey" PRIMARY KEY ("issueId"), + CONSTRAINT "IssueMutationGuard_ownerToken_key" UNIQUE ("ownerToken"), + CONSTRAINT "IssueMutationGuard_issueId_fkey" FOREIGN KEY ("issueId") REFERENCES "Issue"("id") ON DELETE CASCADE ON UPDATE CASCADE +); +CREATE INDEX "IssueMutationGuard_agentName_claimToken_idx" ON "IssueMutationGuard"("agentName", "claimToken"); diff --git a/prisma/schema.prisma b/prisma/schema.prisma index 2caca7dc..7380a530 100644 --- a/prisma/schema.prisma +++ b/prisma/schema.prisma @@ -40,6 +40,7 @@ model Issue { laneHistory IssueLane[] agentWork AgentWork[] leases Lease[] + mutationGuard IssueMutationGuard? groomingRuns GroomingRun[] groomingApplications GroomingApplication[] groomingChildClaims GroomingChildClaim[] @@ -121,6 +122,19 @@ model Issue { @@index([groomingRetryAfter]) } +model IssueMutationGuard { + issueId String @id + issue Issue @relation(fields: [issueId], references: [id], onDelete: Cascade) + operation String + ownerToken String @unique + agentName String? + claimToken String? + executing Boolean @default(true) + createdAt DateTime @default(now()) + + @@index([agentName, claimToken]) +} + enum PrFixLane { NORMAL ESCALATED diff --git a/src/app/api/agents/[agentName]/queue-admission.test.ts b/src/app/api/agents/[agentName]/queue-admission.test.ts index e9d8fedc..eda491a1 100644 --- a/src/app/api/agents/[agentName]/queue-admission.test.ts +++ b/src/app/api/agents/[agentName]/queue-admission.test.ts @@ -378,7 +378,9 @@ describe("admission enforce", () => { mocks.prFixFindMany.mockResolvedValue([PR_FIX]); const body = await getNextTask(); expect(body.type).toBe("followup-pr"); - expect(body.prFixItem).toEqual({ id: "prfix-1", generation: 1 }); + expect(body.prFixItem).toMatchObject({ id: "prfix-1", generation: 1 }); + expect(body.prFixItem.handoutToken).toEqual(expect.any(String)); + expect(body.prFixItem.handoutToken).not.toBe(""); const listed = await getQueue(); expect(listed[0]).toMatchObject({ pr: 77, repo: "org/repo" }); diff --git a/src/app/api/issues/capacity-handoff/route.test.ts b/src/app/api/issues/capacity-handoff/route.test.ts index c773bfb5..e24db157 100644 --- a/src/app/api/issues/capacity-handoff/route.test.ts +++ b/src/app/api/issues/capacity-handoff/route.test.ts @@ -1,3 +1,9 @@ +vi.mock("@/lib/issue-mutation-guard", () => ({ + withIssueMutationGuard: async (_request: unknown, callback: any) => { + const result = await callback(undefined, { resuming: false }); + return { status: result.complete ? "completed" : "retained", value: result.value }; + }, +})); import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { authedRequest, @@ -57,8 +63,7 @@ vi.mock("@/lib/prisma", () => ({ db.lease.id === where.id && db.lease.agentName === where.agentName && db.lease.issueId === where.issueId && - where.expiredAt?.gt != null && - new Date(db.lease.expiredAt).getTime() > new Date(where.expiredAt.gt).getTime() + db.lease.claimToken === where.claimToken ) { db.lease = null; db.leaseDeletes += 1; @@ -332,8 +337,10 @@ describe("POST /api/issues/capacity-handoff (#1256)", () => { expect(db.lease).not.toBeNull(); }); - it("refuses a lost lease race (CAS miss) with zero writes", async () => { - // Another handoff released the lease between the read and the CAS. + it("relies on the issue guard for cooperating writers and retains the lease if a bypassed CAS fails", async () => { + // A cooperating writer cannot enter while this operation owns the durable + // guard. Simulate an out-of-band/bypassing writer winning the lease CAS: + // remote cleanup may already have happened, but no cooldown is recorded. const { prisma } = await import("@/lib/prisma"); const leaseModel = (prisma as any).lease; const original = leaseModel.deleteMany; @@ -342,9 +349,9 @@ describe("POST /api/issues/capacity-handoff (#1256)", () => { const response = await postRequest(); expect(response.status).toBe(409); expect((await response.json()).error).toMatch(/changed concurrently/i); - expect(mocks.removeIssueLabel).not.toHaveBeenCalled(); + expect(mocks.removeIssueLabel).toHaveBeenCalled(); expect(db.handoffs).toHaveLength(0); - expect(db.audit).toHaveLength(0); + expect(db.lease).not.toBeNull(); } finally { leaseModel.deleteMany = original; } @@ -390,7 +397,7 @@ describe("POST /api/issues/capacity-handoff (#1256)", () => { expect(mocks.addIssueLabel).not.toHaveBeenCalledWith("org/repo", 42, "status/ready"); expect(json.labels).toContain("status/in-progress"); expect(json.labels).not.toContain("agent/alpha"); - expect(db.audit[0].notes).toMatch(/ownership changed post-CAS/i); + expect(db.audit[0].notes).toMatch(/ownership changed before cleanup/i); }); it("still succeeds when the cooldown write fails after the release committed", async () => { @@ -438,16 +445,17 @@ describe("POST /api/issues/capacity-handoff (#1256)", () => { expect(db.leaseDeletes).toBe(0); }); - it("audits a partial failure where the lease released but the label release failed", async () => { + it("retains the lease and guard after remote label cleanup fails", async () => { mocks.removeIssueLabel.mockRejectedValueOnce(new Error("GitHub write failed")); const response = await postRequest(); expect(response.status).toBe(500); - // Lease consumed (ownership definitively released); failure audited. - expect(db.lease).toBeNull(); + // Remote cleanup is incomplete: retain ownership state and permit only an + // exact-token retry to resume the durable guard. + expect(db.lease).not.toBeNull(); + expect(db.leaseDeletes).toBe(0); expect(db.audit).toHaveLength(1); expect(db.audit[0]).toMatchObject({ action: "issue_capacity_handoff", success: false }); - expect(db.audit[0].errorMessage).toMatch(/stale-claim recovery/i); - // No cooldown row is minted for a failed handoff. + expect(db.audit[0].errorMessage).toMatch(/lease retained/i); expect(db.handoffs).toHaveLength(0); }); }); diff --git a/src/app/api/issues/claim/route.test.ts b/src/app/api/issues/claim/route.test.ts index 45ee1085..a915be19 100644 --- a/src/app/api/issues/claim/route.test.ts +++ b/src/app/api/issues/claim/route.test.ts @@ -1,3 +1,9 @@ +vi.mock("@/lib/issue-mutation-guard", () => ({ + withIssueMutationGuard: async (_request: unknown, callback: any) => { + const result = await callback(undefined, { resuming: false }); + return { status: result.complete ? "completed" : "retained", value: result.value }; + }, +})); import { describe, expect, it, vi, beforeEach } from "vitest"; import { TEST_AGENT_TOKEN as mockToken, authedRequest } from "@/test/route-helpers"; diff --git a/src/app/api/issues/claim/route.ts b/src/app/api/issues/claim/route.ts index 213f5fce..db09d919 100644 --- a/src/app/api/issues/claim/route.ts +++ b/src/app/api/issues/claim/route.ts @@ -11,6 +11,7 @@ import { upsertLease, findActiveLeasesForIssue, releaseExpiredLeases } from "@/l import { findAndReleaseStaleAgentWorkForIssue } from "@/lib/agent-work"; import { transitionIssueStatus } from "@/lib/issue-status"; import { enforceRateLimit } from "@/lib/rate-limit"; +import { withIssueMutationGuard } from "@/lib/issue-mutation-guard"; const IN_PROGRESS_STATUS = "status/in-progress"; const RATE_LIMIT = { limit: 30, windowMs: 10_000 }; @@ -75,6 +76,9 @@ export async function POST(request: Request) { return errorResponse("Force claim requires a maintainer token", 403); } + const guarded = await withIssueMutationGuard({ issueId: issueId as string, operation: "claim" }, async () => { + let remoteStarted = false; + const value = await (async (): Promise => { // Fetch the issue from the local database to check its state and current labels const issue = await prisma.issue.findUnique({ where: { id: issueId as string }, @@ -181,6 +185,7 @@ export async function POST(request: Request) { // buildNewLabels below drops every conflicting agent label from the // cache, so GitHub must lose all of them too — leaving extras behind // is the same GitHub/cache divergence class #1037 fights. + remoteStarted = true; for (const staleAgentLabel of analysis.existingAgents) { try { await removeIssueLabel(repoFullName, issueNumber, staleAgentLabel); @@ -201,6 +206,7 @@ export async function POST(request: Request) { const updatedLabels = [...labelsWithAgent.filter((l) => !l.startsWith("status/")), IN_PROGRESS_STATUS]; try { + remoteStarted = true; // Add agent label on GitHub await addIssueLabel(repoFullName, issueNumber, agentLabel); @@ -223,6 +229,7 @@ export async function POST(request: Request) { agentName: agentName as string, issueId: issueId as string, checkpoint: "issue_claimed", + mutationGuardHeld: true, }); // Write audit log with conflict analysis details @@ -281,6 +288,11 @@ export async function POST(request: Request) { return errorResponse(errorMessage, 500); } + })(); + return { complete: !remoteStarted || value.status < 500, value }; + }); + if (guarded.status === "busy") return errorResponse("Issue mutation is in progress", 409); + return guarded.value; } catch (error) { console.error("Claim issue failed:", error); return errorResponse("Failed to claim issue", 500); diff --git a/src/lib/issue-capacity-handoff.test.ts b/src/lib/issue-capacity-handoff.test.ts index 3d67152a..ce530c3f 100644 --- a/src/lib/issue-capacity-handoff.test.ts +++ b/src/lib/issue-capacity-handoff.test.ts @@ -1,190 +1,110 @@ import { describe, expect, it, beforeEach, vi } from "vitest"; -// The GitHub-facing surfaces are driven by mocks: the live-label re-reads, -// the targeted label removal, and the shared claim-release policy. Everything -// else (fence evaluation, CAS, cooldown, audit) runs against a fake client. -const { githubMocks, claimGateMocks, claimMocks } = vi.hoisted(() => ({ - githubMocks: { - removeIssueLabel: vi.fn().mockResolvedValue(undefined), - }, - claimGateMocks: { - getLiveIssueLabels: vi.fn(), - }, - claimMocks: { - releaseIssueClaim: vi.fn(), - }, +const { githubMocks, claimGateMocks, claimMocks, guardState } = vi.hoisted(() => ({ + guardState: { resuming: false }, + githubMocks: { removeIssueLabel: vi.fn().mockResolvedValue(undefined) }, + claimGateMocks: { getLiveIssueLabels: vi.fn() }, + claimMocks: { releaseIssueClaim: vi.fn() }, })); vi.mock("./github", () => ({ removeIssueLabel: githubMocks.removeIssueLabel })); vi.mock("./claim-gate", () => ({ getLiveIssueLabels: claimGateMocks.getLiveIssueLabels })); vi.mock("./issue-claim", () => ({ releaseIssueClaim: claimMocks.releaseIssueClaim })); +vi.mock("./issue-mutation-guard", () => ({ + withIssueMutationGuard: async (_request: unknown, callback: (...args: any[]) => Promise<{ complete: boolean; value: unknown }>) => ({ + status: "completed", value: (await callback(undefined, { resuming: guardState.resuming })).value, + }), +})); import { recoverIssueCapacityHandoff, IssueCapacityHandoffClient } from "./issue-capacity-handoff"; -const REPO = "org/repo"; -const NUMBER = 42; -const ISSUE_ID = "issue-42"; -const AGENT = "alpha"; -const TOKEN = "claim_token_abc"; +const REPO = "org/repo", NUMBER = 42, ISSUE_ID = "issue-42", AGENT = "alpha", TOKEN = "claim_token_abc"; function makeClient(overrides: Record = {}) { - const audits: any[] = []; - const mirrors: any[] = []; - const handoffs: any[] = []; + const audits: any[] = [], mirrors: any[] = [], handoffs: any[] = []; const client: any = { - audits, - mirrors, - handoffs, + audits, mirrors, handoffs, issue: { - findUnique: async () => ({ - id: ISSUE_ID, - number: NUMBER, - state: "open", - labels: ["agent/alpha", "status/in-progress"], - blockedReason: null, - repository: { fullName: REPO }, - }), - update: async ({ where, data }: any) => { - mirrors.push({ where, data }); - }, + findUnique: async () => ({ id: ISSUE_ID, number: NUMBER, state: "open", labels: ["agent/alpha", "status/in-progress"], blockedReason: null, repository: { fullName: REPO } }), + update: async ({ where, data }: any) => mirrors.push({ where, data }), }, lease: { - findUnique: async () => ({ - id: "lease-1", - agentName: AGENT, - issueId: ISSUE_ID, - claimToken: TOKEN, - expiredAt: new Date(Date.now() + 600_000), - }), - deleteMany: async () => ({ count: 1 }), - }, - agentWork: { - findMany: async () => [], - update: async () => ({}), + findUnique: async () => ({ id: "lease-1", agentName: AGENT, issueId: ISSUE_ID, claimToken: TOKEN, expiredAt: new Date(Date.now() + 600_000) }), + deleteMany: vi.fn(async () => ({ count: 1 })), }, + agentWork: { findMany: async () => [], update: async () => ({}) }, agentWorkHistory: { create: async () => ({}) }, - auditLog: { - create: async ({ data }: any) => { - audits.push(data); - }, - }, - capacityHandoff: { - create: async ({ data }: any) => { - const row = { id: `handoff-${handoffs.length + 1}`, ...data }; - handoffs.push(row); - return row; - }, - }, + auditLog: { create: async ({ data }: any) => audits.push(data) }, + capacityHandoff: { create: async ({ data }: any) => { const row = { id: `handoff-${handoffs.length + 1}`, ...data }; handoffs.push(row); return row; } }, ...overrides, }; - return client as IssueCapacityHandoffClient & { - audits: any[]; - mirrors: any[]; - handoffs: any[]; - }; + return client as IssueCapacityHandoffClient & { audits: any[]; mirrors: any[]; handoffs: any[] }; } -function call(client: any, extra: Record = {}) { - return recoverIssueCapacityHandoff({ - client, - issueId: ISSUE_ID, - repoFullName: REPO, - issueNumber: NUMBER, - agentName: AGENT, - claimToken: TOKEN, - capacity: "provider/model-a", - actor: AGENT, - ...extra, - }); +function call(client: any) { + return recoverIssueCapacityHandoff({ client, issueId: ISSUE_ID, repoFullName: REPO, issueNumber: NUMBER, agentName: AGENT, claimToken: TOKEN, capacity: "provider/model-a", actor: AGENT }); } -describe("recoverIssueCapacityHandoff (#1256)", () => { +describe("recoverIssueCapacityHandoff", () => { beforeEach(() => { vi.clearAllMocks(); + guardState.resuming = false; githubMocks.removeIssueLabel.mockResolvedValue(undefined); claimGateMocks.getLiveIssueLabels.mockResolvedValue(["agent/alpha", "status/in-progress"]); - claimMocks.releaseIssueClaim.mockResolvedValue({ - labels: ["status/ready"], - status: "status/ready", - statusNote: null, - }); + claimMocks.releaseIssueClaim.mockResolvedValue({ labels: ["status/ready"], status: "status/ready", statusNote: null }); }); - it("happy path: same live owner post-CAS takes the full release policy", async () => { + it("releases through the full policy after both live reads succeed", async () => { const client = makeClient(); - const result = await call(client); - expect(result.ok).toBe(true); + expect((await call(client)).ok).toBe(true); expect(claimMocks.releaseIssueClaim).toHaveBeenCalledTimes(1); - // The full policy owns the label writes; the handoff itself adds none. - expect(githubMocks.removeIssueLabel).not.toHaveBeenCalled(); - expect(client.handoffs[0]).toMatchObject({ kind: "issue", capacity: "provider/model-a" }); - expect(client.audits.at(-1)).toMatchObject({ success: true }); - expect(client.audits.at(-1).notes).not.toMatch(/ownership-only/); + expect(client.lease.deleteMany).toHaveBeenCalledTimes(1); }); - it("post-CAS re-read shows a new owner: ownership-only release, never a status transition (#1256 review)", async () => { - claimGateMocks.getLiveIssueLabels - .mockResolvedValueOnce(["agent/alpha", "status/in-progress"]) - .mockResolvedValueOnce(["agent/omega", "status/in-progress"]); + it("late blocked state uses targeted ownership cleanup", async () => { + claimGateMocks.getLiveIssueLabels.mockResolvedValueOnce(["agent/alpha", "status/in-progress"]).mockResolvedValueOnce(["agent/alpha", "status/blocked"]); const client = makeClient(); const result = await call(client); expect(result.ok).toBe(true); - // The shared release policy (which flips in-progress → ready) is skipped. expect(claimMocks.releaseIssueClaim).not.toHaveBeenCalled(); - // Only our own label is removed; the new owner's status survives. - expect(githubMocks.removeIssueLabel).toHaveBeenCalledTimes(1); expect(githubMocks.removeIssueLabel).toHaveBeenCalledWith(REPO, NUMBER, "agent/alpha"); - if (!result.ok) throw new Error("unreachable"); - expect(result.status).toBe("status/in-progress"); - expect(result.statusNote).toMatch(/claim moved to agent\/omega/); - expect(result.statusNote).toMatch(/no status change/); - // The cache mirrors the LIVE set minus our label. - expect(client.mirrors[0].data.labels).toEqual(["agent/omega", "status/in-progress"]); - expect(client.audits.at(-1).notes).toMatch(/ownership changed post-CAS/); }); - it("post-CAS re-read FAILS: ownership-only cleanup, never a status transition (#1256 review)", async () => { - claimGateMocks.getLiveIssueLabels - .mockResolvedValueOnce(["agent/alpha", "status/in-progress"]) - .mockRejectedValueOnce(new Error("github unavailable")); + it("second read failure returns 503 without writes and retains lease", async () => { + claimGateMocks.getLiveIssueLabels.mockResolvedValueOnce(["agent/alpha", "status/in-progress"]).mockRejectedValueOnce(new Error("offline")); const client = makeClient(); const result = await call(client); - // The lease is already consumed — halting would strand the claim — but - // ownership is UNKNOWN, so the release must never touch status. - expect(result.ok).toBe(true); - expect(claimMocks.releaseIssueClaim).not.toHaveBeenCalled(); - expect(githubMocks.removeIssueLabel).toHaveBeenCalledTimes(1); - expect(githubMocks.removeIssueLabel).toHaveBeenCalledWith(REPO, NUMBER, "agent/alpha"); - if (!result.ok) throw new Error("unreachable"); - // The resting status stays in-progress (mirrored), NOT flipped to ready: - // a stale snapshot must not clobber a possible new owner's transition. - expect(result.status).toBe("status/in-progress"); - expect(result.statusNote).toMatch(/re-read failed/); - expect(result.statusNote).toMatch(/no status change/); - expect(client.mirrors[0].data.labels).toEqual(["status/in-progress"]); - expect(client.audits.at(-1).notes).toMatch(/re-read failed/); - expect(client.audits.at(-1).notes).toMatch(/ownership-only/); - // The handoff itself (lease gone, cooldown recorded) still completes. - expect(client.handoffs[0]).toMatchObject({ kind: "issue", capacity: "provider/model-a" }); + expect(result).toMatchObject({ ok: false, httpStatus: 503, code: "github_unavailable" }); + expect(githubMocks.removeIssueLabel).not.toHaveBeenCalled(); + expect(client.lease.deleteMany).not.toHaveBeenCalled(); + expect(client.mirrors).toHaveLength(0); + expect(client.audits.at(-1)).toMatchObject({ success: false }); + }); + + it("failed targeted removal returns 500 without cache mirroring", async () => { + claimGateMocks.getLiveIssueLabels.mockResolvedValueOnce(["agent/alpha", "status/in-progress"]).mockResolvedValueOnce(["agent/omega", "status/in-progress"]); + githubMocks.removeIssueLabel.mockRejectedValueOnce(new Error("write failed")); + const client = makeClient(); + const result = await call(client); + expect(result).toMatchObject({ ok: false, httpStatus: 500, code: "release_failed" }); + expect(client.lease.deleteMany).not.toHaveBeenCalled(); + expect(client.mirrors).toHaveLength(0); + expect(client.audits.at(-1)).toMatchObject({ success: false }); }); - it("no tokened claim: refuses without pointing the worker at an unscoped unclaim", async () => { - const client = makeClient({ - lease: { - findUnique: async () => ({ id: "lease-1", agentName: AGENT, issueId: ISSUE_ID, claimToken: null, expiredAt: new Date(Date.now() + 600_000) }), - deleteMany: async () => ({ count: 1 }), - }, - }); + it("resumes exact-token cleanup after prior label removal without changing resting status", async () => { + guardState.resuming = true; + claimGateMocks.getLiveIssueLabels.mockResolvedValue(["status/blocked"]); + const client = makeClient(); const result = await call(client); - expect(result.ok).toBe(false); - if (result.ok) return; - expect(result.code).toBe("no_active_claim"); - expect(result.message).toMatch(/re-claim/); - // The legacy unclaim path is exactly the unfenced release this endpoint - // replaces; the refusal must not recommend it. - expect(result.message).not.toMatch(/^use unclaim|use unclaim for a legacy claim/); - expect(result.message).toMatch(/do not force an unclaim/); + expect(result).toMatchObject({ ok: true, status: "status/blocked" }); + expect(claimMocks.releaseIssueClaim).not.toHaveBeenCalled(); + expect(client.lease.deleteMany).toHaveBeenCalledWith({ where: { id: "lease-1", agentName: AGENT, issueId: ISSUE_ID, claimToken: TOKEN } }); + }); + + it("refuses a claim without a token", async () => { + const client = makeClient({ lease: { findUnique: async () => ({ id: "lease-1", claimToken: null, expiredAt: new Date(Date.now() + 600_000) }), deleteMany: vi.fn() } }); + expect(await call(client)).toMatchObject({ ok: false, code: "no_active_claim" }); expect(client.audits).toHaveLength(0); - expect(client.handoffs).toHaveLength(0); }); }); diff --git a/src/lib/issue-capacity-handoff.ts b/src/lib/issue-capacity-handoff.ts index 42aa6d1b..d15442d1 100644 --- a/src/lib/issue-capacity-handoff.ts +++ b/src/lib/issue-capacity-handoff.ts @@ -7,42 +7,17 @@ // a human reassignment) moves the label, and lease expiry alone never clears // the `agent/*` label or guarantees rediscovery. // -// This module implements the fenced counterpart: -// -// * The caller presents the claim token Dispatch issued on the claim (the -// authoritative attempt receipt — see `Lease.claimToken`). The release is -// fenced to the EXACT claim: exact agent, exact lease row, still active, -// token equal. A re-claimed issue has a new lease row with a new token, so -// a stale worker's token is inert. -// * Ownership is verified against the LIVE GitHub label set before any write -// (#1037 rule: the claim decision lives on GitHub, not the cache). A -// different `agent/*` label, a missing own label, or a live `status/done` -// means the claim is no longer this worker's — refuse, mutate nothing. -// * Human holds are refused: closed/done issues and a deliberate block -// (`status/blocked` carrying a `blockedReason`) are human decisions this -// operation must never overwrite. -// * The ownership gate itself is a compare-and-delete on the lease row -// (`id + agentName + issueId + expiredAt > now`): exactly one racing -// handoff can take count=1; losers get 409 and write nothing. -// * On success the claim is released with the shared resting-status policy -// (`releaseIssueClaim`: in-progress → ready, so the issue is immediately -// discoverable to another compatible worker), a capacity-SCOPED cooldown -// is recorded, active AgentWork rows are retired with history, and an -// audit row closes the loop. -// -// Consistency boundary (documented in docs/capacity-handoff.md): the lease -// delete is the atomic ownership transition; GitHub label writes and the -// cache mirror happen after it. If a label write fails post-delete, the lease -// is already released (the worker has definitively lost ownership) and the -// residue — a stale `agent/*` label — is repaired by the existing stale-claim -// recovery/sync paths. The failure is audited; it can never resurrect the -// claim or block the item from rediscovery. +// A durable, nonexpiring issue mutation guard excludes cooperating claim +// and lease writers throughout remote cleanup. Failed cleanup retains the +// lease and guard for exact-token retry; a crashed executor requires operator +// recovery. GitHub itself provides no label CAS against external mutations. import { AGENT_PREFIX, getAgentFromLabels } from "@/types"; import { removeIssueLabel } from "./github"; import { getLiveIssueLabels } from "./claim-gate"; import { releaseIssueClaim } from "./issue-claim"; import { recordCapacityHandoff, type CapacityHandoffClient } from "./capacity-handoff"; +import { withIssueMutationGuard } from "./issue-mutation-guard"; export interface IssueCapacityHandoffClient extends CapacityHandoffClient { issue: { @@ -117,6 +92,26 @@ export type IssueCapacityHandoffResult = */ export async function recoverIssueCapacityHandoff( input: IssueCapacityHandoffInput, +): Promise { + const guarded = await withIssueMutationGuard({ + issueId: input.issueId, + operation: "capacity-release", + agentName: input.agentName, + claimToken: input.claimToken, + }, async (_tx, { resuming }) => { + const value = await recoverIssueCapacityHandoffUnlocked(input, resuming); + return { complete: !(value.ok === false && (value.code === "release_failed" || value.code === "github_unavailable")), value }; + }); + if (guarded.status === "busy") return { + ok: false, httpStatus: 409, code: "handoff_race_lost", + message: "Issue mutation is in progress; retry after recovery completes", + }; + return guarded.value; +} + +async function recoverIssueCapacityHandoffUnlocked( + input: IssueCapacityHandoffInput, + resuming: boolean, ): Promise { const { client, issueId, repoFullName, issueNumber, agentName, claimToken, capacity } = input; const now = input.now ?? new Date(); @@ -181,7 +176,7 @@ export async function recoverIssueCapacityHandoff( message: "claimToken does not match the current claim; a re-issued claim invalidates prior tokens", }; } - if (new Date(lease.expiredAt).getTime() <= now.getTime()) { + if (!resuming && new Date(lease.expiredAt).getTime() <= now.getTime()) { return { ok: false, httpStatus: 409, @@ -204,74 +199,46 @@ export async function recoverIssueCapacityHandoff( }; } const refusal = classifyLiveLabels(liveLabels, agentLabel); - if (refusal) return refusal; - - // ── 4. Atomic ownership gate: CAS delete of the exact lease row ────────── - // Exactly one racing release can take count=1. A lease that expired (or was - // otherwise replaced) between the read and here yields count=0 and a 409 - // with zero writes — the new owner is never disturbed. - const { count } = await client.lease.deleteMany({ - where: { id: lease.id, agentName, issueId, expiredAt: { gt: now } }, - }); - if (count !== 1) { - return { - ok: false, - httpStatus: 409, - code: "handoff_race_lost", - message: "Claim changed concurrently (expired or released); no state was modified", - }; - } + // A previous attempt may have removed the label before a later write + // failed. The durable guard still excludes a new claim epoch. + if (refusal && !resuming) return refusal; - // ── 5. Release the claim with the shared resting-status policy ─────────── - // Re-verify live ownership AFTER the commit point. A force-claim (which - // does not delete the previous agent's lease row) or a human label change - // can land between the pre-CAS read and here; acting on the stale snapshot - // would strip a newer owner's status transition. On a changed/missing - // owner — OR WHEN THE RE-READ ITSELF FAILS — we narrow to an - // ownership-ONLY release: remove our own agent label (idempotent — a - // force-claim usually removed it already) and mirror the freshest known - // label set minus our label; touch no status label. A re-read failure must - // never take the full-release path: ownership is unknown, so a status - // transition (e.g. in-progress → ready) could clobber a newer owner's or a - // human's status. Ownership-only cleanup is safe under total ignorance — - // it writes exactly one label, our own, which only ever narrows state. - let labels: string[]; - let status: string | null; - let statusNote: string | null; - let ownershipChangedPostCas = false; - let recheckFailedPostCas = false; + // ── 4. Recheck live labels before any remote mutation ──────────────────── let releaseLabels = liveLabels; + let ownershipChangedPostCas = false; try { const recheck = await getLiveIssueLabels(repoFullName, issueNumber); - if (getAgentFromLabels(recheck) !== agentLabel) { - ownershipChangedPostCas = true; - releaseLabels = recheck; - } + const changed = classifyLiveLabels(recheck, agentLabel, true); + if (changed) ownershipChangedPostCas = true; + releaseLabels = recheck; } catch (error) { - // Ownership unverifiable: assume it may have changed. The lease is - // already consumed, so halting would strand the claim with no owner; - // proceed with ownership-ONLY cleanup on the pre-CAS snapshot and never - // a status transition. - recheckFailedPostCas = true; - console.error(`capacity handoff: post-CAS live-label re-read failed for ${repoFullName}#${issueNumber}; releasing ownership-only:`, error); + const message = error instanceof Error ? error.message : "Unknown error"; + await writeFailureAudit(client, input, liveLabels, `Could not verify live GitHub labels before release: ${message}`); + return { ok: false, httpStatus: 503, code: "github_unavailable", message: `Could not verify live GitHub labels before release: ${message}` }; } - if (ownershipChangedPostCas || recheckFailedPostCas) { + // ── 5. Release the claim with the shared resting-status policy ─────────── + // The fresh label set is authoritative. A changed owner or newly applied + // human hold permits only targeted own-label cleanup, never status changes. + let labels: string[]; + let status: string | null; + let statusNote: string | null; + if (ownershipChangedPostCas) { const newOwner = getAgentFromLabels(releaseLabels); try { // Targeted, idempotent: only our own label. Never a status write — // the new owner's transition stands. await removeIssueLabel(repoFullName, issueNumber, agentLabel); } catch (error) { - console.error(`capacity handoff: post-CAS label cleanup failed for ${repoFullName}#${issueNumber}:`, error); + const message = error instanceof Error ? error.message : "Unknown error"; + await writeFailureAudit(client, input, liveLabels, `Could not remove claim label: ${message}`); + return { ok: false, httpStatus: 500, code: "release_failed", message: `Could not remove claim label; lease and release guard retained: ${message}` }; } labels = releaseLabels.filter((l: string) => l !== agentLabel); status = labels.find((l: string) => l.startsWith("status/")) ?? null; - statusNote = recheckFailedPostCas - ? "live label re-read failed after the lease CAS; ownership unverifiable, so only our own agent label was removed — no status change" - : newOwner - ? `claim moved to ${newOwner} after the lease CAS; only our own agent label was removed — no status change` - : "no agent claim present after the lease CAS; mirrored GitHub minus our label — no status change"; + statusNote = newOwner + ? `claim moved to ${newOwner} before cleanup; only our own agent label was removed — no status change` + : "no agent claim present before cleanup; mirrored GitHub minus our label — no status change"; try { await client.issue.update({ where: { id: issueId }, @@ -293,34 +260,21 @@ export async function recoverIssueCapacityHandoff( statusNote = released.statusNote; } catch (error) { const message = error instanceof Error ? error.message : "Unknown error"; - // The lease is already deleted: the worker has definitively lost - // ownership, and any residue (a stale agent/* label on GitHub) is owned by - // stale-claim recovery. Audit the partial state explicitly. - try { - await client.auditLog.create({ - data: { - actor: input.actor, - action: "issue_capacity_handoff", - repoFullName, - issueNumber, - issueId, - beforeLabels: liveLabels, - afterLabels: [], - success: false, - errorMessage: `Lease released but claim release failed: ${message}; stale-claim recovery owns the residual agent label`, - }, - }); - } catch { - // Audit failure must not mask the refusal. - } + await writeFailureAudit(client, input, liveLabels, `Claim label release failed; lease retained: ${message}`); return { ok: false, httpStatus: 500, code: "release_failed", - message: `Claim lease released but label release failed: ${message}`, + message: `Claim label release failed; lease and release guard retained: ${message}`, }; } + // Consume the exact lease only after GitHub cleanup succeeded. + const { count } = await client.lease.deleteMany({ + where: { id: lease.id, agentName, issueId, claimToken }, + }); + if (count !== 1) return { ok: false, httpStatus: 409, code: "handoff_race_lost", message: "Claim changed concurrently; no lease was released" }; + // ── 6. Retire active AgentWork rows with history (audit trail) ─────────── try { const activeWork = await client.agentWork.findMany({ @@ -386,11 +340,9 @@ export async function recoverIssueCapacityHandoff( notes: `capacity=${capacity}; cooldownUntil=${cooldownUntil ? cooldownUntil.toISOString() : "none (cooldown write failed)"}; ` + `lease ${lease.id} released by claim-token fence${ - recheckFailedPostCas - ? "; post-CAS live-label re-read failed: ownership-only release of our label" - : ownershipChangedPostCas - ? "; ownership changed post-CAS: ownership-only release of our label" - : "" + ownershipChangedPostCas + ? "; ownership changed before cleanup: ownership-only release of our label" + : "" }${input.note ? `; ${input.note}` : ""}`, }, }); @@ -413,6 +365,7 @@ type HandoffRefusal = Extract; function classifyLiveLabels( liveLabels: string[], agentLabel: string, + allowMissingOwner = false, ): HandoffRefusal | null { if (liveLabels.includes("status/done")) { return { ok: false, httpStatus: 409, code: "issue_done", message: "Issue is done on GitHub; nothing to hand off" }; @@ -434,7 +387,7 @@ function classifyLiveLabels( message: `Issue claim on GitHub belongs to ${liveAgent}; refusing to release another owner's claim`, }; } - if (!liveAgent) { + if (!liveAgent && !allowMissingOwner) { return { ok: false, httpStatus: 409, @@ -444,3 +397,28 @@ function classifyLiveLabels( } return null; } + +async function writeFailureAudit( + client: IssueCapacityHandoffClient, + input: IssueCapacityHandoffInput, + beforeLabels: string[], + errorMessage: string, +): Promise { + try { + await client.auditLog.create({ + data: { + actor: input.actor, + action: "issue_capacity_handoff", + repoFullName: input.repoFullName, + issueNumber: input.issueNumber, + issueId: input.issueId, + beforeLabels, + afterLabels: [], + success: false, + errorMessage, + }, + }); + } catch { + // Audit failure must not mask the refusal. + } +} diff --git a/src/lib/issue-mutation-guard.test.ts b/src/lib/issue-mutation-guard.test.ts new file mode 100644 index 00000000..10a823a9 --- /dev/null +++ b/src/lib/issue-mutation-guard.test.ts @@ -0,0 +1,100 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { withIssueMutationGuard } from "./issue-mutation-guard"; + +vi.mock("@/lib/prisma", () => ({ prisma: { + issueMutationGuard: { create: vi.fn(), findUnique: vi.fn(), updateMany: vi.fn(), deleteMany: vi.fn() }, + $transaction: vi.fn(), +} })); + +const request = { issueId: "issue-1", operation: "claim" }; +const row = (overrides = {}) => ({ issueId: "issue-1", ownerToken: "existing", operation: "claim", agentName: null, claimToken: null, executing: false, ...overrides }); + +describe("issue mutation guard", () => { + let prisma: Awaited["prisma"]; + beforeEach(async () => { + vi.clearAllMocks(); + prisma = (await import("@/lib/prisma")).prisma; + }); + + it("creates a committed fence and removes only its owner on completion", async () => { + vi.mocked(prisma.issueMutationGuard.create).mockResolvedValue({} as never); + vi.mocked(prisma.issueMutationGuard.findUnique).mockResolvedValue(row({ executing: true }) as never); + vi.mocked(prisma.issueMutationGuard.deleteMany as any).mockResolvedValue({ count: 1 }); + const callback = vi.fn(async (tx, context) => ({ complete: true, value: [tx, context.resuming] })); + await expect(withIssueMutationGuard(request, callback)).resolves.toMatchObject({ status: "completed", value: [undefined, false] }); + expect(callback).toHaveBeenCalledOnce(); + expect(prisma.issueMutationGuard.deleteMany).toHaveBeenCalledWith({ where: { issueId: "issue-1", ownerToken: "existing", executing: true } }); + expect(prisma.$transaction).not.toHaveBeenCalled(); + }); + + it("allows only the exact nonexecuting release to CAS-resume", async () => { + vi.mocked(prisma.issueMutationGuard.create).mockRejectedValue({ code: "P2002" } as never); + vi.mocked(prisma.issueMutationGuard.findUnique).mockResolvedValue(row({ operation: "capacity-release", agentName: "worker", claimToken: "token" }) as never); + vi.mocked(prisma.issueMutationGuard.updateMany as any).mockResolvedValue({ count: 1 }); + const result = await withIssueMutationGuard({ issueId: "issue-1", operation: "capacity-release", agentName: "worker", claimToken: "token" }, async (_, context) => ({ complete: false, value: context.resuming })); + expect(result).toMatchObject({ status: "retained", value: true }); + expect(prisma.issueMutationGuard.updateMany).toHaveBeenCalledWith({ where: { issueId: "issue-1", ownerToken: "existing", executing: false }, data: { executing: true } }); + expect(prisma.issueMutationGuard.updateMany).toHaveBeenLastCalledWith({ where: { issueId: "issue-1", ownerToken: "existing", executing: true }, data: { executing: false } }); + }); + + it.each([ + { operation: "capacity-release", agentName: "other", claimToken: "token" }, + { operation: "capacity-release", agentName: "worker", claimToken: "other" }, + { operation: "claim", agentName: "worker", claimToken: "token" }, + ])("rejects mismatched credentials", async (attempt) => { + vi.mocked(prisma.issueMutationGuard.create).mockRejectedValue({ code: "P2002" } as never); + vi.mocked(prisma.issueMutationGuard.findUnique).mockResolvedValue(row({ operation: "capacity-release", agentName: "worker", claimToken: "token" }) as never); + const callback = vi.fn(); + expect(await withIssueMutationGuard({ issueId: "issue-1", ...attempt }, callback)).toMatchObject({ status: "busy" }); + expect(callback).not.toHaveBeenCalled(); + expect(prisma.issueMutationGuard.updateMany).not.toHaveBeenCalled(); + }); + + it("keeps an executing exact retry busy", async () => { + vi.mocked(prisma.issueMutationGuard.create).mockRejectedValue({ code: "P2002" } as never); + vi.mocked(prisma.issueMutationGuard.findUnique).mockResolvedValue(row({ operation: "capacity-release", agentName: "worker", claimToken: "token", executing: true }) as never); + expect(await withIssueMutationGuard({ issueId: "issue-1", operation: "capacity-release", agentName: "worker", claimToken: "token" }, vi.fn())).toMatchObject({ status: "busy" }); + expect(prisma.issueMutationGuard.updateMany).not.toHaveBeenCalled(); + }); + + it("blocks a same-agent reclaim while old cleanup is paused, then admits a new epoch", async () => { + let stored: any = null; + vi.mocked(prisma.issueMutationGuard.create as any).mockImplementation(async ({ data }: any) => { + if (stored) throw { code: "P2002" }; + stored = { ...data }; + return stored; + }); + vi.mocked(prisma.issueMutationGuard.findUnique as any).mockImplementation(async () => stored); + vi.mocked(prisma.issueMutationGuard.deleteMany as any).mockImplementation(async ({ where }: any) => { + if (stored?.ownerToken !== where.ownerToken) return { count: 0 }; + stored = null; + return { count: 1 }; + }); + let resume!: () => void; + let started!: () => void; + const entered = new Promise((resolve) => { started = resolve; }); + const pause = new Promise((resolve) => { resume = resolve; }); + const release = withIssueMutationGuard({ issueId: "issue-1", operation: "capacity-release", agentName: "worker", claimToken: "old-token" }, async () => { + started(); + await pause; + return { complete: true, value: "old cleanup finished" }; + }); + await entered; + const newClaim = vi.fn(async () => ({ complete: true, value: "new-token" })); + expect(await withIssueMutationGuard({ ...request, agentName: "worker" }, newClaim)).toMatchObject({ status: "busy" }); + expect(newClaim).not.toHaveBeenCalled(); + resume(); + await release; + expect(await withIssueMutationGuard({ ...request, agentName: "worker" }, newClaim)).toMatchObject({ status: "completed", value: "new-token" }); + }); + + it("reopens a thrown callback but keeps the owner barrier", async () => { + vi.mocked(prisma.issueMutationGuard.create).mockResolvedValue({} as never); + vi.mocked(prisma.issueMutationGuard.findUnique).mockResolvedValue(row({ executing: true }) as never); + vi.mocked(prisma.issueMutationGuard.updateMany as any).mockResolvedValue({ count: 1 }); + const error = new Error("failure"); + await expect(withIssueMutationGuard(request, async () => { throw error; })).rejects.toBe(error); + expect(prisma.issueMutationGuard.updateMany).toHaveBeenCalledWith({ where: { issueId: "issue-1", ownerToken: "existing", executing: true }, data: { executing: false } }); + expect(prisma.issueMutationGuard.deleteMany).not.toHaveBeenCalled(); + }); +}); diff --git a/src/lib/issue-mutation-guard.ts b/src/lib/issue-mutation-guard.ts new file mode 100644 index 00000000..82737b29 --- /dev/null +++ b/src/lib/issue-mutation-guard.ts @@ -0,0 +1,61 @@ +import { Prisma } from "@prisma/client"; +import { prisma } from "@/lib/prisma"; + +export interface IssueMutationRequest { + issueId: string; + operation: string; + agentName?: string; + claimToken?: string; +} + +export interface IssueMutationContext { resuming: boolean } +export type IssueMutationResult = + | { status: "busy"; ownerToken: string; operation: string } + | { status: "completed"; value: T } + | { status: "retained"; ownerToken: string; value: T }; + +/** A committed guard is a fail-closed executor fence; it is never time-expired. */ +export async function withIssueMutationGuard( + request: IssueMutationRequest, + callback: (tx: Prisma.TransactionClient | undefined, context: IssueMutationContext) => Promise<{ complete: boolean; value: T }>, +): Promise> { + const ownerToken = crypto.randomUUID(); + let created = false; + try { + await prisma.issueMutationGuard.create({ data: { ...request, ownerToken, executing: true } }); + created = true; + } catch (error) { + if (!(error instanceof Prisma.PrismaClientKnownRequestError) && (typeof error !== "object" || error === null || !("code" in error))) throw error; + if ((error as { code?: string }).code !== "P2002") throw error; + } + + let guard = created ? await prisma.issueMutationGuard.findUnique({ where: { issueId: request.issueId } }) : null; + let resuming = false; + if (!created) { + guard = await prisma.issueMutationGuard.findUnique({ where: { issueId: request.issueId } }); + if (!guard) return { status: "busy", ownerToken, operation: request.operation }; + const exactRelease = request.operation === "capacity-release" && guard.operation === request.operation + && request.agentName !== undefined && request.claimToken !== undefined + && guard.agentName === request.agentName && guard.claimToken === request.claimToken; + if (!exactRelease || guard.executing) return { status: "busy", ownerToken: guard.ownerToken, operation: guard.operation }; + const acquired = await prisma.issueMutationGuard.updateMany({ + where: { issueId: request.issueId, ownerToken: guard.ownerToken, executing: false }, data: { executing: true }, + }); + if (!acquired.count) return { status: "busy", ownerToken: guard.ownerToken, operation: guard.operation }; + resuming = true; + } + if (!guard) return { status: "busy", ownerToken, operation: request.operation }; + + try { + const result = await callback(undefined, { resuming }); + if (result.complete) { + await prisma.issueMutationGuard.deleteMany({ where: { issueId: request.issueId, ownerToken: guard.ownerToken, executing: true } }); + return { status: "completed", value: result.value }; + } + await prisma.issueMutationGuard.updateMany({ where: { issueId: request.issueId, ownerToken: guard.ownerToken, executing: true }, data: { executing: false } }); + return { status: "retained", ownerToken: guard.ownerToken, value: result.value }; + } catch (error) { + await prisma.issueMutationGuard.updateMany({ where: { issueId: request.issueId, ownerToken: guard.ownerToken, executing: true }, data: { executing: false } }); + throw error; + } +} diff --git a/src/lib/lease.test.ts b/src/lib/lease.test.ts index 5e141741..536505fe 100644 --- a/src/lib/lease.test.ts +++ b/src/lib/lease.test.ts @@ -34,6 +34,13 @@ vi.mock("@/lib/prisma", () => ({ }, })); +vi.mock("@/lib/issue-mutation-guard", () => ({ + withIssueMutationGuard: async (_request: unknown, callback: any) => { + const result = await callback(undefined, { resuming: false }); + return { status: "completed", value: result.value }; + }, +})); + vi.mock("@/lib/next-action", () => ({ buildResumeContext: vi.fn((input) => ({ ...input, nextAction: "inspect_issue" })), isValidCheckpoint: vi.fn((cp) => ["issue_claimed", "branch_created", "changes_made"].includes(cp)), diff --git a/src/lib/lease.ts b/src/lib/lease.ts index 4ceeb4ed..9558e7ab 100644 --- a/src/lib/lease.ts +++ b/src/lib/lease.ts @@ -6,6 +6,7 @@ // cron workers and harnesses resume interrupted work without overlapping // another agent's claimed work. +import { withIssueMutationGuard } from "./issue-mutation-guard"; import { Prisma } from "@prisma/client"; import { prisma } from "@/lib/prisma"; import { newClaimToken } from "@/lib/capacity-handoff"; @@ -41,7 +42,16 @@ export async function upsertLease(params: { branch?: string; prUrl?: string; ttlMs?: number; + /** Internal: the claim route already holds the issue mutation guard. */ + mutationGuardHeld?: boolean; }): Promise<{ created: boolean; lease: any }> { + if (!params.mutationGuardHeld) { + const result = await withIssueMutationGuard({ issueId: params.issueId, operation: "lease", agentName: params.agentName }, async () => ({ + complete: true, value: await upsertLease({ ...params, mutationGuardHeld: true }), + })); + if (result.status === "busy") throw new Error("Issue mutation pending; lease cannot be renewed or reissued"); + return result.value; + } const now = new Date(); const checkpoint = params.checkpoint ?? "issue_claimed"; const expiredAt = calculateLeaseExpiry(params.ttlMs); diff --git a/src/lib/pr-fix-queue.test.ts b/src/lib/pr-fix-queue.test.ts index 7d16dd89..6c484e06 100644 --- a/src/lib/pr-fix-queue.test.ts +++ b/src/lib/pr-fix-queue.test.ts @@ -95,6 +95,9 @@ function makeClient(): PrFixQueueClient & { (where?.kind === undefined || h.kind === where.kind) && (where?.repoFullName === undefined || String(h.repoFullName).toLowerCase() === String(where.repoFullName).toLowerCase()) && (where?.prNumber === undefined || h.prNumber === where.prNumber) && + (where?.generation === undefined || h.generation === where.generation) && + (where?.agentName === undefined || h.agentName === where.agentName) && + (where?.note?.contains === undefined || h.note?.includes(where.note.contains)) && (where?.cooldownUntil === undefined || (where.cooldownUntil.gt != null && new Date(h.cooldownUntil).getTime() > new Date(where.cooldownUntil.gt).getTime())), ), @@ -4266,6 +4269,50 @@ describe("PR-fix outcome separation (#1218)", () => { expect(client.history.filter((h: any) => h.action === "capacity_handoff")).toHaveLength(1); }); + it("settles two agents at the same capacity and generation independently; replaying A cannot consume a new receipt (#1256 review)", async () => { + await enqueuePrFixItem(client, { repo: "org/repo", pr: 315, lane: "NORMAL", reason: "r", feedback: "f", evidenceKey: "k315" }); + const row = client.items[0]; + seedHandout(row, "worker-a", 1, "hnd_aaaaaaaaaaaaaaaa"); + row.agentHandouts.push("worker-b@1"); + row.handoutReceipts.push("hnd_bbbbbbbbbbbbbbbb@worker-b@1"); + const report = (token: string, agentName = "worker-a") => resolvePrFixFromAgentReport({ + client: client as PrFixQueueClient, repoFullName: "org/repo", pullRequestNumber: 315, + outcome: "capacity-exhausted", capacity: "same-capacity", agentName, + attempt: { itemId: row.id, generation: 1, handoutToken: token }, + }); + expect((await report("hnd_aaaaaaaaaaaaaaaa")).action).toBe("deferred"); + expect(client.items[0].handoutReceipts).toEqual(["hnd_bbbbbbbbbbbbbbbb@worker-b@1"]); + expect((await report("hnd_bbbbbbbbbbbbbbbb", "worker-b")).action).toBe("deferred"); + expect(client.items[0].handoutReceipts).toEqual([]); + expect(client.items[0].agentHandouts).toEqual([]); + expect(client.history.filter((h: any) => h.action === "capacity_handoff")).toHaveLength(2); + expect((await report("hnd_aaaaaaaaaaaaaaaa")).action).toBe("deferred"); + expect(client.handoffs).toHaveLength(2); + + // A new receipt at the same generation is not consumed by A's replay. + seedHandout(client.items[0], "worker-a", 1, "hnd_cccccccccccccccc"); + expect((await report("hnd_aaaaaaaaaaaaaaaa")).action).toBe("deferred"); + expect(client.items[0].handoutReceipts).toEqual(["hnd_cccccccccccccccc@worker-a@1"]); + expect(client.items[0].dispatchedGeneration).toBe(1); + }); + + it("legacy receiptless hand-outs keep their dispatch stamps when a receipt owner settles", async () => { + await enqueuePrFixItem(client, { repo: "org/repo", pr: 316, lane: "NORMAL", reason: "r", feedback: "f", evidenceKey: "k316" }); + const row = client.items[0]; + seedHandout(row, "worker-a", 1, "hnd_aaaaaaaaaaaaaaaa"); + row.agentHandouts.push("worker-legacy@1"); + const result = await resolvePrFixFromAgentReport({ + client: client as PrFixQueueClient, repoFullName: "org/repo", pullRequestNumber: 316, + outcome: "capacity-exhausted", capacity: "model-a", agentName: "worker-a", + attempt: { itemId: row.id, generation: 1, handoutToken: "hnd_aaaaaaaaaaaaaaaa" }, + }); + expect(result.action).toBe("deferred"); + expect(client.items[0].agentHandouts).toEqual(["worker-legacy@1"]); + expect(client.items[0].handoutReceipts).toEqual([]); + expect(client.items[0].dispatchedGeneration).toBe(1); + expect(client.items[0].dispatchedAt).toBeInstanceOf(Date); + }); + it("a different capacity exhausting the same live generation records its own scoped cooldown (#1256)", async () => { await enqueuePrFixItem(client, { repo: "org/repo", pr: 305, lane: "NORMAL", reason: "r", feedback: "f", evidenceKey: "k305" }); const row = client.items[0]; diff --git a/src/lib/pr-fix-queue.ts b/src/lib/pr-fix-queue.ts index 1c9a8de8..5a41043f 100644 --- a/src/lib/pr-fix-queue.ts +++ b/src/lib/pr-fix-queue.ts @@ -2855,29 +2855,6 @@ export async function resolvePrFixFromAgentReport( if (!handoutToken || !reportingAgent) { return { matched: true, action: "skipped", itemId: existing.id ?? null, reason: "capacity-exhausted requires the issued prFixItem handoutToken receipt (exact source-attempt fencing)" }; } - // Idempotent replay: an active cooldown recorded for THIS capacity at - // THIS generation means the report was already processed. The check is - // keyed on both so a different capacity exhausting the same live - // generation (possible: hand-outs are cleared, another worker can be - // dispatched at the same generation) still records its own scoped - // cooldown, and an older generation's window never absorbs a fresh one. - const activeHandoffs = client.capacityHandoff?.findMany - ? await client.capacityHandoff.findMany({ - where: { kind: "pr_fix", repoFullName: repo, prNumber: pr, cooldownUntil: { gt: new Date() } }, - }) - : []; - if ( - activeHandoffs.some( - (h: any) => - h.prNumber === pr && - String(h.repoFullName).toLowerCase() === repo && - h.generation === expectedGeneration && - typeof h.capacity === "string" && - capacityEquals(h.capacity, capacity), - ) - ) { - return { matched: true, action: "deferred", itemId: existing.id ?? null, reason: `capacity handoff already recorded for capacity ${capacity} at generation ${expectedGeneration}`, attemptGeneration: expectedGeneration }; - } const cooldownUntil = new Date(Date.now() + capacityCooldownMs()); const queueClient = client as PrFixQueueClient; const result = await queueClient.$transaction(async (tx) => { @@ -2886,13 +2863,16 @@ export async function resolvePrFixFromAgentReport( // next-task stamped): exactly one concurrent report can consume the // stamps, so two racing duplicates cannot both mint a cooldown. const live = await tx.prFixQueueItem.findUnique({ where: { id: existing.id } }); + if (!live) return "stale" as const; if ( - !live || live.status !== "QUEUED" || live.generation !== expectedGeneration || live.dispatchedGeneration !== expectedGeneration ) { - return "stale" as const; + const prior = await tx.capacityHandoff?.findMany?.({ + where: { kind: "pr_fix", repoFullName: repo, prNumber: pr, generation: expectedGeneration, agentName: reportingAgent, note: { contains: `receipt=${handoutReceipt(handoutToken, reportingAgent, expectedGeneration)};` } }, + }) ?? []; + return prior.length ? "replayed" as const : "stale" as const; } const receipts: string[] = Array.isArray(live.handoutReceipts) ? live.handoutReceipts : []; const handouts: string[] = Array.isArray(live.agentHandouts) ? live.agentHandouts : []; @@ -2906,7 +2886,12 @@ export async function resolvePrFixFromAgentReport( generation: expectedGeneration, token: handoutToken, }); - if (!owned) return "unauthorized" as const; + if (!owned) { + const prior = await tx.capacityHandoff?.findMany?.({ + where: { kind: "pr_fix", repoFullName: repo, prNumber: pr, generation: expectedGeneration, agentName: reportingAgent, note: { contains: `receipt=${handoutReceipt(handoutToken, reportingAgent, expectedGeneration)};` } }, + }) ?? []; + return prior.length ? "replayed" as const : "unauthorized" as const; + } const remainingReceipts = receipts.filter((r) => r !== owned); const ownEntry = agentHandoutToken(reportingAgent, expectedGeneration); const remainingHandouts = handouts.filter((h) => h !== ownEntry); @@ -2917,7 +2902,9 @@ export async function resolvePrFixFromAgentReport( // report that consumes the last live hand-out resets the stamps and the // post-dispatch evidence list, making the generation freshly // dispatchable. - const lastHandout = remainingReceipts.length === 0; + // Legacy hand-outs may predate receipt tokens; handouts remain the + // authoritative liveness set until all agents at this generation settle. + const lastHandout = remainingHandouts.length === 0; const { count } = await tx.prFixQueueItem.updateMany({ where: { id: live.id, @@ -2942,13 +2929,13 @@ export async function resolvePrFixFromAgentReport( if (count !== 1) return "stale" as const; await tx.prFixHistory.create({ data: { itemId: existing.id, action: "capacity_handoff", status: "QUEUED", lane: existing.lane, reason: capacity, note: `Capacity exhausted: ${capacity}; attempt generation ${expectedGeneration} released by ${reportingAgent} without consuming retry budget; item stays QUEUED and discoverable to compatible capacities.${remainingReceipts.length > 0 ? ` ${remainingReceipts.length} other live hand-out(s) at this generation preserved (dispatch stamps retained).` : ""}` } }); if (tx.capacityHandoff) { - await tx.capacityHandoff.create({ data: { kind: "pr_fix", capacity, repoFullName: repo, prNumber: pr, issueNumber: existing.issue ?? null, agentName: reportingAgent, generation: expectedGeneration, cooldownUntil, note: `PR-fix attempt generation ${expectedGeneration} handed off by capacity report` } }); + await tx.capacityHandoff.create({ data: { kind: "pr_fix", capacity, repoFullName: repo, prNumber: pr, issueNumber: existing.issue ?? null, agentName: reportingAgent, generation: expectedGeneration, cooldownUntil, note: `PR-fix attempt generation ${expectedGeneration} handed off by capacity report; receipt=${handoutReceipt(handoutToken, reportingAgent, expectedGeneration)};` } }); } return true; }); return { matched: true, - action: result === true ? "deferred" : "skipped", + action: result === true || result === "replayed" ? "deferred" : "skipped", itemId: existing.id ?? null, reason: result === true From 9e71553f04379fadfb8ffc56db3de2eb4df5b81a Mon Sep 17 00:00:00 2001 From: Jory Irving Date: Wed, 14 Oct 2026 10:38:18 -0600 Subject: [PATCH 3/5] fix(recovery): resume failed claims and refuse late human holds (#1256) --- docs/capacity-handoff.md | 14 +++--- src/app/api/issues/claim/route.test.ts | 68 +++++++++++++++++++++++--- src/app/api/issues/claim/route.ts | 24 ++++++--- src/lib/issue-capacity-handoff.test.ts | 15 +++--- src/lib/issue-capacity-handoff.ts | 5 +- src/lib/issue-mutation-guard.test.ts | 30 ++++++++++++ src/lib/issue-mutation-guard.ts | 10 +++- src/lib/lease.test.ts | 13 +++++ 8 files changed, 149 insertions(+), 30 deletions(-) diff --git a/docs/capacity-handoff.md b/docs/capacity-handoff.md index 01a4b5d2..708737a4 100644 --- a/docs/capacity-handoff.md +++ b/docs/capacity-handoff.md @@ -74,11 +74,9 @@ verification. If ownership moved, the release narrows to **ownership-only** — our own `agent/*` label is removed idempotently and the cache mirrors the freshest known set minus that label — and no status label is ever touched, so a newer owner's transition survives. A re-read failure -must not take the full-release path: ownership is unknown, and a status -transition on a stale snapshot could clobber it. Ownership-only cleanup is -safe under total ignorance (one targeted write, our own label, narrowing -only), and the lease is already consumed, so halting would strand the claim -with no owner. +returns 503 without remote writes, retaining the lease and guard for retry. +A blocked/done hold found on either read is refused without label or lease +mutation; ownership-only cleanup is reserved for genuine changed owners. ## What success does @@ -188,7 +186,11 @@ Dispatch's coordination layer — never write it as if GitHub were behind.** execution fence: callback work runs outside database transactions, and only the matching owner can reopen a completed-but-incomplete attempt. A crash leaves `executing: true`; an operator must confirm process termination - before resetting it to false. + before resetting it to false. Routine local-only lease failures remove + their own guard safely. A failed claim can resume under the same agent's + nonexecuting guard: it re-verifies live ownership and human holds, repairs + the cache plus lifecycle event transaction and lease, and does not steal a + changed claim. Other agents cannot enter until reconciliation finishes. * Capacity release retains its lease until GitHub label cleanup succeeds; the exact lease CAS delete follows remote cleanup. A cleanup failure retains both lease and guard for an exact-token retry. Failed GitHub reads/removals must diff --git a/src/app/api/issues/claim/route.test.ts b/src/app/api/issues/claim/route.test.ts index d96e0822..8c973e5c 100644 --- a/src/app/api/issues/claim/route.test.ts +++ b/src/app/api/issues/claim/route.test.ts @@ -1,6 +1,7 @@ +const { guardState } = vi.hoisted(() => ({ guardState: { resuming: false } })); vi.mock("@/lib/issue-mutation-guard", () => ({ withIssueMutationGuard: async (_request: unknown, callback: any) => { - const result = await callback(undefined, { resuming: false }); + const result = await callback(undefined, { resuming: guardState.resuming }); return { status: result.complete ? "completed" : "retained", value: result.value }; }, })); @@ -73,6 +74,7 @@ vi.mock("@/lib/prisma", () => ({ })); vi.mock("@/lib/github", () => ({ addIssueLabel: mocks.addIssueLabel, removeIssueLabel: mocks.removeIssueLabel })); +vi.mock("@/lib/issue-status", () => ({ transitionIssueStatus: vi.fn(async (_repo: string, _number: number, labels: string[], status: string) => { for (const label of labels.filter((value) => value.startsWith("status/") && value !== status)) await mocks.removeIssueLabel(_repo, _number, label); await mocks.addIssueLabel(_repo, _number, status); }) })); vi.mock("@/lib/claim-gate", () => ({ getLiveIssueLabels: mocks.getLiveIssueLabels })); @@ -83,6 +85,7 @@ import { POST } from "./route"; // shared TEST_AGENT_TOKEN buckets cannot bleed across describes (same pattern // as move/webhook suites). beforeEach(() => { + guardState.resuming = false; resetRateLimits(); }); @@ -156,8 +159,15 @@ describe("POST /api/issues/claim — business logic", () => { mocks.leaseFindMany.mockResolvedValue([]); mocks.leaseDeleteMany.mockResolvedValue({ count: 0 }); mocks.removeIssueLabel.mockResolvedValue(undefined); - mocks.leaseFindMany.mockResolvedValue([]); - mocks.leaseDeleteMany.mockResolvedValue({ count: 0 }); + mocks.transaction.mockImplementation((arg: any) => { + if (typeof arg === "function") return Promise.resolve(arg({ + issue: { update: mocks.updateIssue }, + issueLifecycleEvent: { create: mocks.createLifecycleEvent, upsert: mocks.upsertLifecycleEvent }, + agentWork: { update: mocks.agentWorkUpdate }, + agentWorkHistory: { create: vi.fn().mockResolvedValue({}) }, + })); + return Promise.all(arg.map((op: any) => op)); + }); }); it("adds agent label and moves a fresh claim to in-progress", async () => { @@ -191,12 +201,52 @@ describe("POST /api/issues/claim — business logic", () => { expect((await res.json()).error).toBe("Cannot claim a closed issue"); }); - it("refuses done issues", async () => { - mocks.findUnique.mockResolvedValue({ id: "issue-1", state: "open", number: 42, repository: { fullName: "org/repo" }, labels: ["status/done"] }); - mocks.getLiveIssueLabels.mockResolvedValueOnce(["status/done"]); + it.each(["status/blocked", "status/done"])("refuses %s while resuming without remote or lease writes", async (status) => { + guardState.resuming = true; + mocks.findUnique.mockResolvedValue({ id: "issue-1", state: "open", number: 42, repository: { fullName: "org/repo" }, labels: [status] }); + mocks.getLiveIssueLabels.mockResolvedValueOnce([status]); const res = await POST(makeRequest()); expect(res.status).toBe(400); - expect((await res.json()).error).toBe("Cannot claim a done issue"); + expect(mocks.addIssueLabel).not.toHaveBeenCalled(); + expect(mocks.removeIssueLabel).not.toHaveBeenCalled(); + expect(mocks.leaseCreate).not.toHaveBeenCalled(); + expect(mocks.leaseUpdate).not.toHaveBeenCalled(); + }); + + it("repairs a failed claim transaction on exact-agent retry without repeating remote labels", async () => { + guardState.resuming = false; + mocks.getLiveIssueLabels.mockResolvedValueOnce(["status/ready"]).mockResolvedValue(["agent/test-agent", "status/in-progress"]); + mocks.transaction.mockRejectedValueOnce(new Error("db transaction failed")); + const first = await POST(makeRequest()); + expect(first.status).toBe(500); + guardState.resuming = true; + const retry = await POST(makeRequest()); + expect(retry.status).toBe(200); + expect(mocks.addIssueLabel).toHaveBeenCalledTimes(2); + expect(mocks.removeIssueLabel).toHaveBeenCalledTimes(1); + expect(mocks.createLifecycleEvent).toHaveBeenCalledTimes(1); + expect(mocks.leaseCreate).toHaveBeenCalledTimes(1); + }); + + it("refuses a resumed claim after live ownership changes without writes", async () => { + mocks.getLiveIssueLabels.mockResolvedValueOnce(["status/ready"]); + mocks.transaction.mockRejectedValueOnce(new Error("db transaction failed")); + const first = await POST(makeRequest()); + expect(first.status).toBe(500); + expect(mocks.addIssueLabel).toHaveBeenCalledTimes(2); + const remoteAddCount = mocks.addIssueLabel.mock.calls.length; + const remoteRemoveCount = mocks.removeIssueLabel.mock.calls.length; + const issueUpdateCount = mocks.updateIssue.mock.calls.length; + const leaseCreateCount = mocks.leaseCreate.mock.calls.length; + + guardState.resuming = true; + mocks.getLiveIssueLabels.mockResolvedValueOnce(["agent/other-agent", "status/in-progress"]); + const res = await POST(makeRequest()); + expect(res.status).toBe(409); + expect(mocks.addIssueLabel).toHaveBeenCalledTimes(remoteAddCount); + expect(mocks.removeIssueLabel).toHaveBeenCalledTimes(remoteRemoveCount); + expect(mocks.updateIssue).toHaveBeenCalledTimes(issueUpdateCount); + expect(mocks.leaseCreate).toHaveBeenCalledTimes(leaseCreateCount); }); // The 20-day stall: an issue left at status/ready while still carrying its own @@ -505,6 +555,8 @@ describe("POST /api/issues/claim — #1037 live label gate", () => { describe("POST /api/issues/claim — worker tier (#1111)", () => { beforeEach(() => { vi.clearAllMocks(); + guardState.resuming = false; + mocks.getLiveIssueLabels.mockResolvedValue([]); mocks.findUnique.mockResolvedValue({ id: "issue-1", state: "open", number: 42, repository: { fullName: "org/repo" }, labels: [] as string[] }); mocks.updateIssue.mockResolvedValue(undefined); mocks.createAuditLog.mockResolvedValue({ id: "log-1" }); @@ -660,6 +712,7 @@ describe("POST /api/issues/claim — cross-issue identity invariant (#1129 revie describe("POST /api/issues/claim — lifecycle evidence (#1202)", () => { beforeEach(() => { vi.clearAllMocks(); + guardState.resuming = false; mocks.findUnique.mockResolvedValue({ id: "issue-1", state: "open", number: 42, repository: { fullName: "org/repo" }, labels: [] as string[] }); mocks.updateIssue.mockResolvedValue(undefined); mocks.createAuditLog.mockResolvedValue({ id: "log-1" }); @@ -725,6 +778,7 @@ describe("POST /api/issues/claim — lifecycle evidence (#1202)", () => { expect(first.status).toBe(500); expect(mocks.createLifecycleEvent).toHaveBeenCalledTimes(1); + guardState.resuming = true; const retry = await POST(makeRequest()); expect(retry.status).toBe(200); diff --git a/src/app/api/issues/claim/route.ts b/src/app/api/issues/claim/route.ts index 5ea32867..b8365809 100644 --- a/src/app/api/issues/claim/route.ts +++ b/src/app/api/issues/claim/route.ts @@ -77,7 +77,7 @@ export async function POST(request: Request) { return errorResponse("Force claim requires a maintainer token", 403); } - const guarded = await withIssueMutationGuard({ issueId: issueId as string, operation: "claim" }, async () => { + const guarded = await withIssueMutationGuard({ issueId: issueId as string, operation: "claim", agentName: agentName as string }, async (_tx, { resuming }) => { let remoteStarted = false; const value = await (async (): Promise => { // Fetch the issue from the local database to check its state and current labels @@ -147,12 +147,21 @@ export async function POST(request: Request) { return errorResponse(`Could not verify live GitHub labels: ${errorMessage}`, 503); } - // Refuse done issues (live labels are the authoritative base — #1037). - // Any live status/done makes the issue done, even alongside other status labels. + // Refuse human holds (live labels are authoritative), including on a retry + // after GitHub succeeded but the local mirror failed. + if (liveLabels.includes("status/blocked")) { + return errorResponse("Cannot claim a blocked issue", 400); + } if (liveLabels.includes("status/done")) { return errorResponse("Cannot claim a done issue", 400); } + const alreadyOwned = liveLabels.includes(`${AGENT_PREFIX}${agentName}`); + const hasOtherOwner = liveLabels.some((label) => label.startsWith(AGENT_PREFIX) && label !== `${AGENT_PREFIX}${agentName}`); + if (resuming && (!alreadyOwned || hasOtherOwner)) { + return errorResponse("Cannot resume claim because live issue ownership changed", 409); + } + // Check for active leases from OTHER agents — refuse unless force=true const activeLeases = await findActiveLeasesForIssue(issueId as string); const otherAgentLeases = activeLeases.filter((l) => l.agentName !== agentName); @@ -208,14 +217,17 @@ export async function POST(request: Request) { try { remoteStarted = true; - // Add agent label on GitHub - await addIssueLabel(repoFullName, issueNumber, agentLabel); + // A resumed claim already landed remotely. Repair only the local mirror, + // lifecycle event and lease; do not repeat GitHub label mutations. + if (!resuming) await addIssueLabel(repoFullName, issueNumber, agentLabel); // Remove ALL existing status labels (not just the first) before adding // status/in-progress — keeps GitHub and the Prisma cache from // diverging when an issue carries more than one status label. // Base is the live GitHub label set (authoritative — #1037). - await transitionIssueStatus(repoFullName, issueNumber, liveLabels, IN_PROGRESS_STATUS); + if (!resuming || !liveLabels.includes(IN_PROGRESS_STATUS)) { + await transitionIssueStatus(repoFullName, issueNumber, liveLabels, IN_PROGRESS_STATUS); + } // Update local cache and record the lifecycle event in one transaction: // a claim whose event failed to commit would leave the stream missing an diff --git a/src/lib/issue-capacity-handoff.test.ts b/src/lib/issue-capacity-handoff.test.ts index ce530c3f..7d895d47 100644 --- a/src/lib/issue-capacity-handoff.test.ts +++ b/src/lib/issue-capacity-handoff.test.ts @@ -61,13 +61,14 @@ describe("recoverIssueCapacityHandoff", () => { expect(client.lease.deleteMany).toHaveBeenCalledTimes(1); }); - it("late blocked state uses targeted ownership cleanup", async () => { - claimGateMocks.getLiveIssueLabels.mockResolvedValueOnce(["agent/alpha", "status/in-progress"]).mockResolvedValueOnce(["agent/alpha", "status/blocked"]); + it.each(["status/blocked", "status/done"])("refuses late %s without ownership or lease mutation", async (status) => { + claimGateMocks.getLiveIssueLabels.mockResolvedValueOnce(["agent/alpha", "status/in-progress"]).mockResolvedValueOnce(["agent/alpha", status]); const client = makeClient(); - const result = await call(client); - expect(result.ok).toBe(true); + expect(await call(client)).toMatchObject({ ok: false, code: status === "status/blocked" ? "human_hold" : "issue_done" }); expect(claimMocks.releaseIssueClaim).not.toHaveBeenCalled(); - expect(githubMocks.removeIssueLabel).toHaveBeenCalledWith(REPO, NUMBER, "agent/alpha"); + expect(githubMocks.removeIssueLabel).not.toHaveBeenCalled(); + expect(client.lease.deleteMany).not.toHaveBeenCalled(); + expect(client.mirrors).toHaveLength(0); }); it("second read failure returns 503 without writes and retains lease", async () => { @@ -94,10 +95,10 @@ describe("recoverIssueCapacityHandoff", () => { it("resumes exact-token cleanup after prior label removal without changing resting status", async () => { guardState.resuming = true; - claimGateMocks.getLiveIssueLabels.mockResolvedValue(["status/blocked"]); + claimGateMocks.getLiveIssueLabels.mockResolvedValue(["status/in-progress"]); const client = makeClient(); const result = await call(client); - expect(result).toMatchObject({ ok: true, status: "status/blocked" }); + expect(result).toMatchObject({ ok: true, status: "status/in-progress" }); expect(claimMocks.releaseIssueClaim).not.toHaveBeenCalled(); expect(client.lease.deleteMany).toHaveBeenCalledWith({ where: { id: "lease-1", agentName: AGENT, issueId: ISSUE_ID, claimToken: TOKEN } }); }); diff --git a/src/lib/issue-capacity-handoff.ts b/src/lib/issue-capacity-handoff.ts index d15442d1..cfcbf251 100644 --- a/src/lib/issue-capacity-handoff.ts +++ b/src/lib/issue-capacity-handoff.ts @@ -201,7 +201,7 @@ async function recoverIssueCapacityHandoffUnlocked( const refusal = classifyLiveLabels(liveLabels, agentLabel); // A previous attempt may have removed the label before a later write // failed. The durable guard still excludes a new claim epoch. - if (refusal && !resuming) return refusal; + if (refusal && (!resuming || refusal.code === "human_hold" || refusal.code === "issue_done")) return refusal; // ── 4. Recheck live labels before any remote mutation ──────────────────── let releaseLabels = liveLabels; @@ -209,7 +209,8 @@ async function recoverIssueCapacityHandoffUnlocked( try { const recheck = await getLiveIssueLabels(repoFullName, issueNumber); const changed = classifyLiveLabels(recheck, agentLabel, true); - if (changed) ownershipChangedPostCas = true; + if (changed?.code === "human_hold" || changed?.code === "issue_done") return changed; + if (changed || !recheck.includes(agentLabel)) ownershipChangedPostCas = true; releaseLabels = recheck; } catch (error) { const message = error instanceof Error ? error.message : "Unknown error"; diff --git a/src/lib/issue-mutation-guard.test.ts b/src/lib/issue-mutation-guard.test.ts index 10a823a9..87d3645a 100644 --- a/src/lib/issue-mutation-guard.test.ts +++ b/src/lib/issue-mutation-guard.test.ts @@ -50,6 +50,15 @@ describe("issue mutation guard", () => { expect(prisma.issueMutationGuard.updateMany).not.toHaveBeenCalled(); }); + it("allows only the exact same agent to resume a nonexecuting claim", async () => { + vi.mocked(prisma.issueMutationGuard.create).mockRejectedValue({ code: "P2002" } as never); + vi.mocked(prisma.issueMutationGuard.findUnique).mockResolvedValue(row({ agentName: "worker" }) as never); + vi.mocked(prisma.issueMutationGuard.updateMany as any).mockResolvedValue({ count: 1 }); + const callback = vi.fn(async (_, context) => ({ complete: true, value: context.resuming })); + await expect(withIssueMutationGuard({ ...request, agentName: "worker" }, callback)).resolves.toMatchObject({ status: "completed", value: true }); + expect(callback).toHaveBeenCalledOnce(); + }); + it("keeps an executing exact retry busy", async () => { vi.mocked(prisma.issueMutationGuard.create).mockRejectedValue({ code: "P2002" } as never); vi.mocked(prisma.issueMutationGuard.findUnique).mockResolvedValue(row({ operation: "capacity-release", agentName: "worker", claimToken: "token", executing: true }) as never); @@ -88,6 +97,27 @@ describe("issue mutation guard", () => { expect(await withIssueMutationGuard({ ...request, agentName: "worker" }, newClaim)).toMatchObject({ status: "completed", value: "new-token" }); }); + it("removes a new lease guard after transient failure so a retry can acquire it", async () => { + let stored: any = null; + vi.mocked(prisma.issueMutationGuard.create as any).mockImplementation(async ({ data }: any) => { + if (stored) throw { code: "P2002" }; + stored = { ...data }; + return stored; + }); + vi.mocked(prisma.issueMutationGuard.findUnique as any).mockImplementation(async () => stored); + vi.mocked(prisma.issueMutationGuard.deleteMany as any).mockImplementation(async ({ where }: any) => { + if (stored?.ownerToken !== where.ownerToken) return { count: 0 }; + stored = null; + return { count: 1 }; + }); + const leaseRequest = { issueId: "issue-1", operation: "lease", agentName: "worker" }; + await expect(withIssueMutationGuard(leaseRequest, async () => { throw new Error("transient"); })).rejects.toThrow("transient"); + expect(stored).toBeNull(); + const retry = await withIssueMutationGuard(leaseRequest, async () => ({ complete: true, value: "lease created" })); + expect(retry).toMatchObject({ status: "completed", value: "lease created" }); + expect(prisma.issueMutationGuard.create).toHaveBeenCalledTimes(2); + }); + it("reopens a thrown callback but keeps the owner barrier", async () => { vi.mocked(prisma.issueMutationGuard.create).mockResolvedValue({} as never); vi.mocked(prisma.issueMutationGuard.findUnique).mockResolvedValue(row({ executing: true }) as never); diff --git a/src/lib/issue-mutation-guard.ts b/src/lib/issue-mutation-guard.ts index 82737b29..fde26ccc 100644 --- a/src/lib/issue-mutation-guard.ts +++ b/src/lib/issue-mutation-guard.ts @@ -37,7 +37,9 @@ export async function withIssueMutationGuard( const exactRelease = request.operation === "capacity-release" && guard.operation === request.operation && request.agentName !== undefined && request.claimToken !== undefined && guard.agentName === request.agentName && guard.claimToken === request.claimToken; - if (!exactRelease || guard.executing) return { status: "busy", ownerToken: guard.ownerToken, operation: guard.operation }; + const exactClaim = request.operation === "claim" && guard.operation === "claim" + && request.agentName !== undefined && guard.agentName === request.agentName; + if ((!exactRelease && !exactClaim) || guard.executing) return { status: "busy", ownerToken: guard.ownerToken, operation: guard.operation }; const acquired = await prisma.issueMutationGuard.updateMany({ where: { issueId: request.issueId, ownerToken: guard.ownerToken, executing: false }, data: { executing: true }, }); @@ -55,7 +57,11 @@ export async function withIssueMutationGuard( await prisma.issueMutationGuard.updateMany({ where: { issueId: request.issueId, ownerToken: guard.ownerToken, executing: true }, data: { executing: false } }); return { status: "retained", ownerToken: guard.ownerToken, value: result.value }; } catch (error) { - await prisma.issueMutationGuard.updateMany({ where: { issueId: request.issueId, ownerToken: guard.ownerToken, executing: true }, data: { executing: false } }); + if (request.operation === "lease" && !resuming) { + await prisma.issueMutationGuard.deleteMany({ where: { issueId: request.issueId, ownerToken: guard.ownerToken, executing: true } }); + } else { + await prisma.issueMutationGuard.updateMany({ where: { issueId: request.issueId, ownerToken: guard.ownerToken, executing: true }, data: { executing: false } }); + } throw error; } } diff --git a/src/lib/lease.test.ts b/src/lib/lease.test.ts index 536505fe..821d4fa4 100644 --- a/src/lib/lease.test.ts +++ b/src/lib/lease.test.ts @@ -170,6 +170,19 @@ describe("upsertLease", () => { await expect(upsertLease({ agentName: "saffron", issueId: "i-1" })).rejects.toThrow("connection lost"); expect(mocks.leaseFindUnique).toHaveBeenCalledTimes(1); }); + + it("succeeds when a transient create failure is retried", async () => { + mocks.leaseFindUnique.mockResolvedValue(null); + mocks.leaseCreate + .mockRejectedValueOnce(new Error("connection lost")) + .mockImplementation(async ({ data }: any) => ({ id: "l-1", ...data })); + + await expect(upsertLease({ agentName: "saffron", issueId: "i-1" })).rejects.toThrow("connection lost"); + const retry = await upsertLease({ agentName: "saffron", issueId: "i-1" }); + expect(retry.created).toBe(true); + expect(retry.lease.claimToken).toMatch(/^claim_/); + expect(mocks.leaseCreate).toHaveBeenCalledTimes(2); + }); }); describe("renew path", () => { From e7dec83a0c37c25f1061dd30359cff04cd39f23d Mon Sep 17 00:00:00 2001 From: Jory Irving Date: Wed, 14 Oct 2026 10:44:29 -0600 Subject: [PATCH 4/5] fix(groomer): contain issue guard contention during lease acquisition (#1256) --- src/lib/groomer/run.test.ts | 19 +++++++++++++++++++ src/lib/groomer/run.ts | 27 ++++++++++++++++----------- 2 files changed, 35 insertions(+), 11 deletions(-) diff --git a/src/lib/groomer/run.test.ts b/src/lib/groomer/run.test.ts index a3bb551b..e577d9ea 100644 --- a/src/lib/groomer/run.test.ts +++ b/src/lib/groomer/run.test.ts @@ -1464,6 +1464,25 @@ describe("runHostedGroomer", () => { ); }); + it.each(["active capacity-release", "retained non-executing guard"])("backs off a candidate with %s and can service the next issue", async () => { + mocks.upsertLease.mockRejectedValueOnce(new Error("Issue mutation pending; lease cannot be renewed or reissued")); + expect(await runHostedGroomer()).toBeNull(); + expect(mocks.callGroomerLLM).not.toHaveBeenCalled(); + expect(mocks.releaseLease).not.toHaveBeenCalled(); + expect(mocks.prisma.groomingRun.update).toHaveBeenCalledWith(expect.objectContaining({ + where: { id: "gr-1" }, + data: expect.objectContaining({ status: "failed", stage: "lease_acquisition", retryable: true }), + })); + expect(mocks.prisma.issue.update).toHaveBeenCalledWith(expect.objectContaining({ + where: { id: mockCandidate.id }, + data: { groomingRetryAfter: expect.any(Date) }, + })); + mocks.selectGroomingCandidate.mockResolvedValueOnce({ ...mockCandidate, id: "next-issue", number: 43 }); + await runHostedGroomer(); + expect(mocks.callGroomerLLM).toHaveBeenCalledOnce(); + expect(mocks.releaseLease).toHaveBeenCalledWith("lease-1"); + }); + it("failure after groomingRun creation completes run as failed", async () => { mocks.callGroomerLLM.mockRejectedValue(new Error("LLM timeout")); diff --git a/src/lib/groomer/run.ts b/src/lib/groomer/run.ts index 4a46f2ea..020f54fb 100644 --- a/src/lib/groomer/run.ts +++ b/src/lib/groomer/run.ts @@ -249,16 +249,7 @@ async function executeGroomerRun( staleReasons: candidate.staleReasons, }); - const activeLeases = await deps.findActiveLeases(candidate.id); - const hasOtherLease = activeLeases.some((lease: { agentName?: string }) => lease.agentName !== "hosted-groomer"); - if (hasOtherLease && !options.force) return null; - - const { lease } = await deps.upsertLease({ - agentName: "hosted-groomer", - issueId: candidate.id, - checkpoint: "issue_claimed", - ttlMs: GROOMER_LEASE_TTL_MS, - }); + let lease: { id: string } | undefined; const leaseExpiresAt = Date.now() + GROOMER_LEASE_TTL_MS; // Freshness (#1064): a human comment after this instant is new evidence. @@ -272,6 +263,17 @@ async function executeGroomerRun( let unreadableBackoffWritten = false; try { + stage = "lease_acquisition"; + const activeLeases = await deps.findActiveLeases(candidate.id); + const hasOtherLease = activeLeases.some((active: { agentName?: string }) => active.agentName !== "hosted-groomer"); + if (hasOtherLease && !options.force) throw new Error("Candidate has another active lease"); + ({ lease } = await deps.upsertLease({ + agentName: "hosted-groomer", + issueId: candidate.id, + checkpoint: "issue_claimed", + ttlMs: GROOMER_LEASE_TTL_MS, + })); + stage = "selected"; let comments: Awaited> = []; try { comments = await deps.fetchComments(candidate.repoFullName, candidate.number); @@ -1345,9 +1347,12 @@ async function executeGroomerRun( errorMessage: message, }, }); + // Acquisition failures are candidate-local; their backoff lets the next + // scheduled run select other work rather than repeatedly aborting here. + if (stage === "lease_acquisition") return null; throw error; } finally { - await deps.releaseLease(lease.id); + if (lease) await deps.releaseLease(lease.id); } } From 0babd0fbdb26016db679a0f47e151a953e3d9343 Mon Sep 17 00:00:00 2001 From: Jory Irving Date: Wed, 14 Oct 2026 10:50:55 -0600 Subject: [PATCH 5/5] fix(groomer): defer normal lease contention without failure backoff (#1256) --- src/lib/groomer/run.test.ts | 41 +++++++++++++++++++++++++++++-------- src/lib/groomer/run.ts | 19 ++++++++++++----- src/lib/lease.ts | 10 ++++++++- 3 files changed, 55 insertions(+), 15 deletions(-) diff --git a/src/lib/groomer/run.test.ts b/src/lib/groomer/run.test.ts index e577d9ea..783f312a 100644 --- a/src/lib/groomer/run.test.ts +++ b/src/lib/groomer/run.test.ts @@ -96,7 +96,8 @@ vi.mock("@/lib/prisma", () => ({ prisma: mocks.prisma, })); -vi.mock("@/lib/lease", () => ({ +vi.mock("@/lib/lease", async (importOriginal) => ({ + ...(await importOriginal()), findActiveLeasesForIssue: mocks.findActiveLeasesForIssue, upsertLease: mocks.upsertLease, releaseLease: mocks.releaseLease, @@ -134,6 +135,7 @@ import { runHostedGroomer, UNVERIFIABLE_RETRY_BACKOFF_MINUTES, } from "./run"; +import { IssueMutationBusyError } from "@/lib/lease"; import { computeGroomingIssueFingerprint } from "./freshness"; const mockCandidate: GroomingCandidate = { @@ -1464,23 +1466,33 @@ describe("runHostedGroomer", () => { ); }); - it.each(["active capacity-release", "retained non-executing guard"])("backs off a candidate with %s and can service the next issue", async () => { - mocks.upsertLease.mockRejectedValueOnce(new Error("Issue mutation pending; lease cannot be renewed or reissued")); + it.each(["active capacity-release", "retained non-executing guard"])("defers %s without failure backoff and can service the next issue", async () => { + mocks.upsertLease.mockRejectedValueOnce(new IssueMutationBusyError()); expect(await runHostedGroomer()).toBeNull(); expect(mocks.callGroomerLLM).not.toHaveBeenCalled(); expect(mocks.releaseLease).not.toHaveBeenCalled(); + expect(mocks.prisma.agentRun.create).not.toHaveBeenCalled(); + expect(mocks.prisma.auditLog.create).not.toHaveBeenCalled(); + expect(mocks.prisma.issue.update).not.toHaveBeenCalled(); expect(mocks.prisma.groomingRun.update).toHaveBeenCalledWith(expect.objectContaining({ where: { id: "gr-1" }, - data: expect.objectContaining({ status: "failed", stage: "lease_acquisition", retryable: true }), - })); - expect(mocks.prisma.issue.update).toHaveBeenCalledWith(expect.objectContaining({ - where: { id: mockCandidate.id }, - data: { groomingRetryAfter: expect.any(Date) }, + data: expect.objectContaining({ status: "deferred", stage: "lease_acquisition", completedAt: expect.any(Date) }), })); mocks.selectGroomingCandidate.mockResolvedValueOnce({ ...mockCandidate, id: "next-issue", number: 43 }); await runHostedGroomer(); expect(mocks.callGroomerLLM).toHaveBeenCalledOnce(); - expect(mocks.releaseLease).toHaveBeenCalledWith("lease-1"); + }); + + it("records and propagates unexpected lease-store errors", async () => { + mocks.upsertLease.mockRejectedValueOnce(new Error("database unavailable")); + await expect(runHostedGroomer()).rejects.toThrow("database unavailable"); + expect(mocks.callGroomerLLM).not.toHaveBeenCalled(); + expect(mocks.releaseLease).not.toHaveBeenCalled(); + expect(mocks.prisma.groomingRun.update).toHaveBeenCalledWith(expect.objectContaining({ + data: expect.objectContaining({ status: "failed", stage: "lease_acquisition", errorMessage: "database unavailable" }), + })); + expect(mocks.prisma.agentRun.create).toHaveBeenCalled(); + expect(mocks.prisma.auditLog.create).toHaveBeenCalled(); }); it("failure after groomingRun creation completes run as failed", async () => { @@ -1648,6 +1660,17 @@ describe("runHostedGroomer", () => { expect(result).toBeNull(); expect(mocks.upsertLease).not.toHaveBeenCalled(); expect(mocks.callGroomerLLM).not.toHaveBeenCalled(); + expect(mocks.prisma.agentRun.create).not.toHaveBeenCalled(); + expect(mocks.prisma.auditLog.create).not.toHaveBeenCalled(); + expect(mocks.prisma.issue.update).not.toHaveBeenCalled(); + expect(mocks.prisma.groomingRun.update).toHaveBeenCalledWith(expect.objectContaining({ + data: expect.objectContaining({ status: "deferred", completedAt: expect.any(Date) }), + })); + // No retry timestamp was written: the same candidate can run immediately + // once the other agent releases its lease. + mocks.findActiveLeasesForIssue.mockResolvedValue([]); + await runHostedGroomer(); + expect(mocks.callGroomerLLM).toHaveBeenCalledOnce(); }); it("force option overrides another active lease", async () => { diff --git a/src/lib/groomer/run.ts b/src/lib/groomer/run.ts index 020f54fb..e325d66a 100644 --- a/src/lib/groomer/run.ts +++ b/src/lib/groomer/run.ts @@ -1,7 +1,7 @@ import { prisma } from "@/lib/prisma"; import { addIssueComment, addIssueLabel, closeIssue, createIssue, updateIssueLabels, updateIssueTitleAndBody } from "@/lib/github"; import { fetchCollaboratorPermission } from "@/lib/github-issues"; -import { findActiveLeasesForIssue, releaseLease, upsertLease } from "@/lib/lease"; +import { findActiveLeasesForIssue, IssueMutationBusyError, releaseLease, upsertLease } from "@/lib/lease"; import { acquireGroomerLock, heartbeatGroomerLock, HEARTBEAT_MS, releaseGroomerLock } from "./groomer-lock"; import { selectGroomingCandidate } from "./selector"; import { buildIssueContext, fetchIssueComments } from "./context"; @@ -266,7 +266,12 @@ async function executeGroomerRun( stage = "lease_acquisition"; const activeLeases = await deps.findActiveLeases(candidate.id); const hasOtherLease = activeLeases.some((active: { agentName?: string }) => active.agentName !== "hosted-groomer"); - if (hasOtherLease && !options.force) throw new Error("Candidate has another active lease"); + if (hasOtherLease && !options.force) { + await completeGroomingRunRecord(deps.prisma, groomingRun.id, { + status: "deferred", stage, errorMessage: null, retryable: true, + }); + return null; + } ({ lease } = await deps.upsertLease({ agentName: "hosted-groomer", issueId: candidate.id, @@ -1306,6 +1311,13 @@ async function executeGroomerRun( } catch (error) { const message = error instanceof Error ? error.message : "Unknown groomer error"; + if (stage === "lease_acquisition" && error instanceof IssueMutationBusyError) { + await completeGroomingRunRecord(deps.prisma, groomingRun.id, { + status: "deferred", stage, errorMessage: null, retryable: true, + }); + return null; + } + // Complete GroomingRun as failed try { await completeGroomingRunRecord(deps.prisma, groomingRun.id, { @@ -1347,9 +1359,6 @@ async function executeGroomerRun( errorMessage: message, }, }); - // Acquisition failures are candidate-local; their backoff lets the next - // scheduled run select other work rather than repeatedly aborting here. - if (stage === "lease_acquisition") return null; throw error; } finally { if (lease) await deps.releaseLease(lease.id); diff --git a/src/lib/lease.ts b/src/lib/lease.ts index 9558e7ab..38546919 100644 --- a/src/lib/lease.ts +++ b/src/lib/lease.ts @@ -19,6 +19,14 @@ import { buildResumeContext, isValidCheckpoint } from "./next-action"; export const DEFAULT_LEASE_TTL_MS = 30 * 60 * 1000; // 30 minutes +/** Expected issue-mutation contention, not a lease-store failure. */ +export class IssueMutationBusyError extends Error { + constructor() { + super("Issue mutation pending; lease cannot be renewed or reissued"); + this.name = "IssueMutationBusyError"; + } +} + /** * Calculate the expiry timestamp for a lease with the given TTL. */ @@ -49,7 +57,7 @@ export async function upsertLease(params: { const result = await withIssueMutationGuard({ issueId: params.issueId, operation: "lease", agentName: params.agentName }, async () => ({ complete: true, value: await upsertLease({ ...params, mutationGuardHeld: true }), })); - if (result.status === "busy") throw new Error("Issue mutation pending; lease cannot be renewed or reissued"); + if (result.status === "busy") throw new IssueMutationBusyError(); return result.value; } const now = new Date();