mirror of
https://github.com/openclaw/openclaw.git
synced 2026-08-18 16:41:45 -06:00
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.
This commit is contained in:
committed by
GitHub
parent
dc9c7693f7
commit
7214577cf1
@@ -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
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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 {
|
||||
|
||||
Reference in New Issue
Block a user