Skip to content

Commit b2e2938

Browse files
authored
fix: bound PTY overflow abort latency (#2805)
1 parent 2705446 commit b2e2938

4 files changed

Lines changed: 89 additions & 12 deletions

File tree

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
- Fireworks: track 30-day rated billing spend with an API key and account slug (#2687). Thanks @x0mh0x!
77

88
### Fixed
9+
- PTY probes: abort output-overflow children through a bounded process-group kill path, avoiding minute-long cleanup stalls under load (refs #2792).
910
- Cost store: give the store actor's custom serial executor an `isIsolatingCurrentContext()` implementation — macOS 26+ runtimes consult it before `checkIsolated()`, and its default cannot see through `DispatchQueue.sync`, so every synchronous store bridge tripped "Incorrect actor executor assumption" and the app died on launch.
1011
- Codex: reject Standard/Fast pricing rows that exceed canonical fork-deduplicated usage, preventing copied fork rows and their Fast surcharge from inflating cost estimates (#2754). Thanks @1328189205 for the report and @Yuxin-Qiao for the initial fix and regression-test approach!
1112
- Claude: stop rotating Claude Code's own refresh-token chain on keychain-only installs — ownership evidence is now tri-state with indeterminate treated as CLI-owned, so delegated refreshes can never invalidate credentials CodexBar cannot read (#2745, refs #2634). Thanks @avenoxai!

Sources/CodexBarCore/Host/PTY/TTYCommandRunner.swift

Lines changed: 22 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -266,6 +266,7 @@ enum TTYProcessTreeTerminator {
266266

267267
private enum TTYCommandRunnerTestingOverrides {
268268
@TaskLocal static var postDeadlineDrainDuration: TimeInterval?
269+
@TaskLocal static var outputLimitBytes: Int?
269270
}
270271

271272
/// Executes an interactive CLI inside a pseudo-terminal and returns all captured text.
@@ -661,13 +662,14 @@ public struct TTYCommandRunner {
661662

662663
var cleanedUp = false
663664
var launchedProcess: SpawnedProcessGroup?
665+
var didExceedOutputLimit = false
664666
/// Always tear down the PTY child (and its process group) even if we throw early
665667
/// while bootstrapping the CLI (e.g. when it prompts for login/telemetry).
666668
func cleanup() {
667669
guard !cleanedUp else { return }
668670
cleanedUp = true
669671

670-
if let launchedProcess, launchedProcess.isRunning {
672+
if !didExceedOutputLimit, let launchedProcess, launchedProcess.isRunning {
671673
Self.log.debug("PTY stopping", metadata: ["binary": binaryName])
672674
let exitData = Data("/exit\n".utf8)
673675
try? writeAllToPrimary(exitData)
@@ -679,8 +681,16 @@ public struct TTYCommandRunner {
679681
try? primaryHandle.close()
680682
return
681683
}
682-
launchedProcess.terminateSynchronously()
683-
try? primaryHandle.close()
684+
if didExceedOutputLimit {
685+
// Once the bounded buffer overflows, do not spend seconds sweeping every process's
686+
// descriptors before signaling. Closing the master unblocks a child stuck writing,
687+
// and the scoped abort escalates within its fixed grace window.
688+
try? primaryHandle.close()
689+
launchedProcess.abortSynchronously()
690+
} else {
691+
launchedProcess.terminateSynchronously()
692+
try? primaryHandle.close()
693+
}
684694
TTYCommandRunnerActiveProcessRegistry.unregister(pid: launchedProcess.pid)
685695
}
686696

@@ -740,8 +750,8 @@ public struct TTYCommandRunner {
740750
let isCodex = ttyStatusCommand != nil || options.forceCodexStatusMode
741751
let isCodexStatus = isCodex && trimmed == (ttyStatusCommand ?? "/status")
742752

743-
var buffer = BoundedOutputBuffer()
744-
var didExceedOutputLimit = false
753+
let outputLimitBytes = TTYCommandRunnerTestingOverrides.outputLimitBytes ?? BoundedOutputBuffer.defaultMaxBytes
754+
var buffer = BoundedOutputBuffer(maxBytes: outputLimitBytes)
745755

746756
func checkOutputLimit() throws {
747757
if didExceedOutputLimit {
@@ -1177,6 +1187,13 @@ extension TTYCommandRunner {
11771187
try TTYCommandRunnerTestingOverrides.$postDeadlineDrainDuration.withValue(duration, operation: operation)
11781188
}
11791189

1190+
static func withOutputLimitOverrideForTesting<T>(
1191+
_ maxBytes: Int,
1192+
operation: () throws -> T) rethrows -> T
1193+
{
1194+
try TTYCommandRunnerTestingOverrides.$outputLimitBytes.withValue(maxBytes, operation: operation)
1195+
}
1196+
11801197
public static func which(_ tool: String) -> String? {
11811198
if let cli = ProviderDescriptorRegistry.all.first(where: { $0.cli.name == tool })?.cli,
11821199
cli.prefersBinaryLocatorForWhich,

Sources/CodexBarCore/Host/Process/SpawnedProcessGroup.swift

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -590,6 +590,35 @@ package final class SpawnedProcessGroup: @unchecked Sendable {
590590
return self.finishSynchronously()
591591
}
592592

593+
/// Abort a process whose output channel is no longer safe to inspect.
594+
///
595+
/// Unlike normal cleanup, this deliberately avoids the system-wide output-holder scan. The caller closes
596+
/// the output descriptor first, then this method targets only the launch process tree and dedicated process
597+
/// group so TERM-to-KILL escalation remains bounded even when process enumeration is slow under load.
598+
@discardableResult
599+
package func abortSynchronously(grace: TimeInterval = 0.4) -> Int32? {
600+
let grace = max(0, grace)
601+
let termDeadline = Date().addingTimeInterval(grace)
602+
var processIdentities = self.currentAbortProcessIdentities()
603+
604+
self.signalOwnedProcessGroup(SIGTERM)
605+
Self.signal(processIdentities: processIdentities, signal: SIGTERM)
606+
607+
while self.abortTargetsRemain(processIdentities), Date() < termDeadline {
608+
usleep(20000)
609+
}
610+
611+
processIdentities.formUnion(self.currentAbortProcessIdentities())
612+
self.signalOwnedProcessGroup(SIGKILL)
613+
Self.signal(processIdentities: processIdentities, signal: SIGKILL)
614+
615+
let killDeadline = Date().addingTimeInterval(grace)
616+
while self.abortTargetsRemain(processIdentities), Date() < killDeadline {
617+
usleep(20000)
618+
}
619+
return self.finishSynchronously()
620+
}
621+
593622
@discardableResult
594623
package func finishSynchronously(timeout: TimeInterval = 1) -> Int32? {
595624
self.termination.requestReap()
@@ -753,6 +782,35 @@ package final class SpawnedProcessGroup: @unchecked Sendable {
753782
return identities
754783
}
755784

785+
private func currentAbortProcessIdentities() -> Set<TTYProcessTreeTerminator.ProcessIdentity> {
786+
var identities = self.observedProcessGroupMembers.snapshot
787+
identities.formUnion(self.currentProcessGroupMemberIdentities())
788+
guard let rootIdentity = self.rootIdentity,
789+
TTYProcessTreeTerminator.isCurrent(rootIdentity)
790+
else {
791+
return identities
792+
}
793+
794+
identities.insert(rootIdentity)
795+
identities.formUnion(
796+
TTYProcessTreeTerminator.descendantPIDs(of: self.pid)
797+
.compactMap(TTYProcessTreeTerminator.processIdentity(for:)))
798+
return identities
799+
}
800+
801+
private func signalOwnedProcessGroup(_ signal: Int32) {
802+
guard let rootIdentity = self.rootIdentity,
803+
TTYProcessTreeTerminator.isCurrent(rootIdentity)
804+
else { return }
805+
_ = kill(-self.processGroup, signal)
806+
}
807+
808+
private func abortTargetsRemain(
809+
_ processIdentities: Set<TTYProcessTreeTerminator.ProcessIdentity>) -> Bool
810+
{
811+
processIdentities.contains(where: TTYProcessTreeTerminator.isCurrent(_:)) || self.hasResidualProcessGroup
812+
}
813+
756814
private func currentOutputHolderIdentities() -> Set<TTYProcessTreeTerminator.ProcessIdentity> {
757815
let excludedPIDs: Set<pid_t> = [getpid(), self.pid]
758816
var holderPIDs = OutputPipeIdentity.holderPIDs(for: self.outputPipes)

Tests/CodexBarTests/BoundedChildProcessProofTests.swift

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -33,19 +33,20 @@ struct BoundedChildProcessProofTests {
3333
let runner = TTYCommandRunner()
3434
let start = ContinuousClock.now
3535
do {
36-
_ = try runner.run(
37-
binary: scriptURL.path,
38-
send: "",
39-
options: .init(timeout: 60, baseEnvironment: environment, initialDelay: 0))
36+
_ = try TTYCommandRunner.withOutputLimitOverrideForTesting(64 * 1024) {
37+
try runner.run(
38+
binary: scriptURL.path,
39+
send: "",
40+
options: .init(timeout: 60, baseEnvironment: environment, initialDelay: 0))
41+
}
4042
Issue.record("Expected the synthetic child to exceed the PTY output limit")
4143
} catch TTYCommandRunner.Error.outputTooLarge {
4244
// Expected: the production runner propagated the bounded-output error.
4345
} catch {
4446
Issue.record("Unexpected overflow error: \(error)")
4547
}
46-
// Prove early abort (well under the 60s timeout) without a load-sensitive tight bound:
47-
// heavily loaded CI shards have taken >10s for PTY drain alone (refs #2792 fallout).
48-
#expect(start.duration(to: .now) < .seconds(30))
48+
// A small test-only limit isolates abort latency from the host's PTY throughput.
49+
#expect(start.duration(to: .now) < .seconds(5))
4950

5051
let pidText = try String(contentsOf: pidURL, encoding: .utf8)
5152
.trimmingCharacters(in: .whitespacesAndNewlines)

0 commit comments

Comments
 (0)