Skip to content

fix(codex): stop timed-out resume process trees on Windows#111902

Open
ooiuuii wants to merge 10 commits into
openclaw:mainfrom
ooiuuii:fix/codex-resume-timeout-process-tree
Open

fix(codex): stop timed-out resume process trees on Windows#111902
ooiuuii wants to merge 10 commits into
openclaw:mainfrom
ooiuuii:fix/codex-resume-timeout-process-tree

Conversation

@ooiuuii

@ooiuuii ooiuuii commented Jul 20, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Closes #111900

What Problem This Solves

Fixes an issue where Windows users resuming a Codex CLI session could have shell or tool descendants continue running after the resume timed out. OpenClaw previously signaled only the direct CLI child, which does not terminate its process tree on Windows.

Why This Change Was Made

Windows timeout cleanup now invokes a trusted, absolute System32\\taskkill.exe path with /F /T /PID to terminate the complete process tree. Resolution is private to the Codex extension, so this PR adds no public Plugin SDK contract. Two bounded tree-level attempts share the existing two-second hard-kill budget, and retries stop once the original child exits so a reused PID cannot be targeted. An active taskkill is allowed to finish descendant cleanup after it causes the parent to exit; if it stalls, the watchdog stops it without retrying the exited PID. Non-Windows behavior remains unchanged.

User Impact

Timed-out Codex CLI resume work no longer leaves descendant processes running on Windows. This prevents lingering resource use and commands continuing after OpenClaw has reported a timeout.

Evidence

Real Windows process-tree proof

Environment: Windows 11, Node v22.22.3, PR head ccea7cc8f22e9378fddbee1cf02eb65bda0fe609.

The proof driver starts a real hidden PowerShell parent that creates a long-lived descendant, calls the production signalCodexResumeProcessTree helper, and polls both OS PIDs after cleanup:

PLATFORM=win32
PROOF_PATH=real PowerShell parent+descendant -> production terminateCodexResumeProcess
TASKKILL_TRUSTED_PATH=true
PARENT_ALIVE_BEFORE=true
DESCENDANT_ALIVE_BEFORE=true
PARENT_ALIVE_AFTER=false
DESCENDANT_ALIVE_AFTER=false
RESULT=PASS

Focused regression tests

node scripts/run-vitest.mjs extensions/codex/src/node-cli-process-tree.test.ts extensions/codex/src/node-cli-sessions.test.ts

Test Files  2 passed (2)
Tests       22 passed (22)

Coverage includes trusted absolute System32 resolution, malicious/invalid Windows root inputs, successful termination, spawn errors, nonzero exits, the shared two-second hard-kill deadline, bounded watchdog behavior, retry suppression after the original child exits, allowing an active helper to finish descendant cleanup, no-PID fallback, and unchanged non-Windows signaling.

Additional checks

  • oxlint passed on the changed production and test files.
  • oxfmt completed cleanly on the changed files.
  • Extensions test TypeScript check passed with test/tsconfig/tsconfig.extensions.test.json.
  • git diff --check passed.
  • Plugin SDK baseline remained unchanged; the earlier public resolver export was removed.
  • Final branch autoreview: autoreview clean: no accepted/actionable findings reported; overall patch is correct (0.90). The final type-only commit was also clean (patch is correct, 0.92).

Proof limitation: this exercises the production Windows process-tree helper with real OS processes rather than waiting for a live Codex provider session to time out. The resume timeout integration and both timeout signals are covered by focused tests.

@clawsweeper clawsweeper Bot mentioned this pull request Jul 20, 2026
Open
@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. labels Jul 20, 2026
@clawsweeper

clawsweeper Bot commented Jul 20, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed July 20, 2026, 12:52 PM ET / 16:52 UTC.

Summary
The branch replaces direct-child timeout signals for Codex CLI resume on Windows with a bounded taskkill /T process-tree helper and adds regression tests.

PR surface: Source +90, Tests +142. Total +232 across 4 files.

Reproducibility: no. —not at high confidence in this review. The linked issue gives a concrete Windows path and the source-level timeout path is clear, but the PR provides no observed after-fix Windows process-tree run.

Review metrics: 1 noteworthy metric.

  • Plugin SDK exports: 1 added public resolver. A new externally importable SDK function creates a compatibility contract beyond this one Codex timeout fix.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #111900
Summary: This PR is the linked candidate implementation for the concrete Windows descendant-process timeout defect tracked by the canonical issue.

Members:

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

Merge readiness
Overall: 🦪 silver shellfish
Proof: 🦪 silver shellfish
Patch quality: 🦐 gold shrimp
Result: blocked until real behavior proof from a real setup is added.

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

Rank-up moves:

  • Provide a redacted real Windows timeout run proving the descendant process tree is gone after codex exec resume times out.
  • Get maintainer confirmation for the new plugin SDK resolver boundary.

