From 7214577cf111ac5f4e28ff76c0658d6ff2e189ce Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sun, 2 Aug 2026 14:03:57 -0700 Subject: [PATCH] test(macos): fix three races in the bounded process suites (#118196) BoundedCommandTests and BoundedProcessTests failed nondeterministically: 4/20 runs idle, 7/8 under CPU saturation. Three separate causes. 1. #require inside a retry loop. waitForPID polled through readPID with `try?`, but #require records an issue even when its error is swallowed, so the first read of a created-but-not-yet-written pid file failed the test outright. Added a non-recording pollPID for the polling path and kept the recording read as the authoritative final attempt. The two single-read call sites now poll too - echo $$ > file creates and writes in two steps, so any single read can see a missing or empty file. 2. A 0.1s deadline racing process spawn. BoundedCommand starts its timeout concurrently with the spawn, so the deadline also bounded /bin/sh starting and publishing its pid; under load the child was killed before it ever wrote the file. Wait for the pid while the run is in flight and give the child a deadline well clear of spawn cost. 3. A 1s per-process budget on the concurrent fan-outs. Instrumenting the deadline showed a stalled run observed all 64 exits at ~3.1s, clustered within 100ms of each other - a global stall, not a straggler. Those tests assert that no exit is lost during monitor registration, not latency, so the timeout should not double as a performance assertion. A missed exit still fails: the 50ms pollUntilExit fallback would never complete. Also widened waitUntilGone, since reaping is asynchronous. No production code changed. Proof: 30/30 idle and 20/20 under full 32-core saturation, against 16/20 and 1/8 before. --- .../BoundedCommandTests.swift | 63 ++++++++++++++++--- .../BoundedProcessTests.swift | 47 ++++++++++---- 2 files changed, 87 insertions(+), 23 deletions(-) diff --git a/apps/macos/Tests/OpenClawIPCTests/BoundedCommandTests.swift b/apps/macos/Tests/OpenClawIPCTests/BoundedCommandTests.swift index c3e4ce17603e..5929bff7e189 100644 --- a/apps/macos/Tests/OpenClawIPCTests/BoundedCommandTests.swift +++ b/apps/macos/Tests/OpenClawIPCTests/BoundedCommandTests.swift @@ -22,18 +22,61 @@ struct BoundedCommandTests { let clock = ContinuousClock() let startedAt = clock.now - let output = await BoundedCommand.run( - path: "/bin/sh", - arguments: ["-c", "echo $$ > \"$PID_FILE\"; trap '' TERM; exec /bin/sleep 30"], - environment: ["PID_FILE": pidFile.path], - timeout: 0.1) + // BoundedCommand starts its deadline concurrently with the spawn, so the + // timeout also bounds `/bin/sh` starting up and publishing its pid. A + // deadline near spawn latency turns that into a coin flip: under load the + // child is killed before it ever writes the file. Keep it well clear of + // spawn cost; what this test asserts is the force-kill, not spawn speed. + let runTask = Task { + await BoundedCommand.run( + path: "/bin/sh", + arguments: ["-c", "echo $$ > \"$PID_FILE\"; trap '' TERM; exec /bin/sleep 30"], + environment: ["PID_FILE": pidFile.path], + timeout: 2.0) + } + + let pid = try await Self.waitForPID(in: pidFile) + let output = await runTask.value #expect(output == nil) - #expect(startedAt.duration(to: clock.now) < .seconds(1)) - let pidString = try String(contentsOf: pidFile, encoding: .utf8) + #expect(startedAt.duration(to: clock.now) < .seconds(10)) + #expect(Self.waitUntilGone(pid)) + } + + /// Non-recording parse for polling. `#require` records an issue even when the + /// error it throws is swallowed by `try?`, so a retry loop must not use it or + /// the first not-yet-written read fails the test outright. + private static func pollPID(in file: URL) -> pid_t? { + guard let text = try? String(contentsOf: file, encoding: .utf8) else { return nil } + return pid_t(text.trimmingCharacters(in: .whitespacesAndNewlines)) + } + + /// The child creates the pid file and writes to it in two steps, so a single + /// read can observe a missing *or* empty file. Poll until it parses. + private static func waitForPID(in file: URL) async throws -> pid_t { + let deadline = ContinuousClock.now + .seconds(10) + while ContinuousClock.now < deadline { + if let pid = self.pollPID(in: file) { + return pid + } + try await Task.sleep(for: .milliseconds(10)) + } + let text = try String(contentsOf: file, encoding: .utf8) .trimmingCharacters(in: .whitespacesAndNewlines) - let pid = try #require(pid_t(pidString)) - #expect(kill(pid, 0) == -1) - #expect(errno == ESRCH) + return try #require(pid_t(text)) + } + + /// Reaping is asynchronous, so the process can still be visible for a moment + /// after `run` returns. + private static func waitUntilGone(_ pid: pid_t) -> Bool { + let deadline = Date().addingTimeInterval(5) + while Date() < deadline { + errno = 0 + if kill(pid, 0) == -1, errno == ESRCH { + return true + } + usleep(10000) + } + return false } } diff --git a/apps/macos/Tests/OpenClawIPCTests/BoundedProcessTests.swift b/apps/macos/Tests/OpenClawIPCTests/BoundedProcessTests.swift index 0f221dbdf1bf..50914ff5dec4 100644 --- a/apps/macos/Tests/OpenClawIPCTests/BoundedProcessTests.swift +++ b/apps/macos/Tests/OpenClawIPCTests/BoundedProcessTests.swift @@ -4,6 +4,19 @@ import Testing @testable import OpenClaw struct BoundedProcessTests { + /// The two fan-out tests below assert that no exit notification is *lost* when a + /// child exits while its monitor is being registered. They are not latency + /// assertions, so their timeout must not double as one. + /// + /// Spawning dozens of processes at once periodically stalls the whole fan-out: + /// instrumenting the deadline showed every one of the 64 exits observed at + /// ~3.1s, clustered within 100ms of each other, rather than a single slow + /// straggler. A 1s per-process budget lost that coin flip roughly one run in + /// five, failing 63 of 64 at once. A generous budget still fails hard if an + /// exit is genuinely missed — the 50ms `pollUntilExit` fallback would never + /// complete — while a merely-loaded machine passes. + private static let concurrentSpawnTimeout: TimeInterval = 30 + @Test func `captures output without waiting for inherited handles`() async throws { let startedAt = ContinuousClock.now let result = try await BoundedProcess.run( @@ -36,7 +49,7 @@ struct BoundedProcessTests { try await BoundedProcess.run( path: "/usr/bin/true", arguments: [], - timeout: 1).terminationStatus + timeout: Self.concurrentSpawnTimeout).terminationStatus } } return try await group.reduce(into: []) { $0.append($1) } @@ -61,7 +74,7 @@ struct BoundedProcessTests { try await BoundedProcess.run( path: script.path, arguments: [], - timeout: 1).terminationStatus + timeout: Self.concurrentSpawnTimeout).terminationStatus } } return try await group.reduce(into: []) { $0.append($1) } @@ -103,8 +116,8 @@ struct BoundedProcessTests { #expect(error is BoundedProcessError) } - let parentPID = try self.readPID(from: parentPIDFile) - let childPID = try self.readPID(from: childPIDFile) + let parentPID = try await self.waitForPID(in: parentPIDFile) + let childPID = try await self.waitForPID(in: childPIDFile) #expect(ContinuousClock.now - startedAt < .seconds(3)) #expect(self.waitUntilGone(parentPID)) #expect(self.waitUntilGone(childPID)) @@ -178,30 +191,38 @@ struct BoundedProcessTests { #expect(!(error is BoundedProcessError)) } - let producerPID = try self.readPID(from: pidFile) + let producerPID = try await self.waitForPID(in: pidFile) #expect(ContinuousClock.now - startedAt < .seconds(2)) #expect(self.waitUntilGone(producerPID)) } - private func readPID(from file: URL) throws -> pid_t { - let value = try String(contentsOf: file, encoding: .utf8) - .trimmingCharacters(in: .whitespacesAndNewlines) - return try #require(pid_t(value)) + /// Non-recording parse for polling. `#require` records an issue even when the + /// error it throws is swallowed by `try?`, so `waitForPID` cannot retry through + /// `readPID`: the first read of a created-but-not-yet-written pid file would + /// fail the test despite the retry succeeding a moment later. + private func pollPID(from file: URL) -> pid_t? { + guard let text = try? String(contentsOf: file, encoding: .utf8) else { return nil } + return pid_t(text.trimmingCharacters(in: .whitespacesAndNewlines)) } + /// `echo $$ > file` creates the file and writes to it in two steps, so a single + /// read can observe a missing *or* empty file. Poll until it parses, and only + /// then fall back to the recording read so a genuine absence still fails. private func waitForPID(in file: URL) async throws -> pid_t { - let deadline = ContinuousClock.now + .seconds(1) + let deadline = ContinuousClock.now + .seconds(10) while ContinuousClock.now < deadline { - if let value = try? self.readPID(from: file) { + if let value = self.pollPID(from: file) { return value } try await Task.sleep(for: .milliseconds(10)) } - return try self.readPID(from: file) + let text = try String(contentsOf: file, encoding: .utf8) + .trimmingCharacters(in: .whitespacesAndNewlines) + return try #require(pid_t(text)) } private func waitUntilGone(_ pid: pid_t) -> Bool { - let deadline = Date().addingTimeInterval(1) + let deadline = Date().addingTimeInterval(5) while Date() < deadline { errno = 0 if kill(pid, 0) == -1, errno == ESRCH {