fix(gateway): a stalled reader holds a relay for five hours, the connection limit does not limit, and a zero-length write spins a core - #1245
Merged
Conversation
Both pre-gate relays select! on the watchdog and the two reads, then await a bare write_all in the winning arm. write_all is not cancel-safe, so it sits outside the select! and nothing polls the watchdog for as long as it blocks. A peer that stops reading fills the socket buffer and parks the write there, so the connection survives until timeouts.total (5h by default) -- a relay each for a client that sends a request and never reads the answer. The drain phase below already had the fix; this lifts its partial-write loop into a write_watched! macro and uses it in the main loop too, so there is one watched write rather than two shapes.
The buffered bridge's Write step ignored what write_buf returned. A writer that reports Ok(0) with a non-empty buffer leaves the direction in Write, the loop re-enters immediately, and the zero-length write still bumped the progress counter -- so the idle watchdog saw a busy connection and the bridge span hot on a core until timeouts.total. The two relay_until paths already bail on a zero-length write; this is the third implementation of that loop agreeing with them.
take_sni read into a fixed 4 KiB buffer that never grew. Once full, the next read landed in an empty slice and returned Ok(0), so the loop broke and the connection was refused with "no sni found" -- the same message a client that hung up gets, and nothing named a limit. 4 KiB is not the margin it looks like: a TLS 1.3 hello with a post-quantum key share (X25519MLKEM768 is ~1.2 KiB), a session ticket and ECH is already past 2 KiB, and the client chooses extension order, so server_name can sit behind all of it. The buffer now grows to one record's worth of handshake (2^14 bytes plus the record header), which is as much as extract_sni can parse anyway, and the refusal past that says which limit it hit.
extract_sni skipped the 5-byte record header without looking at it, so any protocol whose sixth byte happens to be 1 was parsed as a ClientHello. Records are still not reassembled, and the module now says so: a hello fragmented across records parses differently here than in rustls, which decides routing and never trust -- rustls re-parses the hello on the terminate path and the app terminates the TLS itself on the passthrough path, and the client picks both SNIs in any case.
check_connection_limit summed the instances' counters, compared the total to max_connections_per_app, and left the increment to connect_multiple_hosts below it. Connections that arrive together all read the same pre-increment total and all pass: 32 connections released at once took 9-20 slots against a limit of 4, over 8 runs. That is the one regime a connection limit exists for. The reservation is now the check. fetch_update on the first candidate's counter -- the same counter every connection of the app races from, so nothing is counted twice -- decides and increments in one step, and hands two connections racing for the last slot different values. The other candidates' counts are still read outside that step, so a multi-instance app can overshoot by up to connect_top_n - 1 at the boundary, bounded by the candidate count rather than by arrival concurrency; a single-instance app is exact. The winner's slot is a ConnectionSlot rather than an EnteredCounter because that type increments when it is built, and here the count is already taken by the time there is a guard to hold it.
This was referenced Sep 20, 2026
Merged
kvinwang
added a commit
that referenced
this pull request
Sep 25, 2026
…te abort PR #1284 refuses an SNI that is not a DNS name without echoing it, PR #1245 reads a ClientHello past the first 4096 bytes up to one record and requires a handshake record, and PR #1278 fixes a parser abort on a length byte that counts itself. tc-gw-proxy-prot-003 drives the first two through the live proxy; tc-gw-internal-004 loads the parser with #[path], since its inner doc comment broke include!, adds the self-counting and non-handshake rows, and sends the abort probe to the live listener. Signed-off-by: Kevin Wang <wy721@qq.com>
kvinwang
added a commit
that referenced
this pull request
Sep 25, 2026
PR #1245 made the per-app connection limit check take its slot atomically, reaps relays whose client stops reading at the idle timeout, and turns a zero-length write into an error. tc-gw-proxy-prot-006 now opens its over-limit connections as one burst and runs the relay tests that need a stalled reader. Signed-off-by: Kevin Wang <wy721@qq.com>
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
1 participant
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.
Four availability defects in the gateway proxy data path, plus a small SNI parser fix.
Problems and fixes
relay_until(splice and adaptive kTLS) awaitedwrite_alloutside theselect!, so a peer that stops reading parked the write and held the connection untiltimeouts.total(5 h). The drain phase already used a watched partial-write loop; it is now awrite_watched!macro used by the main loop too.Pre-fix:
a_client_that_stops_reading_is_reaped_by_the_idle_timeout ... FAILED: the relay outlived its idle window.max_connections_per_appchecked, then incremented later. A burst all passed on the same total: 32 simultaneous connections took 9–20 slots against a limit of 4. The check is nowEnteredCounter::try_enter, afetch_updateon the first candidate's counter, so check and increment are one step. Other candidates' counts are still read outside it, so a multi-instance app can overshoot by at mostconnect_top_n - 1.io_bridgespins. Awrite_bufreturning 0 with a non-empty buffer re-entered at once and counted as progress. It now fails withwrite accepted no bytes, like the relay paths.5 + 2^14, allextract_snican parse) and reports the limit past that.extract_sniignored the record content type, so a non-handshake record could yield an SNI. It now requires0x16.Verification
cargo test -p dstack-gateway(325 passed),cargo clippy -p dstack-gateway --all-features --all-targets -- -D warnings --allow unused_variables,cargo fmt --all -- --check. Each new test fails onnext.Conflicts with #1284 in
gateway/src/proxy.rstests only; see #1284 for the resolution.