mirror of
https://github.com/stablyai/orca.git
synced 2026-10-26 16:02:42 +00:00
wip(ssh): probePtyLiveness unverifiable work rescued after agent death
The reviewing agent died with this uncommitted while implementing the P1 fix for probePtyLiveness returning false for PTYs absent from both liveness sets. Introduces ssh-pty-liveness-state.ts. Not reviewed, not verified, not complete.
This commit is contained in:
@@ -20,6 +20,15 @@ export function isSshPtyLivenessUnverifiableError(error: unknown): boolean {
|
||||
return message.includes(SSH_PTY_LIVENESS_UNVERIFIABLE_ERROR)
|
||||
}
|
||||
|
||||
export function isSshPtyExitedEvidenceError(error: unknown): boolean {
|
||||
const message = error instanceof Error ? error.message : String(error)
|
||||
return (
|
||||
!isSshPtyIdentityMismatchError(error) &&
|
||||
(message.includes(SSH_SESSION_EXPIRED_ERROR) ||
|
||||
message === 'agent_session_exited_during_start')
|
||||
)
|
||||
}
|
||||
|
||||
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)
|
||||
|
||||
@@ -0,0 +1,58 @@
|
||||
export const MAX_SSH_PTY_EXIT_TOMBSTONES = 1000
|
||||
|
||||
export class SshPtyLivenessState {
|
||||
readonly livePtyIds = new Set<string>()
|
||||
private readonly unverifiablePtyIds = new Set<string>()
|
||||
private readonly exitedPtyIds = new Set<string>()
|
||||
|
||||
constructor(
|
||||
private readonly toAppPtyId: (id: string) => string,
|
||||
initialUnverifiablePtyIds: Iterable<string>
|
||||
) {
|
||||
for (const id of initialUnverifiablePtyIds) {
|
||||
this.acceptUnverifiable(id)
|
||||
}
|
||||
}
|
||||
|
||||
clear(): void {
|
||||
this.livePtyIds.clear()
|
||||
this.unverifiablePtyIds.clear()
|
||||
this.exitedPtyIds.clear()
|
||||
}
|
||||
|
||||
hasRoutingOwnership(id: string): boolean {
|
||||
return this.livePtyIds.has(id) || this.unverifiablePtyIds.has(id)
|
||||
}
|
||||
|
||||
probe(id: string): boolean | null {
|
||||
return this.livePtyIds.has(id) ? true : this.exitedPtyIds.has(id) ? false : null
|
||||
}
|
||||
|
||||
acceptLive(id: string): void {
|
||||
const appPtyId = this.toAppPtyId(id)
|
||||
this.unverifiablePtyIds.delete(appPtyId)
|
||||
this.exitedPtyIds.delete(appPtyId)
|
||||
this.livePtyIds.add(appPtyId)
|
||||
}
|
||||
|
||||
acceptUnverifiable(id: string): void {
|
||||
const appPtyId = this.toAppPtyId(id)
|
||||
this.livePtyIds.delete(appPtyId)
|
||||
this.exitedPtyIds.delete(appPtyId)
|
||||
this.unverifiablePtyIds.add(appPtyId)
|
||||
}
|
||||
|
||||
acceptExited(id: string): void {
|
||||
const appPtyId = this.toAppPtyId(id)
|
||||
this.livePtyIds.delete(appPtyId)
|
||||
this.unverifiablePtyIds.delete(appPtyId)
|
||||
this.exitedPtyIds.delete(appPtyId)
|
||||
this.exitedPtyIds.add(appPtyId)
|
||||
if (this.exitedPtyIds.size > MAX_SSH_PTY_EXIT_TOMBSTONES) {
|
||||
const oldest = this.exitedPtyIds.values().next().value
|
||||
if (oldest !== undefined) {
|
||||
this.exitedPtyIds.delete(oldest)
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -23,6 +23,7 @@ import { SshPtySpawnExitRaceTracker } from './ssh-pty-spawn-exit-race'
|
||||
import { SshAgentSessionCapabilities } from './ssh-agent-session-capabilities'
|
||||
import type { PtyProcessInspection } from './pty-process-inspection'
|
||||
import { writeToSshPty, writeToSshPtyWithSettlement } from './ssh-pty-write'
|
||||
import { SshPtyLivenessState } from './ssh-pty-liveness-state'
|
||||
|
||||
// Why: sequential relay teardown calls share one absolute budget; convert to the mux-relative timeout only at dispatch.
|
||||
function relayTimeoutOptions(deadlineMs: number | undefined): { timeoutMs: number } | undefined {
|
||||
@@ -33,8 +34,7 @@ function relayTimeoutOptions(deadlineMs: number | undefined): { timeoutMs: numbe
|
||||
export class SshPtyProvider implements IPtyProvider {
|
||||
private mux: SshChannelMultiplexer
|
||||
private connectionId: string
|
||||
private livePtyIds = new Set<string>()
|
||||
private unverifiablePtyIds = new Set<string>()
|
||||
private readonly livenessState: SshPtyLivenessState
|
||||
readonly getAppliedSize: NonNullable<IPtyProvider['getAppliedSize']>
|
||||
private readonly agentSessionCapabilities: SshAgentSessionCapabilities
|
||||
private spawnExitRaces = new SshPtySpawnExitRaceTracker()
|
||||
@@ -47,19 +47,21 @@ export class SshPtyProvider implements IPtyProvider {
|
||||
connectionId: string,
|
||||
mux: SshChannelMultiplexer,
|
||||
private readonly remoteCliBridgeEnv?: RemoteCliBridgeEnv,
|
||||
readonly providerGeneration = 1
|
||||
readonly providerGeneration = 1,
|
||||
initialUnverifiablePtyIds: Iterable<string> = []
|
||||
) {
|
||||
this.connectionId = connectionId
|
||||
this.mux = mux
|
||||
this.agentSessionCapabilities = new SshAgentSessionCapabilities(mux)
|
||||
this.getAppliedSize = createSshPtyAppliedSizeReader(mux, connectionId)
|
||||
this.livenessState = new SshPtyLivenessState(this.toAppPtyId, initialUnverifiablePtyIds)
|
||||
|
||||
this.outputState = new SshPtyProviderOutputState(providerGeneration, {
|
||||
mux,
|
||||
toAppPtyId: (id) => this.toAppPtyId(id),
|
||||
livePtyIds: this.livePtyIds,
|
||||
livePtyIds: this.livenessState.livePtyIds,
|
||||
recordExit: (relayPtyId, incarnationId) => {
|
||||
this.unverifiablePtyIds.delete(this.toAppPtyId(relayPtyId))
|
||||
this.acceptExitedPty(this.toAppPtyId(relayPtyId))
|
||||
this.spawnExitRaces.recordExit(relayPtyId, incarnationId)
|
||||
}
|
||||
})
|
||||
@@ -67,8 +69,7 @@ export class SshPtyProvider implements IPtyProvider {
|
||||
|
||||
dispose(): void {
|
||||
this.outputState.dispose()
|
||||
this.livePtyIds.clear()
|
||||
this.unverifiablePtyIds.clear()
|
||||
this.livenessState.clear()
|
||||
}
|
||||
|
||||
getConnectionId = (): string => this.connectionId
|
||||
@@ -104,7 +105,8 @@ export class SshPtyProvider implements IPtyProvider {
|
||||
rememberPtyIncarnation: (relayPtyId, incarnationId) =>
|
||||
this.outputState.rememberPtyIncarnation(relayPtyId, incarnationId),
|
||||
acceptLivePty: (relayPtyId) => this.acceptLivePty(relayPtyId),
|
||||
acceptUnverifiablePty: (relayPtyId) => this.acceptUnverifiablePty(relayPtyId)
|
||||
acceptUnverifiablePty: (relayPtyId) => this.acceptUnverifiablePty(relayPtyId),
|
||||
acceptExitedPty: (relayPtyId) => this.acceptExitedPty(relayPtyId)
|
||||
})
|
||||
}
|
||||
|
||||
@@ -162,6 +164,7 @@ export class SshPtyProvider implements IPtyProvider {
|
||||
rememberPtyIncarnation: (ptyId, incarnationId) =>
|
||||
this.outputState.rememberPtyIncarnation(ptyId, incarnationId)
|
||||
})
|
||||
this.acceptLivePty(id)
|
||||
}
|
||||
|
||||
async attachForReconnect(
|
||||
@@ -181,7 +184,7 @@ export class SshPtyProvider implements IPtyProvider {
|
||||
...(expected?.tabId ? { expectedTabId: expected.tabId } : {})
|
||||
}
|
||||
const relayPtyId = this.toRelayPtyId(id)
|
||||
return await requestSshPtyAttach({
|
||||
const result = await requestSshPtyAttach({
|
||||
mux: this.mux,
|
||||
relayPtyId,
|
||||
params,
|
||||
@@ -191,6 +194,8 @@ export class SshPtyProvider implements IPtyProvider {
|
||||
rememberPtyIncarnation: (ptyId, incarnationId) =>
|
||||
this.outputState.rememberPtyIncarnation(ptyId, incarnationId)
|
||||
})
|
||||
this.acceptLivePty(id)
|
||||
return result
|
||||
}
|
||||
|
||||
write(id: string, data: string): boolean {
|
||||
@@ -218,8 +223,7 @@ export class SshPtyProvider implements IPtyProvider {
|
||||
},
|
||||
relayTimeoutOptions(opts.deadlineMs)
|
||||
)
|
||||
this.livePtyIds.delete(id)
|
||||
this.unverifiablePtyIds.delete(id)
|
||||
this.acceptExitedPty(id)
|
||||
}
|
||||
|
||||
async sendSignal(id: string, signal: string): Promise<void> {
|
||||
@@ -294,19 +298,20 @@ export class SshPtyProvider implements IPtyProvider {
|
||||
}
|
||||
|
||||
// `hasPty` retains routing ownership; liveness callers must use the tri-state probe.
|
||||
hasPty = (id: string): boolean => this.livePtyIds.has(id) || this.unverifiablePtyIds.has(id)
|
||||
hasPty = (id: string): boolean => this.livenessState.hasRoutingOwnership(id)
|
||||
|
||||
probePtyLiveness = async (id: string): Promise<boolean | null> =>
|
||||
this.unverifiablePtyIds.has(id) ? null : this.livePtyIds.has(id)
|
||||
probePtyLiveness = async (id: string): Promise<boolean | null> => this.livenessState.probe(id)
|
||||
|
||||
private acceptLivePty(id: string): void {
|
||||
this.unverifiablePtyIds.delete(id)
|
||||
this.livePtyIds.add(id)
|
||||
acceptLivePty(id: string): void {
|
||||
this.livenessState.acceptLive(id)
|
||||
}
|
||||
|
||||
private acceptUnverifiablePty(id: string): void {
|
||||
this.livePtyIds.delete(id)
|
||||
this.unverifiablePtyIds.add(id)
|
||||
acceptUnverifiablePty(id: string): void {
|
||||
this.livenessState.acceptUnverifiable(id)
|
||||
}
|
||||
|
||||
acceptExitedPty(id: string): void {
|
||||
this.livenessState.acceptExited(id)
|
||||
}
|
||||
|
||||
async getDefaultShell(): Promise<string> {
|
||||
|
||||
@@ -4,6 +4,7 @@ import {
|
||||
SSH_PTY_RESTORE_REQUIRED_ERROR,
|
||||
SSH_SESSION_EXPIRED_ERROR
|
||||
} from './ssh-pty-errors'
|
||||
import { MAX_SSH_PTY_EXIT_TOMBSTONES } from './ssh-pty-liveness-state'
|
||||
import { SshPtyProvider } from './ssh-pty-provider'
|
||||
|
||||
// The relay answers `restoreRequired` from its DELIVERY layer only: `requireRestore`
|
||||
@@ -41,6 +42,33 @@ async function spawnError(provider: SshPtyProvider): Promise<string> {
|
||||
}
|
||||
|
||||
describe('SSH PTY reattach when the relay requires source restoration', () => {
|
||||
it('keeps an unprobed PTY unverifiable', async () => {
|
||||
const provider = new SshPtyProvider('conn-1', createMockMux() as never)
|
||||
|
||||
expect(provider.hasPty('ssh:conn-1@@pty-unprobed')).toBe(false)
|
||||
await expect(provider.probePtyLiveness('ssh:conn-1@@pty-unprobed')).resolves.toBeNull()
|
||||
})
|
||||
|
||||
it('retains routing ownership for a reconnecting PTY without reporting it live', async () => {
|
||||
const id = 'ssh:conn-1@@pty-reconnecting'
|
||||
const provider = new SshPtyProvider('conn-1', createMockMux() as never, undefined, 1, [id])
|
||||
|
||||
expect(provider.hasPty(id)).toBe(true)
|
||||
await expect(provider.probePtyLiveness(id)).resolves.toBeNull()
|
||||
})
|
||||
|
||||
it('bounds exited PTY evidence and evicts to unverifiable', async () => {
|
||||
const provider = new SshPtyProvider('conn-1', createMockMux() as never)
|
||||
for (let index = 0; index <= MAX_SSH_PTY_EXIT_TOMBSTONES; index += 1) {
|
||||
provider.acceptExitedPty(`pty-${index}`)
|
||||
}
|
||||
|
||||
await expect(provider.probePtyLiveness('ssh:conn-1@@pty-0')).resolves.toBeNull()
|
||||
await expect(
|
||||
provider.probePtyLiveness(`ssh:conn-1@@pty-${MAX_SSH_PTY_EXIT_TOMBSTONES}`)
|
||||
).resolves.toBe(false)
|
||||
})
|
||||
|
||||
it('re-attaches over the live PTY instead of reporting exited', async () => {
|
||||
const mux = createMockMux()
|
||||
// Why: the relay retires the stale delivery record as the restoreRequired response settles,
|
||||
|
||||
@@ -5,6 +5,7 @@ import {
|
||||
SSH_PTY_LIVENESS_UNVERIFIABLE_ERROR,
|
||||
SSH_PTY_RESTORE_REQUIRED_ERROR,
|
||||
SSH_SESSION_EXPIRED_ERROR,
|
||||
isSshPtyExitedEvidenceError,
|
||||
isSshPtyIdentityMismatchError,
|
||||
isSshPtyNotFoundError
|
||||
} from './ssh-pty-errors'
|
||||
@@ -262,6 +263,7 @@ export async function reattachSshPtySessionForSpawn(
|
||||
args: Parameters<typeof reattachSshPtySessionWithExitFence>[0] & {
|
||||
acceptLivePty: (relayPtyId: string) => void
|
||||
acceptUnverifiablePty: (relayPtyId: string) => void
|
||||
acceptExitedPty: (relayPtyId: string) => void
|
||||
}
|
||||
): Promise<PtySpawnResult> {
|
||||
let result: SshPtyReattachResult | undefined
|
||||
@@ -277,11 +279,7 @@ export async function reattachSshPtySessionForSpawn(
|
||||
try {
|
||||
result = await reattachSshPtySessionWithExitFence(args)
|
||||
} catch (error) {
|
||||
const message = error instanceof Error ? error.message : String(error)
|
||||
if (
|
||||
message.includes(SSH_SESSION_EXPIRED_ERROR) ||
|
||||
message === 'agent_session_exited_during_start'
|
||||
) {
|
||||
if (isSshPtyIdentityMismatchError(error) || isSshPtyExitedEvidenceError(error)) {
|
||||
throw error
|
||||
}
|
||||
// Why: losing contact during verification is unverifiable, not live or exited.
|
||||
@@ -304,6 +302,9 @@ export async function reattachSshPtySessionForSpawn(
|
||||
return spawnResult
|
||||
} catch (error) {
|
||||
await result?.sourceActivationLease?.rollback()
|
||||
if (isSshPtyExitedEvidenceError(error)) {
|
||||
args.acceptExitedPty(result?.id ?? toAppSshPtyId(args.connectionId, args.sessionId))
|
||||
}
|
||||
throw error
|
||||
}
|
||||
}
|
||||
|
||||
@@ -66,6 +66,9 @@ vi.mock('../providers/ssh-pty-provider', () => ({
|
||||
isSshPtyIdentityMismatchError: (err: unknown) =>
|
||||
(err instanceof Error ? err.message : String(err)).includes('identity mismatch'),
|
||||
SshPtyProvider: class MockSshPtyProvider {
|
||||
private livePtyIds = new Set<string>()
|
||||
private unverifiablePtyIds = new Set<string>()
|
||||
private exitedPtyIds = new Set<string>()
|
||||
onData = vi.fn().mockReturnValue(() => {})
|
||||
onReplay = vi.fn().mockReturnValue(() => {})
|
||||
onExit = vi.fn().mockReturnValue(() => {})
|
||||
@@ -73,6 +76,32 @@ vi.mock('../providers/ssh-pty-provider', () => ({
|
||||
attachForReconnect = vi.fn().mockResolvedValue({})
|
||||
setPtyDeliveryPauseAdapter = pauseAdapterMock
|
||||
dispose = vi.fn()
|
||||
|
||||
constructor(
|
||||
_connectionId: string,
|
||||
_mux: unknown,
|
||||
_remoteCliBridgeEnv: unknown,
|
||||
_providerGeneration: number,
|
||||
initialUnverifiablePtyIds: Iterable<string> = []
|
||||
) {
|
||||
this.unverifiablePtyIds = new Set(initialUnverifiablePtyIds)
|
||||
}
|
||||
|
||||
hasPty = (id: string): boolean => this.livePtyIds.has(id) || this.unverifiablePtyIds.has(id)
|
||||
probePtyLiveness = vi.fn(
|
||||
async (id: string): Promise<boolean | null> =>
|
||||
this.livePtyIds.has(id) ? true : this.exitedPtyIds.has(id) ? false : null
|
||||
)
|
||||
acceptUnverifiablePty = vi.fn((id: string) => {
|
||||
this.livePtyIds.delete(id)
|
||||
this.exitedPtyIds.delete(id)
|
||||
this.unverifiablePtyIds.add(id)
|
||||
})
|
||||
acceptExitedPty = vi.fn((id: string) => {
|
||||
this.livePtyIds.delete(id)
|
||||
this.unverifiablePtyIds.delete(id)
|
||||
this.exitedPtyIds.add(id)
|
||||
})
|
||||
}
|
||||
}))
|
||||
|
||||
@@ -206,6 +235,41 @@ describe('SshRelaySession', () => {
|
||||
expect(registerSshGitProvider).toHaveBeenCalledWith('target-1', expect.anything())
|
||||
})
|
||||
|
||||
it('keeps a transiently unreattached PTY unverifiable after the relay becomes ready', async () => {
|
||||
const { mockConn, mockStore, mockPortForward, getMainWindow } = createMockDeps()
|
||||
const { getSshPtyProvider } = await import('../ipc/pty')
|
||||
vi.mocked(getPtyIdsForConnection).mockReturnValue(['pty-unverifiable'])
|
||||
vi.mocked(getSshPtyProvider).mockImplementationOnce(() => {
|
||||
const provider = vi.mocked(registerSshPtyProvider).mock.calls.at(-1)?.[1] as {
|
||||
attachForReconnect: ReturnType<typeof vi.fn>
|
||||
}
|
||||
provider.attachForReconnect.mockRejectedValue(new Error('transport interrupted'))
|
||||
return provider as never
|
||||
})
|
||||
const warn = vi.spyOn(console, 'warn').mockImplementation(() => {})
|
||||
const session = new SshRelaySession('target-1', getMainWindow, mockStore, mockPortForward)
|
||||
|
||||
try {
|
||||
await session.establish(mockConn)
|
||||
const provider = vi.mocked(registerSshPtyProvider).mock.calls.at(-1)?.[1] as {
|
||||
hasPty: (id: string) => boolean
|
||||
probePtyLiveness: (id: string) => Promise<boolean | null>
|
||||
}
|
||||
const appPtyId = 'ssh:target-1@@pty-unverifiable'
|
||||
|
||||
expect(session.getState()).toBe('ready')
|
||||
expect(provider.hasPty(appPtyId)).toBe(true)
|
||||
await expect(provider.probePtyLiveness(appPtyId)).resolves.toBeNull()
|
||||
expect(mockStore.markSshRemotePtyLease).not.toHaveBeenCalledWith(
|
||||
'target-1',
|
||||
'pty-unverifiable',
|
||||
'expired'
|
||||
)
|
||||
} finally {
|
||||
warn.mockRestore()
|
||||
}
|
||||
})
|
||||
|
||||
it('continues provider registration when the relay managed-hook request fails', async () => {
|
||||
process.env.ORCA_FEATURE_REMOTE_AGENT_HOOKS = '1'
|
||||
muxRequestMock.mockImplementation(async (method: string) => {
|
||||
|
||||
@@ -1009,11 +1009,21 @@ export class SshRelaySession {
|
||||
this.wireUpRemoteOrcaCli(mux, connectionIncarnation)
|
||||
|
||||
const providerGeneration = allocateSshPtyProviderGeneration()
|
||||
const initialUnverifiablePtyIds = new Set(
|
||||
[
|
||||
...getPtyIdsForConnection(this.targetId),
|
||||
...this.store
|
||||
.getSshRemotePtyLeases(this.targetId)
|
||||
.filter((lease) => lease.state !== 'terminated' && lease.state !== 'expired')
|
||||
.map((lease) => lease.ptyId)
|
||||
].map((ptyId) => toAppSshPtyId(this.targetId, ptyId))
|
||||
)
|
||||
const ptyProvider = new SshPtyProvider(
|
||||
this.targetId,
|
||||
mux,
|
||||
this.remoteCliBridgeEnv ?? undefined,
|
||||
providerGeneration
|
||||
providerGeneration,
|
||||
initialUnverifiablePtyIds
|
||||
)
|
||||
const consumerOwnerState = this.activePtyConsumerOwner()
|
||||
if (consumerOwnerState) {
|
||||
@@ -2509,7 +2519,7 @@ export class SshRelaySession {
|
||||
if (!shouldContinue()) {
|
||||
return
|
||||
}
|
||||
this.handlePtyReattachFailure(ptyId, appPtyId, pendingReattach, error)
|
||||
this.handlePtyReattachFailure(ptyProvider, ptyId, appPtyId, pendingReattach, error)
|
||||
} finally {
|
||||
recoveryActivationLease?.retire()
|
||||
sourceActivationLease?.rollback()
|
||||
@@ -2649,12 +2659,14 @@ export class SshRelaySession {
|
||||
}
|
||||
|
||||
private handlePtyReattachFailure(
|
||||
ptyProvider: SshPtyProvider,
|
||||
ptyId: string,
|
||||
appPtyId: string,
|
||||
pending: PendingPtyReattach,
|
||||
error: unknown
|
||||
): void {
|
||||
if (!isSshPtyNotFoundError(error)) {
|
||||
ptyProvider.acceptUnverifiablePty?.(appPtyId)
|
||||
pending.restoreRequired = 'reattachAttemptsExhausted'
|
||||
this.wakeRecovery(pending)
|
||||
console.warn(
|
||||
@@ -2672,6 +2684,7 @@ export class SshRelaySession {
|
||||
)
|
||||
return
|
||||
}
|
||||
ptyProvider.acceptExitedPty?.(appPtyId)
|
||||
console.warn(
|
||||
`[ssh-relay-session] Dropping stale PTY ${ptyId} for ${this.targetId} after relay reattach failed: ${
|
||||
error instanceof Error ? error.message : String(error)
|
||||
|
||||
Reference in New Issue
Block a user