Skip to content

Commit 24105e5

Browse files
bryant24haoclaude
andcommitted
Address Codex bot review: fix timeout race classification
- terminateProcess() now returns Bool: true if it actually killed the process, false if it had already exited. The timeout task only throws .timedOut when the process was actually killed, avoiding false timeouts for near-threshold commands. - Race guard uses both terminationReason == .uncaughtSignal AND elapsed time >= timeout to distinguish our timeout kills from unrelated signal crashes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 710061c commit 24105e5

1 file changed

Lines changed: 18 additions & 8 deletions

File tree

Sources/CodexBarCore/Host/Process/SubprocessRunner.swift

Lines changed: 18 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -60,8 +60,10 @@ public enum SubprocessRunner {
6060
}
6161

6262
/// Terminates a process and its process group, escalating from SIGTERM to SIGKILL.
63-
private static func terminateProcess(_ process: Process, processGroup: pid_t?) {
64-
guard process.isRunning else { return }
63+
/// Returns `true` if the process was actually killed, `false` if it had already exited.
64+
@discardableResult
65+
private static func terminateProcess(_ process: Process, processGroup: pid_t?) -> Bool {
66+
guard process.isRunning else { return false }
6567
process.terminate()
6668
if let pgid = processGroup {
6769
kill(-pgid, SIGTERM)
@@ -76,6 +78,7 @@ public enum SubprocessRunner {
7678
}
7779
kill(process.processIdentifier, SIGKILL)
7880
}
81+
return true
7982
}
8083

8184
// MARK: - Public API
@@ -138,19 +141,26 @@ public enum SubprocessRunner {
138141
group.addTask {
139142
try await Task.sleep(for: .seconds(timeout))
140143
// Kill the process BEFORE throwing so the exit-code task can complete
141-
// and withThrowingTaskGroup can exit promptly.
142-
self.terminateProcess(process, processGroup: processGroup)
144+
// and withThrowingTaskGroup can exit promptly. Only throw if we
145+
// actually killed the process; if it already exited, let the exit
146+
// code win the race naturally.
147+
guard self.terminateProcess(process, processGroup: processGroup) else {
148+
return await exitCodeTask.value
149+
}
143150
throw SubprocessRunnerError.timedOut(label)
144151
}
145152
let code = try await group.next()!
146153
group.cancelAll()
147154
return code
148155
}
149156

150-
// Race guard: if the timeout task killed the process but the exit code arrived
151-
// at group.next() before the .timedOut throw, the process will have been killed
152-
// by a signal. Reclassify as timeout so callers get the correct error type.
153-
if process.terminationReason == .uncaughtSignal {
157+
// Race guard: our timeout task killed the process (SIGTERM/SIGKILL), but
158+
// the exit code arrived at group.next() before the .timedOut throw.
159+
// Detect by requiring BOTH a signal termination AND elapsed time at/past the
160+
// timeout — this avoids misclassifying processes that crash on their own.
161+
if process.terminationReason == .uncaughtSignal,
162+
Date().timeIntervalSince(start) >= timeout - 0.5
163+
{
154164
let duration = Date().timeIntervalSince(start)
155165
self.log.warning(
156166
"Subprocess error",

0 commit comments

Comments
 (0)