Skip to content

fix(adapters): terminate Windows coding-agent trees - #5224

Merged
lidge-jun merged 1 commit into
lidge-jun:devfrom
luvs01:fix/windows-agent-tree-kill
Sep 20, 2026
Merged

lidge-jun merged 1 commit into
lidge-jun:devfrom
luvs01:fix/windows-agent-tree-kill

Conversation

@luvs01

@luvs01 luvs01 commented Sep 19, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

  • On Windows, aborting or timing out a coding-agent CLI turn called child.kill("SIGTERM")/SIGKILL on the spawned cmd shim, which terminated only the shim and orphaned the real CLI process tree. On win32 the turn now terminates the whole tree via taskkill /PID <pid> /T /F, falling back to the direct SIGTERM-to-SIGKILL path if taskkill fails. The terminator is injectable through CodingAgentDeps.killWindowsProcessTree for tests.

Verification

  • bun test tests/providers/codebuddy-adapter.test.ts — 41 pass, 0 fail (includes the new "a Windows abort terminates the cmd shim process tree" regression test).
  • bun x tsc --noEmit — clean.
  • Bun 1.4.2 on Windows.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed.
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults.

Review readiness checklist

This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:

  • All CI tests are green on my local testing.

  • I pushed my PR to the latest dev commit.

  • I resolved all correct Codex and CodeRabbit findings.

  • My PR is ready for review.

Summary by CodeRabbit

  • Bug Fixes
    • Improved cancellation of coding-agent operations on Windows by terminating the full child-process tree.
    • Added fallback termination behavior when process-tree termination is unavailable or fails.
    • Ensured aborted operations report a non-retryable error consistently.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

@github-actions

Copy link
Copy Markdown
Contributor

✅ Deterministic PR hygiene checks passed.

@github-actions github-actions Bot added the bug Something isn't working label Sep 19, 2026
@github-actions

github-actions Bot commented Sep 19, 2026 •

Copy link
Copy Markdown
Contributor

✅ READY

  • all PR quality gates passed; the review readiness checklist is complete.

Review readiness checklist

  • ✅ All CI tests are green on my local testing.
  • ✅ I pushed my PR to the latest dev commit.
  • ✅ I resolved all correct Codex and CodeRabbit findings.
  • ✅ My PR is ready for review.

✅ 4/4 boxes ticked.

This pull request is already Ready for Review.
The review-ready label marks this PR as ready; review automation runs independently.
Maintainers notified: @lidge-jun @Ingwannu

@coderabbitai

coderabbitai Bot commented Sep 19, 2026 •

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: lidge-jun/opencodex/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 7e1fc569-8b8a-498f-bea9-e0e08baed0ba

📥 Commits

Reviewing files that changed from the base of the PR and between c193f06 and def2765.

📒 Files selected for processing (2)
  • src/adapters/coding-agent/turn.ts
  • tests/providers/codebuddy-adapter.test.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.


📝 Walkthrough

Walkthrough

Changes

Windows process termination

Layer / File(s) Summary
Windows process-tree termination
src/adapters/coding-agent/turn.ts
Adds KillWindowsProcessTreeFn, an injectable dependency, and a default taskkill.exe /PID /T /F implementation. runCodingAgentTurn resolves the platform once and uses process-tree termination for Windows child PIDs, with direct signal fallback.
Windows abort validation
tests/providers/codebuddy-adapter.test.ts
Adds an optional fake child PID and verifies that a Windows abort calls the process-tree hook with PID 4242 without sending direct signals.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~12 minutes

Change: Bug fix

Suggested reviewers: lidge-jun

Sequence Diagram(s)

sequenceDiagram
  participant runCodingAgentTurn
  participant killWindowsProcessTree
  participant taskkill.exe
  participant ChildProcess
  runCodingAgentTurn->>killWindowsProcessTree: terminate child PID
  killWindowsProcessTree->>taskkill.exe: invoke /PID /T /F
  taskkill.exe->>ChildProcess: terminate process tree
  runCodingAgentTurn->>ChildProcess: fallback SIGTERM then SIGKILL
