fix(terminal): terminate running process when task is cancelled (#245)#261
Conversation
…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.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR implements a fix for issue ChangesTerminal Process Abort on Task Release
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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
src/integrations/terminal/TerminalRegistry.tsESLint 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.tsESLint 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. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Summary
Fixes #245. Pressing cancel (✕) on a running command didn't terminate the underlying process — the terminal stayed
busyand 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 stuckbusy.Fix
In
releaseTerminalsForTask, abort the running process for any terminal stillbusybefore disassociating it. The abort path already exists on both backends and is a no-op when idle:ExecaTerminalProcess.abort()→ SIGINT → SIGKILL on the owned subprocessTerminalProcess.abort()→ Ctrl+C (\x03), gated on an active streamWrapped in try/catch so a failing abort never blocks task teardown.
Testing
Adds 4
TerminalRegistrytests: 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 +
CustomExecutionfor 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