GeezerVison is showing github.com — fix(intra): don't discard rwext read/write deadlines when TCP_USER_TIMEOUT is set by varunagarwal-pro · Pull Request #249 · celzero/firestack
Conversation
varunagarwal-pro
commented
Sep 12, 2026
•
edited by coderabbitai
Bot
Loading
Uh oh!
There was an error while loading. Please reload this page.
forward() unwraps the remote conn from its rwext deadline wrapper whenever rwext.SetTimeout() returns didSet=true, treating a successfully-applied low-level sockopt as equivalent to a software deadline.
SetTimeout() only sets TCP_USER_TIMEOUT via core.SetTimeoutSockOpt(). TCP_USER_TIMEOUT bounds how long unacknowledged outbound data may go unacked before the kernel force-closes the connection - it has no effect on a blocking Read() simply waiting for more data from a peer that has gone idle without sending RST/FIN. It is not a receive/idle timeout.
Once unwrapped, the only mechanism that could bound such a Read() - rwext's extendr()/extendw(), which apply real per-call SetReadDeadline/SetWriteDeadline via settings.DialerOpts - is discarded entirely. A relayed TCP connection whose peer silently stops sending (idle CDN/load-balancer connection reuse, or a NAT/middlebox timeout that never surfaces an RST) then blocks forward()'s Read() forever. The socket stays ESTABLISHED with zero rx/tx queue activity indefinitely, with no path to recovery short of killing the connection/process.
Reproduced consistently on-device: a live TCP socket to a video CDN entered this idle-ESTABLISHED state with zero queue bytes and never recovered, while process CPU stayed idle (ruling out a busy loop) and DNS/WAN connectivity stayed healthy - pointing squarely at a stuck blocking Read() in the relay path.
Fix: only unwrap remote from rwext when timeoutsecs <= 0 (i.e. no deadline configured at all, so rwext.Read/Write is a true no-op). When a positive timeout is configured, keep remote wrapped so extendr()/extendw() continue enforcing a real per-call deadline in addition to (not instead of) the TCP_USER_TIMEOUT sockopt optimization.
Verified: go build/go vet clean for linux/arm64 (matches Android's kernel ABI for the syscalls touched).
Summary by CodeRabbit
- Bug Fixes
- Improved connection forwarding when read/write timeouts are configured.
- Operations now respect configured per-call deadlines, preventing stalled connections from blocking indefinitely.
- Preserved optimized connection handling when no timeout is set.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
…MEOUT is set
forward() unwrapped the remote conn from its rwext deadline wrapper
whenever rwext.SetTimeout() reported didSet=true, treating a
successfully-applied low-level sockopt as equivalent to having a
software read/write deadline in place.
SetTimeout() only sets TCP_USER_TIMEOUT via core.SetTimeoutSockOpt().
TCP_USER_TIMEOUT bounds how long unacknowledged outbound data may go
unacked before the kernel force-closes the connection - it has no
effect on a blocking Read() that is simply waiting to receive more
data from a peer that has gone idle without sending RST/FIN. It does
not implement a receive/idle timeout.
Once remote was unwrapped, the only mechanism that could bound such a
Read() - rwext's extendr()/extendw(), which apply Go's real per-call
SetReadDeadline/SetWriteDeadline via settings.DialerOpts - was
discarded entirely. As a result, a relayed TCP connection to a peer
that silently stops sending (common with some CDN/load-balancer
behavior on idle keep-alive connections, or after a NAT/middlebox
timeout that never surfaces an RST) blocks forward()'s Read() forever.
The socket stays visibly ESTABLISHED with zero rx/tx queue activity
indefinitely, and the app-level effect is a permanent hang (e.g., a
media player stuck in a buffering state) with no path to recovery
short of killing the connection/process.
This was reproduced consistently on-device: a live TCP socket to a
video CDN would enter this idle-ESTABLISHED state with zero queue
bytes and never recover, while process CPU stayed idle (ruling out a
busy loop) and DNS/WAN connectivity remained healthy throughout -
pointing squarely at a stuck blocking Read() in the relay path.
Fix: only unwrap remote from rwext when timeoutsecs <= 0, i.e. when
no read/write deadline is configured at all and rwext.Read/Write
would be a true no-op. When a positive timeout is configured, keep
remote wrapped so extendr()/extendw() continue to enforce a genuine
per-call deadline in addition to (not instead of) the TCP_USER_TIMEOUT
sockopt optimization.
coderabbitai
Bot
commented
Sep 12, 2026
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
|
 
No actionable comments were generated in the recent review. 🎉
ℹ️ Recent review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: ccdaf160-af2a-4674-9e87-fe7bc63a1c54
📥 Commits
Reviewing files that changed from the base of the PR and between 8677a52 and 2beeb63.
📒 Files selected for processing (1)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review.
📜 Recent review details
⏰ Context from checks skipped due to timeout. (1)
🔇 Additional comments (1)
intra/common.go (1)
315-325: LGTM!
Also applies to: 328-328
📝 Walkthrough
Walkthrough
The forwarding path now retains the rwext wrapper when timeoutsecs is positive. This preserves per-call read deadlines and prevents a stalled Read() from blocking forward() indefinitely.
Changes
Forwarding timeout handling
| Layer / File(s) |
Summary |
Retain wrappers for timed forwarding
intra/common.go |
baseHandler.forward unwraps the remote connection only when timeoutsecs <= 0. Timed connections retain rwext so extendr and extendw can enforce per-call deadlines. |
Priority: ➖ Normal
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Bug fix · Severity of issue fixed: Medium
Suggested reviewers: ignoramous
Merge Risk: ⚪ Minimal · up to 2beeb
The change preserves deadline enforcement for timed forwarding and is ready to merge after normal checks.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
| Check name |
Status |
Explanation |
| Docstring Coverage |
✅ Passed |
No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1… |
| Linked Issues check |
✅ Passed |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check |
✅ Passed |
Check skipped because no linked issues were found for this pull request. |
| Description Check |
✅ Passed |
Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check |
✅ Passed |
The title clearly and concisely describes the main change: preserving rwext read/write deadline enforcement when TCP_USER_TIMEOUT is set. |
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
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.
❤️ Share
Comment @coderabbitai help to get the list of available commands.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
🔍 OpenCodeReview found 1 issue(s) in this PR.
- ✅ Successfully posted inline: 1 comment(s)
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
![github-actions[bot]](/api/image?u=https%3A%2F%2Favatars.githubusercontent.com%2Fin%2F15368%3Fs%3D60%26v%3D4)
|
|
if r, ok := remote.(rwext); ok {
|
|
|
isrwext = true
|
|
|
if timeoutsecs, didSet = r.SetTimeout(); didSet || timeoutsecs <= 0 {
|
|
|
if timeoutsecs, didSet = r.SetTimeout(); timeoutsecs <= 0 {
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[bug · medium]
Dropping the didSet clause means remote now stays wrapped in rwext for every positive-timeout case (previously TCP with a successfully-set sockopt was unwrapped to *net.TCPConn). This has a side effect beyond the intended deadline fix: rwext embeds net.Conn but does not implement core.DuplexCloser/core.TCPConn (no CloseRead/CloseWrite; see the TODO? var _ core.DuplexCloser = (*rwext)(nil) in rwconn.go). Consequently core.CloseOp(remote, core.CopW/CopR) in upload/download falls through to the io.Closer case and performs a full Close() instead of a TCP half-close, which can abort the opposite direction of the relay (e.g. a client that half-closes after its request body while the server is still sending the response). Suggest making rwext implement CloseRead/CloseWrite (delegating to the underlying conn when it supports it) so per-call deadline extension and graceful half-close can coexist.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
Consolidating into #250, which contains this fix (rwext read/write deadline preservation) together with the proxyFor lock-read timeout fix, for a single combined, easier-to-review PR. Closing this one in favor of #250.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
Milestone
No milestone
Development
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
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.
|