mirror of
https://github.com/stablyai/orca
synced 2026-04-21 14:17:16 +00:00
fix(terminal): coalesce concurrent daemon respawns with a lock
Addresses review feedback: if multiple spawn() calls detect daemon death simultaneously, only the first forks a new daemon — the rest await the same promise. Also adds tests for concurrent respawn coalescing and respawn failure propagation.
This commit is contained in:
parent
f6d5f62e60
commit
ea8f5249f4
2 changed files with 67 additions and 5 deletions
|
|
@ -686,5 +686,53 @@ describe('DaemonPtyAdapter (IPtyProvider)', () => {
|
|||
|
||||
noRespawnAdapter.dispose()
|
||||
})
|
||||
|
||||
it('coalesces concurrent respawns so only one daemon is forked', async () => {
|
||||
let respawnServer: DaemonServer | undefined
|
||||
const respawnFn = vi.fn(async () => {
|
||||
respawnServer = new DaemonServer({
|
||||
socketPath,
|
||||
tokenPath,
|
||||
spawnSubprocess: () => createMockSubprocess()
|
||||
})
|
||||
await respawnServer.start()
|
||||
})
|
||||
|
||||
const respawnAdapter = new DaemonPtyAdapter({ socketPath, tokenPath, respawn: respawnFn })
|
||||
|
||||
// First spawn connects
|
||||
await respawnAdapter.spawn({ cols: 80, rows: 24 })
|
||||
|
||||
// Kill daemon
|
||||
await server.shutdown()
|
||||
|
||||
// Fire two spawns concurrently — both should succeed but only one respawn
|
||||
const [r1, r2] = await Promise.all([
|
||||
respawnAdapter.spawn({ cols: 80, rows: 24 }),
|
||||
respawnAdapter.spawn({ cols: 80, rows: 24 })
|
||||
])
|
||||
expect(r1.id).toBeDefined()
|
||||
expect(r2.id).toBeDefined()
|
||||
expect(respawnFn).toHaveBeenCalledOnce()
|
||||
|
||||
respawnAdapter.dispose()
|
||||
await respawnServer?.shutdown()
|
||||
})
|
||||
|
||||
it('propagates respawn failure to the caller', async () => {
|
||||
const respawnFn = vi.fn(async () => {
|
||||
throw new Error('Daemon entry file missing')
|
||||
})
|
||||
|
||||
const respawnAdapter = new DaemonPtyAdapter({ socketPath, tokenPath, respawn: respawnFn })
|
||||
await respawnAdapter.spawn({ cols: 80, rows: 24 })
|
||||
await server.shutdown()
|
||||
|
||||
await expect(respawnAdapter.spawn({ cols: 80, rows: 24 })).rejects.toThrow(
|
||||
'Daemon entry file missing'
|
||||
)
|
||||
|
||||
respawnAdapter.dispose()
|
||||
})
|
||||
})
|
||||
})
|
||||
|
|
|
|||
|
|
@ -36,6 +36,11 @@ export class DaemonPtyAdapter implements IPtyProvider {
|
|||
private historyManager: HistoryManager | null
|
||||
private historyReader: HistoryReader | null
|
||||
private respawnFn: (() => Promise<void>) | null
|
||||
// Why: multiple pane mounts can call spawn() concurrently. If the daemon is
|
||||
// dead, all calls enter withDaemonRetry's catch block at once. Without a
|
||||
// lock, each would fork its own daemon process. This promise coalesces
|
||||
// concurrent respawns so only the first caller forks; the rest await it.
|
||||
private respawnPromise: Promise<void> | null = null
|
||||
private dataListeners: ((payload: { id: string; data: string }) => void)[] = []
|
||||
private exitListeners: ((payload: { id: string; code: number }) => void)[] = []
|
||||
private removeEventListener: (() => void) | null = null
|
||||
|
|
@ -388,15 +393,24 @@ export class DaemonPtyAdapter implements IPtyProvider {
|
|||
if (!this.respawnFn || !isDaemonGoneError(err)) {
|
||||
throw err
|
||||
}
|
||||
console.warn('[daemon] Operation failed, respawning:', (err as Error).message)
|
||||
this.removeEventListener?.()
|
||||
this.removeEventListener = null
|
||||
this.client.disconnect()
|
||||
await this.respawnFn()
|
||||
if (!this.respawnPromise) {
|
||||
this.respawnPromise = this.doRespawn().finally(() => {
|
||||
this.respawnPromise = null
|
||||
})
|
||||
}
|
||||
await this.respawnPromise
|
||||
return await fn()
|
||||
}
|
||||
}
|
||||
|
||||
private async doRespawn(): Promise<void> {
|
||||
console.warn('[daemon] Daemon died — respawning')
|
||||
this.removeEventListener?.()
|
||||
this.removeEventListener = null
|
||||
this.client.disconnect()
|
||||
await this.respawnFn!()
|
||||
}
|
||||
|
||||
private setupEventRouting(): void {
|
||||
if (this.removeEventListener) {
|
||||
return
|
||||
|
|
|
|||
Loading…
Reference in a new issue