fix(client): harden supervised connection recovery edge cases - #583
Merged
sleipnir merged 4 commits intoSep 1, 2026
Merged
Conversation
Follow-ups to elixir-grpc#568: - cancel pending retry timers on adopt/reschedule so a stale long-backoff timer can't delay redial after a fresh transport death - guard rebalance_after_reconcile adoption with channel_alive?, matching the :retry_establish handler, to avoid adopting an already-dead channel - republish LB state to :persistent_term when lb_mod.update/2 returns a new state, so pickers don't read stale state from behaviour-compliant LBs - seed retry_attempt from the flap count so a dial failure after a flap-death continues the backoff ladder instead of restarting it - log non-ok resolver update/2 results instead of silently ignoring them - liveness-check fallback channels carrying conn_pid (e.g. the documented disconnect/1 return) and return UNAVAILABLE instead of crashing in the adapter - align resolve_channel with channel_alive? on payloads without conn_pid so pid-less adapters aren't treated as permanently dead - resolve connection config in unavailable_result so failure results run the connection's interceptors with the caller's codec/compressor - stop re-picking when the LB policy returns the same channel (PickFirst)
sleipnir
requested changes
Sep 1, 2026
enilsen16
force-pushed
the
fix/connection-recovery-followups
branch
from
September 1, 2026 19:56
da2eff3 to
a5bfa20
Compare
Collaborator
|
Thank @enilsen16 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-ups to #568 — a set of recovery edge cases found while reviewing that change after merge.
Connection (
lib/grpc/client/connection.ex)schedule_retrydeduped on a boolean but never cancelled a pending timer, so a transport death after a resolver-driven recovery could sit out the remainder of an old long backoff (up to ~2min) instead of redialing at the newly computed delay. Now stores the timer ref, cancels/reschedules, andadopt_establishedcancels any pending retry.rebalance_after_reconcileadopted onconnected != []without thechannel_alive?check the:retry_establishhandler applies for exactly this race, so a channel that died with its:DOWNstill queued could produce a false:connected(waiters released, telemetry emitted) until the:DOWNunwound it.lb_mod.update/2was only kept in GenServer state;pick_channelreads:persistent_term, which was only re-put inestablish/1. A behaviour-compliant LB that returns fresh state fromupdate/2(rather than mutating ETS in place) left pickers on the stale state indefinitely. Now republished when the state changes (no-op for the built-ins).adopt_establishedresetretry_attemptto 0 but notflaps, so an establish failure right after a flap-death restarted the dial ladder at the shortest delay — the backoff could shrink after a failure.handle_channel_downnow seedsretry_attemptfrom the flap count.{:ok, _}returns fromresolver.update/2were silently discarded (the removedhandle_cast(:resolve_now)used to assert the match). Now logged, keeping the previous resolver state.Stub (
lib/grpc/stub.ex)fallback_channel's catch-all passed a payload like%{conn_pid: nil}— the documented return ofConnection.disconnect/1— straight to the adapter, crashing on a nil pid. Any payload carrying:conn_pidis now liveness-checked (nil/dead/remote → UNAVAILABLE); only payloads without the key pass through.resolve_channelusedMap.get(payload, :conn_pid), conflating a missing key with nil, whilechannel_alive?treats the same shape as alive — so a healthy connection over an adapter that exposes no transport pid could never serve an RPC from the stub side. Now aligned withchannel_alive?.unavailable_resultran the caller handle's (often empty) interceptor list with struct-default codec/compressor, while the healthy path uses the picked channel's config — so bare%Channel{ref: name}handles skipped the connection's interceptors on failures only. Now resolves config viaConnection.get_channel/1and honors the caller'scodec/compressoropts.Logger.warningper attempt). The loop now bails when the policy returns the same pid again.Verification
mix test: 10 doctests, 360 tests, 0 failures (2 pre-existing skips).mix format --check-formattedclean on both files.