fix: bound git worktree helper subprocesses

[SebTardif]

What Problem This Solves

Fixes an issue where ocm dev, setup, and upgrade would hang forever when a git worktree helper (worktree add / remove / status / ls-files / submodule foreach) blocked on a lock or a stuck child. Those helpers called Command::output() with no kill deadline.

The first timeout patch waited only on the direct child, then joined stdout/stderr readers with no deadline. A descendant that still held those inherited pipes could block the caller after the child had already exited. The drain path then called terminate on that already-exited child; try_wait() returned Some and skipped process-group KILL, so a TERM-resistant descendant stayed alive.

On origin/main the hang is here:

ocm/src/openclaw_repo.rs

Lines 123 to 129 in e04c101

	let output = Command::new("git")
	.arg("-C")
	.arg(&repo_root)
	.args(["worktree", "add", "--detach"])
	.arg(&worktree_root)
	.output()
	.map_err(|error| format!("failed to run git worktree add: {error}"))?;

e04c101

Why This Change Was Made

Production worktree helpers now go through a shared timed runner. The child is polled, then terminated (process group TERM, then KILL) when the deadline expires. The same deadline stays active while the captured pipes drain. Cleanup waits for the process group, not only the direct child, so a descendant that ignores TERM still gets KILL. Fixture git used only to build temp repos stays unbounded. The 15s default is unchanged.

User Impact

A wedged git during worktree setup or cleanup fails after 15s instead of blocking the CLI. A helper whose direct child already exited no longer hangs on leftover pipe holders, and TERM-resistant leftovers are killed with the group. Successful worktree add/remove/status behavior is unchanged.

Evidence

Live analog of the remaining descendant leak. The grandchild ignores SIGTERM and holds a pipe. TERM alone leaves it alive. KILL removes it:

$ python3 /tmp/proof-ocm136-p2-live.py
descendant_pid=37239
alive_after_term_only=True
gone_after_kill=True
DONE analog

The patched command_output runs that same shape (parent exits, TERM-resistant descendant holds stdout) with an 800ms deadline. It returns in 1.50s and the descendant is gone. A real ensure_openclaw_worktree add/reuse/remove on a temp OpenClaw fixture returns in 1.02s through the same git_output path. The 15s production default is unchanged.

$ cargo test --offline --lib git_timeout_kills_term_resistant -- --nocapture
test openclaw_repo::tests::git_timeout_kills_term_resistant_descendant ... ok
finished in 1.50s

$ cargo test --offline --lib owned_worktree_uses_canonical -- --nocapture
test openclaw_repo::tests::owned_worktree_uses_canonical_repository_paths ... ok
finished in 1.02s

Earlier live analog of the unbounded drain hang, plus rustc /bin/sleep 30 vs the 200ms deadline, remains in the prior evidence.

Real behavior proof

• Behavior or issue addressed: Git worktree helpers no longer block ocm forever. After the direct child exits, inherited stdout/stderr drains stay under the same deadline. A TERM-resistant descendant that still holds those pipes is killed with the process group.
• Real environment tested: macOS (Darwin 25.6.0 arm64), rustc 1.98.0, perl 5, ocm checkout /private/tmp/ocm136-p1 on fix/git-worktree-timeout.
• Exact steps or command run after this patch: Ran python3 /tmp/proof-ocm136-p2-live.py to show TERM-only leaves the descendant alive. Then ran the patched command_output against perl that forks, ignores SIGTERM, writes its pid, and sleeps 30s. Then ran production ensure_openclaw_worktree add/reuse/remove on a temp OpenClaw git repo.
• Evidence after fix: terminal output above. alive_after_term_only=True, gone_after_kill=True. The patched runner returned in 1.50s with the descendant gone. ensure_openclaw_worktree completed add/reuse/remove in 1.02s. GIT_COMMAND_TIMEOUT is still 15 seconds.
• Observed result after fix: The helper returns while a 30s descendant would still be holding stdout. Group KILL removes a SIG_IGN leftover. A normal worktree add through git_output still succeeds. The 15s default is unchanged.
• What was not tested: A live index.lock hang inside a full OpenClaw checkout during ocm setup, the Windows job-object kill path, and lengthening the 15s default for valid slow worktree operations. That last item is an owner decision.

Summary

Shared timed runner for git worktree helpers. Deadline covers the direct child wait and the inherited pipe drain. Process-group cleanup escalates to KILL even when the direct child has already exited. 15s default unchanged.

[clawsweeper]

