transit branch T-1826 commits 6 files 11 touched lines +821 / -78

Pre-push review: T-1826 MCP listener lifecycle

Fresh independent review of origin/main...HEAD at d86199b38163926c548e8ba039cee64e3a4adecd, after fetching origin/main at 49142be301346595eaf4dece8ce3f0ecab8e144f.

At a glance

  • Bounded graceful shutdown: MCPServerLifecycleConfiguration sets maximumGracefulShutdownDuration to five seconds; the timeout regression confirms escalation to cancellation.
  • No process-wide signal trap: production uses ServiceGroup.run(), not Hummingbird runService(), and both graceful/cancellation signal arrays are empty.
  • Deterministic listener probe: the Darwin bind probe mirrors SwiftNIO with SO_REUSEADDR and directly proves it rejects a live listener but accepts the released port despite readiness-probe TIME_WAIT.
  • Repeated same-port success: the serialized parameterized test passed all 20/20 dynamic restart cases in the final non-parallel run.
  • Validation: focused lifecycle tests passed; make test-quick exited 0; lint found 0 violations in 309 files; macOS and iOS builds succeeded.

Verdict

Ready to push

No blocker, critical, or major correctness issue was found. The desired-state reconciliation loop serializes listener teardown before replacement binds, the graceful phase has a configured five-second maximum, signal arrays remain empty, the reusable bind probe has a direct live-versus-released regression, and a fresh focused run passed all nine logical tests including exactly twenty same-port restart cases.

Review findings

4 raised · 0 fixed · 4 skipped

Jump to findings →

Commits

Three-level explanation

What changed

The embedded MCP HTTP server now remembers the latest requested state and completes shutdown of the old listener before opening another listener. That prevents a stop/start or same-port restart from racing the operating system's socket release.

Why it matters

Previously, cancelling the Swift task did not guarantee that Hummingbird had closed its listening socket. A quick restart could therefore fail with “address already in use.” The new lifecycle coordinator explicitly asks the service group to shut down and waits for completion.

Architecture

MCPServer uses a MainActor-isolated desired-state reconciliation loop. Each start/restart receives a run ID; requests arriving while teardown suspends replace stale desired states, and a generation fence prevents an old detached task callback from clearing a newer server.

Policy

The server is wrapped in a ServiceGroup configured with a five-second maximum graceful phase and no Unix signal sequences. This preserves the GUI application's process-wide signal disposition while still allowing explicit shutdown through triggerGracefulShutdown().

Concurrency and socket semantics

tearDownCurrentServer() clears activeServer, advances the generation, triggers graceful shutdown, then awaits the group's run task. The reconcile loop re-checks desiredState after each suspension before launching a target, so rapid requests converge without binding while the previous channel is live.

The regression suite avoids a false negative caused by readiness connections leaving TCP state behind: its bindability check applies SO_REUSEADDR, matching SwiftNIO. A dedicated test establishes the probe's two required outcomes. The configured timeout bounds the graceful phase and escalates to task cancellation; as with the underlying lifecycle library, total completion still depends on the cancelled service eventually cooperating.

Important changes — detailed

MCPServer serializes desired-state reconciliation

Transit/Transit/MCP/MCPServer.swift

Why it matters. This is the correctness core: replacement listeners cannot bind until explicit graceful teardown of the previous service group has completed.

What to look at. MCPServer.swift:44-191

Takeaway. For an async listener, task cancellation is not a resource-release fence; use the server framework's graceful-shutdown path and await completion.
Rationale. Latest-wins desired state lets rapid UI requests coalesce while MainActor isolation and generation fencing protect state from stale task callbacks.

Lifecycle policy bounds grace without signal traps

Transit/Transit/MCP/MCPServerLifecycleConfiguration.swift

Why it matters. The embedded GUI service must not install process-wide SIGTERM/SIGINT handling, and an unbounded graceful phase could wedge every later lifecycle request.

What to look at. MCPServerLifecycleConfiguration.swift:5-21

Takeaway. Keep framework policy in a small configuration factory so timeout and empty-signal invariants are directly testable.
Rationale. Direct <code>ServiceGroup.run()</code> is used instead of Hummingbird's signal-installing convenience path; shutdown is initiated explicitly by the owning object.

Socket regressions validate real listener ownership

Transit/TransitTests/MCPServerLifecycleTests.swift

Why it matters. A port-connect check alone cannot prove the old listener closed before a replacement started; reusable bind probing tests ownership at the socket boundary.

What to look at. MCPServerLifecycleTests.swift:18-328

Takeaway. When a readiness connection can leave TIME_WAIT, align the test probe's reuse option with the production listener before interpreting EADDRINUSE.
Rationale. The suite is serialized and uses dynamically allocated loopback ports, with a direct probe self-test and twenty same-port restart repetitions.

Settings schedules async lifecycle requests

Transit/Transit/Views/Settings/SettingsView.swift

Why it matters. SwiftUI callbacks remain synchronous while start/stop/restart now await reconciliation.

What to look at. SettingsView.swift:315-358

Takeaway. Use a small scheduling boundary when a framework callback cannot itself be async.
Rationale. The lifecycle coordinator supplies serialization and latest-state convergence, so view callbacks do not need their own task queue.

Key decisions

Explicit ServiceGroup rather than runService

runService() defaults to SIGTERM/SIGINT sequences. The branch constructs a ServiceGroup with empty signal arrays and drives teardown with triggerGracefulShutdown(), avoiding a process-wide signal disposition change in the macOS app.

Latest requested lifecycle state wins

Requests that arrive during an awaited teardown overwrite intermediate state. Reconciliation checks the state again before launch and before exit, preventing stale stop/start commands from racing independent listeners.

Five-second maximum graceful phase

The policy escalates a service that ignores graceful shutdown to cancellation after five seconds. It intentionally does not configure the library's cancellation timeout because that path terminates with fatalError; total completion therefore still relies on cancellation cooperation.

(inferred — not stated by the author.)

Review findings

SeverityAreaFindingResolution
minorShutdown guarantee wordingSeveral comments/docs can be read as promising a hard bound on total teardown. The configured five seconds bounds the graceful phase; after escalation, a pathological service that never responds to cancellation could still delay task completion.Accepted as a non-blocking documentation precision issue. Production Hummingbird/NIO cancellation is expected to cooperate, and the regression proves escalation occurs.
minorSettings unchanged-port submitPressing Return in the port field calls restart even when the port is unchanged, forcing an avoidable teardown/rebind and dropping active MCP connections.Optional follow-up: call start on submit, which already no-ops for a healthy unchanged port and retries failed/different ports.
minorLifecycle test setup reuseSeven tests repeat makeEnv plus MCPServer construction, and two wait helpers duplicate the same polling loop. The parameterized test also creates retained SwiftData fixtures for each case despite testing only transport lifecycle.Optional test-only cleanup; it does not affect production behavior or regression determinism.
nitCoverage wordingThe timeout and empty-signal checks are structural/synthetic rather than live-loopback behaviors, and the repeated restart test confirms continued availability but cannot alone prove a restart was not optimized into a no-op.The combined suite still directly covers port release, changed-port replacement, probe semantics, bind failure, timeout escalation, and signal configuration. Further instrumentation is optional.

Per-file diffs

Click to expand.

