mirror of
https://github.com/stablyai/orca.git
synced 2026-10-24 00:04:01 +00:00
fix(ssh): a relay that reports a PTY absent must authorise a fresh spawn
reattachSshPtySession flattened the relay's `PTY "<id>" not found` into a bare `SSH_SESSION_EXPIRED`, discarding the evidence. isPtyAlreadyGoneError classifies on that message via /PTY ".+" not found/, so it could never match — and attachStablePaneOwner's existing already-gone fallback (retire the pane binding, onPtyExit, spawn fresh) is gated on exactly that predicate. The recovery machinery was already correct; only the classification was broken, which is why `pty:spawn` rejected outright after a relay restart renumbered its PTYs from pty-1. SshPtyAbsentFromRelayError carries the verdict the message cannot: a reachable relay answered for this exact id and reported it absent. Both thrown messages stay byte-identical to the previous ones, so all six message-based consumers are untouched; only callers that can act on the stronger verdict test the class. Deliberately not raised for a transport failure, timeout, disposed multiplexer, identity mismatch, or restoreRequired — none of those observe the process, and treating them as absence would orphan live remote work.
This commit is contained in:
@@ -0,0 +1,63 @@
|
||||
import { describe, expect, it, vi } from 'vitest'
|
||||
import {
|
||||
SSH_SESSION_EXPIRED_ERROR,
|
||||
SshPtyAbsentFromRelayError
|
||||
} from '../../../providers/ssh-pty-errors'
|
||||
import type { IPtyProvider } from '../../../providers/types'
|
||||
import { spawnForStablePane, type StablePaneOwner } from './stable-owner'
|
||||
|
||||
const OWNER: StablePaneOwner = {
|
||||
tabId: 'tab-1',
|
||||
leafId: '1b3f2c4d-5e6a-4b7c-8d9e-0f1a2b3c4d5e',
|
||||
ptyId: 'ssh:conn-1@@pty-1'
|
||||
}
|
||||
|
||||
function spawnAfterAttachRejection(error: unknown): {
|
||||
run: () => ReturnType<typeof spawnForStablePane>
|
||||
spawn: ReturnType<typeof vi.fn>
|
||||
} {
|
||||
const spawn = vi
|
||||
.fn()
|
||||
.mockRejectedValueOnce(error)
|
||||
.mockResolvedValueOnce({ id: 'ssh:conn-1@@pty-1', isReattach: false })
|
||||
return {
|
||||
spawn,
|
||||
run: () =>
|
||||
spawnForStablePane({
|
||||
runtime: undefined,
|
||||
provider: { spawn } as unknown as IPtyProvider,
|
||||
spawnOptions: { cols: 80, rows: 24 },
|
||||
owner: OWNER,
|
||||
connectionId: 'conn-1',
|
||||
resolveOwner: () => null
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
describe('stable pane adoption after the relay reports the PTY absent', () => {
|
||||
it('spawns fresh once the relay has positively answered for that id', async () => {
|
||||
const { run, spawn } = spawnAfterAttachRejection(
|
||||
new SshPtyAbsentFromRelayError(`${SSH_SESSION_EXPIRED_ERROR}: pty-1`)
|
||||
)
|
||||
|
||||
const result = await run()
|
||||
|
||||
expect(spawn).toHaveBeenCalledTimes(2)
|
||||
expect(spawn.mock.calls[0]?.[0]).toMatchObject({ attachOnly: true })
|
||||
expect(spawn.mock.calls[1]?.[0]).not.toHaveProperty('sessionId')
|
||||
expect(result.owner).toBeNull()
|
||||
})
|
||||
|
||||
// A restarted relay renumbers from pty-1, so the message alone cannot distinguish absence from a
|
||||
// lost link — only the class may authorise abandoning the binding.
|
||||
it.each([
|
||||
['an expired session with no relay evidence', `${SSH_SESSION_EXPIRED_ERROR}: pty-1`],
|
||||
['a lost link', 'SSH connection lost, reconnecting...'],
|
||||
['a request timeout', 'Request "pty.attach" timed out after 10000ms']
|
||||
])('keeps the binding and refuses to respawn after %s', async (_label, message) => {
|
||||
const { run, spawn } = spawnAfterAttachRejection(new Error(message))
|
||||
|
||||
await expect(run()).rejects.toThrow(message)
|
||||
expect(spawn).toHaveBeenCalledTimes(1)
|
||||
})
|
||||
})
|
||||
@@ -3,7 +3,10 @@ import type { AgentSessionOwnerBinding } from '../../../../shared/agent-session-
|
||||
import { agentSessionOwnerBindingsEqual } from '../../../../shared/claimed-agent-pty-owner'
|
||||
import { addNodePtyRecoveryHint } from '../../../daemon/node-pty-error-hints'
|
||||
import type { Store } from '../../../persistence'
|
||||
import { isSshPtyNotFoundError } from '../../../providers/ssh-pty-errors'
|
||||
import {
|
||||
isSshPtyAbsentFromRelayError,
|
||||
isSshPtyNotFoundError
|
||||
} from '../../../providers/ssh-pty-errors'
|
||||
import type { IPtyProvider } from '../../../providers/types'
|
||||
import { markClaudePtyExited } from '../../../claude-accounts/live-pty-gate'
|
||||
import { ptyIncarnationById, ptyOwnership } from './ownership-state'
|
||||
@@ -54,7 +57,13 @@ export function normalizeNodePtySpawnError(err: unknown): Error {
|
||||
|
||||
export function isPtyAlreadyGoneError(err: unknown): boolean {
|
||||
const message = err instanceof Error ? err.message : String(err)
|
||||
return isSshPtyNotFoundError(err) || /Session not found/i.test(message)
|
||||
return (
|
||||
isSshPtyNotFoundError(err) ||
|
||||
// Why: the reattach path rewrites the relay's wording to SSH_SESSION_EXPIRED and only this
|
||||
// class preserves that the relay itself answered "absent" rather than the link dropping.
|
||||
isSshPtyAbsentFromRelayError(err) ||
|
||||
/Session not found/i.test(message)
|
||||
)
|
||||
}
|
||||
|
||||
export function delay(ms: number): Promise<void> {
|
||||
|
||||
@@ -10,3 +10,25 @@ export function isSshPtyIdentityMismatchError(error: unknown): boolean {
|
||||
const message = error instanceof Error ? error.message : String(error)
|
||||
return message.includes(SSH_PTY_IDENTITY_MISMATCH_ERROR) || /identity mismatch/i.test(message)
|
||||
}
|
||||
|
||||
/**
|
||||
* A reachable relay answered for this exact PTY id and reported it absent — positive evidence of
|
||||
* absence from the execution host, so `exited` rather than `unverifiable`
|
||||
* (docs/reference/ssh-execution-boundary.md). Deliberately NOT raised for a transport failure, a
|
||||
* request timeout, a disposed multiplexer, an identity mismatch (the id names a live PTY belonging
|
||||
* to another pane), or `restoreRequired` (the PTY is live, only its source stream is not) — none of
|
||||
* those observe the process, and treating them as absence orphans live remote work.
|
||||
*
|
||||
* Carries the same `SSH_SESSION_EXPIRED` message so message-based consumers are unaffected; only
|
||||
* callers that can act on the stronger verdict test the class.
|
||||
*/
|
||||
export class SshPtyAbsentFromRelayError extends Error {
|
||||
constructor(message: string) {
|
||||
super(message)
|
||||
this.name = 'SshPtyAbsentFromRelayError'
|
||||
}
|
||||
}
|
||||
|
||||
export function isSshPtyAbsentFromRelayError(error: unknown): boolean {
|
||||
return error instanceof SshPtyAbsentFromRelayError
|
||||
}
|
||||
|
||||
@@ -0,0 +1,78 @@
|
||||
import { describe, expect, it, vi } from 'vitest'
|
||||
import {
|
||||
SSH_PTY_IDENTITY_MISMATCH_ERROR,
|
||||
SSH_SESSION_EXPIRED_ERROR,
|
||||
isSshPtyAbsentFromRelayError
|
||||
} from './ssh-pty-errors'
|
||||
import { SshPtyProvider } from './ssh-pty-provider'
|
||||
|
||||
function providerRejectingAttachWith(error: unknown): SshPtyProvider {
|
||||
const mux = {
|
||||
request: vi.fn().mockRejectedValue(error),
|
||||
notify: vi.fn(),
|
||||
onNotification: vi.fn().mockReturnValue(vi.fn())
|
||||
}
|
||||
return new SshPtyProvider('conn-1', mux as never)
|
||||
}
|
||||
|
||||
async function reattachRejection(error: unknown): Promise<unknown> {
|
||||
const provider = providerRejectingAttachWith(error)
|
||||
return await provider.spawn({ cols: 80, rows: 24, sessionId: 'pty-1' }).then(
|
||||
() => undefined,
|
||||
(rejection: unknown) => rejection
|
||||
)
|
||||
}
|
||||
|
||||
describe('SSH PTY relay absence verdict', () => {
|
||||
it('reports a relay-delivered not-found as positive absence', async () => {
|
||||
const rejection = await reattachRejection(new Error('PTY "pty-1" not found'))
|
||||
|
||||
expect(isSshPtyAbsentFromRelayError(rejection)).toBe(true)
|
||||
expect((rejection as Error).message).toBe(`${SSH_SESSION_EXPIRED_ERROR}: pty-1`)
|
||||
})
|
||||
|
||||
it('does not report absence when the id names a live PTY owned by another pane', async () => {
|
||||
const rejection = await reattachRejection(
|
||||
new Error('PTY "pty-1" not found (identity mismatch)')
|
||||
)
|
||||
|
||||
expect(isSshPtyAbsentFromRelayError(rejection)).toBe(false)
|
||||
// Byte-identical to the pre-change message: only the class is new, so the toast humanizer,
|
||||
// both spawn-execute lease paths, and the renderer's substring checks are untouched.
|
||||
expect((rejection as Error).message).toBe(
|
||||
`${SSH_SESSION_EXPIRED_ERROR}: pty-1 ${SSH_PTY_IDENTITY_MISMATCH_ERROR}`
|
||||
)
|
||||
})
|
||||
|
||||
// Loss of contact never observes the process: each of these must stay `unverifiable`.
|
||||
it.each([
|
||||
['a lost link', 'SSH connection lost, reconnecting...'],
|
||||
['a disposed multiplexer', 'Multiplexer disposed'],
|
||||
['a request timeout', 'Request "pty.attach" timed out after 10000ms']
|
||||
])('does not report absence for %s', async (_label, message) => {
|
||||
const rejection = await reattachRejection(new Error(message))
|
||||
|
||||
expect(isSshPtyAbsentFromRelayError(rejection)).toBe(false)
|
||||
expect((rejection as Error).message).toBe(message)
|
||||
})
|
||||
|
||||
it('does not report absence when the PTY is live but its source needs restoring', async () => {
|
||||
const mux = {
|
||||
request: vi.fn().mockResolvedValue({
|
||||
incarnationId: 'incarnation-1',
|
||||
sourceRecovery: { status: 'restoreRequired', reason: 'checkpointUnavailable' }
|
||||
}),
|
||||
notify: vi.fn(),
|
||||
onNotification: vi.fn().mockReturnValue(vi.fn())
|
||||
}
|
||||
const provider = new SshPtyProvider('conn-1', mux as never)
|
||||
|
||||
const rejection = await provider.spawn({ cols: 80, rows: 24, sessionId: 'pty-1' }).then(
|
||||
() => undefined,
|
||||
(error: unknown) => error
|
||||
)
|
||||
|
||||
expect(isSshPtyAbsentFromRelayError(rejection)).toBe(false)
|
||||
expect((rejection as Error).message).toContain(SSH_SESSION_EXPIRED_ERROR)
|
||||
})
|
||||
})
|
||||
@@ -3,6 +3,7 @@ import { isPtyIncarnationId, type PtyIncarnationId } from '../../shared/pty-inca
|
||||
import {
|
||||
SSH_PTY_IDENTITY_MISMATCH_ERROR,
|
||||
SSH_SESSION_EXPIRED_ERROR,
|
||||
SshPtyAbsentFromRelayError,
|
||||
isSshPtyIdentityMismatchError,
|
||||
isSshPtyNotFoundError
|
||||
} from './ssh-pty-errors'
|
||||
@@ -225,10 +226,16 @@ export async function reattachSshPtySession(args: {
|
||||
// Why: an expired relay lease must be surfaced distinctly so the renderer clears its binding.
|
||||
console.warn(`[ssh-pty] pty.attach FAILED for ${args.sessionId}:`, error)
|
||||
if (isSshPtyNotFoundError(error)) {
|
||||
const mismatchMarker = isSshPtyIdentityMismatchError(error)
|
||||
? ` ${SSH_PTY_IDENTITY_MISMATCH_ERROR}`
|
||||
: ''
|
||||
throw new Error(`${SSH_SESSION_EXPIRED_ERROR}: ${relaySessionId}${mismatchMarker}`)
|
||||
if (isSshPtyIdentityMismatchError(error)) {
|
||||
// The id names a LIVE PTY owned by another pane, so this is not evidence of absence.
|
||||
throw new Error(
|
||||
`${SSH_SESSION_EXPIRED_ERROR}: ${relaySessionId} ${SSH_PTY_IDENTITY_MISMATCH_ERROR}`
|
||||
)
|
||||
}
|
||||
// Why the class: the relay answered for this exact id, so callers holding a pane binding may
|
||||
// retire it and spawn fresh. Plain `SSH_SESSION_EXPIRED` cannot say that — a restarted relay
|
||||
// renumbers from pty-1, so the message alone is indistinguishable from a lost link.
|
||||
throw new SshPtyAbsentFromRelayError(`${SSH_SESSION_EXPIRED_ERROR}: ${relaySessionId}`)
|
||||
}
|
||||
throw error
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user