Skip to content
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
907 changes: 907 additions & 0 deletions REPORT.md

Large diffs are not rendered by default.

56 changes: 7 additions & 49 deletions Sources/App/CmuxCLIPathInstaller.swift
Original file line number Diff line number Diff line change
Expand Up @@ -261,24 +261,19 @@ struct CmuxCLIPathInstaller {
]
let stdout = Pipe()
let stderr = Pipe()
let stdoutBuffer = PrivilegedCommandOutputBuffer()
let stderrBuffer = PrivilegedCommandOutputBuffer()
process.standardOutput = stdout
process.standardError = stderr
let outputGroup = DispatchGroup()
startDraining(stdout, into: stdoutBuffer, group: outputGroup)
startDraining(stderr, into: stderrBuffer, group: outputGroup)
defer {
stdout.fileHandleForReading.readabilityHandler = nil
stderr.fileHandleForReading.readabilityHandler = nil
}
try process.run()
// Drain both pipes synchronously to EOF before waiting on exit so a child
// that fills a pipe buffer (>64KB) cannot block, then wait for termination.
// Avoids blocking the calling thread on async readabilityHandler callbacks.
let stdoutData = ProcessPipeReader.readDataToEndOfFileOrEmpty(from: stdout.fileHandleForReading)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Sequential pipe drain reintroduces the pipe-buffer deadlock

Reading stdout to EOF then stderr to EOF is safe only when the child writes < 64 KB to each pipe independently. If the child fills its stderr pipe buffer (≥ 64 KB) while the main thread is blocked inside readDataToEndOfFileOrEmpty(stdout), the child stalls writing to stderr, never closes stdout, and both sides hang indefinitely. The previous concurrent readabilityHandler approach on both pipes explicitly avoided this class of deadlock.

A safe synchronous alternative is to drain both descriptors on two concurrent threads (or use Thread.detachNewThread for each) and join them before calling waitUntilExit(). The existing startDraining(_:into:group:) + outputGroup.wait() pattern was already correct for this; the only actual fix needed there was replacing NSLock-backed PrivilegedCommandOutputBuffer with OSAllocatedUnfairLock.

let stderrData = ProcessPipeReader.readDataToEndOfFileOrEmpty(from: stderr.fileHandleForReading)
process.waitUntilExit()
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
outputGroup.wait()

guard process.terminationStatus == 0 else {
let stderrText = stderrBuffer.text.trimmingCharacters(in: .whitespacesAndNewlines)
let stdoutText = stdoutBuffer.text.trimmingCharacters(in: .whitespacesAndNewlines)
let stderrText = (String(data: stderrData, encoding: .utf8) ?? "").trimmingCharacters(in: .whitespacesAndNewlines)
let stdoutText = (String(data: stdoutData, encoding: .utf8) ?? "").trimmingCharacters(in: .whitespacesAndNewlines)
let details = stderrText.isEmpty ? stdoutText : stderrText
let message = details.isEmpty
? "osascript exited with status \(process.terminationStatus)."
Expand All @@ -287,25 +282,6 @@ struct CmuxCLIPathInstaller {
}
}

private static func startDraining(
_ pipe: Pipe,
into buffer: PrivilegedCommandOutputBuffer,
group: DispatchGroup
) {
group.enter()
pipe.fileHandleForReading.readabilityHandler = { fileHandle in
switch ProcessPipeReader.readAvailableDataOrEndOfFile(from: fileHandle) {
case .data(let data):
buffer.append(data)
case .wouldBlock:
return
case .endOfFile:
fileHandle.readabilityHandler = nil
group.leave()
}
}
}

private static func shellQuoted(_ value: String) -> String {
"'" + value.replacingOccurrences(of: "'", with: "'\\''") + "'"
}
Expand Down Expand Up @@ -337,21 +313,3 @@ struct CmuxCLIPathInstaller {
return false
}
}

