swapclientserver: scope reconnect waiting

[bhandras]

Problem

The daemon configures one gRPC channel for swapd. PR #949 made that entire channel use WaitForReady(true) so a state-changing swap RPC can survive swapd starting or reconnecting after the daemon.

That channel-wide policy also changed optional read-only calls. In particular, swapwallet.Service.fetchBalance first obtains the Ark balance and then calls ListCredits as an optional enrichment. The wallet deliberately ignores a failed credit read and can return the Ark balance alone. With the global wait-for-ready option, however, an unavailable swapd makes ListCredits consume the parent request's full deadline. The already-computed balance can no longer be returned because the wallet RPC context has expired.

This surfaced in swapdk-server PR #228 as TestWalletBalanceSurfacesUnilateralExit failing its first balance request with DeadlineExceeded.

Changes

• Replace the channel-wide default call option with a unary client interceptor.
• Apply wait-for-ready to RPCs that create or advance swap and credit protocol state.
• Keep QuoteInSwap, ListCredits, mailbox calls, and unknown methods fail-fast.
• Preserve the reconnect behavior from swapclientserver: Wait for swapd reconnects #949 for CreateInSwap and the other durable protocol operations.

Policy

Wait for swapd to become ready:

• RequestChannelId
• CreateInSwap
• CreateCredit
• RedeemCredit
• AuthorizeInSwapRefund
• AcknowledgeOutSwapHtlc
• SignInSwapForfeit
• SubmitOutSwapForfeitSignature

Fail fast when swapd is unavailable:

• QuoteInSwap
• ListCredits
• mailbox RPCs
• unknown future RPCs, until their retry semantics are chosen explicitly

Tests

• Added an unavailable-server regression proving ListCredits returns Unavailable before its context expires.

• Added a table test that locks down the method-level reconnect policy.

• Preserved and reran the swapclientserver: Wait for swapd reconnects #949 regression proving CreateInSwap waits through a late server start.

• Reran the exact downstream failure against swapdk-server PR btcwbackend: add neutrino+btcwallet wallet mode #228:

  TestWalletBalanceSurfacesUnilateralExit passed in 37.2 seconds.

• go test -tags='swapruntime' ./swapclientserver -count=1

• make fmt-changed-check base=origin/main

• make lint-local

• make commitmsg-lint range=origin/main..HEAD

Fixes #951.

[bhandras]

@claude Please review this follow-up with the regression context from #951 and #949 in mind.

The key invariant is that durable/state-changing swap RPCs must still wait through a late swapd start or reconnect, while optional reads such as ListCredits must fail fast so callers can return partial results without losing their own deadline. The existing late-CreateInSwap regression remains green, the new unavailable-ListCredits regression is green, and swapdk-server's previously failing TestWalletBalanceSurfacesUnilateralExit passes against this branch.

[claude]

Claude finished @bhandras's task in 2m 4s —— View job

---

Review — swapclientserver: scope reconnect waiting

I read the diff against origin/main, cross-checked the policy against the full SwapService surface, and traced how the swap channel is shared with the mailbox client. This is a clean, well-scoped fix. Details below.

Correctness — the invariant holds ✅

• Complete coverage of mutating RPCs. SwapService defines exactly 10 RPCs. The wait-list covers all 8 state-changing ones (RequestChannelId, CreateInSwap, CreateCredit, RedeemCredit, AuthorizeInSwapRefund, AcknowledgeOutSwapHtlc, SignInSwapForfeit, SubmitOutSwapForfeitSignature); only QuoteInSwap and ListCredits fall to the fail-fast default. There is no protocol operation left uncovered, so the durable/late-start behavior from swapclientserver: Wait for swapd reconnects #949 is fully preserved.
• Idiomatic interceptor. Appending grpc.WaitForReady(true) to opts before calling invoker (service.go:930-935) is the correct per-call way to conditionally set the option, and WithChainUnaryInterceptor is the only interceptor on this conn so there's no ordering concern.
• Background resume still waits. The resume path drives the mutating RPCs (CreateInSwap, forfeit signing, etc.), so it still gets WaitForReady(true) and remains bounded by the daemon root context — the swapclientserver: Wait for swapd reconnects #949 semantics survive.
• Mailbox fail-fast is deliberate and safe. The mailbox client shares swapConn (service.go:624-625), and its RPCs (Send/Pull/AckUpTo) are all unary, so they now hit the fail-fast default instead of the old channel-wide WaitForReady(true). That's the right call: serverconn owns its own reconnect/retry loop, and masking fast failures with WaitForReady would fight that loop rather than help it. Consistent with the stated policy.

Tests ✅

• TestSwapServerOperationWaitsForReady locks the method-level policy (all 8 wait, QuoteInSwap/ListCredits/mailbox fail fast) — good regression guard against accidental policy drift.
• TestSwapServerClientsBestEffortReadsFailFast proves ListCredits returns Unavailable well within its 1s deadline (require.NoError(t, ctx.Err())), which is precisely the swapdk-server TestWalletBalanceSurfacesUnilateralExit failure mode.
• The late-CreateInSwap regression is preserved.

