fix(runtime): clean up sessions on disconnect#17
Open
NianJiuZst wants to merge 4 commits into
Open
Conversation
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
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.
What problem this solves
BrowserSkill keeps a session registry in two processes: the daemon tracks the CLI-facing session id and dispatch queue, while the extension owns the Agent Window, ref store, CDP attachments, and borrowed-tab return metadata.
Before this change, losing the extension WebSocket caused the daemon to purge its side immediately, but the extension only cleared its handshake state. The local
SessionManagerstayed alive. Disabling the connection from the popup had the same behavior. After reconnecting, the CLI no longer knew those session ids, so it could not stop their Agent Windows or return borrowed tabs. Closing an orphan Agent Window could then close a user tab that was still inside it.There was a second race in the daemon's reconnect generation guard. If a new socket registered before the old socket's cleanup completed, the guard correctly preserved the new browser registration, but it also preserved sessions that the extension no longer had. That produced the inverse split: daemon rows with no matching Agent Window.
How this fixes it
This change adopts an explicit safety policy: transport loss terminates local browser sessions instead of attempting to preserve unverifiable in-memory state across the disconnect.
The extension now uses the same teardown path as a normal
session stop:Cleanup is invoked in two lifecycle paths:
The disconnect cleanup is coalesced, so an explicit disconnect event and the transport state callback cannot run two overlapping teardown passes. For unexpected loss, the controller first cancels WSTransport's pending automatic reconnect, performs local cleanup, and only then reconnects.
On the daemon side, a fresh handshake for an existing browser instance now purges any stale sessions and their queues/interrupt markers before replacing the browser registration. The generation guard still protects the new browser connection when the old socket task exits, but stale sessions are no longer carried across generations.
User impact
After disabling BrowserSkill or losing the loopback connection, users no longer have hidden sessions that the CLI cannot manage. Borrowed tabs are returned before Agent Windows close. A reconnect starts from a clean state and the next automation task creates a fresh session id.
This intentionally trades transparent session continuation for a deterministic safety boundary. BrowserSkill session state is currently in-memory and cannot be proven consistent after a disconnect; preserving it was the source of the orphan behavior.
If Chrome refuses to return a borrowed tab, cleanup reports the failure and leaves that Agent Window open instead of risking data loss. The failure is logged for the user to resolve manually.
Validation
cargo fmt --all -- --checkcargo clippy --workspace --all-targets --locked -- -D warningscargo test --workspace --lockedcorepack pnpm --filter @browser-skill/extension testcorepack pnpm --filter @browser-skill/extension compilenode --test scripts/*.test.mjscorepack pnpm exec stylelint "**/*.css"git diff --checkNew regression tests verify safe cleanup ordering, cleanup of multiple sessions, coalescing of overlapping disconnect notifications, intentional-disconnect ordering, unexpected-disconnect cleanup, and daemon-side stale-session removal during a same-instance reconnect.
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.