Skip to content

fix(process): bound Windows exec timeout cleanup#104234

Merged
steipete merged 1 commit into
openclaw:mainfrom
mikasa0818:fix/windows-exec-timeout-cleanup
Jul 21, 2026
Merged

fix(process): bound Windows exec timeout cleanup#104234
steipete merged 1 commit into
openclaw:mainfrom
mikasa0818:fix/windows-exec-timeout-cleanup

Conversation

@mikasa0818

@mikasa0818 mikasa0818 commented Jul 11, 2026 •
edited by steipete
Loading

Copy link
Copy Markdown
Contributor

What Problem This Solves

Foreground exec deadlines on Windows could take roughly 10–12 seconds to return because the supervisor attempted a graceful phase even though Windows has no catchable SIGTERM equivalent. Earlier repairs also risked truncating output from ordinary descendants that retained inherited stdout/stderr.

Why This Change Was Made

  • Overall and no-output deadlines request a tree-aware hard kill immediately on Windows; Unix and manual-cancel grace behavior remain unchanged.
  • The process-tree owner reports taskkill /F /T completion, bounded at three seconds, and the child adapter does not settle a forced Windows kill before that completion signal.
  • Forced stream settlement is restricted to explicit Windows hard kills. Ordinary exit retains lossless descendant-output draining.
  • Overlapping no-output and overall deadlines are coalesced so only one hard kill and one taskkill traversal can run while cleanup is pending.
  • The Windows end → exit → no close event ordering rechecks already-drained streams after recording exit, preventing an ordinary completed run from waiting forever.

The three-second taskkill bound is an intentional availability policy: when Windows itself stops reporting completion, foreground exec returns after bounded best-effort cleanup rather than waiting indefinitely.

User Impact

Windows timeout results return promptly, timed-out process trees are normally gone before return, and ordinary descendant output is not truncated. No API, config, schema, dependency, PTY, Unix cancellation, or manual-cancel contract changes.

Evidence

  • Contributor live Windows proof: current main took 11,642 ms; the repaired production path returned around 2.5–2.6 seconds, retained late ordinary stdout/stderr, and verified the timed-out descendant PID was gone at return.
  • node scripts/run-vitest.mjs src/process/kill-tree.test.ts src/process/supervisor/adapters/child.test.ts src/process/supervisor/supervisor.test.ts — 55 tests passed on exact rebased head dec42ee210e.
  • node scripts/check-changed.mjs -- <six touched files> — core/core-test typecheck, lint, format, import cycles, dependency/API/database guards passed on Node 24.18. Blacksmith status timed out before repository proof, so the documented trusted-source local fallback ran the same plan.
  • AutoReview: two actionable lifecycle races were accepted and fixed; final integrated review clean with no accepted/actionable findings.
  • git diff --check passed.

Fixes the Windows deadline regression introduced by #85865. Original fix by @mikasa0818; replacement commit preserves Co-authored-by: mikasa0818 <[email protected]>.

@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 other 🚨 Merging this PR has meaningful risk outside the owned taxonomy. labels Jul 11, 2026
@clawsweeper

clawsweeper Bot commented Jul 11, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed July 17, 2026, 9:16 AM ET / 13:16 UTC.

Summary
On Windows, the PR sends overall and no-output deadline cancellation directly to tree-aware SIGKILL, reports bounded taskkill completion, and gates forced child settlement while preserving ordinary output draining.

PR surface: Source +76, Tests +171. Total +247 across 6 files.

Reproducibility: yes. The PR provides a current-main Windows production reproduction around 11.6 seconds and exact-head runs around 2.5–2.6 seconds that also verify descendant termination and ordinary late-output retention.

Review metrics: none identified.

Root-cause cluster
Relationship: canonical
Canonical: #104234
Summary: This PR is the canonical Windows deadline-cleanup correction; the earlier merged supervisor PR is historical overlap and the Microsoft tracker is administrative context.

Members:

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

Merge readiness
Overall: 🦞 diamond lobster
Proof: 🦀 challenger crab
Patch quality: 🦞 diamond lobster
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Risk before merge

  • [P1] If taskkill neither closes nor errors within three seconds, foreground exec settlement can proceed without confirmed descendant termination; this is an intentional availability-versus-cleanup guarantee tradeoff.

