mirror of
https://github.com/stablyai/orca.git
synced 2026-09-23 16:02:24 +00:00
fix(ssh): stop fabricating an exit when a reattach fails
A failed attach never proves the shell exited. The relay answers not-found for
a pane-identity mismatch and for any id it merely cannot hand back, so treating
it as death sent the pane a synthetic `pty:exit { code: -1 }`, cleared provider
state, deleted ownership and expired the lease — four claims about a process we
know nothing about, on a shell that is usually still running.
Collapse every failure into the non-destructive branch that already existed a
few lines above (`restoreRequired = 'reattachAttemptsExhausted'` + wakeRecovery).
A branch collapse, not a new mechanism: goalpost S3.
Two tests pinned the deleted premise and are INVERTED rather than patched, so
the new intent stays covered:
- ssh-relay-orphan-abandon-paths: "retires the lease without a kill when the
relay proves the PTY is gone" -> "leaves the shell running when the relay only
reports the PTY as not found". Its comment claimed attach verifies liveness
before answering not-found; it does not.
- ssh-relay-session: "invalidates and broadcasts remote PTYs that cannot
reattach" -> "leaves an unreattachable remote PTY alone while its sibling
reattaches".
Also repairs two clauses left red by c51be8072b (step A), which dropped the
expected-identity parameter and the expectedIdentityByPtyId map.
Mutation proof: restoring the destructive block reddens 6 of the 8 oracle
clauses in ssh-relay-reattach-exit-proof.test.ts; the 2 producer pins stay
green. Verified the mutation landed before believing the result.
Net production: -21 lines.
Co-authored-by: Orca <help@stably.ai>
This commit is contained in:
@@ -186,24 +186,24 @@ describe('SshRelaySession abandoned remote PTYs', () => {
|
||||
expect(clearProviderPtyState).not.toHaveBeenCalledWith(APP_PTY_ID)
|
||||
})
|
||||
|
||||
it('retires the lease without a kill when the relay proves the PTY is gone', async () => {
|
||||
// pty.attach verifies process liveness before answering not-found, so this is the one branch
|
||||
// with positive proof of death — and a dead process needs no shutdown request.
|
||||
// INVERTED for STA-3077 step E-0. This case previously asserted that a bare not-found retires the
|
||||
// lease and reports `pty:exit { code: -1 }`, on the premise that attach proves liveness before
|
||||
// answering not-found. It does not: the relay answers not-found for a pane-identity mismatch and
|
||||
// for any id it merely cannot hand back, so the premise licensed a fabricated death certificate.
|
||||
// The clause is kept — inverted — so the new intent stays covered rather than silently dropped.
|
||||
it('leaves the shell running when the relay only reports the PTY as not found', async () => {
|
||||
const { deps, shutdown } = await establishWithFailingReattach(
|
||||
new Error('PTY "pty-live" not found')
|
||||
)
|
||||
|
||||
expect(shutdown).not.toHaveBeenCalled()
|
||||
expect(deps.mockStore.markSshRemotePtyLease).toHaveBeenCalledWith(
|
||||
expect(deps.mockStore.markSshRemotePtyLease).not.toHaveBeenCalledWith(
|
||||
'target-1',
|
||||
'pty-live',
|
||||
'expired'
|
||||
)
|
||||
expect(clearProviderPtyState).toHaveBeenCalledWith(APP_PTY_ID)
|
||||
expect(deps.mockWindow.webContents.send).toHaveBeenCalledWith('pty:exit', {
|
||||
id: APP_PTY_ID,
|
||||
code: -1
|
||||
})
|
||||
expect(clearProviderPtyState).not.toHaveBeenCalledWith(APP_PTY_ID)
|
||||
expect(deps.mockWindow.webContents.send).not.toHaveBeenCalledWith('pty:exit', expect.anything())
|
||||
})
|
||||
|
||||
it('keeps a recovered session attached when reattach succeeds after an earlier drop', async () => {
|
||||
|
||||
@@ -246,7 +246,8 @@ describe('SshRelaySession rejected PTY delivery recovery', () => {
|
||||
const recovery = reattachKnownPty.mock.calls[0]?.[0]
|
||||
expect(recovery?.ptyId).toBe('pty-bad')
|
||||
expect(Array.from(recovery?.activeLeaseByPtyId.keys() ?? [])).toEqual(['pty-bad'])
|
||||
expect(Array.from(recovery?.expectedIdentityByPtyId.keys() ?? [])).toEqual([])
|
||||
// The sibling `expectedIdentityByPtyId` clause is gone with the map itself (STA-3077 step A):
|
||||
// reattach no longer carries pane identity at all, which subsumes "carries none for this pty".
|
||||
expect(recovery?.targetedDeliveryRecovery).toBe('fresh-activation')
|
||||
expect(deps.mockStore.markSshRemotePtyLeasesAttachedAsync).toHaveBeenCalledWith('target-1', [
|
||||
'pty-bad'
|
||||
@@ -356,7 +357,8 @@ describe('SshRelaySession rejected PTY delivery recovery', () => {
|
||||
internals.reattachRejectedPty('pty-bad', mux, 23, 'confirm-existing')
|
||||
).resolves.toBe(true)
|
||||
|
||||
expect(attachForReconnect).toHaveBeenCalledWith('pty-bad', undefined, checkpoint)
|
||||
// Two args since step A dropped the expected-identity parameter from the attach call.
|
||||
expect(attachForReconnect).toHaveBeenCalledWith('pty-bad', checkpoint)
|
||||
expect(commit).toHaveBeenCalledOnce()
|
||||
expect(mux.dispose).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
@@ -648,7 +648,11 @@ describe('SshRelaySession', () => {
|
||||
expect(mockStore.markSshRemotePtyLeasesAttachedAsync).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('invalidates and broadcasts remote PTYs that cannot reattach after relay reconnect', async () => {
|
||||
// INVERTED for STA-3077 step E-0. This case previously asserted that a PTY the relay reports as
|
||||
// not-found is invalidated and broadcast as an exit. A not-found is not proof of death, so the
|
||||
// broadcast was a fabricated one; what must still hold is that the healthy sibling reattaches and
|
||||
// the unproven one is left alone rather than written off.
|
||||
it('leaves an unreattachable remote PTY alone while its sibling reattaches', async () => {
|
||||
const { mockConn, mockStore, mockPortForward, getMainWindow, mockWindow } = createMockDeps()
|
||||
const session = new SshRelaySession('target-1', getMainWindow, mockStore, mockPortForward)
|
||||
await session.establish(mockConn)
|
||||
@@ -670,12 +674,9 @@ describe('SshRelaySession', () => {
|
||||
|
||||
expect(mockAttach).toHaveBeenCalledWith('pty-stale')
|
||||
expect(mockAttach).toHaveBeenCalledWith('pty-live')
|
||||
expect(clearProviderPtyState).toHaveBeenCalledWith('ssh:target-1@@pty-stale')
|
||||
expect(deletePtyOwnership).toHaveBeenCalledWith('ssh:target-1@@pty-stale')
|
||||
expect(mockWindow.webContents.send).toHaveBeenCalledWith('pty:exit', {
|
||||
id: 'ssh:target-1@@pty-stale',
|
||||
code: -1
|
||||
})
|
||||
expect(clearProviderPtyState).not.toHaveBeenCalledWith('ssh:target-1@@pty-stale')
|
||||
expect(deletePtyOwnership).not.toHaveBeenCalledWith('ssh:target-1@@pty-stale')
|
||||
expect(mockWindow.webContents.send).not.toHaveBeenCalledWith('pty:exit', expect.anything())
|
||||
})
|
||||
|
||||
it('retries transient reattach failure without tearing down provider registration', async () => {
|
||||
|
||||
@@ -11,7 +11,7 @@ import { SshPtyProvider } from '../providers/ssh-pty-provider'
|
||||
import type { SshPtyAttachResult } from '../providers/ssh-pty-session-reattach'
|
||||
import type { SshPtyDataCallback, SshPtyExitCallback } from '../providers/ssh-pty-provider-contract'
|
||||
import type { SshPtyRecoveryActivationLease } from '../providers/ssh-pty-notification-routing'
|
||||
import { isSshPtyIdentityMismatchError, isSshPtyNotFoundError } from '../providers/ssh-pty-errors'
|
||||
import { isSshPtyNotFoundError } from '../providers/ssh-pty-errors'
|
||||
import { toAppSshPtyId, toRelaySshPtyId } from '../providers/ssh-pty-id'
|
||||
import { SshFilesystemProvider } from '../providers/ssh-filesystem-provider'
|
||||
import { isMethodNotFoundError } from './ssh-filesystem-stream-reader'
|
||||
@@ -2418,7 +2418,7 @@ export class SshRelaySession {
|
||||
if (!shouldContinue()) {
|
||||
return
|
||||
}
|
||||
this.handlePtyReattachFailure(ptyId, appPtyId, pendingReattach, error)
|
||||
this.handlePtyReattachFailure(ptyId, pendingReattach, error)
|
||||
} finally {
|
||||
recoveryActivationLease?.retire()
|
||||
sourceActivationLease?.rollback()
|
||||
@@ -2561,40 +2561,19 @@ export class SshRelaySession {
|
||||
|
||||
private handlePtyReattachFailure(
|
||||
ptyId: string,
|
||||
appPtyId: string,
|
||||
pending: PendingPtyReattach,
|
||||
error: unknown
|
||||
): void {
|
||||
if (!isSshPtyNotFoundError(error)) {
|
||||
pending.restoreRequired = 'reattachAttemptsExhausted'
|
||||
this.wakeRecovery(pending)
|
||||
console.warn(
|
||||
`[ssh-relay-session] Leaving PTY ${ptyId} detached for ${this.targetId} after bounded reattach attempts failed: ${
|
||||
error instanceof Error ? error.message : String(error)
|
||||
}`
|
||||
)
|
||||
return
|
||||
}
|
||||
if (isSshPtyIdentityMismatchError(error)) {
|
||||
console.warn(
|
||||
`[ssh-relay-session] Ignoring stale PTY ${ptyId} for ${this.targetId} after relay identity mismatch: ${
|
||||
error instanceof Error ? error.message : String(error)
|
||||
}`
|
||||
)
|
||||
return
|
||||
}
|
||||
// No attach failure proves the shell exited — not even a not-found, which the relay also
|
||||
// returns when it merely cannot hand this id back. Only an exit the relay observed on a live
|
||||
// stream may report one, so every failure leaves the pane detached and recoverable.
|
||||
pending.restoreRequired = 'reattachAttemptsExhausted'
|
||||
this.wakeRecovery(pending)
|
||||
console.warn(
|
||||
`[ssh-relay-session] Dropping stale PTY ${ptyId} for ${this.targetId} after relay reattach failed: ${
|
||||
`[ssh-relay-session] Leaving PTY ${ptyId} detached for ${this.targetId} after reattach failed: ${
|
||||
error instanceof Error ? error.message : String(error)
|
||||
}`
|
||||
)
|
||||
clearProviderPtyState(appPtyId)
|
||||
deletePtyOwnership(appPtyId)
|
||||
this.store.markSshRemotePtyLease(this.targetId, ptyId, 'expired')
|
||||
const win = this.getMainWindow()
|
||||
if (win && !win.isDestroyed()) {
|
||||
win.webContents.send('pty:exit', { id: appPtyId, code: -1 })
|
||||
}
|
||||
}
|
||||
|
||||
private async sourceRecoveryRequest(
|
||||
|
||||
Reference in New Issue
Block a user