Skip to content

[codex] fix(vscode): force-kill unresponsive reviews#489

Draft
cosin2077 wants to merge 1 commit into
alibaba:mainfrom
cosin2077:fix/vscode-cancel-escalation
Draft

[codex] fix(vscode): force-kill unresponsive reviews#489
cosin2077 wants to merge 1 commit into
alibaba:mainfrom
cosin2077:fix/vscode-cancel-escalation

Conversation

@cosin2077

Copy link
Copy Markdown

Background

The VS Code extension sends SIGTERM when a user cancels an active review and is intended to escalate to SIGKILL after three seconds if the CLI process does not exit.

Root cause

ChildProcess.killed only indicates that a signal was successfully sent. It does not indicate that the child process has exited. A process that ignores SIGTERM therefore has killed === true while exitCode and signalCode remain null, preventing the existing timeout from ever sending SIGKILL.

Fix

  • Check exitCode and signalCode to determine whether the child is still running before escalating to SIGKILL.
  • Clear the escalation timer when the child emits close to avoid signaling a process that already exited.
  • Add regression tests for both an unresponsive child and a child that exits normally after SIGTERM.

Verification

  • yarn install --frozen-lockfile (Node.js 24.18.0)
  • yarn lint - 0 errors (1 pre-existing warning in IdleView.tsx)
  • yarn test --runInBand - 11 suites, 94/94 tests passed
  • yarn build - production webpack build passed

Related

@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@github-actions

Copy link
Copy Markdown
Contributor

OpenCodeReview: No comments generated. Looks good to me.

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.

2 participants