fix(mxc): bound termination and output drain cleanup

[miketripblog]

Summary

This addresses NODE-2 in issue #1122. It intentionally does not close #1122 because that issue contains multiple findings.

Root cause

Cancellation could block synchronously in process-tree termination. The normal launcher-exit path could also wait indefinitely when a descendant retained inherited stdout or stderr handles. The first bounded-kill implementation could retain one dedicated worker per permanently blocked kill.

Change

• Bounds process-tree kill and redirected-output drain waits.
• Caps process-tree kill workers process-wide at eight.
• Waits for transient worker capacity within the existing cleanup timeout, then uses only the remaining timeout for the kill.
• Releases worker capacity on completion and worker-start failure.
• Preserves cancellation, result, containment, approval, and output semantics.

Regression coverage

• A blocking kill cannot pin the caller.
• Ten repeated permanently blocked kills start no more than eight process-wide workers, all callers remain bounded, and capacity recovers after release.
• A kill queued behind eight workers starts when transient capacity returns inside the same cleanup budget.
• A real descendant retaining inherited redirected handles cannot pin a completed launcher result.

Validation

Current head: bb12539e

• powershell.exe -NoProfile -ExecutionPolicy Bypass -File .\build.ps1: passed. Shared, CLI, WinNode CLI, SetupEngine, and WinUI built successfully; documentation validation passed for 46 Markdown files.
• dotnet test .\tests\OpenClaw.Shared.Tests\OpenClaw.Shared.Tests.csproj --no-restore --filter FullyQualifiedName~Mxc: 200 passed, 7 environment-gated skips, 0 failed.
• dotnet test .\tests\OpenClaw.Shared.Tests\OpenClaw.Shared.Tests.csproj --no-restore: 3,698 passed, 32 skipped, 4 failed. The four failures are unchanged ExecReusableCommandBinderTests failures and all four reproduced on pristine base fc9add75 in a detached temporary worktree.
• dotnet test .\tests\OpenClaw.Tray.Tests\OpenClaw.Tray.Tests.csproj --no-restore: 2,469 passed, 0 skipped, 0 failed.
• python .\.agents\skills\autoreview\scripts\autoreview --mode local ...: clean, with no accepted or actionable findings after the concurrency and test-determinism fixes.

The four base-reproduced Shared failures are:

• CarrierPayloadWithExplicitRelativePath_IsNotTreatedAsAmbiguous
• AcceptedSpaceGrammar_MatchesRealCmdChildArgv
• AcceptedMultiElementTail_MatchesRealCmdChildArgv
• AcceptedTabGrammar_MatchesRealCmdChildArgv

Real behavior proof

Current-head local Windows process regressions passed:

Repeated blocked-kill regression:
10 attempts
8 process-wide workers started
all attempts returned within the cleanup bound
worker capacity recovered after release

Transient saturation regression:
8 worker slots occupied
9th kill waited without starting
1 slot released
9th kill started and completed inside its cleanup budget

MXC-focused result:
Failed: 0, Passed: 200, Skipped: 7, Total: 207

The descendant-retained-handle test launches a real Windows child process that inherits stdout and stderr handles after its parent exits. The parent result returned successfully inside the bound, and the test cleaned up the descendant.

Proof limitation

Gateway MXC E2E was not run because this task explicitly prohibited Gateway MXC E2E. It is not presented as passed. No Gateway, pairing, permission, credential, or Pavilion install path was used.

Security and capability impact

No new permissions, identity, pairing, network, sandbox, or command authority.

