Fix stale packaged terminal daemon reuse (#3995)
This commit is contained in:
parent
b7ea489456
commit
b235ff1e4d
|
|
@ -1,5 +1,6 @@
|
|||
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
import { mkdtempSync, rmSync, writeFileSync } from 'fs'
|
||||
import { spawn } from 'child_process'
|
||||
import { mkdtempSync, rmSync, utimesSync, writeFileSync } from 'fs'
|
||||
import { tmpdir } from 'os'
|
||||
import { join } from 'path'
|
||||
import { createServer, connect, type Server } from 'net'
|
||||
|
|
@ -8,6 +9,7 @@ import { getDaemonPidPath, serializeDaemonPidFile } from './daemon-spawner'
|
|||
import {
|
||||
getProcessStartedAtMs,
|
||||
healthCheckDaemon,
|
||||
isDaemonOlderThanPathMtime,
|
||||
killStaleDaemon,
|
||||
parseDaemonPidFile,
|
||||
startTimeMatches
|
||||
|
|
@ -264,3 +266,60 @@ describe('killStaleDaemon pid identity guards', () => {
|
|||
}
|
||||
})
|
||||
})
|
||||
|
||||
describe('isDaemonOlderThanPathMtime', () => {
|
||||
let dir: string
|
||||
let socketPath: string
|
||||
let tokenPath: string
|
||||
|
||||
beforeEach(() => {
|
||||
dir = mkdtempSync(join(tmpdir(), 'daemon-health-mtime-test-'))
|
||||
socketPath = join(dir, 'daemon.sock')
|
||||
tokenPath = join(dir, 'daemon.token')
|
||||
})
|
||||
|
||||
afterEach(() => {
|
||||
rmSync(dir, { recursive: true, force: true })
|
||||
})
|
||||
|
||||
it('detects a daemon that started before the current bundle entry was written', async () => {
|
||||
if (process.platform === 'win32') {
|
||||
return
|
||||
}
|
||||
|
||||
const child = spawn(
|
||||
process.execPath,
|
||||
[
|
||||
'-e',
|
||||
'setTimeout(() => {}, 30000)',
|
||||
'daemon-entry',
|
||||
'--socket',
|
||||
socketPath,
|
||||
'--token',
|
||||
tokenPath
|
||||
],
|
||||
{ stdio: 'ignore' }
|
||||
)
|
||||
try {
|
||||
await new Promise((resolve) => setTimeout(resolve, 100))
|
||||
const startedAtMs = getProcessStartedAtMs(child.pid!)
|
||||
if (startedAtMs === null) {
|
||||
return
|
||||
}
|
||||
|
||||
const entryPath = join(dir, 'daemon-entry.js')
|
||||
writeFileSync(entryPath, '', 'utf8')
|
||||
const future = new Date(startedAtMs + 10_000)
|
||||
utimesSync(entryPath, future, future)
|
||||
writeFileSync(
|
||||
getDaemonPidPath(dir),
|
||||
serializeDaemonPidFile({ pid: child.pid!, startedAtMs, entryPath }),
|
||||
{ mode: 0o600 }
|
||||
)
|
||||
|
||||
expect(isDaemonOlderThanPathMtime(dir, socketPath, tokenPath, entryPath)).toBe(true)
|
||||
} finally {
|
||||
child.kill('SIGKILL')
|
||||
}
|
||||
})
|
||||
})
|
||||
|
|
|
|||
|
|
@ -1,7 +1,7 @@
|
|||
/* oxlint-disable max-lines -- Why: pid validation shares process-identity
|
||||
helpers with kill escalation so the SIGKILL safety checks stay co-located. */
|
||||
import { execFileSync } from 'child_process'
|
||||
import { existsSync, readFileSync, unlinkSync } from 'fs'
|
||||
import { existsSync, readFileSync, statSync, unlinkSync } from 'fs'
|
||||
import { connect, type Socket } from 'net'
|
||||
import { encodeNdjson } from './ndjson'
|
||||
import { getDaemonPidPath } from './daemon-spawner'
|
||||
|
|
@ -498,6 +498,38 @@ export function getDaemonLaunchIdentity(
|
|||
return commandLine.includes(expectedEntryPath) ? 'match' : 'mismatch'
|
||||
}
|
||||
|
||||
export function isDaemonOlderThanPathMtime(
|
||||
runtimeDir: string,
|
||||
socketPath: string,
|
||||
tokenPath: string,
|
||||
path: string,
|
||||
protocolVersion = PROTOCOL_VERSION
|
||||
): boolean {
|
||||
let parsedPid: ParsedDaemonPid | null
|
||||
try {
|
||||
parsedPid = parseDaemonPidFile(
|
||||
readFileSync(getDaemonPidPath(runtimeDir, protocolVersion), 'utf8')
|
||||
)
|
||||
} catch {
|
||||
return false
|
||||
}
|
||||
|
||||
if (!parsedPid || !isDaemonProcess(parsedPid.pid, socketPath, tokenPath, parsedPid.startedAtMs)) {
|
||||
return false
|
||||
}
|
||||
|
||||
const startedAtMs = parsedPid.startedAtMs ?? getProcessStartedAtMs(parsedPid.pid)
|
||||
if (startedAtMs === null) {
|
||||
return false
|
||||
}
|
||||
|
||||
try {
|
||||
return startedAtMs + START_TIME_TOLERANCE_MS < statSync(path).mtimeMs
|
||||
} catch {
|
||||
return false
|
||||
}
|
||||
}
|
||||
|
||||
export async function killStaleDaemon(
|
||||
runtimeDir: string,
|
||||
socketPath: string,
|
||||
|
|
|
|||
|
|
@ -22,6 +22,7 @@ const {
|
|||
healthCheckDaemonMock,
|
||||
getMacDaemonSystemResolverHealthMock,
|
||||
getDaemonLaunchIdentityMock,
|
||||
isDaemonOlderThanPathMtimeMock,
|
||||
killStaleDaemonMock,
|
||||
getProcessStartedAtMsMock,
|
||||
daemonClientMock,
|
||||
|
|
@ -65,6 +66,7 @@ const {
|
|||
const healthCheckDaemonMock = vi.fn(async () => true)
|
||||
const getMacDaemonSystemResolverHealthMock = vi.fn(() => 'healthy')
|
||||
const getDaemonLaunchIdentityMock = vi.fn(() => 'match')
|
||||
const isDaemonOlderThanPathMtimeMock = vi.fn(() => false)
|
||||
const killStaleDaemonMock = vi.fn(async () => true)
|
||||
const getProcessStartedAtMsMock = vi.fn(() => 1_000_000)
|
||||
|
||||
|
|
@ -97,6 +99,7 @@ const {
|
|||
healthCheckDaemonMock,
|
||||
getMacDaemonSystemResolverHealthMock,
|
||||
getDaemonLaunchIdentityMock,
|
||||
isDaemonOlderThanPathMtimeMock,
|
||||
killStaleDaemonMock,
|
||||
getProcessStartedAtMsMock,
|
||||
daemonClientMock,
|
||||
|
|
@ -165,6 +168,7 @@ vi.mock('./daemon-health', () => ({
|
|||
getDaemonLaunchIdentity: getDaemonLaunchIdentityMock,
|
||||
getMacDaemonSystemResolverHealth: getMacDaemonSystemResolverHealthMock,
|
||||
healthCheckDaemon: healthCheckDaemonMock,
|
||||
isDaemonOlderThanPathMtime: isDaemonOlderThanPathMtimeMock,
|
||||
killStaleDaemon: killStaleDaemonMock,
|
||||
getProcessStartedAtMs: getProcessStartedAtMsMock
|
||||
}))
|
||||
|
|
@ -256,6 +260,8 @@ async function importFresh() {
|
|||
getMacDaemonSystemResolverHealthMock.mockReset()
|
||||
getMacDaemonSystemResolverHealthMock.mockReturnValue('healthy')
|
||||
getDaemonLaunchIdentityMock.mockClear()
|
||||
isDaemonOlderThanPathMtimeMock.mockReset()
|
||||
isDaemonOlderThanPathMtimeMock.mockReturnValue(false)
|
||||
killStaleDaemonMock.mockClear()
|
||||
getAppPathMock.mockReset()
|
||||
getAppPathMock.mockReturnValue('/fake/app')
|
||||
|
|
@ -1023,7 +1029,7 @@ describe('daemon-init: runRestartDaemon (7-step sequence)', () => {
|
|||
expect(child.unref).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('keeps packaged healthy-daemon reuse independent of dev app-path identity', async () => {
|
||||
it('preserves a packaged healthy daemon when its app bundle is current', async () => {
|
||||
const mod = await importFresh()
|
||||
await mod.initDaemonPtyProvider()
|
||||
|
||||
|
|
@ -1035,12 +1041,76 @@ describe('daemon-init: runRestartDaemon (7-step sequence)', () => {
|
|||
killStaleDaemonMock.mockClear()
|
||||
forkMock.mockClear()
|
||||
isPackagedMock.mockReturnValue(true)
|
||||
getDaemonLaunchIdentityMock.mockReturnValueOnce('mismatch')
|
||||
|
||||
await launcher('/fake/socket', '/fake/token')
|
||||
|
||||
expect(getDaemonLaunchIdentityMock).not.toHaveBeenCalled()
|
||||
expect(getDaemonLaunchIdentityMock).toHaveBeenCalledWith(
|
||||
'/fake/userData/daemon',
|
||||
'/fake/socket',
|
||||
'/fake/token',
|
||||
'/fake/app/out/main/daemon-entry.js'
|
||||
)
|
||||
expect(isDaemonOlderThanPathMtimeMock).toHaveBeenCalledWith(
|
||||
'/fake/userData/daemon',
|
||||
'/fake/socket',
|
||||
'/fake/token',
|
||||
'/fake/app/out/main/daemon-entry.js'
|
||||
)
|
||||
expect(killStaleDaemonMock).not.toHaveBeenCalled()
|
||||
expect(forkMock).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('respawns a packaged daemon that predates the current app bundle', async () => {
|
||||
const mod = await importFresh()
|
||||
await mod.initDaemonPtyProvider()
|
||||
|
||||
const launcher = spawnerInstances[0].launcher as (
|
||||
socketPath: string,
|
||||
tokenPath: string
|
||||
) => Promise<{ shutdown(): Promise<void> }>
|
||||
isPackagedMock.mockReturnValue(true)
|
||||
isDaemonOlderThanPathMtimeMock.mockReturnValueOnce(true)
|
||||
forkMock.mockImplementationOnce(() => {
|
||||
const handlers: Record<string, ((arg?: unknown) => void)[]> = {
|
||||
message: [],
|
||||
error: [],
|
||||
exit: []
|
||||
}
|
||||
return {
|
||||
pid: 12345,
|
||||
on(event: string, cb: (arg?: unknown) => void) {
|
||||
handlers[event]?.push(cb)
|
||||
if (event === 'message') {
|
||||
queueMicrotask(() => cb({ type: 'ready' }))
|
||||
}
|
||||
return this
|
||||
},
|
||||
off(event: string, cb: (arg?: unknown) => void) {
|
||||
handlers[event] = handlers[event]?.filter((handler) => handler !== cb) ?? []
|
||||
return this
|
||||
},
|
||||
disconnect: vi.fn(),
|
||||
unref: vi.fn()
|
||||
}
|
||||
})
|
||||
|
||||
await launcher('/fake/socket', '/fake/token')
|
||||
|
||||
expect(isDaemonOlderThanPathMtimeMock).toHaveBeenCalledWith(
|
||||
'/fake/userData/daemon',
|
||||
'/fake/socket',
|
||||
'/fake/token',
|
||||
'/fake/app/out/main/daemon-entry.js'
|
||||
)
|
||||
expect(killStaleDaemonMock).toHaveBeenCalledWith(
|
||||
'/fake/userData/daemon',
|
||||
'/fake/socket',
|
||||
'/fake/token'
|
||||
)
|
||||
expect(forkMock).toHaveBeenCalledWith(
|
||||
'/fake/app/out/main/daemon-entry.js',
|
||||
['--socket', '/fake/socket', '--token', '/fake/token'],
|
||||
expect.objectContaining({ detached: true })
|
||||
)
|
||||
})
|
||||
})
|
||||
|
|
|
|||
|
|
@ -32,6 +32,7 @@ import {
|
|||
getDaemonLaunchIdentity,
|
||||
getProcessStartedAtMs,
|
||||
healthCheckDaemon,
|
||||
isDaemonOlderThanPathMtime,
|
||||
killStaleDaemon
|
||||
} from './daemon-health'
|
||||
import {
|
||||
|
|
@ -162,15 +163,19 @@ function createOutOfProcessLauncher(runtimeDir: string): DaemonLauncher {
|
|||
console.warn('[daemon] Replacing daemon with unavailable macOS system resolver')
|
||||
await cleanupDaemonForProtocol(runtimeDir, PROTOCOL_VERSION)
|
||||
} else {
|
||||
// Why: dev worktrees share the same orca-dev userData, so a daemon from
|
||||
// a deleted sibling checkout can pass protocol health checks while still
|
||||
// pointing at missing native modules. Packaged app paths are stable and
|
||||
// should preserve existing warm daemon reuse semantics.
|
||||
const identity = app.isPackaged
|
||||
? 'match'
|
||||
: getDaemonLaunchIdentity(runtimeDir, socketPath, tokenPath, entryPath)
|
||||
if (identity === 'mismatch') {
|
||||
console.warn('[daemon] Replacing daemon launched from a different app path')
|
||||
// Why: a protocol-healthy daemon can outlive the app bundle that
|
||||
// launched it. In dev this happens after deleting/rebuilding a
|
||||
// worktree; in packaged apps it happens when the stable
|
||||
// /Applications/Orca.app path is replaced during update.
|
||||
const identity = getDaemonLaunchIdentity(runtimeDir, socketPath, tokenPath, entryPath)
|
||||
const stalePackagedBundle =
|
||||
app.isPackaged && isDaemonOlderThanPathMtime(runtimeDir, socketPath, tokenPath, entryPath)
|
||||
if (identity === 'mismatch' || stalePackagedBundle) {
|
||||
console.warn(
|
||||
stalePackagedBundle
|
||||
? '[daemon] Replacing daemon launched before the current app bundle was installed'
|
||||
: '[daemon] Replacing daemon launched from a different app path'
|
||||
)
|
||||
await cleanupDaemonForProtocol(runtimeDir, PROTOCOL_VERSION)
|
||||
} else {
|
||||
// Why: daemon is already running from a previous app session and
|
||||
|
|
|
|||
|
|
@ -0,0 +1,17 @@
|
|||
import { describe, expect, it } from 'vitest'
|
||||
import { shouldOfferDaemonRestart } from './TerminalErrorToast'
|
||||
|
||||
describe('shouldOfferDaemonRestart', () => {
|
||||
it('matches stale daemon node-pty install failures', () => {
|
||||
expect(
|
||||
shouldOfferDaemonRestart(
|
||||
"Daemon's node-pty install is gone (worktree deleted?). Restart Orca. node-pty: posix_spawn failed: ENOENT (errno 2, No such file or directory) - helper='/Applications/Orca.app/Contents/Resources/app.asar.unpacked/node_modules/node-pty/build/Release/spawn-helper'"
|
||||
)
|
||||
).toBe(true)
|
||||
})
|
||||
|
||||
it('does not match unrelated terminal spawn errors', () => {
|
||||
expect(shouldOfferDaemonRestart('SSH connection is not active.')).toBe(false)
|
||||
expect(shouldOfferDaemonRestart('node-pty: open_slave failed: EMFILE (errno 24)')).toBe(false)
|
||||
})
|
||||
})
|
||||
|
|
@ -1,17 +1,28 @@
|
|||
const SSH_PREFIX = 'SSH connection is not active'
|
||||
const STALE_NODE_PTY_DAEMON_MARKERS = [
|
||||
"Daemon's node-pty install is gone",
|
||||
'node-pty: posix_spawn failed: ENOENT'
|
||||
]
|
||||
|
||||
function isSshError(error: string): boolean {
|
||||
return error.startsWith(SSH_PREFIX)
|
||||
}
|
||||
|
||||
export function shouldOfferDaemonRestart(error: string): boolean {
|
||||
return STALE_NODE_PTY_DAEMON_MARKERS.every((marker) => error.includes(marker))
|
||||
}
|
||||
|
||||
export function TerminalErrorToast({
|
||||
error,
|
||||
onDismiss
|
||||
onDismiss,
|
||||
onRestartDaemon
|
||||
}: {
|
||||
error: string
|
||||
onDismiss: () => void
|
||||
onRestartDaemon?: () => void
|
||||
}): React.JSX.Element {
|
||||
const ssh = isSshError(error)
|
||||
const showDaemonRestart = !ssh && onRestartDaemon && shouldOfferDaemonRestart(error)
|
||||
|
||||
return (
|
||||
<div
|
||||
|
|
@ -33,9 +44,14 @@ export function TerminalErrorToast({
|
|||
}}
|
||||
>
|
||||
<div style={{ display: 'flex', justifyContent: 'space-between', alignItems: 'start' }}>
|
||||
<span>
|
||||
<span style={{ minWidth: 0 }}>
|
||||
{error}
|
||||
{!ssh && (
|
||||
{showDaemonRestart ? (
|
||||
<>
|
||||
{'\n'}
|
||||
Restart the terminal daemon from here to clear stale daemon state.
|
||||
</>
|
||||
) : !ssh ? (
|
||||
<>
|
||||
{'\n'}
|
||||
If this persists, please{' '}
|
||||
|
|
@ -47,8 +63,27 @@ export function TerminalErrorToast({
|
|||
</a>
|
||||
.
|
||||
</>
|
||||
)}
|
||||
) : null}
|
||||
</span>
|
||||
{showDaemonRestart ? (
|
||||
<button
|
||||
onClick={onRestartDaemon}
|
||||
style={{
|
||||
marginLeft: 12,
|
||||
border: '1px solid rgba(252, 165, 165, 0.45)',
|
||||
borderRadius: 6,
|
||||
background: 'rgba(127, 29, 29, 0.35)',
|
||||
color: '#fecaca',
|
||||
cursor: 'pointer',
|
||||
fontSize: 12,
|
||||
padding: '4px 8px',
|
||||
whiteSpace: 'nowrap',
|
||||
flexShrink: 0
|
||||
}}
|
||||
>
|
||||
Restart daemon
|
||||
</button>
|
||||
) : null}
|
||||
<button
|
||||
onClick={onDismiss}
|
||||
style={{
|
||||
|
|
|
|||
|
|
@ -7,6 +7,7 @@ import { X } from 'lucide-react'
|
|||
import { useAppStore } from '../../store'
|
||||
import { Button } from '@/components/ui/button'
|
||||
import { Tooltip, TooltipContent, TooltipTrigger } from '@/components/ui/tooltip'
|
||||
import { DaemonActionDialog, useDaemonActions } from '@/components/shared/useDaemonActions'
|
||||
import {
|
||||
DEFAULT_TERMINAL_DIVIDER_DARK,
|
||||
isTerminalBackgroundLight,
|
||||
|
|
@ -212,6 +213,7 @@ export default function TerminalPane({
|
|||
const [agentSessionFork, setAgentSessionFork] = useState<PreparedAgentSessionFork | null>(null)
|
||||
const [terminalError, setTerminalError] = useState<string | null>(null)
|
||||
const [sessionStateSaveFailureOpen, setSessionStateSaveFailureOpen] = useState(false)
|
||||
const daemonActions = useDaemonActions()
|
||||
// Why: override state lives in a plain Map for perf (safeFit reads it on
|
||||
// every resize). This counter forces a re-render when overrides change so
|
||||
// the mobile-fit banner appears/disappears. When an override is cleared
|
||||
|
|
@ -1742,8 +1744,13 @@ export default function TerminalPane({
|
|||
}}
|
||||
/>
|
||||
{terminalError && isActive && (
|
||||
<TerminalErrorToast error={terminalError} onDismiss={() => setTerminalError(null)} />
|
||||
<TerminalErrorToast
|
||||
error={terminalError}
|
||||
onDismiss={() => setTerminalError(null)}
|
||||
onRestartDaemon={() => daemonActions.setPending('restart')}
|
||||
/>
|
||||
)}
|
||||
<DaemonActionDialog api={daemonActions} />
|
||||
{isActive && (
|
||||
<TerminalSessionStateSaveFailureDialog
|
||||
open={sessionStateSaveFailureOpen}
|
||||
|
|
|
|||
Loading…
Reference in New Issue