Skip to content

Give Windows taskkill a 5s budget to reduce flaky tree cleanup - #28577

Merged
AmethystLiang merged 2 commits into
mainfrom
fix-ci-4-2
Oct 14, 2026
Merged

AmethystLiang merged 2 commits into
mainfrom
fix-ci-4-2

Conversation

@AmethystLiang

@AmethystLiang AmethystLiang commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 2 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​43 $\color{#cf222e}{\Huge{\mathbf{−}}}$​2 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​41
Prod 4 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​5 $\color{#cf222e}{\Huge{\mathbf{−}}}$​3 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​2

Problem

On Windows, Orca sometimes stops a program's leftover child processes by running taskkill. It only gave that command 2 seconds. On a busy CI runner, taskkill can take longer than that. Orca then gives up and reports the cleanup as unverified, and the related Windows test times out.

Solution

Give taskkill 5 seconds, the same budget the other Windows cleanup paths already use. The Windows batch-launcher test also gets more room on slow runners. This is a partial fix. It makes slow runners less likely to time out, but if taskkill truly hangs, Orca still gives up after 5 seconds and reports the cleanup as unverified.

ELI5

When Orca shuts down a program and its helpers on Windows, it waited too short a time for the shutdown command to finish. On a slow computer, it gave up too early and reported a failure. Now it waits a bit longer. If the command is truly stuck, it still gives up.

What Changed

  • process-tree-termination.ts: the Windows taskkill timeout goes from 2 seconds to 5 seconds (WINDOWS_TASKKILL_TIMEOUT_MS). The POSIX and other paths keep their existing timing.
  • process-tree-termination.test.ts: new tests check that a slow taskkill (3 seconds) still verifies tree termination, and that a hung taskkill is cut off at 5 seconds and reported as unverified.
  • preflight-runnable-local-cli.test.ts: the hung-launcher test's child runs for 60 seconds instead of 30, and its timeout goes from 15 to 30 seconds, so the probe, cleanup deadline, and recovery probe fit on a busy runner.

Why

The 2-second limit was shorter than the other Windows cleanup paths already allow, so it failed on slow runners even when the cleanup would have worked. Matching the 5-second budget keeps the behavior consistent. Alternatives considered: removing the timeout (a truly hung taskkill would block forever) and retrying (more complexity for the same effect). Neither was chosen.

Linked Issue

If you do not have one and are an outside contributors, your PR wiil be ignored. Refs is not sufficient. Link an actual issue

Fixes #

Visual Proof

N/A. This change touches only process-termination timing and tests. It has no UI or visible behavior change.

Testing

  • I manually tested these changes locally
  • Automated tests added/updated, or explained why not below

Added unit tests for the slow and hung taskkill cases in process-tree-termination.test.ts, and updated the Windows batch-launcher test timing. Not yet run locally; the Windows-only tests need a Windows runner.

AI Disclosure

Review

Agent skill upstream boundary

  • Not applicable, or this change copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.

Notes

Ensure no issues in: Security, Cross-platoform support (Linux, Windows, Mac), Remote SSH, Mobile, general backwards compatibility, performance

Checklist

  • This PR is small and focused
  • I explained what changed and why (ELI5, the user-facing before/after, the mechanism, and why over the alternatives)
  • Before/after screenshots or videos attached for UI changes, or N/A with reason
  • Self-reviewed for correctness, security, and performance
  • Cross-platform, SSH/remote, and path/shortcut impact considered (or N/A)
  • pnpm lint, pnpm typecheck, pnpm test, and pnpm build pass (or CI will cover; local preferred)

On busy Windows machines, `taskkill` could take longer than the 2s budget
used for other subprocesses. Tree termination then gave up before the
process tree was confirmed gone. The taskkill timeout is now 5s, matching
the other Windows tree-cleanup paths.

Tests cover a slow taskkill that still verifies termination, and a hung
taskkill that is bounded and falls back to SIGKILL. The batch-shim recovery
test gets a longer hang and a 30s timeout so it has room on slow runners.
@coderabbitai

coderabbitai Bot commented Oct 14, 2026

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: c4a4ef9a-9293-46ec-a7ec-4dc73e65ce5c
📥 Commits

Reviewing files that changed from the base of the PR and between 6cdb4b1 and accc1e0.

📒 Files selected for processing: 3
  • src/main/ipc/preflight-runnable-local-cli.test.ts
  • src/packages/process-host/src/process-tree-termination.test.ts
  • src/packages/process-host/src/process-tree-termination.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 5 remain after this review.

📝 Walkthrough

The Windows taskkill tree-cleanup timeout increases from 2 seconds to 5 seconds. New tests check behavior when taskkill completes before the timeout or remains pending after it. The Windows recovery test also increases the child timer and test timeout.

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to accc1

The five-second cleanup budget and accompanying test changes have no identified merge-blocking issue. Run the Windows tests as a normal validation step.

Pre-merge checks: Passed 3 · Failed 2

Failed Failed checks: 2 warnings

Check name Status Explanation Resolution
Description check Warning The description clearly explains the problem, solution, scope, alternatives, tests, and lack of UI impact. However, the required Linked Issue field is empty, and the testing and self-review checklist … Add the actual linked issue after "Fixes #". Complete or explain the testing, self-review, cross-platform review, and validation checklist items, including whether CI will run the required commands.
Docstring Coverage Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 3 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 3 passed
Check name Status Explanation
Title check Passed The title clearly and concisely describes the primary change: increasing the Windows taskkill timeout to 5 seconds to reduce flaky cleanup.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Description check

Explanation

The description clearly explains the problem, solution, scope, alternatives, tests, and lack of UI impact. However, the required Linked Issue field is empty, and the testing and self-review checklist items remain incomplete.

  • Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 1
📝 Generate docstrings: 💡
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

- Expose the injected `now` clock to the structured agent session option writer, so timestamps come from the same source as the rest of the context.
- Update the main and renderer layout-writer baselines to match the reduced line counts in `src/main/ipc/session.ts` and the removed `set-active-folder-workspace.ts` entry.
@AmethystLiang
AmethystLiang enabled auto-merge (squash) October 14, 2026 05:37
@AmethystLiang
AmethystLiang merged commit ea7942e into main Oct 14, 2026
39 checks passed
nwparker added a commit that referenced this pull request Oct 14, 2026
#28571 and #28577 both added the clock; oxlint fails on the duplicate key.

Co-Authored-By: Claude <noreply@anthropic.com>
nwparker added a commit that referenced this pull request Oct 14, 2026
…28599)

#28571 and #28577 both added the clock; oxlint fails on the duplicate key.

Co-authored-by: Claude <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant