multi: fail a cooperative send when the operator can't fund the round - #915
Conversation
In this commit, we give the operator a way to tell a client *why* a round it had already joined failed, beyond the opaque reason string it gets today. We add a RoundFailureCode enum and a failure_code field to ClientRoundFailedResp. The first code we classify is ROUND_FAILURE_INSUFFICIENT_OPERATOR_FUNDS: the operator could not fund the round's commitment transaction from its own on-chain wallet. A client that sees this knows the round is dead on arrival, so it can terminally fail the originating job (a cooperative on-chain send, say) instead of returning its inputs to the live set and replaying them into an operator that has no coins to spend. Older servers, and failures we do not specifically classify, leave the field at ROUND_FAILURE_UNKNOWN, which a client treats as a generic recoverable failure and falls back to the reason string.
Regenerated via `make rpc` after adding the RoundFailureCode enum and the failure_code field to round.proto. No hand edits.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a terminal-failure status to the pending-intent outbox to prevent infinite replays of failed intents (e.g., when the operator cannot fund a commitment transaction). It updates the database schema, SQL queries, and state transitions to track and handle these terminal failures, ensuring that failed intents are retired while retaining their anchors for correlation. The review feedback suggests database optimizations, including adding a CHECK constraint and a composite index on (kind, status) to prevent full table scans as failed intents accumulate, along with dropping this index in the down migration. Additionally, a defensive nil check is recommended in the state transition logic to prevent potential nil-pointer dereferences.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 275e08253c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@claude review this |
|
Claude finished @Roasbeef's task in 2m 23s —— View job Review: PR #915 — fail a cooperative send when the operator can't fund the round
Nicely scoped change. The typed-code-at-the-RPC-boundary approach keeps proto types out of the FSM cleanly, and routing the terminal drop through the existing Below are the things worth addressing before merge, then my read on the existing bot comments. 1. (confirming Codex P2) Re-persisting the same failed send silently drops the retry
ON CONFLICT (intent_id) DO UPDATE
SET requested_at_unix = excluded.requested_at_unix;so the retained row stays Fix is small: reset the status/failure fields on re-persist so a reused ID re-arms as pending. ON CONFLICT (intent_id) DO UPDATE
SET requested_at_unix = excluded.requested_at_unix,
status = 'pending',
failure_reason = NULL,
failure_code = 0;Fix this → — remember to 2. (partly agreeing with Gemini) index on the retained rowsSince failed rows are retained indefinitely while successful ones are deleted, The 3. (disagreeing with Gemini) the nil-transition check at
|
In this commit, we teach the pending-intent outbox to record that an intent is dead. Until now an intent had exactly two fates: a round adopts it (the row is cleared in the same transaction that checkpoints the round), or it replays on the next restart. A round that fails terminally, because the operator cannot fund its commitment tx, had no third path, so the send replayed forever into the same wall and the user's activity entry sat pending indefinitely. Migration 000011 adds a status column (defaulting to 'pending') plus a failure_reason and a typed failure_code to the kind-agnostic pending_intents header. The send and board replay queries now select only status = 'pending' rows, so a failed intent is never re-submitted. MarkPendingSendIntentFailedByOutpoint flips the intent anchored to a forfeited outpoint to 'failed', recording the reason and code, and is idempotent under the status guard so a second forfeit outpoint of the same intent is a no-op. We keep the anchors on a failed intent rather than clearing them, so the record stays correlatable by its consumed outpoint for the activity projection that will later surface it. We bump LatestMigrationVersion to 11 so the daemon's downgrade guard knows about the new migration.
9668fce to
9e1574f
Compare
Regenerated via `make sqlc` after migration 000011 and the new pending-intent queries. No hand edits.
In this commit, we consume the operator's typed round-failure code on the client and use it to stop a doomed cooperative send from replaying forever. This is the client half of the #889 fix: on signet and testnet an empty operator wallet failed every send at the operator's FundPsbt, the client could not tell that apart from a transient hiccup, so it released its inputs, left the intent pending, and re-submitted it into the same broke operator on every restart. We add a native round.RoundFailureCode enum, mapped to and from the wire roundpb value only at the RPC boundary (FromProto / ToProto) so the FSM never traffics in proto types. BoardingFailed and ClientFailedState both carry it, and every pre-signing BoardingFailed handler stamps it onto the failed state. The release chokepoint does the rest. releaseForfeitsOnFailure already runs for the pre-signing states, already detects the transition into ClientFailedState, and already holds the forfeit set, which is exactly the originating job's pending-intent anchors. When the failure code is terminal-for-job, it emits a TerminalJobFailedNotification carrying those outpoints alongside the existing forfeit release, so the job is failed in the same breath its inputs return to the live set. The round actor handles that notification by asking the store to fail the intent; the call is best-effort, so a store hiccup degrades to the prior replay behavior rather than wedging the actor. A generic or unknown failure is untouched: it releases the forfeit and leaves the intent eligible for replay, which is the right thing when the operator is only briefly unavailable.
In this commit, we wire the store side of the terminal send-failure path. FailForfeitIntents is the failure counterpart to the anchor clear CommitState performs when a round succeeds: given the forfeited outpoints of a round that died terminally, it marks each anchored send intent 'failed' with the reason and typed code, in one write transaction. We mark rather than delete deliberately. Marking both stops the replay, since the replay query now skips non-pending rows, and leaves a durable, anchor-correlatable record the activity projection can later surface as failed instead of leaving the entry stuck pending. To share the per-outpoint anchor walk with the success path, the existing clearForfeitIntentAnchors is refactored to run over a plain outpoint slice.
In this commit, we test the client half of the fix. The round tests pin the wire-to-native failure-code mapping (including an unrecognized newer-server code degrading to unknown), assert BoardingFailed decodes the code, and check that releaseForfeitsOnFailure emits the terminal drop for a classified failure with the right forfeited outpoints while a generic failure only releases the forfeit and leaves the intent to replay. The db test proves FailForfeitIntents marks the send intent failed and excludes it from the replay list, that its header and anchors are retained (not deleted) so the record stays correlatable, and that a repeat call is a no-op that never overwrites the original reason.
9e1574f to
57181f9
Compare
|
Thanks @gemini-code-assist and @chatgpt-codex-connector for the passes, and the Claude review too. Round 1 is resolved in-thread; here's the roundup plus a second, deeper review round. Round 1 (bot threads, resolved)
Round 2 (Fable advisor review)Ran an independent Fable review over both branches. It caught a real one:
New end-to-end coverageAdded Both this and darepo#663 are clean locally on build/test/lint/commitmsg. See each commit for the incremental detail. |
|
To use Codex here, create an environment for this repo. |
|
Warning Gemini encountered an error creating the issue-comment-reply. You can try again by commenting |
In this commit, we add the end-to-end reproduction of #889 on the cooperative send path. A send --onchain --sweep-all reserves the VTXO and registers a round, the fake operator admits it and then pushes a typed insufficient-operator-funds ClientRoundFailedResp, and we assert the client classifies it, returns the VTXO to LIVE, and durably retires the send intent. The teeth of the test are the restart-and-assert-no-replay check. A send that only released its VTXO but stayed pending would replay on the next daemon start and fire a second JoinRound into the same broke operator, which is the #889 "hangs forever" loop. So after the failure we restart the daemon and assert no second JoinRound is ever sent. This is the same path the release chokepoint used to swallow the terminal drop on: the client sits in IntentSentState, whose handler builds its own failure outbox, so the test would have caught that regression. To drive it, the fake mailbox server grows a setFailRoundOnJoin lever that pushes the ClientRoundFailedResp back as a KIND_EVENT round-failed event on every JoinRound.
e58a238 to
fbc921c
Compare
In this commit, we close a gap the operator fund-failure review surfaced. QuoteReceivedState, CommitmentTxValidatedState, and NoncesAggregatedState had no BoardingFailed case, so a server-pushed round failure in any of them self-looped. Both the forfeit release, which returns the reserved inputs to LiveState, and the pending-intent retirement on a terminal-for-job code were silently dropped, stranding the VTXOs in pending-forfeit until a higher-level timeout and, for a future terminal code that could fire that early, replaying the job forever. All three states already wrap their handler in releaseForfeitsOnFailure, so the fix is just to give them a BoardingFailed case that fails into ClientFailedState carrying the typed code; the wrapper releases the forfeits and retires the job. We extend the pre-signing release and terminal-retire tables to drive a failure through each of the three. The current insufficient-operator-funds code does not reach these states (it fires while the client sits in RoundJoinedState, which was already handled), so this hardens against recoverable mid-quote or mid-signing failures and any future terminal code, not a live #889 regression.
7f7ff42 to
6dce664
Compare
The send-failure tests from #915 verified pending_intents state with raw QueryRowContext calls using SQLite '?' placeholders. The pgx stdlib driver does not rewrite '?' to $N, so on postgres "intent_id = ?" parses '?' as a dangling operator and fails with "syntax error at end of input" (SQLSTATE 42601): the tests passed on sqlite and failed on test_postgres. Add GetPendingIntentByID, CountPendingIntentAnchorsByIntentID, and CountPendingSendIntentsByIntentID to the pending_intents sqlc queries and route the test assertions through the generated methods, dropping the raw SQL entirely (repo rule: no raw SQL in Go). sqlc emits the correct placeholder per dialect, so the verification is portable by construction.
The send-failure tests from #915 verified pending_intents state with raw QueryRowContext calls using SQLite '?' placeholders. The pgx stdlib driver does not rewrite '?' to $N, so on postgres "intent_id = ?" parses '?' as a dangling operator and fails with "syntax error at end of input" (SQLSTATE 42601): the tests passed on sqlite and failed on test_postgres. Add GetPendingIntentByID, CountPendingIntentAnchorsByIntentID, and CountPendingSendIntentsByIntentID to the pending_intents sqlc queries and route the test assertions through the generated methods, dropping the raw SQL (repo rule: no raw SQL in Go). sqlc emits the correct placeholder per dialect, so the verification is portable by construction.
The send-failure tests from #915 verified pending_intents state with raw QueryRowContext calls using SQLite '?' placeholders. The pgx stdlib driver does not rewrite '?' to $N, so on postgres "intent_id = ?" parses '?' as a dangling operator and fails with "syntax error at end of input" (SQLSTATE 42601): passed on sqlite, failed on test_postgres. Add GetPendingIntentByID, CountPendingIntentAnchorsByIntentID, and CountPendingSendIntentsByIntentID to the pending_intents sqlc queries and route the test assertions through the generated methods, dropping the raw SQL (repo rule: no raw SQL in Go). sqlc emits the correct placeholder per dialect, so the verification is portable by construction.
1 participant
In this PR, we fix the client half of #889: a
send --onchain --sweep-all(or any cooperative consumption) that hangs forever as "pending" while the balance comes back, and re-tries into the same wall on every restart.The root cause turned out to be operator-side. On signet and testnet the operator's lnd wallet was empty, so arkd's FundPsbt failed for every commitment tx. But the client couldn't tell that apart from a transient hiccup. It got an opaque reason string, treated the failure as recoverable, released its forfeit (so the VTXO went back to LIVE, which is the "balance returned" the reporter saw), but left the persisted send intent in place. On the next restart the intent replayer re-submitted it into the same broke operator, and the activity entry sat pending indefinitely. This is not the vHTLC/unroll path from #909: that repro was an OOR coin, this is a plain cooperative round.
The typed failure code
We add a native
round.RoundFailureCodeenum, carried onBoardingFailedandClientFailedState, and mapped to and from the wireroundpb.RoundFailureCodeonly at the RPC boundary (FromProto/ToProto) so the FSM never traffics in proto types. The sharedClientRoundFailedRespproto grows afailure_codefield. An older server, or a failure we don't classify, leaves itROUND_FAILURE_UNKNOWN, and the client falls back to the reason string exactly as before.Retiring the dead send job
The forfeit-release chokepoint does the work.
releaseForfeitsOnFailurealready runs for the pre-signing states (where an operator-funding failure lands), already detects the transition intoClientFailedState, and already holds the forfeit set, which is exactly the originating job's pending-intent anchors. When the failure code is terminal-for-job, it emits aTerminalJobFailedNotificationcarrying those outpoints alongside the existing forfeit release, so the job is failed in the same breath its inputs return to the live set. The round actor hands that to the store'sFailForfeitIntents.On the db side we mark the intent failed rather than delete it (migration
000011adds a status, reason, and typed code to the pending-intents header). Marking both stops the replay, since the replay query now selects onlypendingrows, and leaves a durable, anchor-correlatable record. A generic or unknown failure is untouched: it releases the forfeit and leaves the intent eligible for replay, which is right when the operator is only briefly down.Since failed rows are retained while successful ones are deleted on adoption, the outbox accumulates only failures over time, so
000011also adds a composite(kind, status)index to keep the replayer's startup scan off that pile.Re-arming a retried send
NewPendingIntentIDis deterministic over the consumed inputs and payload, so a user retrying the exact same failed send reuses the sameintent_idand upserts straight onto the retainedfailedrow.UpsertPendingIntentHeadernow resetsstatus/failure_reason/failure_codeon conflict, so the retry re-arms aspending. Without that reset, a retry persisted but not yet adopted by a round would be silently dropped by the newpending-only replay filter if the daemon crashed in that window, which is the same "hangs forever" failure mode one hop removed. Thanks to the Codex and Gemini review passes for catching this and the index.Follow-up
Surfacing the activity entry as FAILED, rather than dropping it back to pending, is deliberately left to the C2 activity-correlation work. The cooperative-leave EXIT is a runtime-local row that only the SDK runtime can project with fidelity, and it has no daemon read for a failed cooperative send yet. The durable failure record this PR lands is what that follow-up will read.
Testing
Unit coverage for the code mapping in both directions (including an unrecognized newer-server code degrading to unknown), the release chokepoint emitting the terminal drop only for a classified failure, the store marking the intent failed and excluding it from replay while keeping the record, and a regression that fails an intent, re-persists the identical one, and asserts it replays again with the failure fields cleared. The server-side end-to-end regression lives in the darepo PR.
See each commit message for the detailed reasoning w.r.t the incremental changes.