feat(recovery): add fenced capacity-exhaustion handoffs

[joryirving]

Changes

• Add issue capacity-handoff API fenced to the issued claim token and authenticated owner, preserving human holds and changed ownership.
• Issue per-handout PR-fix tokens; release only the exact reporting handout while retaining other workers, PR/branch identity, generation, and repair retry budget.
• Persist bounded capacity-scoped cooldowns, filter next-task pickup by declared capacity, and retain audit/history.
• Add schema migration, worker contract documentation, and ownership/race/auth/cooldown coverage.

Verification

• Typecheck and lint pass.
• 14 affected test suites: 630 tests pass.
• Earlier full-suite run: 4356 passed, two agent-work checkpoint/finish bound-worker failures reproduce on the unchanged base.
• Independent review passed after ownership and shared-handout fixes.
• Prisma generation/validation passed; no live PostgreSQL migration integration test was run.

Operational Notes

• Deploy the included Prisma migration.
• GitHub label writes and database updates are not a distributed transaction; consistency boundaries and conservative recovery behavior are documented in docs/capacity-handoff.md.
• Capacity identifiers are trusted caller declarations, not credentials.

Closes #1256

[joryirving]

Reviewed exact head 6c07dee against #1256, the issue claim/lease writer, post-CAS release path, PR-fix handout receipt/settlement, next-task capacity filtering, and the exact-head GitHub Actions logs. Because this PR is user-authored, this is a COMMENT review with blocking-for-merge findings, not REQUEST_CHANGES. I did not merge it.

[P1] Post-CAS issue cleanup can steal a new claim by the same agent (src/lib/issue-capacity-handoff.ts:213-215,225-294). Deleting the old lease is the local ownership CAS, but it does NOT serialize the later removeIssueLabel/releaseIssueClaim operations with a fresh same-agent claim. Concrete interleaving: capacity handoff A consumes lease L1 via deleteMany; that agent reclaims the same issue, creating new lease L2 with a new claimToken, and re-establishing its identical agent/A label; old handoff then re-reads GitHub, sees the same agent/A and takes the full releaseIssueClaim path, removing the new agent label and resetting status/in-progress → ready. Even the 'ownership-only' branch on failed post-CAS read removes agent/A after a possible new claim. The pre/post forge checks only identify the agent label, not the claim epoch. Need a cross-boundary serialization/pending-release/claim barrier or a fail-closed, resumable remote cleanup protocol so a g release cannot mutate g+1 of the same identity. Tests must pause immediately after old lease CAS, perform same-agent reclaim with a distinct token, then resume old remote cleanup and assert new claim/label/status survive; also remote failure/crash replay. This is the same risk flagged on design PR #1246 but now reaches working code.

[P1] PR-fix handoff deduplication conflates distinct handouts on the same capacity/generation (src/lib/pr-fix-queue.ts:2858-2880). The early return considers a pre-existing active CapacityHandoff with same repo,PR,generation,capacity already settled before checking the reporting handoutToken. But #1256 intentionally allows multiple workers/capacities to hand out the same PR-fix generation, and the same capacity may receive a new receipt for a reissued handout at that same generation. Sequence: token A reports exhausted and is removed, cooldown A@g recorded; token B (already handed or later issued at same generation) reports exhausted with its own live receipt and same capacity; the early return says deferred but leaves B's handout/ack/receipt active, preventing truthful settled ownership, and any downstream worker may wait for an acknowledgement that can never arrive. A replay by token A must be idempotent, but cannot stand in for B's distinct handout. Use exact receipt/token (and agent, generation) as the durable handoff identity for dedupe, and still consume B atomically; tests: two handouts same capacity/generation, A then B; old A replay after B has a fresh active receipt; cross-agent coexistence. A shared capacity cooldown can be reused as a view, not as proof B was released.

[P2, immediate CI failure] Exact-head workflow CI has failing Tests and Coverage jobs for the same assertion: src/app/api/agents/[agentName]/queue-admission.test.ts:381 expects prFixItem={id:'prfix-1',generation:1}, but new next-task correctly adds handoutToken:'hnd_...'. This is an outdated exact-shape test, not evidence that the handout implementation is wrong. Update the assertion to require a nonempty token in addition to the existing identity/generation. I read both job logs; the other CI jobs (lint, typecheck, build, database migrations/integration), image build, PR Smoke and security audit succeeded at this SHA; automated PR Review was still running at check time.

