From fb3018ef31faffc1551002767b54b62fcc633076 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sun, 31 May 2026 10:28:23 +0100 Subject: [PATCH] fix(exec): keep timeout classification deterministic --- src/process/supervisor/supervisor.test.ts | 53 +++++++++++++++++++++++ src/process/supervisor/supervisor.ts | 52 +++++++++++++++++++--- 2 files changed, 99 insertions(+), 6 deletions(-) diff --git a/src/process/supervisor/supervisor.test.ts b/src/process/supervisor/supervisor.test.ts index 08bd10ab0f4..af9683e21f8 100644 --- a/src/process/supervisor/supervisor.test.ts +++ b/src/process/supervisor/supervisor.test.ts @@ -1,3 +1,4 @@ +import { performance } from "node:perf_hooks"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import type { SpawnProcessAdapter } from "./types.js"; @@ -266,6 +267,58 @@ describe("process supervisor", () => { expect(exit.timedOut).toBe(true); }); + it("classifies a natural close after a missed overall deadline as timed out", async () => { + vi.useFakeTimers(); + const nowSpy = vi.spyOn(performance, "now").mockReturnValue(1_000); + const adapter = createStubChildAdapter(); + createChildAdapterMock.mockResolvedValue(adapter); + + const supervisor = createProcessSupervisor(); + const run = await spawnChild(supervisor, { + sessionId: "s-timeout-race", + argv: createSilentIdleArgv(), + timeoutMs: 10, + stdinMode: "pipe-closed", + }); + + const exitPromise = run.wait(); + nowSpy.mockReturnValue(1_011); + adapter.settle(0); + + const exit = await exitPromise; + expect(adapter.killMock).not.toHaveBeenCalled(); + expect(exit.reason).toBe("overall-timeout"); + expect(exit.timedOut).toBe(true); + }); + + it("uses the refreshed no-output deadline when a missed timer races natural close", async () => { + vi.useFakeTimers(); + const nowSpy = vi.spyOn(performance, "now").mockReturnValue(1_000); + const adapter = createStubChildAdapter(); + createChildAdapterMock.mockResolvedValue(adapter); + + const supervisor = createProcessSupervisor(); + const run = await spawnChild(supervisor, { + sessionId: "s-no-output-race", + argv: createSilentIdleArgv(), + timeoutMs: 100, + noOutputTimeoutMs: 10, + stdinMode: "pipe-closed", + }); + + const exitPromise = run.wait(); + nowSpy.mockReturnValue(1_005); + adapter.emitStdout("progress"); + nowSpy.mockReturnValue(1_016); + adapter.settle(0); + + const exit = await exitPromise; + expect(adapter.killMock).not.toHaveBeenCalled(); + expect(exit.reason).toBe("no-output-timeout"); + expect(exit.noOutputTimedOut).toBe(true); + expect(exit.timedOut).toBe(true); + }); + it("can stream output without retaining it in RunExit payload", async () => { const adapter = createStubChildAdapter(); createChildAdapterMock.mockResolvedValue(adapter); diff --git a/src/process/supervisor/supervisor.ts b/src/process/supervisor/supervisor.ts index eb86dfc8843..10eff612415 100644 --- a/src/process/supervisor/supervisor.ts +++ b/src/process/supervisor/supervisor.ts @@ -1,4 +1,5 @@ import crypto from "node:crypto"; +import { performance } from "node:perf_hooks"; import { normalizeOptionalString } from "@openclaw/normalization-core/string-coerce"; import { getShellConfig } from "../../agents/shell-utils.js"; import { createChildAdapter } from "./adapters/child.js"; @@ -63,6 +64,34 @@ function isTimeoutReason(reason: TerminationReason) { return reason === "overall-timeout" || reason === "no-output-timeout"; } +function resolveElapsedTimeoutReason(params: { + nowMs: number; + overallTimeoutDeadlineMs: number | null; + noOutputTimeoutDeadlineMs: number | null; +}): TerminationReason | null { + const elapsedDeadlines: Array<{ reason: TerminationReason; deadlineMs: number }> = []; + if (params.overallTimeoutDeadlineMs !== null && params.nowMs >= params.overallTimeoutDeadlineMs) { + elapsedDeadlines.push({ + reason: "overall-timeout", + deadlineMs: params.overallTimeoutDeadlineMs, + }); + } + if ( + params.noOutputTimeoutDeadlineMs !== null && + params.nowMs >= params.noOutputTimeoutDeadlineMs + ) { + elapsedDeadlines.push({ + reason: "no-output-timeout", + deadlineMs: params.noOutputTimeoutDeadlineMs, + }); + } + if (elapsedDeadlines.length === 0) { + return null; + } + elapsedDeadlines.sort((a, b) => a.deadlineMs - b.deadlineMs); + return elapsedDeadlines[0].reason; +} + export function createProcessSupervisor(): ProcessSupervisor { const registry = createRunRegistry(); const active = new Map(); @@ -122,6 +151,8 @@ export function createProcessSupervisor(): ProcessSupervisor { const overallTimeoutMs = clampTimeout(input.timeoutMs); const noOutputTimeoutMs = clampTimeout(input.noOutputTimeoutMs); + let overallTimeoutDeadlineMs: number | null = null; + let noOutputTimeoutDeadlineMs: number | null = null; const setForcedReason = (reason: TerminationReason) => { if (forcedReason) { @@ -143,6 +174,7 @@ export function createProcessSupervisor(): ProcessSupervisor { if (!noOutputTimeoutMs || settled) { return; } + noOutputTimeoutDeadlineMs = performance.now() + noOutputTimeoutMs; if (noOutputTimer) { clearTimeout(noOutputTimer); } @@ -210,11 +242,13 @@ export function createProcessSupervisor(): ProcessSupervisor { }; if (overallTimeoutMs) { + overallTimeoutDeadlineMs = performance.now() + overallTimeoutMs; timeoutTimer = setTimeout(() => { requestCancel("overall-timeout"); }, overallTimeoutMs); } if (noOutputTimeoutMs) { + noOutputTimeoutDeadlineMs = performance.now() + noOutputTimeoutMs; noOutputTimer = setTimeout(() => { requestCancel("no-output-timeout"); }, noOutputTimeoutMs); @@ -237,16 +271,22 @@ export function createProcessSupervisor(): ProcessSupervisor { const waitPromise = (async (): Promise => { const result = await adapter.wait(); + const deadlineReason = resolveElapsedTimeoutReason({ + nowMs: performance.now(), + overallTimeoutDeadlineMs, + noOutputTimeoutDeadlineMs, + }); + const terminalReason = forcedReason ?? deadlineReason; if (settled) { return { - reason: forcedReason ?? "exit", + reason: terminalReason ?? "exit", exitCode: result.code, exitSignal: result.signal, durationMs: Date.now() - startedAtMs, stdout, stderr, - timedOut: isTimeoutReason(forcedReason ?? "exit"), - noOutputTimedOut: forcedReason === "no-output-timeout", + timedOut: isTimeoutReason(terminalReason ?? "exit"), + noOutputTimedOut: terminalReason === "no-output-timeout", }; } settled = true; @@ -255,7 +295,7 @@ export function createProcessSupervisor(): ProcessSupervisor { active.delete(runId); const reason: TerminationReason = - forcedReason ?? (result.signal != null ? ("signal" as const) : ("exit" as const)); + terminalReason ?? (result.signal != null ? ("signal" as const) : ("exit" as const)); const exit: RunExit = { reason, exitCode: result.code, @@ -263,8 +303,8 @@ export function createProcessSupervisor(): ProcessSupervisor { durationMs: Date.now() - startedAtMs, stdout, stderr, - timedOut: isTimeoutReason(forcedReason ?? reason), - noOutputTimedOut: forcedReason === "no-output-timeout", + timedOut: isTimeoutReason(terminalReason ?? reason), + noOutputTimedOut: terminalReason === "no-output-timeout", }; registry.finalize(runId, { reason: exit.reason,