fix(supervisor): bound run_once child wait

[SebTardif]

What Problem This Solves

Fixes an issue where ocm service --once (the __daemon run --once path) would hang forever when a planned env child never exited. After spawn, run_once called child.wait() with no deadline.

On origin/main the hang is here:

ocm/src/supervisor/mod.rs

Lines 809 to 811 in e04c101

	let status = child.wait().map_err(|error| {
	format!("failed waiting for env \"{}\": {error}", spec.env_name)
	})?;

e04c101

This is separate from PR #136, which bounds git worktree helpers.

Why This Change Was Made

run_once now waits through wait_child_with_timeout. The child is polled, then terminated (process group TERM, then KILL) when SERVICE_ONCE_CHILD_TIMEOUT_MS (15s) expires, matching the restart-handoff wait helper. On timeout the supervisor returns an error instead of blocking. Long-running run_until_stopped is unchanged.

User Impact

A stuck env child in once-mode fails after 15s instead of hanging ocm service --once. Children that exit on their own still report their exit status.

Evidence

terminal output from a rustc one-off against /bin/sleep 30. Naive child.wait() is still running after 1s. The timed waiter returns an error at the 200ms deadline and the sleep process is gone.

$ rustc /tmp/naive_sleep_wait.rs -o /tmp/naive_sleep_wait && /tmp/naive_sleep_wait
naive child.wait() still running after 1006ms: true
sleep pid 47907 still alive: true

$ rustc /tmp/timed_sleep_wait.rs -o /tmp/timed_sleep_wait && /tmp/timed_sleep_wait
sleep timed out after 200ms after 223.674792ms
sleep pid 47991 still alive: false

The same deadline-and-kill path is what run_once now uses after spawn_supervisor_child.

$ /tmp/ocm-F005/target/debug/deps/ocm-c8830440f3406352 supervisor::tests::wait_child_with_timeout_kills_sleep_after_deadline --exact --nocapture
running 1 test
test supervisor::tests::wait_child_with_timeout_kills_sleep_after_deadline ... ok
test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 334 filtered out; finished in 0.78s

Real behavior proof

• Behavior or issue addressed: ocm service --once no longer blocks forever on child.wait(). A child that does not exit is killed when the 15s deadline is reached, and the supervisor returns an error.

• Real environment tested: macOS (Darwin 25.6.0 arm64), rustc 1.98.0, ocm checkout /tmp/ocm-F005 on fix/supervisor-run-once-timeout above origin/main e04c10166d6b58932213bf8422ccfc201cf6ac37.

• Exact steps or command run after this patch:

  rustc /tmp/naive_sleep_wait.rs and /tmp/timed_sleep_wait.rs, then run both binaries against /bin/sleep 30. Then run the built lib binary ocm-c8830440f3406352 supervisor::tests::wait_child_with_timeout_kills_sleep_after_deadline --exact --nocapture.

• Evidence after fix: terminal output above. Naive child.wait() was still running after 1006ms with sleep still alive. The timed waiter printed sleep timed out after 200ms after 223.674792ms and sleep pid 47991 still alive: false.

• Observed result after fix: The helper returns a timeout error in about 200ms and the sleep child is gone. run_once uses that same waiter with a 15s named deadline.

• What was not tested: A live launchd/systemd ocm service --once with a real OpenClaw gateway that refuses to exit, and the Windows job-object kill path.

[clawsweeper]

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

[clawsweeper]

Codex review: needs real behavior proof before merge. Reviewed September 2, 2026, 6:53 PM ET / 22:53 UTC.

ClawSweeper review

What this changes

The PR replaces the once-only supervisor’s unbounded child wait with a 15-second timed wait that terminates a stuck child and adds Unix tests.

Merge readiness

⛔ Blocked before merge - 4 items remain

This PR fixes a real unbounded wait, but its new timeout cleanup duplicates and weakens the supervisor’s established process-group shutdown invariant. It also still lacks real proof through the actual once-only OCM command path.

Priority: P2
Reviewed head: c85f0e65004d99f6d74d9ec0fd60d39e6047b0fd

Review scores

