Skip to content

Commit d94ba4b

Browse files
committed
fix(desktop): bound the Windows tree kill and correct the installer docs
spawnSync blocks the Electron main loop, so a stalled taskkill could freeze shutdown indefinitely. Cap it and let the existing fallback degrade to a single-process kill. The README claimed a per-user installer with no elevation required. With oneClick and perMachine both false, electron-builder shows an install-mode page, so per-user is the default rather than the contract. Describe the real behavior and pin it with a test.
1 parent 7321b89 commit d94ba4b

6 files changed

Lines changed: 59 additions & 9 deletions

File tree

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'@pymodel/pythinker-desktop': patch
3+
---
4+
5+
Bound the Windows process-tree kill so a stalled taskkill cannot freeze desktop shutdown

apps/desktop/README.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,7 @@ rmdir "$MOUNT_POINT"
6666

6767
### Windows
6868

69-
Run `pnpm run dist:win` on a native Windows x64 host; cross-building from macOS is not possible because the staged Host closure contains platform-gated native packages. The output is `dist/Pythinker-<version>-x64-Setup.exe`, a per-user NSIS installer with no elevation required and a selectable installation directory. Artifacts are unsigned unless `WIN_CSC_LINK` and `WIN_CSC_KEY_PASSWORD` are set.
69+
Run `pnpm run dist:win` on a native Windows x64 host; cross-building from macOS is not possible because the staged Host closure contains platform-gated native packages. The output is `dist/Pythinker-<version>-x64-Setup.exe`, an assisted NSIS installer that defaults to a per-user install, offers a per-machine option that requires elevation, and lets you select the installation directory. Artifacts are unsigned unless `WIN_CSC_LINK` and `WIN_CSC_KEY_PASSWORD` are set.
7070

7171
## Known limitations
7272

apps/desktop/src/host-supervisor.ts

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import type { Readable } from 'node:stream'
66
const READINESS_PREFIX = 'Pythinker server: '
77
const DEFAULT_READINESS_TIMEOUT_MS = 90_000
88
const DEFAULT_SHUTDOWN_TIMEOUT_MS = 5_000
9+
const TASKKILL_TIMEOUT_MS = 5_000
910
const MAX_STARTUP_OUTPUT_CHARS = 32_768
1011

1112
/** Incremental parser for the Web Host's canonical readiness line. */
@@ -303,6 +304,8 @@ export function spawnPythinkerServer(options: SpawnPythinkerServerOptions): Host
303304
* Windows has no signal delivery: `child.kill` calls TerminateProcess on one
304305
* PID, so the Host's own children (node-pty shells, subagent hosts) survive and
305306
* keep holding the loopback port. `taskkill /T` walks the tree instead.
307+
* Because this call is synchronous, keep it bounded so a stalled taskkill falls
308+
* back to a single-process kill instead of blocking the Electron main loop indefinitely.
306309
*
307310
* ponytail: /F makes every Windows stop a forced stop — Node cannot deliver a
308311
* graceful SIGTERM to a Windows child at all. Add a stdin or IPC shutdown
@@ -313,7 +316,10 @@ function killProcessTree(child: ChildProcessByStdio<null, Readable, Readable>, s
313316
child.kill(signal)
314317
return
315318
}
316-
const result = spawnSync('taskkill', ['/pid', String(child.pid), '/T', '/F'], { windowsHide: true })
319+
const result = spawnSync('taskkill', ['/pid', String(child.pid), '/T', '/F'], {
320+
windowsHide: true,
321+
timeout: TASKKILL_TIMEOUT_MS,
322+
})
317323
if (result.error !== undefined || result.status !== 0) child.kill(signal)
318324
}
319325

apps/desktop/tests/host-supervisor.spec.ts

Lines changed: 35 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -331,7 +331,7 @@ describe('desktop Host process', () => {
331331
stdout: Buffer.alloc(0),
332332
stderr: Buffer.alloc(0),
333333
status: 0,
334-
signal: null,
334+
signal: 'SIGTERM',
335335
})
336336
vi.spyOn(process, 'platform', 'get').mockReturnValue('win32')
337337

@@ -347,11 +347,44 @@ describe('desktop Host process', () => {
347347
expect(spawnSync).toHaveBeenCalledWith(
348348
'taskkill',
349349
['/pid', '4242', '/T', '/F'],
350-
{ windowsHide: true },
350+
{ windowsHide: true, timeout: 5_000 },
351351
)
352352
expect(child.kill).not.toHaveBeenCalled()
353353
})
354354

