fix(net): never let the reader block on delivery — pong-starvation was the hours-idle disconnect (v1.81.6)#45
Merged
Conversation
…s the hours-idle disconnect (v1.81.6) Root cause of the remaining 'disconnected after hours minimized' reports (v1.81.3-v1.81.5 didn't resolve them): coder/websocket answers the server's WS PING control frames only from inside an in-flight ws.Read (read.go handleControl), and readLoop blocked on the full 256-cap incoming channel once the OS-stalled render loop stopped draining it. At idle-room packet rates the queue took hours to fill; the moment it did, the reader stopped re-entering ws.Read, the server's pings went unanswered, and its ping-timeout (python websockets: 20s/20s defaults) dropped a perfectly healthy client. v1.81.5 moved only the WRITE-side CH keepalive off the render loop — which kept the app-level idle timer satisfied and left WS pong-starvation as the sole kill path. Fix: split delivery off the read goroutine so it always returns to ws.Read. - readLoop now does only a NON-blocking send into a new bounded backlog channel (readBacklogCap=32768 packets, readBacklogMaxBytes=32MiB tracked via atomic backlogBytes) and immediately re-enters ws.Read — pongs flow for the entire stall, however long, whatever the window state. - New deliverLoop goroutine does the blocking backlog -> Incoming hand-off (blocking THERE is fine — it is not the goroutine that must keep reading). Single delivery path preserves FIFO order; on restore the backlog drains to the UI immediately, and the courtroom's bounded IC queue (drop-oldest + packed-room catch-up) collapses the replay, so depth never becomes a replay storm. - readBacklogCap is sized for a BUSY room's full occluded stretch (~0.5 packets/s -> ~18h; idle rooms -> days), because an occluded-but-healthy client is indistinguishable from a wedged one at this layer and must not be torn down inside any plausible stall. ~1.6MiB preallocated per conn. - Overflow of either bound = the drain has been dead for many busy hours with traffic still arriving — torn down deliberately with errReadBacklogOverflow (distinct from transport drops). - Transport drop while stalled: deliberately NO Close() from the read-error path — closing c.closed would make deliverLoop abandon parked packets, and surfacing 'immediately' buys nothing while nobody is watching; on resume pumpConnection drains the parked packets and hits the channel close in the same call. - DialOptions.ReadBacklogCap: test hook, mirroring KeepaliveInterval. - Parked background tabs shared Conn.readLoop, so their identical wedge is fixed for free. Tests: TestReaderKeepsPongingWhileConsumerStalled (server pings must keep succeeding while the consumer stalls past the old 256 limit, then all 400 frames arrive in order — fails on the pre-fix reader), TestReadBacklogOverflowDisconnects (wedged-client contract + sentinel), TestIdleOnlyPingsNeverWedge (pure-idle baseline). Adversarially reviewed (concurrency / behavior / efficacy lenses). Gate: go test -race -count=1 ./internal/protocol/ green (5x repeat); courtroom/assets/network/config/ cache/theme suites green.
.gitignore already lists scheduled_tasks.lock, but the file was committed before the rule existed, and ignore rules don't apply to tracked files — so every session that schedules a task rewrote it and dirtied the tree. Untrack it; the existing ignore rule takes over from here.
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.
The bug
Users left minimized in the background for hours got disconnected on perfectly stable networks and servers. v1.81.3–v1.81.5 didn't resolve it.
Root cause
coder/websocket answers the server's WebSocket PING control frames only from inside an in-flight
ws.Read()(read.gohandleControl).readLoopstopped re-enteringws.Read()the moment its 256-capincomingchannel filled, because it then blocked on the channel send — and that channel is drained only by the render loop, which Windows stalls while the app is minimized behind a fullscreen window (the v1.81.5 finding). At idle-room packet rates the 256 slots take hours to fill — the reported timescale. Once full: no goroutine inws.Read()→ server pings unanswered → the server's ping-timeout (pythonwebsockets: 20 s/20 s defaults) drops a healthy client.v1.81.5 moved only the write-side CH keepalive off the render loop — which kept the server's app-level idle timer satisfied and left WS pong-starvation as the sole kill path.
The fix
Split delivery off the read goroutine so it always returns to
ws.Read():readLoopnow does only a non-blocking send into a new boundedbacklogchannel (readBacklogCap= 32768 packets,readBacklogMaxBytes= 32 MiB via atomic accounting) and immediately re-entersws.Read()— pongs flow for the entire stall, whatever the window state.deliverLoopgoroutine does the blocking backlog →Incominghand-off (blocking there is fine — it isn't the goroutine that must keep reading). Single delivery path preserves FIFO order; on restore the backlog drains immediately, and the courtroom's bounded IC queue (drop-oldest + packed-room catch-up) collapses the replay.readBacklogCapis sized for a busy room's full occluded stretch (~0.5 pkt/s → ~18 h; idle rooms → days), since an occluded-but-healthy client is indistinguishable from a wedged one at this layer. ~1.6 MiB preallocated per connection.errReadBacklogOverflow.Close()from the read-error path — that would abandon parked packets for zero benefit; on resumepumpConnectiondrains the parked packets and surfaces the close in the same call.Conn.readLoop, so their identical wedge is fixed for free.DialOptions.ReadBacklogCap: test hook, mirroringKeepaliveInterval.Tests
TestReaderKeepsPongingWhileConsumerStalled— server WS pings must keep succeeding while the consumer stalls past the old 256 limit, then all 400 frames arrive in FIFO order. Fails on the pre-fix reader.TestReadBacklogOverflowDisconnects— wedged-client contract + sentinel error.TestIdleOnlyPingsNeverWedge— pure-idle baseline.Gate:
go test -race -count=1 ./internal/protocol/green (5× repeat, flake-checked);courtroom/assets/network/config/cache/themesuites green. Root cause and fix were additionally adversarially reviewed from concurrency / behavior / efficacy angles with all findings incorporated.Releasing
Merging this PR does not publish a release by itself — the release pipeline triggers on a
v*tag push. After merging, tag the merge commitv1.81.6and push the tag; the pipeline builds all platforms and publishes with the CHANGELOG's## v1.81.6section as the notes.Generated by Claude Code