From 0b2e7a23443b68eaa451075a461d8b7b0e86128a Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Thu, 20 Aug 2026 02:43:38 -0700 Subject: [PATCH] fix(ssh): deliver a remote pane's exit that no operation could attribute classifyPendingExit stamped every same-id exit as classified before deciding its verdict, including the 'unverifiable' returns where the operation said it could not attribute the exit at all. On relays that send no incarnation ids, an operation that bound to the PTY after the exit was recorded could stamp the quarantine it does not hold; the real holder's finish() then refused to release it and zero pty:exit reached the renderer for a PTY the host reported dead. Claim an exit only when the operation actually attributed it: it owned the PTY when the exit arrived, or both incarnations are known and comparable. recordExit also returned false for an unheld exit and let the notification router publish it directly, bypassing the quarantine's published latch, so a late binder that later attributed the same exit published a second pane death. Publish it through the latch instead and report that the tracker owns it. --- .../providers/ssh-pty-spawn-exit-race.test.ts | 69 +++++++++++++++++++ src/main/providers/ssh-pty-spawn-exit-race.ts | 21 +++++- 2 files changed, 87 insertions(+), 3 deletions(-) diff --git a/src/main/providers/ssh-pty-spawn-exit-race.test.ts b/src/main/providers/ssh-pty-spawn-exit-race.test.ts index 4a911e4b8fb..19624dd55df 100644 --- a/src/main/providers/ssh-pty-spawn-exit-race.test.ts +++ b/src/main/providers/ssh-pty-spawn-exit-race.test.ts @@ -30,3 +30,72 @@ it('does not release a quarantined exit when one operation finishes twice', () = expect(publish).not.toHaveBeenCalled() }) + +it('releases a held exit when a late binder without an incarnation cannot attribute it', () => { + const tracker = new SshPtySpawnExitRaceTracker() + const publish = vi.fn() + const holder = tracker.begin('pty-1') + const late = tracker.begin() + + // Relays that send no incarnation id leave both sides unidentified, so nothing is attributable. + tracker.recordExit('pty-1', undefined, publish) + tracker.bind(late, 'pty-1') + expect(tracker.classifyPendingExit(late, { id: 'pty-1' })).toBe('unverifiable') + tracker.finish(late) + tracker.finish(holder) + + expect(publish).toHaveBeenCalledOnce() +}) + +it('releases a held exit when a late binder attaches without an incarnation', () => { + const tracker = new SshPtySpawnExitRaceTracker() + const publish = vi.fn() + const holder = tracker.begin('pty-1') + const late = tracker.begin() + + tracker.recordExit('pty-1', 'incarnation-old', publish) + tracker.bind(late, 'pty-1') + expect(tracker.classifyPendingExit(late, { id: 'pty-1' })).toBe('unverifiable') + tracker.finish(late) + tracker.finish(holder) + + expect(publish).toHaveBeenCalledOnce() +}) + +it('publishes an unheld exit once even after a late binder attributes it', () => { + const tracker = new SshPtySpawnExitRaceTracker() + const publish = vi.fn() + const late = tracker.begin() + + // The notification router publishes whatever the tracker does not take ownership of. + if (!tracker.recordExit('pty-1', 'incarnation-1', publish)) { + publish() + } + tracker.bind(late, 'pty-1') + expect(tracker.classifyPendingExit(late, { id: 'pty-1', incarnationId: 'incarnation-1' })).toBe( + 'exited' + ) + tracker.finish(late) + + expect(publish).toHaveBeenCalledOnce() +}) + +it('cannot recall an exit released before a late binder attributes it', () => { + const tracker = new SshPtySpawnExitRaceTracker() + const publish = vi.fn() + const holder = tracker.begin('pty-1') + const late = tracker.begin() + + tracker.recordExit('pty-1', 'incarnation-1', publish) + // holders reaches zero before the late binder reaches its verdict, so the exit escapes. + tracker.finish(holder) + expect(publish).toHaveBeenCalledOnce() + + tracker.bind(late, 'pty-1') + expect(tracker.classifyPendingExit(late, { id: 'pty-1', incarnationId: 'incarnation-1' })).toBe( + 'exited' + ) + tracker.finish(late) + + expect(publish).toHaveBeenCalledOnce() +}) diff --git a/src/main/providers/ssh-pty-spawn-exit-race.ts b/src/main/providers/ssh-pty-spawn-exit-race.ts index 9ac2fbceebd..859868ea1bb 100644 --- a/src/main/providers/ssh-pty-spawn-exit-race.ts +++ b/src/main/providers/ssh-pty-spawn-exit-race.ts @@ -32,6 +32,7 @@ export class SshPtySpawnExitRaceTracker { operation.relayPtyId = relayPtyId } + /** True when the tracker owns publication of this exit: deferred to a holder, or already sent. */ recordExit(relayPtyId: string, incarnationId: unknown, publish?: () => void): boolean { let published = false const quarantine: QuarantinedSshPtyExit = { @@ -60,7 +61,15 @@ export class SshPtySpawnExitRaceTracker { ...(tracked ? { held, quarantine } : {}) }) } - return quarantine.holders > 0 + if (publish === undefined) { + return false + } + if (quarantine.holders === 0) { + // Why: no operation can fence this exit, so send it now — through the quarantine, so a + // later binder that attributes it cannot deliver the same death a second time. + quarantine.publish() + } + return true } classifyPendingExit( @@ -69,8 +78,14 @@ export class SshPtySpawnExitRaceTracker { ): SshPtyPendingExitOutcome { const sameIdExits = operation.exits.filter((exit) => exit.relayPtyId === result.id) for (const exit of sameIdExits) { - // Why: a reached verdict owns the exit, so finish() must not second-guess it and re-publish. - if (exit.quarantine) { + // Why: classifying claims the exit and silences its holder's release, so only claim what this + // operation actually attributed — it owned the PTY when the exit arrived, or both incarnations + // are known and comparable. A late binder that cannot compare identities has proven nothing, + // and stamping its "I can't tell" would strand a death the host reported. + const attributed = + exit.held === true || + (result.incarnationId !== undefined && exit.incarnationId !== undefined) + if (exit.quarantine && attributed) { exit.quarantine.classified = true } }