swapclientserver: Wait for swapd reconnects

[bhandras]

Summary

Fixes #877.

The swapruntime client daemon can create its daemon-owned gRPC channel to swapd before swapd is listening. If the first swap RPC lands while the channel is still in gRPC's transient failure/backoff state, the call can fail with the stale connection refused from startup.

This PR adds grpc.WaitForReady(true) as a default call option on the daemon's swapd gRPC dial options. That keeps swapd-bound daemon RPCs waiting for the channel to become ready until the caller context expires, while leaving REST transport and direct SDK-created gRPC clients unchanged.

It also adds a regression test that:

• creates the daemon swap server clients before any swap server is listening,
• starts CreateInSwap,
• verifies it does not fail before the server starts,
• starts a fake gRPC swap server on the same address,
• verifies the original CreateInSwap completes.

Validation

• go test -tags=swapruntime ./swapclientserver
• make fmt-changed
• make lint-changed-local
• make commitmsg-lint range="origin/main..HEAD"

[bhandras]

@claude please review this PR. Context: this fixes #877 by setting wait-for-ready only on the swapruntime daemon's swapd gRPC dial options, plus a regression where the daemon client starts before the fake swap server listener exists and the first CreateInSwap waits until the server starts.

[gemini-code-assist]

Code Review

This pull request configures the gRPC swap server clients to use the WaitForReady(true) default call option, ensuring they wait through startup connection refusals instead of failing immediately. A new unit test, TestSwapServerClientsWaitForLateGRPCServer, has been added to verify this behavior. There are no review comments, and I have no additional feedback to provide.

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.

[claude]

Claude finished @bhandras's task in 1m 58s —— View job

---

PR Review: swapclientserver: Wait for swapd reconnects

• Read changed files (service.go, service_test.go)
• Analyze the WaitForReady dial option change
• Review the regression test for correctness & flakiness
• Post review feedback

---

Overall this is a clean, well-targeted fix. grpc.WaitForReady(true) is exactly the right primitive for the "channel dialed before the server is listening" race, and scoping it to swapServerDialOptions (leaving REST and SDK-created clients untouched) is correct. The regression test is genuinely a regression test — without the fix, NewClient is lazy, the first RPC hits TRANSIENT_FAILURE on connection refused, and CreateInSwap would return an error inside the 50 ms window and fail the test. Nicely done. A few points below, mostly for discussion.

1. Design note: WaitForReady applies to the whole channel, not just CreateInSwap

waitForReadyOpt is a WithDefaultCallOptions, and the resulting swapConn backs both swaps.NewGRPCSwapServerConn(swapConn) and the authenticated mailbox client (service.go:615-627). So every swapd-bound unary/stream RPC now waits for channel readiness rather than failing fast.

The consequence worth confirming: this converts fast failures into deadline-bound waits, and for callers that use the daemon rootCtx (no deadline) — e.g. the resumePending path that drives persisted sessions at startup — a permanently-unreachable swapd will now cause those RPCs to block until the channel becomes ready instead of erroring and letting the FSM retry with visible backoff. For the issue #877 scenario (swapd a few hundred ms late) this is exactly what we want; for a genuinely misconfigured/down swapd it changes the observable behavior from "fast error + FSM retry" to "silent block."

This is a legitimate trade-off, not necessarily a bug — but please confirm all swapd callers either pass a bounded context or that indefinite waiting on rootCtx is acceptable here.

2. Style: the new dial option has no explanatory comment

Per CLAUDE.md (comments explain WHY, not what), a change that subtly alters failure semantics for the whole channel is a good candidate for a short comment:

// WaitForReady makes daemon swapd RPCs block until the channel is
// ready (bounded by the caller context) rather than failing fast on
// the stale connection-refused error when swapd starts after the
// daemon dials it. See #877.
waitForReadyOpt := grpc.WithDefaultCallOptions(
        grpc.WaitForReady(true),
)

Fix this →

3. Test nit: redundant assertion in the first select