Minor suggestions (non-blocking)

1. Stale variable name. waitForReadyOpt (service.go:879) now holds a chain-unary-interceptor dial option, not a call option. A name like waitForReadyInterceptorOpt would read more truthfully at the three return sites. Fix this →

2. Lost rationale in the code comment. The old comment on waitForReadyOpt documented that user RPCs stay bounded by their request context while background resume RPCs are bounded by the daemon root context. That nuance is gone from the source. Since it explains why fail-fast on reads is safe and why resume still waits, it's worth keeping a trimmed version near the interceptor so a future reader doesn't have to reconstruct it.

3. Implicit default-deny for future RPCs. New mutating RPCs will silently fail-fast until explicitly added to swapServerOperationWaitsForReady. The PR description calls this out intentionally, and the default branch is the safe direction — but a one-line comment on the default case (or a table-test case asserting an unknown SwapService method returns false) would make the "add me when you add a new mutating RPC" contract self-documenting. This also guards the (currently non-existent) case of a future streaming mutating RPC, which this unary-only interceptor would not touch.

None of these affect correctness — the change does exactly what the invariant requires. Nice, surgical fix.
· agent/scoped-swapserver-wait-for-ready

[gemini-code-assist]

Code Review

This pull request replaces the global WaitForReady gRPC dial option with a unary interceptor (swapServerWaitForReadyInterceptor) that selectively applies the reconnect policy. This ensures that only protocol-advancing swap operations wait for the server to reconnect, while read-only queries (such as quotes and credit snapshots) fail fast to prevent consuming the caller's context deadline. Accompanying unit tests have been added to verify this behavior. I have no feedback to provide as the implementation is clean and well-tested.

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.

[bhandras]

@claude Thanks, all three non-blocking suggestions are folded into the original commit:

• renamed the dial option to waitForReadyInterceptorOpt;
• restored the distinction between request-context-bounded user operations and daemon-root-context-bounded background resumes; and
• documented the fail-fast default for future RPCs and added an explicit unknown future swap-method test.

After folding, the focused package tests, formatter check, commit-message lint, and local lint all pass. I also reran swapdk-server's exact downstream TestWalletBalanceSurfacesUnilateralExit regression against the new SHA; it passed in 39.593 seconds. Please re-review the final form.

[claude]

Claude finished @bhandras's task in 44s —— View job

---

Re-review — swapclientserver: scope reconnect waiting

Confirmed the final form against origin/main. All three non-blocking suggestions are folded in correctly.

• Read the folded service.go diff
• Verify the three suggestions landed
• Check the new unknown-method test
• Post verdict

Suggestions verified ✅

1. Rename — waitForReadyOpt → waitForReadyInterceptorOpt, applied at all three return sites (service.go:880, and the three dial-option branches). Reads truthfully now that it holds a grpc.WithChainUnaryInterceptor dial option rather than a call option.

2. Restored rationale — the interceptor doc comment now carries the nuance that was lost:

   User operations remain bounded by their request context, while background resume operations remain bounded by the daemon root context. Read-only quotes and credit snapshots retain gRPC's fail-fast behavior.
   That captures both why fail-fast on reads is safe and why resume still waits.

3. Future-RPC contract — the default branch now documents the default-deny (Future RPCs fail fast until their retry and recovery semantics are explicitly classified above), and TestSwapServerOperationWaitsForReady adds the "future swap method" case (/swaprpc.SwapService/FutureOperation → false). The "add me when you add a mutating RPC" contract is now self-documenting and test-guarded.

Still correct after the fold ✅

• The wait-list in swapServerOperationWaitsForReady still covers all 8 mutating SwapService RPCs; QuoteInSwap, ListCredits, mailbox, and unknown methods fall through to fail-fast. Policy is unchanged from my first pass — only names and comments moved.
• The interceptor appends grpc.WaitForReady(true) to opts before delegating to invoker, which is the correct per-call application. WithChainUnaryInterceptor remains the only interceptor on swapConn, so no ordering concern.
• Tests: TestSwapServerClientsBestEffortReadsFailFast still asserts codes.Unavailable with require.NoError(t, ctx.Err()) (the swapdk-server TestWalletBalanceSurfacesUnilateralExit failure mode), the method-policy table test locks the surface, and the late-CreateInSwap regression is preserved.

Verdict

LGTM. The three edits are exactly what was asked for, correctness is unchanged, and the future-RPC test closes the one self-documentation gap from the first review. Thanks for the thorough downstream re-run (TestWalletBalanceSurfacesUnilateralExit, 39.6s) — that's the signal that matters. Nice surgical follow-up.
· agent/scoped-swapserver-wait-for-ready