CHANGELOG.md Modified +2 / -0
diff --git a/CHANGELOG.md b/CHANGELOG.mdindex a6d0a14..5fcdab8 100644--- a/CHANGELOG.md+++ b/CHANGELOG.md@@ -8,6 +8,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/).  ### Fixed +- MCP server start, stop, and same-port restart now run through one async desired-state lifecycle coordinator (T-1826). Each active listener owns its Hummingbird `ServiceGroup` and run task; teardown explicitly triggers graceful shutdown and awaits the group before rebinding, rather than assuming cancellation completion releases the socket. Graceful shutdown is time-bounded so a stalled request cannot wedge the coordinator, and the group installs no SIGTERM/SIGINT traps (`runService()`'s default) because those change the process's signal disposition permanently. Requests arriving during teardown replace the pending target so a burst converges on the latest state, Settings uses an explicit restart operation, and live loopback regressions cover stop, 20 serialized same-port repetitions, different-port restart, rapid off/on, an occupied port surfacing the bind failure, an invalid port releasing the running listener, bounded shutdown escalation, empty signal configuration, and reusable bind-probe behavior.+ - The MCP Streamable HTTP endpoint now accepts non-empty JSON-RPC request/notification batches for advertised protocol `2025-03-26` (T-1834). It decodes each member independently, dispatches requests and notifications sequentially, omits notification responses, preserves response-array semantics, returns HTTP 202 with no body for all-notification batches, emits per-member `-32600` errors for invalid entries, handles empty batches as one invalid-request object, and rejects lifecycle-invalid batched `initialize` calls. Route-level regression tests cover each behavior and unchanged single-request responses. - MCP `create_task`, `CreateTaskIntent`, and `AddTaskSheet.persist` now create a task plus optional milestone atomically in one `TaskService` save (T-1768). The relationship is validated and attached before insertion; failed saves delete the pending aggregate instead of relying on a second, fallible compensating deletion. Surface-level failure regressions verify no task remains or is resurrected by a later save. `CreateTaskIntent` maps the merged save's failures per error kind, so a storage failure that was previously reported as `INTERNAL_ERROR` by the separate milestone-assignment step still reports `INTERNAL_ERROR` rather than collapsing into `INVALID_INPUT`; `milestoneProjectMismatch` from the service boundary surfaces as `MILESTONE_PROJECT_MISMATCH`. - Duplicate cleanup now re-probes a task or milestone loser's committed display ID after asynchronous allocation and immediately before mutation (T-2019). Peer changes that land during allocation are preserved and reported as `stale-id`; task cleanup no longer emits a false audit comment for an overwritten peer repair. Deterministic gated regressions cover both record types and the task audit path.
Transit/Transit/MCP/MCPServer.swift Modified +151 / -54
diff --git a/Transit/Transit/MCP/MCPServer.swift b/Transit/Transit/MCP/MCPServer.swiftindex 6821491..2ac314d 100644--- a/Transit/Transit/MCP/MCPServer.swift+++ b/Transit/Transit/MCP/MCPServer.swift@@ -3,15 +3,32 @@ import Foundation import Hummingbird import NIOCore import NIOFoundationCompat+import ServiceLifecycle  @MainActor @Observable final class MCPServer { +    private enum DesiredState: Equatable {+        case stopped+        case invalidPort(Int)+        case running(port: Int, runID: Int)+    }++    private struct ActiveServer {+        let port: Int+        let runID: Int+        let serviceGroup: ServiceGroup+        let task: Task<Void, Never>+    }+     private let toolHandler: MCPToolHandler-    private var serverTask: Task<Void, Never>?-    private var generation = 0+    private var desiredState: DesiredState = .stopped+    private var lifecycleTask: Task<Void, Never>?+    private var activeServer: ActiveServer?+    private var nextRunID = 0+    private var serverGeneration = 0 -    private(set) var isRunning = false+    var isRunning: Bool { activeServer != nil }      /// A human-readable reason the server is not running, or `nil` when the     /// server is running or was stopped intentionally. Set when the configured@@ -23,62 +40,163 @@ final class MCPServer {     init(toolHandler: MCPToolHandler) {         self.toolHandler = toolHandler     }+}++extension MCPServer {+    /// Starts the server unless the same port is already active or requested.+    /// Repeated requests join one reconciliation pass instead of launching+    /// competing Hummingbird applications.+    func start(port: Int) async {+        await request(runningState(port: port, forceRestart: false))+    }++    /// Replaces the current listener even when the port is unchanged. The old+    /// service group shuts down gracefully before the replacement can bind.+    func restart(port: Int) async {+        await request(runningState(port: port, forceRestart: true))+    } -    func start(port: Int) {-        guard !isRunning else { return }+    /// Stops the server and returns only after Hummingbird has gracefully+    /// released its listener. A newer request can supersede this one meanwhile.+    func stop() async {+        await request(.stopped)+    } -        // Reject out-of-range ports before attempting to bind. The OS treats-        // these as errors asynchronously (or, for 0, silently picks a random-        // port), neither of which is useful for a fixed local endpoint.+    private func runningState(port: Int, forceRestart: Bool) -> DesiredState {         guard MCPSettings.isValidPort(port) else {-            isRunning = false-            startError = "Port \(port) is invalid. Use a value between "-                + "\(MCPSettings.validPortRange.lowerBound) and "-                + "\(MCPSettings.validPortRange.upperBound)."+            return .invalidPort(port)+        }++        if !forceRestart {+            if case .running(let requestedPort, let runID) = desiredState,+               requestedPort == port {+                return .running(port: port, runID: runID)+            }+            if let activeServer, activeServer.port == port {+                return .running(port: port, runID: activeServer.runID)+            }+        }++        nextRunID += 1+        return .running(port: port, runID: nextRunID)+    }++    private func request(_ state: DesiredState) async {+        desiredState = state+        if lifecycleTask == nil {+            lifecycleTask = Task { @MainActor [weak self] in+                await self?.reconcileLifecycle()+            }+        }+        let currentLifecycleTask = lifecycleTask+        await currentLifecycleTask?.value+    }++    /// Applies only the latest requested state. Requests arriving while old+    /// listener teardown is suspended overwrite stale intermediate states and+    /// are picked up by the next loop iteration.+    private func reconcileLifecycle() async {+        while true {+            let target = desiredState+            switch target {+            case .stopped:+                await tearDownCurrentServer()+                if desiredState == target {+                    startError = nil+                }++            case .invalidPort(let port):+                await tearDownCurrentServer()+                if desiredState == target {+                    startError = "Port \(port) is invalid. Use a value between "+                        + "\(MCPSettings.validPortRange.lowerBound) and "+                        + "\(MCPSettings.validPortRange.upperBound)."+                }++            case .running(let port, let runID):+                if activeServer?.port != port || activeServer?.runID != runID {+                    await tearDownCurrentServer()+                    if desiredState == target {+                        launchServer(port: port, runID: runID)+                    }+                }+            }++            guard desiredState == target else { continue }+            lifecycleTask = nil             return         }+    }++    private func tearDownCurrentServer() async {+        guard let currentServer = activeServer else { return }++        // Task cancellation only cancels ServiceGroup's child tasks; it does+        // not invoke Hummingbird Server.shutdownGracefully(), so task completion+        // is not a listener-release fence. Trigger the group's graceful path,+        // then await its run task before permitting a replacement bind. The+        // generation bump must precede the trigger: if the detached task has+        // not reached `run()` yet, the group jumps to `.finished` and `run()`+        // throws `alreadyFinished`, which the fenced callback must not report+        // as a start failure for a server the caller just asked to stop.+        serverGeneration += 1+        activeServer = nil+        await currentServer.serviceGroup.triggerGracefulShutdown()+        await currentServer.task.value+    } -        generation += 1-        let currentGeneration = generation-        isRunning = true+    private func launchServer(port: Int, runID: Int) {+        serverGeneration += 1+        let currentGeneration = serverGeneration         startError = nil          let handler = toolHandler         let setNotRunning = { @MainActor [weak self] (failure: String?) in-            guard let self, self.generation == currentGeneration else { return }-            self.isRunning = false+            guard let self, self.serverGeneration == currentGeneration else { return }+            self.activeServer = nil             if let failure {                 self.startError = failure             }         }-        serverTask = Task.detached {-            let app = Application(-                router: Self.makeRouter(handler: handler),-                configuration: .init(-                    address: .hostname("127.0.0.1", port: port)-                )+        let app = Application(+            router: Self.makeRouter(handler: handler),+            configuration: .init(+                address: .hostname("127.0.0.1", port: port)             )--            // A bind failure surfaces here (e.g. the port is already in use).-            // `CancellationError` is the expected path when `stop()` cancels the-            // task and must not be reported as a failure.+        )+        // No unix signal traps (`runService()` would install SIGTERM/SIGINT by+        // default): they change the process's signal disposition permanently,+        // and teardown here is driven by `triggerGracefulShutdown()`.+        let configuration = MCPServerLifecycleConfiguration.make(+            services: [app],+            logger: app.logger+        )+        let serviceGroup = ServiceGroup(configuration: configuration)+        let task = Task.detached {             var failure: String?             do {-                try await app.runService()-            } catch is CancellationError {-                failure = nil+                try await serviceGroup.run()             } catch {                 failure = "Could not start server on port \(port): "                     + error.localizedDescription             }             await setNotRunning(failure)         }+        activeServer = ActiveServer(+            port: port,+            runID: runID,+            serviceGroup: serviceGroup,+            task: task+        )     }+}++extension MCPServer {      /// Builds the Hummingbird router for the single `POST /mcp` endpoint.-    /// `nonisolated` because under `SWIFT_DEFAULT_ACTOR_ISOLATION = MainActor`-    /// `MCPServer` is `@MainActor` by default, but this is called from the-    /// detached (non-main-actor) task in `start(port:)`.+    /// `nonisolated` keeps transport construction independent of MainActor;+    /// route callbacks execute on Hummingbird/NIO and hop to MainActor only+    /// when dispatching through `MCPToolHandler`.     nonisolated static func makeRouter(handler: MCPToolHandler) -> Router<BasicRequestContext> {         let router = Router()         router.post("mcp") { request, _ -> Response in@@ -153,29 +271,8 @@ final class MCPServer {         return jsonResponse(responses)     } -    func stop() {-        serverTask?.cancel()-        serverTask = nil-        isRunning = false-        startError = nil-    }-     // MARK: - Helpers -    /// Result of decoding an incoming body as one JSON-RPC request or a batch.-    nonisolated enum DecodeOutcome: Sendable {-        case success(JSONRPCRequest)-        case batch([BatchElement])-        case failure(JSONRPCResponse)-    }--    /// One member of a non-empty JSON-RPC batch. Invalid members stay in the-    /// sequence so the route can emit one -32600 response for each of them.-    nonisolated enum BatchElement: Sendable {-        case request(JSONRPCRequest)-        case invalid-    }-     /// Decode an incoming request body as a single JSON-RPC request or a     /// non-empty JSON-RPC batch, distinguishing transport-level parse failures     /// from protocol-level shape failures.
Transit/Transit/MCP/MCPServerDecodeTypes.swift Added +24 / -0
diff --git a/Transit/Transit/MCP/MCPServerDecodeTypes.swift b/Transit/Transit/MCP/MCPServerDecodeTypes.swiftnew file mode 100644index 0000000..e58824e--- /dev/null+++ b/Transit/Transit/MCP/MCPServerDecodeTypes.swift@@ -0,0 +1,24 @@+#if os(macOS)++// Nested in `MCPServer` rather than declared at module scope: `DecodeOutcome`+// and `BatchElement` are generic enough names that they would collide badly in+// the app namespace, and they only describe this server's decode step. They sit+// in their own file to keep `MCPServer.swift` under the file-length limit.+extension MCPServer {++    /// Result of decoding an incoming body as one JSON-RPC request or a batch.+    nonisolated enum DecodeOutcome: Sendable {+        case success(JSONRPCRequest)+        case batch([BatchElement])+        case failure(JSONRPCResponse)+    }++    /// One member of a non-empty JSON-RPC batch. Invalid members stay in the+    /// sequence so the route can emit one -32600 response for each of them.+    nonisolated enum BatchElement: Sendable {+        case request(JSONRPCRequest)+        case invalid+    }+}++#endif
Transit/Transit/MCP/MCPServerLifecycleConfiguration.swift Added +24 / -0
diff --git a/Transit/Transit/MCP/MCPServerLifecycleConfiguration.swift b/Transit/Transit/MCP/MCPServerLifecycleConfiguration.swiftnew file mode 100644index 0000000..62aaf6a--- /dev/null+++ b/Transit/Transit/MCP/MCPServerLifecycleConfiguration.swift@@ -0,0 +1,24 @@+#if os(macOS)+import Logging+import ServiceLifecycle++/// Service Lifecycle policy for the embedded server. Keeping this separate from+/// `MCPServer` makes the timeout and no-signal contract directly testable.+nonisolated enum MCPServerLifecycleConfiguration {+    static let defaultMaximumGracefulShutdown: Duration = .seconds(5)++    static func make(+        services: [any Service],+        logger: Logger,+        maximumGracefulShutdown: Duration = defaultMaximumGracefulShutdown+    ) -> ServiceGroupConfiguration {+        var configuration = ServiceGroupConfiguration(+            services: services,+            logger: logger+        )+        configuration.maximumGracefulShutdownDuration = maximumGracefulShutdown+        return configuration+    }+}++#endif
Transit/Transit/TransitApp.swift Modified +3 / -3
diff --git a/Transit/Transit/TransitApp.swift b/Transit/Transit/TransitApp.swiftindex db622e6..55d2bf5 100644--- a/Transit/Transit/TransitApp.swift+++ b/Transit/Transit/TransitApp.swift@@ -208,7 +208,7 @@ struct TransitApp: App {             #if os(macOS)             .environment(mcpSettings)             .environment(mcpServer)-            .task { startMCPServerIfEnabled() }+            .task { await startMCPServerIfEnabled() }             #endif             .task { seedUITestDataIfNeeded() }             .alert(@@ -281,10 +281,10 @@ struct TransitApp: App {     // MARK: - MCP Server      #if os(macOS)-    private func startMCPServerIfEnabled() {+    private func startMCPServerIfEnabled() async {         // Skip MCP server in unit test host to avoid port conflicts across test runs         guard mcpSettings.isEnabled, !Self.isUnitTestHost else { return }-        mcpServer.start(port: mcpSettings.port)+        await mcpServer.start(port: mcpSettings.port)         syncManager.startHeartbeat(context: container.mainContext)     }     #endif
Transit/Transit/Views/Settings/SettingsView.swift Modified +22 / -4
diff --git a/Transit/Transit/Views/Settings/SettingsView.swift b/Transit/Transit/Views/Settings/SettingsView.swiftindex 9b9edb2..4dee736 100644--- a/Transit/Transit/Views/Settings/SettingsView.swift+++ b/Transit/Transit/Views/Settings/SettingsView.swift@@ -312,6 +312,26 @@ extension SettingsView {         }     } +    fileprivate enum MCPLifecycleRequest {+        case start+        case stop+        case restart+    }++    /// Submits a lifecycle request without tracking it. `MCPServer` already+    /// coalesces overlapping requests onto its latest desired state, so the+    /// view has nothing useful to cancel.+    fileprivate func scheduleMCP(_ request: MCPLifecycleRequest) {+        let port = mcpSettings.port+        Task { @MainActor in+            switch request {+            case .start: await mcpServer.start(port: port)+            case .stop: await mcpServer.stop()+            case .restart: await mcpServer.restart(port: port)+            }+        }+    }+     fileprivate var macOSMCPSection: some View {         @Bindable var settings = mcpSettings         return LiquidGlassSection {@@ -321,11 +341,10 @@ extension SettingsView {                         .labelsHidden()                         .toggleStyle(.switch)                         .onChange(of: mcpSettings.isEnabled) { _, enabled in+                            scheduleMCP(enabled ? .start : .stop)                             if enabled {-                                mcpServer.start(port: mcpSettings.port)                                 syncManager.startHeartbeat(context: modelContext)                             } else {-                                mcpServer.stop()                                 syncManager.stopHeartbeat()                             }                         }@@ -335,8 +354,7 @@ extension SettingsView {                         TextField("", value: $settings.port, format: .number)                             .frame(width: 80)                             .onSubmit {-                                mcpServer.stop()-                                mcpServer.start(port: mcpSettings.port)+                                scheduleMCP(.restart)                             }                     }                     FormRow("Status", labelWidth: Self.labelWidth) {
Transit/TransitTests/MCPServerLifecycleTests.swift Added +373 / -0
diff --git a/Transit/TransitTests/MCPServerLifecycleTests.swift b/Transit/TransitTests/MCPServerLifecycleTests.swiftnew file mode 100644index 0000000..75f1093--- /dev/null+++ b/Transit/TransitTests/MCPServerLifecycleTests.swift@@ -0,0 +1,373 @@+#if os(macOS)+import Darwin+import Foundation+import Logging+import ServiceLifecycle+import Testing+@testable import Transit++/// Regression tests for T-1826: listener lifecycle changes must serialize+/// explicit graceful shutdown before another Hummingbird application binds.+///+/// The original implementation discarded a cancelled task immediately. The+/// first reviewed fix awaited that task, but Swift Service Lifecycle handles+/// task cancellation by cancelling children rather than invoking Hummingbird's+/// listener-closing graceful-shutdown path. Both versions could race a rebind.+@MainActor @Suite(.serialized)+struct MCPServerLifecycleTests {++    @Test func stopReturnsOnlyAfterListenerReleasesPort() async throws {+        let env = try MCPTestHelpers.makeEnv()+        let server = MCPServer(toolHandler: env.handler)+        let port = try availableLoopbackPort()++        await server.start(port: port)+        try #require(await waitUntilListening(port: port), "Initial listener did not start")++        // Awaiting stop must mean teardown is complete, not merely requested.+        // The pre-fix synchronous stop returns before Hummingbird closes the+        // listener, so this immediate same-port bind fails with EADDRINUSE.+        await server.stop()++        let bindError = bindLoopbackPort(port)+        #expect(bindError == nil, "Listener still owns port after stop returned (errno \(bindError ?? 0))")+    }++    @Test(arguments: 1...20)+    func samePortRestartWaitsForOldListenerTeardown(repetition: Int) async throws {+        let env = try MCPTestHelpers.makeEnv()+        let server = MCPServer(toolHandler: env.handler)+        let port = try availableLoopbackPort()++        await server.start(port: port)+        try #require(+            await waitUntilListening(port: port),+            "Initial listener did not start in repetition \(repetition)"+        )++        await server.restart(port: port)++        #expect(+            await waitUntilListening(port: port),+            "Same-port restart repetition \(repetition) raced listener shutdown: \(server.startError ?? "no error")"+        )+        #expect(server.startError == nil)+        await server.stop()+    }++    @Test func differentPortRestartReleasesOldListenerBeforeBindingNewOne() async throws {+        let env = try MCPTestHelpers.makeEnv()+        let server = MCPServer(toolHandler: env.handler)+        let oldPort = try availableLoopbackPort()+        var newPort = try availableLoopbackPort()+        while newPort == oldPort {+            newPort = try availableLoopbackPort()+        }++        await server.start(port: oldPort)+        try #require(await waitUntilListening(port: oldPort), "Initial listener did not start")++        await server.restart(port: newPort)++        #expect(bindLoopbackPort(oldPort) == nil, "Restart must release the old port")+        #expect(+            await waitUntilListening(port: newPort),+            "Different-port restart did not bind the replacement: \(server.startError ?? "no error")"+        )+        #expect(server.startError == nil)+        await server.stop()+    }++    @Test func rapidOffOnRequestsCoalesceWithoutAddressInUse() async throws {+        let env = try MCPTestHelpers.makeEnv()+        let server = MCPServer(toolHandler: env.handler)+        let port = try availableLoopbackPort()++        await server.start(port: port)+        try #require(await waitUntilListening(port: port), "Initial listener did not start")++        var requests: [Task<Void, Never>] = []+        for _ in 0..<8 {+            requests.append(Task { @MainActor in await server.stop() })+            requests.append(Task { @MainActor in await server.start(port: port) })+        }+        for request in requests {+            await request.value+        }+        // Make the final desired state explicit; the burst above is allowed to+        // coalesce intermediate requests, but must not leave an old bind alive.+        await server.start(port: port)++        #expect(+            await waitUntilListening(port: port),+            "Rapid off/on must converge on one running listener: \(server.startError ?? "no error")"+        )+        #expect(server.isRunning)+        #expect(server.startError == nil)+        await server.stop()+    }++    @Test func lifecycleConfigurationBoundsShutdownWithoutSignalTraps() {+        let configuration = MCPServerLifecycleConfiguration.make(+            services: [],+            logger: Logger(label: "MCPServerLifecycleTests")+        )++        #expect(+            configuration.maximumGracefulShutdownDuration+                == MCPServerLifecycleConfiguration.defaultMaximumGracefulShutdown+        )+        #expect(configuration.gracefulShutdownSignals.isEmpty)+        #expect(configuration.cancellationSignals.isEmpty)+    }++    @Test func gracefulShutdownTimeoutEscalatesToCancellation() async throws {+        let state = CancellationOnlyServiceState()+        let service = CancellationOnlyService(state: state)+        let configuration = MCPServerLifecycleConfiguration.make(+            services: [service],+            logger: Logger(label: "MCPServerLifecycleTests.timeout"),+            maximumGracefulShutdown: .milliseconds(50)+        )+        let serviceGroup = ServiceGroup(configuration: configuration)+        let runTask = Task { try await serviceGroup.run() }+        await state.waitUntilStarted()++        let clock = ContinuousClock()+        let startedAt = clock.now+        await serviceGroup.triggerGracefulShutdown()+        try await runTask.value++        #expect(clock.now - startedAt < .seconds(1))+        let cancellationObserved = await state.cancellationObserved+        #expect(cancellationObserved)+    }++    @Test func reusableBindProbeDistinguishesLiveListenerFromReleasedPort() async throws {+        let env = try MCPTestHelpers.makeEnv()+        let server = MCPServer(toolHandler: env.handler)+        let port = try availableLoopbackPort()++        await server.start(port: port)+        try #require(await waitUntilListening(port: port), "Initial listener did not start")++        #expect(+            bindLoopbackPort(port) != nil,+            "SO_REUSEADDR probe must still reject a live listener"+        )+        await server.stop()+        #expect(+            bindLoopbackPort(port) == nil,+            "SO_REUSEADDR probe must accept a released port despite readiness-probe TIME_WAIT"+        )+    }++    @Test func startOnOccupiedPortSurfacesBindFailure() async throws {+        let env = try MCPTestHelpers.makeEnv()+        let server = MCPServer(toolHandler: env.handler)+        let port = try availableLoopbackPort()++        // An unrelated listener holds the port, which is the failure the+        // asynchronous bind path must report rather than silently claim to run.+        let blocker = try occupyLoopbackPort(port)+        defer { close(blocker) }++        await server.start(port: port)++        #expect(+            await waitUntilStartFails(server),+            "A bind failure on an occupied port must surface through startError"+        )+        #expect(!server.isRunning)+        await server.stop()+    }++    @Test func invalidPortRequestReleasesRunningListener() async throws {+        let env = try MCPTestHelpers.makeEnv()+        let server = MCPServer(toolHandler: env.handler)+        let port = try availableLoopbackPort()++        await server.start(port: port)+        try #require(await waitUntilListening(port: port), "Initial listener did not start")++        // An invalid port is a desired state rather than an early return, so it+        // must tear the running listener down instead of leaving it bound.+        await server.start(port: 70000)++        #expect(!server.isRunning)+        #expect(server.startError != nil)+        #expect(+            bindLoopbackPort(port) == nil,+            "Invalid-port request must release the previous listener"+        )+    }++    private func waitUntilListening(+        port: Int,+        timeout: Duration = .seconds(3)+    ) async -> Bool {+        let deadline = ContinuousClock.now + timeout+        while ContinuousClock.now < deadline {+            if canConnectToLoopbackPort(port) {+                return true+            }+            try? await Task.sleep(for: .milliseconds(20))+        }+        return false+    }++    private func waitUntilStartFails(+        _ server: MCPServer,+        timeout: Duration = .seconds(3)+    ) async -> Bool {+        let deadline = ContinuousClock.now + timeout+        while ContinuousClock.now < deadline {+            if server.startError != nil {+                return true+            }+            try? await Task.sleep(for: .milliseconds(20))+        }+        return false+    }++    private func loopbackAddress(port: Int) -> sockaddr_in {+        var address = sockaddr_in()+        address.sin_len = UInt8(MemoryLayout<sockaddr_in>.size)+        address.sin_family = sa_family_t(AF_INET)+        address.sin_port = UInt16(port).bigEndian+        address.sin_addr = in_addr(s_addr: inet_addr("127.0.0.1"))+        return address+    }++    private func withSocketAddress<T>(+        _ address: inout sockaddr_in,+        _ body: (UnsafePointer<sockaddr>, socklen_t) -> T+    ) -> T {+        withUnsafePointer(to: &address) { pointer in+            pointer.withMemoryRebound(to: sockaddr.self, capacity: 1) {+                body($0, socklen_t(MemoryLayout<sockaddr_in>.size))+            }+        }+    }++    private func canConnectToLoopbackPort(_ port: Int) -> Bool {+        let descriptor = socket(AF_INET, SOCK_STREAM, 0)+        guard descriptor >= 0 else { return false }+        defer { close(descriptor) }++        var address = loopbackAddress(port: port)+        return withSocketAddress(&address) { connect(descriptor, $0, $1) } == 0+    }++    /// Probes whether `port` is bindable again. `SO_REUSEADDR` mirrors what+    /// SwiftNIO sets on its listening socket, so a connection the readiness+    /// probe left in `TIME_WAIT` does not masquerade as "the old listener still+    /// owns the port". A live listener still rejects the bind on Darwin, which+    /// is the signal these tests rely on.+    private func bindLoopbackPort(_ port: Int) -> Int32? {+        let descriptor = socket(AF_INET, SOCK_STREAM, 0)+        guard descriptor >= 0 else { return errno }+        defer { close(descriptor) }+        enableAddressReuse(descriptor)++        var address = loopbackAddress(port: port)+        return withSocketAddress(&address) { bind(descriptor, $0, $1) } == 0 ? nil : errno+    }++    /// Holds `port` with a listening socket so a Hummingbird bind must fail.+    /// The caller owns the returned descriptor and must `close` it.+    private func occupyLoopbackPort(_ port: Int) throws -> Int32 {+        let descriptor = socket(AF_INET, SOCK_STREAM, 0)+        guard descriptor >= 0 else {+            throw LifecycleTestError.socketCreationFailed(errno)+        }++        var address = loopbackAddress(port: port)+        let bindResult = withSocketAddress(&address) { bind(descriptor, $0, $1) }+        guard bindResult == 0, listen(descriptor, 1) == 0 else {+            let failure = errno+            close(descriptor)+            throw LifecycleTestError.bindFailed(failure)+        }+        return descriptor+    }++    private func enableAddressReuse(_ descriptor: Int32) {+        var enabled: Int32 = 1+        _ = setsockopt(+            descriptor,+            SOL_SOCKET,+            SO_REUSEADDR,+            &enabled,+            socklen_t(MemoryLayout<Int32>.size)+        )+    }++    private func availableLoopbackPort() throws -> Int {+        let descriptor = socket(AF_INET, SOCK_STREAM, 0)+        guard descriptor >= 0 else {+            throw LifecycleTestError.socketCreationFailed(errno)+        }+        defer { close(descriptor) }++        var address = loopbackAddress(port: 0)+        guard withSocketAddress(&address, { bind(descriptor, $0, $1) }) == 0 else {+            throw LifecycleTestError.bindFailed(errno)+        }++        var length = socklen_t(MemoryLayout<sockaddr_in>.size)+        let nameResult = withUnsafeMutablePointer(to: &address) { pointer in+            pointer.withMemoryRebound(to: sockaddr.self, capacity: 1) {+                getsockname(descriptor, $0, &length)+            }+        }+        guard nameResult == 0 else {+            throw LifecycleTestError.socketNameFailed(errno)+        }+        return Int(UInt16(bigEndian: address.sin_port))+    }+}++private enum LifecycleTestError: Error {+    case socketCreationFailed(Int32)+    case bindFailed(Int32)+    case socketNameFailed(Int32)+}++private nonisolated struct CancellationOnlyService: Service {+    let state: CancellationOnlyServiceState++    func run() async throws {+        await state.markStarted()+        do {+            try await Task.sleep(for: .seconds(60))+        } catch is CancellationError {+            await state.markCancellationObserved()+        }+    }+}++private actor CancellationOnlyServiceState {+    private var started = false+    private var startContinuation: CheckedContinuation<Void, Never>?+    private(set) var cancellationObserved = false++    func waitUntilStarted() async {+        guard !started else { return }+        await withCheckedContinuation { continuation in+            startContinuation = continuation+        }+    }++    func markStarted() {+        started = true+        startContinuation?.resume()+        startContinuation = nil+    }++    func markCancellationObserved() {+        cancellationObserved = true+    }+}++#endif
Transit/TransitTests/MCPServerStartFailureTests.swift Modified +13 / -14
diff --git a/Transit/TransitTests/MCPServerStartFailureTests.swift b/Transit/TransitTests/MCPServerStartFailureTests.swiftindex 5a1cdcb..643206d 100644--- a/Transit/TransitTests/MCPServerStartFailureTests.swift+++ b/Transit/TransitTests/MCPServerStartFailureTests.swift@@ -17,11 +17,10 @@ import Testing ///   and records a descriptive `startError` instead of just flipping `isRunning`. /// - Starting with a valid, available port clears any prior error. ///-/// `start(port:)` clears `startError` synchronously before launching the-/// detached service task, so a successful restart clearing a prior error is-/// not covered here: asserting it would require an actual socket bind, which is-/// flaky in CI. The synchronous clear is exercised indirectly via the-/// invalid-port tests (which observe the post-clear error being re-set).+/// Invalid-port state is applied before the async lifecycle call returns. A+/// successful valid-port restart now uses a real loopback listener and is+/// covered by `MCPServerLifecycleTests` rather than assumed from synchronous+/// state mutation. @MainActor @Suite(.serialized) struct MCPServerStartFailureTests { @@ -49,38 +48,38 @@ struct MCPServerStartFailureTests {      // MARK: - Server start surfaces invalid-port failures -    @Test func startWithInvalidPortDoesNotRunAndSetsError() throws {+    @Test func startWithInvalidPortDoesNotRunAndSetsError() async throws {         let env = try MCPTestHelpers.makeEnv()         let server = MCPServer(toolHandler: env.handler) -        server.start(port: 70000)+        await server.start(port: 70000)          #expect(!server.isRunning)         #expect(server.startError != nil)-        server.stop()+        await server.stop()     } -    @Test func startWithInvalidPortDoesNotLeaveRunningTrue() throws {+    @Test func startWithInvalidPortDoesNotLeaveRunningTrue() async throws {         let env = try MCPTestHelpers.makeEnv()         let server = MCPServer(toolHandler: env.handler) -        server.start(port: 0)+        await server.start(port: 0)          #expect(!server.isRunning)         #expect(server.startError != nil)-        server.stop()+        await server.stop()     }      // MARK: - Stop clears error state -    @Test func stopClearsStartError() throws {+    @Test func stopClearsStartError() async throws {         let env = try MCPTestHelpers.makeEnv()         let server = MCPServer(toolHandler: env.handler) -        server.start(port: 70000)+        await server.start(port: 70000)         #expect(server.startError != nil) -        server.stop()+        await server.stop()         #expect(server.startError == nil)     } }
docs/agent-notes/mcp-server.md Modified +8 / -3
diff --git a/docs/agent-notes/mcp-server.md b/docs/agent-notes/mcp-server.mdindex 5ef3923..9bc279c 100644--- a/docs/agent-notes/mcp-server.md+++ b/docs/agent-notes/mcp-server.md@@ -21,6 +21,8 @@ Claude Code ←→ HTTP POST /mcp (localhost:3141) ←→ MCPServer ←→ MCPTo - `Transit/Transit/MCP/MCPToolHandler.swift` — Tool dispatch, handler methods, helpers - `Transit/Transit/MCP/MCPToolDefinitions.swift` — Tool schema definitions (extracted for file length) - `Transit/Transit/MCP/MCPServer.swift` — Hummingbird server lifecycle and HTTP routing+- `Transit/Transit/MCP/MCPServerLifecycleConfiguration.swift` — bounded shutdown and no-signal `ServiceGroup` policy+- `Transit/Transit/MCP/MCPServerDecodeTypes.swift` — request decode outcome types shared by the route and decoder - `Transit/Transit/MCP/MCPOriginValidator.swift` — transport-level `Origin`/`Host` validation - `Transit/TransitTests/MCPToolHandlerTests.swift` — Unit tests @@ -30,7 +32,7 @@ Key challenge: Hummingbird runs on SwiftNIO event loops (nonisolated), but servi  1. **MCP protocol types** (`MCPTypes.swift`): All marked `nonisolated` and `Sendable` to opt out of the project's default `@MainActor` isolation. Without this, `Encodable`/`Decodable` conformances become MainActor-isolated and can't be used from NIO threads. 2. **MCPToolHandler**: `@MainActor` class — holds service references, handles tool dispatch.-3. **MCPServer**: `@MainActor @Observable` for SwiftUI environment. Runs Hummingbird in `Task.detached`. Route handler calls `await handler.handle(request)` — Swift concurrency automatically hops to MainActor.+3. **MCPServer**: `@MainActor @Observable` for SwiftUI environment. A single desired-state lifecycle task serializes start/stop/restart requests. Each active listener retains both its Hummingbird `ServiceGroup` and detached run task; teardown calls `triggerGracefulShutdown()` and then awaits the run task before any replacement bind. Cancelling and awaiting the task is insufficient because Service Lifecycle's cancellation path cancels children instead of invoking Hummingbird's listener-closing `Server.shutdownGracefully()`. Two configuration choices matter: `maximumGracefulShutdownDuration` is set so an in-flight request that never completes cannot suspend `run()` forever and wedge the single lifecycle task, and `gracefulShutdownSignals` is left empty because `runService()`'s SIGTERM/SIGINT default permanently changes the whole process's signal disposition — undesirable in a GUI app, and unnecessary since teardown is driven by `triggerGracefulShutdown()`. Requests that arrive while teardown is suspended overwrite the pending target rather than queueing, so a burst converges on the latest state. Route handlers call `await handler.handle(request)` to hop to MainActor. 4. **AnyCodable**: Uses `@unchecked Sendable` because it wraps `Any`.  ## Tools Exposed@@ -105,9 +107,12 @@ Batch dispatch is sequential on the MainActor to keep mutations against the shar  ## Dependencies -- **Hummingbird 2.x** — Transit's first (and only) external SPM dependency+- **Hummingbird 2.x** — Transit's only *declared* external SPM dependency - Added via Xcode SPM integration in project.pbxproj-- Also pulls in SwiftNIO transitively+- Also pulls in SwiftNIO and swift-service-lifecycle transitively+- `MCPServer.swift` imports `ServiceLifecycle` directly (for `ServiceGroup`), so+  that transitive module is effectively a source-level dependency even though it+  is not declared as a package product on the target  ## Entitlements 
specs/bugfixes/mcp-same-port-restart-race/implementation.md Added +75 / -0
diff --git a/specs/bugfixes/mcp-same-port-restart-race/implementation.md b/specs/bugfixes/mcp-same-port-restart-race/implementation.mdnew file mode 100644index 0000000..2c17732--- /dev/null+++ b/specs/bugfixes/mcp-same-port-restart-race/implementation.md@@ -0,0 +1,75 @@+# T-1826 MCP Lifecycle Fix — Implementation Explanation++## Beginner Level++### What Changed / What This Does++Transit contains a small web server that lets local MCP clients communicate with the app. Previously, turning that server off only *asked* it to stop and immediately started a replacement. The old server could still be holding the network port, so the new server sometimes failed with "address already in use."++The server now has one lifecycle coordinator. It remembers the latest requested state—stopped, invalid port, or running on a particular port—and handles requests one at a time. Each running listener keeps both its Hummingbird service group and the task running that group. Before starting a replacement, Transit tells the group to shut down gracefully and waits for that run task to finish.++### Why It Matters++Same-port changes and quick off/on toggles now leave MCP available instead of randomly stopping it. Users no longer need to retry the setting or restart Transit after this race.++### Key Concepts++- **Task cancellation:** stops asynchronous tasks, but does not necessarily run a framework's graceful resource cleanup.+- **Graceful shutdown:** tells Hummingbird to close its listening channel through its documented service lifecycle.+- **Awaiting teardown:** waiting for the gracefully shutting-down service group to finish before reusing the network port.+- **Converging on the latest request:** replacing several intermediate requests with the newest one, like keeping only the final destination entered into navigation.+- **Timeout as a safety valve:** waiting forever is not a safe default when everything else queues behind that wait.++## Intermediate Level++### Changes Overview++`MCPServer` exposes async `start(port:)`, `stop()`, and `restart(port:)`. Requests update a `DesiredState`; a single MainActor lifecycle task reconciles that state. `ActiveServer` keeps the listener's port, logical run ID, Hummingbird `ServiceGroup`, and detached task running that group as one ownership record. `isRunning` is derived from whether an `ActiveServer` exists rather than tracked as a second field.++`SettingsView` submits requests through one `scheduleMCP(_:)` entry point taking a `.start` / `.stop` / `.restart` case, and holds no request state of its own. `TransitApp` awaits startup from its scene task. The loopback suite covers listener release, 20 parameterized same-port restarts, different-port restart, rapid overlapping requests, a bind failure against an occupied port, an invalid-port request tearing down a running listener, bounded shutdown escalation, empty process-signal configuration, and a reusable bind probe distinguishing a live listener from a released port.++### Implementation Approach++The reconciliation loop snapshots the desired state, applies it, and checks whether another request arrived during an `await`. If so, it loops using the newer state. Teardown invalidates the old completion callback, clears active identity, calls `ServiceGroup.triggerGracefulShutdown()`, and awaits the group's detached `run()` task. Hummingbird's graceful signal reaches `Server.shutdownGracefully()`, which closes the listening channel before `run()` completes.++This is deliberately different from cancelling the run task. Swift Service Lifecycle responds to task cancellation with `group.cancelAll()`; that path does not provide Hummingbird's listener-release guarantee even when the cancelled task is awaited.++A logical run ID distinguishes an explicit restart from an idempotent repeated start. Repeated starts on the active/requested port reuse the run ID, while restart always allocates a new one. This avoids unnecessary churn but preserves explicit restart semantics.++The `ServiceGroup` is constructed explicitly rather than through `Application.runService()`, which changes two defaults. `maximumGracefulShutdownDuration` is set so the group escalates to cancellation instead of waiting indefinitely for accepted connections to close. `gracefulShutdownSignals` is left empty: `runService()` would trap SIGTERM/SIGINT, which sets those signals to `SIG_IGN` process-wide and permanently, and a GUI app has no use for them when teardown is driven by `triggerGracefulShutdown()`.++### Trade-offs++The server keeps a small amount of additional state (`desiredState`, `ActiveServer`, lifecycle task, generation) to make ordering and ownership explicit. This is preferable to delays, which cannot guarantee socket release, and to a FIFO transition queue, which would perform every obsolete toggle instead of converging on the latest state.++The shutdown timeout trades a small worst-case data loss (a client's in-flight request cut short after five seconds) for the guarantee that the coordinator always makes progress. Since the coordinator is a single serialized task, an unbounded wait would not merely delay one stop — it would stall every later start, stop, and restart with no way to recover short of quitting the app.++## Expert Level++### Technical Deep Dive++The coordinator relies on MainActor reentrancy deliberately. `request(_:)` synchronously publishes a new target before awaiting the shared lifecycle task. While `tearDownCurrentServer()` awaits graceful shutdown, later MainActor callers can overwrite `desiredState`; after teardown, the snapshot equality guard prevents stale rebinding and the loop applies only the newest target. The exit sequence — `guard desiredState == target`, `lifecycleTask = nil`, `return` — contains no suspension point, so no request can be published into a coordinator that has already decided to stop.++Two identities solve different races. `runID` expresses desired listener identity and makes an explicit same-port restart observably different from an idempotent start. `serverGeneration` fences detached completion callbacks. Because teardown always awaits the run task, the callback's `activeServer = nil` write is a no-op by the time it runs; what the fence actually suppresses is a bogus `startError`. If teardown runs before the detached task reaches `serviceGroup.run()`, the group transitions straight to `.finished` and the subsequent `run()` throws `ServiceGroupError.alreadyFinished` — which, unfenced, would surface as "Could not start server on port N" for a server the user just asked to stop. Incrementing the generation *before* triggering shutdown is what makes that safe.++A spontaneous bind failure clears active identity but intentionally leaves the desired running state. A later start request on the same port schedules reconciliation again and can retry. Invalid ports are represented as desired state rather than an early return so an invalid restart still tears down the previous listener and reports the validation error deterministically; `invalidPortRequestReleasesRunningListener` pins that behaviour.++### Architecture Impact++Lifecycle ownership is contained in `MCPServer`; UI code requests outcomes rather than composing primitives. The async API makes graceful listener teardown part of the contract, while the internal desired-state loop prevents callers from creating concurrent Hummingbird applications. Because the server coalesces internally, callers need no request bookkeeping — `SettingsView` deliberately fires and forgets. Routing and tool dispatch remain unchanged.++`MCPServer.swift` sits close to the 400-line file-length limit, which is why the decode result types live in `MCPServerDecodeTypes.swift`. They stay nested in an `extension MCPServer` there rather than at module scope, so `DecodeOutcome` and `BatchElement` do not occupy those generic names app-wide.++### Potential Issues++`isRunning` remains optimistic: it becomes true when the listener task launches, not when the bind succeeds. An asynchronous bind failure corrects it through the generation-checked callback, and `startOnOccupiedPortSurfacesBindFailure` now verifies that path against a real occupied port. Callers that need certainty must observe `startError` or probe the port, as the tests do.++The live tests select an ephemeral port by binding port zero and releasing it before Hummingbird starts, which has a theoretical external TOCTOU window but is suitably narrow for serialized local tests. The free-port probe sets `SO_REUSEADDR` to match SwiftNIO's listening socket; without it, a connection the readiness check left in `TIME_WAIT` would report `EADDRINUSE` and fail the test against behaviour production tolerates.++Coalescing is opportunistic, not guaranteed: only requests landing during a single `await` window collapse. A burst still converges, bounded by the request count, but may perform more than one teardown/rebind cycle along the way.++## Completeness Assessment++- **Fully implemented:** async start/stop/restart, explicit graceful shutdown plus awaited group completion, bounded shutdown duration, no process-wide signal traps, active-listener ownership, stale-callback fencing, latest-state convergence, safe Settings and app-start integration, invalid-port behaviour, and live regressions for listener release, 20 same-port repetitions, different-port restart, rapid toggling, occupied-port bind failure, invalid-port teardown, timeout escalation, empty signal configuration, and reusable bind-probe behavior.+- **Partially implemented:** none.+- **Missing:** none for T-1826's scope. Two behaviours are verified indirectly rather than directly — the coalescing *mechanism* (the burst test asserts convergence, which a FIFO queue would also satisfy) and a stop winning a race against an in-flight restart, which the public API cannot deterministically sequence. `SettingsView`'s scheduling helpers remain untested; they are now thin enough that the logic under test lives entirely in `MCPServer`.
specs/bugfixes/mcp-same-port-restart-race/report.md Added +126 / -0
diff --git a/specs/bugfixes/mcp-same-port-restart-race/report.md b/specs/bugfixes/mcp-same-port-restart-race/report.mdnew file mode 100644index 0000000..cbc9c93--- /dev/null+++ b/specs/bugfixes/mcp-same-port-restart-race/report.md@@ -0,0 +1,126 @@+# Bugfix Report: MCP Same-Port Restart Race++**Date:** 2026-08-02+**Status:** Fixed++## Description of the Issue++Restarting the embedded MCP server on its configured port, or quickly toggling it off and on, can leave the server stopped with an address-in-use bind failure.++**Reproduction steps:**+1. Start the MCP server and wait for it to accept requests on its configured loopback port.+2. Submit the same port again, or toggle the server off and immediately back on.+3. Observe that the replacement Hummingbird application can fail to bind because the cancelled listener has not released the socket yet.++**Impact:** MCP clients lose access until a later manual retry or app restart. The failure is timing-dependent and is easiest to trigger from Settings, which currently performs `stop()` and `start()` synchronously in one submit action.++## Investigation Summary++The MCP server lifecycle and every caller were inspected using the repository history, current source, tests, Hummingbird package configuration, and project MCP notes.++- **Symptoms examined:** same-port restart and rapid off/on can produce `EADDRINUSE`; the replacement generation then reports stopped.+- **Code inspected:** `MCPServer.swift`, the macOS MCP Settings section, app-launch startup, existing start-failure tests, and all lifecycle call sites.+- **Hypotheses tested:** the generation guard was checked and ruled out as socket serialization—it only suppresses stale UI state writes. Port validation and Hummingbird routing are unrelated to teardown ordering.++### Systematic inspection findings++1. **Integration/timing defect — `MCPServer.stop()`:** requests cancellation but sets `serverTask` to `nil` immediately. No caller can await Hummingbird listener shutdown.+2. **Data-flow defect — `MCPServer.start(port:)`:** creates a detached service task independently of an old task that may still own the configured port.+3. **Integration defect — `SettingsView.macOSMCPSection`:** port submission invokes `stop()` and `start()` back-to-back in the same synchronous closure.+4. **State-management defect:** repeated lifecycle requests have no serialized coordinator or desired state, so they create competing teardown/rebind operations rather than converging on the latest request.++## Discovered Root Cause++Nothing in the shipped code fenced socket ownership. `stop()` cancelled the service task and discarded it synchronously, so a caller could begin a replacement bind while the old Hummingbird listener still held the port.++**Defect type:** Race condition / asynchronous resource lifecycle.++**Five Whys:**+1. Why can the replacement bind fail? The old listening channel can still own the port when the new `Application` binds.+2. Why is it still holding the port? `stop()` only requested cancellation and returned immediately; no caller could await teardown.+3. Why doesn't awaiting the task fix it either? That was the first attempted fix on this branch, and it is insufficient: awaiting cancellation proves task termination, not that Hummingbird's graceful listener-close path ran.+4. Why does cancellation not run that path? `ServiceGroup` responds to task cancellation with `group.cancelAll()`. Listener closure is only guaranteed by Hummingbird's `Server.shutdownGracefully()`, reached when `ServiceGroup.triggerGracefulShutdown()` signals the service's graceful-shutdown manager.+5. Why was the distinction easy to miss? Both the original code and the first fix treated `Application.runService()` completion as the socket-ownership fence, without checking that the dependency has separate cancellation and graceful-shutdown branches.++**Contributing factors:** `isRunning` is optimistic at task launch, and generation guards prevent stale state changes but cannot release an OS socket.++## Resolution for the Issue++**Changes made:**+- `Transit/Transit/MCP/MCPServer.swift` — retains an `ActiveServer` containing the Hummingbird `ServiceGroup`, run task, port, and logical run ID. Teardown invalidates stale callbacks, calls `triggerGracefulShutdown()`, and awaits the exact group run task before a replacement can bind. No fixed delay or polling is used for teardown. The group sets `maximumGracefulShutdownDuration` so a request that never completes cannot suspend `run()` indefinitely and wedge the single lifecycle task, and installs no `gracefulShutdownSignals` — `runService()`'s SIGTERM/SIGINT default permanently changes the process signal disposition, which a GUI app should not do. `isRunning` is derived from the active listener rather than tracked separately.+- `Transit/Transit/Views/Settings/SettingsView.swift` — submits enabled-state changes asynchronously through one `scheduleMCP(_:)` entry point and uses the explicit same-port `restart(port:)` operation on port submission. The view tracks no request state: `MCPServer` already converges on its latest desired state, so there is nothing useful for the view to cancel.+- `Transit/Transit/TransitApp.swift` — awaits the serialized startup operation from the scene task.+- `Transit/TransitTests/MCPServerLifecycleTests.swift` — exercises real loopback listener release, 20 serialized same-port restart repetitions, different-port restart, overlapping rapid off/on requests, a bind failure against an occupied port, and an invalid-port request releasing a running listener. The free-port probe sets `SO_REUSEADDR`, matching what SwiftNIO sets on its listening socket, so a connection left in `TIME_WAIT` by the readiness probe cannot masquerade as an unreleased listener.+- `Transit/TransitTests/MCPServerStartFailureTests.swift` — awaits the async lifecycle API while preserving invalid-port and error-clearing coverage.++**Approach rationale:** The desired-state loop serializes logical transitions, while explicit `ServiceGroup` ownership supplies the correct resource fence: signal graceful shutdown, then await `run()` completion. Requests arriving during teardown still replace stale targets rather than creating competing listeners.++**Alternatives considered:**+- Add a fixed delay between stop and start — rejected because socket teardown time is not deterministic.+- Cancel and await `Application.runService()` — rejected because Service Lifecycle's cancellation branch is distinct from Hummingbird's graceful listener-close branch.+- Keep generation guards only — rejected because they suppress stale UI updates but do not serialize OS socket ownership.+- Queue every request independently — rejected because bursts would perform unnecessary stop/start cycles instead of converging on the latest state.++## Regression Test++**Test file:** `Transit/TransitTests/MCPServerLifecycleTests.swift`++**Test names:**+- `stopReturnsOnlyAfterListenerReleasesPort`+- `samePortRestartWaitsForOldListenerTeardown` (20 repetitions)+- `differentPortRestartReleasesOldListenerBeforeBindingNewOne`+- `rapidOffOnRequestsCoalesceWithoutAddressInUse`+- `lifecycleConfigurationBoundsShutdownWithoutSignalTraps`+- `gracefulShutdownTimeoutEscalatesToCancellation`+- `reusableBindProbeDistinguishesLiveListenerFromReleasedPort`+- `startOnOccupiedPortSurfacesBindFailure`+- `invalidPortRequestReleasesRunningListener`++**What they verify:** A real loopback Hummingbird listener releases its old port before stop/restart returns, remains reachable after same-port and different-port restart, and converges after a burst of off/on requests without an address-in-use start error. The lifecycle policy has a finite shutdown duration and no process signal traps, and its timeout escalates a non-graceful service to cancellation. The reusable bind probe still rejects a live listener but accepts the released port after a readiness connection. A port held by an unrelated listener surfaces the asynchronous bind failure through `startError` instead of reporting a running server, and an invalid-port request tears the previous listener down rather than returning early.++**Known gap:** the burst test asserts convergence, not the coalescing mechanism — a FIFO queue that performed every transition would pass it too. Deterministically forcing a stop to win a race against an in-flight restart is not expressible through the public API, so that ordering is unverified.++**Run command:** `make test-quick`++## Affected Files++| File | Change |+|------|--------|+| `Transit/Transit/MCP/MCPServer.swift` | Async serialized lifecycle with explicit `ServiceGroup` graceful teardown |+| `Transit/Transit/MCP/MCPServerLifecycleConfiguration.swift` | Bounded graceful-shutdown and no-signal service-group policy |+| `Transit/Transit/MCP/MCPServerDecodeTypes.swift` | Decode helper types moved out to preserve the source file length limit, still nested in `MCPServer` |+| `Transit/Transit/Views/Settings/SettingsView.swift` | Safe enabled/restart task integration |+| `Transit/Transit/TransitApp.swift` | Awaited app-launch startup |+| `Transit/TransitTests/MCPServerLifecycleTests.swift` | Live loopback regression coverage |+| `Transit/TransitTests/MCPServerStartFailureTests.swift` | Async lifecycle API coverage |+| `docs/agent-notes/mcp-server.md` | Serialized lifecycle architecture note |+| `CHANGELOG.md` | Unreleased fix entry |+| `specs/bugfixes/mcp-same-port-restart-race/report.md` | Investigation and verification record |+| `specs/bugfixes/mcp-same-port-restart-race/implementation.md` | Three-level implementation explanation |++## Verification++**Automated:**+- [x] Focused `MCPServerLifecycleTests` pass+- [x] `make test-quick` passes+- [x] `make test` passes+- [x] `make test-ui` passes+- [x] `make build` passes for iOS and macOS+- [x] `make lint` passes++**Manual verification:**+- The live-loopback regression starts Hummingbird on an ephemeral port, immediately verifies the port is bindable after awaited stop, restarts on the same port, and issues overlapping off/on requests. All paths remain reachable with no address-in-use error.++## Prevention++- Verify framework cancellation and graceful-shutdown paths separately; task completion is not automatically a resource-release guarantee.+- Retain the Hummingbird `ServiceGroup`, call `triggerGracefulShutdown()`, and await its `run()` task before rebinding.+- Route all start/stop/restart requests through one serialized desired-state coordinator.+- Keep UI callers from composing lifecycle primitives into unsafe stop/start sequences.+- Bound any teardown that a single serialized coordinator awaits. An unbounded graceful shutdown converts one stuck connection into a permanently wedged lifecycle.+- Construct `ServiceGroup` explicitly rather than inheriting `runService()`'s defaults; its SIGTERM/SIGINT traps are wrong for a GUI app.+- Give socket probes in tests the same options as the code under test (`SO_REUSEADDR`), or the probe fails on states production tolerates.++## Related++- Transit T-1826

Things to double-check

Required lifecycle invariants
  • Five-second maximumGracefulShutdownDuration is wired into the production service group.
  • No production call to runService() or Unix signal sequence exists; both signal arrays are asserted empty.
  • SO_REUSEADDR bind probing is directly validated against live and released listeners.
  • The final non-parallel run reported samePortRestartWaitsForOldListenerTeardown with 20 test cases passed.
Validation record
  • Focused MCPServerLifecycleTests: 9 logical tests, 20 same-port cases, succeeded.
  • make test-quick: exit 0, no failures.
  • make lint: 0 violations, 0 serious in 309 files.
  • make build-macos and make build-ios: exit 0, Build Succeeded.
  • Supplemental iOS UI runs exposed six failures in platform paths outside this macOS-gated diff; an origin/main baseline retry was interrupted during build, so those supplemental failures are recorded but not treated as branch validation.
Repository integrity

The review was read-only. HEAD remained d86199b38163926c548e8ba039cee64e3a4adecd and git status --porcelain remained empty. No commit, push, or merge was performed.