fix(tracker): bound client-info OK exchange and fix short-read handling - #291
Merged
Conversation
The post-handshake CLIENTINFO/OK exchange runs on the connection's critical path (sing-box calls ConnHandshakeSuccess before starting the copy goroutines), where dial-layer timeouts no longer apply. Three fixes: - Set a 10s deadline around the exchange in both sendInfo variants, cleared afterward so the long-lived piped conn is unaffected. A server that stalls after the handshake (e.g. under DPI throttling) previously held the flow and its healthy-pool slot for 10+ minutes. - TCP: read the OK response with io.ReadFull; a legal short read previously failed the exchange as "invalid response", fatally tearing down a healthy connection. - UDP: read the response into a datagram-sized buffer and compare the prefix; wrapped conns return io.ErrShortBuffer for a 2-byte buffer whenever the reply carries transport overhead, killing successful QUIC/UDP handshakes (getlantern/engineering#3718). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughClient-info exchanges now use bounded deadlines for TCP and packet connections. TCP reads require the complete ChangesClient-info exchange handling
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens the post-handshake CLIENTINFO → OK exchange on both stream and packet connections to avoid unbounded stalls and to handle short reads/buffer behavior correctly on the critical path immediately after handshake.
Changes:
- Add a bounded exchange deadline (default 10s) around the client-info write +
OKread, clearing the deadline afterward. - Fix TCP
OKhandling by usingio.ReadFullto tolerate short reads. - Fix UDP
OKhandling by reading into a larger buffer and validating only the 2-byte prefix; add targeted tests for the new behaviors.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tracker/clientcontext/injector.go | Bounds the client-info exchange with a deadline and fixes TCP/UDP OK response handling. |
| tracker/clientcontext/injector_test.go | Adds tests covering timeouts, split TCP reads, deadline clearing, and oversized UDP replies. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Formatting the [2]byte array with %s produced a malformed message (%!s([2]uint8=...)). Slice it and use %q so unexpected bytes are readable. Applied to both the TCP and UDP sendInfo paths. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Comment on lines
+276
to
+280
| // wait for `OK` response; the buffer must be able to hold a full datagram — | ||
| // wrapped conns return io.ErrShortBuffer instead of truncating, so a 2-byte | ||
| // buffer would reject any reply carrying transport overhead. | ||
| resp := make([]byte, 512) | ||
| n, _, err := conn.ReadFrom(resp) |
Contributor
Author
There was a problem hiding this comment.
Leaving this one as-is (not a real code path). The OK reply isn't arbitrary — it's a fixed 2-byte control message our own server sends: respBuffer.WriteString("OK") at tracker/clientcontext/manager.go:232. There's no configuration or input that makes it larger, so "the peer sends a larger reply" can't happen here.
The transport framing you mention (VMess headers, uot length prefixes, etc.) is added/stripped by the wrapping conn below this layer — the front/rear headroom reserved at manager.go:227-231 is exactly that lower-layer header space, and it's consumed before the payload reaches our ReadFrom. So the delivered datagram payload is the 2 bytes "OK", and 512 is already a ~250x margin.
A truly universal guarantee against io.ErrShortBuffer for an arbitrary datagram would require a max-UDP-sized (64KB) buffer, which would be wasteful here given the reply is fixed and tiny — the guarantee comes from the protocol contract, not the buffer size. Leaving the thread open in case a maintainer wants the 64KB version anyway.
myleshorton
added a commit
to getlantern/samizdat
that referenced
this pull request
Jul 24, 2026
* fix: make read deadline interrupt an in-flight blocked stream read streamConn.Read checked the deadline once on entry and then blocked in rwc.Read (an HTTP/2 resp.Body read fed by a background frame-reader goroutine). A deadline set before the block only flipped a boolean that nothing rechecked, so SetReadDeadline could not interrupt a stalled read: a server that completed the handshake and then went silent (e.g. under DPI throttling) held the connection — and its slot in the client's healthy-outbound pool — indefinitely. Replace the boolean deadline with a channel-based one and give streamConn a single reader-pump goroutine. Read now selects the pump's output against the deadline and the done channel, so an expired deadline returns a timeout promptly and the timeout stays recoverable (a later SetReadDeadline + Read succeeds). The pump is the sole reader of the stream body, which also lets Close keep draining to EOF rather than aborting with an RST_STREAM — preserving the existing anti-fingerprint close behavior, now centralized in the pump instead of a separate drain goroutine in h2StreamRWC.Close. This is the samizdat side of the client-info OK-read stall; the injector deadline in lantern-box (getlantern/lantern-box#291) is a no-op for samizdat without it because the stream conn's deadline did not propagate. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * address review: single-send terminal read result to avoid pump leak readLoop sent data and a terminal error as two separate channel sends. When a read returned both n>0 and an error (e.g. final read with io.EOF), a caller that consumed the data but never read again or closed would leave the pump blocked forever on the second send. Deliver data and error in one readResult and exit after a single send. Read now holds the terminal error in a sticky readErr, so reads past EOF return the error (matching io.Reader) instead of blocking on the exited pump, and buffered data is fully drained before the error surfaces. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * address review: serialize Read with a dedicated mutex net.Conn permits concurrent method calls, so guard readBuf/readErr with a readMu instead of assuming callers serialize Read. Uncontended on the single-reader relay path; removes the data-race window on concurrent Read. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * address review: pool read buffers, guard rwc.Close, narrow wait() type - Pool the pump's read buffers (sync.Pool) instead of allocating a readChunkSize buffer per read; Read copies out and returns the buffer. The rare short-read tail is copied into readBuf so the pooled buffer can be reused without racing the pump's next read. - Funnel rwc.Close through rwcCloseOnce so the drain force-timer and the post-drain goroutine can't close a generic ReadWriteCloser twice. - Return <-chan struct{} from pipeDeadline.wait (receive-only). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * h2: restore self-contained drain-before-close in h2StreamRWC.Close Close now drains the response body to EOF (bounded by drainForceTimeout) before closing it, and documents why: closing resp.Body with data pending makes net/http2 send RST_STREAM, an abrupt reset a censor could fingerprint, versus a clean END_STREAM. This restores the self-safety the pre-pump code had, so a direct caller of Close no longer risks an RST. To avoid a second reader racing the pump, the force path is split: the streamConn force-timer now calls abort() (raw reader close) to unblock a stalled pump, and only the finalize goroutine — after the pump has exited — runs the graceful drain-and-close via closeRWC. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * address review: close rwc immediately when half-close is unavailable If rwc has no CloseWrite, streamConn.Close can't elicit a clean remote EOF, so draining is pointless and the pump would stay blocked until the force timer fires (delaying resource release and lingering a goroutine). Close rwc right away in that case. The production H2 stream always supports CloseWrite, so the graceful drain path is unchanged; this only affects plain conns (e.g. tests). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * address review: zero-len read, pointer pool, write-after-close - Read returns (0, nil) immediately for a zero-length buffer instead of blocking on the pump/deadline/done (io.Reader/net.Conn convention). - Pool *[]byte rather than []byte so Put doesn't box a slice into an interface and allocate every time (staticcheck SA6002). - Write returns net.ErrClosed once done is closed, so a write after Close fails regardless of whether rwc's own write side was closed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * address review: stop Read promptly once closed Read checked done only in its final select, so after Close it could still return buffered tail bytes or the sticky readErr from the early-return paths. Check done first and return net.ErrClosed (matching Write and net.Conn semantics), and return net.ErrClosed from the select's done case too for consistency. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Pulls in getlantern/samizdat#14, which makes streamConn's read deadline actually interrupt a blocked stream read. Without it, the client-info OK-read deadline added in this PR is a silent no-op for the samizdat transport (its stream conn's deadline was only an entry-time guard). The two changes are the two halves of the same client-info-stall fix. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
myleshorton
added a commit
to getlantern/radiance
that referenced
this pull request
Jul 24, 2026
Pulls in getlantern/lantern-box#291 (client-info OK-read deadline + short-read fixes) and the transitive getlantern/samizdat#14 (read deadline actually interrupts a blocked stream read). Together these stop a proxy that stalls after the handshake — e.g. Rostelecom/DPI throttling in Russia — from holding a flow and its healthy-outbound-pool slot for minutes instead of failing fast. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This was referenced Aug 5, 2026
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.
Problem
The post-handshake
CLIENTINFO/OKexchange intracker/clientcontext/injector.goruns on every connection's critical path: sing-box callsConnHandshakeSuccessafter the upstream dial but before starting the copy goroutines, and dial-layer timeouts no longer apply at that point. Three defects:OK-read has no deadline. A server that stalls after the handshake (e.g. under DPI throttling — the Rostelecom AS9009 behavior in Freshdesk #180563, tracked in getlantern/engineering#3726) holds the flow and its healthy-pool slot for 10+ minutes. That ticket's log showed 300+report handshake success: … reading response: … timed outerrors with durations up to 10m.conn.Readinto a 2-byte buffer can legally return 1 byte, making a validOKfail asinvalid response— which sing-box treats as fatal, tearing down a healthy connection.ReadFrombuffer returnsio.ErrShortBufferon wrapped conns whenever the reply datagram carries transport overhead, killing successful QUIC/UDP handshakes. This is the root cause of getlantern/engineering#3718.sequenceDiagram autonumber participant U as User flow<br/>tun inbound participant CM as ConnectionManager<br/>sing-box-minimal route/conn.go participant WC as writeConn<br/>lantern-box injector.go participant S as Lantern server<br/>DPI-degraded path U->>CM: NewConnection CM->>S: route/conn.go:65<br/>dial outbound — bounded by dial timeout ✅ S-->>CM: handshake complete CM->>WC: route/conn.go:85<br/>ReportConnHandshakeSuccess WC->>S: injector.go:154<br/>write CLIENTINFO json rect rgba(255, 200, 200, 0.3) Note over WC: injector.go:160<br/>conn.Read waits for 2-byte OK<br/>no deadline 🐛 Note over S: DPI blackholes stream after handshake ⚠️ end Note over WC,S: read blocks 10+ minutes<br/>holding a healthy-pool slot WC-->>CM: error only when stall detector kills conn Note over CM: route/conn.go:86-92<br/>hook error is fatal — conn torn down Note over U,CM: route/conn.go:105-106<br/>copy goroutines never started —<br/>user flow never moves a byteLine references:
injector.golines are this repo'smainpre-fix;route/conn.golines aregetlantern/sing-box-minimalat the currently-pinned version.Fix
SetDeadlinein bothsendInfovariants, cleared afterward (the conn becomes the long-lived piped data connection on success). Best-effort: conns without deadline support keep the previous behavior rather than failing.io.ReadFullfor the 2-byteOK.Net effect: a post-handshake blackhole now fails in 10s instead of parking the flow for minutes, so
MutableAutoSelectmoves to the next candidate an order of magnitude faster; and successful UDP/QUIC handshakes are no longer torn down by the brokenOKcheck (partially addresses getlantern/engineering#3718 — the remaining design question there, whether a client-info hook failure should be non-fatal insing-box-minimal route/conn.go:86-92, is intentionally out of scope).Testing
OKacross two writes, TCP stalled-server timeout, TCP deadline-cleared-after-success, UDP oversized reply accepted, UDP stalled-server timeout.go test -race -count=1 ./tracker/...clean;go build ./...,go vet,gofmtclean.🤖 Generated with Claude Code
Summary by CodeRabbit