diff --git a/.github/workflows/git-command-termination-runtime.yml b/.github/workflows/git-command-termination-runtime.yml new file mode 100644 index 00000000000..1602bae956a --- /dev/null +++ b/.github/workflows/git-command-termination-runtime.yml @@ -0,0 +1,22 @@ +name: Git command termination runtime +on: + pull_request: + paths: + - 'src/main/git/command-runner/spawned-command-tree-kill*' + - '.github/workflows/git-command-termination-runtime.yml' + workflow_dispatch: +permissions: + contents: read +jobs: + windows-exit: + runs-on: windows-latest + timeout-minutes: 20 + env: + ORCA_BACKGROUND_LAUNCH: '1' + steps: + - uses: actions/checkout@v6 + with: + persist-credentials: false + - uses: ./.github/actions/install-node-dependencies + - name: Verify exited native child does not trigger taskkill + run: node node_modules/vitest/vitest.mjs run --config config/vitest.config.ts src/main/git/command-runner/spawned-command-tree-kill.test.ts diff --git a/src/main/git/command-runner/spawned-command-tree-kill.test.ts b/src/main/git/command-runner/spawned-command-tree-kill.test.ts new file mode 100644 index 00000000000..758fb296227 --- /dev/null +++ b/src/main/git/command-runner/spawned-command-tree-kill.test.ts @@ -0,0 +1,122 @@ +import { ChildProcess } from 'node:child_process' +import { once } from 'node:events' +import { spawnProcess } from '../../../shared/child-process/run-process' +import type * as NodeChildProcess from 'node:child_process' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +const { spawnMock, admitMock } = vi.hoisted(() => ({ + spawnMock: vi.fn(), + admitMock: vi.fn(() => true) +})) + +vi.mock('node:child_process', async (importOriginal) => ({ + ...(await importOriginal()), + spawn: spawnMock +})) +vi.mock('../../own-chromium-tree-kill-guard', () => ({ + admitSelfInitiatedTreeKill: admitMock +})) + +import { killSpawnedCommandTree } from './spawned-command-tree-kill' + +const originalPlatform = process.platform + +function childWithPid(pid: number): ChildProcess { + const child = new ChildProcess() + Object.defineProperty(child, 'pid', { value: pid }) + vi.spyOn(child, 'kill').mockReturnValue(true) + vi.spyOn(child, 'unref').mockImplementation(() => {}) + return child +} + +describe('Git command tree termination', () => { + beforeEach(() => { + Object.defineProperty(process, 'platform', { value: 'win32', configurable: true }) + spawnMock.mockReset() + admitMock.mockReset().mockReturnValue(true) + }) + + afterEach(() => { + Object.defineProperty(process, 'platform', { value: originalPlatform, configurable: true }) + vi.restoreAllMocks() + }) + + it.each([0, 128])( + 'never taskkills a child that exited with code %i before close', + async (code) => { + const child = childWithPid(1234) + Object.defineProperty(child, 'exitCode', { value: code }) + + await killSpawnedCommandTree(child) + + expect(spawnMock).not.toHaveBeenCalled() + expect(admitMock).not.toHaveBeenCalled() + expect(child.kill).toHaveBeenCalledOnce() + } + ) + + it('never taskkills a child that exited by signal before close', async () => { + const child = childWithPid(1234) + Object.defineProperty(child, 'signalCode', { value: 'SIGTERM' }) + + await killSpawnedCommandTree(child) + + expect(spawnMock).not.toHaveBeenCalled() + expect(admitMock).not.toHaveBeenCalled() + }) + + it('still waits for tree termination when the Windows root has not exited', async () => { + const child = childWithPid(1234) + const killer = childWithPid(5678) + spawnMock.mockReturnValue(killer) + let settled = false + const pending = killSpawnedCommandTree(child).then(() => { + settled = true + }) + + await Promise.resolve() + expect(settled).toBe(false) + expect(spawnMock).toHaveBeenCalledWith('taskkill', ['/pid', '1234', '/t', '/f'], { + stdio: 'ignore', + windowsHide: true + }) + killer.emit('close', 0) + await pending + expect(child.kill).not.toHaveBeenCalled() + }) + + it('preserves handle termination on POSIX', async () => { + Object.defineProperty(process, 'platform', { value: 'linux', configurable: true }) + const child = childWithPid(1234) + + await killSpawnedCommandTree(child) + + expect(child.kill).toHaveBeenCalledOnce() + expect(spawnMock).not.toHaveBeenCalled() + }) + it.skipIf(originalPlatform !== 'win32').each([0, 128])( + 'does not taskkill an actual native Windows child after exit %i', + async (exitCode) => { + const original = await vi.importActual('node:child_process') + spawnMock.mockImplementation((program, args, options) => { + if (program !== process.execPath) { + throw new Error('Unexpected external process in native exit probe') + } + return original.spawn(program, args, options) + }) + const child = spawnProcess({ + program: process.execPath, + args: ['-e', `process.exit(${exitCode})`] + }) + const closed = once(child, 'close') + await once(child, 'exit') + expect(child.exitCode).toBe(exitCode) + expect(child.pid).toBeGreaterThan(0) + spawnMock.mockClear() + await killSpawnedCommandTree(child) + expect(spawnMock).not.toHaveBeenCalled() + expect(admitMock).not.toHaveBeenCalled() + await closed + } + ) +}) diff --git a/src/main/git/command-runner/spawned-command-tree-kill.ts b/src/main/git/command-runner/spawned-command-tree-kill.ts index 324e04f1db3..035764d5249 100644 --- a/src/main/git/command-runner/spawned-command-tree-kill.ts +++ b/src/main/git/command-runner/spawned-command-tree-kill.ts @@ -9,6 +9,11 @@ export function killSpawnedCommandTree(child: ChildProcess): Promise { child.kill() return Promise.resolve() } + // Windows may reuse the pid after exit while inherited pipes still delay close. + if ((child.exitCode ?? null) !== null || (child.signalCode ?? null) !== null) { + child.kill() + return Promise.resolve() + } if ( !admitSelfInitiatedTreeKill({ pid, site: 'git-command-tree-kill', scope: 'win-taskkill-tree' }) ) {