diff --git a/src/main/daemon/degraded-daemon-fresh-spawn-routing.test.ts b/src/main/daemon/degraded-daemon-fresh-spawn-routing.test.ts index 70a6292e091..6044957696c 100644 --- a/src/main/daemon/degraded-daemon-fresh-spawn-routing.test.ts +++ b/src/main/daemon/degraded-daemon-fresh-spawn-routing.test.ts @@ -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, diff --git a/src/main/daemon/degraded-daemon-fresh-spawn-routing.ts b/src/main/daemon/degraded-daemon-fresh-spawn-routing.ts index accef808d86..6b524219bc2 100644 --- a/src/main/daemon/degraded-daemon-fresh-spawn-routing.ts +++ b/src/main/daemon/degraded-daemon-fresh-spawn-routing.ts @@ -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(