fix(daemon): make request-timeout cleanup and hints platform-aware (#1751)

* fix(daemon): make request-timeout cleanup and hints platform-aware

Every local request timeout ran the Apple-runner xcodebuild pkill cleanup
and emitted Apple-specific hint wording ("Apple runner work was aborted",
"Timed-out Apple runner xcodebuild processes were terminated") regardless
of the session's actual platform, which was misleading on Android/web/
Harmony timeouts.

The client already carries the request's declared --platform on the exact
DaemonRequest object both timeout call sites hold (req.flags?.platform), so
no new plumbing or src/platforms import is needed. When the platform is
known non-Apple, the Apple pkill cleanup is skipped and the hint drops its
Apple-runner claim; when it's unknown, both stay on their historical
fail-safe behavior.

Part of #1739 (independent cleanups)

* fix(daemon): split timeout cleanup eligibility from hint evidence

Maintainer review on #1751 found a real correctness inversion in the
first pass: gating the Apple xcodebuild pkill cleanup on the request's
declared --platform flag is wrong in both directions.

- req.flags.platform is not authoritative for session-bound execution.
  applyStripLockPolicy (request-lock-policy.ts) lets an existing
  session's real device platform silently override a conflicting
  declared selector under --session-lock strip, so a request declaring
  a non-Apple platform can still legitimately execute Apple work.
  Skipping cleanup on that declared flag would skip real cleanup —
  the dangerous direction.
- The common session-bound request omits --platform entirely, so the
  previous fail-safe (undeclared -> treat as Apple) left the hint
  wrongly claiming Apple involvement on the motivating case
  (Android/web/Harmony session timeouts) too.

Redesign: separate the two decisions.

- Cleanup eligibility is unconditional again for every local timeout,
  matching the original pre-fix behavior: the pkill patterns are
  Apple-process-name-specific, so sweeping them on a non-Apple host or
  session matches nothing and costs a few no-op subprocess spawns,
  never a wrong skip. There is no client-visible signal that proves a
  session-bound request cannot touch an Apple runner, so eligibility
  does not try to prove one.
- The hint may only name Apple-runner involvement on evidence this call
  site actually has: an explicitly declared Apple platform selector, or
  the cleanup itself having terminated a matching process
  (appleCleanupEvidence). Anything else gets platform-neutral wording.

Adds production-seam route tests
(src/daemon/client/__tests__/daemon-client-timeout-route.test.ts) that
spy on the real exec seam and drive actual socket/HTTP timeouts through
sendRequest, covering the rebound-session and unknown-session cases a
pure formatter test cannot catch. Proved red against the prior commit
(82aa5ad) via a scoped git stash before restoring the fix.

Part of #1739 (independent cleanups)

Author: thymikee

src/daemon/client/__tests__/daemon-client-timeout-route.test.ts

@@ -0,0 +1,241 @@
+// Production-seam coverage for the real request-timeout route.
+//
+// src/utils/__tests__/daemon-client.test.ts covers `resolveRequestTimeoutHint`
+// as a pure formatter, but a pure-formatter test cannot catch a bug in
+// CLEANUP ELIGIBILITY: whether `cleanupTimedOutIosRunnerBuilds` (the Apple
+// xcodebuild pkill sweep) actually runs. This file spies on the real
+// process-execution seam (`runCmdSync`, src/utils/exec.ts) and drives an
+// actual socket/HTTP timeout through `sendRequest` so the assertions exercise
+// the same code path a real client does.
+//
+// Why cleanup eligibility must stay unconditional for local timeouts: the
+// client's declared --platform is not authoritative for session-bound
+// execution. `applyStripLockPolicy` (src/daemon/request-lock-policy.ts) lets
+// an existing session's real device platform silently override a conflicting
+// declared selector under --session-lock strip, and the common session-bound
+// request omits --platform entirely. So a request declaring `platform:
+// 'android'` can still legitimately execute against an Apple-bound session
+// (the "rebound-session" case below), and a request with no platform at all
+// (the "unknown-session" case) is the common route the original bug misled.
+// A design that skips the pkill sweep based on the declared flag alone would
+// skip real cleanup in the rebound case — the dangerous direction. This test
+// proves the sweep always fires for local timeouts, and that the HINT text
+// (not the cleanup) is what carries the platform-evidence gating.
+
+import net from 'node:net';
+import http from 'node:http';
+import os from 'node:os';
+import path from 'node:path';
+import assert from 'node:assert/strict';
+import { beforeEach, test, vi } from 'vitest';
+
+const { mockRunCmdSync } = vi.hoisted(() => ({ mockRunCmdSync: vi.fn() }));
+
+vi.mock('../../../utils/exec.ts', async () => {
+  const actual =
+    await vi.importActual<typeof import('../../../utils/exec.ts')>('../../../utils/exec.ts');
+  return { ...actual, runCmdSync: mockRunCmdSync };
+});
+
+import { AppError } from '@agent-device/kernel/errors';
+import { sendRequest } from '../daemon-client-transport.ts';
+import type { DaemonRequest } from '../../types.ts';
+import type { DaemonInfo } from '../daemon-client-metadata.ts';
+import type { DaemonPaths } from '../../config.ts';
+
+const TIMEOUT_MS = 120;
+
+// `snapshot`'s timeout policy preserves the daemon (onTimeout !==
+// 'reset-daemon'), so `handleRequestTimeout` never reaches
+// `resetDaemonAfterTimeout` (`process.kill`) here — keeping this suite
+// side-effect-free outside the mocked pkill sweep.
+const SNAPSHOT_COMMAND = 'snapshot';
+
+function dummyStatePaths(): DaemonPaths {
+  const baseDir = path.join(os.tmpdir(), 'agent-device-timeout-route-test');
+  return {
+    baseDir,
+    infoPath: path.join(baseDir, 'daemon.json'),
+    lockPath: path.join(baseDir, 'daemon.lock'),
+    logPath: path.join(baseDir, 'daemon.log'),
+    sessionsDir: path.join(baseDir, 'sessions'),
+  };
+}
+
+function buildRequest(platform: 'android' | 'ios' | undefined): DaemonRequest {
+  return {
+    token: 'test-token',
+    session: 'default',
+    command: SNAPSHOT_COMMAND,
+    positionals: [],
+    flags: platform ? { platform } : {},
+    meta: { requestId: 'req-timeout-route' },
+  };
+}
+
+function startHangingSocketServer(): Promise<{ server: net.Server; port: number }> {
+  return new Promise((resolve, reject) => {
+    const server = net.createServer((socket) => {
+      // Accept the connection but never write a response — forces the
+      // client's own request-timeout envelope to fire.
+      socket.on('error', () => {});
+    });
+    server.on('error', reject);
+    server.listen(0, '127.0.0.1', () => {
+      const address = server.address();
+      if (address && typeof address === 'object') {
+        resolve({ server, port: address.port });
+      } else {
+        reject(new Error('failed to bind hanging socket test server'));
+      }
+    });
+  });
+}
+
+function startHangingHttpServer(): Promise<{ server: http.Server; port: number }> {
+  return new Promise((resolve, reject) => {
+    const server = http.createServer((_req, res) => {
+      // Never call res.end() — forces the client's own request-timeout
+      // envelope to fire instead of a real response.
+      res.on('error', () => {});
+    });
+    server.on('clientError', (_err, socket) => socket.destroy());
+    server.on('error', reject);
+    server.listen(0, '127.0.0.1', () => {
+      const address = server.address();
+      if (address && typeof address === 'object') {
+        resolve({ server, port: address.port });
+      } else {
+        reject(new Error('failed to bind hanging http test server'));
+      }
+    });
+  });
+}
+
+beforeEach(() => {
+  mockRunCmdSync.mockReset();
+});
+
+test('socket timeout: pkill cleanup still runs for a declared non-Apple platform that actually terminates a runner (rebound-session case), and the hint claims Apple on that evidence', async () => {
+  // Simulates --session-lock strip silently rebinding this request onto an
+  // existing Apple session: the client declared `platform: 'android'`, but
+  // real Apple xcodebuild work was in flight and the pkill sweep kills it.
+  mockRunCmdSync.mockImplementation((cmd: string) =>
+    cmd === 'pkill'
+      ? { exitCode: 0, stdout: '', stderr: '' }
+      : { exitCode: 1, stdout: '', stderr: '' },
+  );
+
+  const { server, port } = await startHangingSocketServer();
+  try {
+    const info: DaemonInfo = { port, token: 'test-token', pid: process.pid };
+    const req = buildRequest('android');
+
+    await assert.rejects(
+      sendRequest(info, req, 'socket', dummyStatePaths(), TIMEOUT_MS),
+      (error: unknown) => {
+        assert.ok(error instanceof AppError);
+        assert.match(error.details?.hint as string, /Apple runner work was aborted when detected/);
+        return true;
+      },
+    );
+  } finally {
+    server.close();
+  }
+
+  // The eligibility assertion: cleanup ran (all three kill patterns
+  // attempted) even though the request declared a non-Apple platform. A
+  // design that skips cleanup based on the declared flag would fail this.
+  const pkillCalls = mockRunCmdSync.mock.calls.filter(([cmd]) => cmd === 'pkill');
+  assert.equal(pkillCalls.length, 3);
+});
+
+test('http timeout: pkill cleanup still runs for an undeclared platform (unknown-session case) that terminates nothing, and the hint stays platform-neutral', async () => {
+  // Simulates the common session-bound request that never repeats
+  // --platform, on a real Android/web/Harmony session: no processes match
+  // the Apple-specific kill patterns.
+  mockRunCmdSync.mockImplementation(() => ({ exitCode: 1, stdout: '', stderr: '' }));
+
+  const { server, port } = await startHangingHttpServer();
+  try {
+    const info: DaemonInfo = { httpPort: port, token: 'test-token', pid: process.pid };
+    const req = buildRequest(undefined);
+
+    await assert.rejects(
+      sendRequest(info, req, 'http', dummyStatePaths(), TIMEOUT_MS),
+      (error: unknown) => {
+        assert.ok(error instanceof AppError);
+        const hint = error.details?.hint as string;
+        assert.doesNotMatch(hint, /Apple/);
+        assert.match(
+          hint,
+          /The timed-out snapshot request was canceled; the daemon was kept alive/,
+        );
+        return true;
+      },
+    );
+  } finally {
+    server.close();
+  }
+
+  // Cleanup still ran — this is the regression this suite exists to catch:
+  // an eligibility design keyed off the (here, absent) declared platform
+  // would either skip cleanup entirely or — under the original unconditional
+  // hint — falsely claim Apple involvement anyway. Neither happens here.
+  const pkillCalls = mockRunCmdSync.mock.calls.filter(([cmd]) => cmd === 'pkill');
+  assert.equal(pkillCalls.length, 3);
+});
+
+test('http timeout: an explicitly declared Apple platform keeps the Apple hint even when the sweep terminates nothing', async () => {
+  mockRunCmdSync.mockImplementation(() => ({ exitCode: 1, stdout: '', stderr: '' }));
+
+  const { server, port } = await startHangingHttpServer();
+  try {
+    const info: DaemonInfo = { httpPort: port, token: 'test-token', pid: process.pid };
+    const req = buildRequest('ios');
+
+    await assert.rejects(
+      sendRequest(info, req, 'http', dummyStatePaths(), TIMEOUT_MS),
+      (error: unknown) => {
+        assert.ok(error instanceof AppError);
+        assert.match(error.details?.hint as string, /Apple runner work was aborted when detected/);
+        return true;
+      },
+    );
+  } finally {
+    server.close();
+  }
+
+  const pkillCalls = mockRunCmdSync.mock.calls.filter(([cmd]) => cmd === 'pkill');
+  assert.equal(pkillCalls.length, 3);
+});
+
+test('remote HTTP timeout never runs the Apple pkill cleanup and uses the remote-specific hint', async () => {
+  mockRunCmdSync.mockImplementation(() => ({ exitCode: 0, stdout: '', stderr: '' }));
+
+  const { server, port } = await startHangingHttpServer();
+  try {
+    const info: DaemonInfo = {
+      baseUrl: `http://127.0.0.1:${port}`,
+      token: 'test-token',
+      pid: process.pid,
+    };
+    const req = buildRequest('android');
+
+    await assert.rejects(
+      sendRequest(info, req, 'http', dummyStatePaths(), TIMEOUT_MS),
+      (error: unknown) => {
+        assert.ok(error instanceof AppError);
+        assert.match(
+          error.details?.hint as string,
+          /verify the remote daemon URL, auth token, and remote host logs/,
+        );
+        return true;
+      },
+    );
+  } finally {
+    server.close();
+  }
+
+  assert.equal(mockRunCmdSync.mock.calls.length, 0);
+});

src/daemon/client/daemon-client-timeout.ts

@@ -10,6 +10,7 @@ import type {
 } from '../../core/command-descriptor/types.ts';
 import type { DaemonRequest } from '../types.ts';
 import type { DaemonPaths } from '../config.ts';
+import type { PlatformSelector } from '@agent-device/kernel/device';
 import {
   removeDaemonInfo,
   removeDaemonLock,
@@ -23,6 +24,22 @@ const IOS_RUNNER_XCODEBUILD_KILL_PATTERNS = [
   'xcodebuild build-for-testing .*apple/runner/AgentDeviceRunner/AgentDeviceRunner\\.xcodeproj',
 ];
 
+// `--platform` selectors that AFFIRMATIVELY name (or alias) an Apple device.
+// This is deliberately narrower than "not proven non-Apple": the client's
+// declared platform is not authoritative for session-bound execution (see
+// the eligibility note on `handleRequestTimeout` below), so an undeclared or
+// declared-non-Apple platform is not evidence of anything — it only counts
+// as Apple evidence when it says so outright.
+const AFFIRMATIVE_APPLE_PLATFORM_SELECTORS: ReadonlySet<PlatformSelector> = new Set([
+  'apple',
+  'ios',
+  'macos',
+]);
+
+function isAffirmativelyApplePlatform(platform: PlatformSelector | undefined): boolean {
+  return platform !== undefined && AFFIRMATIVE_APPLE_PLATFORM_SELECTORS.has(platform);
+}
+
 type BoundedTimeoutPolicy = CommandTimeoutPolicy & { envelopeMs: number };
 type FlagTimeoutBudget = Extract<CommandTimeoutBudget, { source: 'flag' }>;
 type RequestFlags = Omit<DaemonRequest, 'token'>['flags'];
@@ -98,12 +115,32 @@ export function handleRequestTimeout(
   command: string | undefined,
   remote: boolean,
   timeoutMs: number,
+  platform: PlatformSelector | undefined,
 ): AppError {
+  // Cleanup eligibility stays UNCONDITIONAL for every local (non-remote)
+  // timeout, on purpose: the request's declared --platform is not
+  // authoritative for session-bound execution. An existing session's real
+  // device platform can silently override a conflicting declared selector
+  // (`applyStripLockPolicy` in request-lock-policy.ts, reached via
+  // --session-lock strip), and the common session-bound request omits
+  // --platform entirely — so there is no client-visible signal that proves
+  // a request cannot touch an Apple runner. The pkill patterns are
+  // Apple-process-name-specific, so sweeping them on a non-Apple host or
+  // session matches nothing and costs a few no-op subprocess spawns, never
+  // a wrong skip.
   const cleanup = remote ? { terminated: 0 } : cleanupTimedOutIosRunnerBuilds();
   const resetDaemon = !remote && shouldResetDaemonAfterRequestTimeout(command);
   const daemonReset = resetDaemon
     ? resetDaemonAfterTimeout(info, statePaths)
     : { forcedKill: false };
+  // The HINT, unlike cleanup, may only name Apple-runner involvement on
+  // evidence this call site actually has: an explicitly declared Apple
+  // platform selector, or the cleanup itself having terminated a matching
+  // process (proof positive regardless of what --platform claimed). Any
+  // other combination — undeclared platform, declared non-Apple platform,
+  // zero processes terminated — gets platform-neutral wording instead of
+  // asserting Apple specifics the client cannot back up.
+  const appleCleanupEvidence = isAffirmativelyApplePlatform(platform) || cleanup.terminated > 0;
   emitDiagnostic({
     level: 'error',
     phase: 'daemon_request_timeout',
@@ -122,7 +159,7 @@ export function handleRequestTimeout(
   return new AppError('COMMAND_FAILED', 'Daemon request timed out', {
     timeoutMs,
     requestId,
-    hint: resolveRequestTimeoutHint({ remote, resetDaemon, command }),
+    hint: resolveRequestTimeoutHint({ remote, resetDaemon, command, appleCleanupEvidence }),
   });
 }
 
@@ -135,23 +172,36 @@ export function shouldResetDaemonAfterRequestTimeout(command: string | undefined
   return resolveCommandTimeoutPolicy(command).onTimeout === 'reset-daemon';
 }
 
-function resolveRequestTimeoutHint(params: {
+// Exported for direct hint-matrix testing: handleRequestTimeout also triggers
+// real pkill/process-kill side effects, so its wording is verified through
+// this pure sub-function rather than the full timeout path (see also the
+// production-seam route tests in
+// src/daemon/client/__tests__/daemon-client-timeout-route.test.ts, which
+// prove the cleanup-eligibility side of this contract that a pure formatter
+// test cannot).
+export function resolveRequestTimeoutHint(params: {
   remote: boolean;
   resetDaemon: boolean;
   command: string | undefined;
+  appleCleanupEvidence: boolean;
 }): string {
-  const { remote, resetDaemon, command } = params;
+  const { remote, resetDaemon, command, appleCleanupEvidence } = params;
   if (remote) {
     return 'Retry with --debug and verify the remote daemon URL, auth token, and remote host logs.';
   }
   if (!resetDaemon) {
     const iosPrepareHint =
-      command === PUBLIC_COMMANDS.snapshot
+      appleCleanupEvidence && command === PUBLIC_COMMANDS.snapshot
         ? ' If this was the first Apple-platform snapshot on the device, run agent-device prepare ios-runner with the same --platform before snapshot/test so runner startup is handled explicitly.'
         : '';
-    return `Retry with --debug and check daemon diagnostics logs. The timed-out ${command ?? 'request'} request was canceled and Apple runner work was aborted when detected; the daemon was kept alive so the session can still be closed or inspected.${iosPrepareHint}`;
+    const appleCleanupNote = appleCleanupEvidence
+      ? ' and Apple runner work was aborted when detected'
+      : '';
+    return `Retry with --debug and check daemon diagnostics logs. The timed-out ${command ?? 'request'} request was canceled${appleCleanupNote}; the daemon was kept alive so the session can still be closed or inspected.${iosPrepareHint}`;
   }
-  return 'Retry with --debug and check daemon diagnostics logs. Timed-out Apple runner xcodebuild processes were terminated when detected.';
+  return appleCleanupEvidence
+    ? 'Retry with --debug and check daemon diagnostics logs. Timed-out Apple runner xcodebuild processes were terminated when detected.'
+    : 'Retry with --debug and check daemon diagnostics logs. The daemon was reset after the timeout.';
 }
 
 function cleanupTimedOutIosRunnerBuilds(): { terminated: number; error?: string } {

src/daemon/client/daemon-client-transport.ts

@@ -322,6 +322,7 @@ async function sendSocketRequest(
                 req.command,
                 false,
                 timeoutMs,
+                req.flags?.platform,
               ),
             );
           }, timeoutMs)
@@ -436,6 +437,7 @@ async function sendHttpRequest(
                 req.command,
                 remote,
                 timeoutMs,
+                req.flags?.platform,
               ),
             );
           }, timeoutMs)