mirror of
https://github.com/stablyai/orca.git
synced 2026-10-22 00:03:47 +00:00
fix(ssh): let a due park past the retired-delivery filter, and prove it
The cooldown added in 124e00e8a8 was reactive: it needed a later rejected frame
to reach recovery. But retirement is ours, not the host's — a stalled source keeps
publishing the very token we retired, and acceptPtyData drops those frames before
classification. So the wake-up could never fire in the case that actually happens,
and the pane stayed dark exactly as before.
A parked PTY past its cooldown is now let through that filter. It costs nothing:
the frame is re-classified as rejected and re-retired, so no output reaches the
terminal — it only regains the ability to ask for recovery.
The test shipped with that commit could not have caught this: it woke recovery
with a fabricated NEW delivery token. Retirement holds one key per relay PTY, so
that frame overwrote the key and un-retired the original — the assertion passed
with the wake-up path fully broken. It now reuses the original token throughout,
and reverting the guard above reddens it.
This commit is contained in:
@@ -448,17 +448,20 @@ describe('SshRelaySession rejected PTY delivery recovery', () => {
|
||||
)
|
||||
expect(reattach).toHaveBeenCalledTimes(12)
|
||||
|
||||
// Each later frame must carry its own delivery token — a retired delivery is dropped at the
|
||||
// top of acceptPtyData, so reusing one would make both clauses below pass without proving
|
||||
// anything. A re-announcing source is exactly what a new token looks like.
|
||||
const nextFrame = (n: number) =>
|
||||
rejectedPayload({
|
||||
source: source({ spanId: `token-bad-${n}:0:4`, deliveryToken: `token-bad-${n}` })
|
||||
})
|
||||
|
||||
// Parked: a further rejected frame must not retry yet, or the cooldown means nothing.
|
||||
// Let the exhaustion loop go quiet first: its last retries are timer-driven and would
|
||||
// otherwise land after the clear below and satisfy the final clause on their own.
|
||||
await new Promise((resolve) => setTimeout(resolve, 1500))
|
||||
reattach.mockClear()
|
||||
await internals.acceptPtyData(nextFrame(1))
|
||||
|
||||
// Every frame below reuses the ORIGINAL delivery token, because that is what actually happens:
|
||||
// retirement is ours, not the host's, and a stalled source keeps publishing the token we
|
||||
// retired. It also matters that they are identical — retirement holds ONE key per relay PTY,
|
||||
// so slipping a different token in here would overwrite that key, un-retire the original and
|
||||
// let the final clause pass with the wake-up path still broken.
|
||||
|
||||
// Before the cooldown: parked, so nothing retries.
|
||||
await internals.acceptPtyData(rejectedPayload())
|
||||
await new Promise((resolve) => setTimeout(resolve, 600))
|
||||
expect(reattach).not.toHaveBeenCalled()
|
||||
|
||||
// Backdate the park instead of burning a minute of real time — the property under test is
|
||||
@@ -468,7 +471,7 @@ describe('SshRelaySession rejected PTY delivery recovery', () => {
|
||||
}
|
||||
parked.parkedAt -= 60_000
|
||||
|
||||
await internals.acceptPtyData(nextFrame(2))
|
||||
await internals.acceptPtyData(rejectedPayload())
|
||||
|
||||
await vi.waitFor(() => expect(reattach).toHaveBeenCalled(), { timeout: 10_000 })
|
||||
})
|
||||
|
||||
@@ -1774,7 +1774,8 @@ export class SshRelaySession {
|
||||
const offeredSource = payload.source
|
||||
if (
|
||||
offeredSource &&
|
||||
this.retiredSourceDeliveries.has(payload.providerGeneration, offeredSource)
|
||||
this.retiredSourceDeliveries.has(payload.providerGeneration, offeredSource) &&
|
||||
!this.isParkedRecoveryDue(payload.id)
|
||||
) {
|
||||
return Promise.resolve()
|
||||
}
|
||||
@@ -1831,6 +1832,19 @@ export class SshRelaySession {
|
||||
})
|
||||
}
|
||||
|
||||
/**
|
||||
* A parked PTY whose cooldown has elapsed must be let back past the retired-delivery filter.
|
||||
* Retirement is ours, not the host's: a stalled source keeps publishing the very token we
|
||||
* retired, so that filter would swallow every frame that could re-arm the park and the pane
|
||||
* would stay dark for good — the failure the cooldown exists to rule out. Letting one through
|
||||
* costs nothing: it is re-classified as rejected and re-retired, so no output reaches the
|
||||
* terminal; it only regains the ability to ask for recovery.
|
||||
*/
|
||||
private isParkedRecoveryDue(appPtyId: string): boolean {
|
||||
const attempt = this.rejectedPtyRecoveryAttempts.get(appPtyId)
|
||||
return !!attempt?.parkedAt && Date.now() - attempt.parkedAt >= SSH_REJECTED_PTY_RECOVERY_PARK_MS
|
||||
}
|
||||
|
||||
private recoverRejectedPtyDelivery(
|
||||
payload: SshPtyDataPayload,
|
||||
source: SshPtyDataPayload['source']
|
||||
|
||||
Reference in New Issue
Block a user