mirror of
https://github.com/stablyai/orca.git
synced 2026-09-22 00:02:31 +00:00
fix(pty): make the unproven-absence veto transient and route attaches to the real owner
Review found the proven-absence gate correct in direction but terminal in effect: "probe says alive" and "probe says unknown" both became a hard spawn failure with no consumer, no retry, and a raw internal token in a toast. - Graceful teardown (should-fix): a session being killed reports alive (getAppliedSize answers while isAlive, and isTerminating does not flip _state) while createOrAttach already throws SessionNotFoundError, so for up to KILL_TIMEOUT_MS the pane failed instead of retiring the doomed binding. The veto now re-proves on a bounded 6s budget: the moment any owner answers "absent" the pane retires and spawns fresh exactly as before. - Unprovable owners (should-fix): DaemonPtyAdapter.probePtyLiveness never called ensureConnected, so a merely disconnected adapter answered null forever and one null poisons probePtyOwners; it also took no deadline, so a wedged daemon burned the client's 30s request timeout per attach. It now connects first (like listProcesses) and threads an absolute deadline through the fan-out. The thrown error is user-legible English instead of terminal_pane_owner_unverified. - Durability barrier (should-fix): flushPendingOrThrowAsync is not a twin of flushOrThrow — it defaults to the drain-to-stable-generation loop the sibling best-effort caller deliberately avoids, adds active-view and GitHub sidecar writes, re-serializes state per iteration, and rejects when writes are frozen where the sync path no-ops. Reverted to flushOrThrow, matching the sibling retire paths in orca-runtime. - Misrouting root cause (nit): DegradedDaemonFreshSpawnRouter.spawn sent an unmapped sessionId to the local fallback and DaemonPtyRouter.spawn sent it to the current daemon, both faking "Session not found" for a live session. Both now resolve an existing owner first (the same resolution every non-spawn path already uses); minted ids still route to the fallback/current daemon. The pane reattaches instead of only surviving. Tests: retry-until-proven, teardown-window retire, misroute-then-reattach, probe deadline threading, unconnected-adapter probe, and both routers' owner resolution. Each fails with only the source reverted.
This commit is contained in:
@@ -1202,6 +1202,35 @@ describe('DaemonPtyAdapter (IPtyProvider)', () => {
|
||||
|
||||
await expect(adapter.probePtyLiveness('session')).resolves.toBeNull()
|
||||
})
|
||||
|
||||
it('connects first so an unconnected adapter still answers authoritatively', async () => {
|
||||
// Why it matters: one unknown poisons the owner fan-out, and an unprovable owner
|
||||
// leaves the pane's binding unretireable — a never-connected adapter must not be it.
|
||||
const { id } = await adapter.spawn({ cols: 80, rows: 24 })
|
||||
const unconnected = new DaemonPtyAdapter({ socketPath, tokenPath })
|
||||
try {
|
||||
await expect(unconnected.probePtyLiveness(id)).resolves.toBe(true)
|
||||
await expect(unconnected.probePtyLiveness('missing-session')).resolves.toBe(false)
|
||||
} finally {
|
||||
unconnected.dispose()
|
||||
}
|
||||
})
|
||||
|
||||
it('bounds the probe request by the caller deadline', async () => {
|
||||
// Why: unbounded, a wedged daemon answers `null` only after the client's 30s default,
|
||||
// stalling the attach that is waiting on the answer.
|
||||
const { id } = await adapter.spawn({ cols: 80, rows: 24 })
|
||||
const client = (adapter as unknown as { client: DaemonClient }).client
|
||||
const request = vi.spyOn(client, 'request')
|
||||
|
||||
await expect(adapter.probePtyLiveness(id, { deadlineMs: Date.now() + 750 })).resolves.toBe(
|
||||
true
|
||||
)
|
||||
|
||||
const timeoutMs = request.mock.calls.at(-1)?.[2]
|
||||
expect(timeoutMs).toBeGreaterThan(0)
|
||||
expect(timeoutMs).toBeLessThanOrEqual(750)
|
||||
})
|
||||
})
|
||||
|
||||
describe('getBufferSnapshot', () => {
|
||||
|
||||
@@ -896,11 +896,15 @@ export class DaemonPtyAdapter implements IPtyProvider {
|
||||
return this.activeSessionIds.has(id)
|
||||
}
|
||||
|
||||
async probePtyLiveness(id: string): Promise<boolean | null> {
|
||||
async probePtyLiveness(id: string, opts?: { deadlineMs?: number }): Promise<boolean | null> {
|
||||
try {
|
||||
// Why connect first (like listProcesses): a merely disconnected adapter would otherwise
|
||||
// answer `null` forever, and one unknown poisons the owner fan-out into "unprovable".
|
||||
await this.ensureConnected(opts?.deadlineMs)
|
||||
const result = await this.client.request<{ size: { cols: number; rows: number } | null }>(
|
||||
'getSize',
|
||||
{ sessionId: id }
|
||||
{ sessionId: id },
|
||||
remainingRequestTimeoutMs(opts?.deadlineMs)
|
||||
)
|
||||
return result.size !== null
|
||||
} catch {
|
||||
|
||||
@@ -4,14 +4,17 @@ import type { DaemonPtyAdapter } from './daemon-pty-adapter'
|
||||
export async function probePtyOwners(
|
||||
id: string,
|
||||
routed: IPtyProvider | undefined,
|
||||
possibleOwners: readonly DaemonPtyAdapter[]
|
||||
possibleOwners: readonly DaemonPtyAdapter[],
|
||||
opts?: { deadlineMs?: number }
|
||||
): Promise<boolean | null> {
|
||||
if (routed) {
|
||||
return routed.probePtyLiveness
|
||||
? await routed.probePtyLiveness(id)
|
||||
? await routed.probePtyLiveness(id, opts)
|
||||
: (routed.hasPty?.(id) ?? null)
|
||||
}
|
||||
const results = await Promise.all(possibleOwners.map((provider) => provider.probePtyLiveness(id)))
|
||||
const results = await Promise.all(
|
||||
possibleOwners.map((provider) => provider.probePtyLiveness(id, opts))
|
||||
)
|
||||
return results.some((result) => result === true)
|
||||
? true
|
||||
: results.every((result) => result === false)
|
||||
|
||||
@@ -146,6 +146,12 @@ function createAdapter(
|
||||
}
|
||||
},
|
||||
emitExit: (id: string, code: number, incarnationId?: string) => {
|
||||
// Mirrors DaemonPtyAdapter: the exit event drops the id from activeSessionIds, so
|
||||
// hasPty stops claiming it.
|
||||
const idx = sessions.indexOf(id)
|
||||
if (idx !== -1) {
|
||||
sessions.splice(idx, 1)
|
||||
}
|
||||
for (const listener of exitListeners) {
|
||||
listener({ id, code, ...(incarnationId ? { incarnationId } : {}) })
|
||||
}
|
||||
@@ -478,8 +484,56 @@ describe('DaemonPtyRouter', () => {
|
||||
const router = new DaemonPtyRouter({ current, legacy: [legacy] })
|
||||
|
||||
await expect(router.probePtyLiveness('surviving-session')).resolves.toBe(true)
|
||||
expect(current.probePtyLiveness).toHaveBeenCalledExactlyOnceWith('surviving-session')
|
||||
expect(legacy.probePtyLiveness).toHaveBeenCalledExactlyOnceWith('surviving-session')
|
||||
expect(current.probePtyLiveness).toHaveBeenCalledExactlyOnceWith('surviving-session', undefined)
|
||||
expect(legacy.probePtyLiveness).toHaveBeenCalledExactlyOnceWith('surviving-session', undefined)
|
||||
})
|
||||
|
||||
it('shares one caller deadline across every probed owner', async () => {
|
||||
// Why: an unbounded probe burns the client's 30s request timeout per wedged owner,
|
||||
// stalling the attach that is waiting on the answer.
|
||||
const current = createAdapter('current')
|
||||
const legacy = createAdapter('legacy')
|
||||
const router = new DaemonPtyRouter({ current, legacy: [legacy] })
|
||||
|
||||
await router.probePtyLiveness('unmapped-session', { deadlineMs: 4_242 })
|
||||
|
||||
expect(current.probePtyLiveness).toHaveBeenCalledWith('unmapped-session', {
|
||||
deadlineMs: 4_242
|
||||
})
|
||||
expect(legacy.probePtyLiveness).toHaveBeenCalledWith('unmapped-session', { deadlineMs: 4_242 })
|
||||
})
|
||||
|
||||
it('routes an undiscovered existing session to the adapter that still owns it', async () => {
|
||||
// Why: legacy discovery is fail-open, so an unmapped id can still be live on an old
|
||||
// daemon; sending its attach to the current daemon fakes "Session not found".
|
||||
const current = createAdapter('current')
|
||||
const legacy = createAdapter('legacy', ['undiscovered-legacy-session'])
|
||||
const warn = vi.spyOn(console, 'warn').mockImplementation(() => {})
|
||||
vi.mocked(legacy.listProcesses).mockRejectedValueOnce(new Error('legacy discovery failed'))
|
||||
const router = new DaemonPtyRouter({ current, legacy: [legacy] })
|
||||
|
||||
await router.discoverLegacySessions()
|
||||
await router.spawn({
|
||||
sessionId: 'undiscovered-legacy-session',
|
||||
attachOnly: true,
|
||||
cols: 80,
|
||||
rows: 24
|
||||
})
|
||||
await router.spawn({ sessionId: 'minted-session', cols: 80, rows: 24 })
|
||||
|
||||
expect(legacy.spawn).toHaveBeenCalledWith({
|
||||
sessionId: 'undiscovered-legacy-session',
|
||||
attachOnly: true,
|
||||
cols: 80,
|
||||
rows: 24
|
||||
})
|
||||
// A minted id nobody owns still routes to the current daemon.
|
||||
expect(current.spawn).toHaveBeenCalledWith({
|
||||
sessionId: 'minted-session',
|
||||
cols: 80,
|
||||
rows: 24
|
||||
})
|
||||
warn.mockRestore()
|
||||
})
|
||||
|
||||
it('does not report absence while any possible daemon owner is unavailable', async () => {
|
||||
|
||||
@@ -41,7 +41,14 @@ export class DaemonPtyRouter implements IPtyProvider {
|
||||
}
|
||||
|
||||
async spawn(opts: PtySpawnOptions): Promise<PtySpawnResult> {
|
||||
const adapter = opts.sessionId ? this.sessionAdapters.get(opts.sessionId) : undefined
|
||||
// Why the hasPty scan: an id the map lost (legacy discovery failed) still has a live owner,
|
||||
// and sending it to the current daemon makes that owner read as "Session not found".
|
||||
// Fresh ids match no adapter and still route to current.
|
||||
const { sessionId } = opts
|
||||
const adapter = sessionId
|
||||
? (this.sessionAdapters.get(sessionId) ??
|
||||
this.allAdapters().find((candidate) => candidate.hasPty(sessionId)))
|
||||
: undefined
|
||||
const target = adapter ?? this.current
|
||||
const result = await target.spawn(opts)
|
||||
// Why: the adapter filters intentional recovery exits and canonical-ID races before publishing proof.
|
||||
@@ -85,8 +92,8 @@ export class DaemonPtyRouter implements IPtyProvider {
|
||||
return this.current.hasPty(id) || this.legacy.some((adapter) => adapter.hasPty(id))
|
||||
}
|
||||
|
||||
async probePtyLiveness(id: string): Promise<boolean | null> {
|
||||
return await probePtyOwners(id, this.sessionAdapters.get(id), this.allAdapters())
|
||||
async probePtyLiveness(id: string, opts?: { deadlineMs?: number }): Promise<boolean | null> {
|
||||
return await probePtyOwners(id, this.sessionAdapters.get(id), this.allAdapters(), opts)
|
||||
}
|
||||
|
||||
write(id: string, data: string): void {
|
||||
|
||||
@@ -11,7 +11,9 @@ export class DegradedDaemonFreshSpawnRouter {
|
||||
private readonly current: IPtyProvider,
|
||||
private readonly fallback: IPtyProvider,
|
||||
private readonly sessionProviders: Map<string, IPtyProvider>,
|
||||
private readonly probeCurrent: (() => Promise<boolean>) | null
|
||||
private readonly probeCurrent: (() => Promise<boolean>) | null,
|
||||
private readonly findExistingSessionProvider: (sessionId: string) => IPtyProvider | null = () =>
|
||||
null
|
||||
) {
|
||||
this.target = fallback
|
||||
}
|
||||
@@ -66,7 +68,15 @@ export class DegradedDaemonFreshSpawnRouter {
|
||||
}
|
||||
|
||||
async spawn(opts: PtySpawnOptions): Promise<PtySpawnResult> {
|
||||
const mapped = opts.sessionId ? this.sessionProviders.get(opts.sessionId) : undefined
|
||||
// Why the second lookup: an id the map lost (discovery failed, or it never ran) still has a
|
||||
// real owner. Sending it to the fallback makes a live daemon session read as "Session not
|
||||
// found", which is how a running agent's pane got retired. Every non-spawn path already
|
||||
// resolves it this way; fresh ids match nobody and still route to the fallback.
|
||||
const mapped = opts.sessionId
|
||||
? (this.sessionProviders.get(opts.sessionId) ??
|
||||
this.findExistingSessionProvider(opts.sessionId) ??
|
||||
undefined)
|
||||
: undefined
|
||||
const target = mapped ?? this.target
|
||||
const result = await target.spawn(opts)
|
||||
if (!result.exitedBeforeSpawnReply) {
|
||||
|
||||
@@ -97,6 +97,12 @@ function createProvider(
|
||||
}
|
||||
},
|
||||
emitExit: (id: string, code: number) => {
|
||||
// Mirrors DaemonPtyAdapter: the exit event drops the id from activeSessionIds, so
|
||||
// hasPty stops claiming it.
|
||||
const idx = sessions.indexOf(id)
|
||||
if (idx !== -1) {
|
||||
sessions.splice(idx, 1)
|
||||
}
|
||||
for (const listener of exitListeners) {
|
||||
listener({ id, code })
|
||||
}
|
||||
@@ -231,6 +237,41 @@ describe('DegradedDaemonPtyProvider', () => {
|
||||
expect(fallback.write).toHaveBeenCalledWith(fresh.id, 'new\n')
|
||||
})
|
||||
|
||||
it('routes an undiscovered existing session to its owner instead of the fallback', async () => {
|
||||
// Why: discovery is fail-open (a listProcesses failure just warns), so a live daemon
|
||||
// session can be unmapped. Sending its attach to the fallback makes a running agent
|
||||
// answer "Session not found", which is how its pane got retired.
|
||||
const current = createDaemonAdapter('daemon', ['undiscovered-session'])
|
||||
const legacy = createDaemonAdapter('legacy', ['legacy-session'])
|
||||
const fallback = createProvider('fallback')
|
||||
const provider = new DegradedDaemonPtyProvider({ current, legacy: [legacy], fallback })
|
||||
|
||||
await provider.spawn({
|
||||
sessionId: 'undiscovered-session',
|
||||
attachOnly: true,
|
||||
cols: 80,
|
||||
rows: 24
|
||||
})
|
||||
await provider.spawn({ sessionId: 'legacy-session', attachOnly: true, cols: 80, rows: 24 })
|
||||
const fresh = await provider.spawn({ sessionId: 'minted-session', cols: 80, rows: 24 })
|
||||
|
||||
expect(current.spawn).toHaveBeenCalledWith({
|
||||
sessionId: 'undiscovered-session',
|
||||
attachOnly: true,
|
||||
cols: 80,
|
||||
rows: 24
|
||||
})
|
||||
expect(legacy.spawn).toHaveBeenCalledWith({
|
||||
sessionId: 'legacy-session',
|
||||
attachOnly: true,
|
||||
cols: 80,
|
||||
rows: 24
|
||||
})
|
||||
// A minted id nobody owns still routes to the degraded fallback.
|
||||
expect(fallback.spawn).toHaveBeenCalledWith({ sessionId: 'minted-session', cols: 80, rows: 24 })
|
||||
expect(fresh.id).toBe('minted-session')
|
||||
})
|
||||
|
||||
it('routes later fresh PTYs to the daemon after spawn health recovers', async () => {
|
||||
const current = createDaemonAdapter('daemon')
|
||||
const fallback = createProvider('fallback')
|
||||
|
||||
@@ -44,7 +44,8 @@ export class DegradedDaemonPtyProvider implements IPtyProvider {
|
||||
opts.current,
|
||||
opts.fallback,
|
||||
this.sessionProviders,
|
||||
opts.probeCurrentDaemonSpawn ?? null
|
||||
opts.probeCurrentDaemonSpawn ?? null,
|
||||
(sessionId) => this.findProviderForExistingSession(sessionId)
|
||||
)
|
||||
|
||||
for (const provider of this.allProviders()) {
|
||||
@@ -89,8 +90,8 @@ export class DegradedDaemonPtyProvider implements IPtyProvider {
|
||||
return mapped ? (mapped.hasPty?.(id) ?? true) : this.findProviderForExistingSession(id) !== null
|
||||
}
|
||||
|
||||
async probePtyLiveness(id: string): Promise<boolean | null> {
|
||||
return await probePtyOwners(id, this.sessionProviders.get(id), this.allDaemonAdapters())
|
||||
async probePtyLiveness(id: string, opts?: { deadlineMs?: number }): Promise<boolean | null> {
|
||||
return await probePtyOwners(id, this.sessionProviders.get(id), this.allDaemonAdapters(), opts)
|
||||
}
|
||||
|
||||
// Why: an unknown id cannot borrow listing authority from the fresh-spawn provider.
|
||||
|
||||
+115
-32
@@ -231,6 +231,7 @@ import {
|
||||
getLocalPtyProvider,
|
||||
isCurrentPtyExit,
|
||||
restorePtyIncarnation,
|
||||
STABLE_PANE_OWNER_UNVERIFIED_MESSAGE,
|
||||
type PrepareCodexSessionResume
|
||||
} from './pty'
|
||||
import { resetMacosLoginShellPreflightForTests } from '../providers/macos-tcc-login-shell'
|
||||
@@ -9042,9 +9043,9 @@ describe('registerPtyHandlers', () => {
|
||||
command: 'codex resume exact-dead-provider-session'
|
||||
})
|
||||
expect(store.setWorkspaceSession).toHaveBeenCalledOnce()
|
||||
// Why: the retire is durable before the rebind, but off the synchronous fsync path.
|
||||
expect(store.flushPendingOrThrowAsync).toHaveBeenCalledOnce()
|
||||
expect(store.flushOrThrow).not.toHaveBeenCalled()
|
||||
// Why: the retire must be durable before the fresh spawn rebinds the pane.
|
||||
expect(store.flushOrThrow).toHaveBeenCalledOnce()
|
||||
expect(store.flushPendingOrThrowAsync).not.toHaveBeenCalled()
|
||||
expect(runtime.onPtyExit).toHaveBeenCalledWith(
|
||||
'pty-dead-persisted-owner',
|
||||
0,
|
||||
@@ -9053,26 +9054,32 @@ describe('registerPtyHandlers', () => {
|
||||
}
|
||||
)
|
||||
|
||||
it.each([
|
||||
{ label: 'another owner reports it alive', liveness: true },
|
||||
{ label: 'no owner could answer', liveness: null }
|
||||
])('keeps a persisted owner whose absence is unproven ($label)', async ({ liveness }) => {
|
||||
const mountUnprovenStablePaneOwner = (opts: {
|
||||
attachAttempt: (attempt: number) => { id: string; incarnationId?: string; isReattach?: true }
|
||||
liveness: (attempt: number) => boolean | null
|
||||
}) => {
|
||||
const worktreeId = 'repo-1::/tmp/unproven-owner'
|
||||
const cwd = '/tmp/unproven-owner'
|
||||
const tabId = 'tab-unproven-owner'
|
||||
const leafId = '56565656-5656-4656-8656-565656565656'
|
||||
const paneKey = makePaneKey(tabId, leafId)
|
||||
let attachAttempts = 0
|
||||
let probeAttempts = 0
|
||||
// Why: a degraded router answers unmapped ids from the local fallback, which never
|
||||
// owned this daemon session — the same "Session not found" a truly dead PTY yields.
|
||||
const providerSpawn = vi.fn(
|
||||
async (options: { attachOnly?: boolean; command?: string; sessionId?: string }) => {
|
||||
if (options.attachOnly) {
|
||||
throw new Error('Session not found: pty-unproven-owner')
|
||||
attachAttempts += 1
|
||||
return opts.attachAttempt(attachAttempts)
|
||||
}
|
||||
return { id: 'pty-fresh-unproven', incarnationId: 'inc-fresh-unproven' }
|
||||
}
|
||||
)
|
||||
const probePtyLiveness = vi.fn(async () => liveness)
|
||||
const probePtyLiveness = vi.fn(async () => {
|
||||
probeAttempts += 1
|
||||
return opts.liveness(probeAttempts)
|
||||
})
|
||||
setLocalPtyProvider({
|
||||
spawn: providerSpawn,
|
||||
probePtyLiveness,
|
||||
@@ -9152,31 +9159,107 @@ describe('registerPtyHandlers', () => {
|
||||
store as never
|
||||
)
|
||||
|
||||
await expect(
|
||||
handlers.get('pty:spawn')!(null, {
|
||||
cols: 80,
|
||||
rows: 24,
|
||||
cwd,
|
||||
command: 'codex resume unproven-owner-session',
|
||||
worktreeId,
|
||||
tabId,
|
||||
leafId,
|
||||
env: {
|
||||
ORCA_PANE_KEY: paneKey,
|
||||
ORCA_TAB_ID: tabId,
|
||||
ORCA_WORKTREE_ID: worktreeId
|
||||
}
|
||||
})
|
||||
).rejects.toThrow('terminal_pane_owner_unverified')
|
||||
return {
|
||||
providerSpawn,
|
||||
probePtyLiveness,
|
||||
runtime,
|
||||
store,
|
||||
worktreeId,
|
||||
getSession: () => session,
|
||||
spawn: () =>
|
||||
handlers.get('pty:spawn')!(null, {
|
||||
cols: 80,
|
||||
rows: 24,
|
||||
cwd,
|
||||
command: 'codex resume unproven-owner-session',
|
||||
worktreeId,
|
||||
tabId,
|
||||
leafId,
|
||||
env: {
|
||||
ORCA_PANE_KEY: paneKey,
|
||||
ORCA_TAB_ID: tabId,
|
||||
ORCA_WORKTREE_ID: worktreeId
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
expect(probePtyLiveness).toHaveBeenCalledWith('pty-unproven-owner')
|
||||
const sessionNotFound = (): never => {
|
||||
throw new Error('Session not found: pty-unproven-owner')
|
||||
}
|
||||
|
||||
it.each([
|
||||
{ label: 'another owner reports it alive', liveness: true },
|
||||
{ label: 'no owner could answer', liveness: null }
|
||||
])('keeps a persisted owner whose absence is unproven ($label)', async ({ liveness }) => {
|
||||
vi.useFakeTimers()
|
||||
const pane = mountUnprovenStablePaneOwner({
|
||||
attachAttempt: sessionNotFound,
|
||||
liveness: () => liveness
|
||||
})
|
||||
|
||||
const settled = Promise.resolve(pane.spawn()).then(
|
||||
() => null,
|
||||
(error: Error) => error
|
||||
)
|
||||
await vi.advanceTimersByTimeAsync(30_000)
|
||||
const failure = await settled
|
||||
|
||||
expect(failure?.message).toBe(STABLE_PANE_OWNER_UNVERIFIED_MESSAGE)
|
||||
// A raw internal token would reach the pane toast verbatim; this one reads as English.
|
||||
expect(failure?.message).not.toMatch(/^[a-z_]+$/)
|
||||
expect(pane.probePtyLiveness).toHaveBeenCalledWith('pty-unproven-owner', {
|
||||
deadlineMs: expect.any(Number)
|
||||
})
|
||||
// The live PTY keeps its pane binding, gets no synthetic exit, and is not duplicated.
|
||||
expect(providerSpawn).toHaveBeenCalledOnce()
|
||||
expect(providerSpawn.mock.calls[0]?.[0]).toMatchObject({ attachOnly: true })
|
||||
expect(runtime.onPtyExit).not.toHaveBeenCalled()
|
||||
expect(store.setWorkspaceSession).not.toHaveBeenCalled()
|
||||
expect(store.flushOrThrow).not.toHaveBeenCalled()
|
||||
expect(session.tabsByWorktree[worktreeId]).toHaveLength(1)
|
||||
expect(pane.providerSpawn.mock.calls.every(([options]) => options.attachOnly === true)).toBe(
|
||||
true
|
||||
)
|
||||
expect(pane.runtime.onPtyExit).not.toHaveBeenCalled()
|
||||
expect(pane.store.setWorkspaceSession).not.toHaveBeenCalled()
|
||||
expect(pane.store.flushOrThrow).not.toHaveBeenCalled()
|
||||
expect(pane.getSession().tabsByWorktree[pane.worktreeId]).toHaveLength(1)
|
||||
})
|
||||
|
||||
it('re-proves absence instead of wedging a pane whose owner is still tearing down', async () => {
|
||||
vi.useFakeTimers()
|
||||
// A session in graceful teardown reports alive while createOrAttach already refuses it;
|
||||
// the force-kill fallback ends that window, so the pane must retire, not fail forever.
|
||||
const pane = mountUnprovenStablePaneOwner({
|
||||
attachAttempt: sessionNotFound,
|
||||
liveness: (attempt) => attempt < 3
|
||||
})
|
||||
|
||||
const settled = pane.spawn()
|
||||
await vi.advanceTimersByTimeAsync(30_000)
|
||||
|
||||
await expect(settled).resolves.toMatchObject({ id: 'pty-fresh-unproven' })
|
||||
expect(pane.probePtyLiveness).toHaveBeenCalledTimes(3)
|
||||
expect(pane.runtime.onPtyExit).toHaveBeenCalledWith(
|
||||
'pty-unproven-owner',
|
||||
0,
|
||||
'inc-unproven-owner'
|
||||
)
|
||||
expect(pane.store.flushOrThrow).toHaveBeenCalledOnce()
|
||||
})
|
||||
|
||||
it('reattaches when the retry reaches the owner the first attach was misrouted past', async () => {
|
||||
vi.useFakeTimers()
|
||||
const pane = mountUnprovenStablePaneOwner({
|
||||
attachAttempt: (attempt) =>
|
||||
attempt === 1
|
||||
? sessionNotFound()
|
||||
: { id: 'pty-unproven-owner', incarnationId: 'inc-unproven-owner', isReattach: true },
|
||||
liveness: () => true
|
||||
})
|
||||
|
||||
const settled = pane.spawn()
|
||||
await vi.advanceTimersByTimeAsync(30_000)
|
||||
|
||||
await expect(settled).resolves.toMatchObject({ id: 'pty-unproven-owner' })
|
||||
expect(pane.runtime.onPtyExit).not.toHaveBeenCalled()
|
||||
expect(pane.store.setWorkspaceSession).not.toHaveBeenCalled()
|
||||
expect(pane.store.flushOrThrow).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('retires a dead owner from the exact SSH host session before fresh recovery', async () => {
|
||||
|
||||
+71
-40
@@ -667,12 +667,12 @@ function resolveStablePaneOwner(
|
||||
}
|
||||
}
|
||||
|
||||
async function retirePersistedStablePaneOwner(
|
||||
function retirePersistedStablePaneOwner(
|
||||
store: Store | undefined,
|
||||
owner: StablePaneOwner,
|
||||
worktreeId: string,
|
||||
connectionId: string | null | undefined
|
||||
): Promise<boolean> {
|
||||
): boolean {
|
||||
if (!store) {
|
||||
return false
|
||||
}
|
||||
@@ -694,10 +694,7 @@ async function retirePersistedStablePaneOwner(
|
||||
return false
|
||||
}
|
||||
store.setWorkspaceSession(retired, hostId)
|
||||
// Why: the retirement must be durable before the fresh spawn rebinds the pane, but
|
||||
// writeToDiskSync fsyncs the whole multi-MB state from the main thread — a restore that
|
||||
// retires N dead panes paid that stall N times. The async twin keeps the ordering.
|
||||
await store.flushPendingOrThrowAsync()
|
||||
store.flushOrThrow()
|
||||
return true
|
||||
}
|
||||
|
||||
@@ -713,39 +710,73 @@ type StablePaneSpawnContext = {
|
||||
onFreshSpawn?: (result: PtySpawnResult) => void
|
||||
}
|
||||
|
||||
/** Signals that every provider that could own this PTY answered that it is absent. */
|
||||
const STABLE_PANE_OWNER_PROVEN_GONE = Symbol('stable_pane_owner_proven_gone')
|
||||
|
||||
// Why: a doomed session in graceful teardown still reports alive while refusing attach, and
|
||||
// the daemon's force-kill fallback (KILL_TIMEOUT_MS) bounds that window — so re-prove past it
|
||||
// instead of failing the pane for the whole window.
|
||||
const STABLE_PANE_OWNER_ABSENCE_PROOF_TIMEOUT_MS = 6_000
|
||||
const STABLE_PANE_OWNER_ABSENCE_PROOF_RETRY_MS = 250
|
||||
export const STABLE_PANE_OWNER_UNVERIFIED_MESSAGE =
|
||||
'This terminal is still claimed by a session that did not answer; its history was kept. Try again in a moment.'
|
||||
|
||||
/**
|
||||
* Re-attaches until the owner either answers or is provably gone. A "Session not found" that
|
||||
* coincides with a live probe is never proof: a degraded router answers unmapped ids from the
|
||||
* local fallback, which never owned the daemon session, and a session mid-teardown refuses
|
||||
* attach while still alive. Both are transient, so the veto retries rather than wedging the
|
||||
* pane — only a provider that answers "absent" authorizes destroying the binding.
|
||||
*/
|
||||
async function attachStablePaneOwnerUntilProvenGone(
|
||||
provider: IPtyProvider,
|
||||
owner: StablePaneOwner,
|
||||
spawnOptions: PtySpawnOptions
|
||||
): Promise<PtySpawnResult | typeof STABLE_PANE_OWNER_PROVEN_GONE> {
|
||||
const deadlineMs = Date.now() + STABLE_PANE_OWNER_ABSENCE_PROOF_TIMEOUT_MS
|
||||
for (;;) {
|
||||
try {
|
||||
return await provider.spawn({
|
||||
...spawnOptions,
|
||||
sessionId: owner.ptyId,
|
||||
attachOnly: true,
|
||||
isNewSession: undefined,
|
||||
command: undefined,
|
||||
commandDelivery: undefined,
|
||||
startupCommandDelivery: undefined,
|
||||
launchAgent: undefined,
|
||||
startupIngress: undefined,
|
||||
agentSessionEnsure: undefined,
|
||||
agentSessionCreateOperationId: undefined,
|
||||
onPtySpawnCommitted: undefined
|
||||
})
|
||||
} catch (error) {
|
||||
if (!isPtyAlreadyGoneError(error)) {
|
||||
throw error
|
||||
}
|
||||
// Providers without a probe are their own sole owner, so their refusal is authoritative.
|
||||
if (!provider.probePtyLiveness) {
|
||||
return STABLE_PANE_OWNER_PROVEN_GONE
|
||||
}
|
||||
// Why the deadline: a wedged endpoint would otherwise burn the client's 30s request
|
||||
// timeout per probe; the shared budget also caps the whole unproven wait.
|
||||
if ((await provider.probePtyLiveness(owner.ptyId, { deadlineMs })) === false) {
|
||||
return STABLE_PANE_OWNER_PROVEN_GONE
|
||||
}
|
||||
if (Date.now() >= deadlineMs) {
|
||||
throw new Error(STABLE_PANE_OWNER_UNVERIFIED_MESSAGE)
|
||||
}
|
||||
await delay(STABLE_PANE_OWNER_ABSENCE_PROOF_RETRY_MS)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
async function attachStablePaneOwner(
|
||||
args: StablePaneSpawnContext & { owner: StablePaneOwner }
|
||||
): Promise<{ result: PtySpawnResult; owner: StablePaneOwner } | null> {
|
||||
const { owner, provider, runtime, spawnOptions } = args
|
||||
let result: PtySpawnResult
|
||||
try {
|
||||
result = await provider.spawn({
|
||||
...spawnOptions,
|
||||
sessionId: owner.ptyId,
|
||||
attachOnly: true,
|
||||
isNewSession: undefined,
|
||||
command: undefined,
|
||||
commandDelivery: undefined,
|
||||
startupCommandDelivery: undefined,
|
||||
launchAgent: undefined,
|
||||
startupIngress: undefined,
|
||||
agentSessionEnsure: undefined,
|
||||
agentSessionCreateOperationId: undefined,
|
||||
onPtySpawnCommitted: undefined
|
||||
})
|
||||
} catch (error) {
|
||||
if (!isPtyAlreadyGoneError(error)) {
|
||||
throw error
|
||||
}
|
||||
// Why: "Session not found" only proves the provider we asked has no such PTY — and a
|
||||
// degraded router answers unmapped ids from the local fallback, which never owned a
|
||||
// daemon session. Retiring on that would signal exit and delete a live agent's pane
|
||||
// binding. Absence must be proven across every possible owner first; `null` (nobody
|
||||
// could answer) is not absence. Providers without a probe are their own sole owner,
|
||||
// so their refusal stays authoritative.
|
||||
if (provider.probePtyLiveness && (await provider.probePtyLiveness(owner.ptyId)) !== false) {
|
||||
throw new Error('terminal_pane_owner_unverified')
|
||||
}
|
||||
const attached = await attachStablePaneOwnerUntilProvenGone(provider, owner, spawnOptions)
|
||||
if (attached === STABLE_PANE_OWNER_PROVEN_GONE) {
|
||||
const ownerBeforeRetire = args.resolveOwner?.()
|
||||
if (
|
||||
ownerBeforeRetire &&
|
||||
@@ -761,7 +792,7 @@ async function attachStablePaneOwner(
|
||||
ptyOwnership.delete(owner.ptyId)
|
||||
if (
|
||||
args.worktreeId &&
|
||||
!(await retirePersistedStablePaneOwner(args.store, owner, args.worktreeId, args.connectionId))
|
||||
!retirePersistedStablePaneOwner(args.store, owner, args.worktreeId, args.connectionId)
|
||||
) {
|
||||
throw new Error('terminal_pane_owner_changed')
|
||||
}
|
||||
@@ -771,13 +802,13 @@ async function attachStablePaneOwner(
|
||||
return null
|
||||
}
|
||||
if (
|
||||
result.id !== owner.ptyId ||
|
||||
result.isReattach !== true ||
|
||||
(owner.incarnationId !== undefined && result.incarnationId !== owner.incarnationId)
|
||||
attached.id !== owner.ptyId ||
|
||||
attached.isReattach !== true ||
|
||||
(owner.incarnationId !== undefined && attached.incarnationId !== owner.incarnationId)
|
||||
) {
|
||||
throw new Error('terminal_pane_owner_changed')
|
||||
}
|
||||
return { result, owner }
|
||||
return { result: attached, owner }
|
||||
}
|
||||
|
||||
async function spawnForStablePane(
|
||||
|
||||
@@ -112,8 +112,12 @@ export type IPtyProvider = {
|
||||
supportsAgentSessionCreateOperations?: (options?: PtyProbeOptions) => boolean | Promise<boolean>
|
||||
attach(id: string): Promise<void>
|
||||
hasPty?: (id: string) => boolean
|
||||
/** Exact provider readback: false only when the provider answered that the PTY is absent. */
|
||||
probePtyLiveness?: (id: string) => Promise<boolean | null>
|
||||
/**
|
||||
* Exact provider readback: false only when the provider answered that the PTY is absent.
|
||||
* `deadlineMs` is absolute and bounds connect + request so a wedged endpoint answers
|
||||
* `null` inside the caller's budget instead of the client's default request timeout.
|
||||
*/
|
||||
probePtyLiveness?: (id: string, opts?: { deadlineMs?: number }) => Promise<boolean | null>
|
||||
write(id: string, data: string): void
|
||||
resize(id: string, cols: number, rows: number): void
|
||||
/**
|
||||
|
||||
Reference in New Issue
Block a user