The PR's conservative intent (tokened per-attempt fencing, bounded capacity cooldowns, preserving other worker handouts, no automatic fix-budget burn) is good, but the two concurrent-ownership cases matter more than green static checks. No new issue needed; fix these within #1256 and re-request review on the new head.

[joryirving]

P1: This local lease delete CAS does NOT fence the later GitHub agent/A removal. After old lease L1 is deleted, same agent A can re-claim with L2/new token before the post-CAS re-read; identical agent/A label makes releaseIssueClaim remove L2's label and move status back to ready. Even ownership-only cleanup on read failure removes the same label. Need cross-system release/claim serialization or fail-closed resumable cleanup. Pin CAS→same-agent reclaim→old cleanup race.

[joryirving]

P1: This early duplicate return is keyed only on capacity+generation+repo/PR, before checking the current handoutToken. A different, live receipt B from the same exhausted capacity and same generation must still be consumed/settled after receipt A. As written B returns deferred while its live handout/ack remain; use exact issued receipt identity for idempotency, not just an active cooldown row. Test A→B→replay A.

[joryirving]

Pushed review fixes in 0322ca3: durable nonexpiring issue mutation guard serializes claim/lease creation against release cleanup; failed cleanup retains lease and guard for exact-token recovery, while crashed execution requires explicit operator recovery. Late blocked/done labels preserve status, and failed reads/removals no longer report fictitious cleanup success. PR-fix dedupe now matches the exact issued receipt rather than a shared capacity cooldown; surviving legacy handouts retain dispatch stamps. Updated queue-admission token assertion and added ownership/receipt regression coverage.

Verification at the updated worktree: typecheck, lint, and diff whitespace checks pass. Full Vitest: 4377 passed, 2 failed (the previously identified agent-work checkpoint/finish bound-worker assertions), 16 skipped, 1 todo. No live database migration execution was performed locally. Includes an additional IssueMutationGuard migration. Leaving review to the maintainer as requested.

[joryirving]

Re-review of exact head 0322ca3, following previous review #5519853980. Because this is your PR, I am posting a COMMENT review with findings, not a formal REQUEST_CHANGES, and am not merging it.

Fixed since my last pass:

• The old same-agent claim/reclaim race is addressed for cooperating claim and lease writers by the committed per-issue mutation guard spanning remote cleanup; cleanup errors now retain the lease/guard instead of reporting false release success.
• PR-fix capacity handoff dedupe is now tied to the exact issued handout receipt; A then B at the same capacity/generation can settle separately, and legacy live handouts preserve dispatch stamps. A replay of A can be recognized without consuming B.
• queue-admission.test.ts:381 now asserts a nonempty handout token instead of assuming the old exact object shape. CI, Security Audit, PR Smoke, and image build have passed on 0322ca3. AI PR Review was still in progress at last check.

Remaining [P1]: Routine lease/claim failures create non-resumable permanent issue locks. src/lib/lease.ts:48-53 now places all direct upsertLease calls (including ordinary renewals) inside withIssueMutationGuard(operation:"lease"). On any transient DB exception, src/lib/issue-mutation-guard.ts:57-59 leaves the guard row present with executing=false, but acquisition at :37-40 allows resumption only for operation:"capacity-release". Every later lease/claim sees P2002/busy and cannot recover without manual database/operator intervention. Similarly, claim/route.ts:79,208-293 retains a nonresumable operation:"claim" row whenever a post-GitHub 500 occurs (e.g. lease write/audit failure). These are normal retryable failures, not only crashes. Provide safe retry/cleanup for no-remote-write lease failures and a durable replay/reconciliation protocol for uncertain partially completed claims; tests: lease-update fails once→retry succeeds; remote claim succeeds then DB fails→retry repairs without losing evidence or stealing a new epoch; live capacity-release remains fenced.

Remaining [P1]: Incorporate the already-merged #1247 claim-event transaction. Current main contains recordClaimEvent and an atomic prisma.$transaction wrapping the cache mirror and lifecycle event in src/app/api/issues/claim/route.ts:214-245; mirroredBefore:issue.labels repairs the GitHub-success/DB-failure window. This branch's head still has the old bare prisma.issue.update at :219-223 and no recordClaimEvent import or invocation. The base SHA a8fd35e predates #1247. Merge/rebase current main and preserve #1247's lifecycle-event atomicity inside the new guard, plus add claim-with-guard transition and failure/retry tests. Do not choose the old bare update when resolving the likely overlapping claim-route changes.

