fix(ports): report a Stop as succeeded when the listener already exited (#15888)

* fix(ports): report a Stop as succeeded when the listener already exited

`killWorkspacePort` surfaced the raw `kill ESRCH` when the pid exited between
the authorizing re-scan and the signal. The port is free at that point -- which
is exactly what Stop was asked for -- so the UI reported a failure for work that
had already completed.

Also pins the pid we signal: `netstat -ano` and `lsof` report the process that
owns the socket, so the scanned pid is the listener itself, not a supervising
wrapper. Escalating this to a tree kill would reach descendants nobody asked to
stop without freeing anything extra.

* test(ports): give the spawn-stall test a budget longer than the stall

It blocks the calling thread for the full watchdog budget plus margin (5.2s) and
then asserts against vitest's 5s default, so it has been failing deterministically
since #12217.
This commit is contained in:
Neil
2026-08-22 00:19:26 -07:00
committed by GitHub
parent a04fe24c80
commit 84659e2eab
3 changed files with 65 additions and 1 deletions
@@ -42,7 +42,10 @@ describe('runPortScanCommandInProcess', () => {
expect(result.stdout).toBe(LSOF_OUTPUT)
expect(result.spawnMs).toBeGreaterThanOrEqual(SPAWN_STALL_MS - 100)
})
// Why the explicit budget: the stall is deliberately longer than the whole
// watchdog budget (5.2s), so on vitest's 5s default this test could never
// pass -- it has been red since #12217 landed.
}, 15_000)
it('kills the child and times out when the callback never arrives', async () => {
vi.useFakeTimers()
@@ -7,6 +7,25 @@ vi.mock('./local-workspace-port-scanner', () => ({
scanWorkspacePorts: scanWorkspacePortsMock
}))
function workspacePortScan(pid: number, port: number) {
return {
platform: 'win32' as const,
scannedAt: 0,
ports: [
{
id: `127.0.0.1:${port}:${pid}`,
bindHost: '127.0.0.1',
connectHost: '127.0.0.1',
port,
pid,
protocol: 'http' as const,
kind: 'workspace' as const,
worktreeId: worktrees[0]!.id
}
]
}
}
const worktrees = [{ id: 'repo::/repo', repoId: 'repo', displayName: 'main', path: '/repo' }]
describe('killWorkspacePort', () => {
@@ -15,6 +34,41 @@ describe('killWorkspacePort', () => {
vi.restoreAllMocks()
})
it('reports success when the listener exited before the signal landed', async () => {
// Why: the re-scan authorizes the pid, then the dev server exits on its own
// before `kill` runs. The port is free -- which is what Stop asked for --
// but the raw ESRCH surfaced as "Stop failed" on a port that was gone.
vi.spyOn(process, 'kill').mockImplementation(() => {
throw Object.assign(new Error('kill ESRCH'), { code: 'ESRCH' })
})
scanWorkspacePortsMock.mockResolvedValue(workspacePortScan(123, 5173))
expect(await killWorkspacePort(worktrees, { pid: 123, port: 5173 })).toEqual({ ok: true })
})
it('still reports a real failure to stop', async () => {
vi.spyOn(process, 'kill').mockImplementation(() => {
throw Object.assign(new Error('kill EPERM'), { code: 'EPERM' })
})
scanWorkspacePortsMock.mockResolvedValue(workspacePortScan(123, 5173))
expect(await killWorkspacePort(worktrees, { pid: 123, port: 5173 })).toEqual({
ok: false,
reason: 'kill EPERM'
})
})
it('signals the pid the socket scan named, which is the listener itself', async () => {
// Why pinned: netstat -ano / lsof report the process that owns the socket,
// not a supervising wrapper, so escalating this to a tree kill would reach
// descendants nobody asked to stop without freeing anything extra.
const killSpy = vi.spyOn(process, 'kill').mockImplementation(() => true)
scanWorkspacePortsMock.mockResolvedValue(workspacePortScan(123, 5173))
expect(await killWorkspacePort(worktrees, { pid: 123, port: 5173 })).toEqual({ ok: true })
expect(killSpy).toHaveBeenCalledExactlyOnceWith(123, 'SIGTERM')
})
// Regression for #11161 review: on an EDR-hooked host the background poller
// alternates the metadata skip, so an unscoped skip would fail Stop with
// "Only workspace-owned local processes can be stopped here" every other try.
@@ -109,6 +109,13 @@ export async function killWorkspacePort(
process.kill(pid, 'SIGTERM')
return { ok: true }
} catch (error) {
// Why ESRCH is success: the pid exited between the authorizing re-scan and
// this signal, so the listener is gone and the port is free -- which is
// what Stop was asked for. Surfacing the raw `kill ESRCH` made a Stop that
// had already succeeded read as a failure.
if ((error as NodeJS.ErrnoException)?.code === 'ESRCH') {
return { ok: true }
}
const message = error instanceof Error ? error.message : String(error)
return { ok: false, reason: message || 'Failed to stop the process.' }
}