fix(tracker): judge the client-info prefix on a stream, not one Read - #294
Conversation
readInfo decided whether a flow carried client info from a single Conn.Read
into a 32-byte buffer. TCP is a stream, so the 11-byte "CLIENTINFO " prefix can
arrive split across reads -- routine under DPI throttling, where segments are
small and fragmented. When it did, HasPrefix failed and the server classified
real client info as ordinary traffic.
Two consequences, both of which show up in the field. The server never wrote
"OK", so the client blocked on its response read; before the client-side
deadline landed in v0.0.106 that meant blocking until the kernel abandoned the
connection, which in RU Android logs is a 954s median and a 1964s maximum. And
the fall-through replays the consumed bytes upstream, so the destination
received "CLIENTINFO {...}" where it expected a TLS ClientHello and reset the
connection.
Reads until the prefix can actually be judged. Anything shorter, or a read
error, now falls through with the bytes preserved rather than failing: this
hook is telemetry and must not tear down a working flow.
The UDP path is unaffected -- datagram boundaries keep the prefix intact -- and
matching field logs show every hang and reset on the TCP path.
The client half of this exchange already reads its response with io.ReadFull
and has TestStreamSendInfoReadsSplitOK; this is the same treatment for the
server half, which had gone unchanged since #77.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RVgb2MDpZ4wpH6fywKC2hE
|
Warning Review limit reached
Next review available in: 52 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
ChangesClient-info reading
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tracker/clientcontext/manager_readinfo_test.go (1)
39-39: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the
"OK"response.
io.Copydrains any response. The test passes ifreadInfowrites incorrect bytes. Read exactly two bytes after the packet writes, then assert that the response is"OK".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tracker/clientcontext/manager_readinfo_test.go` at line 39, Update the test around readInfo to read exactly two response bytes from client after the packet writes, then assert they equal "OK"; do not use io.Copy, so incorrect readInfo output causes the test to fail.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tracker/clientcontext/manager_readinfo_test.go`:
- Line 39: Update the test around readInfo to read exactly two response bytes
from client after the packet writes, then assert they equal "OK"; do not use
io.Copy, so incorrect readInfo output causes the test to fail.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c83f53cb-70ee-4c1d-8e04-3c5a7932d2b9
📒 Files selected for processing (2)
tracker/clientcontext/manager.gotracker/clientcontext/manager_readinfo_test.go
There was a problem hiding this comment.
Pull request overview
Fixes server-side client-context detection on TCP streams by ensuring the CLIENTINFO prefix is judged across the stream (not a single Read), preventing misclassification under segmented/fragmented delivery (e.g., DPI throttling) and adding regression tests for split-prefix and pass-through behavior.
Changes:
- Update
readConn.readInfoto read enough bytes to reliably decide whether the stream begins with the client-info prefix. - Preserve non-client-info bytes via pass-through behavior when the prefix doesn’t match (or can’t be confidently read).
- Add tests covering split-prefix recognition and byte-identical pass-through for non-client-info traffic.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tracker/clientcontext/manager.go | Adjusts server-side stream reading logic to correctly detect CLIENTINFO prefix across segmented reads. |
| tracker/clientcontext/manager_readinfo_test.go | Adds regression tests for split-prefix detection and pass-through invariants. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| n, err := io.ReadAtLeast(c.Conn, buf[:], len(packetPrefix)) | ||
| if n == 0 { | ||
| c.readErr = err | ||
| c.n = n | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
You caught a real inconsistency, though I fixed it in the comment rather than the behaviour — pushing back on the code change, with the reason below.
The n == 0 branch is deliberate. RoutedConnection compares the two errors by identity:
// manager.go:71-74
info, err := c.readInfo()
if err != c.readErr {
m.logger.Error("failed to read client info ", ...)
}That is how a dead connection is distinguished from malformed client info — only the latter is logged. If readInfo swallowed the zero-byte error and returned (nil, nil), every closed connection would either log a spurious "failed to read client info", or need readErr left unset, which makes the field vestigial and drops the error entirely.
On the substance: with zero bytes there is nothing to pass through, so there is no working flow to preserve — the pass-through guarantee is about not discarding bytes we already consumed. And this matches the pre-change behaviour exactly (c.Conn.Read → if err != nil { c.readErr = err; return nil, err }), so it is not a new hazard. Nothing in this repo sets a read deadline on that conn, so the transient-timeout case needs an upstream deadline to arise, and it would have fired the same way before.
Fixed in 3f7e195:
- the zero-byte branch now states why it returns the error, citing the caller comparison
- the pass-through claim is scoped to "bytes arrived but are short of the prefix", which is what it actually covers
- added
TestReadInfoPropagatesErrorWhenNothingWasRead, asserting the error is returned and stored by identity so the caller's check keeps working
Leaving this thread open for a human to weigh in on the behaviour question, since I did not adopt the suggested change.
Review feedback on #294. The pass-through comment overclaimed: it read as though every error falls through, while the n == 0 branch still returns one. That branch is deliberate. With zero bytes there is nothing to pass through, and RoutedConnection compares the returned error against c.readErr by identity to tell a dead connection from malformed client info, logging only the latter -- swallowing the error there would produce a spurious "failed to read client info" on every closed connection. The comment now says so, and the pass-through claim is scoped to the case where bytes actually arrived. Adds TestReadInfoPropagatesErrorWhenNothingWasRead to pin the contract. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RVgb2MDpZ4wpH6fywKC2hE
|
@coderabbitai full review |
|
Final step of the chain from getlantern/lantern-box#294 (getlantern/engineering#3773). Brings lantern-box v0.0.106 -> v0.0.107 transitively, and radiance up to current main. The server half: readInfo judged the "CLIENTINFO " prefix from a single Read, so a prefix split across reads -- routine under DPI throttling -- was classified as ordinary traffic. The server never sent OK, leaving the client blocked on its response read, and forwarded the prefix bytes to the destination, which reset the connection. RU Android logs show a 954s median stall, 1964s max. The radiance range also carries the removal of the reject-quic rule (#585), the jsDelivr config mirror (#575), and the smart-dialer per-host fix (#579), all of which bear on the same reports. go mod tidy also drops a stale radiance 76fcac0ceebb pair that an earlier bump left behind -- the same untidied-go.sum hazard that shipped lantern-box v0.0.58 in a release on 2026-04-13. Claude-Session: https://claude.ai/code/session_01RVgb2MDpZ4wpH6fywKC2hE Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Answers the open question behind getlantern/engineering#3718, #3772 and the current RU reports: why the server never responds to client info.
The bug
readConn.readInfodecided whether a flow carried client info from a singleConn.Readinto a 32-byte buffer:TCP is a stream. If those 11 bytes don't all land in the first
Read, the check fails and the server treats real client info as ordinary traffic. Under DPI throttling — small, fragmented segments — that is not an edge case.Two field-visible consequences
The client hangs. The server never writes
"OK", so the client blocks on its 2-byte response read. Before the client-side deadline shipped in v0.0.106 that meant blocking until the kernel gave up. In RU Android logs (ticket 180956, 438 occurrences over Aug 1–5):~954s is essentially the Linux
tcp_retries2=15ceiling, i.e. no application deadline at all.The destination gets garbage. The fall-through replays the consumed bytes upstream via
io.MultiReader, so the real destination receivesCLIENTINFO {"deviceID":…}where it expected a TLS ClientHello — and resets. That matches the 156software caused connection aborterrors in the same logs.The fix
Read until the prefix can actually be judged. Anything shorter, or a read error, falls through with the bytes preserved rather than failing — this hook is telemetry and must never tear down a working flow, which is also what #3718 argues for.
Why the UDP path is fine
Datagram boundaries keep the prefix intact, so
readPacketConn.readInfoisn't affected. The field logs agree: every hang and reset is onread tcp; the only UDP failures are a different mode (connection refused).Verification
TestReadInfoRecognizesSplitPrefixdelivers the packet in 1, 5, 10, 11 and 32-byte chunks. With the one-shot read restored, exactly the split cases fail:Two more tests cover the pass-through contract: a peer whose entire opening is shorter than the prefix, and ordinary HTTP traffic. Both must reach the destination byte-identical and must not fail the flow.
Full
./...suite passes.Note on rollout
This is the server half. Bumping lantern-box in the client (v0.0.104 → v0.0.106, which both v9.1.17-beta and v9.1.18-beta still miss) converts the 16-minute hang into a 10s failure and stops one bad flow holding a healthy-pool slot — worth shipping on its own — but it doesn't make the exchange succeed. This change has to reach the proxy fleet for these connections to work.
The client half already reads its response with
io.ReadFulland hasTestStreamSendInfoReadsSplitOK; this is the same treatment for the server half, which had gone untouched since #77.🤖 Generated with Claude Code
https://claude.ai/code/session_01RVgb2MDpZ4wpH6fywKC2hE
Summary by CodeRabbit
Bug Fixes
Tests