private final class PrivilegedCommandOutputBuffer {
private let lock = NSLock()
private var data = Data()

var text: String {
lock.lock()
defer { lock.unlock() }
return String(data: data, encoding: .utf8) ?? ""
}

func append(_ chunk: Data) {
guard !chunk.isEmpty else { return }
lock.lock()
data.append(chunk)
lock.unlock()
}
}
5 changes: 2 additions & 3 deletions Sources/App/ShortcutRoutingSupport.swift
Original file line number Diff line number Diff line change
Expand Up @@ -613,6 +613,7 @@ func shouldRouteBrowserDocumentEditingCommandEquivalentThroughWebContentFirst(
/// For browser content, let the page try browser-local Find-family commands before cmux's menu fallback.
/// Cmd+F is excluded because cmux chooses terminal, browser, or right-sidebar
/// find from the current focus owner.
@MainActor
func shouldRouteBrowserFindCommandEquivalentThroughWebContentFirst(
_ event: NSEvent,
responder: NSResponder? = nil,
Expand All @@ -636,9 +637,7 @@ func shouldRouteBrowserFindCommandEquivalentThroughWebContentFirst(

if shortcut.keepsCmuxBrowserFindBarOwnershipWhenVisible,
let owningWebView {
let browserFindBarIsVisible = MainActor.assumeIsolated {
AppDelegate.shared?.browserFindBarIsVisible(for: owningWebView) == true
}
let browserFindBarIsVisible = AppDelegate.shared?.browserFindBarIsVisible(for: owningWebView) == true
if browserFindBarIsVisible {
return false
}
Expand Down
43 changes: 31 additions & 12 deletions Sources/Auth/AuthManager.swift
Original file line number Diff line number Diff line change
Expand Up @@ -8,26 +8,43 @@ import StackAuth
import Security
#endif

private final class AuthPresentationContext: NSObject, ASWebAuthenticationPresentationContextProviding {
private final class AuthPresentationContext: NSObject, ASWebAuthenticationPresentationContextProviding, @unchecked Sendable {
static let shared = AuthPresentationContext()

// ASWebAuthenticationSession invokes presentationAnchor(for:) synchronously
// on whichever thread called session.start(). When beginSignIn() fires from
// the socket command dispatch thread (cmux auth login), that callback lands
// off-main, but the anchor (NSApp.keyWindow/...) is @MainActor state. Rather
// than block the caller with DispatchQueue.main.sync, beginSignIn() (already
// @MainActor) pre-fetches the anchor on main via cacheCurrentAnchor() right
// before start(), and the off-main callback reads that cached window under a
// lock. The lock-guarded NSWindow is why this type is @unchecked Sendable.
private let lock = NSLock()
private var cachedAnchor: NSWindow?

func presentationAnchor(for session: ASWebAuthenticationSession) -> ASPresentationAnchor {
// ASWebAuthenticationSession invokes this on whichever thread called
// session.start(). When beginSignIn() fires from the socket command
// dispatch thread (cmux auth login), this callback lands off-main,
// and any NSApp access must hop to main before returning.
if Thread.isMainThread {
return Self.currentAnchor()
}
var result: ASPresentationAnchor = NSWindow()
DispatchQueue.main.sync {
result = Self.currentAnchor()
}
return result
lock.lock()
let cached = cachedAnchor
lock.unlock()
return cached ?? NSWindow()
}

/// Snapshot the current presentation anchor on the main actor so the
/// synchronous, possibly off-main protocol callback can read it without
/// blocking on main. Call immediately before `session.start()`.
@MainActor
func cacheCurrentAnchor() {
let anchor = Self.currentAnchor()
lock.lock()
cachedAnchor = anchor
lock.unlock()
}

@MainActor
private static func currentAnchor() -> ASPresentationAnchor {
private static func currentAnchor() -> NSWindow {
NSApp.keyWindow ?? NSApp.mainWindow ?? (NSApp.windows.first ?? NSWindow())
}
}
Expand Down Expand Up @@ -277,7 +294,9 @@ final class AuthManager: ObservableObject {
}
}
}
session.presentationContextProvider = AuthPresentationContext.shared
let presentationContext = AuthPresentationContext.shared
presentationContext.cacheCurrentAnchor()
session.presentationContextProvider = presentationContext
session.prefersEphemeralWebBrowserSession = false

if session.start() {
Expand Down
10 changes: 6 additions & 4 deletions Sources/BackgroundWorkspacePrimeCoordinator.swift
Original file line number Diff line number Diff line change
Expand Up @@ -262,7 +262,7 @@ final class BackgroundWorkspacePrimeCoordinator {
let self,
let waiter,
let tabManager else { return }
Task { @MainActor in
MainActor.assumeIsolated {
self.evaluate(waiter: waiter, workspaceId: workspaceId, tabManager: tabManager)
}
}
Expand All @@ -278,33 +278,35 @@ final class BackgroundWorkspacePrimeCoordinator {
let self,
let waiter,
let tabManager else { return }
Task { @MainActor in
MainActor.assumeIsolated {
self.evaluate(waiter: waiter, workspaceId: workspaceId, tabManager: tabManager)
}
}
waiter.addObserver(hostedViewObserver)

let pendingObserver = tabManager.$pendingBackgroundWorkspaceLoadIds
.dropFirst()
.receive(on: DispatchQueue.main)
.sink { [weak self, weak waiter, weak tabManager] pendingIds in
guard !pendingIds.contains(workspaceId),
let self,
let waiter,
let tabManager else { return }
Task { @MainActor in
MainActor.assumeIsolated {
self.evaluate(waiter: waiter, workspaceId: workspaceId, tabManager: tabManager)
}
}
waiter.addCancellable(pendingObserver)

let tabsObserver = tabManager.$tabs
.dropFirst()
.receive(on: DispatchQueue.main)
.sink { [weak self, weak waiter, weak tabManager] tabs in
guard !tabs.contains(where: { $0.id == workspaceId }),
let self,
let waiter,
let tabManager else { return }
Task { @MainActor in
MainActor.assumeIsolated {
self.evaluate(waiter: waiter, workspaceId: workspaceId, tabManager: tabManager)
}
}
Expand Down
75 changes: 46 additions & 29 deletions Sources/CmuxConfig.swift
Original file line number Diff line number Diff line change
Expand Up @@ -1909,7 +1909,9 @@ final class CmuxConfigStore: ObservableObject {
private var resolvedNewWorkspaceCommandCache: CmuxResolvedCommand?
private var resolvedNewWorkspaceActionCache: CmuxResolvedConfigAction?
private var parsedConfigCache: [String: ParsedConfigCacheEntry] = [:]
private var lifetimeCancellables = Set<AnyCancellable>()
private var trustObservationTask: Task<Void, Never>?
private var localReattachTask: Task<Void, Never>?
private var globalReattachTask: Task<Void, Never>?
private var trackingCancellables = Set<AnyCancellable>()
private var localFileWatchSource: DispatchSourceFileSystemObject?
private var localFileDescriptor: Int32 = -1
Expand Down Expand Up @@ -1946,13 +1948,20 @@ final class CmuxConfigStore: ObservableObject {
self.localConfigPath = localConfigPath
self.fileWatchingEnabled = startFileWatchers
self.localConfigSearchDirectory = localConfigPath.map(Self.searchDirectoryForLocalConfigPath(_:))
NotificationCenter.default.publisher(for: CmuxActionTrust.didChangeNotification)
.receive(on: DispatchQueue.main)
.sink { [weak self] _ in
// The store is @MainActor, so this task inherits main-actor isolation and
// loadAll() runs on main without an explicit dispatch hop. The handle is
// cancelled in deinit so the observer lives exactly as long as the store.
trustObservationTask = Task { [weak self] in
// Map to Void so the non-Sendable `Notification` never crosses into
// this main-actor task; only the change signal is needed, not the value.
let signals = NotificationCenter.default.notifications(
named: CmuxActionTrust.didChangeNotification
).map { _ in () }
for await _ in signals {
guard let self else { return }
self.loadAll()
}
.store(in: &lifetimeCancellables)
}
if startFileWatchers {
if localConfigPath != nil {
startLocalFileWatcher()
Expand All @@ -1962,6 +1971,9 @@ final class CmuxConfigStore: ObservableObject {
}

deinit {
trustObservationTask?.cancel()
localReattachTask?.cancel()
globalReattachTask?.cancel()
localFileWatchSource?.cancel()
for source in localHookFileWatchSources.values {
source.cancel()
Expand Down Expand Up @@ -2961,7 +2973,7 @@ final class CmuxConfigStore: ObservableObject {
DispatchQueue.main.async {
self.stopLocalFileWatcher()
self.loadAll()
self.scheduleLocalReattach(attempt: 1)
self.scheduleLocalReattach()
}
} else {
DispatchQueue.main.async {
Expand Down Expand Up @@ -3102,18 +3114,20 @@ final class CmuxConfigStore: ObservableObject {
startLocalFileWatcher()
}

private func scheduleLocalReattach(attempt: Int) {
guard attempt <= Self.maxReattachAttempts else { return }
watchQueue.asyncAfter(deadline: .now() + Self.reattachDelay) { [weak self] in
guard let self else { return }
DispatchQueue.main.async {
guard let path = self.localConfigPath else { return }
if FileManager.default.fileExists(atPath: path) {
self.loadAll()
self.startLocalFileWatcher()
} else {
self.startLocalDirectoryWatcher()
}
private func scheduleLocalReattach() {
// A deleted config has no inode to attach a DispatchSource to, so this is a
// genuine poll for the file's reappearance, not a timing hack. A superseding
// event or deinit cancels the pending retry via the stored handle.
localReattachTask?.cancel()
localReattachTask = Task { @MainActor [weak self] in
try? await Task.sleep(for: .seconds(Self.reattachDelay))
if Task.isCancelled { return }
guard let self, let path = self.localConfigPath else { return }
if FileManager.default.fileExists(atPath: path) {
self.loadAll()
self.startLocalFileWatcher()
} else {
self.startLocalDirectoryWatcher()
}
}
}
Comment on lines 3102 to 3119
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Task.sleep in retry loops — cmux-swift-blocking-runtime still flagged even for genuine polls

Both scheduleLocalReattach() and scheduleGlobalReattach() now use Task { @MainActor ... try? await Task.sleep(for: .seconds(...)) } for inter-attempt delays. The inline comment correctly identifies this as "genuine poll for file reappearance, not a timing hack" — but the cmux-swift-blocking-runtime rule explicitly lists Task.sleep and polling alongside asyncAfter as patterns to flag regardless of intent. The old implementation used watchQueue.asyncAfter (also flagged), so this is a lateral change that improves structure but does not resolve the policy concern. A DispatchSourceFileSystemObject watching the parent directory for kqueue/vnode events would eliminate the sleep entirely; if that isn't feasible here, these two sites should be listed in REPORT.md with the other deferred Task.sleep findings.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Expand Down Expand Up @@ -3155,7 +3169,7 @@ final class CmuxConfigStore: ObservableObject {
DispatchQueue.main.async {
self.stopGlobalFileWatcher()
self.loadAll()
self.scheduleGlobalReattach(attempt: 1)
self.scheduleGlobalReattach()
}
} else {
DispatchQueue.main.async {
Expand All @@ -3172,21 +3186,24 @@ final class CmuxConfigStore: ObservableObject {
globalFileWatchSource = source
}

private func scheduleGlobalReattach(attempt: Int) {
guard attempt <= Self.maxReattachAttempts else {
startGlobalDirectoryWatcher()
return
}
watchQueue.asyncAfter(deadline: .now() + Self.reattachDelay) { [weak self] in
guard let self else { return }
DispatchQueue.main.async {
private func scheduleGlobalReattach() {
// A deleted config has no inode to watch, so poll for reappearance with an
// explicit bounded retry count instead of recursing through the call stack.
// The stored handle lets a superseding event or deinit cancel the loop.
globalReattachTask?.cancel()
globalReattachTask = Task { @MainActor [weak self] in
for _ in 0..<Self.maxReattachAttempts {
try? await Task.sleep(for: .seconds(Self.reattachDelay))
if Task.isCancelled { return }
guard let self else { return }
if FileManager.default.fileExists(atPath: self.globalConfigPath) {
self.loadAll()
self.startGlobalFileWatcher()
} else {
self.scheduleGlobalReattach(attempt: attempt + 1)
return
}
}
guard let self else { return }
self.startGlobalDirectoryWatcher()
}
}

Expand Down
Loading
Loading