Remaining [P2]: A human hold arriving at the second live-read is not refused. src/lib/issue-capacity-handoff.ts:209-232: if first read is agent/alpha,status/in-progress and a human adds status/blocked before the second read, classifyLiveLabels(recheck,...,true) returns a human_hold refusal, but the caller folds it into ownershipChangedPostCas and removes agent/alpha, then consumes its lease. The blocked status stays, but ownership is altered, contrary to #1256's no-mutation human-hold refusal and docs. Return immediately on new human hold/done without label or lease writes; reserve ownership-only cleanup for genuine changed owners. Test the two-read interleaving.

These are three precise follow-ups, not a request for new product scope. I did not merge or change the branch.

Superseded by a newer automated review for this pull request.

[joryirving]

Addressed review 5521166017 in the new head: merged current main (384ca59), retaining #1247 recordClaimEvent and the atomic cache/lifecycle transaction with mirroredBefore inside the mutation guard. Local-only lease exceptions now release their own guard; partially completed claims can retry under the same-agent nonexecuting guard after live ownership/hold verification, repairing the local transaction and lease without taking another epoch. Executing and capacity-release guards remain fenced. Blocked/done holds found on the second recovery read now refuse immediately without label, cache, or lease mutation.

Verification: 123 targeted tests pass; typecheck, lint, and diff whitespace checks pass. Full suite: 4475 passed, the same two previously identified agent-work checkpoint/finish bound-worker assertions fail, 16 skipped, 1 todo. Prisma client regenerated after merging main. No reviewer agents were run; leaving the new head for your review.

[joryirving]

Re-reviewed exact head 9e71553 following review #5521166017. All three requested fixes are now implemented, with matching targeted regression tests:

1. Routine lease failures: withIssueMutationGuard removes the owned guard on a local-only operation:"lease" exception. issue-mutation-guard.test.ts now exercises failed first lease update → fresh acquisition and successful retry. An incomplete operation:"claim" can be resumed only by the same agent after it becomes non-executing, and the claim route rechecks GitHub ownership and human holds, repairs the DB mirror + lifecycle evidence in one prisma.$transaction and creates/renews its tokened lease. Other agents remain fenced.
2. #1247 integration: Current main is integrated. claim/route.ts:232-263 retains recordClaimEvent(tx) in the same transaction as the cache mirror, including mirroredBefore: issue.labels for the GitHub-success/DB-failure retry case. IssueLifecycleEvent and documentation are retained, not silently overwritten.
3. Late human holds: recoverIssueCapacityHandoffUnlocked returns immediately for human_hold / issue_done on either live read, before label/lease/cache mutation. issue-capacity-handoff.test.ts explicitly pins late blocked/done with zero writes.

I also rechecked earlier same-agent handoff guard and exact-receipt PR-fix handout fixes; those remain in place. Exact-head CI, Security Audit, and image build are now green; PR Smoke and AI PR Review were still in progress at my last check. I did not independently run the local test suite and do not treat reported local totals as CI verification.

One remaining [P2] integration issue: The new guarded upsertLease converts a perfectly normal guard contention into a thrown error (src/lib/lease.ts:48-53: if (result.status === "busy") throw new Error(...)). The production hosted groomer invokes deps.upsertLease in src/lib/groomer/run.ts:256-261, before entering its candidate-scoped try/catch/finally at :274. Reproducer: a selected issue is undergoing a capacity-release, same-agent claim or retained non-executing guard; its upsertLease returns busy and throws. The grooming invocation aborts, the already-created GroomingRun at :234-250 remains unfinished, and no per-issue failure/backoff is recorded. If a pending release guard is retained, each scheduler tick may select the same issue and abort without ever servicing another eligible issue. Before this change, routine lease upserts had no such guard-busy exception. Please handle contention before the candidate's failure boundary: for example mark this grooming record a bounded deferred/blocked candidate and skip without invoking the model, or move the acquisition into a safe catch path with truthful history and avoid poisoning the next selection. Test busy-capacity-release and non-executing retained guard while another issue remains eligible; ensure no stuck GroomingRun record and no model call.

Since this is your PR, this is a COMMENT review, not a formal REQUEST_CHANGES, and I have not merged it. No new feature or issue requested, just containment of the new guard's error at its existing caller.

[joryirving]

Addressed review 5521375865: moved active-lease checks and guarded lease acquisition into the candidate-scoped failure boundary. Acquisition failure now completes GroomingRun as failed at lease_acquisition, records bounded candidate backoff and failure history, skips the model, and returns without aborting the grooming invocation. Finally releases a lease only if one was acquired. Regression tests cover contention from active release and retained guard, then a subsequent eligible issue being serviced. Verification: all 176 groomer tests pass; typecheck, lint, and diff whitespace checks pass. No reviewer agents used.

