From 02854cfebe8669a6108a6a83c58afa13d5554803 Mon Sep 17 00:00:00 2001 From: Kyrill <1942093+poolski@users.noreply.github.com> Date: Mon, 2 Nov 2026 11:42:22 +0000 Subject: [PATCH 1/2] fix(tray): launch over a dead core's socket without the 60s wait When a core exits without cleaning up, it leaves mcpproxy.sock behind. On the next start the tray probed that socket, got a refusal, and waited out unresponsiveCoreTimeout (60s) before checking the data directory lock and launching a core. start() now checks the lock first. If the socket has refused every probe and nobody holds config.db, it launches immediately through the existing path. bbolt takes the lock before a core binds its listener, so a live core, even a saturated one (GH #933), still gets the full wait. An unknown lock state, such as a socket with no database beside it, also keeps the wait. --- .../MCPProxy/Core/CoreProcessManager.swift | 22 ++++++-- .../SaturatedCoreEscalationTests.swift | 56 ++++++++++++++++++- 2 files changed, 71 insertions(+), 7 deletions(-) diff --git a/native/macos/MCPProxy/MCPProxy/Core/CoreProcessManager.swift b/native/macos/MCPProxy/MCPProxy/Core/CoreProcessManager.swift index 69931d20c..c9245611b 100644 --- a/native/macos/MCPProxy/MCPProxy/Core/CoreProcessManager.swift +++ b/native/macos/MCPProxy/MCPProxy/Core/CoreProcessManager.swift @@ -297,6 +297,19 @@ actor CoreProcessManager { // else owns this" rather than as an invitation to spawn. return case .unusable(let reason): + // A socket that has refused every probe beside a database nobody + // holds is a dead core's leftovers, and the deadline below would + // reach exactly that verdict, only 60 seconds later. bbolt takes + // the lock before a core creates its listener, so a live core, even + // a saturated one (GH #933), always holds it. `.unknown` proves + // nothing and keeps the wait. + if maySpawn, socketEvidence.onlyEverRefused, + DataDirectoryLock.probe(path: dataDirectoryLockPath) == .free { + NSLog("[MCPProxy] %@ refuses every probe and nobody holds %@ — " + + "a dead core's socket file; launching without waiting (%@)", + socketPath, dataDirectoryLockPath, reason) + break + } // Something is, or may be, on the other end of that socket. Do NOT // unlink it and do NOT start a competing core against the same data // directory. Wait instead — the attach watch re-probes and attaches @@ -326,11 +339,10 @@ actor CoreProcessManager { guard !shutdownRequested else { return } // Confirmed empty — and the tray does NOT unlink the socket file here. - // A socket file left over from a dead core never reaches this line: it - // probes `.refused`, which `attachIfCoreIsRunningLocked()` reports as - // `.unusable` above, sending us down the wait path. So the only file - // that could be here is one that appeared AFTER the probes — i.e. one - // something just bound — and unlinking that strands a live core. + // A socket file left over from a dead core reaches this line only when + // nobody holds its data directory (the `.unusable` case above); any + // other file could be one that something bound AFTER the probes, and + // unlinking that strands a live core. // // Cleaning up a genuinely stale file is the core's job anyway: it // removes one it can prove nobody is listening on (see diff --git a/native/macos/MCPProxy/MCPProxyTests/SaturatedCoreEscalationTests.swift b/native/macos/MCPProxy/MCPProxyTests/SaturatedCoreEscalationTests.swift index 264829b35..5d36fdfc3 100644 --- a/native/macos/MCPProxy/MCPProxyTests/SaturatedCoreEscalationTests.swift +++ b/native/macos/MCPProxy/MCPProxyTests/SaturatedCoreEscalationTests.swift @@ -151,9 +151,61 @@ final class SaturatedCoreEscalationTests: XCTestCase { + "dead core's leftovers and the tray has to get past it on its own") } + /// A dead core's leftovers are recognisable at startup: the socket refuses + /// every probe and nobody holds the database beside it. The tray must + /// launch straight away instead of sitting out the whole unresponsive-core + /// deadline (60 s in production) before reaching the same verdict. + func testAStaleSocketBesideAFreeDataDirectoryIsLaunchedOverWithoutWaitingForTheDeadline() async throws { + let fakeCore = try FakeCoreBinary(behaviour: "sleep 30") + let restoreEnv = fakeCore.install() + defer { restoreEnv() } + + let socketPath = try refusingSocket() + FileManager.default.createFile( + atPath: directory.appendingPathComponent("config.db").path, contents: Data() + ) + + let appState = await MainActor.run { AppState() } + let manager = makeManager(appState: appState, socketPath: socketPath, unresponsiveCoreTimeout: 30) + self.manager = manager + + await manager.start(maySpawn: true) + try await Task.sleep(nanoseconds: 2_000_000_000) + + let launched = await manager.coresLaunched + XCTAssertGreaterThan(launched, 0, + "a refusing socket beside an unheld database is a dead core's " + + "leftovers; waiting out the deadline only delays the launch") + } + + /// The shortcut needs positive proof that nobody owns the data directory. + /// With no database beside the socket the lock says nothing either way, so + /// the tray keeps the old behaviour and waits for the deadline. + func testAStaleSocketWithNoDatabaseBesideItStillWaitsForTheDeadline() async throws { + let fakeCore = try FakeCoreBinary(behaviour: "sleep 30") + let restoreEnv = fakeCore.install() + defer { restoreEnv() } + + let socketPath = try refusingSocket() + + let appState = await MainActor.run { AppState() } + let manager = makeManager(appState: appState, socketPath: socketPath, unresponsiveCoreTimeout: 30) + self.manager = manager + + await manager.start(maySpawn: true) + try await Task.sleep(nanoseconds: 2_000_000_000) + + let launched = await manager.coresLaunched + XCTAssertEqual(launched, 0, "an unknown lock state is not proof the socket is stale") + let state = await MainActor.run { appState.coreState } + XCTAssertEqual(state, .waitingForCore) + } + // MARK: - Fixtures - private func makeManager(appState: AppState, socketPath: String) -> CoreProcessManager { + private func makeManager( + appState: AppState, socketPath: String, unresponsiveCoreTimeout: TimeInterval = 0.5 + ) -> CoreProcessManager { CoreProcessManager( appState: appState, notificationService: NotificationService(deliveryEnabled: false), @@ -163,7 +215,7 @@ final class SaturatedCoreEscalationTests: XCTestCase { socketPath: socketPath, refreshInterval: 60, probeTimeout: 0.3, - unresponsiveCoreTimeout: 0.5, + unresponsiveCoreTimeout: unresponsiveCoreTimeout, socketWaitTimeout: 1.0 ) } From 2cdd0320a7b76344e364985124959da260664eeb Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Mon, 2 Nov 2026 19:27:15 +0200 Subject: [PATCH 2/2] test(tray): isolate refusing-socket tests from /tmp/config.db and cover maySpawn=false shortcut The startup shortcut derives the data-directory lock from the socket's directory, so CoreLivenessTests that leave a refusing stub at the default /tmp path would consult a stray /tmp/config.db. Give the two such tests a private scratch directory. Add a SaturatedCoreEscalationTests case pinning that start(maySpawn: false) never launches beside a free lock, and update two doc comments to match the new flow. Co-Authored-By: Claude Fable 5.1 --- .../MCPProxy/Core/CoreProcessManager.swift | 4 ++- .../MCPProxyTests/CoreLivenessTests.swift | 21 +++++++++-- .../SaturatedCoreEscalationTests.swift | 35 +++++++++++++++++-- 3 files changed, 54 insertions(+), 6 deletions(-) diff --git a/native/macos/MCPProxy/MCPProxy/Core/CoreProcessManager.swift b/native/macos/MCPProxy/MCPProxy/Core/CoreProcessManager.swift index c9245611b..327efbe0c 100644 --- a/native/macos/MCPProxy/MCPProxy/Core/CoreProcessManager.swift +++ b/native/macos/MCPProxy/MCPProxy/Core/CoreProcessManager.swift @@ -901,7 +901,9 @@ actor CoreProcessManager { /// is gone. Launch a core rather than telling the user to quit a process that /// does not exist — the error we would otherwise show is unactionable, and /// Retry would repeat this same wait forever, because nothing else in the - /// system removes that file. + /// system removes that file. A refused socket beside a free data-directory + /// lock never gets here: `start()` launches at once. This covers the rest — + /// a lock held and then freed, or one that could not be read. /// /// Why this cannot resurrect the double-spawn hazard the tray-side unlink /// had. It never unlinks anything: the dangerous step is done by the core we diff --git a/native/macos/MCPProxy/MCPProxyTests/CoreLivenessTests.swift b/native/macos/MCPProxy/MCPProxyTests/CoreLivenessTests.swift index 2d6d6b574..7be647079 100644 --- a/native/macos/MCPProxy/MCPProxyTests/CoreLivenessTests.swift +++ b/native/macos/MCPProxy/MCPProxyTests/CoreLivenessTests.swift @@ -21,6 +21,21 @@ final class CoreLivenessTests: XCTestCase { private var stub: UnixSocketHTTPStub? private var manager: CoreProcessManager? + private var privateDirectories: [URL] = [] + + /// A scratch directory of its own for a socket that is left REFUSING at + /// `start()`. The data-directory lock is derived from the socket's + /// directory (`DataDirectoryLock.path(forSocket:)`), so a bare `/tmp` socket + /// would consult a stray `/tmp/config.db`, and a free one would make the + /// startup shortcut launch. /tmp, not NSTemporaryDirectory(): sun_path is + /// 104 bytes. + private func refusingSocketPath() throws -> String { + let directory = URL(fileURLWithPath: "/tmp/mcpproxy-live-\(UUID().uuidString.prefix(8))", + isDirectory: true) + try FileManager.default.createDirectory(at: directory, withIntermediateDirectories: true) + privateDirectories.append(directory) + return directory.appendingPathComponent("mcpproxy.sock").path + } /// Async teardown so shutdown actually completes before the next test runs: /// a fire-and-forget `Task { await shutdown() }` would race the next test's @@ -32,6 +47,8 @@ final class CoreLivenessTests: XCTestCase { manager = nil stub?.stop() stub = nil + for directory in privateDirectories { try? FileManager.default.removeItem(at: directory) } + privateDirectories = [] try await super.tearDown() } @@ -545,7 +562,7 @@ final class CoreLivenessTests: XCTestCase { // A socket file that refuses connections. From out here this is // indistinguishable from a live core with a full backlog. - let stub = UnixSocketHTTPStub.healthyCore() + let stub = UnixSocketHTTPStub.healthyCore(at: try refusingSocketPath()) try stub.start() stub.stop(unlinkSocket: false) defer { unlink(stub.path) } @@ -755,7 +772,7 @@ final class CoreLivenessTests: XCTestCase { defer { restoreEnv() } // Exactly what a killed core leaves behind: the file, and nothing on it. - let stub = UnixSocketHTTPStub.healthyCore() + let stub = UnixSocketHTTPStub.healthyCore(at: try refusingSocketPath()) try stub.start() stub.stop(unlinkSocket: false) defer { unlink(stub.path) } diff --git a/native/macos/MCPProxy/MCPProxyTests/SaturatedCoreEscalationTests.swift b/native/macos/MCPProxy/MCPProxyTests/SaturatedCoreEscalationTests.swift index 5d36fdfc3..0182accc6 100644 --- a/native/macos/MCPProxy/MCPProxyTests/SaturatedCoreEscalationTests.swift +++ b/native/macos/MCPProxy/MCPProxyTests/SaturatedCoreEscalationTests.swift @@ -82,9 +82,10 @@ final class SaturatedCoreEscalationTests: XCTestCase { } /// The other half of the same rule: a socket file with NO live holder is - /// still escalated over, exactly as before. Without this the fix would have - /// traded a rare bad launch for a permanent deadlock on every stale socket — - /// nothing else in the system removes that file. + /// still launched over (via the up-front lock check in `start()`, no longer + /// the deadline). Without this the fix would have traded a rare bad launch + /// for a permanent deadlock on every stale socket — nothing else in the + /// system removes that file. func testAGenuinelyStaleSocketIsStillLaunchedOver() async throws { let fakeCore = try FakeCoreBinary(behaviour: "sleep 30") let restoreEnv = fakeCore.install() @@ -220,6 +221,34 @@ final class SaturatedCoreEscalationTests: XCTestCase { ) } + /// The shortcut is a start-time decision about a socket we may not launch + /// over. With spawning disallowed (a tray that only attaches) an unheld + /// database must not launch anything: the tray waits for a core to appear. + func testAStaleSocketBesideAFreeDataDirectoryStillWaitsWhenSpawningIsNotAllowed() async throws { + let fakeCore = try FakeCoreBinary(behaviour: "sleep 30") + let restoreEnv = fakeCore.install() + defer { restoreEnv() } + + let socketPath = try refusingSocket() + FileManager.default.createFile( + atPath: directory.appendingPathComponent("config.db").path, contents: Data() + ) + + let appState = await MainActor.run { AppState() } + let manager = makeManager(appState: appState, socketPath: socketPath, unresponsiveCoreTimeout: 30) + self.manager = manager + + await manager.start(maySpawn: false) + try await Task.sleep(nanoseconds: 2_000_000_000) + + let launched = await manager.coresLaunched + XCTAssertEqual(launched, 0, "maySpawn is false: nothing may be launched, free lock or not") + XCTAssertEqual(fakeCore.launchCount(), 0) + XCTAssertNil(manager.managedProcess) + let state = await MainActor.run { appState.coreState } + XCTAssertEqual(state, .waitingForCore) + } + /// A socket file that refuses every connection — what a saturated backlog /// and a dead core's leftovers look like from the outside, identically. private func refusingSocket() throws -> String {