Browse Source

fix(subprocess): honor teardown and PATH semantics

Tianyi Cui 2 months ago
parent
commit
098f826001

+ 5 - 2
packages/subprocess/subprocess-local/src/index.ts

@@ -67,7 +67,10 @@ export class LocalSubprocessService extends SubprocessService {
         pending.push(terminal.waitForExit().then(() => { this.terminals.delete(terminal) }))
         pending.push(terminal.waitForExit().then(() => { this.terminals.delete(terminal) }))
       }
       }
       this.live.clear()
       this.live.clear()
-      await Promise.all(pending)
+      const outcomes = await Promise.allSettled(pending)
+      for (const outcome of outcomes) {
+        if (outcome.status === 'rejected') throw outcome.reason
+      }
       await rm(this.runtimeRoot, { recursive: true, force: true })
       await rm(this.runtimeRoot, { recursive: true, force: true })
     }, 'local subprocess teardown')
     }, 'local subprocess teardown')
   }
   }
@@ -106,7 +109,7 @@ export class LocalSubprocessService extends SubprocessService {
       ? (environmentValue(env, 'PATHEXT') ?? '.COM;.EXE;.BAT;.CMD').split(';')
       ? (environmentValue(env, 'PATHEXT') ?? '.COM;.EXE;.BAT;.CMD').split(';')
       : ['']
       : ['']
     return path.split(delimiter).flatMap(directory =>
     return path.split(delimiter).flatMap(directory =>
-      directory === '' ? [] : extensions.map(extension => resolve(this.cwd, directory, command + extension)))
+      extensions.map(extension => resolve(this.cwd, directory, command + extension)))
   }
   }
 
 
   spawn(spec: SubprocessSpawnSpec): SubprocessHandle {
   spawn(spec: SubprocessSpawnSpec): SubprocessHandle {

+ 27 - 9
packages/subprocess/subprocess-local/tests/local.spec.ts

@@ -1,7 +1,7 @@
 import { PassThrough } from 'node:stream'
 import { PassThrough } from 'node:stream'
 import { describe, expect, it, vi } from 'vitest'
 import { describe, expect, it, vi } from 'vitest'
 import { rm, stat } from 'node:fs/promises'
 import { rm, stat } from 'node:fs/promises'
-import { basename, delimiter, dirname, relative } from 'node:path'
+import { basename, dirname, relative, resolve } from 'node:path'
 import { Context } from 'cordis'
 import { Context } from 'cordis'
 import LocalSubprocessService from '@deepseek-ai/dsh-subprocess-local'
 import LocalSubprocessService from '@deepseek-ai/dsh-subprocess-local'
 import type { SubprocessSpawnSpec, SubprocessTerminalHandle, SubprocessTerminalSpawnSpec } from '@deepseek-ai/dsh-subprocess'
 import type { SubprocessSpawnSpec, SubprocessTerminalHandle, SubprocessTerminalSpawnSpec } from '@deepseek-ai/dsh-subprocess'
@@ -42,6 +42,9 @@ describe('LocalSubprocessService', () => {
     expect(await ctx.subprocess.resolveExecutable(basename(process.execPath), {
     expect(await ctx.subprocess.resolveExecutable(basename(process.execPath), {
       PATH: relative(process.cwd(), dirname(process.execPath)) || '.',
       PATH: relative(process.cwd(), dirname(process.execPath)) || '.',
     })).toBe(process.execPath)
     })).toBe(process.execPath)
+    Reflect.set(ctx.subprocess, 'cwd', dirname(process.execPath))
+    expect(await ctx.subprocess.resolveExecutable(basename(process.execPath), { PATH: '' }))
+      .toBe(process.execPath)
     await expect(ctx.subprocess.resolveExecutable('')).rejects.toThrow('must be non-empty')
     await expect(ctx.subprocess.resolveExecutable('')).rejects.toThrow('must be non-empty')
     await expect(ctx.subprocess.resolveExecutable('dsh-command-that-does-not-exist', { PATH: '' }))
     await expect(ctx.subprocess.resolveExecutable('dsh-command-that-does-not-exist', { PATH: '' }))
       .rejects.toThrow('was not found on PATH')
       .rejects.toThrow('was not found on PATH')
@@ -54,7 +57,7 @@ describe('LocalSubprocessService', () => {
     await fiber.dispose()
     await fiber.dispose()
   })
   })
 
 
-  it('builds Windows executable candidates without empty PATH entries', async () => {
+  it('builds Windows executable candidates with case-insensitive overrides', async () => {
     const ctx = new Context()
     const ctx = new Context()
     const fiber = await ctx.plugin(LocalSubprocessService)
     const fiber = await ctx.plugin(LocalSubprocessService)
     const service = ctx.subprocess as LocalSubprocessService
     const service = ctx.subprocess as LocalSubprocessService
@@ -64,13 +67,13 @@ describe('LocalSubprocessService', () => {
     const platform = vi.spyOn(process, 'platform', 'get').mockReturnValue('win32')
     const platform = vi.spyOn(process, 'platform', 'get').mockReturnValue('win32')
     try {
     try {
       expect(Object.keys(childEnv()).filter(key => key.toUpperCase() === 'PATH')).toHaveLength(1)
       expect(Object.keys(childEnv()).filter(key => key.toUpperCase() === 'PATH')).toHaveLength(1)
-      const explicit = childEnv({ Path: `${delimiter}/bin`, PathExt: '.EXE;.CMD' })
+      const explicit = childEnv({ Path: '/bin', PathExt: '.EXE;.CMD' })
       expect(Object.keys(explicit).filter(key => key.toUpperCase() === 'PATH')).toEqual(['Path'])
       expect(Object.keys(explicit).filter(key => key.toUpperCase() === 'PATH')).toEqual(['Path'])
       expect(Object.keys(explicit).filter(key => key.toUpperCase() === 'PATHEXT')).toEqual(['PathExt'])
       expect(Object.keys(explicit).filter(key => key.toUpperCase() === 'PATHEXT')).toEqual(['PathExt'])
       expect(candidates('tool', explicit)).toEqual(['/bin/tool.EXE', '/bin/tool.CMD'])
       expect(candidates('tool', explicit)).toEqual(['/bin/tool.EXE', '/bin/tool.CMD'])
       expect(candidates('tool', { Path: '/ambient', PATH: '/explicit', PATHEXT: '.EXE' }))
       expect(candidates('tool', { Path: '/ambient', PATH: '/explicit', PATHEXT: '.EXE' }))
         .toEqual(['/explicit/tool.EXE'])
         .toEqual(['/explicit/tool.EXE'])
-      expect(candidates('tool.exe', {})).toEqual([])
+      expect(candidates('tool.exe', {})).toEqual([resolve(process.cwd(), 'tool.exe')])
       expect(candidates('tool', { PATH: '/bin' })).toHaveLength(4)
       expect(candidates('tool', { PATH: '/bin' })).toHaveLength(4)
     } finally {
     } finally {
       platform.mockRestore()
       platform.mockRestore()
@@ -116,12 +119,12 @@ describe('LocalSubprocessService', () => {
     expect(terminals.size).toBe(0)
     expect(terminals.size).toBe(0)
   })
   })
 
 
-  it('retains an owned terminal when disposal cleanup rejects', async () => {
+  it('waits for every terminal cleanup and retains rejections', async () => {
     const ctx = new Context()
     const ctx = new Context()
     const fiber = await ctx.plugin(LocalSubprocessService)
     const fiber = await ctx.plugin(LocalSubprocessService)
     const service = ctx.subprocess
     const service = ctx.subprocess
     const runtimeRoot = service.runtimeRoot
     const runtimeRoot = service.runtimeRoot
-    const terminal: SubprocessTerminalHandle = {
+    const failedTerminal: SubprocessTerminalHandle = {
       pid: 1,
       pid: 1,
       output: new PassThrough(),
       output: new PassThrough(),
       done: Promise.resolve({ exitCode: 0, signal: null }),
       done: Promise.resolve({ exitCode: 0, signal: null }),
@@ -131,11 +134,26 @@ describe('LocalSubprocessService', () => {
       terminate: vi.fn(),
       terminate: vi.fn(),
       waitForExit: vi.fn(async () => { throw new Error('retryable cleanup failure') }),
       waitForExit: vi.fn(async () => { throw new Error('retryable cleanup failure') }),
     }
     }
+    let finishCleanup!: () => void
+    const cleanup = new Promise<boolean>((resolve) => {
+      finishCleanup = () => { resolve(true) }
+    })
+    const drainingTerminal: SubprocessTerminalHandle = {
+      ...failedTerminal,
+      terminate: vi.fn(),
+      waitForExit: vi.fn(() => cleanup),
+    }
     const terminals = (service as unknown as { terminals: Set<SubprocessTerminalHandle> }).terminals
     const terminals = (service as unknown as { terminals: Set<SubprocessTerminalHandle> }).terminals
-    terminals.add(terminal)
+    terminals.add(failedTerminal)
+    terminals.add(drainingTerminal)
 
 
-    await fiber.dispose()
-    expect(terminals).toEqual(new Set([terminal]))
+    let disposed = false
+    const disposing = fiber.dispose().then(() => { disposed = true })
+    await new Promise(resolve => setImmediate(resolve))
+    expect(disposed).toBe(false)
+    finishCleanup()
+    await disposing
+    expect(terminals).toEqual(new Set([failedTerminal]))
     expect((await stat(runtimeRoot)).isDirectory()).toBe(true)
     expect((await stat(runtimeRoot)).isDirectory()).toBe(true)
     await rm(runtimeRoot, { recursive: true, force: true })
     await rm(runtimeRoot, { recursive: true, force: true })
   })
   })