fix(extension): harden websocket reconnect lifecycle - #19
Conversation
|
One blocking issue remains in the handshake retry path: Please preserve the error code (for example, with a typed |
|
Thank you @NianJiuZst — the websocket reconnect lifecycle hardening (handshake race fixes) still applies cleanly to main and covers a gap nobody else picked up. We verified locally that it merges with zero conflicts. Merging now. 🙏 感谢贡献!websocket 重连加固与 main 零冲突,直接合并。 |
|
Closing in favor of #82, which carries these changes rebased onto current main (conflict with the session-disconnect cleanup from #17 resolved) — full credit to @NianJiuZst for the original work. Thank you! 已由 #82 以 rebase 后的形式合并,感谢贡献! |
|
Closing as merged via #82 — your reconnect-lifecycle hardening was rebased onto current main (which had gained the disconnect-cleanup hooks from #17 in the meantime), the two were composed, and it has landed. Thank you @NianJiuZst for the solid work! 🙏 本 PR 的内容已通过 #82(rebase 到最新 main 并与 #17 的断连清理融合)合并进主干,故关闭。感谢贡献! |
What problem this solves
The extension had two related connection-state races that could leave it apparently connected while no usable daemon channel existed.
At the transport layer, an unexpected close scheduled a reconnect timer. If keepalive or the user explicitly called
connect()before that timer fired, WSTransport opened a replacement socket but did not cancel the pending timer. The timer later opened another socket. In addition, every socket callback wrote to shared state without checking whether that socket was still current. A delayedclosefrom the old socket could therefore setthis.socket = nullafter the new socket was already OPEN. The controller still displayed connected, whilesend()failed with "cannot send while not connected"; one of the extra sockets could also remain open after disconnect.At the controller layer, handshake work was guarded by one global
handshakeInFlightboolean. If a socket disconnected while its handshake was pending and a new socket connected, the new connection skipped its mandatory handshake. Conversely, a handshake error or timeout only changed the controller state: it left the physical transport connected, so keepalive saw an OPEN socket and never retried.How this fixes it
Single-current-socket transport
WSTransport now assigns a monotonically increasing generation to every physical socket. Open, message, and close callbacks first verify both the socket object and generation before touching shared state. Events from a replaced socket are ignored.
Explicit
connect()cancels a pending backoff timer before starting a new attempt. Reconnect timers call the same deduplicated connect path, and a failed physical attempt can create a new socket while preserving the original connect promise.disconnect()invalidates the current generation before closing the socket, preventing its synchronous or delayed close callback from restarting the reconnect loop.Per-connection handshake lifecycle
Each connected event now creates its own handshake generation and AbortController. A disconnect or replacement connection aborts the prior handshake immediately and removes its message listener instead of waiting for the ten-second timeout. Results are applied only when they still belong to the current connection generation.
If a current handshake fails, the controller actively disconnects the unusable physical socket and schedules a bounded retry. A protocol-version rejection remains a deliberate terminal disconnect; transient errors no longer leave transport and controller state disagreeing.
User impact
Rapid disable/re-enable actions, keepalive ticks during backoff, delayed browser events, and service-worker reconnects no longer create multiple active daemon sockets. The popup state reflects the socket that
send()will actually use. A fresh connection always performs its own handshake, and a handshake failure recovers instead of staying permanently stuck until the extension is manually toggled.Validation
corepack pnpm --filter @browser-skill/extension testcorepack pnpm --filter @browser-skill/extension compilecorepack pnpm ext:buildnode --test scripts/*.test.mjscorepack pnpm exec stylelint "**/*.css"git diff --checkNew regression coverage verifies:
CI Baseline Compatibility
This branch also carries the one-line Biome 2.4 formatter output for
apps/extension/vitest.config.ts. The same formatting mismatch currently fails the Frontend job onupstream/main; this minimal compatibility commit is intentionally separate from the functional fix and lets this PR run the repository CI suite to completion.CI Reliability Follow-up
GitHub Actions exposed a pre-existing test-only port allocation race: integration helpers probed an ephemeral port, released it, and then asked the daemon to bind the same number, allowing another process to claim it in between. The resulting
Address already in usefailure was unrelated to the functional changes. The PR now lets each daemon bind port0directly and reads the assigned address from its handle, removing the TOCTOU window across all affected integration helpers.