355+
it('falls back to killing the Windows Host when taskkill times out', async () => {
356+
const child = {
357+
pid: 4242,
358+
stdout: { on: vi.fn(), off: vi.fn() },
359+
stderr: { on: vi.fn(), off: vi.fn() },
360+
on: vi.fn(),
361+
off: vi.fn(),
362+
kill: vi.fn(),
363+
}
364+
vi.mocked(spawn).mockReturnValue(child as never)
365+
vi.mocked(spawnSync).mockReturnValue({
366+
pid: 4242,
367+
output: [],
368+
stdout: Buffer.alloc(0),
369+
stderr: Buffer.alloc(0),
370+
status: null,
371+
signal: null,
372+
error: Object.assign(new Error('spawnSync taskkill ETIMEDOUT'), { code: 'ETIMEDOUT' }),
373+
})
374+
vi.spyOn(process, 'platform', 'get').mockReturnValue('win32')
375+
376+
const { spawnPythinkerServer } = await import('../src/host-supervisor')
377+
const host = spawnPythinkerServer({
378+
nodeExecutable: 'node',
379+
cliEntry: '/tmp/launcher.mjs',
380+
cwd: '/tmp',
381+
env: {},
382+
})
383+
host.kill('SIGTERM')
384+
385+
expect(child.kill).toHaveBeenCalledWith('SIGTERM')
386+
})
387+
355388
it('uses child.kill unchanged on non-Windows', async () => {
356389
const child = {
357390
pid: 4242,

apps/desktop/tests/packaging-config.spec.ts

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -112,6 +112,12 @@ describe('desktop packaging configuration', () => {
112112
expect(desktopPackage.scripts['dist:win']).toBe('node --import tsx scripts/release-win.ts')
113113
})
114114

115+
it('offers an assisted installer that defaults to a per-user install', () => {
116+
expect(desktopPackage.build.nsis.oneClick).toBe(false)
117+
expect(desktopPackage.build.nsis.perMachine).toBe(false)
118+
expect(desktopPackage.build.nsis.allowElevation).toBe(true)
119+
})
120+
115121
it('exposes desktop commands at the repository root', () => {
116122
expect(rootPackage.scripts['dev:desktop']).toBe('pnpm -C apps/desktop run dev')
117123
expect(rootPackage.scripts['package:desktop']).toBe('pnpm -C apps/desktop run package')

apps/desktop/tests/verify-win-installer.spec.ts

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -37,21 +37,21 @@ describe('Windows installer verification', () => {
3737
it('accepts the installer and unpacked shell when both are PE binaries', async () => {
3838
await withFixture(async (root) => {
3939
await createFixture(root, portableExecutable())
40-
expect(() => verifyWindowsInstaller(root)).not.toThrow()
40+
expect(() => { verifyWindowsInstaller(root) }).not.toThrow()
4141
})
4242
})
4343

4444
it('rejects a missing installer', async () => {
4545
await withFixture(async (root) => {
4646
await createFixture(root)
47-
expect(() => verifyWindowsInstaller(root)).toThrow('ENOENT')
47+
expect(() => { verifyWindowsInstaller(root) }).toThrow('ENOENT')
4848
})
4949
})
5050

5151
it('rejects an artifact without a DOS header', async () => {
5252
await withFixture(async (root) => {
5353
await createFixture(root, portableExecutable('ZZ'))
54-
expect(() => verifyWindowsInstaller(root)).toThrow(/no DOS header/)
54+
expect(() => { verifyWindowsInstaller(root) }).toThrow(/no DOS header/u)
5555
})
5656
})
5757

@@ -61,14 +61,14 @@ describe('Windows installer verification', () => {
6161
truncated.write('MZ', 0, 2, 'latin1')
6262
truncated.writeUInt32LE(0x80, 0x3c)
6363
await createFixture(root, truncated)
64-
expect(() => verifyWindowsInstaller(root)).toThrow(/out-of-range PE offset/)
64+
expect(() => { verifyWindowsInstaller(root) }).toThrow(/out-of-range PE offset/u)
6565
})
6666
})
6767

6868
it('rejects an artifact without a PE signature', async () => {
6969
await withFixture(async (root) => {
7070
await createFixture(root, portableExecutable('MZ', 'NE\0\0'))
71-
expect(() => verifyWindowsInstaller(root)).toThrow(/no PE signature/)
71+
expect(() => { verifyWindowsInstaller(root) }).toThrow(/no PE signature/u)
7272
})
7373
})
7474
})

0 commit comments

Comments
 (0)