mirror of
https://github.com/stablyai/orca.git
synced 2026-10-21 00:03:46 +00:00
fix(daemon): hard shutdown barrier so idle exit cannot race a pre-hello socket
An accepted socket that had not completed hello was invisible to the client map: an idle-exit fired in that window could tear down the server while the late hello still registered a client and spawned a live PTY — which process.exit(0) would then silently kill once server.close() drained. The same untracked socket could also hold server.close() (and the idle-exit callback) open forever. shuttingDown is now set synchronously at the top of shutdown(): new connections and late hellos are destroyed, RPCs are refused, all accepted sockets (tracked or pre-hello) are destroyed, TerminalHost refuses createOrAttach after dispose, and a failed idle shutdown still exits instead of stranding a half-shut daemon. Co-authored-by: Orca <help@stably.ai>
This commit is contained in:
committed by
Brennan Benson
co-authored by
Orca
parent
cec34a66d3
commit
5fed9f1790
@@ -640,6 +640,49 @@ describe('DaemonServer', () => {
|
||||
expect(onIdleExit).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('destroys a pre-hello socket at idle exit instead of letting it hold the daemon open', async () => {
|
||||
// Why: sockets that never complete hello are invisible to the client
|
||||
// map. Without the shutdown barrier they would keep server.close() —
|
||||
// and therefore onIdleExit — pending forever (zombie daemon), or worse,
|
||||
// complete hello against a half-shut server.
|
||||
const { idleExited, onIdleExit } = await startServerWithIdleExit(200)
|
||||
|
||||
const rawSocket = connect(socketPath)
|
||||
const rawClosed = new Promise<void>((resolve) => rawSocket.once('close', resolve))
|
||||
rawSocket.on('error', () => {})
|
||||
|
||||
await idleExited
|
||||
await rawClosed
|
||||
|
||||
expect(onIdleExit).toHaveBeenCalledTimes(1)
|
||||
})
|
||||
|
||||
it('refuses hello and createOrAttach once shutdown has begun', async () => {
|
||||
const spawned: unknown[] = []
|
||||
server = new DaemonServer({
|
||||
socketPath,
|
||||
tokenPath,
|
||||
spawnSubprocess: () => {
|
||||
const subprocess = createMockSubprocess()
|
||||
spawned.push(subprocess)
|
||||
return subprocess
|
||||
}
|
||||
})
|
||||
await server.start()
|
||||
await server.shutdown()
|
||||
|
||||
const daemon = server as unknown as DaemonServerPrivate
|
||||
await expect(
|
||||
daemon.routeRequest('late-client', {
|
||||
id: 'req-late-1',
|
||||
type: 'createOrAttach',
|
||||
payload: { sessionId: 'late-session', cols: 80, rows: 24 }
|
||||
})
|
||||
).rejects.toThrow(/shutting down/)
|
||||
expect(spawned).toHaveLength(0)
|
||||
expect(daemon.clients.size).toBe(0)
|
||||
})
|
||||
|
||||
it('never fires while a session is alive and exits after the last session ends', async () => {
|
||||
const { idleExited, onIdleExit, lastSubprocess } = await startServerWithIdleExit(75)
|
||||
const daemon = server as unknown as DaemonServerPrivate
|
||||
|
||||
@@ -103,6 +103,12 @@ export class DaemonServer {
|
||||
private lastInputAtBySessionId = new Map<string, number>()
|
||||
private stopStreamBacklogProbe: () => void = () => {}
|
||||
private idleExit: DaemonIdleExit | null = null
|
||||
// Why: sockets accepted but not yet past hello are invisible to `clients`,
|
||||
// so shutdown could otherwise strand them: a late hello would resurrect a
|
||||
// half-shut server, and an open pre-hello socket would hold server.close()
|
||||
// (and thus idle exit) open forever.
|
||||
private openSockets = new Set<Socket>()
|
||||
private shuttingDown = false
|
||||
|
||||
// Why: main-process PTY IPC has the same recent-input bypass, but daemon
|
||||
// output reaches main only after this stream layer. Keeping the window here
|
||||
@@ -139,7 +145,16 @@ export class DaemonServer {
|
||||
isIdle: () => this.clients.size === 0 && this.host.sessionCount() === 0,
|
||||
onExpired: () => {
|
||||
console.warn('[daemon] No sessions and no clients for the idle grace period — exiting')
|
||||
void this.shutdown().then(() => onIdleExit())
|
||||
// Why: idleness was just verified, so exiting after a failed
|
||||
// shutdown cannot lose sessions — but staying alive would strand a
|
||||
// half-shut daemon that can never re-arm its idle countdown.
|
||||
void this.shutdown().then(
|
||||
() => onIdleExit(),
|
||||
(err) => {
|
||||
console.error('[daemon] Idle shutdown failed — exiting anyway:', err)
|
||||
onIdleExit()
|
||||
}
|
||||
)
|
||||
}
|
||||
})
|
||||
}
|
||||
@@ -173,6 +188,10 @@ export class DaemonServer {
|
||||
}
|
||||
|
||||
async shutdown(): Promise<void> {
|
||||
// Why: hard barrier, set synchronously before any teardown — after this,
|
||||
// no accepted socket may become a client and no RPC may create a session,
|
||||
// so shutdown can never race a live PTY into a half-shut server.
|
||||
this.shuttingDown = true
|
||||
this.stopStreamBacklogProbe()
|
||||
this.transientFactRelay.dispose()
|
||||
// Why: dispose before teardown mutates client/session counts, so the idle
|
||||
@@ -187,6 +206,13 @@ export class DaemonServer {
|
||||
}
|
||||
this.clients.clear()
|
||||
|
||||
// Why: pre-hello sockets are not in `clients`; leaving them open would
|
||||
// hold server.close() — and therefore the idle-exit callback — forever.
|
||||
for (const socket of this.openSockets) {
|
||||
socket.destroy()
|
||||
}
|
||||
this.openSockets.clear()
|
||||
|
||||
return new Promise<void>((resolve) => {
|
||||
if (this.server) {
|
||||
this.server.close(() => {
|
||||
@@ -203,6 +229,12 @@ export class DaemonServer {
|
||||
}
|
||||
|
||||
private handleConnection(socket: Socket): void {
|
||||
if (this.shuttingDown) {
|
||||
socket.destroy()
|
||||
return
|
||||
}
|
||||
this.openSockets.add(socket)
|
||||
socket.once('close', () => this.openSockets.delete(socket))
|
||||
// Why: clients can send multibyte prompt/input text split across socket
|
||||
// chunks; keep UTF-8 sequences intact before NDJSON parsing.
|
||||
const decoder = new StringDecoder('utf8')
|
||||
@@ -222,6 +254,13 @@ export class DaemonServer {
|
||||
msg: unknown,
|
||||
_parser: ReturnType<typeof createNdjsonParser>
|
||||
): void {
|
||||
// Why: a hello queued before shutdown destroyed its socket can still be
|
||||
// delivered in the same tick; accepting it would register a client on a
|
||||
// server whose host and listener are already torn down.
|
||||
if (this.shuttingDown) {
|
||||
socket.destroy()
|
||||
return
|
||||
}
|
||||
const hello = msg as HelloMessage
|
||||
if (hello.type !== 'hello') {
|
||||
this.log.log('client-hello-rejected', { reason: 'expected-hello' })
|
||||
@@ -359,6 +398,9 @@ export class DaemonServer {
|
||||
}
|
||||
|
||||
private async routeRequest(clientId: string, request: DaemonRequest): Promise<unknown> {
|
||||
if (this.shuttingDown) {
|
||||
throw new Error('Daemon server is shutting down')
|
||||
}
|
||||
const client = this.clients.get(clientId)
|
||||
|
||||
switch (request.type) {
|
||||
|
||||
@@ -429,6 +429,20 @@ describe('TerminalHost', () => {
|
||||
expect(lastSubprocess.dispose).toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('rejects createOrAttach after dispose without spawning a subprocess', async () => {
|
||||
host.dispose()
|
||||
|
||||
await expect(
|
||||
host.createOrAttach({
|
||||
sessionId: 'post-dispose',
|
||||
cols: 80,
|
||||
rows: 24,
|
||||
streamClient: { onData: vi.fn(), onExit: vi.fn() }
|
||||
})
|
||||
).rejects.toThrow(/disposed/)
|
||||
expect(spawnFn).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('releases held shell-ready marker prefixes before final checkpoint', async () => {
|
||||
host.dispose()
|
||||
const onFinalCheckpoint = vi.fn()
|
||||
|
||||
@@ -81,6 +81,7 @@ export class TerminalHost {
|
||||
private onFinalCheckpoint: TerminalHostOptions['onFinalCheckpoint']
|
||||
private maxTombstones: number
|
||||
private onSessionCountChange: TerminalHostOptions['onSessionCountChange']
|
||||
private disposed = false
|
||||
|
||||
constructor(opts: TerminalHostOptions) {
|
||||
this.spawnSubprocess = opts.spawnSubprocess
|
||||
@@ -103,6 +104,11 @@ export class TerminalHost {
|
||||
* already deliver them through shell launch arguments.
|
||||
*/
|
||||
async createOrAttach(opts: CreateOrAttachOptions): Promise<CreateOrAttachResult> {
|
||||
// Why: a request that slips in while the daemon is shutting down must not
|
||||
// spawn a PTY the dying process would silently take down with it.
|
||||
if (this.disposed) {
|
||||
throw new Error('TerminalHost is disposed')
|
||||
}
|
||||
const existing = this.sessions.get(opts.sessionId)
|
||||
|
||||
// Why: a session that has been asked to terminate (kill() called but the
|
||||
@@ -366,6 +372,7 @@ export class TerminalHost {
|
||||
}
|
||||
|
||||
dispose(): void {
|
||||
this.disposed = true
|
||||
// Why: write final checkpoints before killing sessions so graceful shutdown
|
||||
// has zero data loss. The checkpoint callback writes synchronously to disk.
|
||||
if (this.onFinalCheckpoint) {
|
||||
|
||||
Reference in New Issue
Block a user