Loading
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
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.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: terminating Windows coding-agent process trees during abort or timeout handling.
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

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.

@github-actions
github-actions Bot marked this pull request as draft September 19, 2026 22:48
@luvs01
luvs01 marked this pull request as ready for review September 19, 2026 22:52
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

@lidge-jun

Copy link
Copy Markdown
Owner

리뷰 · 우선순위 52 / 80

윈도우에서 코딩 도우미를 중간에 멈추면, 껍데기만 죽고 안에 있던 프로그램은 남습니다. 이 PR은 그 구멍을 막습니다.

윈도우는 codebuddy.cmd 같은 파일을 그대로 못 띄웁니다. 그래서 cmd.exe가 껍데기가 되고, 그 안에서 진짜 프로그램이 돕니다. 예전 코드는 그 cmd만 죽였습니다. 자식은 부모를 잃고 계속 돌았습니다. 멈추기든, 시간 초과든 같은 길이었습니다.

이제는 윈도우이고 프로세스 번호가 있으면 taskkill /T /F로 그 번호와 그 아래를 같이 죽입니다. taskkill이 실패하면 예전처럼 직접 죽이고, 잠시 뒤 한 번 더 죽입니다. 테스트는 진짜 taskkill 대신 넣어 준 함수가 4242번을 받았는지, 직접 죽이기는 안 했는지, 나온 에러가 다시 시도하면 안 되는 종류인지를 봅니다.

베이스는 dev입니다. 같은 고침을 하는 다른 열린 PR은 없습니다. 이 글을 쓸 때 GitHub 검사는 위생, 라벨, 타깃뿐입니다. bun 테스트 결과는 그 목록에 없습니다. 작성자는 윈도우에서 41개 통과라고 적었습니다. 그 실행은 여기서 확인하지 못했습니다.

라인 - src/adapters/coding-agent/turn.ts:37 - execFileSync에 시간 제한이 없습니다. 이 호출은 끝날 때까지 다른 일을 멈춥니다. taskkill이 안 돌아오면 멈춤 처리도 거기서 끝납니다. 같은 프로세스의 다른 요청도 같이 멈춥니다. 시간 제한이 있으면 에러가 나고, 바로 아래 catch가 예전 죽이기 길로 갑니다.

라인 - tests/providers/codebuddy-adapter.test.ts:618 - 새 테스트는 잘 죽었을 때만 봅니다. 죽이는 함수가 에러를 던지면 직접 SIGTERM을 보내야 합니다. 그 경우는 테스트가 없습니다. 실패를 받아 주는 catch를 지워도 이 테스트는 통과합니다.

메인테이너의 판단이 필요한 지점

윈도우는 기다리지 않고 바로 강제로 죽입니다. 리눅스는 2초 기다렸다가 더 세게 죽입니다. cmd 껍데기는 부드러운 종료를 안에 있는 프로그램에 넘기지 못합니다. 그래서 바로 /F가 이 버그에는 맞습니다. 윈도우만 이렇게 둘지 정해 주세요.

src/lib/process-control.ts의 killProxy도 시간 제한 없는 taskkill을 씁니다. 그 함수는 서버를 끌 때 한 번 돕니다. 이 함수는 사용자가 멈추거나 시간이 다 됐을 때마다, 요청을 처리하는 도중에 돕니다. 여기만 시간 제한을 넣을지 정해 주세요.

너의 추천

execFileSync에 몇 초 제한을 넣으세요. 시간이 지나면 에러가 나고, 이미 있는 catch가 직접 죽이는 길로 갑니다. 죽이는 함수가 에러를 던지는 테스트도 하나 넣으세요. 고치는 방향은 맞습니다. 그 둘만 반영하면 머지해도 됩니다.

이 댓글은 grok-bot이 작성했습니다

@lidge-jun
lidge-jun merged commit 2a2c01a into lidge-jun:dev Sep 20, 2026
14 of 15 checks passed
@luvs01
luvs01 deleted the fix/windows-agent-tree-kill branch September 20, 2026 04:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working review-ready

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants