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 9ed124229eb..70a6292e091 100644 --- a/src/main/daemon/degraded-daemon-fresh-spawn-routing.test.ts +++ b/src/main/daemon/degraded-daemon-fresh-spawn-routing.test.ts @@ -114,7 +114,59 @@ describe('DegradedDaemonFreshSpawnRouter', () => { // The identity sticks to the provider that may already own it... expect(sessionProviders.get('wt-1@@pane-1')).toBe(current) - // ...and the shared route is untouched, so this failure cannot silently divert other spawns. + // ...and keeps routing there even though the shared route has since demoted, which is the + // 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' + ) + expect(lostReply).toHaveBeenCalledTimes(2) + }) + + it('demotes for a production-shaped fresh spawn, which always carries an id', async () => { + // The regression this pins: gating demotion on the ABSENCE of a sessionId made it + // unreachable outside tests, because every production fresh spawn mints an id before + // reaching the provider (ipc/pty.ts sets spawnOptions.sessionId). A recovered-then-wedged + // 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') + }) + const { + router: r, + current, + sessionProviders + } = router({ + probe: async () => true, + currentSpawn: wedged + }) + + await r.recover() + expect(r.routesToFallback).toBeUndefined() + + // Exactly what ipc/pty.ts sends for a new terminal: a minted id, and no attachOnly. + await expect(r.spawn({ cwd: '/tmp', sessionId: 'wt-1@@pane-9' } as never)).rejects.toThrow( + 'Hello response timed out' + ) + + 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) + }) + + 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') + }) + const { router: r } = router({ probe: async () => true, currentSpawn: wedged }) + + await r.recover() + await expect( + r.spawn({ cwd: '/tmp', sessionId: 'wt-1@@pane-9', attachOnly: true } as never) + ).rejects.toThrow() + expect(r.routesToFallback).toBeUndefined() }) @@ -131,9 +183,11 @@ describe('DegradedDaemonFreshSpawnRouter', () => { expect(r.routesToFallback).toBe(true) }) - it('leaves an explicitly mapped session routed where it is', async () => { - // A mapped id names the provider that actually owns that pty. Rerouting on its failure would - // send a daemon-owned session to a provider that does not have it. + it('keeps a mapped session on its owner while sparing the next terminal', async () => { + // A mapped id names the provider that actually owns that pty, so it must keep routing there + // however the shared route moves. But its failure is still evidence the daemon is failing, + // 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') }) @@ -151,6 +205,7 @@ describe('DegradedDaemonFreshSpawnRouter', () => { expect(r.routesToFallback).toBeUndefined() await expect(r.spawn({ cwd: '/tmp', sessionId: 'session-1' } as never)).rejects.toThrow() - expect(r.routesToFallback).toBeUndefined() + expect(sessionProviders.get('session-1')).toBe(current) + expect(r.routesToFallback).toBe(true) }) }) diff --git a/src/main/daemon/degraded-daemon-fresh-spawn-routing.ts b/src/main/daemon/degraded-daemon-fresh-spawn-routing.ts index 135d7fc1f1a..accef808d86 100644 --- a/src/main/daemon/degraded-daemon-fresh-spawn-routing.ts +++ b/src/main/daemon/degraded-daemon-fresh-spawn-routing.ts @@ -78,16 +78,21 @@ export class DegradedDaemonFreshSpawnRouter { // for the rest of the session. Sending the next one to the fallback costs a terminal // without daemon persistence instead, and the next probe can promote it back. if (target === this.current) { + // Two independent things, and conflating them cost a fix each way. Pinning protects + // THIS id: the spawn may already have created it on the daemon and lost the reply, so + // 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) { - // Why pin instead of demote: a spawn that names a session may already have created it - // on the daemon — the reply can time out after the daemon has acted. Demoting would - // send the retry to the fallback, which would answer with a fresh local shell under - // the same id while the original keeps running, and the pane would bind to the shell. - // `!mapped` cannot tell that apart from a genuinely new session, because the mapping - // is only recorded after a reply arrives. So the identity sticks to the provider that - // may own it, and only anonymous spawns move the shared route. this.sessionProviders.set(opts.sessionId, target) - } else { + } + // Why not `!opts.sessionId`: every production fresh spawn mints an id before it gets + // here (ipc/pty.ts assigns spawnOptions.sessionId), so keying the demotion off its + // absence made the demotion unreachable outside tests — and left every later terminal + // 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) { this.target = this.fallback this.retryAfterMs = Date.now() + DEGRADED_DAEMON_RECOVERY_RETRY_MS console.warn(