fix(transport): apply reconnect snapshot before notifying reconnect subscribers - #175
Conversation
|
@blindpirate thanks for the contribution! I'll review this sometime this week and get it into 2.12 |
frenchie4111
left a comment
There was a problem hiding this comment.
Reviewed b47bc74 — fix is correct and the regression test targets the right ordering.
Three minor non-blocking observations:
-
transport-websocket.ts:177-181— ifsendSnapshotRequestorrequest('transport:getClientId')rejects but the socket stays open,connectPromiseholds a rejected promise until something closes the socket. Not reachable with the current server (those handlers either succeed or the socket dies), but a one-linerthis.ws?.close()on rejection would be defense-in-depth. -
transport-websocket.ts:184-192— the message listener is bound beforefinishOpenHandshakeresolves, so state frames arriving in the gap get applied and then wiped bysetSnapshot(no seq check). Theoretical today since the server sendssnapresahead of any catch-up events on that socket; worth a comment so a future server change doesn't reintroduce it. -
transport-websocket.ts:234-250— snapshot and clientId are awaited serially though they're independent.Promise.allfollowed by sequenced callbacks would preserve ordering and halve handshake latency. Cosmetic.
Test at transport-websocket.test.ts:224-284 is correct but uses fixed 50/100/200ms sleeps — may flake on loaded CI. Promise-deferred sequencing would be sturdier, but not worth blocking on.
Comment left on behalf of @frenchie4111 by Claude via Harness.
…ubscribers Sequence the snapshot fetch and clientId re-fetch into a single finishOpenHandshake() so the state mirror is reconciled before reconnect listeners re-fire join-on-connect side effects. Previously a delayed snapshot could land after terminal:join and overwrite the restored controller roster with stale "old client disconnected" state, silently gating PTY input until a new tab was opened.
b47bc74 to
89dc65b
Compare
Disclaimer
I have 0 knowledge about TypeScript. I love Harness app but run into an issue frequently:
I run a harness server in remote server, and use
ssh -Lto forward the port to localhost. Then I use a backend withlocalhost:xxxx. If the SSH process is disconnected and reconnected, the remote session in Harness app becomes unresponsive - I have to restart new Claude/shell sessions to continue.I asked Claude to fix it. After the fix, I built a local version and verified it perfectly fixed the issue. So I think it's valuable and would like it to be included in the next release.
Summary
finishOpenHandshake()which awaitssendSnapshotRequest()→onSnapshot, then re-fetches the (newly minted)clientIdand notifies reconnect listeners — instead of firing both as independent fire-and-forget chains.Why
Previously the snapshot request and
transport:getClientIdraced. A delayed snapshot could land afterterminal:join, overwriting the restored controller roster with the stale "old client disconnected" state. The result: PTY input was silently gated off until the user opened a new tab.Test plan
applies the reconnect snapshot before notifying reconnect subscriberstest exercising a delayed snapshot across an initial connect and a reconnect, asserting strictsnapshot → reconnectordering on both.npm run typechecknpx electron-vite buildnpx vitest run src/renderer/transport-websocket.test.ts(3 passed)