🦞👀
ClawSweeper picked this up.

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

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

[clawsweeper]

Codex review: needs real behavior proof before merge. Reviewed September 18, 2026, 1:38 PM ET / 17:38 UTC (Revision 9).

ClawSweeper review

What this changes

Adds deadlines and process-group cleanup to Git worktree helpers, bounds output draining, and shares child-waiting logic with restart handoffs.

Regression provenance

Possible regression — suspected (reviewed change). No predecessor PR is attributed.

Merge readiness

⛔ Blocked before merge - 6 items remain

The work remains necessary on main. The earlier pipe-drain and surviving-descendant findings are resolved, but the previously reported slow-operation compatibility defect remains.

Priority: P2
Reviewed head: 9949841619581cc73d6576c5eea548a639765c4e
Owner decision: Required. See Decision needed.

Review scores

Measure	Result	What it means
Overall readiness	🦐 gold shrimp (3/6)	The cleanup repairs and updated evidence improve confidence, but the remaining compatibility defect and recovery-proof gap prevent merge readiness.
Proof confidence	🦐 gold shrimp (3/6)	Needs stronger real behavior proof before merge: The captured macOS evidence supports production command_output cleanup of a TERM-resistant descendant and real Git add/reuse/remove success. It does not show valid slow operations surviving the new policy or recovery after a timed-out Git mutation, so upgrade-safety proof remains incomplete. 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 captured macOS evidence supports production command_output cleanup of a TERM-resistant descendant and real Git add/reuse/remove success. It does not show valid slow operations surviving the new policy or recovery after a timed-out Git mutation, so upgrade-safety proof remains incomplete. 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	7 items	Applicable repository policy: Read the complete root AGENTS.md and contribution guidance; no nested AGENTS.md or maintainer-notes directory was found. Review remained read-only, and no builds or tests were executed. Current main still uses unbounded Git output: Current main still calls Command::output() for worktree add, remove, status, ignored-file inspection, and registration queries. The requested bounding behavior is not implemented there. Latest release retains the previous behavior: The supplied v0.2.47 release commit still runs worktree add through unbounded output capture. GitHub confirms v0.2.47 was published on September 16.
Findings	1 actionable finding	[P1] Preserve valid worktree operations that exceed 15 seconds
Security	None	None.

How this fits together

OCM uses Git helpers to create, validate, and remove source worktrees for managed environments and upgrade simulations. Their results determine whether environment preparation or cleanup can proceed.

flowchart TD
  A[Environment preparation or cleanup] --> B[Git worktree helper]
  B --> C[Timed subprocess runner]
  C --> D{Child and output finished?}
  D -->|Yes| E[Return Git result]
  D -->|Deadline exceeded| F[Terminate process group]
  F --> G[Report timeout or drain result]

Loading

Decision needed

Question	Recommendation
Should Git worktree mutations preserve compatibility with valid operations exceeding 15 seconds, or intentionally enforce the new hard cutoff?	Preserve slow-operation compatibility: Require a compatible default and reserve strict cancellation for an explicit policy, with slow-success and interrupted-mutation recovery evidence.

Why: The existing contract has no wall-clock cutoff, and the contributor explicitly defers this behavior choice to the owner.

Before merge

• Add real behavior proof - Needs stronger real behavior proof before merge: The captured macOS evidence supports production command_output cleanup of a TERM-resistant descendant and real Git add/reuse/remove success. It does not show valid slow operations surviving the new policy or recovery after a timed-out Git mutation, so upgrade-safety proof remains incomplete. 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.
• Preserve valid worktree operations that exceed 15 seconds (P1) - This previously reported blocker remains: every migrated Git call now receives the same hard deadline, including worktree add, removal, and recursive cleanup. A healthy checkout, filter, or large dependency-tree deletion taking longer than 15 seconds is terminated and reported as failed, although main permits it to finish. The limit is unchanged between PR revisions but is newly introduced relative to main. Preserve valid slow operations through an approved compatibility policy and demonstrate recovery when a mutation is interrupted.
• Resolve merge risk (P1) - Existing users with valid Git operations lasting more than 15 seconds would receive failures after upgrading.
• Resolve merge risk (P1) - Safe retry after terminating worktree creation or removal mid-mutation remains unproven; partial filesystem or Git registration state may require operator recovery.
• Complete next step (P2) - Obtain the timeout-policy decision, address the remaining compatibility finding, and provide production-path slow-success and interruption-recovery evidence. Redacted terminal logs or copied output are sufficient; screenshots or recordings are welcome when useful. Remove private paths, endpoints, and credentials. Update the PR body to trigger re-review, or ask a maintainer to comment @clawsweeper re-review.
• Resolve maintainer decision - Resolve the maintainer decision shown above before merge.

