bounty multi-store atomicity + saga (absorbs #227): compensation bugs, handoff reconciler, settle-before-pay

[windoliver]

Background

While reviewing PR #239, codex flagged three correctness bugs in an unlanded bounty.ts compensation refactor that was sitting in the worktree. The refactor adds try/catch compensation for createBountyOperation, claimBountyOperation, and settleBountyOperation, but it assumes each store operation is atomic. It isn't, and the compensation logic can create worse states than it was meant to fix.

These findings were confirmed across three independent codex adversarial-review rounds and are the reason the bounty changes were not landed in #239.

Findings

1. [CRITICAL] settleBounty commits terminal state before payment is captured

`src/core/operations/bounty.ts:319-333`

The reordered flow is:

```typescript
const completed = await deps.bountyStore.completeBounty(...);
const settled = await deps.bountyStore.settleBounty(completed.bountyId);

// Capture payment AFTER state is persisted
if (deps.creditsService && bounty.reservationId && bounty.claimedBy) {
await deps.creditsService.capture(bounty.reservationId, {...});
}
```

The bounty is now `settled` (terminal) before `creditsService.capture()` runs. `capture()` is explicitly allowed to fail for missing / voided / expired reservations and upstream payment errors (see `src/core/in-memory-credits.ts:133-168`). When capture throws, the bounty is already in a terminal state, so the state machine cannot retry settlement, and there is no bounty-payment reconciler in the repository to repair it.

User-visible consequence: a bounty recorded as paid even though the escrow was never captured or transferred to the worker. Silent, unrecoverable.

2. [HIGH] void(reservationId) on any createBounty error can leave persisted bounty without escrow

`src/core/operations/bounty.ts:156-176`

```typescript
try {
const result = await deps.bountyStore.createBounty(bounty);
return ok({...});
} catch (storeErr) {
if (reservationId !== undefined && deps.creditsService !== undefined) {
try { await deps.creditsService.void(reservationId); } catch {}
}
return fromGroveError(storeErr);
}
```

The catch assumes `createBounty()` failed before persistence. That's not true for `NexusBountyStore`: it writes the bounty document first (`nexus-bounty-store.ts:80-102`) and only then writes the status index. If the status-index write throws, the bounty document is already committed, but this handler returns the creator's credits while the open bounty is persisted with the now-empty `reservationId`.

Consequence: an "open" bounty exists in Nexus with no escrow backing it. The creator can re-spend the refunded credits. Later claimants will find the bounty and try to work against it; settlement will hit a voided reservation.

3. [HIGH] release(claim.claimId) on any claimBounty error can strand bounty in claimed with no live claim

`src/core/operations/bounty.ts:258-269`

```typescript
let claimed;
try {
claimed = await deps.bountyStore.claimBounty(input.bountyId, agent, claim.claimId);
} catch (bountyErr) {
try { await deps.claimStore.release(claim.claimId); } catch {}
return fromGroveError(bountyErr);
}
```

Same shape as finding 2. `NexusBountyStore.transitionBounty()` CAS-writes the bounty document first, then writes the status index (`nexus-bounty-store.ts:263-275`). A late failure leaves the bounty already updated to `claimed` in storage. Releasing the claim here leaves `claimId` and `claimedBy` on the bounty pointing at a released claim, which blocks other claimants without giving the system a lease to manage or recover the work.

Consequence: a bounty stuck in `claimed` with no active claim, invisible to claim-expiry sweeps.

Design options

The reviewer's recommendations from rounds 1 and 3:

Do not transition to terminal `settled` until capture succeeds, or introduce a durable `pending_settlement`/outbox flow with retries and reconciliation.

Only void on failures proven to be pre-persistence, or make bounty creation atomic / confirm persistence before compensation. A minimal mitigation is to re-read the bounty by ID after a store error and skip `void()` if it already exists.

Three paths forward, in rough order of complexity:

(a) Reconciler + pending_settlement state (correct, most work)

1. Add a `pending_settlement` bounty status between `claimed` and `settled`.
2. `settleBountyOperation` transitions `claimed → pending_settlement`, then captures, then transitions `pending_settlement → settled`.
3. Add a background reconciler that sweeps `pending_settlement` bounties and retries `capture()` with exponential backoff. Compensating transaction when the reservation is dead.
4. Add post-commit confirmation in the create/claim compensation paths (re-read the bounty record and only compensate if the store state is pre-commit).

Rough size: 200-500 lines + tests + another codex review cycle. Probably 1-2 days.

(b) CAS confirmation only, no reconciler

1. Keep the existing state machine.
2. In each compensation catch, re-read the bounty by id (or check the CAS etag) to distinguish pre-commit from post-commit failures.
3. Skip compensation on post-commit failures; surface them with a clear error instead.
4. Leaves the settle-before-pay race unfixed unless settle is reordered back to "capture first, then persist".

Rough size: 100-200 lines + tests. Half a day.

(c) Revert the compensation refactor entirely (smallest)

1. Drop the try/catch compensation in `createBountyOperation`, `claimBountyOperation`, `settleBountyOperation`.
2. Rely on reservation auto-expiry to reclaim dead escrow and on claim lease timeouts to reclaim dead claims.
3. Keep the cosmetic changes (`MISSING_BOUNTY_STORE` const, pre-flight status check on claim).
4. Accept that failed bounty creates leak "open" state until the reservation deadline and failed claims leak "claimed" state until the lease expires.

Rough size: 50-100 lines. An hour.

Recommendation

Start with (c) to stop the bleed, ship it, then do (a) as a proper saga-log project with its own RFC and test harness. (b) is a middle ground but doesn't fix finding 1 (settle-before-pay), which is the critical one.

References

• Codex review round 1 of PR fix(tui,mcp,acpx): production-ready coder→reviewer loop via Nexus IPC #239 adversarial-review: findings [critical], [high], [high] on `src/core/operations/bounty.ts`.
• Codex review round 3: same three findings re-flagged after the commit excluded bounty.ts.
• PR fix(tui,mcp,acpx): production-ready coder→reviewer loop via Nexus IPC #239: fix(tui,mcp,acpx): production-ready coder→reviewer loop via Nexus IPC #239 — the PR that found these; bounty.ts changes intentionally withheld there.

Current state of the code

The buggy refactor is sitting uncommitted in the `ancient-nibbling-anchor` worktree on files:

• `src/core/operations/bounty.ts` — compensation logic + reordered settlement
• `src/core/operations/bounty.test.ts` — tests for the new compensation paths

Run `git stash` / `git diff` on that branch to see the exact 359-line diff.

[windoliver]

Update + corrections after re-review

Re-reviewing this issue in detail turned up a few things that were wrong or stale in the original filing. Posting as a comment so the original analysis stays as the historical record, but treat this as the authoritative state.

1. Draft PR now exists: #241

The "Current state of the code" section at the bottom is stale. The buggy refactor is no longer in an uncommitted worktree — it's committed to draft PR #241 on branch `bounty-saga-compensation`, explicitly marked `[BLOCKED ON #240]`.

Anyone picking this up should:

• Read the three findings below
• Decide on design option (a) / (b) / (c)
• Implement against the PR feat(bounty): compensation pattern for create/claim/settle [BLOCKED ON #240] #241 branch so the history stays linked

2. `capture()` is idempotent — this changes option (c)'s framing

I verified this in `src/core/in-memory-credits.ts:141-154`:

```typescript
// Idempotent: already captured — verify toAgentId matches
if (reservation.captured) {
const requestedTo = opts?.toAgentId;
const originalTo = reservation.capturedToAgentId;
if (requestedTo !== originalTo) {
throw new PaymentError({...});
}
return; // no-op
}
```

The `CreditsService` interface contract confirms: "Idempotent: capturing an already-captured reservation is a no-op, voiding an already-voided or already-captured reservation is a no-op".

This changes the analysis of option (c). I originally called (c) "stop the bleed", implying main's behavior is still broken. That was wrong. Main's `settleBountyOperation` uses capture → complete → settle ordering, which IS a valid retryable flow:

1. `capture()` — succeeds OR fails-retryable (no state change)
2. `completeBounty()` — if this fails after capture, retry the whole operation. Capture is a no-op on retry, so `complete` runs again.
3. `settleBounty()` — same.

The WIP's complete → settle → capture ordering is strictly worse because once `settleBounty()` persists the terminal `settled` state, the state machine rejects re-entry and the retry path is gone. A failed capture after settled-state leaves the bounty marked paid without funds being transferred, with no recovery.

3. Option (c) is a real fix, not a stopgap

Given the idempotency insight, option (c) (revert the compensation refactor, restore main's capture-first ordering) is sufficient for correctness if the caller retries the operation on failure. It doesn't need a reconciler or pending_settlement state to be safe — the idempotency of capture makes the retry path work.

The only residual bug in main's flow is: what if the client gives up retrying after a failed completeBounty/settleBounty? Then funds are captured but state is wrong. That's operator-repairable (manual advance to settled), whereas the WIP's failure mode (marked settled, no funds) is a silent lie to the worker.

So the updated recommendation is:

• (c) first — revert compensation, restore capture-first ordering. Correct and smallest diff.
• (a) later — pending_settlement + reconciler, as a separate project if we want to eliminate the "client gave up mid-retry" residual risk. Not required for correctness, only for operator ergonomics.
• (b) skip — middle ground doesn't buy enough over (c) to justify the complexity.

4. Finding 2 window is narrower than claimed

I said "`createBounty` writes the bounty document first, then the status index — if the second write throws, the compensation voids escrow while the bounty is persisted". That's still true, but:

• `writeStatusIndex` internally wraps in `withRetry` (see `src/nexus/nexus-bounty-store.ts:309`). Transient Nexus failures are already retried.
• The leak window is only after retry exhaustion (config-defined max attempts), not on a single transient blip.

The bug is real but narrower than the original analysis implied. Still worth fixing as part of option (a), not as a standalone concern.

5. Acceptance criteria (missing from original issue)

To close this issue, the fix must:

• `settleBountyOperation` either (a) never enters a terminal state before `capture()` confirms, OR (b) is explicitly retryable after a failed capture. Main's current capture-first ordering satisfies (b); option (c) restores this.
• `createBountyOperation` and `claimBountyOperation` compensate ONLY for pre-commit failures, or don't compensate at all and rely on reservation/lease expiry. No post-commit rollback of state already persisted to Nexus.
• A regression test that covers:
  • `capture()` throws after successful state transitions (must leave bounty retryable)
  • `bountyStore.createBounty()` throws after the document write but before the status index (must NOT void the reservation)
  • `bountyStore.claimBounty()` throws after the CAS write but before the status index (must NOT release the claim)
• Re-run adversarial review (`/review-loop`) against the diff and confirm no new HIGH/CRITICAL findings on `bounty.ts`.

6. Recommended next action

Go with option (c). Concrete steps:

1. Check out PR feat(bounty): compensation pattern for create/claim/settle [BLOCKED ON #240] #241's branch (`bounty-saga-compensation`).
2. Revert the try/catch compensation blocks in `createBountyOperation` and `claimBountyOperation`. Restore the original capture-first ordering in `settleBountyOperation`.
3. Keep the cosmetic improvements (`MISSING_BOUNTY_STORE` constant, pre-flight `open` status check in `claimBountyOperation`).
4. Prune `bounty.test.ts` tests that asserted the compensation paths; keep tests for the cosmetic changes.
5. Run `bun run check` + `bun test` + `/review-loop` (2 rounds min).
6. Mark PR feat(bounty): compensation pattern for create/claim/settle [BLOCKED ON #240] #241 ready for review, link this issue's acceptance criteria.

Estimated effort: 1-2 hours including the review cycle. Smaller than the original (c) estimate because "revert" is mostly mechanical.

Option (a) — full saga pattern with `pending_settlement` + reconciler — remains available as a follow-up if we want to eliminate the residual "client gave up mid-retry" case, but should be its own RFC-backed project with a dedicated test harness.

[windoliver]

Scope consolidation: absorbing #227's remaining work

This issue is now the single tracking issue for bounty multi-store atomicity and the saga design. The related concerns that used to live in #227 are folded in here so we don't split the work across two places.

What #227 originally covered

#227 had two parts:

1. Handoff creation on non-SQLite stores in `contributeOperation` — contribution written first, then handoff creation as a separate step. If handoff creation fails, the contribution exists but downstream agents aren't notified.
2. Bounty claim is two separate store calls in `claimBountyOperation` — `claimStore.claimOrRenew` then `bountyStore.claimBounty`, non-atomic.

What already landed in PR #239 (handoff half)

The handoff path from #227 has been partially addressed by commit `bc0a945` on #239:

• ✅ Parallel fan-out with logged per-failure warnings — `contributeOperation.writeSerial` now uses `Promise.allSettled` instead of serial loop + silent catch. One handoff failure no longer aborts the remaining handoffs, and every failure surfaces via `console.warn` with the target role and contribution CID. (`src/core/operations/contribute.ts`)

• ✅ `/tmp/grove-debug.log` noise gated — the unconditional inline `appendFileSync` calls in `nexus-handoff-store.ts` and `nexus-contribution-store.ts` are replaced with a shared `debugLog()` helper gated behind `GROVE_DEBUG=1`. Default-off is equivalent to "removed" for normal users but preserves the debugging capability. (`src/tui/debug-log.ts`)

• ❌ Contribution rollback on handoff failure is NOT done and should not be done. protocol: non-atomic multi-store writes in handoff and bounty paths #227's short-term recommendation was "delete the contribution if handoff creation fails". I deliberately did not implement this because rolling back committed agent work on a transient downstream notification failure is strictly worse than leaving the contribution committed with a logged warning. The proper fix for strict atomicity is a reconciler that sweeps contributions without matching handoffs and retries the handoff write, not a rollback. That reconciler is out of scope for PR fix(tui,mcp,acpx): production-ready coder→reviewer loop via Nexus IPC #239 and is now tracked under this issue.

What's still outstanding (this issue's scope)

1. Handoff reconciler (from protocol: non-atomic multi-store writes in handoff and bounty paths #227 part 1, the half that wasn't landed)

   • Sweep contributions whose `routedTo` roles don't have matching handoff records
   • Retry handoff creation with exponential backoff
   • Report orphaned contributions to operators after retry exhaustion
   • Integration tests against `NexusHandoffStore` with simulated mid-write failures

2. Bounty claim atomicity (from protocol: non-atomic multi-store writes in handoff and bounty paths #227 part 2) — `claimBountyOperation` today calls `claimStore.claimOrRenew` then `bountyStore.claimBounty` non-atomically. If step 2 fails post-commit, the bounty is stuck in `claimed` state with no active claim once the compensation is done wrong (see Finding 3 above).

3. Bounty create atomicity (new — mine) — `NexusBountyStore.createBounty` writes the bounty document and the status index as two separate operations. A failure between them leaks an "open" bounty out of the status index. See Finding 2 above.

4. Bounty settle-before-pay ordering (new — mine) — `settleBountyOperation` must either use main's capture-first ordering (which is retryable because `capture()` is idempotent) or introduce a `pending_settlement` state so the state machine allows resuming after a failed capture. See Finding 1 + the update comment above.

Recommended execution order

1. Option (c) first — revert the buggy compensation WIP. ~1-2 hours. Restores the capture-first ordering in `settleBountyOperation`, removes the try/catch compensation in `createBountyOperation` and `claimBountyOperation`, keeps only the cosmetic improvements (`MISSING_BOUNTY_STORE` constant + pre-flight `open` status check in claim). This gets the bounty paths back to a correct retryable state.
2. Handoff reconciler — separate focused project. ~1 day + tests. Add a sweep that covers `contributionStore \ handoffStore`.
3. Option (a) — full saga with `pending_settlement` — optional follow-up if we want to eliminate the "client gave up mid-retry" residual risk. RFC-scale project with its own test harness.

#227 is being closed and all remaining scope from it is tracked here. PR #241 has been closed (it was a draft holding the buggy WIP) — cut a fresh branch from `main` when implementing.

[yw13931835525-cyber]

/apply

I've reviewed the detailed owner guidance on this issue. I'll implement Option (c) — the revert path — which the owner has confirmed is sufficient for correctness.

Implementation Plan

1. Checkout PR feat(bounty): compensation pattern for create/claim/settle [BLOCKED ON #240] #241 branch bounty-saga-compensation

2. Revert compensation blocks:

   • Remove try/catch in createBountyOperation
   • Remove try/catch in claimBountyOperation
   • Restore original capture-first ordering in settleBountyOperation

3. Keep cosmetic improvements:

   • MISSING_BOUNTY_STORE constant
   • Pre-flight open status check in claimBountyOperation

4. Test cleanup:

   • Prune tests asserting compensation paths in bounty.test.ts
   • Keep tests for cosmetic changes

5. Validation:

   • bun run check
   • bun test
   • /review-loop (2 rounds minimum)

6. Mark PR feat(bounty): compensation pattern for create/claim/settle [BLOCKED ON #240] #241 ready for review and link acceptance criteria

Estimated time: 2-3 hours
Key advantage: Owner has already done the analysis — this is mostly mechanical revert work with clear acceptance criteria.

Request: Can you confirm the bounty amount before I begin? Happy to proceed at any reasonable value given the clear guidance provided.