Skip to content

Commit 9dd2abc

Browse files
committed
fix(desktop): correct Windows process teardown, artifact guard, and taskbar identity
Windows has no signal delivery, so child.kill terminated only the Host PID and left node-pty shells and subagent hosts holding the loopback port. Kill the tree with taskkill /T /F in the child adapter, where every caller routes. Also: the packaged-runtime guard tested for a forward slash and so never fired on a Windows exec path; set the Application User Model ID so taskbar pinning survives an installer upgrade; and require the win32 node-pty prebuilds at pack time.
1 parent 17818ea commit 9dd2abc

6 files changed

Lines changed: 138 additions & 5 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+
Fix Windows process-tree shutdown, packaged-runtime guards, and taskbar identity in the desktop app

apps/desktop/scripts/verify-packaged-runtime.ts

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,12 @@ const REQUIRED_HOST_FILES = [
99
['@pymodel', 'pythinker-code', 'dist-web', 'index.html'],
1010
] as const
1111

12+
const REQUIRED_WINDOWS_NODE_PTY_ENTRIES = [
13+
['node-pty', 'prebuilds', 'win32-x64', 'pty.node'],
14+
['node-pty', 'prebuilds', 'win32-x64', 'conpty.node'],
15+
['node-pty', 'prebuilds', 'win32-x64', 'conpty_console_list.node'],
16+
] as const
17+
1218
/**
1319
* Verify the Host files required before the signed application can start.
1420
* @param context - Electron Builder's completed application directory.
@@ -21,6 +27,10 @@ export async function afterPack(context: AfterPackContext): Promise<void> {
2127
for (const segments of REQUIRED_HOST_FILES) {
2228
await access(join(resources, 'host', 'node_modules', ...segments))
2329
}
30+
if (context.electronPlatformName !== 'win32') return
31+
for (const segments of REQUIRED_WINDOWS_NODE_PTY_ENTRIES) {
32+
await access(join(resources, 'host', 'node_modules', ...segments))
33+
}
2434
}
2535

2636
export default afterPack

apps/desktop/src/host-supervisor.ts

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
/** Supervise the loopback Web Host used by the first desktop application. */
22

3-
import { spawn, type ChildProcessByStdio } from 'node:child_process'
3+
import { spawn, spawnSync, type ChildProcessByStdio } from 'node:child_process'
44
import type { Readable } from 'node:stream'
55

66
const READINESS_PREFIX = 'Pythinker server: '
@@ -297,6 +297,26 @@ export function spawnPythinkerServer(options: SpawnPythinkerServerOptions): Host
297297
return nodeChildAdapter(process)
298298
}
299299

300+
/**
301+
* Terminate a child and its descendants.
302+
*
303+
* Windows has no signal delivery: `child.kill` calls TerminateProcess on one
304+
* PID, so the Host's own children (node-pty shells, subagent hosts) survive and
305+
* keep holding the loopback port. `taskkill /T` walks the tree instead.
306+
*
307+
* ponytail: /F makes every Windows stop a forced stop — Node cannot deliver a
308+
* graceful SIGTERM to a Windows child at all. Add a stdin or IPC shutdown
309+
* channel to the Host if graceful Windows teardown is ever needed.
310+
*/
311+
function killProcessTree(child: ChildProcessByStdio<null, Readable, Readable>, signal: 'SIGTERM' | 'SIGKILL'): void {
312+
if (process.platform !== 'win32' || child.pid === undefined) {
313+
child.kill(signal)
314+
return
315+
}
316+
const result = spawnSync('taskkill', ['/pid', String(child.pid), '/T', '/F'], { windowsHide: true })
317+
if (result.error !== undefined || result.status !== 0) child.kill(signal)
318+
}
319+
300320
/** Adapt Node's event overloads to the supervisor's explicit ownership API. */
301321
function nodeChildAdapter(child: ChildProcessByStdio<null, Readable, Readable>): HostChild {
302322
return {
@@ -312,7 +332,7 @@ function nodeChildAdapter(child: ChildProcessByStdio<null, Readable, Readable>):
312332
return () => { child.off('error', listener) }
313333
},
314334
kill(signal) {
315-
child.kill(signal)
335+
killProcessTree(child, signal)
316336
},
317337
}
318338
}

apps/desktop/src/main.ts

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22

