fix(win): tree-kill local PTYs so ports free on terminal stop - #10183
Closed
innocarpe wants to merge 0 commit into
Closed
fix(win): tree-kill local PTYs so ports free on terminal stop#10183innocarpe wants to merge 0 commit into
innocarpe wants to merge 0 commit into
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a Windows-specific 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/ports/workspace-port-ownership.test.ts (1)
45-85: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPlatform override isn't restored if an assertion fails mid-test.
Both tests restore
process.platformonly after the assertions; if anyexpectthrows first, the override leaks into later tests sincevi.restoreAllMocks()doesn't coverObject.definePropertymutations.♻️ Proposed fix: guarantee restoration via try/finally
it('on Windows tree-kills the owning process so npm wrappers free the port', async () => { const platformDescriptor = Object.getOwnPropertyDescriptor(process, 'platform') Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) - scanWorkspacePortsMock.mockResolvedValue({...}) - terminateWindowsProcessTreeMock.mockResolvedValue(undefined) - - await expect(...).resolves.toEqual({ ok: true }) - expect(terminateWindowsProcessTreeMock).toHaveBeenCalledWith(4242) - - if (platformDescriptor) { - Object.defineProperty(process, 'platform', platformDescriptor) - } + try { + scanWorkspacePortsMock.mockResolvedValue({...}) + terminateWindowsProcessTreeMock.mockResolvedValue(undefined) + await expect(...).resolves.toEqual({ ok: true }) + expect(terminateWindowsProcessTreeMock).toHaveBeenCalledWith(4242) + } finally { + if (platformDescriptor) { + Object.defineProperty(process, 'platform', platformDescriptor) + } + } })Apply the equivalent wrapping to the POSIX test (lines 65-85).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7d86942d-4b4c-43cf-984e-455117712dd0
📒 Files selected for processing (8)
src/main/ports/workspace-port-ownership.test.tssrc/main/ports/workspace-port-ownership.tssrc/main/providers/local-pty-provider.test.tssrc/main/providers/local-pty-provider.tssrc/main/pty-descendant-termination.test.tssrc/main/pty-descendant-termination.tssrc/main/windows-process-tree-kill.test.tssrc/main/windows-process-tree-kill.ts
innocarpe
added a commit
to innocarpe/orca
that referenced
this pull request
Jul 23, 2026
terminateWindowsProcessTree is best-effort and always resolves so PTY teardown is never blocked. Ports UI needs a real outcome: after taskkill, probe liveness with process.kill(pid, 0) and return failure if the PID is still alive. Addresses CodeRabbit review on stablyai#10183.
Contributor
Author
Sync update (
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2b5b6169-2d45-40f6-a18a-4687ccd7815f
📒 Files selected for processing (2)
src/main/ports/workspace-port-ownership.test.tssrc/main/ports/workspace-port-ownership.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/ports/workspace-port-ownership.test.ts
innocarpe
added a commit
to innocarpe/orca
that referenced
this pull request
Jul 23, 2026
EPERM from process.kill(pid, 0) means the process is still alive without signal rights; do not report Ports kill success in that case. Addresses CodeRabbit follow-up on stablyai#10183.
Contributor
Author
Sync update (
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e3350071-89bd-4229-9368-e4eaa5831074
📒 Files selected for processing (2)
src/main/ports/workspace-port-ownership.test.tssrc/main/ports/workspace-port-ownership.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/ports/workspace-port-ownership.ts
Comment on lines
+92
to
+114
| it('on Windows reports failure for EPERM liveness probes (still alive)', async () => { | ||
| const platformDescriptor = Object.getOwnPropertyDescriptor(process, 'platform') | ||
| Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) | ||
| scanWorkspacePortsMock.mockResolvedValue({ | ||
| platform: 'win32', | ||
| scannedAt: Date.now(), | ||
| ports: [workspacePort(4242, 5173)] | ||
| }) | ||
| terminateWindowsProcessTreeMock.mockResolvedValue(undefined) | ||
| vi.spyOn(process, 'kill').mockImplementation(() => { | ||
| const err = new Error('kill EPERM') as Error & { code?: string } | ||
| err.code = 'EPERM' | ||
| throw err | ||
| }) | ||
|
|
||
| await expect( | ||
| killWorkspacePort(worktrees, { pid: 4242, port: 5173, repoId: 'repo' }) | ||
| ).resolves.toEqual({ ok: false, reason: 'kill EPERM' }) | ||
|
|
||
| if (platformDescriptor) { | ||
| Object.defineProperty(process, 'platform', platformDescriptor) | ||
| } | ||
| }) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Restore process.platform in a finally block.
If the expectation fails, execution never reaches lines 111-113, leaving the global platform set to win32 and contaminating later tests.
Proposed fix
const platformDescriptor = Object.getOwnPropertyDescriptor(process, 'platform')
-Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' })
-scanWorkspacePortsMock.mockResolvedValue({
- platform: 'win32',
- scannedAt: Date.now(),
- ports: [workspacePort(4242, 5173)]
-})
-terminateWindowsProcessTreeMock.mockResolvedValue(undefined)
-vi.spyOn(process, 'kill').mockImplementation(() => {
- const err = new Error('kill EPERM') as Error & { code?: string }
- err.code = 'EPERM'
- throw err
-})
-
-await expect(
- killWorkspacePort(worktrees, { pid: 4242, port: 5173, repoId: 'repo' })
-).resolves.toEqual({ ok: false, reason: 'kill EPERM' })
-
-if (platformDescriptor) {
- Object.defineProperty(process, 'platform', platformDescriptor)
+try {
+ Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' })
+ // configure mocks and run the assertion
+} finally {
+ if (platformDescriptor) {
+ Object.defineProperty(process, 'platform', platformDescriptor)
+ }
}
innocarpe
force-pushed
the
fix/windows-terminal-port-tree-kill
branch
from
July 24, 2026 08:40
f8985b5 to
981653f
Compare
3 tasks
Contributor
Author
|
Accidentally closed when a bad rebase helper force-pushed an empty tip. Restored and continued as #10346. |
innocarpe
added a commit
to innocarpe/orca
that referenced
this pull request
Jul 24, 2026
terminateWindowsProcessTree is best-effort and always resolves so PTY teardown is never blocked. Ports UI needs a real outcome: after taskkill, probe liveness with process.kill(pid, 0) and return failure if the PID is still alive. Addresses CodeRabbit review on stablyai#10183.
innocarpe
added a commit
to innocarpe/orca
that referenced
this pull request
Jul 24, 2026
EPERM from process.kill(pid, 0) means the process is still alive without signal rights; do not report Ports kill success in that case. Addresses CodeRabbit follow-up on stablyai#10183.
innocarpe
added a commit
to innocarpe/orca
that referenced
this pull request
Jul 25, 2026
terminateWindowsProcessTree is best-effort and always resolves so PTY teardown is never blocked. Ports UI needs a real outcome: after taskkill, probe liveness with process.kill(pid, 0) and return failure if the PID is still alive. Addresses CodeRabbit review on stablyai#10183.
innocarpe
added a commit
to innocarpe/orca
that referenced
this pull request
Jul 25, 2026
EPERM from process.kill(pid, 0) means the process is still alive without signal rights; do not report Ports kill success in that case. Addresses CodeRabbit follow-up on stablyai#10183.
innocarpe
added a commit
to innocarpe/orca
that referenced
this pull request
Jul 27, 2026
terminateWindowsProcessTree is best-effort and always resolves so PTY teardown is never blocked. Ports UI needs a real outcome: after taskkill, probe liveness with process.kill(pid, 0) and return failure if the PID is still alive. Addresses CodeRabbit review on stablyai#10183.
innocarpe
added a commit
to innocarpe/orca
that referenced
this pull request
Jul 27, 2026
EPERM from process.kill(pid, 0) means the process is still alive without signal rights; do not report Ports kill success in that case. Addresses CodeRabbit follow-up on stablyai#10183.
innocarpe
added a commit
to innocarpe/orca
that referenced
this pull request
Aug 6, 2026
terminateWindowsProcessTree is best-effort and always resolves so PTY teardown is never blocked. Ports UI needs a real outcome: after taskkill, probe liveness with process.kill(pid, 0) and return failure if the PID is still alive. Addresses CodeRabbit review on stablyai#10183.
innocarpe
added a commit
to innocarpe/orca
that referenced
this pull request
Aug 6, 2026
EPERM from process.kill(pid, 0) means the process is still alive without signal rights; do not report Ports kill success in that case. Addresses CodeRabbit follow-up on stablyai#10183.
innocarpe
added a commit
to innocarpe/orca
that referenced
this pull request
Aug 6, 2026
terminateWindowsProcessTree is best-effort and always resolves so PTY teardown is never blocked. Ports UI needs a real outcome: after taskkill, probe liveness with process.kill(pid, 0) and return failure if the PID is still alive. Addresses CodeRabbit review on stablyai#10183.
innocarpe
added a commit
to innocarpe/orca
that referenced
this pull request
Aug 6, 2026
EPERM from process.kill(pid, 0) means the process is still alive without signal rights; do not report Ports kill success in that case. Addresses CodeRabbit follow-up on stablyai#10183.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
taskkill /T /Fon the ConPTY root before shell kill, sonpm run dev/ Vite children release listening ports.process.kill.nohupchildren survive.Fixes #10150.
Why
Closing a terminal (or switching projects) on Windows only stopped the shell process. Child servers reparented and kept the port, so Project B appeared to serve Project A’s site.
Relationship to #10004 / #10100
Shares the Windows
taskkill /Thelper andkillWithDescendantSweepbranch. This PR applies tree-kill to all local Windows PTYs (not only agent sessions) and the Ports kill path, which is what frees dev-server ports. #10100 can be closed as partially superseded once this lands, or restacked to keep agent-specific POSIX notes.Test plan
windows-process-tree-kill,pty-descendant-termination(incl. Windows sweep),workspace-port-ownership,local-pty-providertestspnpm typecheck:nodenpm run devin a worktree terminal → close tab → confirm port free / Project B binds cleanly; Ports UI Stop also frees port