[joryirving]

Re-reviewed exact head e7dec83 against the single-commit diff from 9e71553, the hosted-groomer error/finalization path, the GroomingRun failed-run backoff implementation, selector behavior and the newly added tests.

The prior P2 issue is fixed in the intended guard-contention path. runHostedGroomer now acquires/checks the lease inside its candidate-scoped try/catch/finally, records a terminal GroomingRun rather than leaving a running row, avoids calling the model, avoids trying to release an unacquired lease, and returns null so subsequent scheduler invocations can proceed. The two new test cases exercise the upsertLease-busy path and a subsequent candidate. Previous findings about same-agent fencing, handout receipts, ordinary lease failure recovery, #1247 lifecycle evidence and late human holds remain addressed.

[P2] New regression: a legitimately owned issue is now counted as a failed groom and receives escalating failed-run backoff. The preexisting hasOtherLease && !options.force branch used to return null without failing or changing the candidate. The refactor moves it into the catch boundary as throw new Error("Candidate has another active lease") at src/lib/groomer/run.ts:268-270. The catch at :1306-1352 now writes GroomingRun.status="failed", records a failed AgentRun and audit, and calls backOffFailedRun. This function increases groomingRetryAfter from 30 minutes up to 4 hours based on consecutive failed runs. The selector actually excludes backed-off issues, so a normal agent holding a legitimate issue can leave grooming artificially disabled even after that lease is released, while observability incorrectly reports failed grooming. This differs from a real failed model/lease operation and from transient guard contention. Preserve the old conflict-skip behavior while finishing the already-created GroomingRun truthfully (e.g. skipped/deferred, no failed-run history or escalated backoff). If an active guard is busy, a short bounded contention-specific deferral is fine, but distinguish it from a failed groom and do not feed it into the exponential failure streak. Add a negative test that another agent's active lease produces no failed AgentRun/audit or groomingRetryAfter, and an integration/selector assertion that it becomes eligible promptly after lease release. The new test's hardcoded second selectGroomingCandidate result does not prove that negative case.

[P2, same boundary] The stage==="lease_acquisition" catch also suppresses all acquisition exceptions as a successful null return after best-effort failure bookkeeping, including unexpected lease-store or authorization/invariant errors, rather than only the expected mutation pending or genuinely occupied-lease condition. Classify expected contention explicitly; preserve appropriate error reporting for unexpected infrastructure failures. Otherwise the endpoint can report an idle groomer while its lease database is unhealthy.

CI snapshot: Security Audit and CI completed successfully at e7dec83; PR Smoke, image build and AI PR Review were in progress at last check. I did not run local tests, and am not claiming those pending gates are green.

This is your PR, so I'm leaving COMMENT feedback rather than REQUEST_CHANGES, and am not merging. Once the lease-conflict behavior is separated from genuine failure, the original #1259 recovery issues appear resolved in the reviewed paths.

[joryirving]

Fixed remaining groomer contention semantics: another valid lease and typed IssueMutationBusyError now finish GroomingRun as deferred with completedAt, without failed AgentRun/audit or groomingRetryAfter writes. The same issue can run immediately after lease release. Unexpected lease-store/acquisition errors retain failure recording/backoff and propagate instead of returning null. Added regression coverage for active/retained guard contention, another-agent lease release followed by immediate execution, and infrastructure errors. Targeted groomer + lease suites: 209 tests pass; typecheck, lint, and diff whitespace checks pass. Only these scoped changes were pushed.

[its-saffron]

AI Automated Review

Full PR review.

Analysis engine: qwen3.8-flash-next-chat (openai) — fallback (primary failed) · pr-reviewer-action v3.4.1

Partial coverage: required-check coverage is incomplete — this review did not resolve every required check and must not be read as a complete pass.

Findings (1 blocker, 1 major, 2 minor)