Findings

• [P1] Preserve valid worktree operations that exceed 15 seconds — src/openclaw_repo.rs:70-78

Agent review details

Security

None.

Review metrics

Metric	Value	Why it matters
Production and test growth	Production +120 net lines; tests +91 net lines	Production growth implements the shared deadline and cleanup runner; three new tests cover timeout and descendant behavior.

Merge-risk options

Maintainer options:

1. Preserve successful slow Git operations (recommended)
   Resolve the timeout contract before changing the default, then demonstrate slow success and safe recovery after cancellation.
2. Explicitly accept the new limit
   Approve the 15-second cutoff as an intentional compatibility change only with documented operator recovery and supporting runtime evidence.

Technical review

Best possible solution:

Keep bounded pipe draining and descendant cleanup, preserve valid slow operations by default, and make any strict cutoff explicit with demonstrated interruption recovery.

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

Yes, source establishes that main waits indefinitely for a stuck Git child or inherited output pipe. The remaining patch defect is also deterministic for a valid operation exceeding 15 seconds; neither scenario was executed during this read-only review.

Is this the best way to solve the issue?

Partly: the shared runner repairs the reported waiting and cleanup paths, but a universal 15-second limit does not distinguish stalled Git from valid slow work. A compatibility-preserving timeout policy is safer.

Full review comments:

• [P1] Preserve valid worktree operations that exceed 15 seconds — src/openclaw_repo.rs:70-78
  This previously reported blocker remains: every migrated Git call now receives the same hard deadline, including worktree add, removal, and recursive cleanup. A healthy checkout, filter, or large dependency-tree deletion taking longer than 15 seconds is terminated and reported as failed, although main permits it to finish. The limit is unchanged between PR revisions but is newly introduced relative to main. Preserve valid slow operations through an approved compatibility policy and demonstrate recovery when a mutation is interrupted.
  Confidence: 0.98

Overall correctness: patch is incorrect
Overall confidence: 0.96

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against 6ff9fb59f105.

Labels

Label changes:

• remove merge-risk: 🚨 availability: Current PR review merge-risk labels are merge-risk: 🚨 compatibility.

Label justifications:

• P2: This is a bounded subprocess-reliability improvement with an unresolved compatibility choice, without evidence of an urgent widespread outage.
• merge-risk: 🚨 compatibility: The introduced 15-second deadline can terminate valid worktree operations that currently complete successfully.
• 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 captured macOS evidence supports production command_output cleanup of a TERM-resistant descendant and real Git add/reuse/remove success. It does not show valid slow operations surviving the new policy or recovery after a timed-out Git mutation, so upgrade-safety proof remains incomplete. 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

What I checked:

• Applicable repository policy: Read the complete root AGENTS.md and contribution guidance; no nested AGENTS.md or maintainer-notes directory was found. Review remained read-only, and no builds or tests were executed. (AGENTS.md:1, 994984161958)
• Current main still uses unbounded Git output: Current main still calls Command::output() for worktree add, remove, status, ignored-file inspection, and registration queries. The requested bounding behavior is not implemented there. (src/openclaw_repo.rs:486, 6ff9fb59f105)
• Latest release retains the previous behavior: The supplied v0.2.47 release commit still runs worktree add through unbounded output capture. GitHub confirms v0.2.47 was published on September 16. (src/openclaw_repo.rs:486, b7e2802d9ae0)
• Introduced compatibility change: The pinned introduction delta adds a fixed 15-second timeout to every migrated Git call, including worktree creation and removal. Unlike main, it terminates healthy operations solely because their elapsed time exceeds that limit; no override or progress-sensitive policy exists. (src/openclaw_repo.rs:70, 994984161958)
• Previous cleanup findings resolved: Current output receivers retain the deadline, and cleanup now checks the process group before returning and escalates to KILL. The previous head's source, retrieved through GitHub after local historical blob inspection failed, confirms the earlier direct-child-only early return. (src/infra/process.rs:147, 994984161958)
• Updated proof and author disposition: The captured PR body reports macOS production-helper execution against a TERM-resistant descendant, returning in 1.50 seconds with the descendant gone, plus real Git add/reuse/remove in 1.02 seconds. This supports cleanup and ordinary success, but supplies no slow-success or interrupted-mutation recovery result. The author explicitly leaves the timeout policy unresolved in fix: bound git worktree helper subprocesses #136 (comment). Captured context sourceRevision: 24b7af1754da0e06adbe527bac1bc58c47ecb7a74262a6daea2f09456a3e576b. (994984161958)

