Skip to content

Proxy: a client that disconnects before the first upstream response byte is never detected #1406

Description

@ndizazzo

Summary

Both API-proxy route arms fully await the upstream HTTP response probe before they ever look at downstream state. If the client disconnects before the first upstream response byte arrives, nothing detects it and nothing cancels the upstream.

This is the pre-TTFT sibling of the defect fixed in #1405 (RV-DEFECT-003). #1405 closes the window after the response head arrives; this issue is the window before it.

Where

crates/mesh-llm-host-runtime/src/network/openai/response/routing.rs

  • Local arm: route_local_attempt_after_forwardprobe_http_response_local(upstream).await is awaited to completion; tcp_stream is untouched until relay_attempted_response.
  • Remote arm: route_remote_attempt_after_forward — same shape with probe_http_response(quic_recv).await.

probe_http_response_with_timeout (response/probe.rs) reads only from the upstream reader. There is no downstream watcher anywhere in that path.

Exposure (bounded, but not trivially)

Exposure ends the moment upstream produces its first response byte, so this is not the unbounded full-generation leak that RV-DEFECT-003 was. Concretely:

  • Remote first-byte timeout is 5 min (response_first_byte_timeout), local is 10 min (local_response_first_byte_timeout).
  • On the remote arm a first-byte timeout yields RetryableTimeout, which should_retry_uncommitted_remote_attempt retries once (REMOTE_UNCOMMITTED_RETRIES = 1) on a fresh tunnel. So a departed client can drive two full prefill attempts, worst case ~10 min of peer GPU time.
  • Secondary defect in the same function: only the first read is timeout-guarded. Continuation reads go through read_response_chunk, documented "without any timeout." An upstream that sends a partial header and then stalls blocks the arm forever, with no downstream detection and no cap. MAX_HEADER_BYTES bounds size, not time.

Why this is smaller than it looks

Two things that were open questions are already settled by #1405 and by the existing code:

  1. "What does cancelling mean per transport" is already solved. fix: close the four rc6 release-validation defects #1405 centralised it behind the CancelUpstream trait (response/cancellation.rs): shutdown() for local TCP, QUIC STOP_SENDING for the peer stream. A probe-window fix reuses cancel_upstream_if_client_disconnected unchanged.
  2. The detector is one detector, not two. It watches the downstream TcpStream, which is identical on both arms. There is no per-transport detector work.

Also: both upstreams are created fresh per attempt (TcpStream::connect in acquire_local_attempt_upstream, node.open_http_tunnel in route_remote_attempt). Neither is pooled, so cancelling mid-probe cannot poison a reused connection.

Suggested approach

tokio::select! the probe future against a downstream-close watcher. The borrows are disjoint — the probe holds &mut upstream, the watcher only needs &*tcp_stream — so this needs no Arc, no split(), and no spawned task.

/// Resolves when the downstream client is gone. Never consumes request bytes.
async fn wait_for_downstream_close(stream: &TcpStream) {
    let mut byte = [0u8; 1];
    loop {
        match stream.peek(&mut byte).await {
            Ok(0) => return,                        // FIN
            Err(_) => return,                       // RST / socket error
            Ok(_) => std::future::pending().await,  // pipelined bytes: leave them, stop watching
        }
    }
}

tokio::net::TcpStream::peek takes &self and does not consume, which is what keeps a pipelined follow-up request intact.

On trigger, return RouteAttemptResult::ClientDisconnected and hand it to the existing cancel_upstream_if_client_disconnected. ClientDisconnected is already terminal in should_retry_uncommitted_remote_attempt, so this correctly stops the retry from re-firing.

The one genuine design risk

Read-side EOF is not the same as "the client is gone." A client that does shutdown(SHUT_WR) after sending its request and then waits for the response is legal HTTP/1.1, and the watcher above would abort it. nginx accepts exactly this tradeoff (proxy_ignore_client_abort off is the default), and half-close-then-await is not a pattern any OpenAI-style client uses in practice — but it is a real behaviour change and it is why this is not a mechanical fix and did not belong in #1405.

Decide it explicitly before implementing. If we want to be conservative, the Err(_) (RST) arm alone is unambiguous and still catches the common Ctrl-C case on most clients; it just misses graceful closes.

Test shape

Follow the pattern #1405's 1acd40c14 established: drive a real socket rather than asserting on an error kind. Hold the upstream silent (no header at all), close the client with set_zero_linger() to force an RST, and assert the arm returns ClientDisconnected and that the upstream saw its cancel — for both the local TCP and remote QUIC arms.

The no-timeout continuation-read gap should get its own test: send a partial header, stall, assert the arm does not hang.

Not in scope for #1405

#1405 is a release-unblocking PR for four validated rc6 defects. This is a hot-path concurrency change on both transports with a user-visible behaviour decision attached; it gets its own PR and its own validation.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions