From 890794967ffb75d69d254cd38ededd2933b71af2 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Thu, 27 Aug 2026 20:33:20 -0700 Subject: [PATCH] fix(ssh): never authorise uploads from a picture known to be incomplete Reversal of this branch's own exhaustion fallback, on evidence. Hydration authorises uploads (use-app-session-persistence.ts), and an upload is a workspace.patch of kind 'replace-session' (remote-workspace-relay-sync.ts:66) which wholesale replaces the host snapshot (relay/workspace-session-handler.ts). So marking a target hydrated on a picture we know is missing rows does not 'settle back to the old behaviour' - it uploads an empty projection that DELETES the host tabs we failed to adopt. Suppressed uploads are recoverable; a wiped host snapshot is not. That data loss is reachable on main today, because today the apply marks hydrated immediately. Exhaustion therefore reports 'error' and leaves the target un-hydrated, so terminal authority stays 'unverifiable' and no upload can be built from it. Also closes two chain-lifecycle gaps found in review: - the callback dropped its timer guard before awaiting the host, leaving a gap in which a concurrent report armed a second overlapping chain and could trip exhaustion before the pending apply resolved; an in-flight guard now spans it; - resetTarget could not cancel a callback already past its await, so a stale one rescheduled on top of the new connection's chain; chains are now generation stamped and a stale callback exits. --- .../src/hooks/remote-workspace-target-sync.ts | 26 +++---- ...ce-unplaced-snapshot-repull-wiring.test.ts | 13 +++- ...workspace-unplaced-snapshot-repull.test.ts | 77 ++++++++++++++++++- ...mote-workspace-unplaced-snapshot-repull.ts | 29 ++++++- 4 files changed, 123 insertions(+), 22 deletions(-) diff --git a/src/renderer/src/hooks/remote-workspace-target-sync.ts b/src/renderer/src/hooks/remote-workspace-target-sync.ts index 3d9ded5fc12..675e1b69914 100644 --- a/src/renderer/src/hooks/remote-workspace-target-sync.ts +++ b/src/renderer/src/hooks/remote-workspace-target-sync.ts @@ -128,21 +128,19 @@ export function createRemoteWorkspaceTargetSync( getSnapshot: (targetId) => deps.remoteWorkspace.get({ targetId }), applySnapshot: (targetId, snapshot) => applyPreparedSnapshot(targetId, snapshot), reportExhausted: (targetId) => { - // Why hydrate on exhaustion rather than stay un-hydrated: a path can be unplaceable for good - // (its worktree was deleted host-side), and an un-hydrated target is filtered out of - // `hydratedTargetIds` in use-app-session-persistence.ts, so it would never upload again — - // a permanent regression strictly worse than the tab loss this fix targets. Retries cover the - // transient degraded-lineage case; past them we settle back to the pre-fix behaviour. - const store = deps.store.getState() - const previous = store.remoteWorkspaceSyncStatusByTargetId[targetId] - store.markRemoteWorkspaceHydrated(targetId) - store.setRemoteWorkspaceSyncStatus(targetId, { - phase: 'synced', + // Why the target stays UN-hydrated: hydration authorises uploads + // (use-app-session-persistence.ts), and an upload is a `replace-session` patch + // (remote-workspace-relay-sync.ts) that wholesale replaces the host snapshot. Hydrating on a + // picture we know is missing rows would therefore delete the very tabs we failed to adopt — + // unrecoverable, where not uploading is merely deferred. Suppressed uploads are the safe + // side of this trade, so exhaustion reports the failure and leaves authority `unverifiable`. + deps.store.getState().setRemoteWorkspaceSyncStatus(targetId, { + phase: 'error', direction: 'pull', - ...(previous?.revision === undefined ? {} : { revision: previous.revision }), - ...(previous?.updatedAt === undefined ? {} : { updatedAt: previous.updatedAt }), - lastSyncedAt: Date.now(), - message: translate('auto.hooks.useIpcEvents.4f78ba5885', 'Workspace synced') + message: translate( + 'auto.hooks.useIpcEvents.2fe88c2e06', + 'Remote workspace sync unavailable' + ) }) } }) diff --git a/src/renderer/src/hooks/remote-workspace-unplaced-snapshot-repull-wiring.test.ts b/src/renderer/src/hooks/remote-workspace-unplaced-snapshot-repull-wiring.test.ts index 9556795143b..d64d1b8bc99 100644 --- a/src/renderer/src/hooks/remote-workspace-unplaced-snapshot-repull-wiring.test.ts +++ b/src/renderer/src/hooks/remote-workspace-unplaced-snapshot-repull-wiring.test.ts @@ -215,10 +215,15 @@ describe('target sync re-pulling a snapshot it could not place', () => { await vi.advanceTimersByTimeAsync(TOTAL_CHAIN_MS) expect(harness.get).toHaveBeenCalledTimes(UNPLACED_SNAPSHOT_REPULL_DELAYS_MS.length) - // Settling back to hydrated is deliberate: a host-side deletion makes a path unplaceable for - // good, and a permanently un-hydrated target would never upload its workspace again. - expect(isHydrated(harness.store)).toBe(true) - expect(syncPhase(harness.store)).toBe('synced') + // Staying un-hydrated is the data-safety property, not an oversight: hydration authorises + // uploads, and an upload is a `replace-session` patch that wholesale replaces the host + // snapshot. Hydrating on a picture known to be missing rows would delete the very tabs we + // failed to adopt. Suppressed uploads are recoverable; a wiped host snapshot is not. + expect( + isHydrated(harness.store), + 'exhaustion authorised uploads from a picture known to be incomplete' + ).toBe(false) + expect(syncPhase(harness.store)).toBe('error') await vi.advanceTimersByTimeAsync(TOTAL_CHAIN_MS * 10) expect(harness.get, 'the chain kept re-pulling past exhaustion').toHaveBeenCalledTimes( diff --git a/src/renderer/src/hooks/remote-workspace-unplaced-snapshot-repull.test.ts b/src/renderer/src/hooks/remote-workspace-unplaced-snapshot-repull.test.ts index 5213bd1448d..e77d5b89321 100644 --- a/src/renderer/src/hooks/remote-workspace-unplaced-snapshot-repull.test.ts +++ b/src/renderer/src/hooks/remote-workspace-unplaced-snapshot-repull.test.ts @@ -71,7 +71,8 @@ function createHarness(options: HarnessOptions = {}) { stop: () => { stopped = true }, - dropAuthority: (targetId: string) => targetsWithAuthority.delete(targetId) + dropAuthority: (targetId: string) => targetsWithAuthority.delete(targetId), + restoreAuthority: (targetId: string) => targetsWithAuthority.add(targetId) } } @@ -289,7 +290,7 @@ describe('the re-pull chain after an unplaced snapshot', () => { expect(harness.applySnapshot).not.toHaveBeenCalled() }) - it('abandons the chain when the target lost its authority before the retry fired', async () => { + it('abandons the chain when the target lost its authority, stranding no attempt count', async () => { const harness = createHarness() harness.repull.schedule(TARGET, 'unplaced') @@ -297,6 +298,78 @@ describe('the re-pull chain after an unplaced snapshot', () => { await vi.advanceTimersByTimeAsync(LONG_AFTER_MS) expect(harness.applySnapshot).not.toHaveBeenCalled() + expect(harness.exhaustedTargets, 'a target that went away is not an exhausted chain').toEqual( + [] + ) + expect(vi.getTimerCount()).toBe(0) + + // The spent attempt must not outlive the connection. If it did, the target would come back + // with a partly burned chain — observable here as a short first wait and a short chain. + harness.restoreAuthority(TARGET) + harness.repull.schedule(TARGET, 'unplaced') + await vi.advanceTimersByTimeAsync(UNPLACED_SNAPSHOT_REPULL_DELAYS_MS[0] - 1) + expect( + harness.applySnapshot, + 'the recovered target resumed mid-backoff instead of starting over' + ).not.toHaveBeenCalled() + + await vi.advanceTimersByTimeAsync(LONG_AFTER_MS) + expect(harness.applySnapshot).toHaveBeenCalledTimes(UNPLACED_SNAPSHOT_REPULL_DELAYS_MS.length) + expect(harness.exhaustedTargets).toEqual([TARGET]) + }) + + it('keeps the final armed attempt when a concurrent report lands after the count is spent', async () => { + const harness = createHarness() + harness.repull.schedule(TARGET, 'unplaced') + for (const delayMs of UNPLACED_SNAPSHOT_REPULL_DELAYS_MS.slice(0, -1)) { + await vi.advanceTimersByTimeAsync(delayMs) + } + // The last attempt is armed, so the count is already spent while the retry is still pending. + expect(harness.applySnapshot).toHaveBeenCalledTimes( + UNPLACED_SNAPSHOT_REPULL_DELAYS_MS.length - 1 + ) + expect(vi.getTimerCount()).toBe(1) + + // An unsolicited host push reports its own unplaced apply against that spent count. + harness.repull.schedule(TARGET, 'unplaced') + + expect( + harness.exhaustedTargets, + 'exhaustion was declared while the last retry was still pending' + ).toEqual([]) + expect(vi.getTimerCount(), 'the concurrent report cancelled the final retry').toBe(1) + + await vi.advanceTimersByTimeAsync(UNPLACED_SNAPSHOT_REPULL_DELAYS_MS.at(-1) ?? 0) + expect(harness.applySnapshot, 'the chain silently lost its last attempt').toHaveBeenCalledTimes( + UNPLACED_SNAPSHOT_REPULL_DELAYS_MS.length + ) + expect(harness.exhaustedTargets).toEqual([TARGET]) + expect(vi.getTimerCount()).toBe(0) + }) + + it('carries on after a single transient host read failure', async () => { + let call = 0 + const harness = createHarness({ + getSnapshot: async () => { + call += 1 + if (call === 2) { + throw new Error('ssh channel closed') + } + return snapshot() + } + }) + harness.repull.schedule(TARGET, 'unplaced') + + await vi.advanceTimersByTimeAsync(LONG_AFTER_MS) + + // The failed attempt still counts as unplaced, so the chain walks on and settles the target + // rather than stranding it on `pulling`, un-hydrated, with no timer armed. + expect(harness.getSnapshot).toHaveBeenCalledTimes(UNPLACED_SNAPSHOT_REPULL_DELAYS_MS.length) + expect( + harness.applySnapshot, + 'the chain stalled on the transient failure' + ).toHaveBeenCalledTimes(UNPLACED_SNAPSHOT_REPULL_DELAYS_MS.length - 1) + expect(harness.exhaustedTargets).toEqual([TARGET]) expect(vi.getTimerCount()).toBe(0) }) diff --git a/src/renderer/src/hooks/remote-workspace-unplaced-snapshot-repull.ts b/src/renderer/src/hooks/remote-workspace-unplaced-snapshot-repull.ts index e60ff537f3b..b296c31cba1 100644 --- a/src/renderer/src/hooks/remote-workspace-unplaced-snapshot-repull.ts +++ b/src/renderer/src/hooks/remote-workspace-unplaced-snapshot-repull.ts @@ -45,6 +45,14 @@ export function createUnplacedSnapshotRepull( // report still lands here, and re-announcing it would re-mark the target hydrated and rewrite // its status on every host push. Exhaustion is announced once per chain. const exhaustedTargets = new Set() + // Why in-flight is tracked apart from the timer: the callback drops its timer entry before + // awaiting the host, so the timer map alone leaves a gap in which a concurrent report arms a + // second, overlapping chain and can trip exhaustion before the pending apply resolves. + const inFlightTargets = new Set() + // Why a generation: `resetTarget` cannot cancel a callback already past its await, so a stale + // one would reschedule on top of the new connection's chain. It stamps its generation and exits + // if the target has since been reset. + const generationByTarget = new Map() const clearTimer = (targetId: string): void => { const timer = timerByTarget.get(targetId) @@ -66,7 +74,7 @@ export function createUnplacedSnapshotRepull( // Why this precedes the exhaustion check: once the last attempt is armed the count is already // spent, so testing exhaustion first would let a concurrent report cancel that still-pending // retry and declare the chain over one attempt early. - if (timerByTarget.has(targetId)) { + if (timerByTarget.has(targetId) || inFlightTargets.has(targetId)) { return } const attempt = attemptByTarget.get(targetId) ?? 0 @@ -83,6 +91,8 @@ export function createUnplacedSnapshotRepull( targetId, setTimeout(() => { timerByTarget.delete(targetId) + inFlightTargets.add(targetId) + const generation = generationByTarget.get(targetId) ?? 0 void (async () => { if (deps.isStopped()) { return @@ -108,8 +118,19 @@ export function createUnplacedSnapshotRepull( // un-hydrated target never uploads again. Staying `unplaced` walks the chain to // exhaustion, which settles it. } + if ((generationByTarget.get(targetId) ?? 0) !== generation) { + // A reconnect reset this target while we were awaiting; its own chain owns it now. + return + } + // Released before rescheduling: the guard exists to keep *others* out, and holding it + // here would block this chain's own next attempt. + inFlightTargets.delete(targetId) schedule(targetId, outcome) - })().catch(() => {}) + })() + .catch(() => {}) + .finally(() => { + inFlightTargets.delete(targetId) + }) }, delayMs) ) } @@ -120,6 +141,8 @@ export function createUnplacedSnapshotRepull( clearTimer(targetId) attemptByTarget.delete(targetId) exhaustedTargets.delete(targetId) + inFlightTargets.delete(targetId) + generationByTarget.set(targetId, (generationByTarget.get(targetId) ?? 0) + 1) }, stop: () => { for (const timer of timerByTarget.values()) { @@ -128,6 +151,8 @@ export function createUnplacedSnapshotRepull( timerByTarget.clear() attemptByTarget.clear() exhaustedTargets.clear() + inFlightTargets.clear() + generationByTarget.clear() } } }