diff --git a/src/main/daemon/daemon-health.test.ts b/src/main/daemon/daemon-health.test.ts index defbc8fd09a..152ed019029 100644 --- a/src/main/daemon/daemon-health.test.ts +++ b/src/main/daemon/daemon-health.test.ts @@ -7,6 +7,7 @@ import { DaemonServer } from './daemon-server' import { getDaemonPidPath, serializeDaemonPidFile } from './daemon-spawner' import { checkDaemonHealth, + E2E_FORCE_DAEMON_HEALTH_UNREACHABLE_ENV, getProcessStartedAtMs, healthCheckDaemon, killStaleDaemon, @@ -127,6 +128,25 @@ describe('daemon health', () => { await expect(healthCheckDaemon(socketPath, tokenPath)).resolves.toBe(false) }) + it('returns unreachable when the e2e force-health-failure env is set', async () => { + // Why: prove the e2e seam short-circuits even when a real daemon would + // otherwise pass — not the already-covered missing-socket path. + const server = new DaemonServer({ + socketPath, + tokenPath, + spawnSubprocess: () => createMockSubprocess() + }) + await server.start() + vi.stubEnv(E2E_FORCE_DAEMON_HEALTH_UNREACHABLE_ENV, '1') + try { + await expect(checkDaemonHealth(socketPath, tokenPath)).resolves.toBe('unreachable') + await expect(healthCheckDaemon(socketPath, tokenPath)).resolves.toBe(false) + } finally { + vi.unstubAllEnvs() + await server.shutdown() + } + }) + it('classifies a hello-rejected daemon as rejected, not unreachable', async () => { // Why: 'rejected' means the daemon answered and refused adoption — the // launcher may replace it. 'unreachable' also covers a wedged-but-live diff --git a/src/main/daemon/daemon-health.ts b/src/main/daemon/daemon-health.ts index 50e7226f73c..2989f9b2268 100644 --- a/src/main/daemon/daemon-health.ts +++ b/src/main/daemon/daemon-health.ts @@ -24,6 +24,10 @@ const RESOLVER_HEALTH_CHECK_TIMEOUT_MS = 3_000 const KILL_WAIT_MS = 3_000 const KILL_POLL_MS = 100 const START_TIME_TOLERANCE_MS = 1_500 +// Why: e2e forces the failed-health preserve path without SIGSTOP races — +// a stopped daemon also blocks listSessions, so the unhealthy guard cannot +// verify live sessions until SIGCONT, which is flaky under CI load. +export const E2E_FORCE_DAEMON_HEALTH_UNREACHABLE_ENV = 'ORCA_E2E_FORCE_DAEMON_HEALTH_UNREACHABLE' // Why: on Windows the pid file's startedAtMs is the daemon's self-reported // Node start time, while verification reads the OS process creation time — // the gap between them is the exe bootstrap, which AV/disk pressure can @@ -82,6 +86,11 @@ function canConnectSocket(socketPath: string): Promise { export function checkDaemonHealth(socketPath: string, tokenPath: string): Promise { return new Promise((resolve) => { + if (process.env[E2E_FORCE_DAEMON_HEALTH_UNREACHABLE_ENV] === '1') { + resolve('unreachable') + return + } + if (process.platform !== 'win32' && !existsSync(socketPath)) { resolve('unreachable') return diff --git a/src/main/daemon/daemon-init.test.ts b/src/main/daemon/daemon-init.test.ts index 5a3003f767d..4df0a8f85fb 100644 --- a/src/main/daemon/daemon-init.test.ts +++ b/src/main/daemon/daemon-init.test.ts @@ -2160,6 +2160,64 @@ describe('daemon-init: runRestartDaemon (7-step sequence)', () => { expect(forkMock).toHaveBeenCalled() }) + it('stays silent about replacing a daemon on a cold start, where there is none', async () => { + // Why: a first launch reaches the same replace fall-through (unreachable health, + // no socket, nothing to probe); announcing a replacement there reports killing a + // daemon that never existed, on the most common path there is. + const mod = await importFresh() + await mod.initDaemonPtyProvider() + + // Both pre-spawn probes fail: nothing ever answers, so no session count is observed. + const unreachableClient = function MockDaemonClient() { + return { + ensureConnected: vi.fn(async () => { + throw new Error('connect ENOENT') + }), + request: vi.fn(), + disconnect: vi.fn() + } + } + daemonClientMock + .mockImplementationOnce(unreachableClient) + .mockImplementationOnce(unreachableClient) + + const launcher = spawnerInstances[0].launcher as ( + socketPath: string, + tokenPath: string + ) => Promise<{ shutdown(): Promise }> + checkDaemonHealthMock.mockResolvedValueOnce('unreachable') + probeSocketExistsMock.mockReturnValue(false) + forkMock.mockImplementationOnce(() => ({ + pid: 12345, + on(event: string, cb: (arg?: unknown) => void) { + if (event === 'message') { + queueMicrotask(() => cb({ type: 'ready', startedAtMs: 1_000_000 })) + } + return this + }, + once() { + return this + }, + off() { + return this + }, + disconnect: vi.fn(), + unref: vi.fn() + })) + + const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}) + try { + await launcher('/fake/socket', '/fake/token') + + expect(forkMock).toHaveBeenCalled() + expect(warnSpy).not.toHaveBeenCalledWith( + expect.stringContaining('Replacing daemon that failed the health check') + ) + } finally { + warnSpy.mockRestore() + } + }) + // Why: net.connect stub whose 'connect' fires, so probeSocket() reports the pipe alive on every grace re-check. function stubAliveSocketConnect() { const handlers: Record void)[]> = { connect: [], error: [] } @@ -2267,6 +2325,7 @@ describe('daemon-init: runRestartDaemon (7-step sequence)', () => { // Count only the launcher's own session-count probes. daemonClientMock.mockClear() + const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}) try { await launcher('/fake/socket', '/fake/token') @@ -2279,7 +2338,16 @@ describe('daemon-init: runRestartDaemon (7-step sequence)', () => { expect(forkMock).toHaveBeenCalled() // The launcher probes the full grace budget: 1 initial probe + WEDGED_DAEMON_GRACE_RETRIES retries. expect(daemonClientMock).toHaveBeenCalledTimes(3 + WEDGED_DAEMON_GRACE_RETRIES) + // Why: this replace path used to kill the daemon with no log, so a post-hoc + // reader could not tell it apart from an adoption; the verdict must be recorded. + expect(warnSpy).toHaveBeenCalledWith( + expect.stringContaining('Replacing daemon that failed the health check') + ) + expect(warnSpy).toHaveBeenCalledWith( + expect.stringContaining(`graceRetries=${WEDGED_DAEMON_GRACE_RETRIES}`) + ) } finally { + warnSpy.mockRestore() // Restore the answering default: clearAllMocks clears calls not impls, so the throwing impl would leak into later tests. daemonClientMock.mockImplementation(answeringDefault) } diff --git a/src/main/daemon/daemon-init.ts b/src/main/daemon/daemon-init.ts index a748ea10a8b..488b75ef671 100644 --- a/src/main/daemon/daemon-init.ts +++ b/src/main/daemon/daemon-init.ts @@ -428,6 +428,17 @@ function createOutOfProcessLauncher( ) return preserveDaemon() } + // Why: the sibling replace branches announce themselves, but this one used + // to kill a daemon silently — leaving no way to tell a replacement apart + // from an adoption after the fact. A cold start also lands here with + // nothing to replace, so only speak up once something actually answered: + // a probe that returned a count, a socket that survived a grace retry, or + // a refused hello. + if (liveSessionCount !== null || graceRetry > 0 || health === 'rejected') { + console.warn( + `[daemon] Replacing daemon that failed the health check (health=${health}, liveSessions=${liveSessionCount ?? 'unverifiable'}, graceRetries=${graceRetry})` + ) + } } // Why: a raw socket can outlive a broken daemon; kill by PID before respawn so the new daemon doesn't race the stale one. diff --git a/tests/e2e/daemon-slow-health-check-preservation.spec.ts b/tests/e2e/daemon-slow-health-check-preservation.spec.ts index ee583dade14..9e8216c0d34 100644 --- a/tests/e2e/daemon-slow-health-check-preservation.spec.ts +++ b/tests/e2e/daemon-slow-health-check-preservation.spec.ts @@ -13,13 +13,13 @@ import { } from './helpers/terminal' import { ensureTerminalVisible, waitForActiveWorktree, waitForSessionReady } from './helpers/store' import { attachRepoAndOpenTerminal, createRestartSession } from './helpers/orca-restart' +import { E2E_FORCE_DAEMON_HEALTH_UNREACHABLE_ENV } from '../../src/main/daemon/daemon-health' import { PROTOCOL_VERSION } from '../../src/main/daemon/types' import { PTY_SESSION_ID_SEPARATOR } from '../../src/shared/pty-session-id-format' -// Why: must land after the relaunched app's 3s daemon health check has timed -// out (so the unhealthy guard runs) but before the guard's 5s client hello -// budget expires. Daemon init starts within the first ~2s of main startup. -const RESUME_DAEMON_AFTER_MS = 6_500 +// Why: holds the daemon guard's decision until well past the ~600ms it takes +// electron.launch to resolve, which is the earliest the stderr listener can attach. +const GUARD_DECISION_DELAY_MS = 3_000 function readDaemonPid(userDataDir: string): number { const raw = readFileSync( @@ -42,12 +42,10 @@ test('preserves a live daemon PTY when the daemon is too slow for the startup he test.skip(true, 'Global setup did not produce a seeded test repo') return } - test.skip(process.platform === 'win32', 'SIGSTOP/SIGCONT are POSIX-only') const session = createRestartSession(testInfo) let firstApp: ElectronApplication | null = null let secondApp: ElectronApplication | null = null - let daemonPid: number | null = null try { const firstLaunch = await session.launch() @@ -66,70 +64,58 @@ test('preserves a live daemon PTY when the daemon is too slow for the startup he await execInTerminal(firstLaunch.page, ptyId, `echo ${marker}`) await waitForTerminalOutput(firstLaunch.page, marker) - daemonPid = readDaemonPid(session.userDataDir) + const daemonPid = readDaemonPid(session.userDataDir) await session.close(firstApp) firstApp = null - // Why: a stopped daemon still accepts socket connections at the kernel - // level but answers nothing — the same observable behavior as a daemon - // that is too busy to respond within the health-check budget. - process.kill(daemonPid, 'SIGSTOP') - const stderrLines: string[] = [] - const resumeTimer = setTimeout(() => { - if (daemonPid !== null) { - process.kill(daemonPid, 'SIGCONT') - } - }, RESUME_DAEMON_AFTER_MS) - try { - // Why: capture stderr from process start — the daemon guard logs its - // preservation decision during main-process startup, which can complete - // before firstWindow resolves, so a post-launch listener would miss it. - const secondLaunch = await session.launch({ - onStderr: (chunk) => stderrLines.push(chunk) - }) - secondApp = secondLaunch.app + // Why: force the failed-health branch without SIGSTOP. Stopping the daemon + // also blocks listSessions, so the preserve guard races a fixed SIGCONT + // timer under CI load and often takes the healthy path (or misses logs). + // With health forced unreachable, listSessions still succeeds and the only + // way to keep the same daemon PID is the failed-health preserve path. + // + // The init delay is what makes the guard's log observable: Playwright owns + // the child's stderr from spawn and this listener can only attach once + // electron.launch resolves (~600ms in CI). Forced-unreachable health returns + // with no timeout, so an undelayed guard decides at ~500ms and its line is + // lost before the test is listening. + const secondLaunch = await session.launch({ + extraEnv: { + [E2E_FORCE_DAEMON_HEALTH_UNREACHABLE_ENV]: '1', + ORCA_E2E_DAEMON_INIT_DELAY_MS: String(GUARD_DECISION_DELAY_MS) + }, + onStderr: (chunk) => stderrLines.push(chunk) + }) + secondApp = secondLaunch.app - await waitForSessionReady(secondLaunch.page) - await expect - .poll( - async () => secondLaunch.page.evaluate(() => window.__store?.getState().activeWorktreeId), - { timeout: 15_000 } - ) - .toBe(worktreeId) - await ensureTerminalVisible(secondLaunch.page) - await waitForActiveTerminalManager(secondLaunch.page, 30_000) - await waitForPaneCount(secondLaunch.page, 1, 30_000) - await waitForTerminalOutput(secondLaunch.page, marker, 20_000) + await waitForSessionReady(secondLaunch.page) + await expect + .poll( + async () => secondLaunch.page.evaluate(() => window.__store?.getState().activeWorktreeId), + { timeout: 15_000 } + ) + .toBe(worktreeId) + await ensureTerminalVisible(secondLaunch.page) + await waitForActiveTerminalManager(secondLaunch.page, 30_000) + await waitForPaneCount(secondLaunch.page, 1, 30_000) + await waitForTerminalOutput(secondLaunch.page, marker, 20_000) - // The guard path must actually have run and chosen preserve over replace: - // the daemon failed the health check yet was kept because its live session - // was verified. Match the stable "preserve…daemon…health check" concepts - // (not the exact wording) so a benign log reword doesn't flake, and - // confirm the replace path stayed off. - await expect - .poll(() => stderrLines.join(''), { timeout: 10_000 }) - .toMatch(/preserv\w*\s+daemon[^\n]*health check/i) - expect(stderrLines.join('')).not.toMatch(/\breplacing daemon\b/i) - expect(readDaemonPid(session.userDataDir)).toBe(daemonPid) - // Why: a killed daemon cold-restores scrollback from history, so the - // marker text alone cannot distinguish a live session from a dead one. - // The restore banner only appears for cold-restored (dead) sessions. - expect(await getTerminalContent(secondLaunch.page)).not.toContain('--- session restored ---') - } finally { - clearTimeout(resumeTimer) - } + // Why: the same PID alone also holds on the healthy-adoption path, so assert + // the guard's own decision line — otherwise a seam that silently stopped + // working would leave this test passing for the wrong reason. Match the + // stable "preserve…daemon…health check" concepts so a reword doesn't flake. + await expect + .poll(() => stderrLines.join(''), { timeout: 10_000 }) + .toMatch(/preserv\w*\s+daemon[^\n]*health check/i) + expect(readDaemonPid(session.userDataDir)).toBe(daemonPid) + expect(stderrLines.join('')).not.toMatch(/\breplacing daemon\b/i) + // Why: a killed daemon cold-restores scrollback from history, so the + // marker text alone cannot distinguish a live session from a dead one. + // The restore banner only appears for cold-restored (dead) sessions. + expect(await getTerminalContent(secondLaunch.page)).not.toContain('--- session restored ---') } finally { - if (daemonPid !== null) { - try { - // Idempotent: ensures the daemon is resumable for harness cleanup even - // if the test failed before the resume timer fired. - process.kill(daemonPid, 'SIGCONT') - } catch { - // Daemon already gone - } - } if (secondApp) { await session.close(secondApp) } diff --git a/tests/e2e/helpers/electron-launch-args.ts b/tests/e2e/helpers/electron-launch-args.ts index f2c07086543..e2051076bf7 100644 --- a/tests/e2e/helpers/electron-launch-args.ts +++ b/tests/e2e/helpers/electron-launch-args.ts @@ -3,9 +3,14 @@ export function getOrcaElectronLaunchArgs(mainPath: string, headful: boolean): s return [mainPath] } - // Why: Ubuntu CI can fail headless Electron when Chromium's GPU subprocess - // cannot initialize; keep E2E on a low-process software path under Xvfb. + // Why: Ubuntu CI cannot run Electron's setuid chrome-sandbox (not root-owned + // mode 4755 in node_modules). Playwright's electron.launch injects + // --no-sandbox automatically; raw spawn() paths (e.g. second-instance + // activation) must match or Chromium aborts with SIGTRAP before handshake. + // GPU flags keep headless under Xvfb on a software path when the GPU + // subprocess cannot initialize. return [ + '--no-sandbox', '--disable-gpu', '--disable-gpu-compositing', '--disable-gpu-sandbox', diff --git a/tests/e2e/helpers/orca-restart.ts b/tests/e2e/helpers/orca-restart.ts index f31b5edb2e3..4b0083ffba8 100644 --- a/tests/e2e/helpers/orca-restart.ts +++ b/tests/e2e/helpers/orca-restart.ts @@ -42,6 +42,8 @@ type LaunchOptions = { * the test starts capturing. */ onStderr?: (chunk: string) => void + /** Merged into this launch only (not baked into the session's shared env). */ + extraEnv?: Record } type RestartSession = { @@ -153,6 +155,7 @@ export function createRestartSession( args: getOrcaElectronLaunchArgs(mainPath, headful), env: { ...homeIsolation.env, + ...options?.extraEnv, ORCA_E2E_RUNTIME_WS_PORT: String(runtimeWsPort) } })