Maintainer options:

  1. Accept bounded taskkill failure (recommended)
    Merge after the process owner accepts best-effort descendant cleanup only when taskkill exceeds its three-second completion bound.
  2. Redesign for confirmed termination
    Pause the PR if the runtime must guarantee descendant termination before foreground exec settles even when taskkill hangs.

Next step before merge

  • [P2] A process owner should accept or reject the bounded best-effort taskkill behavior, then complete normal exact-head merge review; no automated code repair remains.

Maintainer decision needed

  • Question: Should Windows foreground exec settlement remain bounded when taskkill exceeds the three-second completion limit, accepting that descendant termination is not confirmed in that exceptional path?
  • Rationale: The patch is correct and strongly proven for normal and observed failure paths, but choosing bounded caller availability over guaranteed descendant cleanup when taskkill itself hangs is intentional runtime policy.
  • Likely owner: steipete — He owns the current process-tree/taskkill implementation and merged the related supervisor lifecycle change.
  • Options:
    • Accept bounded settlement (recommended): Merge the current design so foreground exec cannot wait indefinitely when Windows taskkill itself stalls.
    • Require confirmed cleanup: Pause and redesign the contract if every descendant must be confirmed terminated before foreground exec may settle.

Security
Cleared: The diff changes command-process termination timing but adds no dependency, permission, secret, network, installation, publishing, or supply-chain surface.

Review details

Best possible solution:

Land the owner-separated design after the process owner accepts bounded best-effort cleanup: supervisor-owned deadline policy, process-tree-owned completion reporting, and hard-kill-only forced settlement.

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

Yes. The PR provides a current-main Windows production reproduction around 11.6 seconds and exact-head runs around 2.5–2.6 seconds that also verify descendant termination and ordinary late-output retention.

Is this the best way to solve the issue?

Yes. This is the narrowest maintainable fix found: deadline policy stays in the supervisor, completion belongs to the process-tree owner, and forced stream settlement is limited to explicit Windows hard kills.

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

  • P2: The PR fixes a Windows-only foreground exec timeout delay with a focused implementation and limited blast radius.
  • merge-risk: 🚨 availability: The patch changes process-tree termination and wait-settlement ordering, including an intentional bound when taskkill does not complete.
  • rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦀 challenger crab and patch quality is 🦞 diamond lobster.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The PR includes direct before-and-after live Windows production output for timeout timing, descendant termination, and preservation of ordinary late stdout/stderr.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR includes direct before-and-after live Windows production output for timeout timing, descendant termination, and preservation of ordinary late stdout/stderr.
Evidence reviewed

PR surface:

Source +76, Tests +171. Total +247 across 6 files.

View PR surface stats
Area Files Added Removed Net
Source 3 122 46 +76
Tests 3 186 15 +171
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 6 308 61 +247

What I checked:

  • Windows deadline policy: The supervisor bypasses the ineffective graceful phase only for Windows overall and no-output deadlines; manual cancellation and non-Windows cancellation retain their existing escalation behavior. (src/process/supervisor/supervisor.ts:227, edaf85ece2e0)
  • Hard-kill settlement invariant: The child adapter distinguishes explicit Windows hard kills from ordinary exits, waits for tree-kill completion at each settlement entry point, and confines forced stream settlement to that hard-kill lifecycle. (src/process/supervisor/adapters/child.ts:331, edaf85ece2e0)
  • Bounded tree-kill completion: The process-tree helper observes taskkill close or error and reports completion after a three-second upper bound, while retaining fire-and-forget behavior for callers that omit the optional callback. (packages/agent-core/src/harness/env/kill-tree.ts:65, edaf85ece2e0)
  • Regression coverage: Tests cover normal and hung taskkill completion, early child close, fully drained streams, and ordinary Windows descendant-held output that must remain pending beyond the forced-settlement interval. (src/process/supervisor/adapters/child.test.ts:391, edaf85ece2e0)
  • Real Windows proof: The PR reports a production current-main reproduction taking 11,642 ms and exact-head runs around 2.5–2.6 seconds, with the timed-out descendant gone at return and late ordinary stdout/stderr retained. (edaf85ece2e0)
  • Review continuity: The latest completed ClawSweeper cycle at f4d4e62 found no remaining code defect; the later head merges main and the final diff retains the reviewed lifecycle invariants. (packages/agent-core/src/harness/env/kill-tree.ts:65, f4d4e62159e0)

