From f4b94af7719c8a7520e04098888315d17d3e4468 Mon Sep 17 00:00:00 2001 From: arzafran Date: Tue, 4 Aug 2026 13:36:29 -0300 Subject: [PATCH 1/3] test(escrow): wedged holder must cost one retrieve timeout, not one per session A holder that accepts but never answers previously stalled the serial main-thread restore for retrieveRecvTimeout per escrowed panel. The new test drives retrieve against a bind+accept-but-never-respond listener with an injectable sub-second timeout and asserts the SECOND call to the same socket path returns instantly without connecting (the listener must see exactly one connection), then that resetting the breaker allows a fresh attempt. Test-only commit by design: the breaker state and reset seam land here so the test compiles; the skip and record behavior land next, so CI shows this failing without them. --- Sources/SessionEscrow.swift | 62 ++++++++- ...erminalControllerSocketSecurityTests.swift | 120 ++++++++++++++++++ 2 files changed, 177 insertions(+), 5 deletions(-) diff --git a/Sources/SessionEscrow.swift b/Sources/SessionEscrow.swift index d5d9e8a3..7fe54856 100644 --- a/Sources/SessionEscrow.swift +++ b/Sources/SessionEscrow.swift @@ -733,6 +733,33 @@ final class SessionEscrowClient { // MARK: - App-side retrieval (issue #182 slice 2) extension SessionEscrowClient { + /// Time-bounded circuit breaker state for `retrieve`, keyed by holder + /// socket path -- see `retrieve`'s "Escrow follow-up" doc comment for + /// the rationale. `retrieve` itself is only ever called from the + /// main-thread session-restore path (`Workspace+Persistence.swift`'s + /// `attemptSessionReattach`), one call at a time, so nothing here is + /// actually contended in production; the lock exists purely so tests + /// (which may probe from a background accept thread) never have to + /// reason about a data race, at negligible cost on the real serial + /// restore path. + private static var recentRetrieveTimeoutsByPath: [String: Date] = [:] + private static let circuitBreakerLock = NSLock() + /// How long a path stays "open" (skipped without connecting) after a + /// recorded timeout. Comfortably covers a full serial restore sweep + /// (bounded by panel count in a real workspace) while not permanently + /// blacklisting a holder that recovers -- the window doubling as the + /// only reset mechanism is deliberate, see `retrieve`'s doc comment. + private static let circuitBreakerWindow: TimeInterval = 60.0 + + /// Test-only: clears breaker state so test cases don't leak it into + /// each other, and lets a test simulate the window elapsing without an + /// actual 60s sleep by re-arming a fresh state instead. + static func resetCircuitBreakerForTesting() { + circuitBreakerLock.lock() + recentRetrieveTimeoutsByPath.removeAll() + circuitBreakerLock.unlock() + } + /// One-shot fd retrieval for the reattach path /// (`Workspace+Persistence.swift`'s `createPanel(from:inPane:)`). Opens /// a FRESH connection to the holder's socket -- the previous app @@ -747,14 +774,40 @@ extension SessionEscrowClient { /// unknown session, token mismatch, child already exited, timeout) /// returns nil so the caller can fall through to its existing /// spawn-fresh path unchanged. - static func retrieve(sessionId: String, tokenHex: String, socketPath: String) -> Int32? { + /// + /// Escrow follow-up (issue tracker #6, post-#182): app-launch restore + /// (`Workspace+Persistence.swift`'s `attemptSessionReattach`) calls this + /// SERIALLY on the main thread, once per escrowed panel, and nearly + /// every panel shares the SAME deterministic holder socket path. A + /// missing/stale socket fails fast (`connect` returns nil immediately, + /// see `UnixDomainFDPassing.connect`'s doc comment) -- the + /// `retrieveRecvTimeout` cost is only ever paid when a holder ACCEPTS + /// the connection but never answers (wedged). Without a breaker, N + /// sessions against one wedged holder cost N * `retrieveRecvTimeout` of + /// main-thread stall at launch. `recentRetrieveTimeoutsByPath` makes + /// that a one-time cost per launch per path: once a path has timed out, + /// every other `retrieve` call against that SAME path within + /// `circuitBreakerWindow` returns nil immediately, without even + /// attempting to connect. The window is intentionally short and + /// self-healing rather than a manual reset -- 60s comfortably covers a + /// full serial restore sweep (bounded by how many panels a real + /// workspace has) while not permanently blacklisting a holder that + /// recovers (e.g. survives a transient hang and answers normally on the + /// next real request after this launch). + static func retrieve( + sessionId: String, + tokenHex: String, + socketPath: String, + recvTimeout: TimeInterval = SessionEscrowPolicy.retrieveRecvTimeout + ) -> Int32? { let attemptStartedAt = Date() - let timeoutMs = Int(SessionEscrowPolicy.retrieveRecvTimeout * 1000) + let timeoutMs = Int(recvTimeout * 1000) dilog("escrow.retrieve", "attempt session=\(sessionId.prefix(8)) socket=\(socketPath) timeoutMs=\(timeoutMs)") func logOutcome(_ outcome: String) { let elapsedMs = Int(Date().timeIntervalSince(attemptStartedAt) * 1000) dilog("escrow.retrieve", "outcome session=\(sessionId.prefix(8)) result=\(outcome) elapsedMs=\(elapsedMs)") } + guard let token = decodeHexToken(tokenHex) else { logOutcome("error_bad_token_hex") return nil @@ -767,9 +820,8 @@ extension SessionEscrowClient { // Sub-second timeout, so this must split whole seconds from the // fractional remainder rather than truncating straight to - // `tv_sec` -- a bare `Int(retrieveRecvTimeout)` would silently - // become 0 seconds with no microsecond budget at all. - let recvTimeout = SessionEscrowPolicy.retrieveRecvTimeout + // `tv_sec` -- a bare `Int(recvTimeout)` would silently become 0 + // seconds with no microsecond budget at all. let recvTimeoutWholeSeconds = recvTimeout.rounded(.down) var timeout = timeval( tv_sec: Int(recvTimeoutWholeSeconds), diff --git a/programaTests/TerminalControllerSocketSecurityTests.swift b/programaTests/TerminalControllerSocketSecurityTests.swift index 77b87208..f4586d7a 100644 --- a/programaTests/TerminalControllerSocketSecurityTests.swift +++ b/programaTests/TerminalControllerSocketSecurityTests.swift @@ -715,4 +715,124 @@ final class TerminalControllerSocketSecurityTests: XCTestCase { // the process does not survive the `send` call above. XCTAssertFalse(sendResult, "send to a closed peer must fail cleanly (EPIPE), not succeed") } + + // MARK: - Session escrow: per-socket-path circuit breaker (escrow follow-up #6) + + /// Thread-safe bookkeeping for the background "wedged holder" accept + /// loop below: it accepts every connection but never responds, so a + /// `retrieve()` call against it always times out rather than seeing EOF + /// -- the exact failure mode the circuit breaker exists for. + private final class AcceptedConnections: @unchecked Sendable { + private let lock = NSLock() + private var fds: [Int32] = [] + + func append(_ fd: Int32) { + lock.lock() + fds.append(fd) + lock.unlock() + } + + var count: Int { + lock.lock() + defer { lock.unlock() } + return fds.count + } + + func closeAll() { + lock.lock() + let all = fds + fds = [] + lock.unlock() + for fd in all { close(fd) } + } + } + + /// Regression for the escrow follow-up ("per-socket-path circuit + /// breaker for wedged holders"): app-launch restore + /// (`Workspace+Persistence.swift`'s `attemptSessionReattach`) calls + /// `SessionEscrowClient.retrieve` SERIALLY on the main thread, once per + /// escrowed panel, and almost every panel shares the SAME deterministic + /// holder socket path. Without a breaker, N sessions against one + /// wedged (accepts but never answers) holder would each pay the full + /// `retrieveRecvTimeout` -- N times the stall for one dead holder. + /// + /// This drives a real wedged holder (accepts, never responds) against + /// an injectable short `recvTimeout` (avoiding the real 5s + /// `SessionEscrowPolicy.retrieveRecvTimeout`) and asserts: the first + /// retrieve pays the full timeout and opens the breaker for that path; + /// a second retrieve against the SAME path returns near-instantly + /// without attempting a new connection at all (the listener sees no + /// second connection); and after resetting the breaker, a retrieve + /// against that path attempts a genuinely fresh connect again. + func testEscrowRetrieveCircuitBreakerSkipsSecondCallToWedgedHolder() { + SessionEscrowClient.resetCircuitBreakerForTesting() + defer { SessionEscrowClient.resetCircuitBreakerForTesting() } + + let socketPath = makeSocketPath("escrow-cb") + defer { unlink(socketPath) } + + guard let listenFD = UnixDomainFDPassing.bindListening(socketPath: socketPath) else { + XCTFail("bindListening failed") + return + } + + let accepted = AcceptedConnections() + let holderThread = Thread { + while true { + let clientFD = accept(listenFD, nil, nil) + guard clientFD >= 0 else { break } + // Wedged: accept the connection but never send a response + // and never close it -- the client's own recv timeout is + // the only thing that ever ends this connection. + accepted.append(clientFD) + } + } + holderThread.start() + defer { + close(listenFD) // unblocks the accept() loop so the thread exits + accepted.closeAll() + } + + let sessionId = String(repeating: "a", count: EscrowWireFormat.sessionIdSize) + let tokenHex = String(repeating: "00", count: EscrowWireFormat.tokenSize) + let shortTimeout: TimeInterval = 0.2 + + let firstStart = Date() + let firstResult = SessionEscrowClient.retrieve( + sessionId: sessionId, + tokenHex: tokenHex, + socketPath: socketPath, + recvTimeout: shortTimeout + ) + let firstElapsed = Date().timeIntervalSince(firstStart) + XCTAssertNil(firstResult, "a wedged holder must never grant a retrieval") + XCTAssertGreaterThanOrEqual(firstElapsed, shortTimeout, "first retrieve must actually wait out the recv timeout") + XCTAssertEqual(accepted.count, 1, "the wedged holder must have accepted exactly one connection so far") + + let secondStart = Date() + let secondResult = SessionEscrowClient.retrieve( + sessionId: sessionId, + tokenHex: tokenHex, + socketPath: socketPath, + recvTimeout: shortTimeout + ) + let secondElapsed = Date().timeIntervalSince(secondStart) + XCTAssertNil(secondResult) + XCTAssertLessThan(secondElapsed, 0.05, "second retrieve against the same path must be skipped by the open breaker, not pay another timeout") + XCTAssertEqual(accepted.count, 1, "an open breaker must prevent a second connection attempt to the wedged holder") + + SessionEscrowClient.resetCircuitBreakerForTesting() + + let thirdStart = Date() + let thirdResult = SessionEscrowClient.retrieve( + sessionId: sessionId, + tokenHex: tokenHex, + socketPath: socketPath, + recvTimeout: shortTimeout + ) + let thirdElapsed = Date().timeIntervalSince(thirdStart) + XCTAssertNil(thirdResult) + XCTAssertGreaterThanOrEqual(thirdElapsed, shortTimeout, "after resetting the breaker, retrieve must attempt a genuinely fresh connect") + XCTAssertEqual(accepted.count, 2, "the reset breaker must allow a new connection attempt, which the holder accepts") + } } From 270529981bbfe67618edff20a3aae4481e27c402 Mon Sep 17 00:00:00 2001 From: arzafran Date: Tue, 4 Aug 2026 13:36:36 -0300 Subject: [PATCH 2/3] fix(escrow): open a 60s circuit for a holder socket that times out MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit One wedged holder (accepts, never answers) now costs one retrieveRecvTimeout stall per launch instead of one per escrowed panel: the first post-connect receive timeout records the socket path, and every retrieve against that same path within 60s returns nil without connecting. Fast-fail paths (missing socket, connection refused, eof, protocol error) never open the circuit — they are already cheap and may be transient. The 60s window is the only reset: long enough to cover a full serial restore sweep, short enough that a recovered holder is retried on the next real request. Turns the previous commit's regression test green. --- Sources/SessionEscrow.swift | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/Sources/SessionEscrow.swift b/Sources/SessionEscrow.swift index 7fe54856..e86521ba 100644 --- a/Sources/SessionEscrow.swift +++ b/Sources/SessionEscrow.swift @@ -808,6 +808,17 @@ extension SessionEscrowClient { dilog("escrow.retrieve", "outcome session=\(sessionId.prefix(8)) result=\(outcome) elapsedMs=\(elapsedMs)") } + circuitBreakerLock.lock() + let recordedOpenedAt = recentRetrieveTimeoutsByPath[socketPath] + circuitBreakerLock.unlock() + if let openedAt = recordedOpenedAt { + let sinceMs = Int(Date().timeIntervalSince(openedAt) * 1000) + if sinceMs < Int(circuitBreakerWindow * 1000) { + dilog("escrow.retrieve", "skipped session=\(sessionId.prefix(8)) reason=circuit_open path_failed_ago_ms=\(sinceMs)") + return nil + } + } + guard let token = decodeHexToken(tokenHex) else { logOutcome("error_bad_token_hex") return nil @@ -853,6 +864,10 @@ extension SessionEscrowClient { return nil case .timeout: if let receivedFD { close(receivedFD) } + circuitBreakerLock.lock() + recentRetrieveTimeoutsByPath[socketPath] = Date() + circuitBreakerLock.unlock() + dilog("escrow.retrieve", "circuit_opened session=\(sessionId.prefix(8)) path=\(socketPath)") logOutcome("timeout") return nil case .error: From f278b07393f12fa86f65427bd66eda9ad98a1f23 Mon Sep 17 00:00:00 2001 From: arzafran Date: Tue, 4 Aug 2026 13:51:34 -0300 Subject: [PATCH 3/3] fix(escrow): monotonic clock for the breaker window, deflake the regression test The retrieve() circuit breaker keyed recentRetrieveTimeoutsByPath by wall-clock Date, so a backward clock jump (NTP sync, sleep/wake) could keep a path's circuit open past the intended 60s window. Switched to DispatchTime (monotonic CLOCK_UPTIME_RAW) and compare elapsed nanoseconds instead; the path_failed_ago_ms dilog value is unchanged. testEscrowRetrieveCircuitBreakerSkipsSecondCallToWedgedHolder asserted the open-breaker skip completes in < 0.05s (tight against scheduling jitter) and read the accept thread's connection count without waiting for the async accept() to actually land. Loosened the skip bound to < 0.15s (still well under the 0.2s injected timeout) and added AcceptedConnections.waitForCount, a bounded poll used wherever the test expects a new connection to have been accepted, instead of reading count immediately after retrieve() returns. --- Sources/SessionEscrow.swift | 13 +++++++--- ...erminalControllerSocketSecurityTests.swift | 25 ++++++++++++++++--- 2 files changed, 31 insertions(+), 7 deletions(-) diff --git a/Sources/SessionEscrow.swift b/Sources/SessionEscrow.swift index e86521ba..18e89f9d 100644 --- a/Sources/SessionEscrow.swift +++ b/Sources/SessionEscrow.swift @@ -742,7 +742,11 @@ extension SessionEscrowClient { /// (which may probe from a background accept thread) never have to /// reason about a data race, at negligible cost on the real serial /// restore path. - private static var recentRetrieveTimeoutsByPath: [String: Date] = [:] + /// Monotonic (`DispatchTime.now()`, backed by `CLOCK_UPTIME_RAW`) rather + /// than wall-clock `Date` -- a backward clock jump (NTP sync, sleep/wake, + /// manual clock change) must not keep the circuit open longer than + /// `circuitBreakerWindow` actually elapsed. + private static var recentRetrieveTimeoutsByPath: [String: DispatchTime] = [:] private static let circuitBreakerLock = NSLock() /// How long a path stays "open" (skipped without connecting) after a /// recorded timeout. Comfortably covers a full serial restore sweep @@ -812,8 +816,9 @@ extension SessionEscrowClient { let recordedOpenedAt = recentRetrieveTimeoutsByPath[socketPath] circuitBreakerLock.unlock() if let openedAt = recordedOpenedAt { - let sinceMs = Int(Date().timeIntervalSince(openedAt) * 1000) - if sinceMs < Int(circuitBreakerWindow * 1000) { + let sinceNs = DispatchTime.now().uptimeNanoseconds &- openedAt.uptimeNanoseconds + let sinceMs = Int(sinceNs / 1_000_000) + if sinceNs < UInt64(circuitBreakerWindow * 1_000_000_000) { dilog("escrow.retrieve", "skipped session=\(sessionId.prefix(8)) reason=circuit_open path_failed_ago_ms=\(sinceMs)") return nil } @@ -865,7 +870,7 @@ extension SessionEscrowClient { case .timeout: if let receivedFD { close(receivedFD) } circuitBreakerLock.lock() - recentRetrieveTimeoutsByPath[socketPath] = Date() + recentRetrieveTimeoutsByPath[socketPath] = DispatchTime.now() circuitBreakerLock.unlock() dilog("escrow.retrieve", "circuit_opened session=\(sessionId.prefix(8)) path=\(socketPath)") logOutcome("timeout") diff --git a/programaTests/TerminalControllerSocketSecurityTests.swift b/programaTests/TerminalControllerSocketSecurityTests.swift index f4586d7a..3028fdce 100644 --- a/programaTests/TerminalControllerSocketSecurityTests.swift +++ b/programaTests/TerminalControllerSocketSecurityTests.swift @@ -745,6 +745,25 @@ final class TerminalControllerSocketSecurityTests: XCTestCase { lock.unlock() for fd in all { close(fd) } } + + /// Polls (bounded) until the accept thread has recorded at least + /// `expected` connections, or `timeout` elapses. A plain `count` + /// read races the accept thread: `retrieve()` returning (its recv + /// timeout having elapsed) only guarantees the kernel completed the + /// connection, not that this thread's `accept()` call has returned + /// and appended the fd yet. Returns whatever count was actually + /// observed so the caller's assertion message stays meaningful on + /// a genuine failure. + func waitForCount(atLeast expected: Int, timeout: TimeInterval = 2.0) -> Int { + let deadline = Date().addingTimeInterval(timeout) + while true { + let current = count + if current >= expected || Date() >= deadline { + return current + } + Thread.sleep(forTimeInterval: 0.005) + } + } } /// Regression for the escrow follow-up ("per-socket-path circuit @@ -807,7 +826,7 @@ final class TerminalControllerSocketSecurityTests: XCTestCase { let firstElapsed = Date().timeIntervalSince(firstStart) XCTAssertNil(firstResult, "a wedged holder must never grant a retrieval") XCTAssertGreaterThanOrEqual(firstElapsed, shortTimeout, "first retrieve must actually wait out the recv timeout") - XCTAssertEqual(accepted.count, 1, "the wedged holder must have accepted exactly one connection so far") + XCTAssertEqual(accepted.waitForCount(atLeast: 1), 1, "the wedged holder must have accepted exactly one connection so far") let secondStart = Date() let secondResult = SessionEscrowClient.retrieve( @@ -818,7 +837,7 @@ final class TerminalControllerSocketSecurityTests: XCTestCase { ) let secondElapsed = Date().timeIntervalSince(secondStart) XCTAssertNil(secondResult) - XCTAssertLessThan(secondElapsed, 0.05, "second retrieve against the same path must be skipped by the open breaker, not pay another timeout") + XCTAssertLessThan(secondElapsed, 0.15, "second retrieve against the same path must be skipped by the open breaker, not pay another timeout (still well under the 0.2s injected timeout)") XCTAssertEqual(accepted.count, 1, "an open breaker must prevent a second connection attempt to the wedged holder") SessionEscrowClient.resetCircuitBreakerForTesting() @@ -833,6 +852,6 @@ final class TerminalControllerSocketSecurityTests: XCTestCase { let thirdElapsed = Date().timeIntervalSince(thirdStart) XCTAssertNil(thirdResult) XCTAssertGreaterThanOrEqual(thirdElapsed, shortTimeout, "after resetting the breaker, retrieve must attempt a genuinely fresh connect") - XCTAssertEqual(accepted.count, 2, "the reset breaker must allow a new connection attempt, which the holder accepts") + XCTAssertEqual(accepted.waitForCount(atLeast: 2), 2, "the reset breaker must allow a new connection attempt, which the holder accepts") } }