fix(ws): WebSocket durable-delivery resilience — reconnect, per-client scoping, ACK re-drive#149
Open
aakhter wants to merge 6 commits into
Open
fix(ws): WebSocket durable-delivery resilience — reconnect, per-client scoping, ACK re-drive#149aakhter wants to merge 6 commits into
aakhter wants to merge 6 commits into
Conversation
The upstream v1.1.15 merge spliced upstream's transport-object indicator body onto local's _connectionStatus-based _updateConnectionIndicator() without defining `transport`, so every transport.* reference threw ReferenceError on any queued state. That hid the "WS" status and, because _reliableSend() updates the indicator before _drainSession(), made every keystroke skip immediate delivery (input flushed only on the 2s sweep = typing lag). - Rewrite _updateConnectionIndicator() to show the terminal WebSocket transport from _wsState (WS / HTTP / WS… / Offline), falling back to the SSE _connectionStatus only on the idle dashboard. - Only annotate a backlog (· N queued) above 4 bytes so normal typing no longer flickers "sending 1B" on each key press. - test/connection-indicator.test.ts (new): transport labels, the >4B threshold, an exhaustive never-throws guard for the ReferenceError, and the _reliableSend -> _drainSession invariant (typing-lag guard). - test/input-send-order.test.ts: reconcile to local's durable input layer (the prior coalescing-fallback tests had been failing since 1255e28).
…ce + logging Root cause of the WS->HTTP->WS flapping: the v1.1.15 input-delivery merge left a call to the now-undefined _flushHttpFallbackQueuesViaWs() in ws.onopen, so every (re)connect threw a TypeError BEFORE _onWsReady() ran -- durable input was never re-flushed over the fresh socket, the 2s redeliver sweep then saw stale unacked frames and force-closed the socket, reconnect, throw again: a self-sustaining flap loop. Remove the dead call (_onWsReady, 10 lines below, is its replacement). Resilience + observability: - Pure CodemanWsReconnect.plan(code, attempt) (constants.js, TDD, 6 tests): <4004 -> fast reconnect (immediate jittered first retry, faster backoff); 4008/unknown->=4004 -> bounded retry-fallback (HTTP no longer sticks until a tab switch); 4004/4009 -> give up (session gone). Wired into onclose. - Redeliver sweep force-closes only a SILENT socket (no recent recv), not one actively delivering output/ACKs -- stops self-inflicted flaps while typing. - Client logs WS close code/reason to crash-diag; server logs [ws] open/close/terminate/4008 (console -> journald; Fastify runs logger:false). Verified: 6/6 unit, tsc 0, frontend-syntax + prettier clean, build; beta WS reaches connected with zero console errors (onopen TypeError gone), _wsLastRecvAt tracked, server [ws] lines emit.
… gap)
A reliable-input frame could be stranded forever if its server ACK
({t:'ia',seq}) was lost while the WebSocket kept delivering other output.
_drainSession's WS fast path skips records with sentAt!==0, and after
COD-134 the sweep only force-closes a *silent* socket -- so a lost ACK on
an otherwise-live socket (stale && !silent) was never re-sent.
_redeliverSweep now, for an active-WS session whose oldest unacked frame
is stale but the socket is NOT silent, resets sentAt=0 on every stale
unacked frame and lets the existing _drainSession re-drive them over the
live socket (server dedups by seq). The stale && silent force-close
remains the fallback for a genuinely half-open socket. Restores the
exactly-once recovery guarantee without reintroducing the flap.
Tests: new failing-first COD-135 cases in test/input-send-order.test.ts
(re-drive on live socket; leave not-yet-stale alone; keep stale+silent
force-close). 18/18 across input-send-order + reliable-input-dedup +
ws-reconnect-plan; tsc 0, frontend-syntax, build all clean.
…reconnect) MAX_WS_PER_SESSION was gated by a bare Map<sessionId,number> counter, incremented on upgrade and decremented only on the old socket's async close. A client that dropped and immediately reconnected could land its new upgrade before the old socket's close fired, briefly over-counting and tripping a spurious 4008 (-> HTTP fallback). The limit also counted raw sockets, so a reconnecting client consumed a new slot instead of its own. Replace the counter with WsConnectionRegistry (new pure, unit-tested module) that tracks live sockets per session keyed by clientId. A same-cid upgrade SUPERSEDES its own socket (evicts the stale one with close 4010, reuses the slot, no net count change) -> a reconnect can never be rejected by the cap. The reliable-input protocol (shouldApplyInput(cid,seq)) already assumes one logical client per cid per session, so same-cid eviction is principled, not a regression of multi-tab (which already collides on seq). Slots are freed EAGERLY on error/terminate, not just async close; close is identity-matched so a superseded socket's late close is a no-op. cid-less upgrades are admitted anonymously up to the cap and never evict (backward-compat). Client sends cid on the WS upgrade URL (?cid=, encoded, omitted if absent). Tests: ws-connection-registry.test.ts (reconnect-reclaim at cap, rejects N+1th distinct, eager-terminate frees slot, cid-less up-to-limit + no-evict, late-close-no-evict, per-session isolation) + route integration in ws-routes.test.ts (real upgrade through the cap). 45/45 across registry + ws-routes + input-send-order + ws-reconnect-plan; tsc 0, build, prettier, frontend-syntax clean.
_updateConnectionIndicator() ran on every keystroke (_reliableSend) and
every ACK (_ackDelivery), unconditionally writing display/className/
textContent/title. During fast typing the rendered output is usually
identical between calls, so those were wasted main-thread DOM writes.
Extracted the branch logic into a pure DOM-free _computeConnectionDescriptor()
returning { display, dotClass, text, title } (every branch/string preserved
verbatim; hidden state normalizes the three non-display fields to '' so the
compare is well-defined). _updateConnectionIndicator() now computes the
descriptor, compares all four fields against a cached _lastIndicatorDescriptor,
and early-returns when unchanged — otherwise caches and writes the DOM exactly
as before (display always; dotClass/text/title only when shown). First call
renders (cache starts null). Perf only, no behavior change.
Tests: test/connection-indicator.test.ts — 9 descriptor cases pinning the
exact strings per state + 4 skip cases (first call writes; two identical calls
write DOM once via counting setters; state change and hidden->shown re-render).
31/31 with input-send-order regression; build, frontend-syntax, prettier clean.
…int immediately
A freshly created shell session rendered blank until a tab-switch. selectSession()
fetches the terminal buffer, but for a just-started shell that fetch resolves before
the PTY emits its prompt, so the buffer is empty; the prompt then arrives as a live
SSE event queued during the load and _finishBufferLoad() discarded it. The discard is
correct for an established session (its fetched buffer already contains that output),
but harmful when the load painted nothing.
_finishBufferLoad(owner, { flushQueued }) now REPLAYS the queued events through
batchTerminalWrite (after _isLoadingBuffer is cleared, so they write through, not
re-queue) instead of discarding. selectSession passes flushQueued only in the empty
branch (no fresh buffer + no cache), so the established-session de-dup path is
unchanged. TDD: test/terminal-buffer-flush.test.ts exercises the real begin/finish
mixin (vm-harness, no jsdom).
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
1 participant
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.
Summary
Six bug-fix commits hardening the WebSocket layer against reconnect races, spurious disconnects, stale ACKs, and redundant DOM work. Each is self-contained and builds cleanly on the previous.
COD-133 — Fix header WS status indicator + typing lag
Restores the connection-state chip (broken since v1.1.15 merge) and removes a
keydownhandler that was blocking the UI thread on every keystroke.COD-134 — Fix WS flap loop + reconnect resilience
undefined onopen callcrash on rapid connect/disconnect cycles that caused an infinite flap loop._wsLastRecvAt/_wsStatetracking so reconnect back-off is state-machine driven rather than timer-soup.CodemanWsReconnect/planWsReconnectfromconstants.jsfor testability.COD-135 — Re-drive lost input ACK on a live WebSocket
When an input frame is sent but the ACK never arrives (connection drop between send and response), the client now re-drives the pending frame on the next live connection rather than silently dropping it. Closes the durable-delivery gap without requiring a page reload.
COD-137 — Scope WS per-session limit by
clientIdUpstream's per-session connection cap (max 1 WS per session) was keyed on
sessionIdalone, causing spurious4008closes when a client reconnected before the server had cleaned up the old socket. NewWsConnectionRegistry(src/web/ws-connection-registry.ts) scopes the limit by(sessionId, clientId)pair so reconnects are never rejected.COD-136 — Skip redundant connection-indicator DOM writes on hot input path
_updateConnectionIndicator()was called on every received message even when the indicator state hadn't changed. Added a dirty-check; saves severalclassListmutations per second on busy sessions.COD-144 — Flush queued SSE output on empty buffer-load
When a new shell session starts with an empty terminal buffer, the SSE flush was gated behind a non-empty buffer check, causing the first few output lines to be silently dropped until the next poll cycle. Removed the gate; queued output now flushes immediately on an empty-buffer load.
Test plan
test/connection-indicator.test.ts(new, 24 tests)test/input-send-order.test.ts(new, 7 tests)test/ws-reconnect-plan.test.ts(new, 6 tests)test/ws-connection-registry.test.ts(new, 6 tests)test/routes/ws-routes.test.ts(existing + 23 additive tests = 26 total)test/terminal-buffer-flush.test.ts(new, 5 tests)tsc --noEmit: 0 errorsnpm run build: clean