From 63b3f15cc42bf6c6fae5c3f80f6907417f501e27 Mon Sep 17 00:00:00 2001 From: OrcaWin Date: Fri, 9 Oct 2026 21:17:01 -0700 Subject: [PATCH] fix(repos): every repo catalog uses the host-catalog fence (#27116) * fix(repos): every repo catalog uses the host-catalog fence The all-host and active-target repo loaders fenced only by fetch generation, so a catalog fetched over a runtime connection that was replaced mid-fetch still overwrote the current one. All three repo loaders now claim the shared host-catalog fence (generation, SSH-state and runtime-connection generations), which replaces the repo-only per-host generation map and the Connect flow's inline checks. A dropped catalog is not an answer, so the all-host load skips repo-scoped UI validation. * fix(repos): a superseded all-host load claims no remote fences Its late claim would void the newer load's in-flight catalog, so neither would apply. --------- Co-authored-by: m4air --- .../src/store/host-catalog-fencing.ts | 2 +- .../repos/all-host-repo-catalog-actions.ts | 47 ++++--- .../src/store/repos/repo-catalog-actions.ts | 17 +-- .../src/store/repos/repo-catalog-fencing.ts | 28 ---- .../repos/runtime-repo-catalog-actions.ts | 45 +------ .../slices/repos-all-hosts-generation.test.ts | 10 +- .../repos-catalog-connection-fence.test.ts | 124 ++++++++++++++++++ ...repos-module-lifetime-coordinators.test.ts | 6 +- 8 files changed, 170 insertions(+), 109 deletions(-) create mode 100644 src/renderer/src/store/slices/repos-catalog-connection-fence.test.ts diff --git a/src/renderer/src/store/host-catalog-fencing.ts b/src/renderer/src/store/host-catalog-fencing.ts index b94b593cbc2..346b7292ce2 100644 --- a/src/renderer/src/store/host-catalog-fencing.ts +++ b/src/renderer/src/store/host-catalog-fencing.ts @@ -5,7 +5,7 @@ import { getEnvironmentSshStateGeneration } from './slices/runtime-environment-s import { getRuntimeEnvironmentConnectionGeneration } from './slices/runtime-status' import { getRuntimeTargetHostId } from './runtime-target-host' -export type HostCatalogKind = 'project-groups' | 'folder-workspaces' +export type HostCatalogKind = 'repos' | 'project-groups' | 'folder-workspaces' export type HostCatalogFence = { key: string diff --git a/src/renderer/src/store/repos/all-host-repo-catalog-actions.ts b/src/renderer/src/store/repos/all-host-repo-catalog-actions.ts index 34cb6c0a965..38ffb530439 100644 --- a/src/renderer/src/store/repos/all-host-repo-catalog-actions.ts +++ b/src/renderer/src/store/repos/all-host-repo-catalog-actions.ts @@ -7,18 +7,21 @@ import { readRuntimeWorktreeVisibilitySnapshot } from '../slices/worktree-visibi import { getRepoHostIdentity } from '../slices/repo-host-identity' import { getActiveRuntimeTarget } from '../../runtime/runtime-rpc-client' import { filterSetupScriptPromptDismissalsToValidRepos } from '@/lib/setup-script-prompt' -import { getRepoExecutionHostId, LOCAL_EXECUTION_HOST_ID } from '../../../../shared/execution-host' +import { getRepoExecutionHostId } from '../../../../shared/execution-host' import { isRemovedRuntimeHostId } from '../slices/stale-runtime-host-rows' import type { FetchedRepoCatalog } from './repo-catalog-merge' import type { LocalRepoCatalogFetchOutcome } from './repo-catalog-fencing' import type { RepoSlice } from './repo-state' import { arrayElementsUnchanged } from '../catalog-identity' import { - claimRepoCatalogGeneration, - isLatestRepoCatalogGeneration, latestAllHostRepoCatalogGenerationByStore, startLocalRepoCatalogFetch } from './repo-catalog-fencing' +import { + claimHostCatalogFence, + isHostCatalogFenceCurrent, + type HostCatalogFence +} from '../host-catalog-fencing' import { fetchRepoCatalogForTarget, filterSetupsForPrunedRepoRows, @@ -46,15 +49,15 @@ export function createAllHostRepoCatalogActions( return { reposFetchGeneration: generation } }) latestAllHostRepoCatalogGenerationByStore.set(get, generation) - claimRepoCatalogGeneration(get, LOCAL_EXECUTION_HOST_ID, generation) + const isCurrent = (fence: HostCatalogFence): boolean => + latestAllHostRepoCatalogGenerationByStore.get(get) === generation && + isHostCatalogFenceCurrent(get, fence) // Why: fetching only the active host hides every other host's repos ("my projects vanished"); load local + all runtime envs, each failing soft. - const applyCatalog = (catalog: FetchedRepoCatalog): void => { - // Why: a concurrent all-host refresh must not let the older catalog resurrect a migrated SSH owner. - if ( - latestAllHostRepoCatalogGenerationByStore.get(get) !== generation || - !isLatestRepoCatalogGeneration(get, catalog.hostId, generation) - ) { - return + // Returns false for a dropped catalog: a superseded fetch or replaced connection is not an answer. + const applyCatalog = (catalog: FetchedRepoCatalog, fence: HostCatalogFence): boolean => { + // Why: a concurrent refresh or a replaced connection must not let an older catalog resurrect a migrated SSH owner. + if (!isCurrent(fence)) { + return false } let hostRepos: Repo[] = [] set((s) => { @@ -100,6 +103,7 @@ export function createAllHostRepoCatalogActions( }) // Why: keep the safe-auto fork sync (as fetchRepos does) so cold-start, which now routes here, still updates safe-auto forks. scheduleSafeAutoForkSync(get, hostRepos) + return true } const validateRepoScopedUi = (): void => { set((s) => { @@ -121,8 +125,11 @@ export function createAllHostRepoCatalogActions( // Local first so local repos are present even if a remote fetch stalls. let failed = false let localCatalogOutcome: LocalRepoCatalogFetchOutcome = { status: 'fulfilled' } + const localFence = claimHostCatalogFence(get, 'repos', { kind: 'local' }) try { - applyCatalog(await fetchRepoCatalogForTarget({ kind: 'local' })) + if (!applyCatalog(await fetchRepoCatalogForTarget({ kind: 'local' }), localFence)) { + failed = true + } } catch (err) { failed = true localCatalogOutcome = { status: 'rejected', reason: err } @@ -139,6 +146,10 @@ export function createAllHostRepoCatalogActions( } const environments = await listRuntimeEnvironmentsForAllHostLoad() + // Why: a superseded load must not claim remote fences, or it would void the newer load's catalogs. + if (latestAllHostRepoCatalogGenerationByStore.get(get) !== generation) { + return + } // Why: unreachable remotes can spend the full connect timeout; merge each resolved host via the state updater so parallel loads don't clobber. await Promise.all( environments.map(async (environment) => { @@ -146,7 +157,7 @@ export function createAllHostRepoCatalogActions( kind: 'environment' as const, environmentId: environment.id } - claimRepoCatalogGeneration(get, getRuntimeTargetHostId(target), generation) + const fence = claimHostCatalogFence(get, 'repos', target) const [catalogResult, visibilitySnapshot] = await Promise.all([ fetchRepoCatalogForTarget(target).then( (catalog) => ({ ok: true as const, catalog }), @@ -156,11 +167,7 @@ export function createAllHostRepoCatalogActions( ]) const visibilityDefaults = visibilitySnapshot.defaults const hostId = getRuntimeTargetHostId(target) - if ( - visibilityDefaults !== undefined && - latestAllHostRepoCatalogGenerationByStore.get(get) === generation && - isLatestRepoCatalogGeneration(get, hostId, generation) - ) { + if (visibilityDefaults !== undefined && isCurrent(fence)) { set((state) => isRemovedRuntimeHostId(hostId, state.removedRuntimeEnvironmentIds) ? state @@ -180,7 +187,9 @@ export function createAllHostRepoCatalogActions( ) } if (catalogResult.ok) { - applyCatalog(catalogResult.catalog) + if (!applyCatalog(catalogResult.catalog, fence)) { + failed = true + } } else { failed = true console.warn( diff --git a/src/renderer/src/store/repos/repo-catalog-actions.ts b/src/renderer/src/store/repos/repo-catalog-actions.ts index d254923fde9..e9ff0e484f7 100644 --- a/src/renderer/src/store/repos/repo-catalog-actions.ts +++ b/src/renderer/src/store/repos/repo-catalog-actions.ts @@ -18,10 +18,9 @@ import type { RepoSlice } from './repo-state' import { arrayElementsUnchanged } from '../catalog-identity' import { awaitLatestLocalRepoCatalogFetch, - claimRepoCatalogGeneration, - isLatestRepoCatalogGeneration, startLocalRepoCatalogFetch } from './repo-catalog-fencing' +import { claimHostCatalogFence, isHostCatalogFenceCurrent } from '../host-catalog-fencing' import { fetchRepoCatalogForTarget, filterSetupsForPrunedRepoRows, @@ -35,7 +34,6 @@ import { mergeProjectHostSetupCompatibility, projectCompatibilityFromRepos } from '../projects/project-compatibility-core' -import { getRuntimeTargetHostId } from '../runtime-target-host' import { mergeFetchedProjectCompatibilityForHost } from '../projects/project-compatibility-host-merge' import { scheduleSafeAutoForkSync } from './safe-auto-fork-sync' @@ -97,17 +95,12 @@ export function createRepoCatalogActions( target.kind === 'local' ? startLocalRepoCatalogFetch(get) : () => undefined let localCatalogOutcome: LocalRepoCatalogFetchOutcome = { status: 'fulfilled' } // Why: overlapping repos:changed fetches can resolve out of order; a stale one must not overwrite a newer result and resurrect deleted projects (#7020). - let generation = 0 - set((s) => { - generation = s.reposFetchGeneration + 1 - return { reposFetchGeneration: generation } - }) - const targetHostId = getRuntimeTargetHostId(target) - claimRepoCatalogGeneration(get, targetHostId, generation) + set((s) => ({ reposFetchGeneration: s.reposFetchGeneration + 1 })) + const fence = claimHostCatalogFence(get, 'repos', target) try { const catalog = await fetchRepoCatalogForTarget(target) - // A newer same-host fetch superseded us while we awaited — drop this stale result. - if (!isLatestRepoCatalogGeneration(get, targetHostId, generation)) { + // A newer same-host fetch or a replaced connection superseded us — drop this stale result. + if (!isHostCatalogFenceCurrent(get, fence)) { return } let finalizedHostRepos: Repo[] = [] diff --git a/src/renderer/src/store/repos/repo-catalog-fencing.ts b/src/renderer/src/store/repos/repo-catalog-fencing.ts index 38693085fce..42eeea1258f 100644 --- a/src/renderer/src/store/repos/repo-catalog-fencing.ts +++ b/src/renderer/src/store/repos/repo-catalog-fencing.ts @@ -8,11 +8,6 @@ export const latestLocalRepoCatalogFetchByStore = new WeakMap< Promise >() -export const latestRepoCatalogGenerationByHostByStore = new WeakMap< - () => AppState, - Map ->() - export const latestAllHostRepoCatalogGenerationByStore = new WeakMap<() => AppState, number>() export function startLocalRepoCatalogFetch( @@ -41,26 +36,3 @@ export async function awaitLatestLocalRepoCatalogFetch(get: () => AppState): Pro } } } - -export function claimRepoCatalogGeneration( - get: () => AppState, - hostId: string, - generation: number -): void { - let generations = latestRepoCatalogGenerationByHostByStore.get(get) - if (!generations) { - generations = new Map() - latestRepoCatalogGenerationByHostByStore.set(get, generations) - } - if ((generations.get(hostId) ?? 0) < generation) { - generations.set(hostId, generation) - } -} - -export function isLatestRepoCatalogGeneration( - get: () => AppState, - hostId: string, - generation: number -): boolean { - return latestRepoCatalogGenerationByHostByStore.get(get)?.get(hostId) === generation -} diff --git a/src/renderer/src/store/repos/runtime-repo-catalog-actions.ts b/src/renderer/src/store/repos/runtime-repo-catalog-actions.ts index 3c64a0c9a3a..efc4da84d8c 100644 --- a/src/renderer/src/store/repos/runtime-repo-catalog-actions.ts +++ b/src/renderer/src/store/repos/runtime-repo-catalog-actions.ts @@ -10,10 +10,8 @@ import { getActiveRuntimeTarget } from '../../runtime/runtime-rpc-client' import { filterSetupScriptPromptDismissalsToValidRepos } from '@/lib/setup-script-prompt' import { getRepoExecutionHostId } from '../../../../shared/execution-host' import { isRemovedRuntimeHostId } from '../slices/stale-runtime-host-rows' -import { getEnvironmentSshStateGeneration } from '../slices/runtime-environment-ssh' -import { getRuntimeEnvironmentConnectionGeneration } from '../slices/runtime-status' +import { claimHostCatalogFence, isHostCatalogFenceCurrent } from '../host-catalog-fencing' import type { RepoSlice } from './repo-state' -import { claimRepoCatalogGeneration, isLatestRepoCatalogGeneration } from './repo-catalog-fencing' import { fetchRepoCatalogForTarget, filterSetupsForPrunedRepoRows, @@ -26,61 +24,28 @@ import { getRuntimeTargetHostId } from '../runtime-target-host' import { mergeFetchedProjectCompatibilityForHost } from '../projects/project-compatibility-host-merge' import { scheduleSafeAutoForkSync } from './safe-auto-fork-sync' -export const runtimeRepoFetchGenerationByEnvironment = new Map() -const MAX_RUNTIME_REPO_FETCH_GENERATIONS = 512 -let runtimeRepoFetchGenerationSequence = 0 - -function pruneRuntimeRepoFetchGenerations(): void { - while (runtimeRepoFetchGenerationByEnvironment.size > MAX_RUNTIME_REPO_FETCH_GENERATIONS) { - const oldest = runtimeRepoFetchGenerationByEnvironment.keys().next() - if (oldest.done) { - return - } - runtimeRepoFetchGenerationByEnvironment.delete(oldest.value) - } -} - export function createRuntimeRepoCatalogActions( set: Parameters>[0], get: Parameters>[1] ): Pick { return { fetchRuntimeEnvironmentRepos: async (environmentId) => { - const requestGeneration = ++runtimeRepoFetchGenerationSequence - runtimeRepoFetchGenerationByEnvironment.set(environmentId, requestGeneration) - pruneRuntimeRepoFetchGenerations() - const connectionGeneration = getEnvironmentSshStateGeneration(environmentId) - const runtimeConnectionGeneration = getRuntimeEnvironmentConnectionGeneration(environmentId) - let catalogGeneration = 0 - set((s) => { - catalogGeneration = s.reposFetchGeneration + 1 - return { reposFetchGeneration: catalogGeneration } - }) + set((s) => ({ reposFetchGeneration: s.reposFetchGeneration + 1 })) const target = { kind: 'environment' as const, environmentId } const targetHostId = getRuntimeTargetHostId(target) - claimRepoCatalogGeneration(get, targetHostId, catalogGeneration) + const fence = claimHostCatalogFence(get, 'repos', target) try { const [catalog, visibilitySnapshot] = await Promise.all([ fetchRepoCatalogForTarget(target), readRuntimeWorktreeVisibilitySnapshot(environmentId) ]) const visibilityDefaults = visibilitySnapshot.defaults - if ( - runtimeRepoFetchGenerationByEnvironment.get(environmentId) !== requestGeneration || - !isLatestRepoCatalogGeneration(get, targetHostId, catalogGeneration) || - getEnvironmentSshStateGeneration(environmentId) !== connectionGeneration || - getRuntimeEnvironmentConnectionGeneration(environmentId) !== runtimeConnectionGeneration - ) { + if (!isHostCatalogFenceCurrent(get, fence)) { return [] } let finalizedHostRepos: Repo[] = [] set((s) => { - if ( - runtimeRepoFetchGenerationByEnvironment.get(environmentId) !== requestGeneration || - !isLatestRepoCatalogGeneration(get, targetHostId, catalogGeneration) || - getEnvironmentSshStateGeneration(environmentId) !== connectionGeneration || - getRuntimeEnvironmentConnectionGeneration(environmentId) !== runtimeConnectionGeneration - ) { + if (!isHostCatalogFenceCurrent(get, fence)) { return s } // Why: skip merging a runtime env removed while this Connect-flow fetch was in flight, so purged repos aren't re-added (#8881). diff --git a/src/renderer/src/store/slices/repos-all-hosts-generation.test.ts b/src/renderer/src/store/slices/repos-all-hosts-generation.test.ts index 187ede27fe4..2d6e9dfb1b0 100644 --- a/src/renderer/src/store/slices/repos-all-hosts-generation.test.ts +++ b/src/renderer/src/store/slices/repos-all-hosts-generation.test.ts @@ -339,7 +339,7 @@ describe('fetchReposForAllHosts generation', () => { expect(store.getState().folderWorkspacePathStatuses).toEqual({}) }) - it('applies a targeted repo catalog that started before a reconnect', async () => { + it('drops a targeted repo catalog that started before a reconnect', async () => { const { promise: repoList, resolve: resolveRepoList } = Promise.withResolvers() const { promise: repoListStarted, resolve: markRepoListStarted } = Promise.withResolvers() runtimeEnvironmentCall.mockImplementation((args: RuntimeEnvironmentCallRequest) => { @@ -375,11 +375,10 @@ describe('fetchReposForAllHosts generation', () => { }) await pending - // Current contract: targeted fetchRepos has only catalog-generation fencing. - expect(store.getState().repos[0]?.path).toBe(staleRemoteRepo.path) + expect(store.getState().repos[0]?.path).toBe(freshRemoteRepo.path) }) - it('applies an all-host repo result that started before a reconnect', async () => { + it('drops an all-host repo result that started before a reconnect (#20811)', async () => { const { promise: repoList, resolve: resolveRepoList } = Promise.withResolvers() const { promise: repoListStarted, resolve: markRepoListStarted } = Promise.withResolvers() runtimeEnvironmentCall.mockImplementation((args: RuntimeEnvironmentCallRequest) => { @@ -415,10 +414,9 @@ describe('fetchReposForAllHosts generation', () => { }) await pending - // Current contract: all-host remote legs do not carry the reconnect fence. expect( store.getState().repos.find((repo) => repo.executionHostId === 'runtime:env-1')?.path - ).toBe(staleRemoteRepo.path) + ).toBe(freshRemoteRepo.path) }) it('drops a Connect-flow catalog and visibility defaults from before a reconnect', async () => { diff --git a/src/renderer/src/store/slices/repos-catalog-connection-fence.test.ts b/src/renderer/src/store/slices/repos-catalog-connection-fence.test.ts new file mode 100644 index 00000000000..6e80563eb7d --- /dev/null +++ b/src/renderer/src/store/slices/repos-catalog-connection-fence.test.ts @@ -0,0 +1,124 @@ +/** + * #20811: a repo catalog fetched over a runtime connection that was replaced mid-fetch must not + * overwrite the current one, and a dropped catalog is not an answer for repo-scoped UI cleanup. + */ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import type { Repo } from '../../../../shared/repo-types' +import { + createCompatibleRuntimeStatusResponseIfNeeded, + type RuntimeEnvironmentCallRequest +} from '../../runtime/runtime-compatibility-test-fixture' +import { clearRuntimeCompatibilityCacheForTests } from '../../runtime/runtime-rpc-client' +import { createTestStore } from './store-test-helpers' + +const sshState = vi.hoisted(() => ({ generation: 0 })) + +vi.mock('./runtime-environment-ssh', async (importOriginal) => ({ + ...(await importOriginal()), + getEnvironmentSshStateGeneration: () => sshState.generation +})) + +function repo(id: string, path: string): Repo { + return { id, path, displayName: id, badgeColor: '#000', addedAt: 1 } +} + +const runtimeEnvironmentCall = vi.fn() + +function stubRemoteRepoList(remote: Promise, onStart: () => void): void { + runtimeEnvironmentCall.mockImplementation((args: RuntimeEnvironmentCallRequest) => { + if (args.method === 'repo.list') { + onStart() + return remote + } + return { + id: 'rpc-other', + ok: true, + result: { projects: [], setups: [] }, + _meta: { runtimeId: 'runtime-remote' } + } + }) +} + +beforeEach(() => { + sshState.generation = 0 + clearRuntimeCompatibilityCacheForTests() + runtimeEnvironmentCall.mockReset() + vi.stubGlobal('window', { + api: { + repos: { list: vi.fn().mockResolvedValue([repo('local-repo', '/local')]) }, + projects: { + list: vi.fn().mockResolvedValue([]), + listHostSetups: vi.fn().mockResolvedValue([]) + }, + runtimeEnvironments: { + list: vi.fn().mockResolvedValue([{ id: 'env-1', name: 'Remote' }]), + call: (args: RuntimeEnvironmentCallRequest) => + createCompatibleRuntimeStatusResponseIfNeeded(args) ?? runtimeEnvironmentCall(args) + } + }, + dispatchEvent: vi.fn() + }) +}) + +describe('repo catalogs use the host-catalog fence (#20811)', () => { + for (const loader of ['all-hosts', 'connect-flow', 'active-target'] as const) { + it(`drops a ${loader} catalog from a replaced connection`, async () => { + const remote = Promise.withResolvers() + const started = Promise.withResolvers() + stubRemoteRepoList(remote.promise, started.resolve) + const store = createTestStore() + store.setState({ filterRepoIds: ['gone-repo'] }) + + const load = + loader === 'all-hosts' + ? store.getState().fetchReposForAllHosts() + : loader === 'connect-flow' + ? store.getState().fetchRuntimeEnvironmentRepos('env-1') + : store.getState().fetchRepos({ runtimeEnvironmentId: 'env-1' }) + await started.promise + sshState.generation += 1 + remote.resolve({ + id: 'rpc-repo-list', + ok: true, + result: { repos: [repo('remote-repo', '/remote')] }, + _meta: { runtimeId: 'runtime-remote' } + }) + await load + + expect(store.getState().repos.map((entry) => entry.id)).not.toContain('remote-repo') + if (loader === 'all-hosts') { + // The dropped catalog is not an answer, so saved repo filters survive. + expect(store.getState().filterRepoIds).toEqual(['gone-repo']) + } + }) + } + + it('keeps the newer all-host load when a superseded one finishes listing hosts late', async () => { + const olderHosts = Promise.withResolvers<{ id: string; name: string }[]>() + const listHosts = vi + .fn() + .mockReturnValueOnce(olderHosts.promise) + .mockResolvedValue([{ id: 'env-1', name: 'Remote' }]) + vi.mocked(window.api.runtimeEnvironments).list = listHosts + const newerRemote = Promise.withResolvers() + const newerStarted = Promise.withResolvers() + stubRemoteRepoList(newerRemote.promise, newerStarted.resolve) + const store = createTestStore() + + const older = store.getState().fetchReposForAllHosts() + await vi.waitFor(() => expect(listHosts).toHaveBeenCalledOnce()) + const newer = store.getState().fetchReposForAllHosts() + await newerStarted.promise + olderHosts.resolve([{ id: 'env-1', name: 'Remote' }]) + await older + newerRemote.resolve({ + id: 'rpc-repo-list', + ok: true, + result: { repos: [repo('remote-repo', '/remote')] }, + _meta: { runtimeId: 'runtime-remote' } + }) + await newer + + expect(store.getState().repos.map((entry) => entry.id)).toContain('remote-repo') + }) +}) diff --git a/src/renderer/src/store/slices/repos-module-lifetime-coordinators.test.ts b/src/renderer/src/store/slices/repos-module-lifetime-coordinators.test.ts index e8d0c94e585..2f75e8c0744 100644 --- a/src/renderer/src/store/slices/repos-module-lifetime-coordinators.test.ts +++ b/src/renderer/src/store/slices/repos-module-lifetime-coordinators.test.ts @@ -34,7 +34,8 @@ beforeEach(() => { }) describe('repo module-lifetime coordinators', () => { - it('lets a newer runtime fetch in another store supersede the older store request', async () => { + // Why same store: catalog fences are per store; another store's fetch is not this store's answer. + it('lets a newer runtime fetch supersede the older request', async () => { const { promise: olderList, resolve: resolveOlderList } = Promise.withResolvers() let repoListCalls = 0 runtimeCall.mockImplementation((args: RuntimeEnvironmentCallRequest) => { @@ -57,10 +58,9 @@ describe('repo module-lifetime coordinators', () => { } }) const firstStore = createTestStore() - const secondStore = createTestStore() const older = firstStore.getState().fetchRuntimeEnvironmentRepos('shared-env') - await secondStore.getState().fetchRuntimeEnvironmentRepos('shared-env') + await firstStore.getState().fetchRuntimeEnvironmentRepos('shared-env') resolveOlderList({ id: 'rpc-older', ok: true,