Proof guidance:

  • [P1] Needs real behavior proof before merge: Focused mocked tests and static checks are reported, but no redacted after-fix Windows run demonstrates actual Codex descendant-process termination; add terminal output, logs, or a recording and redact private details. 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.

Risk before merge

  • [P1] Merging adds a public plugin SDK resolver without an established maintainer-visible contract for external plugins.
  • [P2] Mocked process tests do not demonstrate that actual Windows taskkill cleanup terminates a live Codex resume descendant or preserves the intended timeout behavior.

Maintainer options:

  1. Confirm the public SDK contract and prove Windows behavior (recommended)
    Obtain an owner decision on the new resolver export, then add redacted real Windows evidence that a timed-out resume removes a live descendant process tree.
  2. Avoid a new public export
    If maintainers do not want this SDK contract, replace it with an approved narrower seam and retain focused regression coverage.

Next step before merge

  • [P1] A maintainer must choose the public plugin SDK boundary, and the contributor must supply real Windows behavior proof before this external PR can merge.

Maintainer decision needed

  • Question: Should resolveWindowsSystem32Executable become a supported plugin SDK API, or should this Codex-specific need use a different approved SDK seam?
  • Rationale: The patch introduces an externally importable plugin SDK function, so maintainers need to choose the durable extension contract rather than treating it as an incidental implementation detail.
  • Likely owner: ooiuuii — They supplied the concrete Codex/Windows scenario and proposed the new SDK seam, though historic ownership evidence is limited.
  • Options:
    • Approve the SDK seam (recommended): Keep the resolver public, document its intended boundary through the SDK contract, and require live Windows proof before merge.
    • Use a narrower seam: Refactor the Codex implementation to an existing approved SDK capability or a deliberately scoped shared helper before merging.

Security
Cleared: The patch uses a trusted System32 resolver rather than PATH or the working directory and introduces no dependency, workflow, secret, or permission change.

Review findings

  • [P1] Confirm the new plugin SDK contract before exporting it — src/plugin-sdk/windows-spawn.ts:9-10
Review details

Best possible solution:

Confirm whether the trusted System32 resolver is an intended plugin SDK contract, then capture redacted after-fix Windows evidence showing a timed-out Codex resume and descendant termination.

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

No—not at high confidence in this review. The linked issue gives a concrete Windows path and the source-level timeout path is clear, but the PR provides no observed after-fix Windows process-tree run.

Is this the best way to solve the issue?

Unclear. taskkill /T is a plausible Windows-specific repair, but the public SDK boundary needs maintainer confirmation and the OS-level behavior still needs real-run proof.

Full review comments:

  • [P1] Confirm the new plugin SDK contract before exporting it — src/plugin-sdk/windows-spawn.ts:9-10
    resolveWindowsSystem32Executable adds a public openclaw/plugin-sdk/windows-spawn API for a single Codex repair without maintainer-visible contract approval. Confirm this is the intended reusable SDK seam, or use an approved narrower boundary before merging.
    Confidence: 0.84

Overall correctness: patch is incorrect
Overall confidence: 0.73

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

  • P2: This is a bounded Windows-only timeout-cleanup bug that can leave descendant work running, but it is not shown to be an emergency-wide outage.
  • merge-risk: 🚨 compatibility: The branch adds a public plugin SDK resolver whose intended long-term external contract has not yet been confirmed.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦐 gold shrimp.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: Focused mocked tests and static checks are reported, but no redacted after-fix Windows run demonstrates actual Codex descendant-process termination; add terminal output, logs, or a recording and redact private details. 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

PR surface:

Source +90, Tests +142. Total +232 across 4 files.

View PR surface stats
Area Files Added Removed Net
Source 3 92 2 +90
Tests 1 142 0 +142
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 4 234 2 +232

What I checked:

Likely related people:

  • ooiuuii: Opened the linked Windows reproduction report and authored the process-tree proposal; no earlier merged current-main ownership evidence was available in the review context. (role: reporter and proposed-fix author; confidence: low; commits: ea1b37c63072, f22614b64922; files: extensions/codex/src/node-cli-sessions.ts, extensions/codex/src/node-cli-process-tree.ts, src/plugin-sdk/windows-spawn.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.

@BradGroux BradGroux mentioned this pull request Jul 21, 2026
Draft
@ooiuuii
ooiuuii force-pushed the fix/codex-resume-timeout-process-tree branch from f22614b to a413b0c Compare July 21, 2026 12:20
@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(codex): stop timed-out resume process trees on Windows 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

extensions: codex merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. P2 Normal backlog priority with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. size: L status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Codex CLI resume timeouts can leave descendant processes running on Windows

2 participants

@ooiuuii @Patrick-Erichsen