fix(linux): guard host state against concurrent shutdown - #80
Merged
Conversation
Closes #12. shutdown deliberately bypasses the transport's request queue so an operator can always stop a stalled browser action (Transport.swift:213-218). Teardown therefore runs on the main thread while a normal command is still in flight, and LinuxBrowserHost held sessions, trace, activeFlows, and recordings as plain dictionaries behind @unchecked Sendable. Two threads mutating the same Swift dictionary is memory corruption, not a lost update: host.stop() clearing recordings while a command inserted one could crash the host and take every session with it. The macOS host already serialises the same state through onAgentMain and a recordings lock; Linux had nothing. All four containers now go through stateLock. The lock is held only around collection access and never across browser I/O, so a stalled command still cannot delay teardown — the property the queue bypass exists to provide. stop() is now idempotent: it flips a stopping flag, snapshots and clears the containers under the lock, then stops recordings, closes sessions, and kills the browser outside it. Session and recording registration re-check stopping under the lock and back out cleanly if teardown has begun. Without that, a recording started during teardown would leak an FFmpeg process that nothing would ever stop, and a session created in the same window would leak a browser target. Regression coverage in Tests/linux-e2e.sh drives the real race: an active recording plus a full-page tour in flight, then stop, asserting the host exits on its own and restarts clean.
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.
Closes #12 — the one
priority:criticalitem in Phase 1.The race
shutdowndeliberately bypasses the transport's request queue so an operator can always stop a stalled browser action (Transport.swift:213-218). That is a feature, and this PR keeps it. The consequence is thathost.stop()runs on the main thread while a normal command is still executing on a client thread.LinuxBrowserHostheldsessions,trace,activeFlows, andrecordingsas plain dictionaries behind@unchecked Sendablewith no lock. Two threads mutating the same Swift dictionary is not a lost update — it is memory corruption.stop()clearingrecordingswhile arecord startinserted into it can crash the host process and take every session with it.The macOS host already serialises the same state through
onAgentMainplus a dedicated recordings lock. Linux had nothing. Asymmetric safety for identical state.The fix
stateLock. The lock is held only around collection access, never across browser I/O, so a stalled command still cannot delay teardown — the exact property the queue bypass exists to provide.stop()is idempotent: flips astoppingflag, snapshots and clears the containers under the lock, then stops recordings, closes sessions, and kills the browser outside it.stoppingunder the lock and back out cleanly if teardown has begun. Without that, a recording started during the teardown window leaks an FFmpeg process nothing will ever stop, and a session created in the same window leaks a browser target.Test
Tests/linux-e2e.shnow drives the real race rather than a proxy: an active recording and a full-page tour in flight, thenheadless stop, asserting the host exits on its own within 30 s and restarts clean with only the default session. It runs in theLinux E2E (Docker)job.Verification note
I cannot compile Swift in my environment — no toolchain, and no Docker access — so CI is the first compile of this change. I re-read every edited region and specifically checked for the Swift trap where a helper named
session/recordingwould resolve to a local variable declared later in the same scope; the helpers are namedlookupSession/lookupRecordingto avoid it. If CI finds something, I will fix it here rather than in a follow-up.