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 <noreply@anthropic.com>

* fix(ssh): decide the failure clear on a read after the update, not before the lock

Co-Authored-By: Claude <noreply@anthropic.com>

---------

Co-authored-by: Claude <noreply@anthropic.com>

src/main/ssh/managed-server-update-check.ts

@@ -37,7 +37,6 @@ export async function checkManagedServerUpdate(
   deps: ManagedServerUpdateDeps,
   onUpdating: () => void
 ): Promise<HostServerUpdateOnConnect> {
-  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)

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<string, SshTarget>()
+// 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<SshTarget>) => {
+      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' })
+  })
 })

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) => {