From 1527a9af55641c86a91221ebfca28e1e13b34bfe Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 24 Jul 2026 15:53:57 +0000 Subject: [PATCH 1/2] =?UTF-8?q?fix(net):=20never=20let=20the=20reader=20bl?= =?UTF-8?q?ock=20on=20delivery=20=E2=80=94=20pong-starvation=20was=20the?= =?UTF-8?q?=20hours-idle=20disconnect=20(v1.81.6)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- internal/protocol/conn.go | 132 ++++++++++++++- internal/protocol/conn_backlog_test.go | 220 +++++++++++++++++++++++++ internal/ui/assets/CHANGELOG.md | 16 ++ 3 files changed, 359 insertions(+), 9 deletions(-) create mode 100644 internal/protocol/conn_backlog_test.go diff --git a/internal/protocol/conn.go b/internal/protocol/conn.go index 87f24f9..d01f1d6 100644 --- a/internal/protocol/conn.go +++ b/internal/protocol/conn.go @@ -3,6 +3,7 @@ package protocol import ( "context" "crypto/tls" + "errors" "fmt" "net/http" "sync" @@ -24,9 +25,34 @@ const ( // occluded by a fullscreen foreground app, which used to freeze the old // loop-driven ping and let the server idle-drop the "silent" client. keepaliveInterval = 45 * time.Second - // incomingQueueCap bounds the read-loop → client channel; the game + // incomingQueueCap bounds the deliver-loop → client channel; the game // loop drains it every frame. incomingQueueCap = 256 + // readBacklogCap bounds the read-loop → deliver-loop parking channel that sits + // in FRONT of Incoming. It exists so readLoop NEVER blocks on a full Incoming: + // coder/websocket answers the server's WS PING control frames only while some + // goroutine is inside ws.Read (read.go handleControl), so a reader parked on a + // channel send stops ponging — and once Incoming filled (hours at idle-room + // packet rates, ~256 × 60 s) the server's ping-timeout dropped a perfectly + // healthy minimized client. Windows provably stalls the render loop that drains + // Incoming while the app is occluded (the v1.81.5 finding), so the socket must + // stay alive without it — for the WHOLE occlusion, which the reporting users + // measure in workdays. Sized for a BUSY room's full occluded stretch, not just + // an idle one (~0.5 packets/s of active courtroom chatter → ~18 h; idle rooms → + // days), because an occluded-but-healthy client is indistinguishable from a + // wedged one down here and must not be torn down inside any plausible stall. + // Costs ~1.6 MiB of preallocated channel buffer per connection (48 B/slot) — + // noise against the 256 MiB budget, and the courtroom's own bounded IC queue + // (messageQueueCap drop-oldest + packed-room catch-up) collapses the replay on + // restore, so depth here never turns into a replay storm. Still a hard, named + // bound: exhaustion means the drain has been dead for many busy hours with + // traffic still arriving, and THEN we disconnect (errReadBacklogOverflow). + readBacklogCap = 32768 + // readBacklogMaxBytes bounds the WIRE bytes parked in the backlog channel, so a + // hostile/buggy server spraying maximum-size frames (maxIncomingBytes each) + // can't turn the packet-count bound into gigabytes of buffered strings. Idle + // traffic is tens of bytes per packet, so real stalls never approach this. + readBacklogMaxBytes = 32 << 20 // maxIncomingBytes bounds one server packet defensively (character // lists on 200-char servers fit comfortably). maxIncomingBytes = 8 << 20 @@ -39,6 +65,19 @@ const ( ClientName = "AsyncAO" ) +// errReadBacklogOverflow is the terminal error when the app has not drained +// Incoming for an entire readBacklogCap of arriving packets (or the backlog's +// byte bound): the render thread is genuinely wedged, not merely napping, so +// the connection is abandoned deliberately rather than buffered without bound. +var errReadBacklogOverflow = errors.New("protocol: incoming packet backlog overflowed (client stopped draining)") + +// sizedPacket carries a parsed packet plus its wire size through the backlog +// channel, so the byte bound can be released when the packet is handed on. +type sizedPacket struct { + p Packet + n int +} + // Conn is a WebSocket AO2 connection: one text frame in = one packet out the // Incoming channel; Send serializes and writes one packet. Legacy raw TCP is // not supported anywhere in AsyncAO. @@ -49,6 +88,15 @@ type Conn struct { closed chan struct{} once sync.Once + // backlog decouples readLoop from Incoming (see readBacklogCap): readLoop is + // its only sender/closer and never blocks on it; deliverLoop moves packets on + // to Incoming with a blocking send, which is safe because deliverLoop is NOT + // the goroutine that must keep re-entering ws.Read to answer server pings. + backlog chan sizedPacket + // backlogBytes tracks wire bytes currently parked in backlog (single adder: + // readLoop; single subtractor: deliverLoop) against readBacklogMaxBytes. + backlogBytes atomic.Int64 + // writeMu serializes every ws.Write: application Sends (render thread) and the // keepalive goroutine share the one underlying socket, and coder/websocket // permits only a single writer at a time (concurrent reads are fine, which is @@ -110,6 +158,11 @@ type DialOptions struct { // keepaliveInterval package default). Tests set a short value to exercise the // goroutine without waiting 45 s; production leaves it zero. KeepaliveInterval time.Duration + // ReadBacklogCap overrides the backlog channel's packet capacity (0 = the + // readBacklogCap package default). Tests set a tiny value to exercise the + // wedged-client overflow disconnect without buffering thousands of packets; + // production leaves it zero. + ReadBacklogCap int } // Dial connects to a ws:// or wss:// AO server URL and starts the read loop. @@ -145,15 +198,21 @@ func Dial(ctx context.Context, wsURL string, opts ...DialOptions) (*Conn, error) } ws.SetReadLimit(maxIncomingBytes) + backlogCap := readBacklogCap + if len(opts) > 0 && opts[0].ReadBacklogCap > 0 { + backlogCap = opts[0].ReadBacklogCap + } c := &Conn{ ws: ws, incoming: make(chan Packet, incomingQueueCap), + backlog: make(chan sizedPacket, backlogCap), closed: make(chan struct{}), } if len(opts) > 0 && opts[0].KeepaliveInterval > 0 { c.keepaliveEvery = opts[0].KeepaliveInterval } go c.readLoop() + go c.deliverLoop() // backlog → Incoming hand-off; exits when backlog closes or on Close (§17.4) go c.keepaliveLoop() // fires the CH ping off the render loop; exits on Close (no per-conn leak, §17.4) return c, nil } @@ -253,13 +312,30 @@ func (c *Conn) Close() { }) } +// readLoop's ONE invariant is that it always returns to ws.Read promptly: the +// library answers server WS PING control frames only from inside an in-flight +// Read, and an unanswered ping is how servers drop a healthy idle client (the +// hours-minimized disconnect — the render loop that drains Incoming is stalled +// by the OS while occluded, so this goroutine is the only thing keeping the +// socket alive). It therefore NEVER blocks on a channel send: packets go into +// backlog non-blockingly, and deliverLoop does the blocking hand-off to +// Incoming. Backlog exhaustion (count or bytes) is the deliberate exception: +// the drain has been dead for the whole bound with traffic still arriving, so +// the client is wedged and disconnecting is correct. func (c *Conn) readLoop() { - // The close wake matters too: a dropped connection must surface (the - // "connection closed" toast + tab teardown) as fast as a packet would. - defer func() { - close(c.incoming) - c.wake() - }() + // readLoop is backlog's only sender, so it closes it on exit; deliverLoop + // then finishes delivering whatever is parked and closes Incoming — a + // dropped connection still surfaces (the "connection closed" toast + tab + // teardown) right after the buffered packets. Deliberately NO Close() on + // the transport-error path below: closing c.closed would make deliverLoop + // abandon parked packets, and surfacing the drop "immediately" buys + // nothing while the drain is stalled (nobody is watching a stalled + // window; on resume pumpConnection drains the parked packets AND hits the + // channel close in the same call, so the drop surfaces the same frame). + // Until then deliverLoop just stays parked — bounded, one per conn, and + // released by the resume, by keepaliveLoop's write-failure Close, or by + // the app's own teardown Close. + defer close(c.backlog) for { msgType, data, err := c.ws.Read(context.Background()) if err != nil { @@ -279,11 +355,49 @@ func (c *Conn) readLoop() { continue // tolerate malformed frames like AO2-Client does } c.received.Add(1) + if c.backlogBytes.Load()+int64(len(data)) > readBacklogMaxBytes { + c.readErr.CompareAndSwap(nil, &errReadBacklogOverflow) + c.Close() + return + } select { - case c.incoming <- packet: - c.wake() + case c.backlog <- sizedPacket{p: packet, n: len(data)}: + c.backlogBytes.Add(int64(len(data))) case <-c.closed: return + default: + // Backlog full: the app hasn't drained Incoming for the entire + // bound while packets kept arriving — wedged, not napping. + c.readErr.CompareAndSwap(nil, &errReadBacklogOverflow) + c.Close() + return + } + } +} + +// deliverLoop moves packets backlog → Incoming. Its send may block for hours — +// that's the point: blocking HERE (instead of in readLoop) is what keeps the +// reader inside ws.Read answering server pings while the render loop is +// stalled. When the drain resumes, delivery resumes instantly (no waiting for +// the next inbound frame). It is Incoming's only closer. On a transport drop +// (readLoop exits WITHOUT Close, so c.closed stays open) the select's closed +// arm is dead and every parked packet is delivered before the range ends — +// nothing is dropped ahead of the close notification. Once c.closed IS closed +// (deliberate Close, overflow, or keepaliveLoop's write-failure Close on an +// already-dead socket) still-parked packets may be abandoned — acceptable, the +// connection is being torn down and a reconnect resyncs full server state. +func (c *Conn) deliverLoop() { + defer func() { + close(c.incoming) + c.wake() // the close wake: a drop must surface as fast as a packet would + }() + for item := range c.backlog { + select { + case c.incoming <- item.p: + c.backlogBytes.Add(-int64(item.n)) + c.wake() + case <-c.closed: + return // deliberate Close / overflow teardown: stop delivering } } } diff --git a/internal/protocol/conn_backlog_test.go b/internal/protocol/conn_backlog_test.go new file mode 100644 index 0000000..d8059af --- /dev/null +++ b/internal/protocol/conn_backlog_test.go @@ -0,0 +1,220 @@ +package protocol + +import ( + "context" + "errors" + "fmt" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/coder/websocket" +) + +// TestReaderKeepsPongingWhileConsumerStalled is the regression test for the +// hours-minimized disconnect (v1.81.6): with the render loop stalled (the +// client NEVER drains Incoming), the server fills the old 256-slot queue and +// beyond, then WS-pings. Before the fix readLoop was parked on the full +// channel send, no goroutine sat in ws.Read, coder/websocket never answered +// the pings, and the server's ping-timeout killed the healthy link. With the +// backlog + deliverLoop split the reader always returns to ws.Read, so every +// ping must succeed — and once draining resumes, every packet must arrive in +// order with none lost. +func TestReaderKeepsPongingWhileConsumerStalled(t *testing.T) { + const frames = 400 // > incomingQueueCap(256): guarantees the old code wedged here + + pingErrs := make(chan error, 1) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + ws, err := websocket.Accept(w, r, nil) + if err != nil { + return + } + ctx := context.Background() + _ = ws.Write(ctx, websocket.MessageText, []byte("decryptor#34#%")) + // The server must keep reading to process the client's pong control + // frames (same rule the client lives by). + go func() { + for { + if _, _, err := ws.Read(ctx); err != nil { + return + } + } + }() + for i := 0; i < frames; i++ { + if err := ws.Write(ctx, websocket.MessageText, []byte(fmt.Sprintf("SEQ#%d#%%", i))); err != nil { + pingErrs <- fmt.Errorf("frame %d write: %w", i, err) + return + } + } + // All frames are in flight and the client is not draining. These pings + // only succeed if the client's read goroutine is still inside ws.Read. + for i := 0; i < 3; i++ { + pctx, cancel := context.WithTimeout(ctx, 2*time.Second) + err := ws.Ping(pctx) + cancel() + if err != nil { + pingErrs <- fmt.Errorf("ping %d: %w", i, err) + return + } + } + pingErrs <- nil + // Hold the socket open until the test finishes draining. + _, _, _ = ws.Read(ctx) + })) + t.Cleanup(srv.Close) + + conn, err := Dial(context.Background(), wsURL(srv)) + if err != nil { + t.Fatalf("Dial: %v", err) + } + defer conn.Close() + + select { + case p := <-conn.Incoming(): + if p.Header != "decryptor" { + t.Fatalf("greeting = %+v", p) + } + case <-time.After(5 * time.Second): + t.Fatal("no greeting") + } + + // Stalled render loop: do not touch Incoming until the server reports. + select { + case err := <-pingErrs: + if err != nil { + t.Fatalf("server-side failure while client stalled (reader must keep ponging): %v", err) + } + case <-time.After(10 * time.Second): + t.Fatal("server never finished its ping round") + } + if err := conn.Err(); err != nil { + t.Fatalf("healthy stalled client must not error, got %v", err) + } + + // Render loop "resumes": every frame arrives, in order, none lost. + deadline := time.After(10 * time.Second) + for i := 0; i < frames; i++ { + select { + case p, ok := <-conn.Incoming(): + if !ok { + t.Fatalf("Incoming closed after %d/%d packets: %v", i, frames, conn.Err()) + } + if p.Header != "SEQ" || p.Field(0) != fmt.Sprint(i) { + t.Fatalf("packet %d = %+v (FIFO order must survive the backlog)", i, p) + } + case <-deadline: + t.Fatalf("drained only %d/%d packets after resume", i, frames) + } + } +} + +// TestReadBacklogOverflowDisconnects pins the wedged-client contract: when the +// app stops draining for the ENTIRE backlog bound while packets keep arriving, +// the connection is torn down deliberately with errReadBacklogOverflow instead +// of buffering without bound. +func TestReadBacklogOverflowDisconnects(t *testing.T) { + // incomingQueueCap(256) + tiny backlog(8) + the one packet deliverLoop may + // hold in hand: 300 frames overflow it with margin. + const frames = 300 + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + ws, err := websocket.Accept(w, r, nil) + if err != nil { + return + } + ctx := context.Background() + _ = ws.Write(ctx, websocket.MessageText, []byte("decryptor#34#%")) + for i := 0; i < frames; i++ { + if err := ws.Write(ctx, websocket.MessageText, []byte(fmt.Sprintf("SEQ#%d#%%", i))); err != nil { + return // client already tore down — expected + } + } + _, _, _ = ws.Read(ctx) // hold until the client closes + })) + t.Cleanup(srv.Close) + + conn, err := Dial(context.Background(), wsURL(srv), DialOptions{ReadBacklogCap: 8}) + if err != nil { + t.Fatalf("Dial: %v", err) + } + defer conn.Close() + + // Never drain while the flood is in flight — touching Incoming here would + // relieve the very backpressure under test. Wait for the overflow verdict. + deadline := time.Now().Add(10 * time.Second) + for conn.Err() == nil { + if time.Now().After(deadline) { + t.Fatal("overflow never tripped: Err() still nil with the client not draining") + } + time.Sleep(10 * time.Millisecond) + } + if err := conn.Err(); !errors.Is(err, errReadBacklogOverflow) { + t.Fatalf("Err() = %v, want errReadBacklogOverflow", err) + } + // The teardown must also close Incoming (after any still-buffered packets). + for { + select { + case _, ok := <-conn.Incoming(): + if !ok { + return + } + case <-time.After(5 * time.Second): + t.Fatal("overflowed connection never closed Incoming") + } + } +} + +// TestIdleOnlyPingsNeverWedge pins the pure-idle baseline: with no data frames +// at all (only server WS pings) the reader stays inside ws.Read and answers +// every ping — the path that was already correct before the fix must stay so. +func TestIdleOnlyPingsNeverWedge(t *testing.T) { + pingErrs := make(chan error, 1) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + ws, err := websocket.Accept(w, r, nil) + if err != nil { + return + } + ctx := context.Background() + _ = ws.Write(ctx, websocket.MessageText, []byte("decryptor#34#%")) + go func() { + for { + if _, _, err := ws.Read(ctx); err != nil { + return + } + } + }() + for i := 0; i < 5; i++ { + pctx, cancel := context.WithTimeout(ctx, 2*time.Second) + err := ws.Ping(pctx) + cancel() + if err != nil { + pingErrs <- fmt.Errorf("ping %d: %w", i, err) + return + } + time.Sleep(10 * time.Millisecond) + } + pingErrs <- nil + _, _, _ = ws.Read(ctx) + })) + t.Cleanup(srv.Close) + + conn, err := Dial(context.Background(), wsURL(srv)) + if err != nil { + t.Fatalf("Dial: %v", err) + } + defer conn.Close() + + // Never drain anything — even the greeting sits buffered. + select { + case err := <-pingErrs: + if err != nil { + t.Fatalf("idle pings must all be answered: %v", err) + } + case <-time.After(10 * time.Second): + t.Fatal("server never finished its ping round") + } + if err := conn.Err(); err != nil { + t.Errorf("idle client must not error, got %v", err) + } +} diff --git a/internal/ui/assets/CHANGELOG.md b/internal/ui/assets/CHANGELOG.md index 8296724..bc7df16 100644 --- a/internal/ui/assets/CHANGELOG.md +++ b/internal/ui/assets/CHANGELOG.md @@ -4,6 +4,22 @@ What changed, newest first. The "What's New" screen renders this embedded file, so every build ships its own history offline. The version you're running is tagged "installed" below. +## v1.81.6 — 2026-07-24 + +- **Fixed the last "disconnected after hours minimized" bug.** v1.81.5 kept the + outgoing keepalive alive while minimized, but the *incoming* side still ran + through the drawing loop that Windows stalls behind a fullscreen window. Over + hours, undelivered server messages slowly filled an internal queue; once it was + full the client stopped reading the connection at all — which also silently + stopped answering the server's low-level "are you alive?" checks — and the + server eventually dropped a perfectly healthy client. The connection now keeps + reading (and answering those checks) no matter what the window is doing: + messages that arrive while you're away are buffered and caught up the moment you + come back. You can stay minimized overnight — behind a fullscreen game or video — + and remain connected: the buffer covers days of a quiet room and a large part of + a day of nonstop busy chat before the client gives up deliberately. (Applies to + parked server tabs too, which shared the same flaw.) + ## v1.81.5 — 2026-07-22 - **Minimizing no longer eventually disconnects you.** The keepalive ping that From c212dccd81b88fe08271e95bfa75d5870c9d32d5 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 24 Jul 2026 16:03:51 +0000 Subject: [PATCH 2/2] chore: untrack .claude/scheduled_tasks.lock (runtime session state) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit .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. --- .claude/scheduled_tasks.lock | 1 - 1 file changed, 1 deletion(-) delete mode 100644 .claude/scheduled_tasks.lock diff --git a/.claude/scheduled_tasks.lock b/.claude/scheduled_tasks.lock deleted file mode 100644 index b70e38c..0000000 --- a/.claude/scheduled_tasks.lock +++ /dev/null @@ -1 +0,0 @@ -{"sessionId":"d306c858-8b7a-4413-ad67-361a9365c5e6","pid":3480,"procStart":"639183173134083060","acquiredAt":1782741639294} \ No newline at end of file