From 5a0b0c8a67d93ef681993fa20bfaefa45d761fce Mon Sep 17 00:00:00 2001 From: Aditya Garud <153842990+yashranaway@users.noreply.github.com> Date: Wed, 5 Aug 2026 19:47:00 +0000 Subject: [PATCH] fix(linux): guard host state against concurrent shutdown MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- apps/headless/LinuxHost/main.swift | 136 +++++++++++++++++++++------ apps/headless/Tests/linux-e2e.sh | 29 ++++++ docs/roadmap/improvements-backlog.md | 14 ++- 3 files changed, 146 insertions(+), 33 deletions(-) diff --git a/apps/headless/LinuxHost/main.swift b/apps/headless/LinuxHost/main.swift index db8113d..fa822eb 100644 --- a/apps/headless/LinuxHost/main.swift +++ b/apps/headless/LinuxHost/main.swift @@ -12,6 +12,8 @@ final class LinuxBrowserHost: @unchecked Sendable { private var recordings: [String: BrowserRecording] = [:] private let traceStartedAt = ProcessInfo.processInfo.systemUptime private let artifacts: ArtifactStore + private let stateLock = NSLock() + private var stopping = false var onShutdown: (() -> Void)? init() throws { @@ -21,11 +23,46 @@ final class LinuxBrowserHost: @unchecked Sendable { trace["default"] = [] } + /// Every mutation of `sessions`, `trace`, `activeFlows`, and `recordings` + /// goes through here. `shutdown` deliberately bypasses the transport's + /// request queue so an operator can always stop a stalled browser action, + /// which means `stop()` runs while a normal command may still be in + /// flight — without this lock both threads mutate the same dictionaries. + /// Hold it only around collection access, never across browser I/O, so a + /// stalled command cannot delay teardown. + private func withState(_ body: () -> T) -> T { + stateLock.lock() + defer { stateLock.unlock() } + return body() + } + + private func lookupSession(_ name: String) -> LinuxBrowserSession? { + withState { sessions[name] } + } + + private func lookupRecording(_ name: String) -> BrowserRecording? { + withState { recordings[name] } + } + + private func traceEvents(for name: String) -> [JSONValue] { + withState { trace[name] ?? [] } + } + func stop() { - for recording in recordings.values { _ = try? recording.stop(timeout: 5) } - recordings.removeAll() - for session in sessions.values { browser.closeSession(session) } - sessions.removeAll() + let (activeRecordings, openSessions) = withState { + () -> ([BrowserRecording], [LinuxBrowserSession]) in + if stopping { return ([], []) } + stopping = true + let capturedRecordings = Array(recordings.values) + let capturedSessions = Array(sessions.values) + recordings.removeAll() + sessions.removeAll() + trace.removeAll() + activeFlows.removeAll() + return (capturedRecordings, capturedSessions) + } + for recording in activeRecordings { _ = try? recording.stop(timeout: 5) } + for session in openSessions { browser.closeSession(session) } browser.stop() } @@ -87,27 +124,51 @@ final class LinuxBrowserHost: @unchecked Sendable { case .sessionCreate: guard let name = request.parameters["name"]?.stringValue else { return failure(request, "MISSING_PARAMETER", "Session name is required.") } try validateIdentifier(name, field: "session") - guard sessions[name] == nil else { return failure(request, "SESSION_EXISTS", "Session already exists: \(name)") } - sessions[name] = try browser.createSession() - trace[name] = [] + guard withState({ sessions[name] == nil }) else { return failure(request, "SESSION_EXISTS", "Session already exists: \(name)") } + let created = try browser.createSession() + // Re-check under the lock: teardown may have started while the + // browser was creating the target. + let rejection = withState { () -> String? in + if stopping { return "HOST_UNAVAILABLE" } + if sessions[name] != nil { return "SESSION_EXISTS" } + sessions[name] = created + trace[name] = [] + return nil + } + if let rejection { + browser.closeSession(created) + return failure( + request, rejection, + rejection == "SESSION_EXISTS" + ? "Session already exists: \(name)" + : "Host is shutting down." + ) + } record(.sessionCreate, session: name) return .success(id: request.id, result: .object(["session": .string(name)])) case .sessionList: - return .success(id: request.id, result: .object(["sessions": .array(sessions.keys.sorted().map(JSONValue.string))])) + let names = withState { sessions.keys.sorted() } + return .success(id: request.id, result: .object(["sessions": .array(names.map(JSONValue.string))])) case .sessionClose: let name = request.session ?? "default" - guard let session = sessions.removeValue(forKey: name) else { return missingSession(request, name) } - if let recording = recordings.removeValue(forKey: name) { _ = try? recording.stop(timeout: 5) } + let closing = withState { () -> (LinuxBrowserSession?, BrowserRecording?) in + let session = sessions.removeValue(forKey: name) + guard session != nil else { return (nil, nil) } + let recording = recordings.removeValue(forKey: name) + trace.removeValue(forKey: name) + activeFlows.removeValue(forKey: name) + return (session, recording) + } + guard let session = closing.0 else { return missingSession(request, name) } + if let recording = closing.1 { _ = try? recording.stop(timeout: 5) } browser.closeSession(session) - trace.removeValue(forKey: name) - activeFlows.removeValue(forKey: name) return .success(id: request.id, result: .object(["closed": .string(name)])) default: break } let name = request.session ?? "default" - guard let session = sessions[name] else { return missingSession(request, name) } + guard let session = lookupSession(name) else { return missingSession(request, name) } let result: JSONValue switch request.command { case .visit: @@ -131,8 +192,8 @@ final class LinuxBrowserHost: @unchecked Sendable { "browserRuntimeSource": .string(browser.runtime.source.rawValue), "browserTransport": .string("inherited-devtools-pipe"), "targetId": .string(session.targetID), "page": try session.state(), - "trace": .array(trace[name] ?? []), - "recording": recordings[name]?.status() ?? .object(["active": .bool(false)]), + "trace": .array(traceEvents(for: name)), + "recording": lookupRecording(name)?.status() ?? .object(["active": .bool(false)]), ]) case .screenshot: if request.parameters["series"]?.stringValue != nil { @@ -153,7 +214,7 @@ final class LinuxBrowserHost: @unchecked Sendable { ) } case .recordStart: - guard recordings[name] == nil else { throw RecordingError.alreadyActive } + guard lookupRecording(name) == nil else { throw RecordingError.alreadyActive } let format = try recordingFormat( explicit: request.parameters["format"]?.stringValue, output: request.parameters["output"]?.stringValue @@ -176,12 +237,24 @@ final class LinuxBrowserHost: @unchecked Sendable { try? FileManager.default.removeItem(at: output) throw error } - recordings[name] = recording + // Registering under the lock keeps a recording started during + // teardown from outliving the host as an orphaned FFmpeg + // process that nothing will ever stop. + let registered = withState { () -> Bool in + guard !stopping, recordings[name] == nil else { return false } + recordings[name] = recording + return true + } + guard registered else { + _ = try? recording.stop(timeout: 5) + try? FileManager.default.removeItem(at: output) + throw CDPError.commandFailed("Host is shutting down") + } result = recording.status() case .recordStatus: - result = recordings[name]?.status() ?? .object(["active": .bool(false)]) + result = lookupRecording(name)?.status() ?? .object(["active": .bool(false)]) case .recordStop: - guard let activeRecording = recordings[name] else { throw RecordingError.notActive } + guard let activeRecording = lookupRecording(name) else { throw RecordingError.notActive } if let output = request.parameters["output"]?.stringValue, let actual = artifactExtension(output), actual != activeRecording.format.fileExtension { @@ -190,7 +263,7 @@ final class LinuxBrowserHost: @unchecked Sendable { actual: actual ) } - guard let recording = recordings.removeValue(forKey: name) else { throw RecordingError.notActive } + guard let recording = withState({ recordings.removeValue(forKey: name) }) else { throw RecordingError.notActive } let recordingStatus: JSONValue do { recordingStatus = try recording.stop() } catch { @@ -252,7 +325,7 @@ final class LinuxBrowserHost: @unchecked Sendable { "format": .string("headless-qa-report-v1"), "createdAt": .number(Date().timeIntervalSince1970), "session": .string(name), "page": .object(["engine": .string("chromium"), "state": try session.state()]), - "qa": try session.qaReport(), "trace": .array(trace[name] ?? []), + "qa": try session.qaReport(), "trace": .array(traceEvents(for: name)), "artifacts": try artifacts.list(), "security": .object(["sensitiveValuesIncluded": .bool(false), "transport": .string("local-unix-socket")]), ]) @@ -260,10 +333,10 @@ final class LinuxBrowserHost: @unchecked Sendable { requestedName: request.parameters["output"]?.stringValue, extension: "json", prefix: "qa-report-\(name)") case .flowStart: - activeFlows[name] = [] + withState { activeFlows[name] = [] } result = .object(["recording": .bool(true), "note": .string("Only safe navigation actions are recorded; typed values and credentials are never stored.")]) case .flowStop: - let steps = activeFlows.removeValue(forKey: name) ?? [] + let steps = withState { activeFlows.removeValue(forKey: name) } ?? [] result = try artifacts.write(ProtocolCodec.encoder.encode(RecordedFlow(commands: steps)), requestedName: request.parameters["output"]?.stringValue, extension: "json", prefix: "flow-\(name)") @@ -286,8 +359,11 @@ final class LinuxBrowserHost: @unchecked Sendable { return failure(request, "INVALID_COMMAND", "Command is not valid in this context.") } record(request.command, session: name, result: result) - if let step = flowStepIfSafe(command: request.command, parameters: request.parameters), activeFlows[name] != nil { - if (activeFlows[name]?.count ?? 0) < 200 { activeFlows[name]?.append(step) } + if let step = flowStepIfSafe(command: request.command, parameters: request.parameters) { + withState { () -> Void in + guard let steps = activeFlows[name], steps.count < 200 else { return } + activeFlows[name] = steps + [step] + } } return .success(id: request.id, result: result) } catch let error as ProtocolValidationError { @@ -350,10 +426,12 @@ final class LinuxBrowserHost: @unchecked Sendable { if case .object(let object) = result, let url = object["url"]?.stringValue { event["url"] = .string(String(decoding: url.utf8.prefix(2_048), as: UTF8.self)) } - var entries = trace[session] ?? [] - entries.append(.object(event)) - if entries.count > 256 { entries.removeFirst(entries.count - 256) } - trace[session] = entries + withState { () -> Void in + var entries = trace[session] ?? [] + entries.append(.object(event)) + if entries.count > 256 { entries.removeFirst(entries.count - 256) } + trace[session] = entries + } } private func missingSession(_ request: CommandRequest, _ name: String) -> CommandResponse { diff --git a/apps/headless/Tests/linux-e2e.sh b/apps/headless/Tests/linux-e2e.sh index 9c4b35c..dce6326 100755 --- a/apps/headless/Tests/linux-e2e.sh +++ b/apps/headless/Tests/linux-e2e.sh @@ -265,6 +265,35 @@ echo "$BOUNDED_SNAPSHOT" | grep -q '"truncated":true' test "$(printf %s "$BOUNDED_SNAPSHOT" | wc -c)" -lt 1048576 headless session close qa | grep -q '"closed":"qa"' +# Shutdown deliberately bypasses the transport's request queue so an operator +# can stop a stalled browser action. Teardown therefore runs while a command is +# still in flight, and before the host state lock both threads mutated the same +# session and recording dictionaries. Reproduce that with an active recording +# and a long tour in flight, then require a clean exit and a clean restart. +headless session create race | grep -q '"session":"race"' +headless --session race visit http://127.0.0.1:41739/designers/dashboard/ >/dev/null +headless --session race record start --fps 5 --output race-shutdown.mp4 >/dev/null +RACE_HOST_PID="$(headless status | sed -n 's/.*"pid":\([0-9][0-9]*\).*/\1/p')" +test -n "$RACE_HOST_PID" +headless --session race tour --full-page --pace 1000 >/dev/null 2>&1 & +RACE_TOUR_PID=$! +sleep 1 +headless stop | grep -q '"stopping":true' +wait "$RACE_TOUR_PID" 2>/dev/null || true +RACE_WAITED=0 +while [ "$RACE_WAITED" -lt 30 ]; do + kill -0 "$RACE_HOST_PID" 2>/dev/null || break + RACE_WAITED=$((RACE_WAITED + 1)) + sleep 1 +done +if kill -0 "$RACE_HOST_PID" 2>/dev/null; then + echo "host did not exit after shutdown during an in-flight command" >&2 + exit 1 +fi +headless start >/dev/null +headless status | grep -q '"ready":true' +headless session list | grep -q '"sessions":\["default"\]' + if [ -n "${HEADLESS_EVIDENCE_DIR:-}" ]; then umask 077 mkdir -p "$HEADLESS_EVIDENCE_DIR" diff --git a/docs/roadmap/improvements-backlog.md b/docs/roadmap/improvements-backlog.md index d1d24b9..d25d665 100644 --- a/docs/roadmap/improvements-backlog.md +++ b/docs/roadmap/improvements-backlog.md @@ -29,10 +29,16 @@ bypasses that queue (`HP/Transport.swift:213-218`) and `host.stop()` (`LinuxHost/main.swift:24-30`) mutates that state concurrently with an in-flight command. macOS guards the same state (`onAgentMain` + `recordingsLock`, `apps/headless/main.swift:807-808,1307-1328`); Linux does -not. Fix: a host-state lock (or actor) used by both paths; keep the -bypass-the-queue property for shutdown. Test: none exists — add a -concurrent-shutdown stress test beside the existing semaphore-based transport -test (`ProtocolTests.swift:676-746`). +not. ~~Fix: a host-state lock (or actor) used by both paths; keep the +bypass-the-queue property for shutdown.~~ **Done:** all four containers are +behind `stateLock`, taken only around collection access and never across +browser I/O, so teardown still cannot be delayed by a stalled command. `stop()` +is idempotent, snapshots and clears under the lock, then tears down outside it. +Session and recording registration re-check `stopping` under the lock, so a +recording started mid-teardown can no longer outlive the host as an orphaned +FFmpeg process. Regression test in `Tests/linux-e2e.sh`: an active recording +plus a tour in flight while `stop` runs, asserting a clean host exit and a +clean restart. **A2. Force-unwraps in a long-lived host.** ([#13](https://github.com/LockInTime/headless/issues/13)) `visual compare` handlers do `request.parameters["before"]!.stringValue!` on both hosts