From 366c643d09065757834f54526855d6e451a9b69c Mon Sep 17 00:00:00 2001 From: Sebastien Tardif Date: Sat, 15 Aug 2026 18:36:39 -0700 Subject: [PATCH] fix(macos): stop cursor reconcile and mailbox waits on cancel PLAN: cursor reconcile caught every error (including CancellationError) then slept with try?, so cancel became another backoff retry. Mailbox timeout Tasks used try? sleep and were not cancelled when a frame arrived. DO: break on CancellationError, cancel the mailbox timeout Task, and honor Task.sleep cancellation. Signed-off-by: Sebastien Tardif --- CHANGELOG.md | 2 + .../CrabfleetMac/MacScreenCapture.swift | 13 +++- .../Sources/CrabfleetMac/VideoMailbox.swift | 63 ++++++++++++++----- .../VideoPipelineTests.swift | 24 +++++++ 4 files changed, 87 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index aaa3f13..580f7cd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ ## Unreleased +- Stop cursor-reconcile retries and mailbox timeout waits when the share task is cancelled, so teardown does not stall. + ## 0.3.1 - 2026-08-28 ### Highlights diff --git a/macos/CrabfleetMac/Sources/CrabfleetMac/MacScreenCapture.swift b/macos/CrabfleetMac/Sources/CrabfleetMac/MacScreenCapture.swift index e4317cf..9f6b42a 100644 --- a/macos/CrabfleetMac/Sources/CrabfleetMac/MacScreenCapture.swift +++ b/macos/CrabfleetMac/Sources/CrabfleetMac/MacScreenCapture.swift @@ -418,7 +418,14 @@ final class MacScreenCapture: NSObject, @unchecked Sendable { try await self.reconcileCursorConfiguration() break } catch { - try? await Task.sleep(for: delay) + guard Self.isRetryableCursorReconcileError(error) else { break } + do { + try await Task.sleep(for: delay) + } catch is CancellationError { + break + } catch { + break + } delay = min(delay * 2, .seconds(5)) } } @@ -434,6 +441,10 @@ final class MacScreenCapture: NSObject, @unchecked Sendable { _ = task } + static func isRetryableCursorReconcileError(_ error: Error) -> Bool { + !(error is CancellationError) + } + private func reconcileCursorConfiguration() async throws { try await configurationGate.run { [self] in guard let stream, let configuration else { return } diff --git a/macos/CrabfleetMac/Sources/CrabfleetMac/VideoMailbox.swift b/macos/CrabfleetMac/Sources/CrabfleetMac/VideoMailbox.swift index 3183d33..f16a144 100644 --- a/macos/CrabfleetMac/Sources/CrabfleetMac/VideoMailbox.swift +++ b/macos/CrabfleetMac/Sources/CrabfleetMac/VideoMailbox.swift @@ -12,6 +12,7 @@ final class VideoMailbox: @unchecked Sendable { private let lock = NSLock() private var latestElement: Element? private var waiter: Waiter? + private var timeoutTask: Task? private var finished = false var isFinished: Bool { @@ -30,27 +31,36 @@ final class VideoMailbox: @unchecked Sendable { } func offer(_ element: Element, onDrop: () -> Void = {}) { - let continuation = withLock { () -> CheckedContinuation? in - guard !finished else { return nil } + let (continuation, timeoutTask) = withLock { + () -> (CheckedContinuation?, Task?) in + guard !finished else { return (nil, nil) } guard let waiter else { if latestElement != nil { onDrop() } latestElement = element - return nil + return (nil, nil) } self.waiter = nil - return waiter.continuation + let timeoutTask = self.timeoutTask + self.timeoutTask = nil + return (waiter.continuation, timeoutTask) } + timeoutTask?.cancel() continuation?.resume(returning: element) } func finish() { - let continuation = withLock { () -> CheckedContinuation? in - guard !finished else { return nil } + let (continuation, timeoutTask) = withLock { + () -> (CheckedContinuation?, Task?) in + guard !finished else { return (nil, nil) } finished = true latestElement = nil - defer { waiter = nil } - return waiter?.continuation + let continuation = waiter?.continuation + let timeoutTask = self.timeoutTask + waiter = nil + self.timeoutTask = nil + return (continuation, timeoutTask) } + timeoutTask?.cancel() continuation?.resume(returning: nil) } @@ -64,6 +74,7 @@ final class VideoMailbox: @unchecked Sendable { var immediateElement: Element? var shouldResume = false var replacedWaiter: Waiter? + var replacedTimeout: Task? lock.lock() if let latestElement { immediateElement = latestElement @@ -73,16 +84,35 @@ final class VideoMailbox: @unchecked Sendable { shouldResume = true } else { replacedWaiter = waiter + replacedTimeout = timeoutTask + timeoutTask = nil waiter = (id, continuation) } lock.unlock() replacedWaiter?.continuation.resume(returning: nil) + replacedTimeout?.cancel() if shouldResume { continuation.resume(returning: immediateElement) } else { - Task { - try? await Task.sleep(for: timeout) + let task = Task { + do { + try await Task.sleep(for: timeout) + self.expire(id: id) + } catch is CancellationError { + // Do not expire a different waiter; expire already guards by id. + } catch { + self.expire(id: id) + } + } + let shouldCancelTimeout = withLock { () -> Bool in + guard waiter?.id == id else { return true } + timeoutTask = task + return false + } + if shouldCancelTimeout { + task.cancel() + } else if Task.isCancelled { self.expire(id: id) } } @@ -93,11 +123,16 @@ final class VideoMailbox: @unchecked Sendable { } private func expire(id: UUID) { - let continuation = withLock { () -> CheckedContinuation? in - guard waiter?.id == id else { return nil } - defer { waiter = nil } - return waiter?.continuation + let (continuation, timeoutTask) = withLock { + () -> (CheckedContinuation?, Task?) in + guard waiter?.id == id else { return (nil, nil) } + let continuation = waiter?.continuation + let timeoutTask = self.timeoutTask + waiter = nil + self.timeoutTask = nil + return (continuation, timeoutTask) } + timeoutTask?.cancel() continuation?.resume(returning: nil) } diff --git a/macos/CrabfleetMac/Tests/CrabfleetMacTests/VideoPipelineTests.swift b/macos/CrabfleetMac/Tests/CrabfleetMacTests/VideoPipelineTests.swift index 1e3ef0c..305ae92 100644 --- a/macos/CrabfleetMac/Tests/CrabfleetMacTests/VideoPipelineTests.swift +++ b/macos/CrabfleetMac/Tests/CrabfleetMacTests/VideoPipelineTests.swift @@ -377,6 +377,30 @@ struct VideoPipelineTests { #expect(await mailbox.next(timeout: .milliseconds(10)) == nil) } + @Test + func mailboxCancelledWaiterReturnsPromptlyWithoutResumingTwice() async { + let mailbox = VideoMailbox() + let startedAt = ContinuousClock().now + let task = Task { + withUnsafeCurrentTask { $0?.cancel() } + return await mailbox.next(timeout: .seconds(5)) + } + let result = await task.value + #expect(result == nil) + #expect(ContinuousClock().now - startedAt < .milliseconds(500)) + + mailbox.offer(4) + #expect(await mailbox.next(timeout: .milliseconds(50)) == 4) + } + + @Test + func cursorReconcileErrorsAreNotRetryableWhenCancelled() { + #expect(!MacScreenCapture.isRetryableCursorReconcileError(CancellationError())) + #expect( + MacScreenCapture.isRetryableCursorReconcileError( + NSError(domain: "CrabfleetMacTests", code: 1))) + } + @Test func videoNegotiationPrefersHEVCThenH264ThenTight() { let offered = [