Likely related people:

• unknown: The claimed source-line change could not be verified from bounded local history. (role: source history unknown; confidence: low)
• TheAngryPit: 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.

• Resolve the timeout policy and preserve valid slow worktree operations.
• Add redacted terminal output from the production worktree path showing slow success and recovery after an interrupted mutation.

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 (8 earlier review cycles)

• reviewed 2026-09-02T20:06:36.861Z sha b080857 :: needs real behavior proof before merge. :: [P1] Keep the deadline active while draining inherited output pipes | [P1] Avoid rejecting valid slow worktrees at a fixed 15 seconds
• reviewed 2026-09-09T20:48:12.274Z sha 0284e88 :: needs real behavior proof before merge. :: [P1] Keep the deadline active while draining inherited output pipes | [P1] Preserve valid worktree operations that exceed 15 seconds
• reviewed 2026-09-10T14:19:46.628Z sha 11e9595 :: needs real behavior proof before merge. :: [P1] Keep the deadline active while draining inherited output pipes | [P1] Preserve valid worktree operations that exceed 15 seconds
• reviewed 2026-09-10T14:32:54.500Z sha 638df70 :: needs real behavior proof before merge. :: [P1] Keep the deadline active while draining inherited output pipes | [P1] Preserve valid worktree operations that exceed 15 seconds
• reviewed 2026-09-10T23:24:35.376Z sha db422a7 :: needs real behavior proof before merge. :: [P1] Keep the deadline active while draining inherited output pipes | [P1] Preserve valid worktree operations that exceed 15 seconds
• reviewed 2026-09-11T16:31:28.227Z sha 6a6dfd1 :: needs real behavior proof before merge. :: [P1] Keep the deadline active while draining inherited output pipes | [P1] Preserve valid worktree operations that exceed 15 seconds
• reviewed 2026-09-18T16:57:26.981Z sha 2bda641 :: needs real behavior proof before merge. :: [P1] Preserve valid worktree operations that exceed 15 seconds | [P2] Escalate cleanup when descendants survive the direct child
• reviewed 2026-09-18T17:07:04.634Z sha 76a2c85 :: needs real behavior proof before merge. :: [P1] Preserve valid worktree operations that exceed 15 seconds | [P2] Escalate cleanup when descendants survive the direct child

[SebTardif]

@clawsweeper re-review

P1 keep-deadline-while-draining-pipes is now in the tip.

command_output no longer unbounded-joins stdout/stderr after the direct child exits. The original deadline stays active across the drain. If a descendant still holds those pipes, the process group is terminated so the readers can finish.

Live analog: after the middle process exits, a raw select on the inherited stdout pipe is still unread at 505ms (raw_pipe_ready_in_500ms=False). The patched runner returns in 0.41s on the same shape.

The 15s default is unchanged. The other P1 (preserve valid worktree operations that exceed 15 seconds) is an owner default decision and is not in this commit.

[clawsweeper]

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event exact_review_queue).
Result: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.

[SebTardif]

@clawsweeper re-review

P2 escalate-cleanup-when-descendants-survive is in the tip.

terminate_child no longer returns when the direct child has already exited. It waits for live process-group members, then sends KILL. A SIGTERM-ignoring descendant that still holds stdout is gone after the runner returns (1.50s on an 800ms deadline). A real ensure_openclaw_worktree add/reuse/remove still finishes in 1.02s.

Live analog: alive_after_term_only=True, gone_after_kill=True.

The 15s default is unchanged.

Owner decision still needed: should valid worktree operations that take longer than 15 seconds keep running (progress-based, override, or a longer default), or is a hard 15s cutoff the intended policy after upgrade? This PR does not change that default.

[clawsweeper]

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event exact_review_queue).
Result: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.

[steipete]

Thanks for working through the pipe-drain and descendant-cleanup cases. I’m closing this version because the new unconditional 15-second deadline also interrupts valid worktree creation, filters, and removal. Main has no such cutoff, so this changes successful operations into failures and can leave an interrupted Git mutation to recover.

A compatible cancellation policy needs a separate design decision; this maintenance pass will preserve existing slow-operation behavior rather than land the hard cutoff.