Measure	Result	What it means
Overall readiness	🦐 gold shrimp (3/6)	The patch is focused and addresses a real defect, but complete cleanup and production-path proof remain merge blockers.
Proof confidence	🦐 gold shrimp (3/6)	Needs stronger real behavior proof before merge: The changed production owner is the once-only supervisor run, but the submitted terminal evidence runs standalone Rust wait examples and a helper test rather than ocm __daemon run --once through persisted child state; it does not show after-fix production timeout and cleanup. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality	🦐 gold shrimp (3/6)	1 actionable review finding remain.

Verification

Check	Result	Evidence
Real behavior	Needs proof	Needs stronger real behavior proof before merge: The changed production owner is the once-only supervisor run, but the submitted terminal evidence runs standalone Rust wait examples and a helper test rather than ocm __daemon run --once through persisted child state; it does not show after-fix production timeout and cleanup. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed	5 items	Introduced timeout path: The PR routes each once-only supervised child through a 15-second timed wait after the production child spawn. Existing process-group invariant: The established long-running supervisor shutdown waits for process-group disappearance after TERM and, if needed, KILL, before reaping the leader. Incomplete introduced cleanup: The new helper waits only for the direct child before SIGKILL, then immediately reaps that child without checking whether the process group has disappeared.
Findings	1 actionable finding	[P2] Wait for process-group disappearance before returning
Security	None	None.

How this fits together

OCM’s supervisor reads persisted environment-child specifications and starts each configured OpenClaw process. In once-only mode it waits for those children, then returns a run summary or an error to the service command.

flowchart LR
  A[Persisted supervisor state] --> B[Once-only service run]
  B --> C[Start environment child]
  C --> D[Timed child wait]
  D -->|child exits| E[Run summary]
  D -->|deadline expires| F[Terminate child group]
  F --> G[Service error]

Loading

Before merge

• Add real behavior proof - Needs stronger real behavior proof before merge: The changed production owner is the once-only supervisor run, but the submitted terminal evidence runs standalone Rust wait examples and a helper test rather than ocm __daemon run --once through persisted child state; it does not show after-fix production timeout and cleanup. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
• Wait for process-group disappearance before returning (P2) - The new cleanup loop checks only whether the direct child exited, then sends group SIGKILL and immediately reaps that child. A TERM-ignoring descendant can still be present while the once-only command returns; preserve the existing group-existence wait used by supervisor shutdown. This is the remaining prior-review finding.
• Resolve merge risk (P2) - The new timeout path can return after reaping only its direct child while a TERM-ignoring descendant remains in the supervised process group, leaving a process running after the once-only command reports failure.
• Complete next step (P2) - Preserve process-group cleanup through timeout return, add production-path descendant coverage, and post a redacted actual once-only command trace.

Findings

• [P2] Wait for process-group disappearance before returning — src/supervisor/mod.rs:1100-1114

Agent review details

Security

None.

Review metrics

Metric	Value	Why it matters
Patch surface	97 added, 4 removed, 1 file changed	The PR introduces a new timeout and process-termination primitive in the supervisor lifecycle path.

Root-cause cluster

Relationship: canonical
Canonical: #137
Summary: This PR covers once-only supervisor child waiting; the related worktree timeout PR covers a separate subprocess owner and command flow.

Members:

• adjacent_distinct: fix: bound git worktree helper subprocesses #136 - It bounds git worktree helper subprocesses rather than supervisor environment children.

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge-risk options

Maintainer options:

1. Preserve complete process-group cleanup (recommended)
   Refactor the timeout path to wait for process-group disappearance after TERM/KILL and cover it through the production supervisor spawn path.
2. Accept direct-child-only cleanup
   Land the bounded wait while accepting that the current implementation does not establish full descendant cleanup before return.

Copy recommended automerge instruction

@clawsweeper automerge

Special instructions:
Reuse or extract the supervisor process-group shutdown invariant, add a production-spawn TERM-ignoring descendant regression, and prove the group is absent before timeout return.

Technical review

Best possible solution:

Reuse or extract the established process-group shutdown behavior, then add a production-spawn regression that proves a TERM-ignoring descendant is gone before the timeout error is returned.