[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 August 16, 2026, 10:23 AM ET / 14:23 UTC.

ClawSweeper review

What this changes

The PR bounds MXC process-tree termination and redirected-output cleanup, caps stuck kill workers at eight, and adds Windows regression coverage for those cases.

Regression provenance

Possible regression — probable (reviewed change; known regression link). No predecessor PR is attributed.

Merge readiness

⛔ Blocked until stronger real behavior proof is added - 5 items remain

The branch addresses a current MXC cleanup availability gap with focused native-process coverage, but it needs the repository-required Gateway-to-Windows-node MXC proof before merge.

Priority: P1
Reviewed head: bb12539e90255f49f3b384cd9360d8949fd72a27

Review scores

Measure	Result	What it means
Overall readiness	🦐 gold shrimp (3/6)	The focused implementation and native-process regressions are solid, but required production-path MXC proof is missing.
Proof confidence	🦐 gold shrimp (3/6)	Needs stronger real behavior proof before merge: The copied current-head Windows process-test output is useful focused evidence, but this MXC executor change lacks the repository-required after-fix real Gateway to Windows-node system.run proof. 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	🐚 platinum hermit (4/6)	1 actionable review finding remain.

Verification

Check	Result	Evidence
Real behavior	Needs proof	Needs stronger real behavior proof before merge: The copied current-head Windows process-test output is useful focused evidence, but this MXC executor change lacks the repository-required after-fix real Gateway to Windows-node system.run proof. 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	Current cleanup owner: DirectAppContainerExecutor creates MxcExecutor for each contained invocation and applies caller and host timeout cancellation before returning the sandbox result. Current-main behavior: Current main still invokes process-tree termination synchronously on cancellation and uses an unbounded post-exit drain, so the central availability issue is not already solved. PR implementation: The branch limits blocked-kill workers, consumes one cleanup budget across worker admission and kill, and bounds post-exit draining.
Findings	1 actionable finding	[P1] Provide the required MXC end-to-end validation
Security	None	None.

How this fits together

MXC executes approved Windows commands inside the node path. Its cleanup runs after cancellation or launcher exit and must return a command result without blocked descendants or inherited output handles exhausting invocation capacity.

flowchart LR
A[Approved system.run request] --> B[MXC launcher]
B --> C{Cancelled or launcher exited}
C --> D[Bounded kill worker]
D --> E[Bounded output drain]
E --> F[Result to gateway]
D --> G[Eight shared worker slots]
G --> D

Loading

Before merge

• Add real behavior proof - Needs stronger real behavior proof before merge: The copied current-head Windows process-test output is useful focused evidence, but this MXC executor change lacks the repository-required after-fix real Gateway to Windows-node system.run proof. 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.
• Provide the required MXC end-to-end validation (P1) - This changes src/OpenClaw.Shared/Mxc, for which repository policy requires scripts/validate-mxc-e2e.ps1. The supplied Windows unit regressions exercise direct processes, but the PR explicitly omits the real WSL Gateway to Windows-node system.run path, leaving containment and runtime behavior unverified.
• Resolve merge risk (P1) - The modified cleanup path lacks the repository-required WSL Gateway to Windows-node MXC proof.
• Resolve merge risk (P1) - The bundled structured-review helper could not inspect the complete commit because a required partial-clone object was unavailable locally.
• Complete next step (P2) - The contributor must supply real MXC runtime proof; an automated repair lane cannot produce proof from the contributor's Windows environment.

Findings

• [P1] Provide the required MXC end-to-end validation — src/OpenClaw.Shared/Mxc/MxcExecutor.cs:195-218

Agent review details

Security

None.

Review metrics

Metric	Value	Why it matters
Production and test delta	production +74/-8, tests +281	The repair adds process-wide concurrency control and four concrete regression scenarios in two files.

Merge-risk options

Maintainer options:

1. Add MXC end-to-end proof (recommended)
   Run scripts/validate-mxc-e2e.ps1 on an MXC-capable Windows host and add redacted current-head results proving the real Gateway to node path remains contained and returns correctly.
2. Pause for unavailable hardware
   Keep the PR open until an MXC-capable Windows environment can produce the required runtime proof.

Technical review

Best possible solution:

Preserve the bounded cleanup design and attach redacted current-head scripts/validate-mxc-e2e.ps1 output from an MXC-capable Windows host before landing.

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

Yes, at source level: the branch adds deterministic Windows regressions for blocking termination, worker saturation, and inherited output handles. The required gateway-mediated MXC reproduction remains unverified.

Is this the best way to solve the issue?

Yes for the code path reviewed: a process-wide bounded worker pool plus bounded drain directly addresses the synchronous kill and inherited-handle waits without changing command authority or policy.

Full review comments:

• [P1] Provide the required MXC end-to-end validation — src/OpenClaw.Shared/Mxc/MxcExecutor.cs:195-218
  This changes src/OpenClaw.Shared/Mxc, for which repository policy requires scripts/validate-mxc-e2e.ps1. The supplied Windows unit regressions exercise direct processes, but the PR explicitly omits the real WSL Gateway to Windows-node system.run path, leaving containment and runtime behavior unverified.
  Confidence: 0.98

Overall correctness: patch is incorrect
Overall confidence: 0.9

AGENTS.md: found and applied where relevant.

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

Labels

Label changes:

• add rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🐚 platinum hermit.
• remove rating: 🧂 unranked krab: Current PR rating is rating: 🦐 gold shrimp, so this older rating label is no longer current.

Label justifications:

• P1: MXC cleanup hangs can retain Windows-node command capacity during approved command execution.
• merge-risk: 🚨 availability: The modified cancellation and output-drain path determines whether real MXC invocations return under stuck descendants or termination.
• rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🐚 platinum hermit.
• status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The copied current-head Windows process-test output is useful focused evidence, but this MXC executor change lacks the repository-required after-fix real Gateway to Windows-node system.run proof. 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:

• Current cleanup owner: DirectAppContainerExecutor creates MxcExecutor for each contained invocation and applies caller and host timeout cancellation before returning the sandbox result. (src/OpenClaw.Shared/Mxc/DirectAppContainerExecutor.cs:93, fc9add75eda7)
• Current-main behavior: Current main still invokes process-tree termination synchronously on cancellation and uses an unbounded post-exit drain, so the central availability issue is not already solved. (src/OpenClaw.Shared/Mxc/MxcExecutor.cs:191, fc9add75eda7)
• PR implementation: The branch limits blocked-kill workers, consumes one cleanup budget across worker admission and kill, and bounds post-exit draining. (src/OpenClaw.Shared/Mxc/MxcExecutor.cs:195, bb12539e9025)
• Regression coverage: New tests cover blocked termination, eight-worker saturation and recovery, transient capacity return, and a real descendant retaining inherited redirected handles. (tests/OpenClaw.Shared.Tests/Mxc/MxcExecutorTests.cs:102, bb12539e9025)
• Area history: The current MXC cleanup behavior appears to date to the tasklist-timeout repair, which explicitly bounded cancellation cleanup while retaining successful output drains. (src/OpenClaw.Shared/Mxc/MxcExecutor.cs:191, 369287826f39)
• Required runtime validation: Repository policy requires scripts/validate-mxc-e2e.ps1 for changes under src/OpenClaw.Shared/Mxc; the PR body states Gateway MXC E2E was not run. (AGENTS.md:40, fc9add75eda7)

Likely related people:

• Scott Hanselman: Authored the current-main MXC timeout cleanup behavior and related Windows sandbox work. (role: recent MXC cleanup contributor; confidence: high; commits: 369287826f39, 4206611f2b2e; files: src/OpenClaw.Shared/Mxc/MxcExecutor.cs, src/OpenClaw.Shared/Mxc/DirectAppContainerExecutor.cs)

Rank-up moves

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

• Run scripts/validate-mxc-e2e.ps1 on an MXC-capable Windows host and attach redacted current-head output.
• After updating the PR body with proof, request @clawsweeper re-review if an automatic re-review does not start.

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-08-15T23:37:50.094Z sha de9e2cb :: needs real behavior proof before merge. :: [P1] Cap abandoned process-kill workers

[miketripblog]

@clawsweeper re-review

[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.