fix(sim): guard stale sidecar-child events, fail writes fast, fix boot/reclaim ownership races

Author: AprilNEA

packages/host/engine/src/__tests__/simulator-service.test.ts

@@ -126,6 +126,59 @@ describe('SimulatorService', () => {
     expect(backend.close).toHaveBeenCalledTimes(1);
   });
 
+  it('reclaims a service-booted device whose session stops mid-boot', async () => {
+    const backend = fakeBackend([device('A', 'Shutdown')]);
+    let resolveBoot!: () => void;
+    backend.boot.mockImplementation(
+      () =>
+        new Promise<void>((resolve) => {
+          resolveBoot = resolve;
+        }),
+    );
+    const service = new SimulatorService(backend, { idleReclaimMs: 1000 });
+
+    const booting = service.boot(S1, 'A');
+    // Let boot() claim the device and reach the (still-pending) backend.boot() call.
+    await vi.advanceTimersByTimeAsync(0);
+    // The owning session stops before the boot finishes, dropping the not-yet-service-booted claim.
+    service.releaseSession(S1);
+    resolveBoot();
+    await booting;
+
+    // The device we booted must not be left running untracked: it is re-tracked for reclaim, so it
+    // is not shut down immediately but is once the idle window elapses (without the fix there is no
+    // claim, so the booted device is never reclaimed).
+    expect(backend.shutdownDevice).not.toHaveBeenCalled();
+    await vi.advanceTimersByTimeAsync(1000);
+    expect(backend.shutdownDevice).toHaveBeenCalledWith('A');
+    expect(service.ownerOf('A')).toBeUndefined();
+  });
+
+  it('holds the claim until the reclaim shutdown settles', async () => {
+    const backend = fakeBackend([device('A', 'Shutdown')]);
+    let resolveShutdown!: () => void;
+    backend.shutdownDevice.mockImplementation(
+      () =>
+        new Promise<void>((resolve) => {
+          resolveShutdown = resolve;
+        }),
+    );
+    const service = new SimulatorService(backend, { idleReclaimMs: 1000 });
+
+    await service.boot(S1, 'A');
+    service.releaseSession(S1);
+    await vi.advanceTimersByTimeAsync(1000); // idle timer fires → shutdown in flight
+    expect(backend.shutdownDevice).toHaveBeenCalledWith('A');
+    // Another session must not grab the udid while its shutdown is still running.
+    await expect(service.openUrl(S2, 'A', 'https://example.com')).rejects.toMatchObject({
+      code: 'conflict',
+    });
+    // The device frees only once the shutdown settles.
+    resolveShutdown();
+    await vi.advanceTimersByTimeAsync(0);
+    expect(service.ownerOf('A')).toBeUndefined();
+  });
+
   it('frees a device on owner-driven shutdown', async () => {
     const backend = fakeBackend([device('A', 'Shutdown')]);
     const service = new SimulatorService(backend);

packages/host/engine/src/simulator/service.ts

@@ -58,7 +58,18 @@ export class SimulatorService {
     if (state === 'Booted') return;
     await this.backend.boot(udid);
     const claim = this.claims.get(udid);
-    if (claim?.sessionId === sessionId) claim.bootedByService = true;
+    if (claim === undefined) {
+      // The owning session stopped while the boot was in flight, so `releaseSession` dropped the
+      // not-yet-service-booted claim. We booted a device that now has no owner: re-track it as
+      // service-booted and arm idle reclaim, exactly as if the stop had arrived after the boot — a
+      // resume within the window re-claims it, otherwise it is shut down instead of left running.
+      const reclaimed: DeviceClaim = { sessionId, bootedByService: true };
+      this.claims.set(udid, reclaimed);
+      this.release(udid, reclaimed);
+    } else if (claim.sessionId === sessionId) {
+      claim.bootedByService = true;
+    }
+    // else: another session claimed the device during our boot — it is theirs to manage now.
   }
 
   /** Shut a device down on the owner's behalf and free it. */
@@ -159,9 +170,14 @@ export class SimulatorService {
     }
     if (claim.idleTimer) clearTimeout(claim.idleTimer);
     claim.idleTimer = setTimeout(() => {
-      this.drop(udid);
-      // Reclaim is best-effort: the device may already be gone (deleted in Xcode, host reboot).
-      this.backend.shutdownDevice(udid).catch(noop);
+      // Shut the device down before releasing its claim: dropping first opens a window where
+      // another session claims and boots the same udid while this shutdown is still in flight,
+      // which would then tear down the device that session just acquired. Reclaim stays best-effort
+      // (the device may already be gone — deleted in Xcode, host reboot).
+      void this.backend
+        .shutdownDevice(udid)
+        .catch(noop)
+        .finally(() => this.drop(udid));
     }, this.idleReclaimMs);
     claim.idleTimer.unref?.();
   }

packages/host/sim/src/client.ts

@@ -1,6 +1,7 @@
 import type { ChildProcessByStdio } from 'node:child_process';
 import { spawn } from 'node:child_process';
 import type { Readable, Writable } from 'node:stream';
+import { extractErrorMessage } from 'foxts/extract-error-message';
 import type { Frame } from './codec';
 import {
   decodeScreenshotFrame,
@@ -137,7 +138,15 @@ export class SimSidecarClient {
       // Don't let a pending reply keep the host's event loop alive on its own.
       timer.unref();
       this.pending.set(requestId, { resolve, reject, timer });
-      writeFrame(child.stdin, REQUEST, body);
+      try {
+        writeFrame(child.stdin, REQUEST, body);
+      } catch (error) {
+        // The child died between ensureChild() and the write: fail this request now instead of
+        // leaving it registered to wait out the full reply deadline. (Async write failures land on
+        // the stdin `error` listener below, which fails every pending request the same way.)
+        this.take(requestId);
+        reject(new Error(extractErrorMessage(error) ?? 'sim sidecar write failed'));
+      }
     });
   }
 
@@ -150,21 +159,28 @@ export class SimSidecarClient {
       windowsHide: true,
     });
     this.child = child;
+    // Every listener is scoped to THIS child: after a crash a new request spawns a replacement, and
+    // the old child's delayed `exit`/`error`/`data` must not tear down (or feed stale bytes into)
+    // the current one. `onChildGone` only fires while `child` is still the live child.
+    const teardown = (): void => {
+      if (this.child === child) this.onChildGone();
+    };
     child.stdout.on('data', (chunk: Buffer) => {
+      if (this.child !== child) return;
       try {
         for (const frame of this.decoder.feed(chunk)) this.handleFrame(frame);
       } catch {
         // A corrupt stream cannot be resynchronized mid-flight; drop the child and start over.
         child.kill();
-        this.onChildGone();
+        teardown();
       }
     });
     // A failed spawn (e.g. missing binary) errors the pipes; a broken pipe means the child is
     // gone. Without these listeners the unhandled stream error would crash the host process.
-    child.stdin.on('error', () => this.onChildGone());
-    child.stdout.on('error', () => this.onChildGone());
-    child.on('exit', () => this.onChildGone());
-    child.on('error', () => this.onChildGone());
+    child.stdin.on('error', teardown);
+    child.stdout.on('error', teardown);
+    child.on('exit', teardown);
+    child.on('error', teardown);
     return child;
   }