Repository navigation
Prevent stale Cloud agent-chat reconnects - #15920
Conversation
|
Warning Review limit reachedNext included review available in 3 minutes. View limit detailsLimit details: You’ve used all 10 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Repository: manaflow-ai/cmux/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
All contributors have signed the CLA ✍️ ✅ |
…anup A review of this branch found two ways the connection helper can strand or retain a view, both of which this now covers. A throw from `createSocket()` was unhandled. The retry timer is the only thing that calls `connect()`, and `connect()` clears `retry` before constructing, so a throw inside the timer left no socket and no pending timer: nothing remained to reconnect. `new WebSocket()` throws on a bad URL and on an opaque origin, and agent-chat renders inside a workspace tab, so the view would sit at `ready` with every send silently returning false and no way back. The throw is caught and re-arms the retry, which is the behaviour the PR already claimed. Cleanup closed the socket without detaching its handlers. They capture the whole `useSession` closure graph, every setState and every ref, and the browser keeps the socket alive until the close handshake finishes, which a server that never answers stretches to a TCP timeout. The identity gates would ignore the callbacks anyway, so nothing was holding that state except the disposed socket. They are nulled before `close()` now. Test changes, all confirmed to fail against the previous source: - The disposed socket's handlers are null after cleanup, and it is closed exactly once across two `disconnect()` calls. - A constructor that throws once publishes no socket, arms a retry, and the retry connects. - The fake timer records delays instead of throwing on an unexpected one, and the 800 ms constant is asserted once at the end. Whoever adds backoff should get a named assertion, not a thrown string from a timer factory. - Dropped two assertions from the first block. `disposed.sockets.length` could not fail there, because `disconnect()` had already cleared the timer, so there was nothing for `flushTimers()` to drain and the comment claiming otherwise was wrong. `disposed.current()` could not fail either, since the close had already pushed null through `onSocket`. Both properties are genuinely covered by the queued-retry and mounted blocks. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Review subagent on Review. It verified the core claim properly rather than reading the diff. The single-socket and single-timer invariant does hold: It then found two ways to strand or retain a view, and I agree with both. Fixed. A throw from Cleanup closed the socket without detaching its handlers. They capture the whole It also went through the new test block by block and told me which assertions had no power, which is what I asked for. Two in the first block could not fail: Three new or strengthened assertions, each confirmed to fail against the previous source and pass now. Left, deliberately, both pre-existing and unchanged by this diff.
There is also no backoff or jitter, so a down server is polled at 1.25 Hz forever by every open tab. Pre-existing, and not something to slip into a correctness fix. Fixes skip team review, so this merges on green. — Raindrop g2 🫧 / Run: run_worker_20260930_3fc64ba6 |
|
Merge receipt for |
5eda931 fix(ios): keep auth operations from missing token store (manaflow-ai#14302) 4d9bec3 fix: restore per-label Blacksmith macOS capacity 572beb6 fix: preserve non-transient cooldown reasons (manaflow-ai#15885) 747aa96 Prevent stale Cloud agent-chat reconnects (manaflow-ai#15920) 33ad0b1 Hibernate only agents whose wake can relaunch the original launcher (manaflow-ai#15287)
1 participant
Cloud agent chat could leave a reconnect timer or stale WebSocket alive after the view was disposed. A delayed retry could reopen the chat after teardown, and callbacks from an older socket could overwrite the current connection's history or state.
The connection lifecycle is now owned by a small helper that cancels retries during cleanup, clears the active socket as soon as it closes, and gates open/message/close callbacks to the current mounted socket. The regression covers disposal with a queued retry, replacement sockets, stale callbacks, duplicate close events, and idempotent cleanup.
Changelog
Validation:
bun run checkfromagent-chatbun test/connection.test.tsgit diff --checkNeed help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Prevents stale Cloud agent-chat reconnects from resurrecting or replacing a session after the view is disposed. A delayed retry could reopen the chat after teardown, and callbacks from an older socket could overwrite the current connection's history or state.
The connection lifecycle now lives in a small helper that cancels retries during cleanup, clears the active socket as soon as it closes, and gates open/message/close callbacks to the current mounted socket. It also re-arms the retry when the socket constructor throws (avoiding a stranded view with no socket and no pending timer) and detaches handlers before closing, so a disposed view isn't retained until the close handshake finishes. A regression test covers disposal with a queued retry, replacement sockets, stale callbacks, duplicate close events, constructor throws, handler detachment, and idempotent cleanup.
Written for commit fe10853. Summary will update on new commits.