Likely related people:

  • steipete: Authored the current agent-core process-tree/taskkill implementation and merged the related graceful-cancellation lifecycle change, making him the strongest available owner for the bounded-cleanup decision. (role: process-tree owner and prior merger; confidence: high; commits: bb46b79d3c14, b13166bc0cca; files: packages/agent-core/src/harness/env/kill-tree.ts, src/process/supervisor/supervisor.ts)
  • IWhatsskill: Authored the merged graceful process-supervisor cancellation lifecycle whose Windows deadline behavior this PR corrects. (role: introduced related behavior; confidence: high; commits: b13166bc0cca; files: src/process/supervisor/supervisor.ts, src/process/supervisor/adapters/child.ts)
  • obviyus: Authored earlier Windows exit-state and close-state settlement fixes directly adjacent to the child-adapter lifecycle changed here. (role: Windows wait-settlement contributor; confidence: high; commits: 4b6b1a3ed358, 063049c0d4de; files: src/process/supervisor/adapters/child.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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.

How this review workflow works
  • 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.
Review history (8 earlier review cycles)
  • reviewed 2026-07-11T07:03:32.616Z sha 35d2f11 :: found issues before merge. :: [P1] Limit stream destruction to forced timeout cleanup
  • reviewed 2026-07-11T11:09:44.798Z sha 6276790 :: needs changes before merge. :: [P1] Wait for the Windows tree kill before settling
  • reviewed 2026-07-11T13:40:30.522Z sha d84e337 :: needs changes before merge. :: [P1] Bound the taskkill wait before settling
  • reviewed 2026-07-11T13:47:30.854Z sha d84e337 :: needs changes before merge. :: [P1] Bound taskkill before allowing wait settlement
  • reviewed 2026-07-11T14:09:45.311Z sha e8cc2df :: needs maintainer review before merge. :: none
  • reviewed 2026-07-11T14:18:20.965Z sha e8cc2df :: needs changes before merge. :: [P1] Gate every hard-kill settlement on tree-kill completion
  • reviewed 2026-07-11T15:20:17.987Z sha f4d4e62 :: needs maintainer review before merge. :: none
  • reviewed 2026-07-11T15:28:42.692Z sha f4d4e62 :: needs maintainer review before merge. :: none

@mikasa0818
mikasa0818 force-pushed the fix/windows-exec-timeout-cleanup branch from 35d2f11 to 6276790 Compare July 11, 2026 10:55
@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. merge-risk: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. merge-risk: 🚨 other 🚨 Merging this PR has meaningful risk outside the owned taxonomy. labels Jul 11, 2026
@BradGroux BradGroux mentioned this pull request Jul 11, 2026
Draft
@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Jul 11, 2026
@mikasa0818

mikasa0818 commented Jul 11, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 11, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@mikasa0818

mikasa0818 commented Jul 11, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 11, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@clawsweeper clawsweeper Bot removed the proof: sufficient ClawSweeper judged the real behavior proof convincing. label Jul 11, 2026
@clawsweeper clawsweeper Bot removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. merge-risk: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Jul 11, 2026
@mikasa0818

mikasa0818 commented Jul 11, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 11, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. merge-risk: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. labels Jul 11, 2026
@MohammedAlkindi MohammedAlkindi mentioned this pull request Jul 20, 2026
Open
@steipete steipete self-assigned this Jul 21, 2026
@steipete
steipete force-pushed the fix/windows-exec-timeout-cleanup branch from edaf85e to dec42ee Compare July 21, 2026 16:17
@clawsweeper

clawsweeper Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

ClawSweeper status: review started.

I am starting a fresh review of this pull request: fix(process): bound Windows exec timeout cleanup This is item 1/1 in the current shard. Shard 0/1.

This placeholder means the worker is alive and reading the current context. I will edit this same comment with the actual review when the claws are done clicking.

Crustacean status: shell secured, claws on keyboard, evidence pebbles being sorted.

@steipete
steipete merged commit a8a6e2a into openclaw:main Jul 21, 2026
122 checks passed
@steipete

steipete commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Merged via squash.

@github-actions github-actions Bot mentioned this pull request Jul 21, 2026
Open
github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request Jul 22, 2026
@mikasa0818 @steipete
fix(process): bound Windows exec timeout cleanup (openclaw#104234)
7c4987d 
Co-authored-by: Peter Steinberger <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@steipete steipete

Labels

merge-risk: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. P2 Normal backlog priority with limited blast radius. proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. size: M status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@mikasa0818 @steipete