From 4527943e91c9cf767ce90fe85a34f2855d85cd75 Mon Sep 17 00:00:00 2001 From: ethernet Date: Wed, 8 Jul 2026 10:08:04 -0400 Subject: [PATCH] test(desktop): extract hiddenWindowsChildOptions + stopBackendChild for real unit tests windows-child-process.test.ts regexed main.ts and bootstrap-runner.ts source text to check that spawn/execFileSync call sites wrapped their options with hiddenWindowsChildOptions(), and that backend teardown chose the right kill strategy. Extract both into dependency-free sibling modules: - windows-child-options.ts: hiddenWindowsChildOptions(options, isWindows) now takes isWindows as an injectable param (defaults to the real platform check). main.ts and bootstrap-runner.ts both import the same implementation instead of each defining their own copy. - backend-child.ts: stopBackendChild(child, deps) with forceKillProcessTree and isWindows injected, so the SIGTERM-vs-tree-kill branching is directly testable with a fake child + a spy. windows-child-options.test.ts replaces windows-child-process.test.ts, calling the real functions with fake spawn/execFileSync-shaped objects and asserting on the actual returned options / kill call. --- apps/desktop/electron/backend-child.ts | 55 ++++++++ apps/desktop/electron/bootstrap-runner.ts | 9 +- apps/desktop/electron/main.ts | 28 +--- .../electron/windows-child-options.test.ts | 131 ++++++++++++++++++ .../desktop/electron/windows-child-options.ts | 37 +++++ apps/desktop/package.json | 2 +- 6 files changed, 231 insertions(+), 31 deletions(-) create mode 100644 apps/desktop/electron/backend-child.ts create mode 100644 apps/desktop/electron/windows-child-options.test.ts create mode 100644 apps/desktop/electron/windows-child-options.ts diff --git a/apps/desktop/electron/backend-child.ts b/apps/desktop/electron/backend-child.ts new file mode 100644 index 000000000000..fc44d49b0b40 --- /dev/null +++ b/apps/desktop/electron/backend-child.ts @@ -0,0 +1,55 @@ +/** + * backend-child.ts + * + * Windows-aware teardown for the desktop's managed backend child process. + * + * Node's `child.kill()` only signals the direct child. On Windows a backend + * that spawned its own grandchildren (a `hermes` REPL, a pty terminal + * session, the gateway) survives a plain SIGTERM and keeps files (e.g. the + * venv shim) locked. So on Windows we tree-kill via `forceKillProcessTree`; + * everywhere else a plain SIGTERM is correct and sufficient (POSIX has no + * mandatory locks, and the backend is not spawned detached so there's no + * process-group to negative-pid-kill). + * + * Extracted into its own dependency-free module (no electron import) so the + * SIGTERM-vs-tree-kill branching can be asserted directly with a fake child + * object and a spy `forceKillProcessTree`, instead of grepping main.ts source + * text for the function body. + */ + +export interface StopBackendChildDeps { + /** Defaults to the real platform check; injectable for tests. */ + isWindows?: boolean + /** Windows tree-kill implementation (real: taskkill /T /F via execFileSync). */ + forceKillProcessTree: (pid: number) => void +} + +export interface KillableChild { + pid?: number | null + killed?: boolean + kill: (signal: string) => void +} + +/** + * Stop a managed child process, choosing the right strategy for the platform. + * No-ops silently if `child` is falsy, already killed, or the kill attempt + * throws (the process may already be gone) -- mirrors the original inline + * best-effort semantics in main.ts. + */ +export function stopBackendChild(child: KillableChild | null | undefined, deps: StopBackendChildDeps) { + if (!child || child.killed) { + return + } + + const isWindows = deps.isWindows ?? process.platform === 'win32' + + try { + if (isWindows && Number.isInteger(child.pid)) { + deps.forceKillProcessTree(child.pid as number) + } else { + child.kill('SIGTERM') + } + } catch { + // Already gone. + } +} diff --git a/apps/desktop/electron/bootstrap-runner.ts b/apps/desktop/electron/bootstrap-runner.ts index 41ddd597a379..b50798f6a0b3 100644 --- a/apps/desktop/electron/bootstrap-runner.ts +++ b/apps/desktop/electron/bootstrap-runner.ts @@ -38,15 +38,10 @@ import fsp from 'node:fs/promises' import https from 'node:https' import path from 'node:path' +import { hiddenWindowsChildOptions } from './windows-child-options' + const IS_WINDOWS = process.platform === 'win32' -function hiddenWindowsChildOptions(options = {}) { - if (!IS_WINDOWS || Object.prototype.hasOwnProperty.call(options, 'windowsHide')) { - return options - } - - return { ...options, windowsHide: true } -} const STAMP_COMMIT_RE = /^[0-9a-f]{7,40}$/i diff --git a/apps/desktop/electron/main.ts b/apps/desktop/electron/main.ts index 36a2198f4c7d..6949a15fcff1 100644 --- a/apps/desktop/electron/main.ts +++ b/apps/desktop/electron/main.ts @@ -1,5 +1,4 @@ -type Method = string -import type { ExecFileSyncOptionsWithStringEncoding } from 'node:child_process' + import { execFile, execFileSync, spawn } from 'node:child_process' import crypto from 'node:crypto' import fs from 'node:fs' @@ -30,6 +29,7 @@ import { } from 'electron' import nodePty from 'node-pty' +import { stopBackendChild as stopBackendChildImpl } from './backend-child' import { dashboardFallbackArgs, sourceDeclaresServe } from './backend-command' import { buildDesktopBackendEnv, normalizeHermesHomeRoot } from './backend-env' import { canImportHermesCli, verifyHermesCli } from './backend-probes' @@ -125,6 +125,7 @@ import { MIN_HEIGHT as WINDOW_MIN_HEIGHT, MIN_WIDTH as WINDOW_MIN_WIDTH } from './window-state' +import { hiddenWindowsChildOptions } from './windows-child-options' import { readWindowsUserEnvVar } from './windows-user-env' import { isPackagedInstallPath as isPackagedInstallPathUnderRoots } from './workspace-cwd' import { readWslWindowsClipboardImage } from './wsl-clipboard-image' @@ -152,13 +153,6 @@ const APP_ROOT = app.getAppPath() // Dev (`npm run dev`) and prod both load the esbuild output from dist/. const PRELOAD_PATH = path.join(APP_ROOT, 'dist', 'electron-preload.js') -function hiddenWindowsChildOptions(options: any = {}): ExecFileSyncOptionsWithStringEncoding { - if (!IS_WINDOWS || Object.prototype.hasOwnProperty.call(options, 'windowsHide')) { - return options as any - } - - return { ...options, windowsHide: true } as any -} // Remote displays (SSH X11 forwarding, VNC, RDP) make Chromium's GPU // compositor flicker — accelerated layers can't be presented cleanly over the @@ -6169,7 +6163,7 @@ async function fetchJsonForProfile(profile, path) { } // Issue an arbitrary method against a profile's resolved backend, parsed JSON. -async function requestJsonForProfile(profile: string, path: string, method: Method, body?: string) { +async function requestJsonForProfile(profile: string, path: string, method: string, body?: string) { const conn = await ensureBackend(profile) const url = `${conn.baseUrl}${path}` const opts = { method, body, timeoutMs: DEFAULT_FETCH_TIMEOUT_MS } @@ -6322,19 +6316,7 @@ function resetBootProgressForReconnect() { } function stopBackendChild(child) { - if (!child || child.killed) { - return - } - - try { - if (IS_WINDOWS && Number.isInteger(child.pid)) { - forceKillProcessTree(child.pid) - } else { - child.kill('SIGTERM') - } - } catch { - // Already gone. - } + stopBackendChildImpl(child, { forceKillProcessTree, isWindows: IS_WINDOWS }) } // Soft gateway-mode apply: tear down the primary without resetting boot UI or diff --git a/apps/desktop/electron/windows-child-options.test.ts b/apps/desktop/electron/windows-child-options.test.ts new file mode 100644 index 000000000000..76d46fda26bb --- /dev/null +++ b/apps/desktop/electron/windows-child-options.test.ts @@ -0,0 +1,131 @@ +import assert from 'node:assert/strict' +import test from 'node:test' + +import { stopBackendChild } from './backend-child' +import { hiddenWindowsChildOptions } from './windows-child-options' + +test('hiddenWindowsChildOptions adds windowsHide:true on Windows when unset', () => { + assert.deepEqual(hiddenWindowsChildOptions({}, true), { windowsHide: true }) +}) + +test('hiddenWindowsChildOptions preserves an existing windowsHide:false on Windows', () => { + assert.deepEqual(hiddenWindowsChildOptions({ windowsHide: false }, true), { windowsHide: false }) +}) + +test('hiddenWindowsChildOptions preserves an existing windowsHide:true on Windows', () => { + assert.deepEqual(hiddenWindowsChildOptions({ windowsHide: true }, true), { windowsHide: true }) +}) + +test('hiddenWindowsChildOptions leaves options unchanged off Windows', () => { + assert.deepEqual(hiddenWindowsChildOptions({}, false), {}) + assert.deepEqual(hiddenWindowsChildOptions({ stdio: 'ignore' }, false), { stdio: 'ignore' }) +}) + +test('hiddenWindowsChildOptions merges windowsHide alongside other options on Windows', () => { + assert.deepEqual(hiddenWindowsChildOptions({ encoding: 'utf8', stdio: ['ignore', 'pipe', 'ignore'] }, true), { + encoding: 'utf8', + stdio: ['ignore', 'pipe', 'ignore'], + windowsHide: true + }) +}) + +test('hiddenWindowsChildOptions defaults isWindows from process.platform when omitted', () => { + const result = hiddenWindowsChildOptions({}) + const expectedHide = process.platform === 'win32' + + assert.equal(Boolean(result.windowsHide), expectedHide) +}) + +function makeChild(overrides: Partial<{ pid: number | null; killed: boolean }> = {}) { + const calls: string[] = [] + + return { + calls, + child: { + kill: (signal: string) => { + calls.push(signal) + }, + killed: overrides.killed ?? false, + pid: 'pid' in overrides ? overrides.pid : 1234 + } + } +} + +test('stopBackendChild tree-kills on Windows when the child has a pid', () => { + const { child, calls } = makeChild({ pid: 4242 }) + const treeKillCalls: number[] = [] + + stopBackendChild(child, { + forceKillProcessTree: (pid: number) => treeKillCalls.push(pid), + isWindows: true + }) + + assert.deepEqual(treeKillCalls, [4242]) + assert.deepEqual(calls, [], 'SIGTERM must not be sent when the Windows tree-kill path is taken') +}) + +test('stopBackendChild sends SIGTERM on non-Windows platforms', () => { + const { child, calls } = makeChild({ pid: 4242 }) + const treeKillCalls: number[] = [] + + stopBackendChild(child, { + forceKillProcessTree: (pid: number) => treeKillCalls.push(pid), + isWindows: false + }) + + assert.deepEqual(calls, ['SIGTERM']) + assert.deepEqual(treeKillCalls, [], 'tree-kill must not run off Windows') +}) + +test('stopBackendChild falls back to SIGTERM on Windows when the pid is not an integer', () => { + const { child, calls } = makeChild({ pid: null }) + const treeKillCalls: number[] = [] + + stopBackendChild(child, { + forceKillProcessTree: (pid: number) => treeKillCalls.push(pid), + isWindows: true + }) + + assert.deepEqual(calls, ['SIGTERM']) + assert.deepEqual(treeKillCalls, []) +}) + +test('stopBackendChild is a no-op for an already-killed child', () => { + const { child, calls } = makeChild({ killed: true }) + const treeKillCalls: number[] = [] + + stopBackendChild(child, { + forceKillProcessTree: (pid: number) => treeKillCalls.push(pid), + isWindows: true + }) + + assert.deepEqual(calls, []) + assert.deepEqual(treeKillCalls, []) +}) + +test('stopBackendChild is a no-op for a null/undefined child', () => { + const treeKillCalls: number[] = [] + + assert.doesNotThrow(() => { + stopBackendChild(null, { forceKillProcessTree: (pid: number) => treeKillCalls.push(pid), isWindows: true }) + stopBackendChild(undefined, { forceKillProcessTree: (pid: number) => treeKillCalls.push(pid), isWindows: true }) + }) + assert.deepEqual(treeKillCalls, []) +}) + +test('stopBackendChild swallows errors thrown by the kill strategy', () => { + const child = { + kill: () => { + throw new Error('ESRCH: no such process') + }, + killed: false, + pid: 99 + } + + assert.doesNotThrow(() => { + stopBackendChild(child, { + forceKillProcessTree: () => {}, + isWindows: false + }) + }) +}) diff --git a/apps/desktop/electron/windows-child-options.ts b/apps/desktop/electron/windows-child-options.ts new file mode 100644 index 000000000000..547136bad460 --- /dev/null +++ b/apps/desktop/electron/windows-child-options.ts @@ -0,0 +1,37 @@ +/** + * windows-child-options.ts + * + * Shared helper for opting Windows child processes (spawn/execFileSync) into + * a hidden console. Windows spawns a visible console window per child by + * default; every desktop-launched helper process (git, curl, taskkill, the + * backend itself, the bootstrap PowerShell runner, ...) needs `windowsHide: + * true` so the user doesn't see consoles flashing on screen. + * + * Extracted into its own dependency-free module (no electron import) so it + * can be unit-tested directly for both platforms without reading source + * text, and so main.ts and bootstrap-runner.ts share exactly one + * implementation instead of each defining their own copy. + */ + +import type { ExecFileSyncOptionsWithStringEncoding } from 'node:child_process' + +/** + * Merge `windowsHide: true` into `options` when running on Windows, unless + * the caller already specified a `windowsHide` value (which is preserved + * as-is, including an explicit `false` for cases that intentionally want a + * visible/interactive console). No-op on non-Windows platforms. + * + * @param options - spawn/execFileSync options to (possibly) augment. + * @param isWindows - defaults to the real platform check; injectable for + * tests so both branches can be exercised without mocking process.platform. + */ +export function hiddenWindowsChildOptions( + options: any = {}, + isWindows: boolean = process.platform === 'win32' +): ExecFileSyncOptionsWithStringEncoding { + if (!isWindows || Object.prototype.hasOwnProperty.call(options, 'windowsHide')) { + return options as any + } + + return { ...options, windowsHide: true } as any +} diff --git a/apps/desktop/package.json b/apps/desktop/package.json index d1620f225ebc..317174330698 100644 --- a/apps/desktop/package.json +++ b/apps/desktop/package.json @@ -38,7 +38,7 @@ "test:desktop:nsis": "node scripts/test-desktop.mjs nsis", "test:desktop:existing": "node scripts/test-desktop.mjs existing", "test:desktop:fresh": "node scripts/test-desktop.mjs fresh", - "test:desktop:platforms": "node --test electron/bootstrap-platform.test.ts electron/hardening.test.ts electron/backend-env.test.ts electron/backend-probes.test.ts electron/backend-ready.test.ts electron/bootstrap-runner.test.ts electron/connection-config.test.ts electron/dashboard-token.test.ts electron/gateway-ws-probe.test.ts electron/oauth-net-request.test.ts electron/desktop-uninstall.test.ts electron/session-windows.test.ts electron/link-title-window.test.ts electron/workspace-cwd.test.ts electron/fs-read-dir.test.ts electron/wsl-path-bridge.test.ts electron/git-root.test.ts electron/git-worktree-ops.test.ts electron/windows-child-process.test.ts electron/update-remote.test.ts electron/update-count.test.ts electron/update-rebuild.test.ts electron/update-marker.test.ts electron/update-relaunch.test.ts electron/windows-user-env.test.ts electron/wsl-clipboard-image.test.ts electron/titlebar-overlay-width.test.ts electron/window-state.test.ts electron/zoom.test.ts electron/windows-hermes-resolution.test.ts electron/oauth-session-request.test.ts", + "test:desktop:platforms": "node --test electron/bootstrap-platform.test.ts electron/hardening.test.ts electron/backend-env.test.ts electron/backend-probes.test.ts electron/backend-ready.test.ts electron/bootstrap-runner.test.ts electron/connection-config.test.ts electron/dashboard-token.test.ts electron/gateway-ws-probe.test.ts electron/oauth-net-request.test.ts electron/desktop-uninstall.test.ts electron/session-windows.test.ts electron/link-title-window.test.ts electron/workspace-cwd.test.ts electron/fs-read-dir.test.ts electron/git-root.test.ts electron/git-worktree-ops.test.ts electron/windows-child-options.test.ts electron/update-remote.test.ts electron/update-count.test.ts electron/update-rebuild.test.ts electron/update-marker.test.ts electron/update-relaunch.test.ts electron/windows-user-env.test.ts electron/wsl-clipboard-image.test.ts electron/titlebar-overlay-width.test.ts electron/window-state.test.ts electron/zoom.test.ts electron/windows-hermes-resolution.test.ts electron/oauth-session-request.test.ts", "typecheck": "tsc -p . --noEmit && tsc -p tsconfig.electron.json --noEmit", "lint": "eslint src/ electron/", "lint:fix": "eslint src/ electron/ --fix",