case result := <-resultChan:
        require.NoError(t, result.err)
        require.Fail(t, "CreateInSwap returned before swap server start")

Any receive on resultChan in this window should fail the test outright. The require.NoError first means an early success fails via require.Fail while an early error fails via require.NoError — the intent ("it must not return yet") reads more clearly as a single require.Fail that includes result.err in the message, e.g. require.Failf(t, "CreateInSwap returned before swap server start", "err=%v", result.err).

4. Minor: reserveLoopbackAddr has a TOCTOU window

Closing the listener and later re-binding the same port leaves a small window where another process could grab it. This is a common and generally acceptable test pattern (and t.Parallel() collision risk is low with an OS-assigned port), so just flagging it — no change required.

Things done well

• The CreateInSwap goroutine correctly hands results back over a channel instead of calling require/FailNow off the test goroutine (which would be unsafe).
• startLateSwapServer cleanly waits on server.Serve's return (nil after Stop()), so require.NoError(t, <-serveErr) is correct.
• Coverage is a true regression: the test would fail without the service.go change.

Nothing here is blocking; items 1–2 are the ones I'd most want addressed before merge.
· branch agent/issue877-swapd-reconnect

[bhandras]

@claude thanks, addressed the review feedback and pushed an updated branch.

Changes made:

• Added a comment above the daemon swapd WaitForReady option explaining why it exists, that it applies to the shared swapd channel, and how user RPCs vs background resume RPCs are bounded by their contexts.
• Simplified the first regression-test select so any early CreateInSwap return fails in one place and includes the returned error.

Validation rerun:

• go test -tags=swapruntime ./swapclientserver
• make fmt-changed
• make lint-changed-local
• make commitmsg-lint range="origin/main..HEAD"

Please re-review with this context.

[claude]

Claude finished @bhandras's task in 1m 3s —— View job

---

Re-review: swapclientserver: Wait for swapd reconnects

• Pull updated branch and diff the two follow-up changes
• Verify the WaitForReady comment (feedback lib: start adding lib helpers #1 & multI: add initial repo scaffolding #2)
• Verify the simplified regression-test select (feedback chainbackend+chainsource: add chainsource actor and backend impl #3)

---

Both follow-ups look good — thanks for addressing them.

1 & 2 — WaitForReady comment (service.go:875-880)

The new comment does exactly what was asked: it explains why the option exists, notes that it's a WithDefaultCallOptions on the shared swapConn, and — importantly — makes the failure-semantics trade-off from review point #1 explicit rather than implicit:

// The daemon may start before its configured swap server listener is
// accepting connections. WaitForReady prevents that stale startup
// refusal from failing the first swap RPC; user RPCs remain bounded by
// their request context, and background resume RPCs are bounded by the
// daemon root context so they keep waiting until swapd returns or the
// daemon shuts down.

This resolves my earlier concern — the "resume on rootCtx blocks until swapd is reachable or shutdown" behavior is now documented at the source, so a future reader won't mistake it for a fast-fail path. ✅

3 — Simplified regression-test select (service_test.go:1082-1090)

Now a single failure site that surfaces the returned error, reading clearly as "it must not return yet":

case result := <-resultChan:
        require.Failf(
                t, "CreateInSwap returned before swap server start",
                "err=%v", result.err,
        )

Matches the suggestion exactly. ✅

Still-good bits (unchanged, worth restating)

• The regression is real: without the service.go change, the lazy channel hits TRANSIENT_FAILURE on connection refused and CreateInSwap returns inside the 50 ms window → test fails.
• Result handoff over resultChan keeps require on the test goroutine.
• startLateSwapServer correctly awaits server.Serve's post-Stop() nil.

Remaining non-blocking note

The reserveLoopbackAddr close-then-rebind TOCTOU window (review point #4) is unchanged, which is fine — it's a standard and acceptable test pattern with OS-assigned ports.

No blocking issues. LGTM. 👍
· branch agent/issue877-swapd-reconnect