mirror of
https://github.com/stablyai/orca.git
synced 2026-10-27 08:02:51 +00:00
fix(ssh): send the pane its screen back on reconnect
A reconnect left every remote terminal blank. Measured on a live relay, not inferred: pty.attach returned no replay for every pane, taking the activation === 'existing' early return in the relay's attach. 'existing' means a source delivery is already open for this client, so it must already be receiving live output and cannot need its screen re-sent. That holds for a duplicate attach. It is false for a reconnect, for a reason neither side can see alone: the client keeps its id across the drop (detachClient refuses to detach the primary, and setWrite revives that same id) so the delivery outlives the dead transport, while the RENDERER has already thrown its terminal away. A reconnect bumps tab.generation, which is the pane's React key, so TerminalPane remounts and the old xterm is disposed with its buffer, and nothing on that path captures it first. Both halves are individually reasonable and together they guarantee a blank pane: the relay reports the client already has the screen, to a client holding a brand-new empty terminal, and nothing paints until new output happens to arrive. Resizing appeared to fix it only because a TUI redraws itself. So the client says which case it is. reattachSshPtySession is by definition painting into a new terminal, so it asks; nobody else does, and the early return keeps working for them. Optional on the wire, so an older relay ignores it and behaves exactly as it does today. Falling through rather than returning the replay inline is deliberate: the path below already drops the pending batched bytes that are also in the buffer, which is what stops the live delivery rendering them twice. Reproduced first as a test against the real dispatcher, source publication and PTY handler (the second attach for one client, which is what a reconnect is) and it fails on the exact symptom before the fix. A second test pins that a caller which does NOT ask still gets nothing, so this cannot become a double-render for the duplicate-attach case the early return exists for. Also updates four provider tests that assert the exact attach params. NOT yet verified in the running app; the log will show replay=true on reconnect.
This commit is contained in:
@@ -584,7 +584,8 @@ describe('SshPtyProvider', () => {
|
||||
id: 'pty-old',
|
||||
cols: 80,
|
||||
rows: 24,
|
||||
suppressReplayNotification: true
|
||||
suppressReplayNotification: true,
|
||||
requireReplay: true
|
||||
},
|
||||
sourceActivationRequestOptions
|
||||
)
|
||||
@@ -630,7 +631,8 @@ describe('SshPtyProvider', () => {
|
||||
id: 'pty-old',
|
||||
cols: 80,
|
||||
rows: 24,
|
||||
suppressReplayNotification: true
|
||||
suppressReplayNotification: true,
|
||||
requireReplay: true
|
||||
})
|
||||
expect(result).toEqual({
|
||||
id: 'ssh:conn-1@@pty-old',
|
||||
@@ -655,6 +657,7 @@ describe('SshPtyProvider', () => {
|
||||
cols: 80,
|
||||
rows: 24,
|
||||
suppressReplayNotification: true,
|
||||
requireReplay: true,
|
||||
expectedPaneKey: 'tab-a:leaf-a',
|
||||
expectedTabId: 'tab-a'
|
||||
})
|
||||
@@ -674,7 +677,8 @@ describe('SshPtyProvider', () => {
|
||||
id: 'pty-old',
|
||||
cols: 80,
|
||||
rows: 24,
|
||||
suppressReplayNotification: true
|
||||
suppressReplayNotification: true,
|
||||
requireReplay: true
|
||||
},
|
||||
sourceActivationRequestOptions
|
||||
)
|
||||
|
||||
@@ -196,6 +196,12 @@ export async function reattachSshPtySession(args: {
|
||||
cols: args.options.cols,
|
||||
rows: args.options.rows,
|
||||
suppressReplayNotification: true,
|
||||
// A reattach always paints into a NEW terminal: a reconnect bumps tab.generation, which is
|
||||
// the pane's React key, so TerminalPane remounts and the old xterm is disposed with its
|
||||
// buffer. Without this the relay sees a delivery still open under our unchanged client id,
|
||||
// answers "you already have this", and the pane stays blank until new output arrives.
|
||||
// Optional on the wire, so an older relay simply ignores it and behaves as it does today.
|
||||
requireReplay: true,
|
||||
...(expectedPaneKey ? { expectedPaneKey } : {}),
|
||||
...(expectedTabId ? { expectedTabId } : {})
|
||||
},
|
||||
@@ -203,7 +209,8 @@ export async function reattachSshPtySession(args: {
|
||||
rememberPtyIncarnation: args.rememberPtyIncarnation
|
||||
})
|
||||
console.warn(
|
||||
`[ssh-pty] pty.attach succeeded for ${args.sessionId}, replay=${!!attachResult.replay}`
|
||||
`[ssh-pty] pty.attach succeeded for ${args.sessionId}, replay=${!!attachResult.replay}` +
|
||||
`, replayLen=${attachResult.replay?.length ?? 0}`
|
||||
)
|
||||
return {
|
||||
id: toAppSshPtyId(args.connectionId, relaySessionId),
|
||||
|
||||
@@ -769,4 +769,47 @@ describe('PtyHandler negotiated source publication', () => {
|
||||
expect(publication.getDebugSnapshot()).toMatchObject({ sendCommitted: 1 })
|
||||
expect(pausePty).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
// The reported defect, in the shape measured against the live relay: the client attaches twice
|
||||
// for the same PTY. The first registers a source delivery under the primary client's id; the
|
||||
// second finds `current.clientId === context.clientId`, answers 'existing', and returns NO
|
||||
// replay. Live, every pane on reconnect logged path=existing-delivery and painted nothing.
|
||||
//
|
||||
// A reattach always lands in a NEW terminal — a reconnect bumps tab.generation, which is the
|
||||
// pane's React key, so TerminalPane remounts and the old xterm is disposed with its buffer.
|
||||
describe('a second attach for the same client', () => {
|
||||
async function attach(id: number, params: Record<string, unknown>) {
|
||||
writes = []
|
||||
dispatcher.feed(requestFrame(id, 'pty.attach', { id: 'pty-1', ...params }))
|
||||
await vi.advanceTimersByTimeAsync(0)
|
||||
return writes.map((buffer) => responseResult(buffer, id)).find(Boolean)
|
||||
}
|
||||
|
||||
beforeEach(async () => {
|
||||
await spawn({})
|
||||
dataCallback!('scrollback-that-must-survive')
|
||||
await vi.advanceTimersByTimeAsync(10)
|
||||
await attach(10, { suppressReplayNotification: true })
|
||||
})
|
||||
|
||||
it('replays the scrollback when the client says it needs it', async () => {
|
||||
const second = await attach(11, {
|
||||
suppressReplayNotification: true,
|
||||
requireReplay: true
|
||||
})
|
||||
|
||||
expect(second, 'second attach returned no response').toBeTruthy()
|
||||
expect(second!.replay).toContain('scrollback-that-must-survive')
|
||||
})
|
||||
|
||||
// The early return is right for a duplicate attach from a client that IS still receiving the
|
||||
// stream; only a client that has thrown its terminal away should ask. Pinned so the fix stays
|
||||
// opt-in and cannot start double-rendering for callers that never asked.
|
||||
it('still sends nothing when the client does not ask', async () => {
|
||||
const second = await attach(12, { suppressReplayNotification: true })
|
||||
|
||||
expect(second, 'second attach returned no response').toBeTruthy()
|
||||
expect(second!.replay).toBeUndefined()
|
||||
})
|
||||
})
|
||||
})
|
||||
|
||||
@@ -1732,7 +1732,23 @@ export class PtyHandler {
|
||||
...(sourceActivation ? { sourceActivation } : {})
|
||||
}
|
||||
}
|
||||
if (activation === 'existing' && this.sourcePublication?.accepts(id)) {
|
||||
// `existing` means a delivery is already open for this client, so it is already receiving live
|
||||
// output and does not need its screen re-sent. That is true for a duplicate attach — and false
|
||||
// for the case this skipped: an SSH reconnect. The client keeps its id there (the dispatcher
|
||||
// refuses to detach the primary, and setWrite revives that same id), so the delivery outlives
|
||||
// the dead transport, while the renderer has thrown its terminal away — a reconnect bumps
|
||||
// tab.generation, which is the React key, so the pane remounts with a brand-new empty xterm and
|
||||
// nothing captures the old buffer. Answering "you already have it" leaves the pane blank
|
||||
// forever, until new output happens to arrive.
|
||||
//
|
||||
// So the client says which it is. Falling through rather than returning the replay inline is
|
||||
// deliberate: the path below already drops the pending batched bytes that are also in the
|
||||
// buffer, which is what stops the live delivery double-rendering them.
|
||||
if (
|
||||
activation === 'existing' &&
|
||||
this.sourcePublication?.accepts(id) &&
|
||||
params.requireReplay !== true
|
||||
) {
|
||||
return {
|
||||
incarnationId: managed.incarnationId,
|
||||
...(sourceActivation ? { sourceActivation } : {})
|
||||
|
||||
Reference in New Issue
Block a user