diff --git a/apps/desktop/electron/main.ts b/apps/desktop/electron/main.ts index d4077c3a4a..2dd0ec290b 100644 --- a/apps/desktop/electron/main.ts +++ b/apps/desktop/electron/main.ts @@ -368,7 +368,7 @@ import { } from './native-oauth' import { runNativeLogin } from './native-oauth-login' import { loadNativeTokenSet, type NativeTokenStoreIo, persistNativeTokenSet } from './native-token-store' -import { execGit, setNoConsoleGitRoots } from './no-console-git' +import { execGit, killTimedGitChildren, setNoConsoleGitRoots } from './no-console-git' import { registerNativeNotifications } from './notification-ipc' import { isExpectedOauthNavigationAbort } from './oauth-navigation' import { serializeJsonBody, setJsonRequestHeaders } from './oauth-net-request' @@ -17767,6 +17767,7 @@ app.on('before-quit', () => { // Close the pooled keep-alive sockets on quit so lingering connections can't // hold the event loop open or leak FDs past app teardown. app.on('will-quit', () => { + killTimedGitChildren() sshIsolatedKeepalives.stopAll() destroyKeepaliveAgents() nativeNotifications.dispose() diff --git a/apps/desktop/electron/no-console-git.test.ts b/apps/desktop/electron/no-console-git.test.ts index 3c5cbd1b80..4e6c07f9f5 100644 --- a/apps/desktop/electron/no-console-git.test.ts +++ b/apps/desktop/electron/no-console-git.test.ts @@ -10,6 +10,7 @@ import { expect, test, vi } from 'vitest' import { CREATE_NO_WINDOW, execGit, + killTimedGitChildren, NO_CONSOLE_GIT_SCRIPT, planNoConsoleGitSpawn, resolveNoConsolePython, @@ -19,11 +20,31 @@ import { const gitArgs = ['-c', 'windows.appendAtomically=false', 'merge-base', '--is-ancestor', 'origin/main', 'HEAD'] const gitBin = 'C:\\Program Files\\Git\\cmd\\git.exe' -test('a timed-out git command reaps descendants, including the Windows Python host', async () => { +// A node "git" that spawns a grandchild owning a TCP listener, then hangs: +// the listener only goes away if the whole tree is killed. +function hangingTreeFixture() { const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'git-timeout-')) const pids = path.join(dir, 'pids.json') const fixture = path.join(dir, 'hang.cjs') + fs.writeFileSync( + fixture, + ` + const { spawn } = require('node:child_process'); + const worker = String.raw\`const fs = require('node:fs'); + const server = require('node:net').createServer(socket => socket.end()); + server.listen(0, '127.0.0.1', () => fs.writeFileSync(process.argv[1], + JSON.stringify({ pids: [Number(process.argv[2]), process.pid], port: server.address().port })));\`; + spawn(process.execPath, ['-e', worker, process.argv[2], String(process.pid)], { stdio: 'inherit' }); + setInterval(() => {}, 1000); + ` + ) + + if (process.platform === 'win32') { + const python = execFileSync('python3', ['-c', 'import sys; print(sys.executable)'], { encoding: 'utf8' }).trim() + vi.stubEnv('HERMES_DESKTOP_PYTHON', python) + } + const listening = (port: number): Promise => new Promise(resolve => { const socket = connect(port, '127.0.0.1') @@ -38,33 +59,13 @@ test('a timed-out git command reaps descendants, including the Windows Python ho socket.setTimeout(2000, () => done(true)) }) - fs.writeFileSync( - fixture, - ` - const { spawn } = require('node:child_process'); - const worker = String.raw\`const fs = require('node:fs'); - const server = require('node:net').createServer(socket => socket.end()); - server.listen(0, '127.0.0.1', () => fs.writeFileSync(process.argv[1], - JSON.stringify({ pids: [Number(process.argv[2]), process.pid], port: server.address().port })));\`; - spawn(process.execPath, ['-e', worker, process.argv[2], String(process.pid)], { stdio: 'inherit' }); - setInterval(() => {}, 1000); - ` - ) + const port = async (): Promise => { + await vi.waitFor(() => expect(fs.existsSync(pids)).toBe(true), { timeout: 5000 }) - try { - if (process.platform === 'win32') { - const python = execFileSync('python3', ['-c', 'import sys; print(sys.executable)'], { encoding: 'utf8' }).trim() - vi.stubEnv('HERMES_DESKTOP_PYTHON', python) - } + return JSON.parse(fs.readFileSync(pids, 'utf8')).port + } - const success = await execGit(process.execPath, ['-e', 'process.stdout.write("ok")'], { timeoutMs: 5000 }) - expect(success).toMatchObject({ code: 0, stdout: 'ok' }) - - const error = await execGit(process.execPath, [fixture, pids], { timeoutMs: 5000 }).catch(error => error) - const { port } = JSON.parse(fs.readFileSync(pids, 'utf8')) - await vi.waitFor(async () => expect(await listening(port)).toBe(false), { timeout: 3000 }) - expect(error).toMatchObject({ code: 'ETIMEDOUT' }) - } finally { + const cleanup = () => { if (fs.existsSync(pids)) { for (const pid of JSON.parse(fs.readFileSync(pids, 'utf8')).pids) { try { @@ -78,6 +79,41 @@ test('a timed-out git command reaps descendants, including the Windows Python ho vi.unstubAllEnvs() fs.rmSync(dir, { recursive: true, force: true }) } + + return { args: [fixture, pids], cleanup, listening, port } +} + +test('a timed-out git command reaps descendants, including the Windows Python host', async () => { + const tree = hangingTreeFixture() + + try { + const success = await execGit(process.execPath, ['-e', 'process.stdout.write("ok")'], { timeoutMs: 5000 }) + expect(success).toMatchObject({ code: 0, stdout: 'ok' }) + + const error = await execGit(process.execPath, tree.args, { timeoutMs: 5000 }).catch(error => error) + const port = await tree.port() + await vi.waitFor(async () => expect(await tree.listening(port)).toBe(false), { timeout: 3000 }) + expect(error).toMatchObject({ code: 'ETIMEDOUT' }) + } finally { + tree.cleanup() + } +}, 20_000) + +test('app quit kills a timed git command still running, descendants included', async () => { + const tree = hangingTreeFixture() + + try { + const pending = execGit(process.execPath, tree.args, { timeoutMs: 60_000 }).catch(error => error) + const port = await tree.port() + expect(await tree.listening(port)).toBe(true) + + killTimedGitChildren() + + await vi.waitFor(async () => expect(await tree.listening(port)).toBe(false), { timeout: 3000 }) + await pending + } finally { + tree.cleanup() + } }, 20_000) test('windows git spawn uses a CREATE_NO_WINDOW host and does not rewrite git argv', () => { diff --git a/apps/desktop/electron/no-console-git.ts b/apps/desktop/electron/no-console-git.ts index dfeffe0e75..cb8ee6600a 100644 --- a/apps/desktop/electron/no-console-git.ts +++ b/apps/desktop/electron/no-console-git.ts @@ -8,7 +8,7 @@ // unchanged. simple-git cannot take a creation flag, so its binary tuple is // [python, this host script]. -import { execFile, spawn, type SpawnOptions } from 'node:child_process' +import { type ChildProcess, execFile, execFileSync, spawn, type SpawnOptions } from 'node:child_process' import fs from 'node:fs' import os from 'node:os' import path from 'node:path' @@ -244,6 +244,58 @@ export function hiddenGitSpawnSpec( } } +// Timed commands still running. On POSIX they lead their own process group, +// so nothing reaches them when the app exits unless we do it here (#125243: +// probes and their promisor fetches outliving a restart, reparented to PID 1). +const timedChildren = new Set() + +const TASKKILL_OPTIONS = { windowsHide: true, timeout: 5000 } + +// On Windows a root that already exited has no tree left for taskkill /T to +// walk, and its pid may already belong to someone else. A POSIX group +// outlives its leader, so it is always signaled. +function hasTreeToKill(child: ChildProcess): child is ChildProcess & { pid: number } { + if (!child.pid) { + return false + } + + return process.platform !== 'win32' || (child.exitCode === null && child.signalCode === null) +} + +function killGroup(child: ChildProcess & { pid: number }): void { + try { + process.kill(-child.pid, 'SIGKILL') + } catch { + child.kill('SIGKILL') + } +} + +/** Kill every timed git command still running, with its descendants. For app quit. */ +export function killTimedGitChildren(): void { + const live = [...timedChildren].filter(hasTreeToKill) + timedChildren.clear() + + if (process.platform !== 'win32') { + live.forEach(killGroup) + + return + } + + if (live.length === 0) { + return + } + + // One synchronous call for every tree: will-quit cannot wait on N serial ones. + try { + execFileSync('taskkill', [...live.flatMap(child => ['/PID', String(child.pid)]), '/T', '/F'], { + ...TASKKILL_OPTIONS, + stdio: 'ignore' + }) + } catch { + live.forEach(child => child.kill('SIGKILL')) + } +} + export function execGit( gitBin: string, args: string[], @@ -294,31 +346,22 @@ export function execGit( // failed tree kill. The timeout remains a failure, never exit 0. child.stdout?.destroy() child.stderr?.destroy() + timedChildren.delete(child) finish(timeoutError) } - if (process.platform === 'win32' && child.pid) { - execFile( - 'taskkill', - ['/PID', String(child.pid), '/T', '/F'], - { windowsHide: true, timeout: 5000 }, - error => { - if (error) { - child.kill('SIGKILL') - } - - done() + if (!hasTreeToKill(child)) { + done() + } else if (process.platform === 'win32') { + execFile('taskkill', ['/PID', String(child.pid), '/T', '/F'], TASKKILL_OPTIONS, error => { + if (error) { + child.kill('SIGKILL') } - ) + + done() + }) } else { - try { - if (child.pid) { - process.kill(-child.pid, 'SIGKILL') - } - } catch { - child.kill('SIGKILL') - } - + killGroup(child) done() } }, options.timeoutMs) @@ -330,18 +373,17 @@ export function execGit( child.stderr?.on('data', chunk => { stderr += chunk.toString() }) - child.once('error', error => { - if (timer) { - clearTimeout(timer) - } + if (timer) { + timedChildren.add(child) + child.once('close', () => timedChildren.delete(child)) + } + + child.once('error', error => { + timedChildren.delete(child) finish(error) }) child.once('close', code => { - if (timer) { - clearTimeout(timer) - } - // On Windows wait for taskkill's completion, not just the root's exit. if (!timeoutError) { finish(undefined, code)