diff --git a/CHANGELOG.md b/CHANGELOG.md index 053cc0ef21..22ba6a3066 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ - Linux CLI: bootstrap the configured IANA timezone before Foundation startup on non-FHS systems, preventing SIGILL on NixOS (#2127). Thanks @xikhar! - Ollama: release temporary dashboard network sessions after each fetch, preventing repeated refreshes from retaining delegates and URL-cache resources. Thanks @astuteprogrammer! - Linux CLI: prevent usage rendering from crashing in Foundation bundle discovery when formatting rate windows. Thanks @thanthi-del! +- CLI: defer login-shell PATH probes until Codex RPC launch, preserve login PATH for explicit script overrides, and reap session-escaped helpers without cross-probe descriptor inheritance. Thanks @anagnorisis2peripeteia! - Menus: keep overview provider-row clicks reliable during live menu rebuilds without stealing nested Copy or plan actions. Thanks @Yuxin-Qiao! - Startup: load persisted plan-utilization history away from the main thread so mature histories no longer delay app launch. Thanks @Yuxin-Qiao! - Provider cleanup: prevent in-flight usage, status, token-cost, and cached-hydration work from republishing stale state after a provider is disabled, unavailable, or re-enabled. Thanks @Yuxin-Qiao! diff --git a/Sources/CodexBarCore/CodexExecutableResolver.swift b/Sources/CodexBarCore/CodexExecutableResolver.swift new file mode 100644 index 0000000000..dacb02f68d --- /dev/null +++ b/Sources/CodexBarCore/CodexExecutableResolver.swift @@ -0,0 +1,51 @@ +import Foundation + +struct CodexExecutableResolution: Sendable { + let executable: String + let loginPATH: [String]? +} + +typealias CodexExecutableResolver = @Sendable ( + _ environment: [String: String], + _ executable: String) -> CodexExecutableResolution? + +private func executableIsScript(_ path: String) -> Bool { + guard let handle = FileHandle(forReadingAtPath: path) else { return true } + defer { try? handle.close() } + return (try? handle.read(upToCount: 2)) == Data("#!".utf8) +} + +func resolveCodexExecutableForRPC( + environment: [String: String], + executable: String, + captureLoginPATH: () -> [String]?) -> CodexExecutableResolution? +{ + if let override = environment["CODEX_CLI_PATH"], + FileManager.default.isExecutableFile(atPath: override) + { + // Native overrides can launch directly. Script overrides can depend on a + // login-shell PATH through `#!/usr/bin/env node` or helper commands. + let loginPATH = executableIsScript(override) ? captureLoginPATH() : nil + return CodexExecutableResolution(executable: override, loginPATH: loginPATH) + } + + // Capture before the general resolver to preserve login-shell PATH precedence + // over bundled fallbacks and to make `/usr/bin/env node` launchers usable. + let loginPATH = captureLoginPATH() + guard let resolved = BinaryLocator.resolveCodexBinary(env: environment, loginPATH: loginPATH) + ?? TTYCommandRunner.which(executable) + else { return nil } + return CodexExecutableResolution(executable: resolved, loginPATH: loginPATH) +} + +let defaultCodexExecutableResolver: CodexExecutableResolver = { environment, executable in + // Resolve the login-shell PATH at the actual RPC launch boundary. Warming it + // from UsageFetcher.init was both racy for env-based launchers and unsafe for + // short-lived CLI hosts because the fire-and-forget probe could outlive them. + resolveCodexExecutableForRPC( + environment: environment, + executable: executable, + captureLoginPATH: { + LoginShellPathCache.shared.currentOrCapture(shell: environment["SHELL"]) + }) +} diff --git a/Sources/CodexBarCore/Host/Process/SpawnedProcessGroup.swift b/Sources/CodexBarCore/Host/Process/SpawnedProcessGroup.swift index ecfb190d8b..96acc3d262 100644 --- a/Sources/CodexBarCore/Host/Process/SpawnedProcessGroup.swift +++ b/Sources/CodexBarCore/Host/Process/SpawnedProcessGroup.swift @@ -322,6 +322,14 @@ package final class SpawnedProcessGroup: @unchecked Sendable { self.startWaiter() } + package static func adopt( + pid: pid_t, + outputFileDescriptors: [Int32]) -> SpawnedProcessGroup + { + let outputPipes = Set(outputFileDescriptors.compactMap(OutputPipeIdentity.resolve(fileDescriptor:))) + return SpawnedProcessGroup(pid: pid, outputPipes: outputPipes) + } + package static func launch( binary: String, arguments: [String], @@ -531,7 +539,7 @@ package final class SpawnedProcessGroup: @unchecked Sendable { let deadline = Date().addingTimeInterval(max(0, grace)) var processIdentities = self.currentResidualProcessIdentities(includeDescendants: true) processIdentities.formUnion(self.currentProcessGroupMemberIdentities()) - if let rootIdentity = self.rootIdentity { + if self.isRunning, let rootIdentity = self.rootIdentity { processIdentities.insert(rootIdentity) } Self.signal(processIdentities: processIdentities, signal: SIGTERM) @@ -546,6 +554,8 @@ package final class SpawnedProcessGroup: @unchecked Sendable { processIdentities.formUnion(self.currentProcessGroupMemberIdentities()) if self.isRunning, let rootIdentity = self.rootIdentity { processIdentities.insert(rootIdentity) + } else if let rootIdentity = self.rootIdentity { + processIdentities.remove(rootIdentity) } Self.signal(processIdentities: processIdentities, signal: SIGKILL) diff --git a/Sources/CodexBarCore/PathEnvironment.swift b/Sources/CodexBarCore/PathEnvironment.swift index 631ac045df..e7ae0eff95 100644 --- a/Sources/CodexBarCore/PathEnvironment.swift +++ b/Sources/CodexBarCore/PathEnvironment.swift @@ -10,6 +10,11 @@ import Musl import Security #endif +#if os(Linux) +@_silgen_name("pipe2") +private func linuxPipe2(_ pipeDescriptors: UnsafeMutablePointer, _ flags: Int32) -> Int32 +#endif + public enum PathPurpose: Hashable, Sendable { case rpc case tty @@ -641,10 +646,11 @@ public enum CodexLaunchPreflight { public enum ShellCommandLocator { #if canImport(Darwin) - private static let shellSpawnFlags = Int16(POSIX_SPAWN_SETSID) + private static let shellSpawnFlags = Int16(POSIX_SPAWN_SETSID | POSIX_SPAWN_CLOEXEC_DEFAULT) #else private static let shellSpawnFlags: Int16 = 0x80 // glibc/musl POSIX_SPAWN_SETSID. #endif + private static let shellSpawnLock = NSLock() static func test_runShellCommand( shell: String, @@ -654,6 +660,10 @@ public enum ShellCommandLocator { self.runShellCommand(shell: shell, arguments: arguments, timeout: timeout) } + static func test_makeCloseOnExecPipe() -> (read: Int32, write: Int32)? { + self.makeCloseOnExecPipe() + } + static var test_shellSpawnFlags: Int16 { self.shellSpawnFlags } @@ -741,29 +751,58 @@ public enum ShellCommandLocator { } } - // swiftlint:disable cyclomatic_complexity function_body_length + private static func makeCloseOnExecPipe() -> (read: Int32, write: Int32)? { + var fds: (read: Int32, write: Int32) = (-1, -1) + #if os(Linux) + // Glibc and Musl export pipe2, but their Swift modules do not consistently declare it. + guard withUnsafeMutablePointer(to: &fds, { + $0.withMemoryRebound(to: Int32.self, capacity: 2) { linuxPipe2($0, O_CLOEXEC) == 0 } + }) else { return nil } + #else + guard withUnsafeMutablePointer(to: &fds, { + $0.withMemoryRebound(to: Int32.self, capacity: 2) { pipe($0) == 0 } + }) else { return nil } + + for fd in [fds.read, fds.write] { + let flags = fcntl(fd, F_GETFD) + guard flags >= 0, fcntl(fd, F_SETFD, flags | FD_CLOEXEC) == 0 else { + close(fds.read) + close(fds.write) + return nil + } + } + #endif + return fds + } + + // swiftlint:disable cyclomatic_complexity /// Runs a shell command, draining both stdout and stderr concurrently so that /// verbose shell init scripts (oh-my-zsh, nvm, pyenv, etc.) cannot deadlock on /// a full pipe buffer. The child is launched via `posix_spawn` with /// `POSIX_SPAWN_SETSID` so it cannot take ownership of the caller's controlling - /// terminal. The new session also makes the child its own process-group leader - /// before `exec`, which guarantees that subsequent `kill(-pgid, ...)` calls reach - /// background helpers spawned by shell init, both after timeout and normal exit. + /// terminal. The new session also makes the child its own process-group leader; + /// cleanup tracks both that group and helpers retaining the command's output pipes. fileprivate static func runShellCommand( shell: String, arguments: [String], timeout: TimeInterval) -> Data? { + // Darwin needs a lock around raw descriptor creation, close-on-exec flagging, + // and spawn. Linux creates close-on-exec descriptors atomically with pipe2. + self.shellSpawnLock.lock() + var shellSpawnLockHeld = true + defer { + if shellSpawnLockHeld { + self.shellSpawnLock.unlock() + } + } + // Pipes for stdout/stderr. stdin is redirected from /dev/null in the child - // via posix_spawn_file_actions_addopen below. - var stdoutFds: (read: Int32, write: Int32) = (-1, -1) - var stderrFds: (read: Int32, write: Int32) = (-1, -1) - guard withUnsafeMutablePointer(to: &stdoutFds, { - $0.withMemoryRebound(to: Int32.self, capacity: 2) { pipe($0) == 0 } - }) else { return nil } - guard withUnsafeMutablePointer(to: &stderrFds, { - $0.withMemoryRebound(to: Int32.self, capacity: 2) { pipe($0) == 0 } - }) else { + // via posix_spawn_file_actions_addopen below. Close-on-exec prevents a + // concurrently spawned probe from retaining these descriptors and being + // mistaken for one of this probe's output holders during cleanup. + guard let stdoutFds = self.makeCloseOnExecPipe() else { return nil } + guard let stderrFds = self.makeCloseOnExecPipe() else { close(stdoutFds.read); close(stdoutFds.write) return nil } @@ -850,15 +889,14 @@ public enum ShellCommandLocator { // once every descendant in the process group also closes them. close(stdoutFds.write) close(stderrFds.write) + self.shellSpawnLock.unlock() + shellSpawnLockHeld = false guard spawnResult == 0 else { close(stdoutFds.read); close(stderrFds.read) return nil } - // POSIX_SPAWN_SETSID guarantees the child's session ID and pgid equal its pid. - let pgid: pid_t = pid - // Track EOF on each pipe so we can wait for full drain instead of sleeping. // The readability handler fires with empty data when every writer end is // closed (i.e. the child *and* any inheriting background helpers are gone). @@ -893,27 +931,18 @@ public enum ShellCommandLocator { } } - // Reap the child on a background queue and signal a semaphore on exit. - let exitSemaphore = DispatchSemaphore(value: 0) - let waitPid = pid - DispatchQueue.global(qos: .userInitiated).async { - var status: Int32 = 0 - while waitpid(waitPid, &status, 0) == -1, errno == EINTR { - // retry - } - exitSemaphore.signal() + // Adopt the already-spawned session so cleanup can also discover helpers + // that escape into a new process group while retaining our output pipes. + let process = SpawnedProcessGroup.adopt( + pid: pid, + outputFileDescriptors: [stdoutFds.read, stderrFds.read]) + let deadline = Date().addingTimeInterval(timeout) + while process.isRunning, Date() < deadline { + usleep(10000) } - let finishedInTime = exitSemaphore.wait(timeout: .now() + timeout) == .success - - if !finishedInTime { - kill(-pgid, SIGTERM) - kill(pid, SIGTERM) - if exitSemaphore.wait(timeout: .now() + 0.4) != .success { - kill(-pgid, SIGKILL) - kill(pid, SIGKILL) - _ = exitSemaphore.wait(timeout: .now() + 1.0) - } + if process.isRunning { + process.terminateSynchronously() stdoutHandle.readabilityHandler = nil stderrHandle.readabilityHandler = nil if stdoutDone.fire() { @@ -925,15 +954,14 @@ public enum ShellCommandLocator { return nil } - // Normal completion — clean up any background children spawned by shell init. - // Without this, helpers that inherited stdout/stderr keep the pipe write ends - // open and we never see EOF on the read ends. - kill(-pgid, SIGTERM) + // Normal completion — clean up background children spawned by shell init, + // including session-escaped helpers that still hold our output pipes open. + process.terminateSynchronously() // Wait for both pipes to deliver EOF so no buffered bytes are lost. // Bounded so a stuck handler can't hang the caller indefinitely. if drainGroup.wait(timeout: .now() + 0.4) != .success { - kill(-pgid, SIGKILL) + process.terminateSynchronously(grace: 0) } if drainGroup.wait(timeout: .now() + 0.6) != .success { stdoutHandle.readabilityHandler = nil @@ -948,7 +976,7 @@ public enum ShellCommandLocator { return stdoutCollector.drain() } - // swiftlint:enable cyclomatic_complexity function_body_length + // swiftlint:enable cyclomatic_complexity private static func runShellCapture(_ shell: String?, _ timeout: TimeInterval, _ command: String) -> String? { let shellPath = (shell?.isEmpty == false) ? shell! : "/bin/zsh" diff --git a/Sources/CodexBarCore/UsageFetcher.swift b/Sources/CodexBarCore/UsageFetcher.swift index 843934fbd6..1c2f4c9b7c 100644 --- a/Sources/CodexBarCore/UsageFetcher.swift +++ b/Sources/CodexBarCore/UsageFetcher.swift @@ -949,13 +949,6 @@ enum RPCWireError: Error, LocalizedError { } } -typealias CodexExecutableResolver = @Sendable (_ environment: [String: String], _ executable: String) -> String? - -let defaultCodexExecutableResolver: CodexExecutableResolver = { environment, executable in - BinaryLocator.resolveCodexBinary(env: environment) - ?? TTYCommandRunner.which(executable) -} - /// RPC helper used on background tasks; safe because we confine it to the owning task. private final class CodexRPCClient: @unchecked Sendable { private static let log = CodexBarLog.logger(LogCategories.codexRPC) @@ -1012,16 +1005,19 @@ private final class CodexRPCClient: @unchecked Sendable { } self.stdoutLineContinuation = stdoutContinuation - let resolvedExec = resolveExecutable(environment, executable) + let resolution = resolveExecutable(environment, executable) - guard let resolvedExec else { + guard let resolution else { Self.log.warning("Codex RPC binary not found", metadata: ["binary": executable]) throw CodexStatusProbeError.codexNotInstalled } + let resolvedExec = resolution.executable var env = environment + let loginPATH = resolution.loginPATH ?? LoginShellPathCache.shared.current env["PATH"] = PathBuilder.effectivePATH( purposes: [.rpc, .nodeTooling], - env: env) + env: env, + loginPATH: loginPATH) self.process.environment = env self.process.executableURL = URL(fileURLWithPath: "/usr/bin/env") @@ -1240,7 +1236,6 @@ public struct UsageFetcher: Sendable { self.requestTimeoutSeconds = 3.0 self.codexExecutableResolver = defaultCodexExecutableResolver self.codexArguments = ["-s", "read-only", "-a", "untrusted", "app-server"] - LoginShellPathCache.shared.captureOnce() } init( @@ -1255,7 +1250,6 @@ public struct UsageFetcher: Sendable { self.requestTimeoutSeconds = requestTimeoutSeconds self.codexExecutableResolver = codexExecutableResolver self.codexArguments = codexArguments - LoginShellPathCache.shared.captureOnce() } public func loadLatestUsage(keepCLISessionsAlive: Bool = false) async throws -> UsageSnapshot { diff --git a/Tests/CodexBarTests/CodexExecutableResolverTests.swift b/Tests/CodexBarTests/CodexExecutableResolverTests.swift new file mode 100644 index 0000000000..72117dfd96 --- /dev/null +++ b/Tests/CodexBarTests/CodexExecutableResolverTests.swift @@ -0,0 +1,42 @@ +import Foundation +import Testing +@testable import CodexBarCore + +struct CodexExecutableResolverTests { + @Test + func `explicit native override skips login path capture`() { + let resolved = resolveCodexExecutableForRPC( + environment: ["CODEX_CLI_PATH": "/usr/bin/true"], + executable: "codex", + captureLoginPATH: { + Issue.record("Native override should not capture a login-shell PATH") + return nil + }) + + #expect(resolved?.executable == "/usr/bin/true") + #expect(resolved?.loginPATH == nil) + } + + @Test + func `explicit script override captures login path for env based launchers`() throws { + let scriptURL = FileManager.default.temporaryDirectory + .appendingPathComponent("codexbar-script-override-\(UUID().uuidString)") + try Data("#!/usr/bin/env node\n".utf8).write(to: scriptURL) + try FileManager.default.setAttributes([.posixPermissions: 0o700], ofItemAtPath: scriptURL.path) + defer { try? FileManager.default.removeItem(at: scriptURL) } + + let loginPATH = ["/custom/node/bin", "/usr/bin"] + var captureCount = 0 + let resolved = resolveCodexExecutableForRPC( + environment: ["CODEX_CLI_PATH": scriptURL.path], + executable: "codex", + captureLoginPATH: { + captureCount += 1 + return loginPATH + }) + + #expect(resolved?.executable == scriptURL.path) + #expect(resolved?.loginPATH == loginPATH) + #expect(captureCount == 1) + } +} diff --git a/Tests/CodexBarTests/PathBuilderTests.swift b/Tests/CodexBarTests/PathBuilderTests.swift index 7a459308db..6c989256bc 100644 --- a/Tests/CodexBarTests/PathBuilderTests.swift +++ b/Tests/CodexBarTests/PathBuilderTests.swift @@ -96,7 +96,7 @@ struct PathBuilderTests { let escapedMarker = Self.shellSingleQuoted(marker) let script = """ ( - trap '' TERM + trap '' HUP TERM touch \(escapedMarker) while :; do sleep 1; done ) & diff --git a/Tests/CodexBarTests/ShellCommandForegroundTests.swift b/Tests/CodexBarTests/ShellCommandForegroundTests.swift index 1880a78aa1..691593dd37 100644 --- a/Tests/CodexBarTests/ShellCommandForegroundTests.swift +++ b/Tests/CodexBarTests/ShellCommandForegroundTests.swift @@ -9,6 +9,7 @@ struct ShellCommandForegroundTests { let flags = ShellCommandLocator.test_shellSpawnFlags #expect(flags & Int16(POSIX_SPAWN_SETSID) != 0) + #expect(flags & Int16(POSIX_SPAWN_CLOEXEC_DEFAULT) != 0) #expect(flags & Int16(POSIX_SPAWN_SETPGROUP) == 0) } } diff --git a/Tests/CodexBarTests/ShellCommandLocatorProcessTests.swift b/Tests/CodexBarTests/ShellCommandLocatorProcessTests.swift new file mode 100644 index 0000000000..7736323586 --- /dev/null +++ b/Tests/CodexBarTests/ShellCommandLocatorProcessTests.swift @@ -0,0 +1,83 @@ +#if canImport(Darwin) +import Darwin +#else +import Glibc +#endif +import Foundation +import Testing +@testable import CodexBarCore + +struct ShellCommandLocatorProcessTests { + @Test + func `shell probe pipe descriptors close across unrelated execs`() throws { + let fds = try #require(ShellCommandLocator.test_makeCloseOnExecPipe()) + defer { + close(fds.read) + close(fds.write) + } + + for fd in [fds.read, fds.write] { + let flags = fcntl(fd, F_GETFD) + #expect(flags >= 0) + #expect(flags & FD_CLOEXEC != 0) + } + } + + @Test + func `shell runner terminates session escaped partial output holders after timeout`() throws { + let pidFile = FileManager.default.temporaryDirectory + .appendingPathComponent("codexbar-shell-runner-timeout-\(UUID().uuidString)") + .path + let stdoutPIDFile = "\(pidFile).stdout" + let stderrPIDFile = "\(pidFile).stderr" + let script = """ + import os + import signal + import sys + import time + + for stream, suffix in ((1, ".stdout"), (2, ".stderr")): + child = os.fork() + if child == 0: + os.setsid() + signal.signal(signal.SIGHUP, signal.SIG_IGN) + signal.signal(signal.SIGTERM, signal.SIG_IGN) + os.close(2 if stream == 1 else 1) + with open(sys.argv[1] + suffix, "w") as handle: + handle.write(str(os.getpid())) + while True: + time.sleep(1) + + while not all(os.path.exists(sys.argv[1] + suffix) for suffix in (".stdout", ".stderr")): + time.sleep(0.01) + time.sleep(1000) + """ + + let start = Date() + let data = ShellCommandLocator.test_runShellCommand( + shell: "/usr/bin/python3", + arguments: ["-c", script, pidFile], + timeout: 0.2) + let elapsed = Date().timeIntervalSince(start) + + let pids = try [stdoutPIDFile, stderrPIDFile].map { file in + let pidText = try String(contentsOfFile: file, encoding: .utf8) + .trimmingCharacters(in: .whitespacesAndNewlines) + return try #require(pid_t(pidText)) + } + defer { + for pid in pids { + kill(pid, SIGKILL) + } + for file in [stdoutPIDFile, stderrPIDFile] { + try? FileManager.default.removeItem(atPath: file) + } + } + + #expect(data == nil) + #expect(elapsed < 3.0, "Timed-out PATH probes should remain bounded") + for pid in pids { + #expect(kill(pid, 0) != 0) + } + } +}