diff --git a/Scripts/Fixtures/test-suite-contract-ledger.tsv b/Scripts/Fixtures/test-suite-contract-ledger.tsv index 678840a9e..69e60a72d 100644 --- a/Scripts/Fixtures/test-suite-contract-ledger.tsv +++ b/Scripts/Fixtures/test-suite-contract-ledger.tsv @@ -1875,6 +1875,10 @@ root/RepoPromptTests.MCPProxyTerminalRecordTests/testLocalSocketReadFailureIsAtt root/RepoPromptTests.MCPProxyTerminalRecordTests/testProxyTaskGroupOutcomesClassifyCleanCompletionAndCancellation root Tests/RepoPromptTests/MCP/MCPProxyTerminalRecordTests.swift RepoPromptTests.MCPProxyTerminalRecordTests testProxyTaskGroupOutcomesClassifyCleanCompletionAndCancellation MCP mcp.proxy.task_group_outcome_classification proxy,cancellation,outcome_classification deterministic_regression root_swiftpm routine 1 MCPServiceProxyTaskGroupPolicy Clean transport completion stays clean while watcher and service cancellation outcomes normalize to host task-cancelled provenance. A canceled proxy could persist a false clean terminal record and hide lifecycle failures. 0.000000 test_case retain 0 PR #252 final proxy cancellation cleanup root/RepoPromptTests.MCPProxyTerminalRecordTests/testSocketReadFailureRemainsRetryablePeerTransportFailure root Tests/RepoPromptTests/MCP/MCPProxyTerminalRecordTests.swift RepoPromptTests.MCPProxyTerminalRecordTests testSocketReadFailureRemainsRetryablePeerTransportFailure MCP unreviewed unreviewed root_swiftpm routine 1 unreviewed unreviewed 0.000000 unreviewed retain_pending_review 0 initial census source line 60 root/RepoPromptTests.MCPProxyTerminalRecordTests/testTerminalRecordCopiesLiveLedgerSnapshotAndServerReason root Tests/RepoPromptTests/MCP/MCPProxyTerminalRecordTests.swift RepoPromptTests.MCPProxyTerminalRecordTests testTerminalRecordCopiesLiveLedgerSnapshotAndServerReason MCP unreviewed unreviewed root_swiftpm routine 1 unreviewed unreviewed 0.000000 unreviewed retain_pending_review 0 initial census source line 128 +root/RepoPromptTests.MCPServiceProxyRaceTests/testAlreadyCancelledCallerDoesNotStartChildren root Tests/RepoPromptTests/MCP/MCPServiceProxyRaceTests.swift RepoPromptTests.MCPServiceProxyRaceTests testAlreadyCancelledCallerDoesNotStartChildren MCP mcp.proxy.pre_cancelled_entry proxy,cancellation,child_start_boundary deterministic_regression root_swiftpm fast 1 A caller cancelled before entering the proxy race surfaces CancellationError without starting, cancelling, or completing any child operation. A pre-cancelled proxy could still launch transport and watcher work, obscuring caller cancellation provenance and extending teardown. 0.001000 test_case retain 0 PR #517 final cancellation-handler ordering regression. +root/RepoPromptTests.MCPServiceProxyRaceTests/testEachChildCanWinWithValueOrErrorAndDrainsLosers root Tests/RepoPromptTests/MCP/MCPServiceProxyRaceTests.swift RepoPromptTests.MCPServiceProxyRaceTests testEachChildCanWinWithValueOrErrorAndDrainsLosers MCP mcp.proxy.first_completion_priority proxy,cancellation,error_priority,task_drain deterministic_regression root_swiftpm fast 6 Each of the three proxy operations can win with its value or error, and both losing operations observe cancellation and complete before the race returns. Changing completion priority or failing to drain a suspended loser could misclassify terminal state or reintroduce the Swift release-runtime teardown abort. 0.001000 test_case retain 0 Issue #513 explicit-task first-completion race coverage. +root/RepoPromptTests.MCPServiceProxyRaceTests/testOuterCancellationCancelsAndDrainsAllChildren root Tests/RepoPromptTests/MCP/MCPServiceProxyRaceTests.swift RepoPromptTests.MCPServiceProxyRaceTests testOuterCancellationCancelsAndDrainsAllChildren MCP mcp.proxy.outer_cancellation_drain proxy,cancellation,task_drain deterministic_regression root_swiftpm fast 1 Caller cancellation reaches all three proxy operations, surfaces as CancellationError, and waits for every operation to complete before returning. Outer cancellation could leak or tear down a suspended proxy operation and persist incorrect terminal provenance. 0.001000 test_case retain 0 Issue #513 cancellation-handler teardown coverage. +root/RepoPromptTests.MCPServiceProxyRaceTests/testRaceDoesNotReturnUntilCancelledLosersComplete root Tests/RepoPromptTests/MCP/MCPServiceProxyRaceTests.swift RepoPromptTests.MCPServiceProxyRaceTests testRaceDoesNotReturnUntilCancelledLosersComplete MCP mcp.proxy.loser_completion_barrier proxy,cancellation,task_drain deterministic_regression root_swiftpm fast 1 A controlled loser-completion gate proves the proxy race remains suspended after cancellation until both losing operations finish. Returning after cancel without awaiting loser completion recreates the compiler-frame teardown shape implicated in swift_task_dealloc aborts. 0.001000 test_case retain 0 Issue #513 structural regression oracle; fails against cancel-without-await. root/RepoPromptTests.MCPReadFileAutoSelectionCoordinatorTests/testAuthoritativeSelectionPreservationRequiresFullCanonicalSuperset root Tests/RepoPromptTests/MCP/MCPReadFileAutoSelectionCoordinatorTests.swift RepoPromptTests.MCPReadFileAutoSelectionCoordinatorTests testAuthoritativeSelectionPreservationRequiresFullCanonicalSuperset MCP mcp.read_file.auto_selection.authoritative_selection_preservation exact,additive_superset,full_supersedes_slice,selected_path_loss,full_to_slice_demotion,auto_codemap_loss,slice_narrowing,codemap_mode_change deterministic_table_unit root_swiftpm routine 8 stored_selection Certificate mint revalidation accepts only a full canonical superset of the authoritative result across selected paths, auto-codemap paths, slices, and codemap mode. critical 0.000000 in_memory per_test_state retain 0 read-file selection rebasing full-state preservation guard root/RepoPromptTests.MCPReadFileAutoSelectionCoordinatorTests/testCancelledCanonicalDrainResumesPromptlyWithoutStoppingWorker root Tests/RepoPromptTests/MCP/MCPReadFileAutoSelectionCoordinatorTests.swift RepoPromptTests.MCPReadFileAutoSelectionCoordinatorTests testCancelledCanonicalDrainResumesPromptlyWithoutStoppingWorker MCP unreviewed unreviewed root_swiftpm routine 1 unreviewed unreviewed 0.004500 unreviewed retain_pending_review 0 initial census source line 141 root/RepoPromptTests.MCPReadFileAutoSelectionCoordinatorTests/testCancelledMirrorDrainResumesPromptlyWithoutStoppingWorker root Tests/RepoPromptTests/MCP/MCPReadFileAutoSelectionCoordinatorTests.swift RepoPromptTests.MCPReadFileAutoSelectionCoordinatorTests testCancelledMirrorDrainResumesPromptlyWithoutStoppingWorker MCP unreviewed unreviewed root_swiftpm routine 1 unreviewed unreviewed 0.004500 unreviewed retain_pending_review 0 initial census source line 175 diff --git a/Sources/RepoPromptMCP/main.swift b/Sources/RepoPromptMCP/main.swift index 4f7196456..a223fb51d 100644 --- a/Sources/RepoPromptMCP/main.swift +++ b/Sources/RepoPromptMCP/main.swift @@ -783,6 +783,46 @@ enum MCPServiceProxyTaskGroupPolicy { } } +private final class MCPServiceProxySettlementState: @unchecked Sendable { + enum Outcome { + case completed(Result) + case callerCancelled + } + + private let lock = NSLock() + private var outcome: Outcome? + private var continuation: CheckedContinuation? + + func claim(_ candidate: Outcome) -> Bool { + lock.lock() + guard outcome == nil else { + lock.unlock() + return false + } + outcome = candidate + let continuation = continuation + self.continuation = nil + lock.unlock() + + continuation?.resume(returning: candidate) + return true + } + + func finish() async -> Outcome { + await withCheckedContinuation { continuation in + lock.lock() + if let outcome { + lock.unlock() + continuation.resume(returning: outcome) + } else { + precondition(self.continuation == nil) + self.continuation = continuation + lock.unlock() + } + } + } +} + // MARK: - Bootstrap Socket Proxy (CLI connects to App) /// Bootstrap socket proxy - connects to app's single socket server. @@ -2100,6 +2140,60 @@ actor MCPService: Service { killSignalContinuation = nil } + static func awaitFirstProxyOutcome( + killSignal: @escaping @Sendable () async throws -> MCPServiceProxyTaskOutcome, + watchdog: @escaping @Sendable () async throws -> MCPServiceProxyTaskOutcome, + transport: @escaping @Sendable () async throws -> MCPServiceProxyTaskOutcome + ) async throws -> MCPServiceProxyTaskOutcome { + let settlement = MCPServiceProxySettlementState() + let outcome = await withTaskCancellationHandler { + guard !Task.isCancelled else { + _ = settlement.claim(.callerCancelled) + return await settlement.finish() + } + + let killSignalTask = Task { + do { + _ = try await settlement.claim(.completed(.success(killSignal()))) + } catch { + _ = settlement.claim(.completed(.failure(error))) + } + } + let watchdogTask = Task { + do { + _ = try await settlement.claim(.completed(.success(watchdog()))) + } catch { + _ = settlement.claim(.completed(.failure(error))) + } + } + let transportTask = Task { + do { + _ = try await settlement.claim(.completed(.success(transport()))) + } catch { + _ = settlement.claim(.completed(.failure(error))) + } + } + + let outcome = await settlement.finish() + killSignalTask.cancel() + watchdogTask.cancel() + transportTask.cancel() + await killSignalTask.value + await watchdogTask.value + await transportTask.value + return outcome + } onCancel: { + _ = settlement.claim(.callerCancelled) + } + + switch outcome { + case let .completed(result): + return try result.get() + case .callerCancelled: + throw CancellationError() + } + } + func run() async throws { // Set up kill signal watcher before starting transport setupKillSignalWatcher() @@ -2110,9 +2204,8 @@ actor MCPService: Service { do { // Race between transport loop, kill signal, and PPID watchdog - try await withThrowingTaskGroup(of: MCPServiceProxyTaskOutcome.self) { group in - // Kill signal monitor task - group.addTask { + let outcome = try await Self.awaitFirstProxyOutcome( + killSignal: { guard let signal = await self.waitForKillSignal() else { return .killSignalWaitCancelled } @@ -2120,38 +2213,21 @@ actor MCPService: Service { reason: signal.reason, message: signal.message ?? CLIKillSignal.messageForReason(signal.reason) )) - } - - // PPID watchdog - detect orphaned CLI when parent dies - group.addTask { + }, + watchdog: { try await self.runPPIDWatchdog(initialPPID: initialPPID) return .ppidWatchdogCancelled - } - - // Main transport task - group.addTask { + }, + transport: { try await self.runTransport() return .transportCompleted } - - // Wait for first to complete (either kill signal, orphan detection, or transport exit), - // then stop the watcher tasks so a clean transport exit can return promptly. - do { - guard let outcome = try await group.next() else { - group.cancelAll() - throw CancellationError() - } - group.cancelAll() - if let runtimeError = MCPServiceProxyTaskGroupPolicy.terminalRuntimeError( - firstCompletedOutcome: outcome, - serviceTaskIsCancelled: Task.isCancelled - ) { - throw runtimeError - } - } catch { - group.cancelAll() - throw error - } + ) + if let runtimeError = MCPServiceProxyTaskGroupPolicy.terminalRuntimeError( + firstCompletedOutcome: outcome, + serviceTaskIsCancelled: Task.isCancelled + ) { + throw runtimeError } try Task.checkCancellation() await persistProxyTerminalRecord( diff --git a/Tests/RepoPromptTests/MCP/MCPServiceProxyRaceTests.swift b/Tests/RepoPromptTests/MCP/MCPServiceProxyRaceTests.swift new file mode 100644 index 000000000..f8ab33a75 --- /dev/null +++ b/Tests/RepoPromptTests/MCP/MCPServiceProxyRaceTests.swift @@ -0,0 +1,293 @@ +@testable import RepoPromptMCP +import XCTest + +final class MCPServiceProxyRaceTests: XCTestCase { + func testEachChildCanWinWithValueOrErrorAndDrainsLosers() async throws { + let outcomes: [MCPServiceProxyTaskOutcome] = [ + .killSignalWaitCancelled, + .ppidWatchdogCancelled, + .transportCompleted + ] + + for winnerID in outcomes.indices { + for winnerThrows in [false, true] { + let probe = MCPProxyRaceProbe(expectedStartCount: outcomes.count) + let winnerResult: Result = winnerThrows + ? .failure(MCPProxyRaceTestError.winner(winnerID)) + : .success(outcomes[winnerID]) + let operations = outcomes.indices.map { childID in + makeOperation( + childID: childID, + winnerID: winnerID, + winnerResult: winnerResult, + probe: probe + ) + } + + do { + let outcome = try await MCPService.awaitFirstProxyOutcome( + killSignal: operations[0], + watchdog: operations[1], + transport: operations[2] + ) + XCTAssertFalse(winnerThrows, "Expected child \(winnerID) to throw") + XCTAssertEqual(proxyOutcomeID(outcome), winnerID) + } catch let error as MCPProxyRaceTestError { + XCTAssertTrue(winnerThrows, "Unexpected child error: \(error)") + XCTAssertEqual(error, .winner(winnerID)) + } + + let snapshot = await probe.snapshot() + XCTAssertEqual(snapshot.completed, Set(outcomes.indices)) + XCTAssertEqual(snapshot.cancelled, Set(outcomes.indices.filter { $0 != winnerID })) + } + } + } + + func testRaceDoesNotReturnUntilCancelledLosersComplete() async throws { + let probe = MCPProxyRaceProbe(expectedStartCount: 3) + let drainGate = MCPProxyRaceGate() + let returned = MCPProxyRaceFlag() + let winnerResult = Result.success(.transportCompleted) + let operations = (0 ..< 3).map { childID in + makeOperation( + childID: childID, + winnerID: 2, + winnerResult: winnerResult, + probe: probe, + drainGate: drainGate + ) + } + let race = Task { + let outcome = try await MCPService.awaitFirstProxyOutcome( + killSignal: operations[0], + watchdog: operations[1], + transport: operations[2] + ) + await returned.set() + return outcome + } + + await probe.waitUntilCancelled(count: 2) + let didReturnBeforeLosersCompleted = await returned.value + XCTAssertFalse(didReturnBeforeLosersCompleted) + + await drainGate.release() + let outcome = try await race.value + XCTAssertEqual(proxyOutcomeID(outcome), 2) + let snapshot = await probe.snapshot() + XCTAssertEqual(snapshot.completed, Set(0 ..< 3)) + } + + func testOuterCancellationCancelsAndDrainsAllChildren() async throws { + let probe = MCPProxyRaceProbe(expectedStartCount: 3) + let drainGate = MCPProxyRaceGate() + let returned = MCPProxyRaceFlag() + let operations = (0 ..< 3).map { childID in + makeOperation( + childID: childID, + winnerID: nil, + winnerResult: .success(.transportCompleted), + probe: probe, + drainGate: drainGate + ) + } + let race = Task { + do { + let outcome = try await MCPService.awaitFirstProxyOutcome( + killSignal: operations[0], + watchdog: operations[1], + transport: operations[2] + ) + await returned.set() + return outcome + } catch { + await returned.set() + throw error + } + } + + await probe.waitUntilAllStarted() + race.cancel() + await probe.waitUntilCancelled(count: 3) + let didReturnBeforeChildrenCompleted = await returned.value + XCTAssertFalse(didReturnBeforeChildrenCompleted) + + await drainGate.release() + do { + _ = try await race.value + XCTFail("Expected outer cancellation") + } catch is CancellationError { + // Expected. + } + + let snapshot = await probe.snapshot() + XCTAssertEqual(snapshot.cancelled, Set(0 ..< 3)) + XCTAssertEqual(snapshot.completed, Set(0 ..< 3)) + } + + func testAlreadyCancelledCallerDoesNotStartChildren() async throws { + let entryGate = MCPProxyRaceGate() + let probe = MCPProxyRaceProbe(expectedStartCount: 3) + let operations = (0 ..< 3).map { childID in + makeOperation( + childID: childID, + winnerID: nil, + winnerResult: .success(.transportCompleted), + probe: probe + ) + } + let race = Task { + await entryGate.wait() + return try await MCPService.awaitFirstProxyOutcome( + killSignal: operations[0], + watchdog: operations[1], + transport: operations[2] + ) + } + + race.cancel() + await entryGate.release() + + do { + _ = try await race.value + XCTFail("Expected outer cancellation") + } catch is CancellationError { + // Expected. + } + + let snapshot = await probe.snapshot() + XCTAssertTrue(snapshot.started.isEmpty) + XCTAssertTrue(snapshot.cancelled.isEmpty) + XCTAssertTrue(snapshot.completed.isEmpty) + } + + private func makeOperation( + childID: Int, + winnerID: Int?, + winnerResult: Result, + probe: MCPProxyRaceProbe, + drainGate: MCPProxyRaceGate? = nil + ) -> @Sendable () async throws -> MCPServiceProxyTaskOutcome { + { + await probe.markStarted(childID) + await probe.waitUntilAllStarted() + if childID == winnerID { + await probe.markCompleted(childID) + return try winnerResult.get() + } + + do { + try await Task.sleep(nanoseconds: UInt64.max) + throw MCPProxyRaceTestError.loserReturnedWithoutCancellation(childID) + } catch is CancellationError { + await probe.markCancelled(childID) + if let drainGate { + await drainGate.wait() + } + await probe.markCompleted(childID) + throw CancellationError() + } + } + } + + private func proxyOutcomeID(_ outcome: MCPServiceProxyTaskOutcome) -> Int { + switch outcome { + case .killSignalWaitCancelled: + 0 + case .ppidWatchdogCancelled: + 1 + case .transportCompleted: + 2 + } + } +} + +private enum MCPProxyRaceTestError: Error, Equatable { + case winner(Int) + case loserReturnedWithoutCancellation(Int) +} + +private actor MCPProxyRaceProbe { + struct Snapshot { + let started: Set + let cancelled: Set + let completed: Set + } + + private let expectedStartCount: Int + private var started: Set = [] + private var cancelled: Set = [] + private var completed: Set = [] + private var startWaiters: [CheckedContinuation] = [] + private var cancellationWaiters: [(count: Int, continuation: CheckedContinuation)] = [] + + init(expectedStartCount: Int) { + self.expectedStartCount = expectedStartCount + } + + func markStarted(_ childID: Int) { + started.insert(childID) + guard started.count == expectedStartCount else { return } + let waiters = startWaiters + startWaiters.removeAll() + waiters.forEach { $0.resume() } + } + + func waitUntilAllStarted() async { + guard started.count < expectedStartCount else { return } + await withCheckedContinuation { continuation in + startWaiters.append(continuation) + } + } + + func markCancelled(_ childID: Int) { + cancelled.insert(childID) + let ready = cancellationWaiters.filter { cancelled.count >= $0.count } + cancellationWaiters.removeAll { cancelled.count >= $0.count } + ready.forEach { $0.continuation.resume() } + } + + func waitUntilCancelled(count: Int) async { + guard cancelled.count < count else { return } + await withCheckedContinuation { continuation in + cancellationWaiters.append((count, continuation)) + } + } + + func markCompleted(_ childID: Int) { + completed.insert(childID) + } + + func snapshot() -> Snapshot { + Snapshot(started: started, cancelled: cancelled, completed: completed) + } +} + +private actor MCPProxyRaceGate { + private var isReleased = false + private var waiters: [CheckedContinuation] = [] + + func wait() async { + guard !isReleased else { return } + await withCheckedContinuation { continuation in + waiters.append(continuation) + } + } + + func release() { + guard !isReleased else { return } + isReleased = true + let waiters = waiters + self.waiters.removeAll() + waiters.forEach { $0.resume() } + } +} + +private actor MCPProxyRaceFlag { + private(set) var value = false + + func set() { + value = true + } +}