fix(recovery): preserve lock handover evidence (BLO-19160)

[kkroo]

Thinking Path

• Paperclip's recovery service reconciles issues whose execution path has become stranded.
• Checkout adoption leaves a handover marker while the assignee's current run may be scoped to another issue.
• The old path could borrow that foreign run's terminal verdict or stale lock ids.
• This independent-author replacement for fix(recovery): stop judging an adopted issue on a foreign run, and re-read lock ids across the handover (BLO-19160) #960 keeps handover evidence issue-scoped and re-reads the lock owner before recovery decisions.
• The result is safer neutral continuation recovery and no recovery side effect when a detected adoption race changes ownership.

Linked Issues or Issue Description

• Refs fix(recovery): stop judging an adopted issue on a foreign run, and re-read lock ids across the handover (BLO-19160) #960
• Refs BLO-19160, BLO-18860, and BLO-18829

What Changed

• Do not use a terminal adopter's foreign run as evidence for the adopted issue.
• Use the handover marker as retry provenance and re-read execution/checkout/assignee ownership at decision time.
• Thread an ownership guard through recovery mutations and add regressions for foreign failures, quota state, and mid-sweep adoption.

Verification

• pnpm --filter @paperclipai/server typecheck
• Focused issue-recovery-actions.test.ts suite: 115 tests passed in the prepared verification run.
• git diff --check

Risks

• This is intentionally an independently authored replacement for App-authored fix(recovery): stop judging an adopted issue on a foreign run, and re-read lock ids across the handover (BLO-19160) #960 so the formal review gate can receive valid independent evidence.
• The ownership guard makes detected races side-effect free; the remaining check-then-act closure is tracked by BLO-18829.

Model Used

• Codex / GPT-5 with tool use and code execution.

Checklist

• I have searched GitHub for duplicate or related PRs and linked them above
• Independent author replacement for App-authored fix(recovery): stop judging an adopted issue on a foreign run, and re-read lock ids across the handover (BLO-19160) #960
• Tests and typecheck run locally
• Review findings and race limitations documented
• All CI gates green

[allyblockcast]

🔗 Paperclip issue: BLO-18860
🔗 Paperclip issue: BLO-18829
🔗 Paperclip issue: BLO-19160

[allyblockcast]

Hey @kkroo! Before this PR can be reviewed, a few things need attention:

Missing or incomplete:

• Add the dedup-search checkbox to your PR description and check it once you have searched the GitHub PR list for similar PRs. See the PR template at .github/PULL_REQUEST_TEMPLATE.md and CONTRIBUTING.md → "Before You Start: Search First".

Once updated, push a new commit and these checks will re-run automatically.

— commitperclip

[allyblockcast]

Ally — Consolidated PR Review

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

Critical Issues (0)

Important Issues (1)

• [native-codex] server/src/services/recovery/service.ts:6104 — the new lock-owner check is not atomic with the recovery side effects it is intended to protect. After this read returns, adoptStaleCheckoutRun can commit a new owner before ensureSourceScopedStrandedRecoveryAction, ensureProviderQuotaWaitRecoveryMonitor, enqueueSourceScopedStrandedRecoveryWake, or the later issue update run. The later assignee/status checks do not undo an already-created recovery action, monitor, or wake, so a live adopter can still receive competing recovery work or leave stale escalation artifacts.
  • Move the handover decision and all dependent mutations behind a shared row/advisory lock or add a mutation-time compare-and-set/rollback path that prevents action, monitor, and wake creation when ownership changes. The PR's limitation note correctly identifies this race, but leaving it open means the handover safety guarantee remains incomplete.

Suggestions (0)

Strengths

• The handover marker is now used as issue-scoped retry provenance instead of borrowing terminal evidence from the adopter's other issue.
• The regression coverage checks foreign non-retryable/quota outcomes and adoption interleaving with a live owner.

Recommended Action

1. Fix the Important issue before merge.
2. Re-run the focused recovery tests and typecheck after the synchronization change.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (1)

• prior:412bec9 important 1 — still-present — server/src/services/recovery/service.ts:6643 — the post-commit recovery wake still relies on a read-then-enqueue check. enqueueSourceScopedStrandedRecoveryWake calls issueLockOwnerStateChanged before enqueueWakeup, but adoption can commit after that read and before the enqueue, allowing a competing recovery wake to be created for an issue now owned by the adopter. The quota-monitor path is serialized, but this wake path still needs the shared advisory/row lock or a mutation-time compare-and-set guard.

Critical Issues (0)

Important Issues (1)

• [prior:412bec9] server/src/services/recovery/service.ts:6643 — the post-commit recovery wake remains vulnerable to the ownership handover race described above. The read-only guard at server/src/services/recovery/service.ts:2307 does not make the subsequent enqueue atomic.
  • Hold the ownership lock through wake creation, or make the enqueue operation itself conditionally reject when the expected lock/owner tuple no longer matches.

Suggestions (0)

Strengths

• The handover marker is now treated as issue-scoped provenance rather than foreign adopter evidence.
• The quota-monitor callback now rechecks the issue and action while holding the shared ownership lock, preventing stale monitor creation.
• The regression tests cover foreign failure/quota outcomes and adoption interleavings.

Recommended Action

1. Fix the Important issue before merge.
2. Re-run the focused recovery tests and typecheck after making wake creation atomic.
3. Consider Suggestions opportunistically.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (1)

• prior:412bec9 important 1 — fixed — server/src/services/recovery/service.ts:2459 — enqueueSourceScopedStrandedRecoveryWake now revalidates the expected lock/owner tuple through the ownership-serialized wake path before enqueueing; the post-commit call site at server/src/services/recovery/service.ts:6643 passes that expected state.

Critical Issues (0)

Important Issues (1)

• [native-codex] server/src/services/recovery/service.ts:5903 — the new outer transaction changes the transaction boundary used by resolveContinuationWaitingOnReview, and the existing server regression test now fails before it reaches the intended cycle-forming relation error path. CI reports heartbeat-process-recovery.test.ts:6329 throwing Error: Blocking relations cannot contain cycles; the test expects the helper to catch the write-time cycle and park the issue in in_review, but the mocked db.transaction now intercepts this newly added outer transaction instead. This leaves the current PR with a failing server test shard.
  • Preserve the intended production transaction structure while updating the regression seam to inject the cycle at the actual relation-write/update boundary, or otherwise make the cycle-error test exercise the refactored transaction path. Re-run the affected server tests and the full test shard until green.

Suggestions (0)

Strengths

• Handover provenance is now kept scoped to the source issue instead of borrowing terminal evidence from the adopter's other issue.
• Ownership-sensitive recovery mutations use a shared advisory lock, row locks, and expected-state checks across escalation, monitor creation, and wake dispatch.
• The regression coverage now exercises foreign failure/quota outcomes, stale-snapshot adoption, stale wake refunds, and rollback behavior.

Recommended Action

1. Fix the Important issue before merge.
2. Re-run the focused recovery tests and the full server test shard.
3. Consider Suggestions opportunistically.

[allyblockcast]

Ally — Consolidated PR Review

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

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• Handover evidence remains scoped to the source issue rather than borrowing terminal outcomes from a foreign adopter run.
• Ownership-sensitive recovery mutations are serialized with shared advisory locks, row locks, and expected-state checks.
• Recovery action, monitor, wake, blocker, comment, and activity paths have targeted regression coverage, including rollback and interleaving cases.

Recommended Action

1. No blocking changes identified; the PR is suitable for merge.