mirror of
https://github.com/stablyai/orca.git
synced 2026-10-22 16:03:47 +00:00
fix(ssh): unknown PTY liveness is not proof of death
hasPty is three-state and says so: null means the provider has not listed the host yet — ignorance, not death. The retry gate tested it with `!`, which reads null and false alike, so it ended recovery on ignorance. That state is not exotic: a reconnect builds a fresh provider with an empty set, which is exactly the moment rejected frames arrive. The attempt was deleted and no retry scheduled, and because the delivery token was already retired, no later frame could revive it — the pane stayed dark. It also short-circuited the park cooldown, since parkedAt was never set on an entry that no longer existed. Only an explicit false stops us now. The proven-exit clause still pins that.
This commit is contained in:
@@ -495,6 +495,30 @@ describe('SshRelaySession rejected PTY delivery recovery', () => {
|
||||
expect(mux.dispose).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
// The counterpart to the clause above, and the distinction the whole design rests on. hasPty is
|
||||
// three-state: null means the provider has not listed the host yet, which is precisely the state
|
||||
// a fresh provider is in right after the reconnect that produces rejected frames. Reading that
|
||||
// as death drops the attempt and schedules nothing, and the delivery token is already retired,
|
||||
// so nothing can revive it — the pane stays dark. Only an explicit false is proof.
|
||||
it('keeps retrying while the PTY liveness is merely unknown', async () => {
|
||||
const { internals, mux, session } = prepareSession()
|
||||
const reattach = vi.fn().mockResolvedValue(false)
|
||||
internals.reattachRejectedPty = reattach
|
||||
getSshPtyProviderMock.mockReturnValue({ hasPty: () => null } as unknown as SshPtyProvider)
|
||||
const onTerminalError = vi.fn()
|
||||
session.setOnTerminalRelayError(onTerminalError)
|
||||
vi.spyOn(console, 'warn').mockImplementation(() => {})
|
||||
|
||||
await internals.acceptPtyData(rejectedPayload())
|
||||
|
||||
// More than one attempt is the whole point: unknown liveness must not end recovery.
|
||||
await vi.waitFor(() => expect(reattach.mock.calls.length).toBeGreaterThan(1), {
|
||||
timeout: 10_000
|
||||
})
|
||||
expect(onTerminalError).not.toHaveBeenCalled()
|
||||
expect(mux.dispose).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
// Why bounded: the bulk reconnect path caps the identical attach work at 8, and a relay that
|
||||
// starts rejecting across every PTY at once would otherwise open one attach round trip per PTY.
|
||||
it('caps concurrent targeted reattaches and coalesces repeats for one PTY', async () => {
|
||||
|
||||
@@ -1939,7 +1939,16 @@ export class SshRelaySession {
|
||||
appPtyId: string
|
||||
): void {
|
||||
const ptyProvider = getSshPtyProvider(this.targetId) as SshPtyProvider | undefined
|
||||
if (!ptyProvider || typeof ptyProvider.hasPty !== 'function' || !ptyProvider.hasPty(appPtyId)) {
|
||||
// Why `=== false` and not `!`: hasPty is three-state, and null means the provider has not
|
||||
// listed the host yet — ignorance, not death, which is exactly the state a fresh provider is
|
||||
// in right after the reconnect that produces these rejected frames. Treating it as death
|
||||
// dropped the recovery attempt and scheduled nothing, and since the delivery token is already
|
||||
// retired, no later frame could revive it: the pane stayed dark. Only proof stops us.
|
||||
if (
|
||||
!ptyProvider ||
|
||||
typeof ptyProvider.hasPty !== 'function' ||
|
||||
ptyProvider.hasPty(appPtyId) === false
|
||||
) {
|
||||
this.rejectedPtyRecoveryAttempts.delete(appPtyId)
|
||||
return
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user