fix(macos): detect a dead externally-attached core - #927
Merged
Conversation
Attach mode is "probe this Unix socket and connect to whatever answers", but the socket path was hard-coded to ~/.mcpproxy/mcpproxy.sock with no override. That made attach mode untestable and un-reproducible: a second MCPProxy.app instance would discover, attach to, and contend with the developer's live core instead of an isolated one. - CoreProcessManager takes an optional socketPath and a refreshInterval (defaults unchanged: the real socket, a 30s tick), so a test can point it at a stub core and drive the periodic tick without waiting 30 seconds. - NotificationService resolves UNUserNotificationCenter lazily. current() raises NSInternalInconsistencyException outside a real app bundle, so merely CONSTRUCTING the service — and therefore CoreProcessManager — crashed the test process. Delivery paths are untouched. - MCPProxyTests/Support/UnixSocketHTTPStub: a minimal HTTP/1.1 server on a Unix socket standing in for an externally-managed core, plus the first test over the new seam: the manager attaches to a core it did not spawn and marks it externalAttached. No behaviour change. Enabling refactor for GH #926. Related #926
…nected"
Attaches to a stub core over the injected socket, then takes the socket away
(the reporter's `kill -9`) and waits. Both new tests fail today:
testDeadExternalCoreLeavesConnectedState
XCTAssertNotEqual failed: ("connected") is equal to ("connected")
testReattachesWhenTheExternalCoreComesBack
precondition: the tray must first notice the core is gone, got connected
coreState never leaves .connected, so the menu bar icon renders healthy for as
long as anyone watches — the reporter measured 5.5 minutes; it is unbounded.
Related #926
The tray's only working liveness detector was Process.terminationHandler, which exists solely for a core the tray spawned. When it ATTACHES to a core it did not spawn there is no Process — and the other two candidates are inert: SSEClient retries forever and never finishes its stream, so handleSSEDisconnect() is unreachable, and every periodic refresh helper swallows its errors by design. coreState could therefore never leave .connected: the menu bar icon showed a healthy core indefinitely after it died. This is not limited to MCPPROXY_TRAY_SKIP_CORE=1 as the report assumed. start() calls attachIfCoreIsRunning() before any spawn decision, so anyone whose core is already running — Homebrew, launchd, a terminal, a leftover core, or "start core on launch" turned off — was on the broken path. The 30s refresh tick now probes liveness first, using the same evidence that made us attach in the first place: socket connectable, then GET /ready. On failure it hands off to the existing attemptReconnection(), which already knows how to re-attach and refuses to spawn a replacement for a core it never owned — its "External core process is no longer available" branch was written for exactly this and was unreachable until now. Detection latency <=30s. Also: - keep watching the socket after an external core is lost, so one that comes back (launchd, brew services, `mcpproxy serve`) is picked up without a menu click, instead of leaving the tray parked in .error; - reconnectInFlight guards the new detector against racing the process-exit handler over the same dead core — attemptReconnection() suspends, and two overlapping runs could each relaunch and orphan a core. The exit handler's own behaviour is unchanged; - handleSSEDisconnect() routes through the same handleCoreLoss() helper. Related #926
Deploying mcpproxy-docs with
|
| Latest commit: |
860c73a
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://16f05631.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://fix-926-external-core-livene.mcpproxy-docs.pages.dev |
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 30520382024 --repo smart-mcp-proxy/mcpproxy-go
|
Six failures, each reproducing a real defect in the first cut of this fix:
testConcurrentLossSignalsRunExactlyOneReconnection
5 connects, expected 2 — all four concurrent loss signals ran their own
reconnection. The in-flight check suspends before it sets the flag, so it
guards nothing.
testProcessExitDuringAReconnectionDoesNotStartASecond
3 connects, expected 2 — the process-exit path never consulted the guard.
testConcurrentAttachesAttachOnce
4 attachments, expected 1 — the attach path has no in-flight guard either,
so a manual retry racing the idle watcher attaches twice.
testOneReadinessMissIsToleratedAndTwoAreNot
one 503 from a core whose socket is still up condemns it immediately.
testAReadinessRecoveryResetsTheFailureCount
same, with no notion of CONSECUTIVE misses.
testProbeGivesUpQuicklyOnAnUnresponsiveCore
10.06s for one probe — it inherits the API client's 30s request timeout.
Supporting changes so those tests can exist at all:
- SocketURLProtocol: carry the socket path per-request in a header set from
the session config, and delete the mutable static override. The global meant
constructing any second client silently redirected every existing one — a
liveness detector reporting a different core's health as its own, and the
reason a concurrent test suite would have been unreproducibly flaky.
Covered by testTwoManagersOnDifferentSocketsDoNotRedirectEachOther.
- APIClient/SocketTransport: per-client request timeout.
- NotificationService(deliveryEnabled:): UNUserNotificationCenter needs a real
app bundle, so tests could not exercise any path that sends a notification —
including the process-exit handler under test above.
- UnixSocketHTTPStub: closes accepted connections and joins its thread on
stop(), counts requests per path (connect counting), and can hang a response
to simulate a core that listens but never answers.
- CoreLivenessTests: async tearDown that awaits manager shutdown instead of
a fire-and-forget Task racing the next test.
- CoreProcessManager: probeTimeout parameter (accepted, not yet honoured) and
internal visibility on the three liveness entry points, so the tests can
drive them directly.
Related #926
Addresses the Codex review of PR #927. **Atomic gate on every connection entry point (P1).** The previous guard checked its flag, then SUSPENDED reading appState before setting it, so a second invocation passed the same check during that window — it guarded nothing, which the new tests demonstrate (four concurrent loss signals ran four reconnections). And the process-exit path never consulted it at all. connectionWorkInFlight is now claimed only through beginConnectionWork(), a synchronous check-and-set: under actor isolation, with no await between check and set, it cannot be interleaved. All four entry points honour it — handleCoreLoss (liveness tick + SSE disconnect), handleProcessExit, and the attach path — with scoped release via defer. handleProcessExit now stands down when a reconnection is already running. That is a deliberate behaviour change: for a core we own, a fast crash loop used to have each exit handler start another nested attemptReconnection while the previous one was still inside launchAndConnect, so several relaunches could overlap, overwrite `process`, and orphan a core holding the BBolt lock. The retry ladder still works for the realistic case (a core that dies after running a while — the reconnection has completed, the gate is clear, retryCount advances); a core that dies instantly now surfaces an error after one relaunch instead of racing three. **Attach is gated too (P2), and "in flight" is not "no core".** A manual retry racing the idle attach watcher used to run two attachToExternalCore() calls, each replacing the other's clients and tasks, with a late failure able to overwrite the other's .connected with .error. attachIfCoreIsRunning now returns an outcome: start() treats .inFlight as "leave it alone" rather than falling through to spawn or idle, and the watcher keeps polling so it still recovers if that attach fails. **Liveness probe: short timeout plus hysteresis (P2).** The probe inherited the API client's 30s request timeout, and one miss condemned the core — a busy core would put "reconnecting" in the menu bar, and a wedged one stalled the whole refresh loop for 30s per sample (measured: 10.06s for a single probe against a hung stub, now 0.33s). - probeTimeout 5s, on a dedicated probe client so a probe is never queued behind a slow user-facing request. Three orders of magnitude above a healthy sub-millisecond socket round-trip, and the same order as the core's own advertised SSE retry interval, so a merely-busy core still answers. - probeFailureBudget 2 CONSECUTIVE misses, reset on any success. One miss is a sample, not evidence. Not higher because each strike costs a full refresh interval; 2 bounds worst-case detection at ~1 minute for this case. - The socket check is NOT subject to the budget: a process that exited cannot hold a socket open, so that signal is unambiguous and acts immediately. `kill -9` detection stays at <=30s. Related #926
…ate holder Codex round 2 on PR #927: three P1s, three P2s, one P3. All verified against a real run first — Codex could not execute the suite (its sandbox refuses Unix socket binds), and one of its P1s turned out to be worse than described. **P1 — a slow /ready at startup could unlink a live core's socket and launch a competitor.** `start()` unlinked the socket whenever it was CONNECTABLE, with a comment calling it stale — the predicate was inverted for its stated intent, and shortening the probe timeout to 5s in round 1 made it far easier to hit. One missed readiness check and the tray would remove a running core's socket and start a second core on the same data directory: two writers on one BBolt database. Attach now reports `.unresponsive` distinctly from `.noCore`, and `start()` treats it as "a core is here, wait for it" — no unlink, no spawn, and the attach watch picks it up when it answers. The socket file is only removed when nothing is listening, which is what "stale" was supposed to mean. **P1 — the gate did not cover the spawn path.** `retry()` kills the managed core and calls `start()`, while that core's termination handler is already on its way to reconnecting; both could launch. The gate now wraps the WHOLE of `start()`, attach and spawn alike. `attachIfCoreIsRunningLocked` assumes the caller holds it; the attach watcher takes it via a gated wrapper. **P1 — standing down on process exit ate the retry ladder.** Confirmed and measured: the new ladder test saw ONE launch and `error("Exit code 1")` where four launches and `maxRetriesExceeded` were correct. The ladder depended on each relaunched core's exit callback starting the next attempt, but that callback now correctly stands down while a reconnection holds the gate. The ladder now lives in `launchWithRetries()`, run by whoever holds the gate — so both properties hold at once: exactly one launch at a time, AND every rung. Note this also covered a case Codex did not name: `start()` holds the gate too, so its own launch failures had no ladder either. **P2 — a successful relaunch now clears the ladder** (`launchAndConnectOnce`), so a core that crashes, recovers, and runs normally gets the full ladder again on its next crash instead of inheriting old strikes. **P2 — the routing hint no longer leaks.** `httpAdditionalHeaders` are attached to every request the session makes, including ones the protocol declines to intercept and ones following a redirect, so the header now carries an opaque per-session token instead of `/Users/<name>/.mcpproxy/mcpproxy.sock`. A routed request is also PINNED: it is intercepted even when the socket is missing and fails, rather than silently falling back to TCP and reporting some other core's health as this one's. **P2 — a refused socket connect is not proof of death** (ruling: hysteresis, with a narrow unambiguous exception). `SocketTransport.probeSocket` now classifies: `.absent` (no socket file — a running core always owns its file, so act immediately), `.refused` (stale file from a killed process AND a live core with a full listen backlog look identical — so it gets the same two-strike tolerance as a readiness miss), `.localFailure` (EMFILE/ENOMEM/EACCES — our problem, not evidence about the core, not even a strike). **P3 — the last fire-and-forget shutdown in the tests is awaited.** Related #926
…oke point
Codex round 3: three more P1s, all the same failure — two cores against one
data directory. Rounds 1 and 2 fixed the paths that were named; each left a
sibling unfixed, because the work was organised around paths rather than around
the property. This commit inverts that.
**The property is now enforced where a core is created, not where it is
requested.** `launchCore()` is the only function in the app that can create a
process, so every precondition lives there, checked with no suspension point
before `proc.run()`: the connection gate is held, shutdown has not been asked
for, we do not already own a live process, and NOTHING IS LISTENING on the
socket. Six callers no longer have to stay disciplined for the invariant to
hold.
Path enumeration — every route that can spawn or unlink, and what stops it:
launchCore the only proc.run(); four preconditions above
launchAndConnectOnce only caller of launchCore
launchWithRetries callers hold the gate; re-checks "is anything listening"
and shutdown before EVERY rung
start() takes the gate for its whole body, spawn included;
confirms nothing is listening before unlink or spawn
retry() goes through start(); reaps via terminateManagedProcess
handleProcessExit takes the gate; ignores stale launch generations
handleCoreLoss takes the gate (liveness tick + SSE disconnect)
attach watcher takes the gate; attaches only, never spawns
awaitExternalCore never spawns; starts the watcher
shutdown/supersede never spawn; shutdown cancels in-flight work first
unlink start() only, after confirmNothingIsListening()
**P1/1 — the classification is now single-source.** `assessCore()` is the one
function that answers "is a core alive at this socket, and how sure are we",
and startup, the liveness tick and the watcher all consult it. The errno split
added in round 2 lived only in `coreIsAlive()`; startup flattened everything
non-connectable to "no core", so a full listen backlog, EMFILE or ENOMEM still
led to unlinking a live core's socket. Before anything destructive, startup now
also confirms over a short window that nothing answers — a live core accepts at
least one connection across several attempts; a stale socket file refuses every
one.
**P1/2 — abandoned launch attempts are reaped.** A core that hung without ever
becoming ready is still a core; the ladder now terminates and reaps it before
the next rung. Launches are stamped with a generation so the reaped process's
termination callback cannot land later and drive a relaunch.
**P1/3 — shutdown cancels work in flight.** `shutdownRequested` is set before
anything else and checked at every step that can lead to a spawn. A ladder
asleep in its backoff no longer wakes up and launches a core after the user
pressed Stop — the tray can no longer show "Stopped" while owning a process.
**P2/4 — waiting is now bounded.** Refusing to fight a core that holds the
socket is right for the database and, alone, unusable for the person:
`.waitingForCore` offers neither Stop nor Retry. After `unresponsiveCoreTimeout`
(60s, matching the socket-wait budget for a core we launch ourselves) it becomes
an error the menu can act on, while the attach watch keeps running underneath so
a core that does eventually answer is still picked up.
Also fixed while unifying the probe: the pre-attach and post-attach probes were
two clients with identical configuration, and the pre-attach one was created per
probe — minting a routing token every 2 seconds while the watcher polled. Now
one client for the manager's life.
Tests count launches inside `launchCore` rather than by lines written by the
launched script. That is not cosmetic: the script-based counter reported ZERO
while four cores were being spawned and reaped, because a reaped process dies
before it can record itself.
Related #926
The unlink after `confirmNothingIsListening()` looked like stale-socket cleanup, but by the time control reaches it, "stale" is exactly what the file cannot be. A leftover file from a dead core probes `.refused`, which `attachIfCoreIsRunningLocked()` reports as `.unusable`, and start() waits instead of spawning — that path never gets here. The only way a file is present at this line is that it appeared AFTER the probes said `.absent`, i.e. something just bound it. So the branch deleted a LIVE core's socket and stranded it: clients keep an unlinked inode, nothing new can connect. Removing it costs nothing. launchCore's preflight only refuses on `.connectable`, so a genuinely stale file still lets the spawn proceed, and the Go core cleans such a file up itself (cleanupStaleSocket in internal/server/listener_unix.go) and refuses to start when the socket is in use. Worst case is now a doomed spawn that exits, after which the ladder sees the live listener and waits for it.
…its comments The four checks in front of `proc.run()` are the whole of "never two cores on one data directory", but they were inline in launchCore where only a full entry point could reach them. Extracted verbatim as `preflightLaunch()` — same order, same errors — so a test can call it directly. `beginConnectionWork()`/`endConnectionWork()` and `confirmNothingIsListening()` drop to internal for the same reason; behaviour is unchanged. Two comments claimed more than the code delivers: - The preflight comment said actor isolation means "nothing can change underneath them". Actor isolation only rules out in-process reentrancy; an external process can still bind the socket in the milliseconds after the last probe. The real backstops are core-side — bbolt's exclusive data-directory lock (loser exits with code 3) and the core's refusal to start on a socket that is in use — so say that. - "Every caller consults assessCore()" was not true: coreIsAlive() switches on probeSocket() itself because it counts strikes per case. It applies the same mapping, which is the claim worth making.
…ation window Every existing test reaches the preflight through a whole entry point (start/retry/handleProcessExit), which proves those paths but leaves the checks themselves covered only in aggregate — a reordering or a dropped guard could still pass. Five tests now hit them head-on: - a connectable socket is refused (nothing to launch); - a still-running managed process is refused, and refused BEFORE the socket check, which is what stops a core that has not yet created its socket from getting a sibling; - a socket FILE nobody answers on is allowed through — the property that makes removing the tray-side unlink safe; - no connection gate is refused; - `confirmNothingIsListening()` returns false when a listener binds between two of its probes, which is the case a single sample misses.
Removing the tray-side unlink fixed a TOCTOU and introduced a worse regression: a socket file left behind by a crashed core probes `.refused`, which the wait path treats as "something may be there", so startup waited forever. Nothing else removes that file — the core's own cleanupStaleSocket does not run until a core is launched — so after the 60s deadline the user was told to quit a process that does not exist, and Retry repeated the same wait. The deadline now decides on evidence rather than on the last sample. Every probe of the connection episode is accumulated: if a connection was ever ACCEPTED, nothing changes and the wait stays a wait (that is the case the whole PR exists to protect, and a full listen backlog is exactly it). If the window was refusals only, the wait escalates to a launch. The escalation unlinks nothing. It takes the connection gate, runs the confirmation window once more as a last look, and goes through launchWithRetries into the single choke point, where preflightLaunch still refuses a connectable socket. The spawned core does the dangerous step itself and does it properly: it dials immediately before removing the file and fails with "socket is in use by another process" if that dial succeeds. It also honours the launch policy — a wait entered with maySpawn false can never end in a spawn. confirmNothingIsListening gains an optional per-probe hook so a test can hold it between two probes; nil in production.
… real Three tests around the wait deadline: - a socket file nothing ever accepts on is recovered by launching, instead of ending in an error about a process that does not exist. Fails before the fix. - a core that IS listening and never answers /ready is still never launched over, and still becomes the "quit that process" error. - a core that accepted a connection EARLY in the window and refuses everything after it (a saturated listen backlog) is never launched over. This is the one the accumulated evidence exists for: at the deadline it looks identical to a stale file, so only the memory of that first accepted connection prevents a second writer. Bypassing the evidence check makes this test — and only this test — fail. The confirmation-window race test no longer relies on timing. It started a task and slept, so a task that began late would see the listener on its FIRST probe and pass without running the race. It now holds the confirmation at probe 0 through the injected hook, binds the listener, and releases — the listener provably appears between two probes. testAnIndeterminateSocketAtStartupNeitherUnlinksNorSpawns encoded the old error-on-deadline behaviour with a 0.5s timeout. Its subject is the FIRST probe (one refusal is not evidence, so startup waits), so its timeout is now long enough that the deadline cannot fire, and the deadline's behaviour is asserted by the tests above instead.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Arming auto-merge: Codex cross-model review CLEAN (round 6). Live QA on real processes: stale-socket recovery end-to-end in 61.7s with the socket inode provably preserved through the tray wait (the NEW core removes and recreates it, 167ms after spawn); live-but-silent core produced ZERO launch attempts, verified by a spawn ledger and pgrep, and held through an explicit Retry; healthy attach clean. Known limitation tracked in #933.
Dumbris
added a commit
that referenced
this pull request
Jul 30, 2026
Conflict in NotificationService.swift: both sides made the center lazy to unblock unit construction. main's version (from #927) is a superset — it also adds the deliveryEnabled flag and its guards — so it is taken whole. The glance branch's other change to the file, the doc comment on sendSensitiveDataAlert, merged cleanly and is preserved.
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
2 participants
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.
Related #926
Problem
When the tray attaches to a core it did not spawn,
proc.terminationHandler— the only working liveness detector — does not exist.SSEClient.connect()retries forever and never finishes its stream, sohandleSSEDisconnect()is unreachable, and the periodic refreshes swallow their errors.coreStatecan therefore never leave.connected, and the menu-bar icon shows healthy indefinitely after the core dies.Wider than the issue reports.
start()callsattachIfCoreIsRunning()before any spawn decision, so this is not limited toMCPPROXY_TRAY_SKIP_CORE=1: anyone whose core is already running — Homebrew, launchd, a terminal, a leftover core, or "start core on launch" off — is on the broken path.Note the issue diagnoses the wrong binary. It blames
cmd/mcpproxy-tray(Go), but DMG/PKG ship the Swift app. The Go tray has the same class of bug on its own attach path, and itsHealthMonitorresults channel has no consumer at all — so the fix proposed in the issue would be a no-op there too. Filed separately.Fix
A liveness probe (socket connect +
apiClient.ready()) at the top of the existing periodic tick, handing off toattemptReconnection()— whose"External core process is no longer available"branch was written for this mode and was unreachable. Detection ≤30s. Probing with exactly the evidence that made us attach means attach and stay-attached cannot disagree.Two supporting changes:
startAttachWatch()on external-core loss, so a core restarted by launchd/brew is picked back up without a menu click; and areconnectInFlightguard, because this adds a second caller of the suspendingattemptReconnection()— without it the exit handler and the tick can each relaunch, orphaning a core that then holds the BBolt lock.Enabling refactor
The socket path was hard-coded, the tick interval fixed, and
NotificationServicewas unconstructible in a test process (UNUserNotificationCenter.current()raises outside an app bundle). All three are now injectable/lazy — that is why nobody could test this path before.Testing
3 new tests driving the real attach path against a Unix-socket HTTP stub core. Red before the fix (3 failures), green after, detection in 270ms on a 200ms tick. Mutation-verified twice: disabling the probe fails exactly those 3 and nothing else; disabling re-attach fails only the re-attach test.
swift test289 → 292, 0 failures;swift buildclean, no new warnings..github/workflows/native-tests.ymldefinesswift-test, gated onif: needs.changes.outputs.native == .true.— it skips on PRs that do not touchnative/, which is why it read as absent. It runs on this PR.