From 2dbcb8c5f2864510fd30a63ea37a2e43fee89484 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Mon, 19 Oct 2026 12:28:19 -0700 Subject: [PATCH] fix(ssh): an overlapping managed-server update check sees the failure the first one recorded (#30835) * fix(ssh): an overlapping managed-server update check reads the failure the first one recorded recordedUpdateFailure read the caller's SshTarget, a copy taken before the check queued on the host lock, so the in-lock re-read added in #30253 never saw the other check's failure: both tried the update, and a skipped second check lost the failure note. Read the stored row instead. Co-Authored-By: Claude * fix(ssh): decide the failure clear on a read after the update, not before the lock Co-Authored-By: Claude --------- Co-authored-by: Claude --- src/main/ssh/managed-server-update-check.ts | 4 +- .../ssh/managed-server-update-deps.test.ts | 52 ++++++++++++++++++- src/main/ssh/managed-server-update-deps.ts | 9 ++-- 3 files changed, 58 insertions(+), 7 deletions(-) diff --git a/src/main/ssh/managed-server-update-check.ts b/src/main/ssh/managed-server-update-check.ts index a3088fba2ee..b4eb157c28f 100644 --- a/src/main/ssh/managed-server-update-check.ts +++ b/src/main/ssh/managed-server-update-check.ts @@ -37,7 +37,6 @@ export async function checkManagedServerUpdate( deps: ManagedServerUpdateDeps, onUpdating: () => void ): Promise { - const failure = deps.recordedUpdateFailure(target) let result: ManagedOrcadAutoUpdateOutcome try { result = await deps.autoUpdate(environmentId, { @@ -58,8 +57,9 @@ export async function checkManagedServerUpdate( } } // Why clear on current too: a Managed servers update may have landed the build since. + // Why read after: an overlapping check may have recorded a failure while this one waited. if ( - failure !== null && + deps.recordedUpdateFailure(target) !== null && (result.outcome === 'updated' || (result.outcome === 'skipped' && result.reason === 'current')) ) { deps.clearUpdateFailure(target) diff --git a/src/main/ssh/managed-server-update-deps.test.ts b/src/main/ssh/managed-server-update-deps.test.ts index f8b89d4da90..2ba173738b8 100644 --- a/src/main/ssh/managed-server-update-deps.test.ts +++ b/src/main/ssh/managed-server-update-deps.test.ts @@ -1,22 +1,72 @@ import { describe, expect, it, vi } from 'vitest' import type { SshTarget } from '../../shared/ssh-types' import { managedServerUpdateDeps } from './managed-server-update-deps' +import { checkManagedServerUpdate } from './managed-server-update-check' vi.mock('../../shared/app-environment', () => ({ getAppEnvironment: () => ({ getVersion: () => '1.4.300' }) })) + +const stored = new Map() +// Like the real store: reads hand out copies, writes land on the stored entry only. vi.mock('../managed-server/lifecycle/orcad-managed-runtime-context', () => ({ - requireManagedOrcadTargetStore: () => ({ updateTarget: vi.fn() }) + requireManagedOrcadTargetStore: () => ({ + getTarget: (id: string) => { + const entry = stored.get(id) + return entry ? { ...entry } : undefined + }, + updateTarget: (id: string, updates: Partial) => { + const entry = stored.get(id) + if (entry) { + stored.set(id, { ...entry, ...updates }) + } + return entry ?? null + } + }) +})) +vi.mock('../managed-server/lifecycle/orcad-managed-auto-update', () => ({ + autoUpdateManagedOrcadEnvironment: vi.fn() })) const target = (id: string): SshTarget => ({ id, label: id, host: id, port: 22, username: 'me' }) describe('remembering a managed-server update failure for this launch', () => { it('counts only an actual failure, and outlives the per-connect deps', () => { + stored.set('a', target('a')) const deps = managedServerUpdateDeps('/tmp/user-data') expect(deps.failedThisLaunch(target('a'))).toBe(false) deps.recordUpdateFailure(target('a'), 'candidate rejected') expect(managedServerUpdateDeps('/tmp/user-data').failedThisLaunch(target('a'))).toBe(true) expect(deps.failedThisLaunch(target('b'))).toBe(false) }) + + it('an overlapping check sees the failure the first one recorded on a copy it took earlier', async () => { + stored.set('c', target('c')) + // Both checks took their copy before either ran, as connect and tunnel restore do. + const first = { ...stored.get('c')! } + const second = { ...stored.get('c')! } + const deps = managedServerUpdateDeps('/tmp/user-data') + let tries = 0 + let lock = Promise.resolve() + const autoUpdate: typeof deps.autoUpdate = (_environmentId, options) => { + const run = lock.then(() => { + if (options.failedBefore()) { + return { outcome: 'skipped' as const, reason: 'failed-before' as const } + } + tries += 1 + return { outcome: 'failed' as const, reason: 'bad' } + }) + lock = run.then(() => undefined) + return run + } + const withLock = { ...deps, autoUpdate } + + const results = await Promise.all([ + checkManagedServerUpdate(first, 'env-c', withLock, () => {}), + checkManagedServerUpdate(second, 'env-c', withLock, () => {}) + ]) + + expect(tries).toBe(1) + expect(results[1].note).toEqual({ state: 'failed', detail: 'bad' }) + }) }) diff --git a/src/main/ssh/managed-server-update-deps.ts b/src/main/ssh/managed-server-update-deps.ts index d37c2cf60f4..31fd611cffb 100644 --- a/src/main/ssh/managed-server-update-deps.ts +++ b/src/main/ssh/managed-server-update-deps.ts @@ -13,10 +13,11 @@ export function managedServerUpdateDeps(userDataPath: string): ManagedServerUpda return { autoUpdate: (environmentId, options) => autoUpdateManagedOrcadEnvironment(userDataPath, { environmentId, appVersion, ...options }), - recordedUpdateFailure: (target) => - target.managedServerUpdateFailure?.appVersion === appVersion - ? target.managedServerUpdateFailure.reason - : null, + // Why the stored row: the caller's target is a copy taken before an overlapping check recorded. + recordedUpdateFailure: (target) => { + const failure = (registry.getTarget(target.id) ?? target).managedServerUpdateFailure + return failure?.appVersion === appVersion ? failure.reason : null + }, // Why only a failure counts: a deferral or skip never tried the update. failedThisLaunch: (target) => failedThisLaunch.has(target.id), recordUpdateFailure: (target, reason) => {