fix(daemon): own the openclaw CLI process tree so its deadline is enforceable (MUL-5467, MUL-5630) - #6275
Conversation
…orceable (MUL-5467) openclawCLITimeout has carried a "Known gap (deliberately not fixed here)" note since multica-ai#6084: the 5s deadline could not actually bound an `openclaw config ...` invocation. This closes it, and fixes a second OpenClaw misbehaviour found alongside it. ## Bug 1 — the deadline was decorative exec.CommandContext kills only the direct child. cmd.Output() blocks in Wait() until the output pipes os/exec manages reach EOF, and cancelling the context does not unblock that io.Copy. So when openclaw leaves a descendant holding stdout — its `openclaw-config` helper, or cmd.exe → node on Windows — the call runs for the *descendant's* lifetime. multica-ai#6084 measured this on linux/dash (a shim whose backgrounded child slept 6s took 6.01s against a 150ms deadline), tried a cmd.WaitDelay backstop, and reverted it on review because it bounds the call by leaving the descendant running — trading a hang for a process leak that nothing on Unix reaps. The note left behind names the real fix: process-tree ownership. Three call sites had the shape: - execenv.execOpenclawCLI on the task's critical path (Prepare → prepareOpenclawConfig) - agent.detectCLIVersion runs for every provider inside the daemon's blocking registration preflight - agent.discoverOpenclawAgents four invocations per call ## Bug 2 — the answer was already on stdout Measured on an OpenClaw host (openclaw 2026.5.27): openclaw --version 258ms exits cleanly openclaw config file 60s correct path printed, then never exits openclaw agents list 60s correct list printed, then never exits Waiting for exit turns two working commands into a task-fatal error: agent_error.process_failure (prepare execution environment: execenv: prepare openclaw config: locate openclaw active config: openclaw config file: context deadline exceeded (process: signal: killed)) The contract of these commands is "print a value". Once the value has arrived and nothing more is coming, whether the process tidies itself up is not the caller's business, and it should not fail a chat task. ## The fix Two helpers in server/pkg/agent, both built on the process-group primitives codex/claude/opencode already use — no new platform-specific functions: - RunCollect: hands os/exec an *os.File instead of a buffer, so os/exec starts no copy goroutine and Wait returns the instant the direct child exits. Then signals the child's process group, which reaps the helper and releases the last write end, so our own readers see EOF. Unlike WaitDelay this reports the CLI's real exit status, which matters because a failed `--version` probe skips runtime registration entirely, and because openclawShimDiagnostic type-switches on *exec.ExitError. - RunCollectQuiet: returns when the process exits, or when stdout has produced output that then stayed idle for 400ms — whichever comes first. Reports quiet=true on the idle path so callers can log the CLI's failure to exit without failing on it. The idle shortcut deliberately cannot mask a silent CLI: with no output there is nothing to salvage, so the deadline still governs and the call still fails. A non-zero exit still fails too, with stderr intact. execOpenclawCLI keeps its shim diagnostics and its context-attribution order byte-for-byte; only the exec mechanism changes. discoverOpenclawAgents keeps its existing 30s function-level budget rather than gaining a per-attempt one. Windows: there is no process group to signal, so calls are bounded (by the drain grace) but a descendant can still outlive one. Owning descendants there needs a Job Object and is not attempted here. ## Verification - Both packages green with -race; 6 consecutive runs, no flakes. - All 51 existing Openclaw* tests in execenv pass, including TestExecOpenclawCLITimeoutIsNotMisdiagnosedAsMissingInterpreter and TestExecOpenclawCLICancellationIsWrapped — their shims produce no stdout, so the salvage path is not reachable and the errors.Is(err, context.DeadlineExceeded) contract is preserved. - 13 new tests. Both mechanisms are mutation-verified: * RunCollect reverted to buffers with no group reap → the guard test hangs and the dump is the production stack: cmd.Wait → awaitGoroutines blocked, writerDescriptor parked in io.Copy. * RunCollectQuiet's idle path disabled → both the unit test and the execenv end-to-end test fail after a full 60s.
|
Someone is attempting to deploy a commit to the IndexLabs Team on Vercel. A member of the Team first needs to authorize it. |
multica-eve
changed the title
multica-eve
left a comment
multica-eve
left a comment
There was a problem hiding this comment.
这个改动希望让用户在准备本地工作环境时，不再因为辅助工具已经给出答案却迟迟不结束而一直等待，也避免反复使用后留下无用的后台工作。产品方向应该接受，但当前版本仍可能把不完整答案当成成功，因此暂不能合入。
请求修改以下阻塞项：
-
server/pkg/agent/run_collect_quiet.go:139-145在ctx.Done()后只要 stdout 非空就返回成功，既没有等待完整的idleGrace，也没有验证路径/JSON 已完整。持续输出到 deadline 的半段结果因此会被 salvage；server/pkg/agent/run_collect_quiet.go:164-165还丢弃了finish()的错误。我的最小复现让后代持续写入到 250ms deadline，10 次中有 9 次被误报成功。请取消 deadline 分支的无条件 salvage，改由调用方用命令专属的完整性规则确认输出，并补“持续输出到超时”“提示后延迟输出正文”“延迟非零退出”的回归测试。 -
quietWriter.Write在server/pkg/agent/run_collect_quiet.go:30-36先解锁 buffer、再更新lastByte。ticker 可能在这个窗口看到新字节配旧时间戳，从而立即误判输出已经安静并截断后续内容。请把 buffer 写入和最后写入时间放在同一临界区内更新。 -
server/pkg/agent/run_collect.go:120-130在 drain grace 超时后直接读取并返回bytes.Buffer，此时io.Copy仍可能并发写入；defer 关闭读端发生在返回值求值之后，存在数据竞争。超时路径应先关闭读端、等待两个读取协程结束，再读取 buffer。RunCollectQuiet在 Windows 上也会在后代仍持有管道时留下被cmd.Wait/复制阻塞的 goroutine；即使 Job Object 留待后续，本 PR 也应保证函数返回前本地管道和 Go goroutine 已可靠收束，并增加 Windows 契约覆盖。
已验证现有 -race 定向测试、ByteIdentical 测试、go vet 和 git diff --check 均通过；上述反例目前不在测试覆盖中。修正这些边界后，这个 EOF/进程组方案可以继续 review。
Three blocking items from review of multica-ai#6275. All three were reproduced before being fixed, and the first one was a genuine correctness hole rather than a style objection. ## 1. The deadline branch salvaged partial answers RunCollectQuiet returned success from the ctx.Done() branch whenever stdout was non-empty, without requiring the idle grace or any check that the answer was finished. A CLI still streaming when the deadline arrived therefore had its truncated output reported as a completed response. Reproduced with a stub emitting a JSON document forever against a 250ms deadline: 9 runs in 10 came back as success carrying an unparseable fragment. The early return now needs two independent conditions, neither sufficient alone: - an OutputComplete rule supplied by the caller accepts the buffer, and - the buffer has since been idle for idleGrace. Reaching the deadline is never success. The captured bytes are still returned alongside the error so a caller with its own rule can inspect them, but the runner no longer decides on the caller's behalf. A nil rule disables the early return entirely, which is the conservative default for a command whose output shape has no completeness rule. The rule cannot be inferred inside the runner, which is the point: only the caller knows whether `{"agents":[{"id":"a"},` is truncated or legitimate. - `--json` subcommands -> agent.JSONOutputComplete (whole buffer parses). - `openclaw config file` -> openclawConfigPathComplete (last non-empty line looks like a path). Deliberately stricter than openclawParseActiveConfigPath, which resolves a relative line through filepath.Abs and would therefore accept a Doctor warning border as an answer. That matters because OpenClaw prints Doctor UI *before* the path (MUL-3136), so an early return on idle output alone hands back the banner as the config path — and nothing downstream catches it. - `agents list` text fallback -> RunCollect (wait for exit). Decorated text has no completeness rule and a silently short agent catalog is worse than a slow one; this path still gains the reaping and the bounded return. ## 2. quietWriter published bytes before their timestamp The buffer write and the last-write timestamp were updated in separate critical sections, so a reader could observe new bytes carrying a stale timestamp and conclude the stream had gone quiet at the moment it was producing — truncating an answer mid-write. Both now move in one critical section (outputBuffer). ## 3. The drain-timeout path raced the readers, and could park a goroutine RunCollect read its buffers after the drain grace expired while io.Copy could still be appending to them; the deferred Close on the read ends ran only after the return values had been evaluated. Separately, RunCollectQuiet handed os/exec io.Writers, so its cmd.Wait goroutine could sit forever on a descendant holding the pipe — on Windows nothing released it, since there is no process group to signal. Both are now structural rather than patched. A single collector owns the pipes for both helpers (the *os.File ownership RunCollect already had), and finish() guarantees that when it returns, every goroutine startCollector spawned has been joined and nothing can still append to the buffers: it reaps the tree, waits bounded for Wait, and if the readers still have not seen EOF it closes the read ends outright — which makes the in-flight Read return — and only then joins them. ## Verification - Both packages pass with -race under CI's flags (-p 2 -parallel 2). - All pre-existing Openclaw* tests still pass, including the timeout attribution and shim diagnostic ones. - New regression tests, all three named in the review: continuous output until the deadline (must not be success), a prompt followed by a delayed body (must not return the prompt), and a late non-zero exit within the grace (must be reported as the failure it is). Plus the completeness rules per subcommand, the banner-then-path case end to end through execOpenclawCLI, goroutine collection on both helpers, and the two outputBuffer contracts. - Windows contract coverage added (run_collect_windows_test.go) with a scoped step in the existing windows-execenv CI job, since the close-the-read-ends path is unreachable on Unix where the group kill releases the pipe. - Mutation-verified: restoring the unconditional salvage fails both the unit and the execenv regression; dropping the completeness rule from execOpenclawCLI makes the Doctor banner come back as the config path.
|
Thanks — all three were real, and item 1 was a correctness hole rather than a style objection. I reproduced each one before fixing it, and your repro number for item 1 reproduced exactly: 9 runs in 10 against a 250ms deadline. 1. The deadline branch salvaged partial answersYou were right that the code conflated "we have some output" with "we have the output". The early return now needs two independent conditions, neither sufficient alone:
Reaching the deadline is never success. The captured bytes are still returned alongside the error so a caller with its own rule can inspect them, but the runner no longer decides on the caller's behalf. A Per-subcommand rules (
That case is not hypothetical. Timestamped stdout from a live host running openclaw 2026.5.27: Without the rule the early return hands back The text fallback moved to New regression tests, the three you named plus the rule table:
On the grace being the observation window: a CLI that prints a complete answer and then exits non-zero is reported as the failure, as long as it does so within 2.
|
The two execenv fixtures printed the literal path the host had printed, `/root/.openclaw/openclaw.json`. openclawParseActiveConfigPath stats what it parses and tolerates only os.ErrNotExist, so that path resolves differently by environment: ENOENT on a developer machine, where /root does not exist, but EACCES on a Linux CI runner, where it exists and is unreadable. The second case is a hard error, so TestExecOpenclawCLIWaitsForThePathAfterDoctorBanner failed in CI for a reason unrelated to the boundary it covers. Both fixtures now print a real file inside t.TempDir(), which stats cleanly everywhere. The second test only inspects raw stdout today, but a hard-coded path there would be a trap for whoever next adds a parse to it. The literal path stays in the completeness-rule table, which calls the predicate directly and never stats, so it keeps documenting the shape the CLI actually emits.
|
CI caught a fixture bug of mine — pushed Two of the new execenv fixtures printed the literal path the host had printed,
Reproduced the mechanism locally before fixing, by pointing the fixture at a Green locally under CI's own parameters, One note on the failed run for anyone reading its log: |
2 participants
What does this PR do?
It closes the gap
openclawCLITimeouthas documented since #6084, and fixes asecond OpenClaw misbehaviour discovered alongside it.
Bug 1 — the 5s deadline was decorative.
exec.CommandContextkills only thedirect child, and
cmd.Output()blocks inWait()until the output pipesos/exec manages reach EOF. Cancelling the context does not unblock that
io.Copy. So when openclaw leaves a descendant holding stdout — itsopenclaw-confighelper, orcmd.exe → nodeon Windows — the call runs for thedescendant's lifetime.
#6084 measured exactly this (a shim whose backgrounded child slept 6s took 6.01s
against a 150ms deadline), tried a
cmd.WaitDelaybackstop, and reverted it onreview because it bounds the call by leaving the descendant running — trading a
hang for a leak nothing on Unix reaps. The note it left behind names the right
fix, which is what this PR implements:
Three call sites had the shape. One is on the task's critical path
(
Prepare→prepareOpenclawConfig), one runs for every provider inside thedaemon's blocking registration preflight, one makes four invocations per call:
execenv.execOpenclawCLIagent.detectCLIVersionagent.discoverOpenclawAgentsBug 2 — the answer was already on stdout. Measured on an OpenClaw host
running openclaw 2026.5.27:
Waiting for exit turns two working commands into a task-fatal error:
The contract of those commands is "print a value". Once the value has arrived
and nothing more is coming, whether the process tidies itself up is not the
caller's business, and it should not fail a chat task.
Why this approach
WaitDelay.WaitDelaywas already tried andreverted upstream. Beyond the leak, it reports
exec.ErrWaitDelay, which turns"the CLI printed its answer and a helper lingered" into a failed probe — and
a failed
--versionprobe skips runtime registration entirely, so the cure isworse than the disease. Owning the pipes reports the CLI's real exit status,
which
openclawShimDiagnosticalso depends on (it type-switches on*exec.ExitError).configureProcessGroup/signalProcessGroupalready exist and are used by the codex, claude, opencodeand deveco paths. No new platform-specific functions were added.
cursor.goalreadytreats a terminal result as a protocol boundary for a CLI that keeps its worker
alive.
Related Issue
Implements the process-tree ownership that #6084 identified as the proper fix and
tracked as MUL-5467. Relates to #3853 (OpenClaw runtime fails in execenv prep) and
#3403 (agent status stuck at working).
Type of Change
Changes Made
New helpers:
server/pkg/agent/run_collect.go—RunCollecthands os/exec an*os.Fileinstead of a buffer, so os/exec starts no copy goroutine and
Waitreturns theinstant the direct child exits.
reapProcessTreethen signals the child'sprocess group, reaping the helper and releasing the last write end so our own
readers see EOF.
server/pkg/agent/run_collect_quiet.go—RunCollectQuietreturns when theprocess exits, or when stdout has produced output that then stayed idle for
400ms, whichever is first.
quiet=truemarks the idle path so callers can logthe CLI's failure to exit without failing on it.
Call sites:
server/internal/daemon/execenv/openclaw_config.go—execOpenclawCLInowgoes through
RunCollectQuiet. Its shim diagnostics and context-attributionorder are unchanged byte-for-byte; only the exec mechanism differs. The
openclawCLITimeoutdoc is rewritten since the gap it described is closed.server/pkg/agent/claude.go—detectCLIVersionusesRunCollect; thereverted-elsewhere
cmd.WaitDelay = 2sbackstop is removed.server/pkg/agent/models.go—discoverOpenclawAgentsusesRunCollectQuiet, keeping its existing 30s function-level budget rather thangaining a per-attempt one.
Tests:
run_collect_test.go(6),run_collect_quiet_test.go(4),internal/daemon/execenv/openclaw_process_tree_test.go(3).Deliberate non-goals
become bounded (by the drain grace) but a descendant can still outlive one.
That needs a Job Object and is the remaining half of MUL-5467; the code says so
rather than implying otherwise.
with no output there is nothing to salvage, so the deadline still governs and
the call still fails. A non-zero exit still fails too, with stderr intact.
TestPrepareOpenclawConfigFailsClosedOnCLIErrorstill passes.How to Test
cd server && go test ./pkg/agent/... ./internal/daemon/execenv/... -race— both packages pass. All 51 existing
Openclaw*tests in execenv still pass,including
TestExecOpenclawCLITimeoutIsNotMisdiagnosedAsMissingInterpreterand
TestExecOpenclawCLICancellationIsWrapped(their shims emit no stdout, sothe salvage path is unreachable and the
errors.Is(err, context.DeadlineExceeded)contract is preserved).Mutation-check bug 1: in
run_collect.goreplace the*os.Filepipes withbytes.Bufferand delete thereapProcessTreecall, thengo test ./pkg/agent/ -run TestRunCollectReturnsDespitePipeHoldingGrandchild -timeout 40s.It hangs, and the dump is the production stack:
Mutation-check bug 2: disable the idle branch in
run_collect_quiet.go, thenrun
TestRunCollectQuietReturnsOnceOutputGoesIdleandTestExecOpenclawCLIToleratesNonExitingCLI. Both fail after a full 60s(
took 1m0.001s — waited for an exit that never comes).Flake check: 6 consecutive runs of both packages, no failures. A clean
origin/mainworktree was run 3× as a baseline for comparison.Checklist
openclawCLITimeoutdoc comment that described this as an open gap)Risks
back-to-back writes, which is what was measured. A CLI that paused mid-response
for longer than that would be cut short — which is why the helper is scoped to
one-shot commands only and the doc comment says so. Agent execution, where a
pause is meaningful, is untouched.
reapProcessTreeissues the group kill just afterWaitreaped the leader, soin principle the leader's pid is already free for reuse. Sequential pid
allocation makes reuse in that window not a practical concern, and it is the
same window the other backends' cancellation paths already live with. Noted in
the code rather than left implicit.
AI Disclosure
AI tool used: Kiro CLI
Prompt / approach: Both bugs were first diagnosed and fixed on a private
branch while running the daemon against a host with openclaw 2026.5.27, which is
where the timings and the error strings in this description come from. For this PR
the fix was re-derived against current
mainrather than cherry-picked: thatbranch predates several relevant changes here, and notably
mainhas since grownits own process-group helpers, so a straight port would have introduced a parallel
set of them. The AI-assisted loop was: fetch each affected file from
mainandconfirm the bug is still present, read the surrounding commit history to find that
#6084 had already diagnosed bug 1 and chosen to leave it open, implement on top of
the existing primitives, then mutation-verify every new test by reverting the
mechanism it guards and confirming the test fails for the stated reason.
Two things I am happy to change if you would rather have them differently, since
both are judgement calls rather than requirements of the fix:
RunCollect/RunCollectQuietlive inpkg/agentbecause that is where theprocess-group primitives already are, but this makes
internal/daemon/execenvdepend on
pkg/agentin production code for the first time. I can move them toa neutral package instead.
PR. Given fix(daemon): diagnose silent OpenClaw npm shim failures on Windows (MUL-5422) #6084 was deliberately kept narrow, say the word and I will split this
into "process-tree ownership" and "tolerate a CLI that prints then hangs".