Skip to content

fix(terminal): terminate running process when task is cancelled (#245)#261

Open
proyectoauraorg wants to merge 1 commit into
Zoo-Code-Org:mainfrom
proyectoauraorg:fix/245-terminate-process-on-cancel
Open

fix(terminal): terminate running process when task is cancelled (#245)#261
proyectoauraorg wants to merge 1 commit into
Zoo-Code-Org:mainfrom
proyectoauraorg:fix/245-terminate-process-on-cancel

Conversation

@proyectoauraorg
Copy link
Copy Markdown
Contributor

@proyectoauraorg proyectoauraorg commented May 23, 2026

Summary

Fixes #245. Pressing cancel (✕) on a running command didn't terminate the underlying process — the terminal stayed busy and the window stayed blocked until a manual kill. Several users hit this (also surfaced in #general-contributors).

Root cause: when a task is torn down (cancel ✕, task switch/removal), Task.dispose()TerminalRegistry.releaseTerminalsForTask() only disassociated the terminal (taskId = undefined) without aborting a still-running command, leaving the process orphaned and the terminal stuck busy.

Fix

In releaseTerminalsForTask, abort the running process for any terminal still busy before disassociating it. The abort path already exists on both backends and is a no-op when idle:

  • ExecaTerminalProcess.abort() → SIGINT → SIGKILL on the owned subprocess
  • TerminalProcess.abort() → Ctrl+C (\x03), gated on an active stream

Wrapped in try/catch so a failing abort never blocks task teardown.

Testing

Adds 4 TerminalRegistry tests: busy terminal → process aborted + disassociated; idle terminal → not aborted; only the target task's terminals are released; abort() errors are swallowed. Full terminal suite green locally (92 passed).

Notes

This is the short-term layer of the two-layer approach discussed with @edelauna in #general-contributors: make ✕ reliably terminate the process today. The long-term direction — owning process lifecycle via the VS Code Task API + CustomExecution for deterministic cross-platform process-tree termination — is a natural follow-up and fits the portable-core/runtime direction. Happy to take a first pass at that separately so it doesn't block this bug.

Summary by CodeRabbit

  • Bug Fixes
    • Fixed an issue where cancelling or tearing down a task would leave the underlying terminal process running in a busy state. Terminal processes are now properly aborted when a task is cancelled or torn down, preventing hung processes.

Review Change Stack

…Code-Org#245)

releaseTerminalsForTask only disassociated the terminal (taskId = undefined)
without aborting a still-running command, so cancel (✕) left the process
orphaned and the terminal stuck "busy" until a manual kill. Now abort the
running process for busy terminals on release. Adds TerminalRegistry tests.
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 23, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: ce8c4de2-d986-4267-ad7c-25c28be082f3

📥 Commits

Reviewing files that changed from the base of the PR and between d63e7bd and 5e494f5.

📒 Files selected for processing (3)
  • .changeset/terminate-process-on-cancel.md
  • src/integrations/terminal/TerminalRegistry.ts
  • src/integrations/terminal/__tests__/TerminalRegistry.spec.ts

📝 Walkthrough

Walkthrough

This PR implements a fix for issue #245 where pressing cancel on a running task would not terminate the underlying terminal process. The change aborts active terminal processes during task teardown, with error handling and test coverage for the new behavior.

Changes

Terminal Process Abort on Task Release

Layer / File(s) Summary
Process abort implementation
src/integrations/terminal/TerminalRegistry.ts, .changeset/terminate-process-on-cancel.md
releaseTerminalsForTask now aborts the underlying process when a terminal is marked busy, wrapped in try-catch to log abort failures by terminal id. Changeset documents the patch-level behavioral change.
Abort behavior tests
src/integrations/terminal/__tests__/TerminalRegistry.spec.ts
Test suite verifies busy terminals are aborted and disassociated, idle terminals are not aborted, only matching task terminals are released, and abort errors are logged but do not prevent disassociation.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

A task once cancelled left processes spinning,
Now the abort stops them—no more window pinning.
Busy terminals yield to the terminal release,
Errors caught gently, bringing the user peace.
🐰✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the primary fix: terminating the running process when a task is cancelled, directly addressing the main objective from issue #245.
Description check ✅ Passed The PR description comprehensively covers the linked issue, root cause, implementation details, testing approach, and future considerations, aligning well with the template's intent.
Linked Issues check ✅ Passed The PR fully addresses issue #245's objective to terminate the spawned process when cancel (✕) is pressed by aborting running processes in releaseTerminalsForTask with appropriate error handling and comprehensive test coverage.
Out of Scope Changes check ✅ Passed All changes (changeset, TerminalRegistry logic, and test suite) are tightly scoped to issue #245's requirements for aborting running processes during task cancellation without unrelated modifications.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

src/integrations/terminal/TerminalRegistry.ts

ESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox.

src/integrations/terminal/__tests__/TerminalRegistry.spec.ts

ESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox.


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 and usage tips.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 23, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

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.

[BUG] Cancel (✕) doesn't terminate the running command — window stays blocked until manual kill

1 participant