fix(daemon): only demote and only pin when the failure earns it

Both guards in the fresh-spawn catch were too broad, and narrowing them is the one
thing worth keeping from the restart-ownership branch.

Demotion fired on any spawn failure. A rejected cwd or a bad profile says nothing
about whether the daemon is reachable, and costing the whole session its daemon
persistence over one of those degrades terminals the daemon would have served fine.
It now requires the failure to look like an unreachable daemon.

Pinning fired on any failure too. The pin exists for a request that was sent and
whose answer was lost — that is the only shape that can hide a session the daemon
already created. A failure that never reached it created nothing, so pinning that
id would strand later attempts on a host holding nothing of theirs. It now requires
an error that could have been dispatched.

The distinction is sharper than the code it replaces: a hello timeout demotes but
does not pin, because a handshake that never completed cannot have created a
session. The old code pinned it anyway.

Test doubles now raise DaemonProtocolError rather than plain Errors, which is what
the client actually produces and what these predicates are written against. Both
narrowings mutation-verified.
This commit is contained in:
Neil
2026-08-13 00:18:46 -07:00
parent 063827d4b1
commit d1cb98a5ef
2 changed files with 76 additions and 17 deletions
@@ -4,6 +4,7 @@ import {
DegradedDaemonFreshSpawnRouter
} from './degraded-daemon-fresh-spawn-routing'
import type { IPtyProvider, PtySpawnResult } from '../providers/types'
import { DaemonProtocolError } from './daemon-errors'
function provider(id: string, spawn?: IPtyProvider['spawn']): IPtyProvider {
return {
@@ -56,7 +57,7 @@ describe('DegradedDaemonFreshSpawnRouter', () => {
// costs a hello timeout plus a full launcher re-classification — per terminal, for the rest
// of the session.
const wedged = vi.fn(async () => {
throw new Error('Hello response timed out')
throw new DaemonProtocolError('Hello response timed out')
})
const { router: r } = router({ probe: async () => true, currentSpawn: wedged })
@@ -71,7 +72,7 @@ describe('DegradedDaemonFreshSpawnRouter', () => {
// Without re-arming the cooldown the next spawn probes again straight away, and a wedged
// daemon that still passes a cheap health check would be re-promoted into the same failure.
const wedged = vi.fn(async () => {
throw new Error('Hello response timed out')
throw new DaemonProtocolError('Hello response timed out')
})
const { router: r } = router({ probe: async () => true, currentSpawn: wedged })
@@ -98,7 +99,7 @@ describe('DegradedDaemonFreshSpawnRouter', () => {
// agent keeps running on the daemon — the pane binds to the shell and the agent is orphaned.
// That is the symptom this whole change exists to prevent, arriving by another door.
const lostReply = vi.fn(async () => {
throw new Error('Request listSessions timed out after 30000ms')
throw new DaemonProtocolError('Request createSession timed out after 30000ms')
})
const {
router: r,
@@ -118,7 +119,7 @@ describe('DegradedDaemonFreshSpawnRouter', () => {
// property that actually prevents the shadow: the pin outranks the route.
expect(r.routesToFallback).toBe(true)
await expect(r.spawn({ cwd: '/tmp', sessionId: 'wt-1@@pane-1' } as never)).rejects.toThrow(
'Request listSessions timed out after 30000ms'
'Request createSession timed out after 30000ms'
)
expect(lostReply).toHaveBeenCalledTimes(2)
})
@@ -130,13 +131,9 @@ describe('DegradedDaemonFreshSpawnRouter', () => {
// daemon would keep every later terminal pointed at itself, each paying a hello timeout
// plus a full launcher re-classification, and each failing anyway.
const wedged = vi.fn(async () => {
throw new Error('Hello response timed out')
throw new DaemonProtocolError('Hello response timed out')
})
const {
router: r,
current,
sessionProviders
} = router({
const { router: r, sessionProviders } = router({
probe: async () => true,
currentSpawn: wedged
})
@@ -150,15 +147,17 @@ describe('DegradedDaemonFreshSpawnRouter', () => {
)
expect(r.routesToFallback).toBe(true)
// ...without giving up the pin that stops this id being answered locally.
expect(sessionProviders.get('wt-1@@pane-9')).toBe(current)
// And correctly does NOT pin: a hello that never completed cannot have created a session,
// so there is nothing on the daemon for a retry to collide with. Pinning here would strand
// later attempts on a host holding nothing of theirs.
expect(sessionProviders.has('wt-1@@pane-9')).toBe(false)
})
it('does not demote for an attach that names a session', async () => {
// An attachOnly spawn is not a fresh terminal; its failure says nothing about whether the
// next new terminal should go local, and attaches with an id are routed elsewhere anyway.
const wedged = vi.fn(async () => {
throw new Error('Hello response timed out')
throw new DaemonProtocolError('Hello response timed out')
})
const { router: r } = router({ probe: async () => true, currentSpawn: wedged })
@@ -170,11 +169,42 @@ describe('DegradedDaemonFreshSpawnRouter', () => {
expect(r.routesToFallback).toBeUndefined()
})
it('does not demote when the failure says nothing about the daemon', async () => {
// A spawn can fail for reasons that are the caller's, not the host's — an unusable cwd, a
// bad profile. Degrading the whole session's persistence for one of those costs the user
// daemon-backed terminals the daemon would have served perfectly well.
const rejected = vi.fn(async () => {
throw new Error('chdir failed: ENOENT /gone')
})
const { router: r } = router({ probe: async () => true, currentSpawn: rejected })
await r.recover()
await expect(r.spawn({ cwd: '/gone' } as never)).rejects.toThrow('chdir failed')
expect(r.routesToFallback).toBeUndefined()
})
it('does not pin a session the daemon cannot have created', async () => {
// The pin exists for a request that was sent and whose answer was lost. A failure that never
// reached the daemon created nothing, so pinning it would strand later attempts on a host
// that has nothing of theirs.
const rejected = vi.fn(async () => {
throw new Error('chdir failed: ENOENT /gone')
})
const { router: r, sessionProviders } = router({
probe: async () => true,
currentSpawn: rejected
})
await r.recover()
await expect(r.spawn({ cwd: '/gone', sessionId: 'wt-1@@pane-3' } as never)).rejects.toThrow()
expect(sessionProviders.has('wt-1@@pane-3')).toBe(false)
})
it('still demotes on an anonymous spawn, which cannot be shadowed', async () => {
// The case demotion exists for: no session identity, so there is nothing a fallback answer
// could shadow, and paying a hello timeout plus a re-classification per terminal is pure loss.
const wedged = vi.fn(async () => {
throw new Error('Hello response timed out')
throw new DaemonProtocolError('Hello response timed out')
})
const { router: r } = router({ probe: async () => true, currentSpawn: wedged })
@@ -189,7 +219,7 @@ describe('DegradedDaemonFreshSpawnRouter', () => {
// and the next *fresh* terminal is a different session that cannot be shadowed by this one —
// so it should not have to discover the same timeout for itself.
const wedged = vi.fn(async () => {
throw new Error('Hello response timed out')
throw new DaemonProtocolError('Hello response timed out')
})
const {
router: r,
@@ -1,4 +1,33 @@
import type { IPtyProvider, PtySpawnOptions, PtySpawnResult } from '../providers/types'
import { isDaemonGoneError } from './daemon-pty-adapter'
import { DaemonProtocolError } from './daemon-errors'
/** client.ts rejects a sent request with this shape once its budget expires. */
const REQUEST_TIMED_OUT = /timed out after \d+ms/
/**
* Only a daemon that looks unreachable should cost the next terminal its persistence. A spawn
* can fail for reasons that say nothing about the daemon's health — an unusable cwd, a bad
* profile — and demoting on those degrades a session the daemon would have served fine.
*/
function daemonLooksUnreachable(error: unknown): boolean {
return (
isDaemonGoneError(error) ||
(error instanceof DaemonProtocolError && REQUEST_TIMED_OUT.test(error.message))
)
}
/**
* Only a request that was actually sent can hide a session the daemon created before the answer
* was lost. A failure that never reached it cannot have created anything, so pinning that id
* would strand later attempts on a daemon that has nothing of theirs.
*/
function mayHaveCreatedTheSession(error: unknown): boolean {
return (
error instanceof DaemonProtocolError &&
(error.message === 'Connection lost' || REQUEST_TIMED_OUT.test(error.message))
)
}
export const DEGRADED_DAEMON_RECOVERY_RETRY_MS = 30_000
@@ -83,7 +112,7 @@ export class DegradedDaemonFreshSpawnRouter {
// letting a retry reach the fallback would answer with a local shell under the same id
// while the original keeps running. Demoting protects the NEXT terminal, which is a
// different session entirely and cannot be shadowed by this one.
if (opts.sessionId) {
if (opts.sessionId && mayHaveCreatedTheSession(error)) {
this.sessionProviders.set(opts.sessionId, target)
}
// Why not `!opts.sessionId`: every production fresh spawn mints an id before it gets
@@ -92,7 +121,7 @@ export class DegradedDaemonFreshSpawnRouter {
// paying a hello timeout plus a full re-classification against a daemon already known
// to be failing. `attachOnly` is the real discriminator: an attach that names a session
// never reaches this router at all.
if (opts.attachOnly !== true) {
if (opts.attachOnly !== true && daemonLooksUnreachable(error)) {
this.target = this.fallback
this.retryAfterMs = Date.now() + DEGRADED_DAEMON_RECOVERY_RETRY_MS
console.warn(