33
import { randomUUID } from 'node:crypto'
44
import { existsSync, readFileSync, writeFileSync } from 'node:fs'
5-
import { join, resolve } from 'node:path'
5+
import { isAbsolute, join, resolve } from 'node:path'
66
import {
77
app,
88
BrowserWindow,
@@ -109,7 +109,7 @@ function hostPaths(): { nodeExecutable: string; cliEntry: string; cwd: string; e
109109
}
110110

111111
function assertHostArtifacts(paths: ReturnType<typeof hostPaths>): void {
112-
if (paths.nodeExecutable.includes('/') && !existsSync(paths.nodeExecutable)) {
112+
if (isAbsolute(paths.nodeExecutable) && !existsSync(paths.nodeExecutable)) {
113113
throw new Error(`desktop Node runtime is missing: ${paths.nodeExecutable}`)
114114
}
115115
if (!existsSync(paths.cliEntry)) {
@@ -298,6 +298,7 @@ function requestAppQuit(): Promise<void> {
298298
}
299299

300300
async function boot(): Promise<void> {
301+
if (process.platform === 'win32') app.setAppUserModelId('com.pythinker.desktop')
301302
if (bootQuitPromise !== undefined) return
302303
initializeDesktopTelemetry()
303304
const paths = hostPaths()

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

Lines changed: 64 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { spawn } from 'node:child_process'
1+
import { spawn, spawnSync } from 'node:child_process'
22
import { afterEach, describe, expect, it, vi } from 'vitest'
33
import {
44
createHostSupervisor,
@@ -314,4 +314,67 @@ describe('desktop Host process', () => {
314314
expect.objectContaining({ env: { PYTHINKER_DESKTOP: '1', ELECTRON_RUN_AS_NODE: '1' } }),
315315
)
316316
})
317+
318+
it('kills the Windows Host process tree', async () => {
319+
const child = {
320+
pid: 4242,
321+
stdout: { on: vi.fn(), off: vi.fn() },
322+
stderr: { on: vi.fn(), off: vi.fn() },
323+
on: vi.fn(),
324+
off: vi.fn(),
325+
kill: vi.fn(),
326+
}
327+
vi.mocked(spawn).mockReturnValue(child as never)
328+
vi.mocked(spawnSync).mockReturnValue({
329+
pid: 4242,
330+
output: [],
331+
stdout: Buffer.alloc(0),
332+
stderr: Buffer.alloc(0),
333+
status: 0,
334+
signal: null,
335+
})
336+
vi.spyOn(process, 'platform', 'get').mockReturnValue('win32')
337+
338+
const { spawnPythinkerServer } = await import('../src/host-supervisor')
339+
const host = spawnPythinkerServer({
340+
nodeExecutable: 'node',
341+
cliEntry: '/tmp/launcher.mjs',
342+
cwd: '/tmp',
343+
env: {},
344+
})
345+
host.kill('SIGTERM')
346+
347+
expect(spawnSync).toHaveBeenCalledWith(
348+
'taskkill',
349+
['/pid', '4242', '/T', '/F'],
350+
{ windowsHide: true },
351+
)
352+
expect(child.kill).not.toHaveBeenCalled()
353+
})
354+
355+
it('uses child.kill unchanged on non-Windows', 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).mockClear()
366+
vi.spyOn(process, 'platform', 'get').mockReturnValue('darwin')
367+
368+
const { spawnPythinkerServer } = await import('../src/host-supervisor')
369+
const host = spawnPythinkerServer({
370+
nodeExecutable: 'node',
371+
cliEntry: '/tmp/launcher.mjs',
372+
cwd: '/tmp',
373+
env: {},
374+
})
375+
host.kill('SIGTERM')
376+
377+
expect(child.kill).toHaveBeenCalledWith('SIGTERM')
378+
expect(spawnSync).not.toHaveBeenCalled()
379+
})
317380
})

apps/desktop/tests/verify-packaged-runtime.spec.ts

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,4 +38,38 @@ describe('packaged desktop runtime verification', () => {
3838
await rm(appOutDir, { recursive: true, force: true })
3939
}
4040
})
41+
42+
it('rejects a Windows shell whose node-pty native closure was filtered out', async () => {
43+
const appOutDir = await mkdtemp(join(tmpdir(), 'pythinker-packaged-runtime-'))
44+
try {
45+
const resources = join(appOutDir, 'resources', 'host', 'node_modules')
46+
const cli = join(resources, '@pymodel', 'pythinker-code', 'dist', 'launcher.mjs')
47+
const web = join(resources, '@pymodel', 'pythinker-code', 'dist-web', 'index.html')
48+
await mkdir(join(cli, '..'), { recursive: true })
49+
await mkdir(join(web, '..'), { recursive: true })
50+
await writeFile(cli, '')
51+
await writeFile(web, '')
52+
53+
await expect(afterPack(context(appOutDir, 'win32'))).rejects.toMatchObject({ code: 'ENOENT' })
54+
} finally {
55+
await rm(appOutDir, { recursive: true, force: true })
56+
}
57+
})
58+
59+
it('does not require Windows node-pty entries for a Darwin shell', async () => {
60+
const appOutDir = await mkdtemp(join(tmpdir(), 'pythinker-packaged-runtime-'))
61+
try {
62+
const resources = join(appOutDir, 'Pythinker.app', 'Contents', 'Resources', 'host', 'node_modules')
63+
const cli = join(resources, '@pymodel', 'pythinker-code', 'dist', 'launcher.mjs')
64+
const web = join(resources, '@pymodel', 'pythinker-code', 'dist-web', 'index.html')
65+
await mkdir(join(cli, '..'), { recursive: true })
66+
await mkdir(join(web, '..'), { recursive: true })
67+
await writeFile(cli, '')
68+
await writeFile(web, '')
69+
70+
await expect(afterPack(context(appOutDir))).resolves.toBeUndefined()
71+
} finally {
72+
await rm(appOutDir, { recursive: true, force: true })
73+
}
74+
})
4175
})

0 commit comments

Comments
 (0)