Skip the synchronous cold-restore replay for live daemon sessions in doSpawn (#7105)
Co-authored-by: Orca <help@stably.ai>
This commit is contained in:
parent
185b768609
commit
4caea00f16
|
|
@ -6,6 +6,7 @@ import { mkdtempSync, mkdirSync, rmSync, existsSync, readFileSync, writeFileSync
|
|||
import { DaemonPtyAdapter } from './daemon-pty-adapter'
|
||||
import { DaemonServer } from './daemon-server'
|
||||
import { getHistorySessionDirName } from './history-paths'
|
||||
import type { HistoryReader } from './history-reader'
|
||||
import type { SubprocessHandle } from './session'
|
||||
import type * as DaemonHealthModule from './daemon-health'
|
||||
import { getDaemonSocketPath } from './daemon-spawner'
|
||||
|
|
@ -1239,6 +1240,159 @@ describe('DaemonPtyAdapter (IPtyProvider)', () => {
|
|||
expect(internals.lastFullCheckpointAt.has(sessionId)).toBe(false)
|
||||
})
|
||||
|
||||
it('skips the cold-restore replay when the daemon session is still alive', async () => {
|
||||
const sessionId = 'warm-reattach-skip-replay'
|
||||
const first = new DaemonPtyAdapter({ socketPath, tokenPath, historyPath: historyDir })
|
||||
await first.spawn({ cols: 80, rows: 24, cwd: '/home/user', sessionId })
|
||||
// Why disconnectOnly: the production app-quit path leaves meta.endedAt
|
||||
// null so the session stays crash-recoverable — the state every app
|
||||
// relaunch with a live daemon sees.
|
||||
await first.disconnectOnly()
|
||||
|
||||
historyAdapter = new DaemonPtyAdapter({ socketPath, tokenPath, historyPath: historyDir })
|
||||
const reader = (historyAdapter as unknown as { historyReader: HistoryReader }).historyReader
|
||||
const detectSpy = vi.spyOn(reader, 'detectColdRestore')
|
||||
const result = await historyAdapter.spawn({ cols: 80, rows: 24, sessionId })
|
||||
|
||||
expect(result.isReattach).toBe(true)
|
||||
expect(result.coldRestore).toBeUndefined()
|
||||
expect(detectSpy).not.toHaveBeenCalled()
|
||||
// The unmanaged-reattach re-anchor must survive the skipped detect.
|
||||
const internals = historyAdapter as unknown as {
|
||||
sessionsNeedingFullCheckpoint: Set<string>
|
||||
lastFullCheckpointAt: Map<string, number>
|
||||
}
|
||||
expect(internals.sessionsNeedingFullCheckpoint.has(sessionId)).toBe(true)
|
||||
expect(internals.lastFullCheckpointAt.has(sessionId)).toBe(false)
|
||||
})
|
||||
|
||||
it('does not probe session aliveness when there is no restorable history', async () => {
|
||||
historyAdapter = new DaemonPtyAdapter({ socketPath, tokenPath, historyPath: historyDir })
|
||||
const client = (
|
||||
historyAdapter as unknown as {
|
||||
client: { request: (type: string, payload?: unknown) => Promise<unknown> }
|
||||
}
|
||||
).client
|
||||
const requestSpy = vi.spyOn(client, 'request')
|
||||
|
||||
await historyAdapter.spawn({ cols: 80, rows: 24, sessionId: 'fresh-no-history' })
|
||||
|
||||
expect(requestSpy.mock.calls.map((call) => call[0])).not.toContain('getSize')
|
||||
})
|
||||
|
||||
it('recovers cold restore when the probed session dies before createOrAttach', async () => {
|
||||
const sessionId = 'probe-race-cold-restore'
|
||||
const sessionDir = join(historyDir, getHistorySessionDirName(sessionId))
|
||||
mkdirSync(sessionDir, { recursive: true })
|
||||
writeFileSync(
|
||||
join(sessionDir, 'meta.json'),
|
||||
JSON.stringify({
|
||||
cwd: '/projects/raced',
|
||||
cols: 100,
|
||||
rows: 30,
|
||||
startedAt: '2026-04-15T10:00:00Z',
|
||||
endedAt: null,
|
||||
exitCode: null
|
||||
})
|
||||
)
|
||||
writeFileSync(join(sessionDir, 'scrollback.bin'), 'raced output\r\n')
|
||||
|
||||
historyAdapter = new DaemonPtyAdapter({ socketPath, tokenPath, historyPath: historyDir })
|
||||
const client = (
|
||||
historyAdapter as unknown as {
|
||||
client: { request: (type: string, payload?: unknown) => Promise<unknown> }
|
||||
}
|
||||
).client
|
||||
const originalRequest = client.request.bind(client)
|
||||
// Why: simulates the probe→createOrAttach race — the probe sees the
|
||||
// session alive, but it is gone by the time createOrAttach runs. The
|
||||
// meta rewrite mimics the dying session's exit event beating the
|
||||
// createOrAttach reply and writing endedAt via closeSession; the
|
||||
// fallback detect must still restore instead of falling through to
|
||||
// openSession (which would delete the checkpoint).
|
||||
vi.spyOn(client, 'request').mockImplementation(async (type: string, payload?: unknown) => {
|
||||
if (type === 'getSize') {
|
||||
return { size: { cols: 100, rows: 30 } }
|
||||
}
|
||||
const response = await originalRequest(type, payload)
|
||||
if (type === 'createOrAttach') {
|
||||
writeFileSync(
|
||||
join(sessionDir, 'meta.json'),
|
||||
JSON.stringify({
|
||||
cwd: '/projects/raced',
|
||||
cols: 100,
|
||||
rows: 30,
|
||||
startedAt: '2026-04-15T10:00:00Z',
|
||||
endedAt: '2026-04-15T10:05:00Z',
|
||||
exitCode: 0
|
||||
})
|
||||
)
|
||||
}
|
||||
return response
|
||||
})
|
||||
|
||||
const result = await historyAdapter.spawn({ cols: 80, rows: 24, sessionId })
|
||||
|
||||
expect(result.coldRestore).toBeDefined()
|
||||
expect(result.coldRestore!.scrollback).toContain('raced output')
|
||||
// Documented race delta: the fresh shell spawns with the renderer's
|
||||
// requested params, not the recovered ones.
|
||||
expect(lastSpawnOpts).toMatchObject({ sessionId, cols: 80, rows: 24 })
|
||||
// The recovery data must survive — openSession would have deleted it.
|
||||
expect(existsSync(join(sessionDir, 'scrollback.bin'))).toBe(true)
|
||||
const internals = historyAdapter as unknown as {
|
||||
sessionsNeedingFullCheckpoint: Set<string>
|
||||
lastFullCheckpointAt: Map<string, number>
|
||||
}
|
||||
expect(internals.sessionsNeedingFullCheckpoint.has(sessionId)).toBe(true)
|
||||
expect(internals.lastFullCheckpointAt.has(sessionId)).toBe(false)
|
||||
})
|
||||
|
||||
it('falls back to the full cold-restore detect when the aliveness probe fails', async () => {
|
||||
const sessionId = 'probe-error-cold-restore'
|
||||
const sessionDir = join(historyDir, getHistorySessionDirName(sessionId))
|
||||
mkdirSync(sessionDir, { recursive: true })
|
||||
writeFileSync(
|
||||
join(sessionDir, 'meta.json'),
|
||||
JSON.stringify({
|
||||
cwd: '/projects/probeless',
|
||||
cols: 132,
|
||||
rows: 43,
|
||||
startedAt: '2026-04-15T10:00:00Z',
|
||||
endedAt: null,
|
||||
exitCode: null
|
||||
})
|
||||
)
|
||||
writeFileSync(join(sessionDir, 'scrollback.bin'), 'probeless output\r\n')
|
||||
|
||||
historyAdapter = new DaemonPtyAdapter({ socketPath, tokenPath, historyPath: historyDir })
|
||||
const client = (
|
||||
historyAdapter as unknown as {
|
||||
client: { request: (type: string, payload?: unknown) => Promise<unknown> }
|
||||
}
|
||||
).client
|
||||
const originalRequest = client.request.bind(client)
|
||||
// Why: an old daemon rejects the unknown getSize method; the spawn must
|
||||
// behave exactly like the unprobed path.
|
||||
vi.spyOn(client, 'request').mockImplementation((type: string, payload?: unknown) => {
|
||||
if (type === 'getSize') {
|
||||
return Promise.reject(new Error('Unknown request type'))
|
||||
}
|
||||
return originalRequest(type, payload)
|
||||
})
|
||||
|
||||
const result = await historyAdapter.spawn({ cols: 80, rows: 24, sessionId })
|
||||
|
||||
expect(result.coldRestore).toBeDefined()
|
||||
expect(result.coldRestore!.scrollback).toContain('probeless output')
|
||||
expect(lastSpawnOpts).toMatchObject({
|
||||
sessionId,
|
||||
cwd: '/projects/probeless',
|
||||
cols: 132,
|
||||
rows: 43
|
||||
})
|
||||
})
|
||||
|
||||
it('returns same cold restore on StrictMode double-mount (sticky cache)', async () => {
|
||||
const sessionId = 'sticky-cache-test'
|
||||
const sessionDir = join(historyDir, getHistorySessionDirName(sessionId))
|
||||
|
|
|
|||
|
|
@ -6,7 +6,7 @@ import { existsSync } from 'node:fs'
|
|||
import { DaemonClient } from './client'
|
||||
import { getMacDaemonSystemResolverHealth } from './daemon-health'
|
||||
import { HistoryManager } from './history-manager'
|
||||
import { HistoryReader } from './history-reader'
|
||||
import { HistoryReader, type ColdRestoreInfo } from './history-reader'
|
||||
import { mintPtySessionId, parsePtySessionId } from './pty-session-id'
|
||||
import { supportsPtyStartupBarrier } from './shell-ready'
|
||||
import { CODEX_SHELL_READY_TIMEOUT_MS } from './session'
|
||||
|
|
@ -141,16 +141,30 @@ export class DaemonPtyAdapter implements IPtyProvider {
|
|||
await this.replaceUnhealthyMacResolverDaemonBeforeNewPty()
|
||||
}
|
||||
|
||||
await this.ensureConnected()
|
||||
|
||||
// Why: detect crash-recovery history before spawning a replacement PTY so
|
||||
// the revived shell inherits the recovered cwd and dimensions instead of
|
||||
// whatever the current renderer happened to request on mount.
|
||||
const restoreInfo = this.historyReader?.detectColdRestore(sessionId) ?? null
|
||||
// Why probe aliveness first: detectColdRestore synchronously replays the
|
||||
// full checkpoint + log (up to ~5MB) through a scratch emulator on the
|
||||
// main process, but a live daemon session ignores spawn params and its
|
||||
// own snapshot supersedes disk — the replay result would be discarded.
|
||||
// getSize is a read-only probe; on error/unsupported it degrades to the
|
||||
// full detect.
|
||||
let restoreInfo: ColdRestoreInfo | null = null
|
||||
let restoreSkippedForLiveSession = false
|
||||
if (this.historyReader?.hasRestorableHistory(sessionId)) {
|
||||
if ((await this.getAppliedSize(sessionId)) !== null) {
|
||||
restoreSkippedForLiveSession = true
|
||||
} else {
|
||||
restoreInfo = this.historyReader.detectColdRestore(sessionId)
|
||||
}
|
||||
}
|
||||
const effectiveCwd = restoreInfo?.cwd ?? opts.cwd
|
||||
const effectiveCols = restoreInfo?.cols ?? opts.cols
|
||||
const effectiveRows = restoreInfo?.rows ?? opts.rows
|
||||
|
||||
await this.ensureConnected()
|
||||
|
||||
const shellReadySupported = opts.command ? supportsPtyStartupBarrier(opts.env ?? {}) : false
|
||||
const isCodexStartupCommand =
|
||||
recognizeAgentProcessFromCommandLine(opts.command)?.agent === 'codex'
|
||||
|
|
@ -208,6 +222,19 @@ export class DaemonPtyAdapter implements IPtyProvider {
|
|||
}
|
||||
}
|
||||
|
||||
// Why: the probe→createOrAttach gap is racy — the session can exit (or
|
||||
// enter termination) in between, so the daemon spawned a fresh shell.
|
||||
// Detect now so scrollback restore matches the unprobed path; only the
|
||||
// new shell's cwd/dims came from the renderer request in this rare case.
|
||||
// Why ignoreCleanEnd: the raced session's exit event (stream socket) can
|
||||
// beat the createOrAttach reply and write endedAt via closeSession; that
|
||||
// must not null the restore here, or the openSession branch below would
|
||||
// delete the checkpoint instead of restoring it.
|
||||
if (result.isNew && restoreSkippedForLiveSession) {
|
||||
restoreInfo =
|
||||
this.historyReader?.detectColdRestore(sessionId, { ignoreCleanEnd: true }) ?? null
|
||||
}
|
||||
|
||||
const wasAlreadyManaged = this.activeSessionIds.has(sessionId)
|
||||
this.activeSessionIds.add(sessionId)
|
||||
|
||||
|
|
|
|||
|
|
@ -28,12 +28,28 @@ export class HistoryReader {
|
|||
this.basePath = basePath
|
||||
}
|
||||
|
||||
detectColdRestore(sessionId: string): ColdRestoreInfo | null {
|
||||
// Why: spawn needs a cheap "could this cold-restore?" predicate before
|
||||
// deciding to pay detectColdRestore's full checkpoint+log replay. Reads only
|
||||
// the small meta.json, using the same unclean-shutdown test detectColdRestore
|
||||
// starts with.
|
||||
hasRestorableHistory(sessionId: string): boolean {
|
||||
const meta = this.readMeta(sessionId)
|
||||
return meta !== null && meta.endedAt === null
|
||||
}
|
||||
|
||||
detectColdRestore(
|
||||
sessionId: string,
|
||||
opts?: { ignoreCleanEnd?: boolean }
|
||||
): ColdRestoreInfo | null {
|
||||
const meta = this.readMeta(sessionId)
|
||||
if (!meta) {
|
||||
return null
|
||||
}
|
||||
if (meta.endedAt !== null) {
|
||||
// Why ignoreCleanEnd: in the spawn probe race, the dying session's exit
|
||||
// event can write endedAt between the aliveness probe and the post-spawn
|
||||
// fallback detect. The caller established restore eligibility before the
|
||||
// probe, so the just-written clean end must not downgrade the restore.
|
||||
if (meta.endedAt !== null && !opts?.ignoreCleanEnd) {
|
||||
return null
|
||||
}
|
||||
|
||||
|
|
|
|||
Loading…
Reference in New Issue