From 2c16edc200d985572e077c079c74fba314492ec5 Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Sun, 2 Aug 2026 23:49:48 -0700 Subject: [PATCH] fix(ssh): two lease-retirement primitives, split by evidence (STA-3077, part 2/5) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every future retirement goes through exactly one of two Store methods, split by what the evidence actually proves rather than by which handler produced it: - retireLeaseAndReap — the pane is provably gone, so the remote PTY is ours to kill. Sets pendingRemoteShutdown. - retireLeaseSparingPty — the row is wrong about the PTY it names. Cannot reach pendingRemoteShutdown by construction, because a reset relay reuses pty-N and the id may already be another pane's live shell. Retirement is terminal: re-retiring is refused, so a spared lease can never later acquire a reap flag. Both retirement fields are added to the type and to normalizeSshRemotePtyLease here rather than in part 3. That normalizer is a strict allowlist, so a field landing without it would work in every in-process test and vanish on the first restart — precisely the durable case the flag exists for. The round-trip test reloads through normalize instead of set-then-read. No callers yet; this commit only constrains what comes after. --- src/main/persistence.test.ts | 108 +++++++++++++++++++++++++++++++++++ src/main/persistence.ts | 53 ++++++++++++++++- src/shared/ssh-types.ts | 8 +++ 3 files changed, 168 insertions(+), 1 deletion(-) diff --git a/src/main/persistence.test.ts b/src/main/persistence.test.ts index 3ee4a49dfcb..df434466aaa 100644 --- a/src/main/persistence.test.ts +++ b/src/main/persistence.test.ts @@ -9386,6 +9386,114 @@ describe('Store', () => { } }) + it('retires and flags for reaping when the evidence is that the pane is gone', async () => { + const store = await createStore() + store.upsertSshRemotePtyLease({ + targetId: 'target-1', + ptyId: 'pty-1', + worktreeId: 'wt1', + tabId: 'tab-1', + leafId: TEST_LEAF_1, + state: 'attached' + }) + + store.retireLeaseAndReap('target-1', 'pty-1', 'superseded by a later write for the same pane') + + const lease = store.getSshRemotePtyLeases('target-1')[0] + expect(lease.state).toBe('terminated') + expect(lease.retiredReason).toBe('superseded by a later write for the same pane') + expect(lease.pendingRemoteShutdown).toBe(true) + }) + + it('retires without ever flagging a reap when the evidence is only that the row is wrong', async () => { + const store = await createStore() + store.upsertSshRemotePtyLease({ + targetId: 'target-1', + ptyId: 'pty-1', + worktreeId: 'wt1', + tabId: 'tab-1', + leafId: TEST_LEAF_1, + state: 'attached' + }) + + store.retireLeaseSparingPty('target-1', 'pty-1', 'relay generation replaced') + + const lease = store.getSshRemotePtyLeases('target-1')[0] + expect(lease.state).toBe('expired') + expect(lease.retiredReason).toBe('relay generation replaced') + // A reset relay reuses `pty-N`, so this id may already be another pane's live shell. + expect(lease.pendingRemoteShutdown).toBeUndefined() + }) + + it('refuses to re-retire, so a spared lease can never later acquire a reap flag', async () => { + const store = await createStore() + store.upsertSshRemotePtyLease({ + targetId: 'target-1', + ptyId: 'pty-1', + worktreeId: 'wt1', + tabId: 'tab-1', + leafId: TEST_LEAF_1, + state: 'attached' + }) + + store.retireLeaseSparingPty('target-1', 'pty-1', 'relay generation replaced') + store.retireLeaseAndReap('target-1', 'pty-1', 'pane closed') + + const lease = store.getSshRemotePtyLeases('target-1')[0] + expect(lease.state).toBe('expired') + expect(lease.retiredReason).toBe('relay generation replaced') + expect(lease.pendingRemoteShutdown).toBeUndefined() + }) + + it('round-trips retirement fields through normalize on restart', async () => { + const store = await createStore() + store.upsertSshRemotePtyLease({ + targetId: 'target-1', + ptyId: 'pty-1', + worktreeId: 'wt1', + tabId: 'tab-1', + leafId: TEST_LEAF_1, + state: 'attached' + }) + store.upsertSshRemotePtyLease({ + targetId: 'target-1', + ptyId: 'pty-2', + worktreeId: 'wt1', + tabId: 'tab-2', + leafId: TEST_LEAF_2, + state: 'attached' + }) + store.retireLeaseAndReap('target-1', 'pty-1', 'pane closed') + store.retireLeaseSparingPty('target-1', 'pty-2', 'relay pty missing') + store.flush() + + // Why: normalize is a strict allowlist, so set-then-read would pass even if the fields never persisted. + const reloaded = await createStore() + const reaped = reloaded.getSshRemotePtyLeases('target-1').find((l) => l.ptyId === 'pty-1') + const spared = reloaded.getSshRemotePtyLeases('target-1').find((l) => l.ptyId === 'pty-2') + expect(reaped?.state).toBe('terminated') + expect(reaped?.retiredReason).toBe('pane closed') + expect(reaped?.pendingRemoteShutdown).toBe(true) + expect(spared?.state).toBe('expired') + expect(spared?.retiredReason).toBe('relay pty missing') + expect(spared?.pendingRemoteShutdown).toBeUndefined() + }) + + it('ignores retirement of a lease that does not exist', async () => { + const store = await createStore() + store.upsertSshRemotePtyLease({ + targetId: 'target-1', + ptyId: 'pty-1', + state: 'attached' + }) + const before = structuredClone(store.getSshRemotePtyLeases()) + + store.retireLeaseAndReap('target-1', 'pty-absent', 'pane closed') + store.retireLeaseSparingPty('target-other', 'pty-1', 'relay generation replaced') + + expect(store.getSshRemotePtyLeases()).toEqual(before) + }) + it('keeps worktree deletion authoritative against stale writes and later same-path reuse', async () => { const store = await createStore() store.setWorktreeMeta('wt1', { displayName: 'Worktree' }) diff --git a/src/main/persistence.ts b/src/main/persistence.ts index 33e0170536f..0a9d08c8c23 100644 --- a/src/main/persistence.ts +++ b/src/main/persistence.ts @@ -1605,7 +1605,13 @@ function normalizeSshRemotePtyLease(value: unknown): SshRemotePtyLease | null { createdAt: typeof raw.createdAt === 'number' ? raw.createdAt : now, updatedAt: typeof raw.updatedAt === 'number' ? raw.updatedAt : now, ...(typeof raw.lastAttachedAt === 'number' ? { lastAttachedAt: raw.lastAttachedAt } : {}), - ...(typeof raw.lastDetachedAt === 'number' ? { lastDetachedAt: raw.lastDetachedAt } : {}) + ...(typeof raw.lastDetachedAt === 'number' ? { lastDetachedAt: raw.lastDetachedAt } : {}), + // Why: this is a strict allowlist — an unlisted field is dropped on load, so a retirement flag would + // work in every in-process test and vanish on the first restart, in exactly the durable case it exists for. + ...(typeof raw.retiredReason === 'string' && raw.retiredReason.length <= 512 + ? { retiredReason: raw.retiredReason } + : {}), + ...(raw.pendingRemoteShutdown === true ? { pendingRemoteShutdown: true } : {}) } } @@ -7182,6 +7188,51 @@ export class Store { this.flush() } + // Why: D0 — the only two ways to retire a lease. The split is by what the evidence proves, not by which + // handler produced it: I3 (the pane is provably gone) authorizes reaping the remote PTY; I6 (the row is + // wrong about the PTY it names) never does. + retireLeaseAndReap(targetId: string, ptyId: string, reason: string): void { + this.retireSshRemotePtyLease(targetId, ptyId, 'terminated', reason, true) + } + + // `pendingRemoteShutdown` is unreachable from here by construction, and that is the point: a reset relay + // reuses `pty-N`, so the id a wrong row names may already be another pane's live shell. + retireLeaseSparingPty(targetId: string, ptyId: string, reason: string): void { + this.retireSshRemotePtyLease(targetId, ptyId, 'expired', reason, false) + } + + private retireSshRemotePtyLease( + targetId: string, + ptyId: string, + state: Extract, + reason: string, + pendingRemoteShutdown: boolean + ): void { + const relayPtyId = this.getRelayPtyIdForSshLeaseStorage(targetId, ptyId) + const lease = this.state.sshRemotePtyLeases?.find( + (entry) => entry.targetId === targetId && entry.ptyId === relayPtyId + ) + if (!lease) { + return + } + // Why: retirement is terminal. Re-retiring would let a spared row later acquire a reap flag for a PTY + // we already decided we have no claim on; keep binding cleanup idempotent as the singular setter does. + if (lease.state === 'terminated' || lease.state === 'expired') { + if (this.clearSshRemotePtyBindingsForLeases(targetId, [lease])) { + this.flush() + } + return + } + lease.state = state + lease.updatedAt = Date.now() + lease.retiredReason = reason + if (pendingRemoteShutdown) { + lease.pendingRemoteShutdown = true + } + this.clearSshRemotePtyBindingsForLeases(targetId, [lease]) + this.flush() + } + removeSshRemotePtyLease(targetId: string, ptyId: string): void { const relayPtyId = this.getRelayPtyIdForSshLeaseStorage(targetId, ptyId) const leases = (this.state.sshRemotePtyLeases ?? []).filter( diff --git a/src/shared/ssh-types.ts b/src/shared/ssh-types.ts index fd6a9a3bcb0..888ec723c5d 100644 --- a/src/shared/ssh-types.ts +++ b/src/shared/ssh-types.ts @@ -203,6 +203,14 @@ export type SshRemotePtyLease = { updatedAt: number lastAttachedAt?: number lastDetachedAt?: number + /** + * Present iff Orca retired this row itself; a relay-driven expiry leaves it absent. That distinction + * is load-bearing, not triage colour: `'expired'` alone cannot authorize pane recovery, because we + * write it too. Free-form so a retirement can carry its detail (expected vs managed identity). + */ + retiredReason?: string + /** Set only by `retireLeaseAndReap` (I3): the pane is provably gone, so this relay PTY is ours to kill. */ + pendingRemoteShutdown?: boolean } /** Main-owned relay lease needed to reclaim PTY delivery after a desktop restart. */