mirror of
https://github.com/stablyai/orca.git
synced 2026-10-24 00:04:01 +00:00
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.
This commit is contained in:
@@ -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'
|
||||
)
|
||||
})
|
||||
}
|
||||
})
|
||||
|
||||
@@ -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(
|
||||
|
||||
@@ -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)
|
||||
})
|
||||
|
||||
|
||||
@@ -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<string>()
|
||||
// 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<string>()
|
||||
// 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<string, number>()
|
||||
|
||||
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()
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user