Do we have a high-confidence way to reproduce the issue?

Yes, at source level: the PR replaces the previous unconditional child wait in the once-only run path with a deadline. The supplied terminal trace exercises a standalone waiter and helper test, not the full persisted-state OCM command path.

Is this the best way to solve the issue?

Mostly yes: bounding the once-only wait is the narrow repair, but it should preserve the repository’s existing whole-process-group cleanup invariant rather than introduce a weaker parallel implementation.

Full review comments:

• [P2] Wait for process-group disappearance before returning — src/supervisor/mod.rs:1100-1114
  The new cleanup loop checks only whether the direct child exited, then sends group SIGKILL and immediately reaps that child. A TERM-ignoring descendant can still be present while the once-only command returns; preserve the existing group-existence wait used by supervisor shutdown. This is the remaining prior-review finding.
  Confidence: 0.93

Overall correctness: patch is incorrect
Overall confidence: 0.93

AGENTS.md: not found in the target repository.

Codex review notes: model internal, reasoning high; reviewed against 4d5defce1237.

Labels

Label justifications:

• P2: A stuck once-only child can hold the service command indefinitely, and the proposed recovery needs a focused lifecycle correction before merge.
• merge-risk: 🚨 availability: Incomplete process-tree cleanup after the new timeout could leave an environment process running after the once-only supervisor exits.
• rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🦐 gold shrimp.
• status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The changed production owner is the once-only supervisor run, but the submitted terminal evidence runs standalone Rust wait examples and a helper test rather than ocm __daemon run --once through persisted child state; it does not show after-fix production timeout and cleanup. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence

Acceptance criteria:

• [P1] cargo test supervisor::tests.
• [P1] cargo test daemon_run_once_executes_planned_children -- --exact.

What I checked:

• Introduced timeout path: The PR routes each once-only supervised child through a 15-second timed wait after the production child spawn. (src/supervisor/mod.rs:809, c85f0e65004d)
• Existing process-group invariant: The established long-running supervisor shutdown waits for process-group disappearance after TERM and, if needed, KILL, before reaping the leader. (src/supervisor/mod.rs:1934, c85f0e65004d)
• Incomplete introduced cleanup: The new helper waits only for the direct child before SIGKILL, then immediately reaps that child without checking whether the process group has disappeared. (src/supervisor/mod.rs:1100, c85f0e65004d)
• Production-path test fixture exists: The repository already invokes the actual internal once-only command through persisted supervisor state, but the PR adds only a direct helper test rather than a timed-out production-path case. (tests/daemon_runtime_tests.rs:1483, c85f0e65004d)
• Feature history: Recent merged supervisor hardening work is associated with Hannes Rudolph; the process-group stopping behavior appears to date to Shakker’s commit. (src/supervisor/mod.rs:1934, 035686953c68)

Likely related people:

• Hannes Rudolph: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
• Shakker: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

• Reuse or extract the existing process-group shutdown behavior and prove a TERM-ignoring descendant is gone before return.
• Post a redacted terminal trace of the actual once-only OCM command timing out and cleaning up its configured child; redact private endpoints, IPs, tokens, and paths. Updating the PR body should trigger re-review, or ask a maintainer to comment @clawsweeper re-review if it does not.

Rating scale

Score	Internal tier	Crab rank	Meaning
6/6	S	🦀 challenger crab	Exceptional readiness
5/6	A	🦞 diamond lobster	Very strong readiness
4/6	B	🐚 platinum hermit	Good normal PR; ordinary maintainer review
3/6	C	🦐 gold shrimp	Useful, but confidence is limited
2/6	D	🦪 silver shellfish	Proof or implementation needs work
1/6	F	🧂 unranked krab	Not merge-ready
N/A	NA	🌊 off-meta tidepool	Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

• ClawSweeper keeps one durable marker-backed review comment per issue or PR.
• Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
• A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
• PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
• Maintainers can also comment @clawsweeper review to request a fresh review only.
• Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
• Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
• Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (1 earlier review cycle)

• reviewed 2026-09-02T20:16:54.317Z sha 40ce43f :: needs real behavior proof before merge. :: [P2] Wait for the entire process group before returning