feat(relay): await owned watcher and agent children on shutdown (#16741 T2 P1)

src/main/ipc/parcel-watcher-child-termination.ts

@@ -10,6 +10,13 @@ export const WATCHER_PROCESS_HARD_KILL_DELAY_MS = 5_000
 export const WATCHER_PROCESS_EXIT_DEADLINE_MS = RUNTIME_FILE_WATCH_EXIT_DEADLINE_MS
 
 const physicalExitPromises = new WeakMap<ChildProcess, Promise<void>>()
+const signalledChildren = new WeakSet<ChildProcess>()
+
+/** Sends the graceful signal once; a later awaited termination only escalates and waits. */
+export function signalWatcherChild(child: ChildProcess): void {
+  signalledChildren.add(child)
+  child.kill()
+}
 
 export function registerWatcherChildPhysicalExit(child: ChildProcess): () => void {
   let resolveExit: () => void = () => undefined
@@ -86,6 +93,10 @@ export function terminateWatcherChild(child: ChildProcess): Promise<boolean> {
     hardKillTimer.unref?.()
     const exitDeadlineTimer = setTimeout(() => finish(false), WATCHER_PROCESS_EXIT_DEADLINE_MS)
     exitDeadlineTimer.unref?.()
+    if (signalledChildren.has(child)) {
+      return
+    }
+    signalledChildren.add(child)
     try {
       child.kill()
     } catch {
@@ -94,11 +105,10 @@ export function terminateWatcherChild(child: ChildProcess): Promise<boolean> {
   })
 }
 
-export function createWatcherChildTerminationFailure(child: ChildProcess): WatcherProcessFailure {
-  const physicalExit =
-    child.exitCode !== null || child.signalCode !== null
-      ? Promise.resolve()
-      : (physicalExitPromises.get(child) ??
+export function watcherChildPhysicalExit(child: ChildProcess): Promise<void> {
+  return child.exitCode !== null || child.signalCode !== null
+    ? Promise.resolve()
+    : (physicalExitPromises.get(child) ??
         new Promise<void>((resolve) => {
           const finish = (): void => {
             child.removeListener('exit', finish)
@@ -108,11 +118,14 @@ export function createWatcherChildTerminationFailure(child: ChildProcess): Watch
           child.once('exit', finish)
           child.once('close', finish)
         }))
+}
+
+export function createWatcherChildTerminationFailure(child: ChildProcess): WatcherProcessFailure {
   return new WatcherProcessFailure(
     'file watcher process did not exit after termination deadline',
     'supervisor',
     'process_unavailable',
-    physicalExit
+    watcherChildPhysicalExit(child)
   )
 }
 
@@ -127,10 +140,12 @@ export async function terminateIdleWatcherChild(
   pendingUnsubscribes: Map<number, PendingWatcherUnsubscribe>,
   onFinished: (exited: boolean) => void
 ): Promise<void> {
+  // Windows directory handles require physical exit, not merely an accepted signal.
   try {
     await requireWatcherChildTermination(child)
     onFinished(true)
   } catch (error) {
+    // Idle children retain capacity but cannot double-watch; the owner may remain reusable.
     onFinished(false)
     resolvePendingWatcherUnsubscribes(
       pendingUnsubscribes,

src/main/ipc/parcel-watcher-entry-path.ts

@@ -4,6 +4,14 @@ import { join } from 'node:path'
 
 type ElectronAppPath = { getAppPath(): string; isPackaged(): boolean }
 
+export function watcherProcessEntryExists(entryPath: string): boolean {
+  if (existsSync(entryPath)) {
+    return true
+  }
+  console.error(`[parcel-watcher-process] entry not found at ${entryPath}; refusing fail-open`)
+  return false
+}
+
 // Why the port and not require('electron'): this module is reachable from plain-Node
 // fork entries, where the literal text require("electron") fails the build guard even
 // inside a try/catch. hasAppEnvironment() gives the same "no app root here" answer.

src/main/ipc/parcel-watcher-owned-children.test.ts

@@ -0,0 +1,120 @@
+import { EventEmitter } from 'node:events'
+import type { ChildProcess } from 'node:child_process'
+import { afterEach, expect, it, vi } from 'vitest'
+import { WatcherOwnedChildren } from './parcel-watcher-owned-children'
+import {
+  registerWatcherChildPhysicalExit,
+  signalWatcherChild,
+  WATCHER_PROCESS_EXIT_DEADLINE_MS,
+  WATCHER_PROCESS_HARD_KILL_DELAY_MS
+} from './parcel-watcher-child-termination'
+
+afterEach(() => vi.useRealTimers())
+
+function child() {
+  const exitState: { exitCode: number | null; signalCode: NodeJS.Signals | null } = {
+    exitCode: null,
+    signalCode: null
+  }
+  const events = Object.assign(new EventEmitter(), exitState, { kill: vi.fn(() => true) })
+  // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: termination reads only the exit fields, kill and events stubbed here.
+  const process = events as unknown as ChildProcess
+  const physicalExit = registerWatcherChildPhysicalExit(process)
+  events.on('exit', physicalExit)
+  events.on('close', physicalExit)
+  events.on('error', () => {})
+  return { process, events, close: () => events.emit('close') }
+}
+
+it('joins disposal and does not confuse disconnect/error with physical exit', async () => {
+  const owner = new WatcherOwnedChildren()
+  const c = child()
+  owner.track(c.process)
+  const logical = vi.fn()
+  const disposed = vi.fn()
+  const first = owner.disposeAndWait(logical)
+  expect(owner.disposeAndWait(logical)).toBe(first)
+  const result = first.then(disposed)
+  c.events.emit('disconnect')
+  c.events.emit('error', new Error('spawn or IPC failure'))
+  await Promise.resolve()
+  expect(disposed).not.toHaveBeenCalled()
+  expect(logical).toHaveBeenCalledOnce()
+  c.close()
+  await result
+  expect(disposed).toHaveBeenCalledOnce()
+})
+
+it('includes children already signaled by synchronous or retired-owner disposal', async () => {
+  const owner = new WatcherOwnedChildren()
+  const old = child()
+  const replacement = child()
+  owner.track(old.process)
+  old.process.kill()
+  owner.track(replacement.process)
+  const disposed = vi.fn()
+  const result = owner.disposeAndWait(() => {}).then(disposed)
+  replacement.close()
+  await Promise.resolve()
+  expect(disposed).not.toHaveBeenCalled()
+  old.close()
+  await result
+})
+
+it('retains a child after termination deadline failure and supports explicit retry', async () => {
+  vi.useFakeTimers()
+  const owner = new WatcherOwnedChildren()
+  const c = child()
+  owner.track(c.process)
+  const result = owner.disposeAndWait(() => {}).catch((error: unknown) => error)
+  await vi.advanceTimersByTimeAsync(WATCHER_PROCESS_EXIT_DEADLINE_MS)
+  expect(await result).toMatchObject({ message: 'watcher_owned_children_shutdown_incomplete' })
+  const completed = vi.fn()
+  const retry = owner.disposeAndWait(() => {}).then(completed)
+  await Promise.resolve()
+  expect(completed).not.toHaveBeenCalled()
+  c.close()
+  await retry
+})
+
+it('waits for other children even when logical disposal and one kill fail', async () => {
+  const owner = new WatcherOwnedChildren()
+  const failed = child()
+  const pending = child()
+  owner.track(failed.process)
+  owner.track(pending.process)
+  failed.events.kill.mockImplementationOnce(() => {
+    throw new Error('kill failed')
+  })
+  const logicalFailure = new Error('logical cleanup failed')
+  const finished = vi.fn()
+  const result = owner
+    .disposeAndWait(() => {
+      throw logicalFailure
+    })
+    .catch((error: unknown) => {
+      finished()
+      return error
+    })
+  await Promise.resolve()
+  expect(finished).not.toHaveBeenCalled()
+  pending.close()
+  expect(await result).toMatchObject({ errors: [logicalFailure, expect.any(Error)] })
+  const retry = owner.disposeAndWait(() => {})
+  failed.close()
+  await retry
+})
+
+it('skips a second graceful signal after the supervisor sent one but still escalates', async () => {
+  vi.useFakeTimers()
+  const owner = new WatcherOwnedChildren()
+  const c = child()
+  owner.track(c.process)
+  const disposal = owner.disposeAndWait(() => signalWatcherChild(c.process))
+  expect(c.events.kill).toHaveBeenCalledTimes(1)
+  expect(c.events.kill).toHaveBeenCalledWith()
+  await vi.advanceTimersByTimeAsync(WATCHER_PROCESS_HARD_KILL_DELAY_MS)
+  expect(c.events.kill).toHaveBeenLastCalledWith('SIGKILL')
+  c.events.emit('exit', null, 'SIGKILL')
+  await expect(disposal).resolves.toBeUndefined()
+})

src/main/ipc/parcel-watcher-owned-children.ts

@@ -0,0 +1,47 @@
+import type { ChildProcessHandle } from '../../shared/child-process/process-spec'
+import {
+  watcherChildPhysicalExit,
+  requireWatcherChildTermination
+} from './parcel-watcher-child-termination'
+
+export class WatcherOwnedChildren {
+  private readonly children = new Set<ChildProcessHandle>()
+  private disposal: Promise<void> | null = null
+
+  track(child: ChildProcessHandle): ChildProcessHandle {
+    this.children.add(child)
+    // Reuse launch-owned physical-exit evidence, including close without exit after spawn failure.
+    void watcherChildPhysicalExit(child).then(() => {
+      this.children.delete(child)
+    })
+    return child
+  }
+
+  disposeAndWait(disposeLogicalOwner: () => void): Promise<void> {
+    if (this.disposal) {
+      return this.disposal
+    }
+    const failures: unknown[] = []
+    try {
+      disposeLogicalOwner()
+    } catch (error) {
+      failures.push(error)
+    }
+    const cleanup = Promise.allSettled([...this.children].map(requireWatcherChildTermination))
+      .then((results) => {
+        for (const result of results) {
+          if (result.status === 'rejected') {
+            failures.push(result.reason)
+          }
+        }
+        if (failures.length > 0) {
+          throw new AggregateError(failures, 'watcher_owned_children_shutdown_incomplete')
+        }
+      })
+      .finally(() => {
+        this.disposal = null
+      })
+    this.disposal = cleanup
+    return cleanup
+  }
+}

src/main/ipc/parcel-watcher-process-entry.test.ts

@@ -86,8 +86,13 @@ describe('parcel watcher process canary', () => {
     await vi.advanceTimersByTimeAsync(0)
 
     expect(sendMock).toHaveBeenCalledWith(
-      expect.objectContaining({ op: 'subscribe-failed', id: 7 })
+      expect.objectContaining({ op: 'subscribe-failed', id: 7 }),
+      expect.any(Function)
     )
+    const failedSend = sendMock.mock.calls.find(([message]) => message.op === 'subscribe-failed')
+    expect(() =>
+      failedSend![1](Object.assign(new Error('host disconnected'), { code: 'EPIPE' }))
+    ).not.toThrow()
     expect(watchMock).not.toHaveBeenCalledWith('/repo/.git', expect.anything(), expect.anything())
   })
 
@@ -313,10 +318,13 @@ describe('parcel watcher process canary', () => {
     finishActiveCrawl?.({ unsubscribe: vi.fn().mockResolvedValue(undefined) })
     await vi.advanceTimersByTimeAsync(0)
 
-    expect(sendMock).toHaveBeenCalledWith({ op: 'unsubscribed', id: 2 })
+    expect(sendMock).toHaveBeenCalledWith({ op: 'unsubscribed', id: 2 }, expect.any(Function))
     expect(subscribeMock).toHaveBeenCalledTimes(2)
-    expect(sendMock).not.toHaveBeenCalledWith({ op: 'subscribe-started', id: 2 })
-    expect(sendMock).not.toHaveBeenCalledWith({ op: 'subscribed', id: 2 })
+    expect(sendMock).not.toHaveBeenCalledWith(
+      { op: 'subscribe-started', id: 2 },
+      expect.any(Function)
+    )
+    expect(sendMock).not.toHaveBeenCalledWith({ op: 'subscribed', id: 2 }, expect.any(Function))
   })
 
   it('unsubscribes a late cancel after the crawl already finished', async () => {
@@ -332,13 +340,16 @@ describe('parcel watcher process canary', () => {
     process.emit('message', { op: 'subscribe', id: 1, dir: '/finished', opts: {} })
     await vi.advanceTimersByTimeAsync(0)
 
-    expect(sendMock).toHaveBeenCalledWith({ op: 'subscribed', id: 1 })
+    expect(sendMock).toHaveBeenCalledWith({ op: 'subscribed', id: 1 }, expect.any(Function))
     process.emit('message', { op: 'cancel-subscribe', id: 1 })
     await vi.advanceTimersByTimeAsync(0)
 
     expect(unsubscribe).toHaveBeenCalledTimes(1)
-    expect(sendMock).toHaveBeenCalledWith({ op: 'unsubscribed', id: 1 })
-    expect(sendMock).not.toHaveBeenCalledWith({ op: 'cancel-requires-restart', id: 1 })
+    expect(sendMock).toHaveBeenCalledWith({ op: 'unsubscribed', id: 1 }, expect.any(Function))
+    expect(sendMock).not.toHaveBeenCalledWith(
+      { op: 'cancel-requires-restart', id: 1 },
+      expect.any(Function)
+    )
   })
 
   it('reports native unsubscribe rejection without acknowledging handle release', async () => {
@@ -356,12 +367,15 @@ describe('parcel watcher process canary', () => {
     process.emit('message', { op: 'unsubscribe', id: 1 })
     await vi.advanceTimersByTimeAsync(0)
 
-    expect(sendMock).toHaveBeenCalledWith({
-      op: 'unsubscribe-failed',
-      id: 1,
-      message: 'native handle still active'
-    })
-    expect(sendMock).not.toHaveBeenCalledWith({ op: 'unsubscribed', id: 1 })
+    expect(sendMock).toHaveBeenCalledWith(
+      {
+        op: 'unsubscribe-failed',
+        id: 1,
+        message: 'native handle still active'
+      },
+      expect.any(Function)
+    )
+    expect(sendMock).not.toHaveBeenCalledWith({ op: 'unsubscribed', id: 1 }, expect.any(Function))
   })
 
   it('asks the host to restart when an active crawl is cancelled', async () => {
@@ -380,10 +394,13 @@ describe('parcel watcher process canary', () => {
     await vi.advanceTimersByTimeAsync(0)
     process.emit('message', { op: 'cancel-subscribe', id: 1 })
 
-    expect(sendMock).toHaveBeenCalledWith({ op: 'cancel-requires-restart', id: 1 })
+    expect(sendMock).toHaveBeenCalledWith(
+      { op: 'cancel-requires-restart', id: 1 },
+      expect.any(Function)
+    )
     finishCrawl?.({ unsubscribe: vi.fn().mockResolvedValue(undefined) })
     await vi.advanceTimersByTimeAsync(0)
-    expect(sendMock).not.toHaveBeenCalledWith({ op: 'subscribed', id: 1 })
+    expect(sendMock).not.toHaveBeenCalledWith({ op: 'subscribed', id: 1 }, expect.any(Function))
   })
 
   it('still restarts after consecutive missed events once every subscription is live', async () => {
@@ -481,11 +498,14 @@ describe('parcel watcher process canary', () => {
     callback?.(null, [{ type: 'update', path: '/repo/after-overflow.txt' }])
     await vi.advanceTimersByTimeAsync(0)
 
-    expect(sendMock).toHaveBeenCalledWith({
-      op: 'watch-error',
-      id: 1,
-      message: 'Events were dropped by the FSEvents client. File system must be re-scanned.'
-    })
+    expect(sendMock).toHaveBeenCalledWith(
+      {
+        op: 'watch-error',
+        id: 1,
+        message: 'Events were dropped by the FSEvents client. File system must be re-scanned.'
+      },
+      expect.any(Function)
+    )
     expect(sendMock).toHaveBeenCalledWith(
       {
         op: 'events',

src/main/ipc/parcel-watcher-process-entry.ts

@@ -118,7 +118,7 @@ async function startCanary(getStableActivityRevision: () => number | null): Prom
 function main(): void {
   const send = (message: WatcherToHostMessage): void => {
     try {
-      process.send?.(message)
+      process.send?.(message, () => undefined)
     } catch {
       // Host is gone; the disconnect handler below exits this process.
     }