mirror of
https://github.com/stablyai/orca.git
synced 2026-09-22 00:02:31 +00:00
fix(ssh): release the relay-loss watcher before teardown mux writes (#13737)
* fix(ssh): release the relay-loss watcher before teardown mux writes
Cancelling the in-flight port scan emits rpc.cancel on the control lane.
If that lane is saturated, enqueue admission fails and the writer calls
fail() -> mux.dispose('connection_lost'), which fires the relay-loss
watcher during our own teardown and schedules a redundant relay redeploy.
teardownProviders already released the watcher first, but stopPortScanning
runs ahead of it and is what emits the frame. Hoist the release into a
named method and call it before stopPortScanning on all four teardown
paths.
Co-authored-by: Orca <help@stably.ai>
* fix(ssh): hoist the watcher release above abort on every teardown path
Review follow-up. Two nits from the readiness pass:
- The "stop scanning before teardownProviders" comment had drifted onto
releaseRelayLossWatcher(); move it back above the stopPortScanning()
call it describes.
- Release the watcher ahead of abortController.abort() too. That signal
reaches no mux request today, so this is not a live hole, but plumbing
it into one would have silently reopened the bug on three paths. Making
the release first keeps the invariant structural rather than incidental.
detachSshPtyConsumerRecovery stays first on the two detach paths, per the
existing synchronous-half-first rule.
Co-authored-by: Orca <help@stably.ai>
---------
Co-authored-by: Orca <help@stably.ai>
This commit is contained in:
@@ -232,6 +232,53 @@ describe('SshRelaySession relay loss during setup', () => {
|
||||
expect(unregisterSshFilesystemProvider).toHaveBeenCalledWith('target-1')
|
||||
})
|
||||
|
||||
// #13548 follow-up: cancelling the in-flight port scan emits rpc.cancel on the control
|
||||
// lane, which a saturated writer turns into mux.dispose('connection_lost'). That fires
|
||||
// during our own teardown, so it must not be reported as a lost relay.
|
||||
it.each([
|
||||
['detach', (session: SshRelaySession) => session.beginShutdownDetach()],
|
||||
['dispose', (session: SshRelaySession) => session.dispose()]
|
||||
])(
|
||||
'does not report relay loss when the port-scan cancel kills the mux on %s',
|
||||
async (_path, teardown) => {
|
||||
const { session, onRelayLost } = createSession()
|
||||
// Mirrors SshChannelMultiplexer.request: an in-flight request whose signal aborts
|
||||
// emits rpc.cancel, and the mock mux dies on that notify like a saturated writer.
|
||||
muxRequestMock.mockImplementation(
|
||||
async (method: string, _params?: unknown, options?: { signal?: AbortSignal }) => {
|
||||
if (method !== 'ports.detect') {
|
||||
return []
|
||||
}
|
||||
const mux = session.getMux() as unknown as {
|
||||
failNotifyMethod: string | null
|
||||
notify: (method: string, params?: unknown) => void
|
||||
}
|
||||
mux.failNotifyMethod = 'rpc.cancel'
|
||||
return new Promise((_resolve, reject) => {
|
||||
options?.signal?.addEventListener(
|
||||
'abort',
|
||||
() => {
|
||||
mux.notify('rpc.cancel', { id: 1 })
|
||||
reject(new Error('cancelled'))
|
||||
},
|
||||
{ once: true }
|
||||
)
|
||||
})
|
||||
}
|
||||
)
|
||||
|
||||
await session.establish({} as SshConnection)
|
||||
expect(session.getState()).toBe('ready')
|
||||
expect(muxRequestMock).toHaveBeenCalledWith('ports.detect', undefined, expect.anything())
|
||||
const mux = session.getMux() as unknown as { notify: ReturnType<typeof vi.fn> }
|
||||
|
||||
teardown(session)
|
||||
|
||||
expect(mux.notify).toHaveBeenCalledWith('rpc.cancel', { id: 1 })
|
||||
expect(onRelayLost).not.toHaveBeenCalled()
|
||||
}
|
||||
)
|
||||
|
||||
// #11953: reattachKnownPtys swallows every per-PTY failure, so a mux killed by the
|
||||
// reattach burst itself never reaches the catch — the post-reattach gate has to notice.
|
||||
it('routes a mux that dies during PTY reattach into relay-loss recovery', async () => {
|
||||
|
||||
@@ -640,6 +640,7 @@ export class SshRelaySession {
|
||||
return
|
||||
}
|
||||
|
||||
this.releaseRelayLossWatcher()
|
||||
// Cancel any in-flight reconnect
|
||||
this.abortController?.abort()
|
||||
const abortController = new AbortController()
|
||||
@@ -835,6 +836,7 @@ export class SshRelaySession {
|
||||
private runDisposal(pendingDetach: Promise<void> | null): Promise<void> {
|
||||
// Why: the whole in-memory half runs before any await so a concurrent connect can never observe
|
||||
// a half-torn session; only the durability barriers below are deferred onto the returned promise.
|
||||
this.releaseRelayLossWatcher()
|
||||
this.abortController?.abort()
|
||||
this.stopPortScanning()
|
||||
this.broadcastEmptyLists()
|
||||
@@ -908,6 +910,7 @@ export class SshRelaySession {
|
||||
return
|
||||
}
|
||||
detachSshPtyConsumerRecovery(this.targetId, this.ptyConsumerClientInstanceId)
|
||||
this.releaseRelayLossWatcher()
|
||||
this.abortController?.abort()
|
||||
this.stopPortScanning()
|
||||
this.broadcastEmptyLists()
|
||||
@@ -927,6 +930,7 @@ export class SshRelaySession {
|
||||
// step — a fast reconnect must reclaim this identity instead of minting one, even if a
|
||||
// teardown call below throws unexpectedly.
|
||||
detachSshPtyConsumerRecovery(this.targetId, this.ptyConsumerClientInstanceId)
|
||||
this.releaseRelayLossWatcher()
|
||||
this.abortController?.abort()
|
||||
this.stopPortScanning()
|
||||
this.broadcastEmptyLists()
|
||||
@@ -947,9 +951,21 @@ export class SshRelaySession {
|
||||
|
||||
// ── Private ───────────────────────────────────────────────────────
|
||||
|
||||
// Why: teardown itself can kill the mux — an aborted request emits rpc.cancel, and a saturated
|
||||
// control lane turns that admission failure into mux.dispose('connection_lost'). Every teardown
|
||||
// path must release the watcher before its first mux write, or our own shutdown re-enters
|
||||
// recovery as a spurious relay loss. teardownProviders is not early enough: stopPortScanning
|
||||
// runs ahead of it and is what emits that frame. Call this ahead of abortController.abort()
|
||||
// too — that signal reaches no mux request today, but plumbing it into one would otherwise
|
||||
// reopen the same hole silently.
|
||||
private releaseRelayLossWatcher(): void {
|
||||
this.muxDisposeCleanup?.()
|
||||
this.muxDisposeCleanup = null
|
||||
}
|
||||
|
||||
// Why: onStateChange only fires on SSH-level reconnects, so watch for relay-channel loss while SSH stays up and fire onRelayLost.
|
||||
private watchMuxForRelayLoss(mux: SshChannelMultiplexer): void {
|
||||
this.muxDisposeCleanup?.()
|
||||
this.releaseRelayLossWatcher()
|
||||
this.muxDisposeCleanup = mux.onDispose((reason) => {
|
||||
if (reason === 'connection_lost' && this.mux === mux && !this.isDisposed()) {
|
||||
console.warn(
|
||||
@@ -1560,8 +1576,7 @@ export class SshRelaySession {
|
||||
reason: 'shutdown' | 'connection_lost',
|
||||
outputGenerationReason: string = reason
|
||||
): void {
|
||||
this.muxDisposeCleanup?.()
|
||||
this.muxDisposeCleanup = null
|
||||
this.releaseRelayLossWatcher()
|
||||
this.muxNotificationCleanup?.()
|
||||
this.muxNotificationCleanup = null
|
||||
for (const cleanup of this.ptyRecoveryNotificationCleanups) {
|
||||
|
||||
Reference in New Issue
Block a user