Session close probes the browser instead of stalling behind it - #333
Merged
Conversation
closeSession queued behind any in-flight recording command, so a wedged command against a dead browser held teardown for the full command timeout (#316). The issue's proposed fix — abort all pending commands first — would have broken auto-finalize-on-close, which needs the connection to deliver an active recording. Probe instead: when the browser's state is unknown, one cheap getTree bounded to two seconds decides the path. Alive → finalize over the connection, then tear down, exactly as before. Dead → abort pending commands first (the mutex frees immediately) and finalize offline: package the trace from memory and embed the engine's live-muxed video when it is readable and non-empty, with the manifest recording why the video is cut short. When the routing goroutine triggers the close it already watched the connection die, so it skips the probe entirely. Measured on a SIGKILLed Firefox mid-recording: close went from 60.1s to 41ms, and the recording delivers instead of vanishing. The 60s was mostly a second bug this surfaced: a client command racing session deletion was dropped with no reply, leaving the client to wait out its full send timeout — it now gets an immediate "browser session is closed" error. The MCP surface gets the same probe before its finalize. Closes #316.
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.
Fixes the #316 stall without breaking auto-finalize-on-close, and rescues recordings from browser crashes while at it.
The stall:
closeSessionqueues behind any in-flight recording command; against a dead browser that command waits out its full timeout, holding teardown. The issue's proposed fix (closestopChanbefore draining the mutex) would have made auto-finalize impossible — it needs the connection alive to deliver an active recording.The fix: decide dead-or-alive first.
CloseAll): onebrowsingContext.getTreeprobe bounded to 2s.stopChanfirst (pending commands abort, the mutex frees immediately) and finalize offline: the trace packages from memory, and the engine's live-muxed video embeds when readable and non-empty, with the manifest recording why the video is cut short. A browser crash mid-recording now yields a usable artifact instead of nothing.browserDeadand skips the probe entirely — finalize runs within milliseconds of the crash.The second bug this surfaced (most of the observed 60s): a client command racing session deletion —
browser.stoparriving just after the browser died — was dropped with no reply, leaving the client to wait out its full 60s send timeout. It now receives an immediate"browser session is closed"error, which client stop paths already handle.Measured (SIGKILL Firefox mid-recording, then
bro.stop()): close latency 60,054ms → 41ms, and the recording zip delivers with the trace and an honestvideo/index.json. The MCP surface gets the same probe before its finalize. Unit tests cover the offline-finalize paths (partial video embeds, empty engine file recorded as an error rather than embedded, bytes-only cleanup); fullmake testgreen (5m41s).Closes #316.