Severity	Location	Finding
🛑 Blocker	src/lib/issue-capacity-handoff.ts	Race condition between lease deletion and GitHub label removal allows a new claim from the same agent to have its label incorrectly removed by the previous claim's cleanup process.
⚠️ Major	src/lib/pr-fix-queue.ts	Verify that the fix for PR-fix deduplication correctly handles multiple live receipts from the same capacity without deferring valid work or leaving orphaned handouts.
Minor	src/lib/pr-fix-queue.ts	P1: This early duplicate return is keyed only on capacity+generation+repo/PR, before checking the current handoutToken. A different, live receipt B from the same exhausted capacity and same generation must still be consumed/settled after receipt A. As written B returns `deferred` while its live ha
Minor	src/lib/issue-capacity-handoff.ts	P1: This local lease delete CAS does NOT fence the later GitHub `agent/A` removal. After old lease L1 is deleted, same agent A can re-claim with L2/new token before the post-CAS re-read; identical `agent/A` label makes `releaseIssueClaim` remove L2's label and move status back to ready. Even own

Recommendation

Request changes. The PR introduces a complex, fenced capacity-handoff mechanism with significant database schema changes and auth implications. While the documentation and test coverage are extensive, there is a critical unresolved race condition in the issue handoff flow regarding GitHub label cleanup vs. lease deletion that could lead to data inconsistency or incorrect state transitions for concurrent claims.

Findings

1. Blocker: Race Condition in Issue Handoff Cleanup (src/lib/issue-capacity-handoff.ts): The review thread PRRT_kwDOSZt6uM6sX6BE highlights a P1 race where a local lease delete CAS does not fence the subsequent GitHub agent/* label removal. If Agent A releases lease L1, and immediately re-claims as L2 before the post-CAS re-read/cleanup of L1 completes, the cleanup logic might remove L2's label because the agent name is identical. The current implementation attempts to mitigate this with an "ownership-only" path on read failure, but the window between lease deletion and label removal remains vulnerable if the re-read succeeds but sees the new claim (L2) as valid ownership for the old release context. This needs explicit serialization or a more robust fencing token check against the live GitHub state after the lease is marked for deletion but before the label is removed, or ensuring the label removal is strictly tied to the specific lease ID's existence in a way that prevents cross-contamination with new leases.

2. Major: Unresolved Review Thread on PR-Fix Deduplication (src/lib/pr-fix-queue.ts): Thread PRRT_kwDOSZt6uM6sX6BO points out that early duplicate returns were keyed only on capacity/generation/repo/PR, potentially allowing a different live receipt B from the same exhausted capacity to be incorrectly deferred while its handout remains active. The author claims this was fixed by using exact issued receipt identity, but the diff shows complex logic around handoutReceipts. Verification is needed to ensure that A -> B -> replay A correctly handles the distinct receipts without leaving orphaned handouts or incorrectly suppressing valid work for capacity B.

3. Minor: Migration Data Loss Risk: The migration adds claimToken to Lease and handoutReceipts to PrFixQueueItem. Existing rows will have null/empty values. The code must handle these legacy states gracefully (e.g., legacy handouts cannot settle capacity-exhausted outcomes). The docs mention this, but strict verification of the fallback paths in next-task and report routes is required to ensure no crashes or silent failures for existing data.

Required Checks Disposition

• review migration for data loss risk: unresolved. The migration adds nullable columns and arrays. While defaults are provided, the interaction with existing production data during the transition period (especially for in-flight PR-fix items) needs careful validation to ensure no data loss or inconsistent state for legacy records.
• test migration on a copy of production schema: unresolved. The PR description explicitly states "No live PostgreSQL migration integration test was run." CI runs migrations on a fresh schema, which does not validate upgrade paths from existing production data structures.
• treat as high priority — verify correctness carefully: unresolved. Given the complexity of the distributed locking and fencing mechanisms, and the identified race conditions in review threads, further verification is needed.
• review auth flow for regression: satisfied. The changes respect the existing bearer token auth model, adding checks for bound workers and refusing unbound tokens for sensitive operations. No obvious regressions in auth logic.
• verify session token handling is correct: satisfied. The new claimToken and handoutToken are opaque strings generated server-side and validated against DB state. They do not replace session cookies but act as API-level fencing tokens, which is consistent with the design.

Linked Issue Fit

The PR addresses PR 1256 by implementing fenced capacity handoffs. It covers the core requirements: atomic settlement/release of exact owners, preserving human holds, and capacity-scoped cooldowns. However, the implementation details around the race conditions (Finding 1) suggest the "atomic" guarantee may be weaker than intended in edge cases involving rapid reclaim.

Unaddressed required checks

The classifier marked these checks as required for this PR's risk profile, but the review does not resolve or disposition them:

• review migration for data loss risk
• test migration on a copy of production schema
• treat as high priority — verify correctness carefully

Unresolved Review Threads

• PRRT_kwDOSZt6uM6sX6BO: open
• PRRT_kwDOSZt6uM6sX6BE: open