mirror of
https://github.com/stablyai/orca.git
synced 2026-10-21 16:03:26 +00:00
fix(daemon): restore the demotion my shadow fix made unreachable
The review is right. Gating demotion on the absence of a sessionId assumed fresh spawns are anonymous, and they are not: every production fresh spawn mints an id before reaching the provider (ipc/pty.ts assigns spawnOptions.sessionId on all three paths). So the branch only ever ran in tests, and a daemon that recovered and wedged again kept every later terminal pointed at itself — each paying a hello timeout plus a full launcher re-classification, each failing anyway. That is the cost the demotion existed to remove, reintroduced while fixing something else. The mistake was treating one condition as two questions. Pinning protects THIS id, which the daemon may already have created before losing the reply, so a retry must never be answered locally under the same name. Demoting protects the NEXT terminal, which is a different session and cannot be shadowed by this one. They are independent, and both now happen. `attachOnly` is the honest discriminator for the second: an attach that names a session never reaches this router, so anything arriving here without it is a fresh terminal whatever id it carries. Both halves mutation-verified separately — restoring the sessionId guard fails three tests, dropping the pin fails two — including a test shaped like what ipc/pty.ts actually sends, which is what the old test suite never had.
This commit is contained in:
@@ -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)
|
||||
})
|
||||
})
|
||||
|
||||
@@ -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(
|
||||
|
||||
Reference in New Issue
Block a user