tonic: `Reconnect::poll_ready` returns `Ok(())` on connection error, breaking tower's p2c `Balance` failover

PR #458 — "fix(transport): reconnect lazy connections after first failure" changed tonic's internal Reconnect service so, if lazy, when it finds a connection error it returns Poll::Ready(Ok(())) in poll_ready, deferring returning the error to call. Tower's p2c Balance, which sits directly on top of these services in Channel::balance, relies on poll_ready returning Err to detect a broken endpoint and route around it. Because Reconnect::poll_ready never does this for lazy/reconnecting endpoints, Balance can select a broken endpoint as "ready" and dispatch a request to it via Service::call, which then fails and forces the caller to retry the RPC instead of tower transparently failing over to a healthy endpoint.

This is affecting services that have a retry budget, since such budget is consumed by connection errors form request errors and are forced to consume such budget in connection errors.

Root cause / affected code

Tonic's src/transport/channel/service/reconnect.rs

Reconnect tracks a one-shot error field and lazy/has-been-connected flags:

// lines 36-47
pub(crate) struct Reconnect<M, Target>
where
    M: Service<Target>,
    M::Error: Into<crate::BoxError>,
{
    mk_service: M,
    state: State<M::Future, M::Response>,
    target: Target,
    error: Option<crate::BoxError>,
    has_been_connected: bool,
    is_lazy: bool,
}

poll_ready has two places where it returns Ok(()) despite a connection error:

// lines 86-91
fn poll_ready(&mut self, cx: &mut Context<'_>) -> Poll<Result<(), Self::Error>> {
    let mut state;

    if self.error.is_some() {
        return Poll::Ready(Ok(()));
    }

If a previous poll already recorded an error, any subsequent poll_ready call short-circuits to Ok(()) — the caller (Buffer worker / balancer) is told the service is ready, even though it is permanently broken until the next call happens to consume the stored error.

// lines 109-133
State::Connecting(ref mut f) => {
    trace!("poll_ready; connecting");
    match Pin::new(f).poll(cx) {
        Poll::Ready(Ok(service)) => {
            state = State::Connected(service);
        }
        Poll::Pending => {
            trace!("poll_ready; not ready");
            return Poll::Pending;
        }
        Poll::Ready(Err(e)) => {
            trace!("poll_ready; error");

            state = State::Idle;

            if !(self.has_been_connected || self.is_lazy) {
                return Poll::Ready(Err(e.into()));
            } else {
                let error = e.into();
                tracing::debug!("reconnect::poll_ready: {:?}", error);
                self.error = Some(error);
                break;
            }
        }
    }
}

// lines 156-161
    self.state = state;
    }

    self.state = state;
    Poll::Ready(Ok(()))
}

When a connect attempt fails and has_been_connected || is_lazy is true, the error is stashed in self.error, the loop breaks, and execution falls through to line 160-161, which still returns Poll::Ready(Ok(())). Only the non-lazy and never-connected case (line 124-125) returns Err immediately from poll_ready.

Towers p2c balancer

Tonic uses tower's p2c balancer which considers a service to be ready by calling poll_ready on it (see here). An endpoint being ready if its service returns Poll::Ready(Ok()) (see here).

In tonic, that inner service is Connection which as said before, if lazy, on connection error will return Poll::Ready(Ok()) and tower's balancer won't try to pick a different one.