[agent-cli]: detached readiness waits for this project's Metro

packages/@expo/agent-cli/CHANGELOG.md

@@ -21,6 +21,7 @@
 
 ### 🐛 Bug fixes
 
+- `dev --detach --wait-ready` rejects another project's Metro and follows its own child's port changes within the original timeout. The same readiness check applies when reusing a running server.
 - Discovery skips a dev server whose `/status` names another project root, on the scanned ports and on the port `start.log` names, and reports the foreign servers in `status` when nothing else answered. A `/status` that does not answer within 3 s is not accepted. The dev-server lock never publishes a port that serves another project, named on the command line or not. ([#132](https://github.com/expo/expo-agent-cli/pull/132) by [@vonovak](https://github.com/vonovak))
 - `status --explain`, the `--eas` device commands, and a failed `eas` step of `dev` no longer quote the first line of the EAS CLI's "project not configured" message with the `eas init` forms cut off. They say the project is not linked to an EAS project and name `npx --yes eas-cli@latest init --account <account> --non-interactive`, with the account filled in when EAS listed one — the answer `deploy` already gave. An unlinked project is its own needs-human scenario, `eas-project-unlinked` (`EAS_PROJECT_NOT_LINKED`), where the generic `eas-prompt` row used to answer for it and tell the caller to answer a question in a terminal. ([#38](https://github.com/expo/expo-agent-cli/pull/38) by [@kudo](https://github.com/kudo))
 - `dev --eas` no longer builds the `development` profile, which is a device build no simulator can install — not the EAS Simulator, and not the one on this Mac, so the `eas build:run` the old plan suggested could not have installed it either. ([#36](https://github.com/expo/expo-agent-cli/pull/36) by [@kudo](https://github.com/kudo))

packages/@expo/agent-cli/e2e/__tests__/dev-detach-test.ts

@@ -9,15 +9,18 @@
 // The dev server is the stub `expo` bin, which holds the lock exactly as the real one does — the
 // wrapper is what takes the lock, and the wrapper is real here.
 import fs from 'node:fs';
+import http from 'node:http';
 import net from 'node:net';
 import path from 'node:path';
 
 import {
   executeAgentCliAsync,
+  holdDevLockAsync,
   installStubFingerprintAsync,
   readDevLockAsync,
   readStubExpoInvocations,
   setupFixtureAsync,
+  startStubDevServerAsync,
   stubExpoEnv,
   waitForAsync,
 } from '../utils';
@@ -65,6 +68,71 @@ async function cleanUpAsync(projectRoot: string): Promise<void> {
 }
 
 describe('@expo/agent-cli dev --detach', () => {
+  it.each(['foreign', 'hanging'] as const)(
+    'follows its lock away from a %s status endpoint before reporting ready',
+    async (answer) => {
+      const projectRoot = await setupFixtureAsync('go-app');
+      const own = await startStubDevServerAsync({ projectRoot, statusDelayMs: 200 });
+      let moveTimer: ReturnType<typeof setTimeout> | undefined;
+      let oldRequestClosed = false;
+      const info = {
+        url: '',
+        port: 0,
+        pid: process.pid,
+        projectRoot,
+        startedAt: new Date().toISOString(),
+      };
+      const previous = http.createServer((request, response) => {
+        if (request.url !== '/status') {
+          response.writeHead(404).end();
+          return;
+        }
+        response.on('close', () => {
+          oldRequestClosed = true;
+        });
+        // Move only after the CLI has actually asked the old port, avoiding a startup-time race
+        // in the test itself. The lock payload is read anew for each connection.
+        moveTimer ??= setTimeout(() => Object.assign(info, { url: own.url, port: own.port }), 500);
+        if (answer === 'foreign') {
+          response.writeHead(200, {
+            'X-React-Native-Project-Root': encodeURI(path.join(projectRoot, '..', 'sibling')),
+          });
+          response.end('packager-status:running');
+        }
+      });
+      await new Promise<void>((resolve) => previous.listen(0, '127.0.0.1', resolve));
+      info.port = (previous.address() as net.AddressInfo).port;
+      info.url = `http://127.0.0.1:${info.port}`;
+      const release = await holdDevLockAsync(projectRoot, info);
+      try {
+        const result = await executeAgentCliAsync(
+          projectRoot,
+          ['dev', '--web', '--detach', '--wait-ready', '--json'],
+          { env: stubExpoEnv(projectRoot), reject: false }
+        );
+        expect(result.exitCode, result.all).toBe(0);
+        expect(JSON.parse(result.stdout)).toMatchObject({
+          url: own.url,
+          port: own.port,
+          pid: process.pid,
+          alreadyRunning: true,
+          ready: true,
+          projectRootMatched: true,
+        });
+        expect(oldRequestClosed).toBe(true);
+        expect(readStubExpoInvocations(projectRoot)).toEqual([]);
+        // The CLI must neither stop nor reconfigure the foreign server while waiting.
+        expect(previous.listening).toBe(true);
+      } finally {
+        clearTimeout(moveTimer);
+        release();
+        previous.closeAllConnections();
+        await new Promise<void>((resolve) => previous.close(() => resolve()));
+        await own.close();
+      }
+    }
+  );
+
   // The whole finding, in one assertion: the command returns, and a dev server is running.
   it('returns while the dev server it started keeps running', async () => {
     const projectRoot = await setupFixtureAsync('go-app');

packages/@expo/agent-cli/llp/0004-smart-start-and-project-state.rfc.md

@@ -454,14 +454,23 @@ is the output the log file exists to hold. `buildDetachSpawn` is a pure function
 argv for that reason.
 
 The parent waits for the lock, not for the child. A detached start is finished when the
-lock answers. Under `--wait-ready` it then holds one `/status` request open, so `ready`
-is `true` or the wait failed. Without the flag `ready` is `null`.
+lock answers. Under `--wait-ready` it holds one `/status` request open while following
+that child's lock. A port change cancels the old request and checks the new URL within
+the remaining budget. A ready response naming another project never completes this wait;
+the parent keeps waiting for its own server. The missing-header compatibility rule in
+§A busy port still applies, with `projectRootMatched: null`. Final readiness checks also
+reject a foreign root. Without the flag `ready` is `null`.
 
 One detached dev server per project. The lock is read before anything is spawned. A
 project that already has one gets the running server reported back with
-`alreadyRunning: true` and exit 0. Two foreground servers in two terminals are a thing
+`alreadyRunning: true`; `--wait-ready` performs the same readiness check before exit 0,
+without spawning another child. Two foreground servers in two terminals are a thing
 people do on purpose. A second detached one is a process nobody could find.
 
+Reuse checks the process and its verdict after tunnel lookup and any final readiness
+probe, and before reporting a readiness timeout. Logs older than the lock's start time
+cannot supply a current verdict. Reuse adds no platform-opening grace period.
+
 The log is one file per project, truncated per run: `.expo/dev/logs/dev-detached.log`. A
 name carrying the port could not be resolved by `dev:logs` before the port was known. A
 file that accumulated across runs would answer with last week's output.

packages/@expo/agent-cli/src/dev/__tests__/detachAsync-test.ts

@@ -5,7 +5,9 @@ import { vol } from 'memfs';
 import path from 'path';
 
 import { readDevServerLockAsync } from '../../devLock';
+import { event as cliEvent } from '../../events';
 import { waitForBundlerReadyAsync, type BundlerReadyResult } from '../../runtime/waitReady';
+import { fetchAdvertisedUrlAsync } from '../advertisedUrl';
 import { devDetachAsync, OPEN_PLATFORM_GRACE_MS } from '../detachAsync';
 import { detachedLogPath } from '../logFile';
 import { isProcessAlive } from '../processLiveness';
@@ -16,6 +18,10 @@ vi.mock('../../runtime/waitReady', () => ({ waitForBundlerReadyAsync: vi.fn() })
 vi.mock('../processLiveness', () => ({ isProcessAlive: vi.fn() }));
 vi.mock('../events', () => ({ event: vi.fn() }));
 vi.mock('../../events', () => ({ event: vi.fn() }));
+vi.mock('../advertisedUrl', async (importOriginal) => ({
+  ...(await importOriginal<typeof import('../advertisedUrl')>()),
+  fetchAdvertisedUrlAsync: vi.fn(),
+}));
 
 const projectRoot = '/project';
 const logFile = detachedLogPath(projectRoot);
@@ -141,6 +147,122 @@ describe.each(['final check', 'grace'] as const)(
   }
 );
 
+describe('reusing a running server', () => {
+  beforeEach(() => {
+    vi.mocked(readDevServerLockAsync)
+      .mockReset()
+      .mockResolvedValue({
+        url: 'http://127.0.0.1:8393',
+        port: 8393,
+        pid: 4242,
+        projectRoot,
+        startedAt: new Date(0).toISOString(),
+      });
+    fs.mkdirSync(path.dirname(logFile), { recursive: true });
+    fs.writeFileSync(logFile, servingPlan);
+    vi.mocked(fetchAdvertisedUrlAsync).mockResolvedValue(null);
+  });
+
+  afterEach(() => expect(spawn).not.toHaveBeenCalled());
+
+  function reuse(waitReady = true) {
+    return devDetachAsync(
+      projectRoot,
+      { ...resolveDevOptions(['--ios', '--detach', '--local', '--json']), waitReady },
+      { print: false }
+    );
+  }
+
+  async function failure(waitReady = true) {
+    const result = reuse(waitReady).catch((error) => error);
+    await vi.runAllTimersAsync();
+    expect(cliEvent).not.toHaveBeenCalledWith('dev_detach', expect.anything());
+    return result;
+  }
+
+  it.each([true, false])('detects death during tunnel lookup (waitReady=%s)', async (waitReady) => {
+    vi.mocked(fetchAdvertisedUrlAsync).mockImplementation(async () => {
+      await new Promise((resolve) => setTimeout(resolve, 100));
+      vi.mocked(isProcessAlive).mockReturnValue(false);
+      return null;
+    });
+
+    expect(await failure(waitReady)).toMatchObject({ code: 'DEV_DETACH_DIED', exitCode: 20 });
+    expect(Date.now()).toBe(2100); // Tunnel lookup plus one bounded verdict wait.
+    if (!waitReady) expect(waitForBundlerReadyAsync).not.toHaveBeenCalled();
+  });
+
+  it.each([100, 300])('relays a readiness failure whose handoff arrives at %sms', async (delay) => {
+    fs.utimesSync(logFile, new Date(-1000), new Date(-1000));
+    vi.mocked(waitForBundlerReadyAsync).mockImplementation(async () => {
+      await new Promise((resolve) => setTimeout(resolve, 200));
+      return readiness(false);
+    });
+    setTimeout(() => {
+      vi.mocked(isProcessAlive).mockReturnValue(false);
+      fs.appendFileSync(logFile, 'NeedsHumanError: macOS refused Automation permission\n');
+    }, 50);
+    setTimeout(() => fs.appendFileSync(logFile, 'Needs a human   macos-automation\n'), delay);
+
+    expect(await failure()).toMatchObject({
+      code: 'DEV_DETACH_NEEDS_HUMAN',
+      exitCode: 7,
+      needsHuman: { scenario: 'macos-automation', detectedBy: 'detached-child-log' },
+    });
+    expect(Date.now()).toBe(Math.max(200, delay));
+  });
+
+  it('reads the handoff after the final status probe', async () => {
+    vi.mocked(waitForBundlerReadyAsync)
+      .mockResolvedValueOnce(readiness())
+      .mockImplementationOnce(async () => {
+        await new Promise((resolve) => setTimeout(resolve, 100));
+        fs.appendFileSync(logFile, 'Needs a human   macos-automation\n');
+        return readiness(false);
+      });
+
+    expect(await failure()).toMatchObject({ code: 'DEV_DETACH_NEEDS_HUMAN', exitCode: 7 });
+  });
+
+  it.each(['silent', 'foreign'])('rejects a %s final status response', async (response) => {
+    vi.mocked(waitForBundlerReadyAsync)
+      .mockResolvedValueOnce(readiness())
+      .mockResolvedValue({
+        ...readiness(response !== 'silent'),
+        projectRootMatched: response !== 'foreign',
+      });
+
+    expect(await failure()).toMatchObject({ code: 'DEV_DETACH_NOT_ANSWERING', exitCode: 20 });
+    expect(Date.now()).toBe(0);
+  });
+
+  it.each([
+    [true, 'old'],
+    [false, 'old'],
+    [true, 'missing'],
+    [false, 'missing'],
+    [true, 'current'],
+  ] as const)('reports a healthy server (waitReady=%s, log=%s)', async (waitReady, log) => {
+    if (log === 'old') {
+      fs.appendFileSync(logFile, 'Needs a human   macos-automation\n');
+      fs.utimesSync(logFile, new Date(-1000), new Date(-1000));
+    } else if (log === 'missing') {
+      fs.unlinkSync(logFile);
+    }
+
+    const result = expect(reuse(waitReady)).resolves.toBe(0);
+    await vi.runAllTimersAsync();
+
+    await result;
+    expect(cliEvent).toHaveBeenCalledWith(
+      'dev_detach',
+      expect.objectContaining({ alreadyRunning: true, pid: 4242, ready: waitReady ? true : null })
+    );
+    expect(waitForBundlerReadyAsync).toHaveBeenCalledTimes(waitReady ? 2 : 0);
+    expect(Date.now()).toBe(0); // Reuse does not acquire the startup grace period.
+  });
+});
+
 it('keeps the same grace window for a child that stays healthy', async () => {
   const result = run();
   await vi.runAllTimersAsync();
@@ -183,6 +305,128 @@ it('reports the port Metro logged, not the one the lock named before a retry', a
   expect(cliEvent).toHaveBeenCalledWith('dev_detach', expect.objectContaining({ port: 8393 }));
 });
 
+describe('readiness belongs to this worktree', () => {
+  const initialLock = {
+    url: 'http://127.0.0.1:8393',
+    port: 8393,
+    pid: 4242,
+    projectRoot,
+    startedAt: new Date(0).toISOString(),
+  };
+  const movedLock = { ...initialLock, url: 'http://127.0.0.1:8394', port: 8394 };
+  const foreign = {
+    ...readiness(),
+    projectRootMatched: false,
+    reportedProjectRoot: '/sibling',
+  };
+
+  function start() {
+    return devDetachAsync(
+      projectRoot,
+      {
+        ...resolveDevOptions(['--ios', '--detach', '--wait-ready', '--local', '--json']),
+        detachTimeoutMs: 5000,
+      },
+      { print: false }
+    );
+  }
+
+  it('waits past a foreign ready server and reports the port the child moves to', async () => {
+    const { event: cliEvent } = await import('../../events');
+    vi.mocked(waitForBundlerReadyAsync).mockImplementation(async (url) =>
+      url === initialLock.url ? foreign : readiness()
+    );
+    setTimeout(() => {
+      vi.mocked(readDevServerLockAsync).mockResolvedValue(movedLock);
+    }, 1000);
+
+    const result = start();
+    await vi.advanceTimersByTimeAsync(500);
+    expect(cliEvent).not.toHaveBeenCalledWith('dev_detach', expect.anything());
+    await vi.runAllTimersAsync();
+
+    await expect(result).resolves.toBe(0);
+    expect(cliEvent).toHaveBeenCalledWith(
+      'dev_detach',
+      expect.objectContaining({ port: 8394, url: movedLock.url, ready: true })
+    );
+  });
+
+  it('cancels a pending status request when the lock moves', async () => {
+    let oldSignal: AbortSignal | undefined;
+    vi.mocked(waitForBundlerReadyAsync).mockImplementation(async (url, { signal }) => {
+      if (url !== initialLock.url) return readiness();
+      oldSignal = signal;
+      await new Promise<void>((resolve) => signal!.addEventListener('abort', () => resolve()));
+      return readiness(false);
+    });
+    setTimeout(() => {
+      vi.mocked(readDevServerLockAsync).mockResolvedValue(movedLock);
+    }, 1000);
+
+    const result = start();
+    await vi.advanceTimersByTimeAsync(3000);
+
+    expect(oldSignal?.aborted).toBe(true);
+    await expect(result).resolves.toBe(0);
+    expect(waitForBundlerReadyAsync).toHaveBeenCalledWith(
+      movedLock.url,
+      expect.objectContaining({ timeoutMs: expect.any(Number), projectRoot })
+    );
+  });
+
+  it('fails within the original budget when only a foreign server answers', async () => {
+    vi.mocked(waitForBundlerReadyAsync).mockResolvedValue(foreign);
+
+    const result = start().catch((error) => error);
+    await vi.runAllTimersAsync();
+
+    expect(await result).toMatchObject({ code: 'DEV_DETACH_NOT_READY' });
+    expect((await result).message).toContain('/sibling');
+    expect(Date.now()).toBe(5000);
+  });
+
+  it('checks readiness even when reusing an existing server', async () => {
+    vi.mocked(readDevServerLockAsync).mockReset().mockResolvedValue(initialLock);
+    vi.mocked(waitForBundlerReadyAsync).mockResolvedValue(foreign);
+
+    const result = start().catch((error) => error);
+    await vi.runAllTimersAsync();
+
+    expect(await result).toMatchObject({ code: 'DEV_DETACH_NOT_READY' });
+    expect(spawn).not.toHaveBeenCalled();
+  });
+
+  it("relays the child's failure instead of waiting on a foreign server until timeout", async () => {
+    vi.mocked(waitForBundlerReadyAsync).mockResolvedValue(foreign);
+    setTimeout(() => {
+      fs.appendFileSync(logFile, 'Needs a human   macos-automation\n');
+      vi.mocked(isProcessAlive).mockReturnValue(false);
+    }, 1000);
+
+    const result = start().catch((error) => error);
+    await vi.runAllTimersAsync();
+
+    expect(await result).toMatchObject({ code: 'DEV_DETACH_NEEDS_HUMAN', exitCode: 7 });
+    expect(Date.now()).toBe(1000);
+  });
+
+  it.each(['final check', 'grace'])(
+    'rejects a foreign server replacing ours during %s',
+    async (check) => {
+      const probe = vi.mocked(waitForBundlerReadyAsync);
+      probe.mockResolvedValueOnce(readiness());
+      if (check === 'grace') probe.mockResolvedValueOnce(readiness());
+      probe.mockResolvedValue(foreign);
+
+      const result = run().catch((error) => error);
+      await vi.runAllTimersAsync();
+
+      expect(await result).toMatchObject({ code: 'DEV_DETACH_NOT_ANSWERING' });
+    }
+  );
+});
+
 // A `run:*` build has not reached Metro yet, so `start.log` names no port. The lock's own
 // `startedAt` (the step's spawn) bounds the wait, as the port watch is bounded.
 describe('a lock whose port Metro has not logged', () => {