From 5d90bfc77261faccc5e12f38de8e47f07dab3b83 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Mon, 6 Jul 2026 23:31:47 -0700 Subject: [PATCH] fix repo enrichment cache retention --- src/main/git/git-username.ts | 7 +- src/main/index.ts | 3 + src/main/ipc/repos.ts | 3 + src/main/persistence.test.ts | 44 +++ src/main/persistence.ts | 49 ++- .../repo-git-remote-identity-coalescing.ts | 49 +++ ...ote-identity-enrichment-coalescing.test.ts | 190 +++++++++ ...epo-git-remote-identity-enrichment.test.ts | 374 +++++++++++++++++- .../repo-git-remote-identity-enrichment.ts | 318 ++++++++++++--- src/main/repo-git-remote-identity.test.ts | 54 +++ src/main/repo-git-remote-identity.ts | 11 +- ...git-username-enrichment-subprocess.test.ts | 110 ++++++ src/main/repo-git-username-enrichment.test.ts | 313 ++++++++++++++- src/main/repo-git-username-enrichment.ts | 251 +++++++++--- ...-runtime-repo-enrichment-authority.test.ts | 48 +++ src/main/runtime/orca-runtime.ts | 9 + 16 files changed, 1702 insertions(+), 131 deletions(-) create mode 100644 src/main/repo-git-remote-identity-coalescing.ts create mode 100644 src/main/repo-git-remote-identity-enrichment-coalescing.test.ts create mode 100644 src/main/repo-git-remote-identity.test.ts create mode 100644 src/main/repo-git-username-enrichment-subprocess.test.ts create mode 100644 src/main/runtime/orca-runtime-repo-enrichment-authority.test.ts diff --git a/src/main/git/git-username.ts b/src/main/git/git-username.ts index f820c6bd9d5..4047552bfae 100644 --- a/src/main/git/git-username.ts +++ b/src/main/git/git-username.ts @@ -17,7 +17,7 @@ const GH_LOGIN_PROBE_TIMEOUT_MS = 2500 const GH_LOGIN_PROBE_WALL_MS = 10_000 // Why: a timed-out probe says nothing about the account, so don't pin '' for // the whole session — retry after a cooldown instead of hammering a stuck gh. -const GH_LOGIN_TIMEOUT_RETRY_MS = 5 * 60 * 1000 +export const LOCAL_GIT_USERNAME_TIMEOUT_RETRY_MS = 5 * 60 * 1000 const LOCAL_GIT_READ_TIMEOUT_MS = 5000 export function normalizeGitUsername(value: string): string { @@ -151,7 +151,10 @@ async function getGhLoginOutcome(): Promise { if (cachedGhLogin !== null) { return { login: cachedGhLogin, timedOut: false } } - if (ghLoginTimedOutAt !== null && Date.now() - ghLoginTimedOutAt < GH_LOGIN_TIMEOUT_RETRY_MS) { + if ( + ghLoginTimedOutAt !== null && + Date.now() - ghLoginTimedOutAt < LOCAL_GIT_USERNAME_TIMEOUT_RETRY_MS + ) { return { login: '', timedOut: true } } if (ghLoginProbeInFlight) { diff --git a/src/main/index.ts b/src/main/index.ts index 27e9aee2aa4..ad9bc025c3b 100644 --- a/src/main/index.ts +++ b/src/main/index.ts @@ -1762,6 +1762,9 @@ app.whenReady().then(async () => { // hosts; the window-only registerCoreHandlers path never runs under serve. getAdditionalAiVaultCodexHomePaths: () => codexRuntimeHome ? [codexRuntimeHome.getHostRuntimeHomePath()] : [], + // Why: the desktop Store can contain paths owned by other runtimes; only + // headless serve may execute local git against runtime-stamped paths. + ownsPersistedRuntimeHostPaths: isServeMode, buildAgentHookPtyEnv: () => isAgentStatusHooksEnabled(store?.getSettings()) ? agentHookServer.buildPtyEnv() : {} }) diff --git a/src/main/ipc/repos.ts b/src/main/ipc/repos.ts index d1b1821081a..770c6cf1ac7 100644 --- a/src/main/ipc/repos.ts +++ b/src/main/ipc/repos.ts @@ -1164,6 +1164,7 @@ export function registerRepoHandlers(mainWindow: BrowserWindow, store: Store): v ipcMain.handle('repos:list', () => { enrichMissingRepoGitRemoteIdentities(store, { + notificationChannel: 'desktop', onChanged: () => notifyReposChanged(mainWindow) }) // Why: username resolution spawns git/gh and must stay off this handler's @@ -1177,6 +1178,7 @@ export function registerRepoHandlers(mainWindow: BrowserWindow, store: Store): v ipcMain.handle('projects:list', () => { enrichMissingRepoGitRemoteIdentities(store, { + notificationChannel: 'desktop', onChanged: () => notifyReposChanged(mainWindow) }) return store.getProjects() @@ -1193,6 +1195,7 @@ export function registerRepoHandlers(mainWindow: BrowserWindow, store: Store): v ipcMain.handle('projectHostSetups:list', () => { enrichMissingRepoGitRemoteIdentities(store, { + notificationChannel: 'desktop', onChanged: () => notifyReposChanged(mainWindow) }) return store.getProjectHostSetups() diff --git a/src/main/persistence.test.ts b/src/main/persistence.test.ts index 4e921151e4f..95982756eab 100644 --- a/src/main/persistence.test.ts +++ b/src/main/persistence.test.ts @@ -3048,6 +3048,50 @@ describe('Store', () => { expect(persisted.repos[0].gitUsername).toBe('testuser') }) + it('targets background enrichment mutations to one duplicate-ID host lifecycle', async () => { + const store = await createStore() + const sshRepo = makeRepo({ + id: 'shared', + path: '/same/path', + connectionId: 'ssh-target', + executionHostId: 'ssh:ssh-target' + }) + const localRepo = makeRepo({ id: 'shared', path: '/same/path', addedAt: 2 }) + store.addRepo(sshRepo) + store.addRepo(localRepo) + + expect(store.setResolvedRepoGitUsername('shared', 'local-user', localRepo)).toBe(true) + const identity = { + canonicalKey: 'git.company.test/team/local', + remoteName: 'origin', + remoteUrl: 'git@git.company.test:team/local.git' + } + expect(store.updateRepo('shared', { gitRemoteIdentity: identity }, localRepo)?.addedAt).toBe(2) + + const [hydratedSsh, hydratedLocal] = store.getRepos() + expect(hydratedSsh?.gitUsername).toBe('') + expect(hydratedSsh?.gitRemoteIdentity).toBeUndefined() + expect(hydratedLocal?.gitUsername).toBe('local-user') + expect(hydratedLocal?.gitRemoteIdentity).toEqual(identity) + + const localSnapshot = { ...localRepo } + expect( + store.updateRepo('shared', { executionHostId: 'runtime:environment-1' }, localRepo) + ?.executionHostId + ).toBe('runtime:environment-1') + expect(store.setResolvedRepoGitUsername('shared', 'stale-user', localSnapshot)).toBe(false) + + store.removeProjectForHost('shared', 'runtime:environment-1') + const replacement = makeRepo({ id: 'shared', path: '/same/path', addedAt: 3 }) + store.addRepo(replacement) + expect(store.setResolvedRepoGitUsername('shared', 'still-stale', localSnapshot)).toBe(false) + expect(store.setResolvedRepoGitUsername('shared', 'replacement-user', replacement)).toBe(true) + + const [remainingSsh, replacementLocal] = store.getRepos() + expect(remainingSsh?.gitUsername).toBe('') + expect(replacementLocal?.gitUsername).toBe('replacement-user') + }) + it('deleteProjectGroup ungroups repos from the deleted group subtree', async () => { const store = await createStore() const root = store.createProjectGroup({ name: 'Platform', createdFrom: 'folder-scan' }) diff --git a/src/main/persistence.ts b/src/main/persistence.ts index 0cac89b3f17..41a63262588 100644 --- a/src/main/persistence.ts +++ b/src/main/persistence.ts @@ -2596,6 +2596,21 @@ export type StoreOptions = { dataFile?: string } +type RepoEnrichmentLifecycle = Pick< + Repo, + 'id' | 'path' | 'connectionId' | 'executionHostId' | 'addedAt' +> + +function repoMatchesEnrichmentLifecycle(repo: Repo, expected: RepoEnrichmentLifecycle): boolean { + return ( + repo.id === expected.id && + repo.addedAt === expected.addedAt && + repo.path === expected.path && + (repo.connectionId ?? null) === (expected.connectionId ?? null) && + (repo.executionHostId ?? null) === (expected.executionHostId ?? null) + ) +} + export class Store { private state: PersistedState private readonly dataFile: string @@ -2615,7 +2630,6 @@ export class Store { private lastWrittenStateHash: string | null = null private firstPendingSaveAt: number | null = null private githubCacheDirty = false - private gitUsernameCache = new Map() private loadNeedsSave = false private settingsChangeListeners = new Set< ( @@ -3900,13 +3914,20 @@ export class Store { * Kept out of updateRepo's whitelist so the renderer-facing update surface * cannot write it directly. Returns true when the hydrated value changed. */ - setResolvedRepoGitUsername(id: string, username: string): boolean { - const repo = this.state.repos.find((r) => r.id === id) + setResolvedRepoGitUsername( + id: string, + username: string, + expectedLifecycle?: RepoEnrichmentLifecycle + ): boolean { + const repo = this.state.repos.find( + (candidate) => + candidate.id === id && + (!expectedLifecycle || repoMatchesEnrichmentLifecycle(candidate, expectedLifecycle)) + ) if (!repo) { return false } - const previous = this.gitUsernameCache.get(repo.path) ?? repo.gitUsername ?? '' - this.gitUsernameCache.set(repo.path, username) + const previous = repo.gitUsername ?? '' if (previous === username) { return false } @@ -4327,9 +4348,14 @@ export class Store { > & { sourceControlAi?: Repo['sourceControlAi'] | null externalWorktreeDiscoverySuppressedAt?: Repo['externalWorktreeDiscoverySuppressedAt'] | null - } + }, + expectedLifecycle?: RepoEnrichmentLifecycle ): Repo | null { - const repo = this.state.repos.find((r) => r.id === id) + const repo = this.state.repos.find( + (candidate) => + candidate.id === id && + (!expectedLifecycle || repoMatchesEnrichmentLifecycle(candidate, expectedLifecycle)) + ) if (!repo) { return null } @@ -4512,12 +4538,9 @@ export class Store { // Why: username resolution spawns git/gh subprocesses, so it must never // run inside hydration — the first getRepos() of a launch executes on the // Electron main thread and a stuck probe froze startup for minutes on - // Windows (issue #7225). Hydration only reads the enrichment cache or the - // value persisted by a previous launch; repo-git-username-enrichment.ts - // refreshes both in the background. - const gitUsername = isFolderRepo(repo) - ? '' - : (this.gitUsernameCache.get(repo.path) ?? repo.gitUsername ?? '') + // Windows (issue #7225). Hydration reads the in-memory persisted value; + // repo-git-username-enrichment.ts refreshes that record in the background. + const gitUsername = isFolderRepo(repo) ? '' : (repo.gitUsername ?? '') return { ...repoWithoutIcon, diff --git a/src/main/repo-git-remote-identity-coalescing.ts b/src/main/repo-git-remote-identity-coalescing.ts new file mode 100644 index 00000000000..bd55ac9626f --- /dev/null +++ b/src/main/repo-git-remote-identity-coalescing.ts @@ -0,0 +1,49 @@ +export type RepoIdentityNotificationChannel = 'desktop' | 'runtime' +export type RepoIdentityEnrichmentRequest = { probeRuntimeHostPaths: boolean } +export type RepoIdentityNotificationRegistry = Map< + 'default' | RepoIdentityNotificationChannel, + () => void +> +export type RepoIdentityBackgroundTask = { + generation: number + promise: Promise + pendingRequest: RepoIdentityEnrichmentRequest | null + onChangedByChannel: RepoIdentityNotificationRegistry +} + +export function enrichmentRequest(probeRuntimeHostPaths: boolean): RepoIdentityEnrichmentRequest { + return { probeRuntimeHostPaths } +} + +export function mergePendingEnrichmentRequest( + pending: RepoIdentityEnrichmentRequest | null, + probeRuntimeHostPaths: boolean +): RepoIdentityEnrichmentRequest { + return { + // Why: authority added while a pass is active applies only to its queued + // rerun; the active pass keeps the immutable request it started with. + probeRuntimeHostPaths: (pending?.probeRuntimeHostPaths ?? false) || probeRuntimeHostPaths + } +} + +export function rememberRepoIdentityNotification( + registry: RepoIdentityNotificationRegistry, + channel: RepoIdentityNotificationChannel | undefined, + onChanged: (() => void) | undefined +): void { + if (onChanged) { + registry.set(channel ?? 'default', onChanged) + } +} + +export function notifyRepoIdentityConsumers(registry: RepoIdentityNotificationRegistry): void { + for (const [channel, onChanged] of registry) { + try { + onChanged() + } catch (error: unknown) { + // Why: one consumer's notification failure must not suppress the other + // semantic channel after the shared enrichment already persisted data. + console.error(`[repo-identity] Failed to notify ${channel} consumer:`, error) + } + } +} diff --git a/src/main/repo-git-remote-identity-enrichment-coalescing.test.ts b/src/main/repo-git-remote-identity-enrichment-coalescing.test.ts new file mode 100644 index 00000000000..866d1a9b746 --- /dev/null +++ b/src/main/repo-git-remote-identity-enrichment-coalescing.test.ts @@ -0,0 +1,190 @@ +import { afterEach, describe, expect, it, vi } from 'vitest' +import type { GitRemoteIdentity } from '../shared/git-remote-identity' +import type { Repo } from '../shared/types' +import { detectGitRemoteIdentity } from './repo-git-remote-identity' +import { + enrichMissingRepoGitRemoteIdentities, + flushRepoGitRemoteIdentityEnrichmentForTests, + getRepoGitRemoteIdentityBackgroundTaskCountForTests, + resetRepoGitRemoteIdentityEnrichmentForTests +} from './repo-git-remote-identity-enrichment' + +vi.mock('./repo-git-remote-identity', () => ({ + detectGitRemoteIdentity: vi.fn() +})) + +const remoteIdentity: GitRemoteIdentity = { + canonicalKey: 'git.company.test/team/app', + remoteName: 'origin', + remoteUrl: 'git@git.company.test:team/app.git' +} + +function repo(id: string, executionHostId?: `runtime:${string}`): Repo { + return { + id, + path: `/workspace/${id}`, + displayName: id, + badgeColor: '#737373', + addedAt: 1, + kind: 'git', + ...(executionHostId ? { executionHostId } : {}) + } +} + +function storeFor(repos: Repo[]) { + return { + getRepos: () => repos, + updateRepo: vi.fn((id: string, updates: Partial, expected?: Repo) => { + const target = repos.find( + (candidate) => + candidate.id === id && + (!expected || + (candidate.path === expected.path && + candidate.addedAt === expected.addedAt && + (candidate.connectionId ?? null) === (expected.connectionId ?? null) && + (candidate.executionHostId ?? null) === (expected.executionHostId ?? null))) + ) + if (!target) { + return null + } + Object.assign(target, updates) + return target + }) + } +} + +function deferred() { + let resolve!: (value: T) => void + let reject!: (error: Error) => void + const promise = new Promise((resolvePromise, rejectPromise) => { + resolve = resolvePromise + reject = rejectPromise + }) + return { promise, resolve, reject } +} + +afterEach(() => { + vi.clearAllMocks() + resetRepoGitRemoteIdentityEnrichmentForTests() +}) + +describe('remote identity enrichment consumer coalescing', () => { + it.each([ + { name: 'desktop then runtime', firstChannel: 'desktop' as const, firstAuthority: false }, + { name: 'runtime then desktop', firstChannel: 'runtime' as const, firstAuthority: true } + ])('notifies both channels exactly once for $name overlap', async (scenario) => { + const firstProbe = deferred() + vi.mocked(detectGitRemoteIdentity) + .mockReturnValueOnce(firstProbe.promise) + .mockResolvedValue(remoteIdentity) + const repos = [repo('local'), repo('remote', 'runtime:environment-1')] + const store = storeFor(repos) + const desktopChanged = vi.fn() + const runtimeChanged = vi.fn() + const callback = scenario.firstChannel === 'desktop' ? desktopChanged : runtimeChanged + + enrichMissingRepoGitRemoteIdentities(store, { + notificationChannel: scenario.firstChannel, + probeRuntimeHostPaths: scenario.firstAuthority, + onChanged: callback + }) + enrichMissingRepoGitRemoteIdentities(store, { + notificationChannel: scenario.firstChannel === 'desktop' ? 'runtime' : 'desktop', + probeRuntimeHostPaths: scenario.firstChannel === 'desktop', + onChanged: scenario.firstChannel === 'desktop' ? runtimeChanged : desktopChanged + }) + // A later low-authority request must not replace the pending authority or callback. + enrichMissingRepoGitRemoteIdentities(store, { + notificationChannel: 'desktop', + onChanged: desktopChanged + }) + firstProbe.resolve(remoteIdentity) + await flushRepoGitRemoteIdentityEnrichmentForTests() + + expect(repos.every((candidate) => candidate.gitRemoteIdentity)).toBe(true) + expect(detectGitRemoteIdentity).toHaveBeenCalledTimes(2) + expect(desktopChanged).toHaveBeenCalledTimes(1) + expect(runtimeChanged).toHaveBeenCalledTimes(1) + }) + + it('does not notify either channel when every pass is unchanged', async () => { + const firstProbe = deferred() + vi.mocked(detectGitRemoteIdentity).mockReturnValueOnce(firstProbe.promise).mockResolvedValue(null) + const store = storeFor([repo('local'), repo('remote', 'runtime:environment-1')]) + const desktopChanged = vi.fn() + const runtimeChanged = vi.fn() + + enrichMissingRepoGitRemoteIdentities(store, { + notificationChannel: 'desktop', + onChanged: desktopChanged + }) + enrichMissingRepoGitRemoteIdentities(store, { + notificationChannel: 'runtime', + probeRuntimeHostPaths: true, + onChanged: runtimeChanged + }) + firstProbe.resolve(null) + await flushRepoGitRemoteIdentityEnrichmentForTests() + + expect(desktopChanged).not.toHaveBeenCalled() + expect(runtimeChanged).not.toHaveBeenCalled() + }) + + it('keeps both channels through an errored pass and notifies once after a changed rerun', async () => { + const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => undefined) + vi.mocked(detectGitRemoteIdentity) + .mockRejectedValueOnce(new Error('probe failed')) + .mockResolvedValue(remoteIdentity) + const store = storeFor([repo('local'), repo('remote', 'runtime:environment-1')]) + const desktopChanged = vi.fn() + const runtimeChanged = vi.fn() + + enrichMissingRepoGitRemoteIdentities(store, { + notificationChannel: 'desktop', + onChanged: desktopChanged + }) + enrichMissingRepoGitRemoteIdentities(store, { + notificationChannel: 'runtime', + probeRuntimeHostPaths: true, + onChanged: runtimeChanged + }) + await flushRepoGitRemoteIdentityEnrichmentForTests() + + expect(errorSpy).toHaveBeenCalledTimes(1) + expect(desktopChanged).toHaveBeenCalledTimes(1) + expect(runtimeChanged).toHaveBeenCalledTimes(1) + }) + + it('drops stale-generation channels on reset instead of leaking them into new work', async () => { + const staleProbe = deferred() + vi.mocked(detectGitRemoteIdentity) + .mockReturnValueOnce(staleProbe.promise) + .mockResolvedValue(remoteIdentity) + const store = storeFor([repo('local')]) + const staleDesktopChanged = vi.fn() + const staleRuntimeChanged = vi.fn() + const currentDesktopChanged = vi.fn() + + enrichMissingRepoGitRemoteIdentities(store, { + notificationChannel: 'desktop', + onChanged: staleDesktopChanged + }) + enrichMissingRepoGitRemoteIdentities(store, { + notificationChannel: 'runtime', + onChanged: staleRuntimeChanged + }) + resetRepoGitRemoteIdentityEnrichmentForTests() + staleProbe.resolve(remoteIdentity) + await vi.waitFor(() => expect(getRepoGitRemoteIdentityBackgroundTaskCountForTests()).toBe(0)) + + enrichMissingRepoGitRemoteIdentities(store, { + notificationChannel: 'desktop', + onChanged: currentDesktopChanged + }) + await flushRepoGitRemoteIdentityEnrichmentForTests() + + expect(staleDesktopChanged).not.toHaveBeenCalled() + expect(staleRuntimeChanged).not.toHaveBeenCalled() + expect(currentDesktopChanged).toHaveBeenCalledTimes(1) + }) +}) diff --git a/src/main/repo-git-remote-identity-enrichment.test.ts b/src/main/repo-git-remote-identity-enrichment.test.ts index 81a8d2dbd55..480493cbff9 100644 --- a/src/main/repo-git-remote-identity-enrichment.test.ts +++ b/src/main/repo-git-remote-identity-enrichment.test.ts @@ -3,8 +3,12 @@ import type { GitRemoteIdentity } from '../shared/git-remote-identity' import type { Repo } from '../shared/types' import { detectGitRemoteIdentity } from './repo-git-remote-identity' import { + MAX_REPO_REMOTE_IDENTITY_NEGATIVE_CACHE_LOCATIONS, enrichMissingRepoGitRemoteIdentities, flushRepoGitRemoteIdentityEnrichmentForTests, + getRepoGitRemoteIdentityBackgroundTaskCountForTests, + getRepoGitRemoteIdentityEnrichmentCountsForTests, + hasRepoGitRemoteIdentityNegativeCacheForTests, resetRepoGitRemoteIdentityEnrichmentForTests } from './repo-git-remote-identity-enrichment' @@ -15,7 +19,11 @@ vi.mock('./repo-git-remote-identity', () => ({ type RepoIdentityStore = { getRepos: () => Repo[] getRepo: (id: string) => Repo | undefined - updateRepo: (id: string, updates: Pick, 'gitRemoteIdentity'>) => Repo | null + updateRepo: ( + id: string, + updates: Pick, 'gitRemoteIdentity'>, + expectedLifecycle?: Repo + ) => Repo | null } const remoteIdentity: GitRemoteIdentity = { @@ -36,22 +44,38 @@ function makeRepo(overrides: Partial = {}): Repo { } } -function makeStore(repo: Repo): RepoIdentityStore & { updateRepo: ReturnType } { - const repos = [repo] +function makeStoreForRepos( + repos: Repo[] +): RepoIdentityStore & { updateRepo: ReturnType } { return { getRepos: () => repos, getRepo: (id) => repos.find((candidate) => candidate.id === id), - updateRepo: vi.fn((id, updates) => { - const target = repos.find((candidate) => candidate.id === id) - if (!target) { - return null + updateRepo: vi.fn( + (id: string, updates: Pick, 'gitRemoteIdentity'>, expectedLifecycle?: Repo) => { + const target = repos.find( + (candidate) => + candidate.id === id && + (!expectedLifecycle || + (candidate.path === expectedLifecycle.path && + candidate.addedAt === expectedLifecycle.addedAt && + (candidate.connectionId ?? null) === (expectedLifecycle.connectionId ?? null) && + (candidate.executionHostId ?? null) === + (expectedLifecycle.executionHostId ?? null))) + ) + if (!target) { + return null + } + Object.assign(target, updates) + return target } - Object.assign(target, updates) - return target - }) + ) } } +function makeStore(repo: Repo): RepoIdentityStore & { updateRepo: ReturnType } { + return makeStoreForRepos([repo]) +} + function deferred(): { promise: Promise resolve: (value: T) => void @@ -87,6 +111,32 @@ describe('enrichMissingRepoGitRemoteIdentities', () => { expect(onChanged).toHaveBeenCalledTimes(1) }) + it('flushes the whole sequential pass, including probes that start later', async () => { + const firstProbe = deferred() + const secondProbe = deferred() + vi.mocked(detectGitRemoteIdentity) + .mockReturnValueOnce(firstProbe.promise) + .mockReturnValueOnce(secondProbe.promise) + const store = makeStoreForRepos([ + makeRepo({ id: 'first', path: '/workspace/first' }), + makeRepo({ id: 'second', path: '/workspace/second' }) + ]) + + enrichMissingRepoGitRemoteIdentities(store) + let flushed = false + const flush = flushRepoGitRemoteIdentityEnrichmentForTests().then(() => { + flushed = true + }) + firstProbe.resolve(null) + await vi.waitFor(() => expect(detectGitRemoteIdentity).toHaveBeenCalledTimes(2)) + + expect(flushed).toBe(false) + secondProbe.resolve(null) + await flush + + expect(flushed).toBe(true) + }) + it('coalesces concurrent probes for the same repo location', async () => { const probe = deferred() vi.mocked(detectGitRemoteIdentity).mockReturnValue(probe.promise) @@ -105,6 +155,115 @@ describe('enrichMissingRepoGitRemoteIdentities', () => { expect(repo.gitRemoteIdentity).toEqual(remoteIdentity) }) + it('bounds repeated list requests to one active pass and one newest rerun', async () => { + const probe = deferred() + vi.mocked(detectGitRemoteIdentity).mockReturnValue(probe.promise) + const repo = makeRepo() + const store = makeStore(repo) + const onChanged = vi.fn() + + enrichMissingRepoGitRemoteIdentities(store, { onChanged }) + for (let index = 0; index < 100; index++) { + enrichMissingRepoGitRemoteIdentities(store, { onChanged }) + } + + expect(detectGitRemoteIdentity).toHaveBeenCalledTimes(1) + expect(getRepoGitRemoteIdentityBackgroundTaskCountForTests()).toBe(1) + probe.resolve(remoteIdentity) + await flushRepoGitRemoteIdentityEnrichmentForTests() + + expect(detectGitRemoteIdentity).toHaveBeenCalledTimes(1) + expect(getRepoGitRemoteIdentityBackgroundTaskCountForTests()).toBe(0) + expect(onChanged).toHaveBeenCalledTimes(1) + }) + + it('skips runtime-host paths that this process cannot probe', async () => { + const store = makeStore( + makeRepo({ executionHostId: 'runtime:environment-1', path: '/runtime/workspace/repo' }) + ) + + enrichMissingRepoGitRemoteIdentities(store) + await flushRepoGitRemoteIdentityEnrichmentForTests() + + expect(detectGitRemoteIdentity).not.toHaveBeenCalled() + expect(store.updateRepo).not.toHaveBeenCalled() + }) + + it('probes a runtime-stamped path when the owning runtime grants authority', async () => { + vi.mocked(detectGitRemoteIdentity).mockResolvedValue(remoteIdentity) + const repo = makeRepo({ + executionHostId: 'runtime:environment-1', + path: '/runtime/workspace/repo' + }) + const store = makeStore(repo) + + enrichMissingRepoGitRemoteIdentities(store, { probeRuntimeHostPaths: true }) + await flushRepoGitRemoteIdentityEnrichmentForTests() + + expect(detectGitRemoteIdentity).toHaveBeenCalledWith('/runtime/workspace/repo', undefined) + expect(repo.gitRemoteIdentity).toEqual(remoteIdentity) + }) + + it('keeps SSH identity probes on the SSH provider path', async () => { + vi.mocked(detectGitRemoteIdentity).mockResolvedValue(remoteIdentity) + const repo = makeRepo({ + connectionId: 'ssh-target', + executionHostId: 'runtime:environment-1', + path: '/remote/workspace/repo' + }) + const store = makeStore(repo) + + enrichMissingRepoGitRemoteIdentities(store) + await flushRepoGitRemoteIdentityEnrichmentForTests() + + expect(detectGitRemoteIdentity).toHaveBeenCalledWith('/remote/workspace/repo', 'ssh-target') + expect(repo.gitRemoteIdentity).toEqual(remoteIdentity) + }) + + it('enriches duplicate repo IDs independently across execution hosts', async () => { + vi.mocked(detectGitRemoteIdentity).mockImplementation(async (_path, connectionId) => ({ + ...remoteIdentity, + canonicalKey: `git.company.test/${connectionId ?? 'local'}` + })) + const localRepo = makeRepo({ id: 'shared', path: '/same/path' }) + const sshRepo = makeRepo({ + id: 'shared', + path: '/same/path', + connectionId: 'ssh-target', + executionHostId: 'ssh:ssh-target' + }) + const store = makeStoreForRepos([localRepo, sshRepo]) + + enrichMissingRepoGitRemoteIdentities(store) + await flushRepoGitRemoteIdentityEnrichmentForTests() + + expect(localRepo.gitRemoteIdentity?.canonicalKey).toBe('git.company.test/local') + expect(sshRepo.gitRemoteIdentity?.canonicalKey).toBe('git.company.test/ssh-target') + expect(store.updateRepo).toHaveBeenCalledTimes(2) + }) + + it('does not let reset-era local work mutate a runtime-host replacement', async () => { + const staleProbe = deferred() + vi.mocked(detectGitRemoteIdentity).mockReturnValue(staleProbe.promise) + const repos = [makeRepo()] + const store = makeStoreForRepos(repos) + + enrichMissingRepoGitRemoteIdentities(store) + resetRepoGitRemoteIdentityEnrichmentForTests() + const replacement = makeRepo({ addedAt: 2, executionHostId: 'runtime:environment-1' }) + repos.splice(0, repos.length, replacement) + enrichMissingRepoGitRemoteIdentities(store) + await flushRepoGitRemoteIdentityEnrichmentForTests() + staleProbe.resolve(remoteIdentity) + await Promise.resolve() + await Promise.resolve() + + expect(detectGitRemoteIdentity).toHaveBeenCalledTimes(1) + expect(store.updateRepo).not.toHaveBeenCalled() + expect(replacement.gitRemoteIdentity).toBeUndefined() + expect(hasRepoGitRemoteIdentityNegativeCacheForTests(replacement)).toBe(false) + }) + it('caches no-identity probes briefly so list calls do not retry every time', async () => { vi.useFakeTimers() vi.setSystemTime(1_000) @@ -120,6 +279,201 @@ describe('enrichMissingRepoGitRemoteIdentities', () => { expect(detectGitRemoteIdentity).toHaveBeenCalledTimes(1) }) + it('keeps runtime-host retry TTLs across desktop passes without granting probe authority', async () => { + vi.useFakeTimers() + vi.setSystemTime(1_000) + vi.mocked(detectGitRemoteIdentity).mockResolvedValue(null) + const repo = makeRepo({ + executionHostId: 'runtime:environment-1', + path: '/runtime/workspace/repo' + }) + const repos = [repo] + const store = makeStoreForRepos(repos) + + enrichMissingRepoGitRemoteIdentities(store, { probeRuntimeHostPaths: true }) + await flushRepoGitRemoteIdentityEnrichmentForTests() + expect(detectGitRemoteIdentity).toHaveBeenCalledTimes(1) + expect(hasRepoGitRemoteIdentityNegativeCacheForTests(repo)).toBe(true) + + enrichMissingRepoGitRemoteIdentities(store) + await flushRepoGitRemoteIdentityEnrichmentForTests() + enrichMissingRepoGitRemoteIdentities(store, { probeRuntimeHostPaths: true }) + await flushRepoGitRemoteIdentityEnrichmentForTests() + + expect(detectGitRemoteIdentity).toHaveBeenCalledTimes(1) + expect(hasRepoGitRemoteIdentityNegativeCacheForTests(repo)).toBe(true) + + repos.splice(0, repos.length) + enrichMissingRepoGitRemoteIdentities(store) + await flushRepoGitRemoteIdentityEnrichmentForTests() + expect(hasRepoGitRemoteIdentityNegativeCacheForTests(repo)).toBe(false) + }) + + it('releases timed-out probe state and retries after the negative-cache TTL', async () => { + vi.useFakeTimers() + vi.setSystemTime(1_000) + vi.mocked(detectGitRemoteIdentity) + .mockResolvedValueOnce(null) + .mockResolvedValueOnce(remoteIdentity) + const repo = makeRepo() + const store = makeStore(repo) + + enrichMissingRepoGitRemoteIdentities(store) + await flushRepoGitRemoteIdentityEnrichmentForTests() + + expect(getRepoGitRemoteIdentityEnrichmentCountsForTests()).toEqual({ + inFlight: 0, + negativeCache: 1 + }) + expect(getRepoGitRemoteIdentityBackgroundTaskCountForTests()).toBe(0) + + vi.advanceTimersByTime(5 * 60 * 1000 + 1) + enrichMissingRepoGitRemoteIdentities(store) + await flushRepoGitRemoteIdentityEnrichmentForTests() + + expect(detectGitRemoteIdentity).toHaveBeenCalledTimes(2) + expect(repo.gitRemoteIdentity).toEqual(remoteIdentity) + expect(getRepoGitRemoteIdentityBackgroundTaskCountForTests()).toBe(0) + }) + + it('prunes no-identity retry entries for removed repo locations', async () => { + vi.useFakeTimers() + vi.setSystemTime(1_000) + vi.mocked(detectGitRemoteIdentity).mockResolvedValue(null) + const oldRepo = makeRepo({ id: 'old', path: '/workspace/old' }) + const newRepo = makeRepo({ id: 'new', path: '/workspace/new' }) + const repos = [oldRepo] + const store = makeStoreForRepos(repos) + + enrichMissingRepoGitRemoteIdentities(store) + await flushRepoGitRemoteIdentityEnrichmentForTests() + + expect(getRepoGitRemoteIdentityEnrichmentCountsForTests()).toEqual({ + inFlight: 0, + negativeCache: 1 + }) + expect(hasRepoGitRemoteIdentityNegativeCacheForTests(oldRepo)).toBe(true) + + repos.splice(0, repos.length, newRepo) + enrichMissingRepoGitRemoteIdentities(store) + await flushRepoGitRemoteIdentityEnrichmentForTests() + + expect(getRepoGitRemoteIdentityEnrichmentCountsForTests()).toEqual({ + inFlight: 0, + negativeCache: 1 + }) + expect(hasRepoGitRemoteIdentityNegativeCacheForTests(oldRepo)).toBe(false) + expect(hasRepoGitRemoteIdentityNegativeCacheForTests(newRepo)).toBe(true) + }) + + it('caps no-identity retry entries while retaining recently refreshed locations', async () => { + vi.useFakeTimers() + vi.setSystemTime(1_000) + vi.mocked(detectGitRemoteIdentity).mockResolvedValue(null) + const repos = Array.from( + { length: MAX_REPO_REMOTE_IDENTITY_NEGATIVE_CACHE_LOCATIONS - 1 }, + (_, index) => makeRepo({ id: `repo-${index}`, path: `/workspace/repo-${index}` }) + ) + const keepRepo = makeRepo({ id: 'keep', path: '/workspace/keep' }) + repos.push(keepRepo) + const store = makeStoreForRepos(repos) + + enrichMissingRepoGitRemoteIdentities(store) + await vi.waitFor(() => + expect(detectGitRemoteIdentity).toHaveBeenCalledTimes( + MAX_REPO_REMOTE_IDENTITY_NEGATIVE_CACHE_LOCATIONS + ) + ) + repos.push(makeRepo({ id: 'new', path: '/workspace/new' })) + enrichMissingRepoGitRemoteIdentities(store) + await flushRepoGitRemoteIdentityEnrichmentForTests() + + expect(detectGitRemoteIdentity).toHaveBeenCalledTimes( + MAX_REPO_REMOTE_IDENTITY_NEGATIVE_CACHE_LOCATIONS + 1 + ) + expect(hasRepoGitRemoteIdentityNegativeCacheForTests(repos[0]!)).toBe(false) + + enrichMissingRepoGitRemoteIdentities(store) + await flushRepoGitRemoteIdentityEnrichmentForTests() + + expect(getRepoGitRemoteIdentityEnrichmentCountsForTests()).toEqual({ + inFlight: 0, + negativeCache: MAX_REPO_REMOTE_IDENTITY_NEGATIVE_CACHE_LOCATIONS + }) + // One overflowed lifecycle is retried per pass; eviction must not cascade + // into a 513-process storm during the same repos:list call. + expect(detectGitRemoteIdentity).toHaveBeenCalledTimes( + MAX_REPO_REMOTE_IDENTITY_NEGATIVE_CACHE_LOCATIONS + 2 + ) + expect(hasRepoGitRemoteIdentityNegativeCacheForTests(repos[0]!)).toBe(true) + expect(hasRepoGitRemoteIdentityNegativeCacheForTests(repos[1]!)).toBe(false) + expect(hasRepoGitRemoteIdentityNegativeCacheForTests(keepRepo)).toBe(true) + expect(hasRepoGitRemoteIdentityNegativeCacheForTests(repos.at(-1)!)).toBe(true) + }) + + it('does not let a late null result suppress a replacement repo lifecycle', async () => { + const oldProbe = deferred() + vi.mocked(detectGitRemoteIdentity) + .mockReturnValueOnce(oldProbe.promise) + .mockResolvedValueOnce(remoteIdentity) + const oldRepo = makeRepo() + const repos = [oldRepo] + const store = makeStoreForRepos(repos) + + enrichMissingRepoGitRemoteIdentities(store) + const replacement = makeRepo({ addedAt: 2 }) + repos.splice(0, repos.length, replacement) + oldProbe.resolve(null) + await flushRepoGitRemoteIdentityEnrichmentForTests() + + expect(hasRepoGitRemoteIdentityNegativeCacheForTests(replacement)).toBe(false) + enrichMissingRepoGitRemoteIdentities(store) + await flushRepoGitRemoteIdentityEnrichmentForTests() + + expect(detectGitRemoteIdentity).toHaveBeenCalledTimes(2) + expect(replacement.gitRemoteIdentity).toEqual(remoteIdentity) + }) + + it('uses addedAt with the repo UUID to reject same-ID and same-path reuse', async () => { + const probe = deferred() + vi.mocked(detectGitRemoteIdentity).mockReturnValue(probe.promise) + const oldRepo = makeRepo() + const repos = [oldRepo] + const store = makeStoreForRepos(repos) + + enrichMissingRepoGitRemoteIdentities(store) + const replacement = makeRepo({ addedAt: 2 }) + repos.splice(0, repos.length, replacement) + probe.resolve(remoteIdentity) + await flushRepoGitRemoteIdentityEnrichmentForTests() + + expect(store.updateRepo).not.toHaveBeenCalled() + expect(replacement.gitRemoteIdentity).toBeUndefined() + }) + + it('isolates reset generations from pending probes', async () => { + const staleProbe = deferred() + vi.mocked(detectGitRemoteIdentity) + .mockReturnValueOnce(staleProbe.promise) + .mockResolvedValueOnce(remoteIdentity) + const repo = makeRepo() + const store = makeStore(repo) + + enrichMissingRepoGitRemoteIdentities(store) + resetRepoGitRemoteIdentityEnrichmentForTests() + enrichMissingRepoGitRemoteIdentities(store) + await flushRepoGitRemoteIdentityEnrichmentForTests() + + expect(repo.gitRemoteIdentity).toEqual(remoteIdentity) + staleProbe.resolve(null) + await vi.waitFor(() => + expect(getRepoGitRemoteIdentityEnrichmentCountsForTests().inFlight).toBe(0) + ) + + expect(hasRepoGitRemoteIdentityNegativeCacheForTests(repo)).toBe(false) + expect(store.updateRepo).toHaveBeenCalledTimes(1) + }) + it('does not write stale identity data after the repo path changes', async () => { const probe = deferred() vi.mocked(detectGitRemoteIdentity).mockReturnValue(probe.promise) diff --git a/src/main/repo-git-remote-identity-enrichment.ts b/src/main/repo-git-remote-identity-enrichment.ts index 166b4d0bec8..d20c91492aa 100644 --- a/src/main/repo-git-remote-identity-enrichment.ts +++ b/src/main/repo-git-remote-identity-enrichment.ts @@ -1,107 +1,337 @@ import type { Repo } from '../shared/types' +import { getRepoExecutionHostId, LOCAL_EXECUTION_HOST_ID } from '../shared/execution-host' import { detectGitRemoteIdentity } from './repo-git-remote-identity' +import * as coalescing from './repo-git-remote-identity-coalescing' const NO_IDENTITY_RETRY_TTL_MS = 5 * 60 * 1000 +export const MAX_REPO_REMOTE_IDENTITY_NEGATIVE_CACHE_LOCATIONS = 512 type RepoIdentityStore = { getRepos(): Repo[] - getRepo?(id: string): Repo | undefined - updateRepo(id: string, updates: Pick, 'gitRemoteIdentity'>): Repo | null + updateRepo( + id: string, + updates: Pick, 'gitRemoteIdentity'>, + expectedLifecycle?: RepoLifecycle + ): Repo | null } +type RepoLifecycle = Pick + type EnrichmentOptions = { onChanged?: () => void + notificationChannel?: coalescing.RepoIdentityNotificationChannel + /** Internal authority: this process owns runtime-stamped repo paths. */ + probeRuntimeHostPaths?: boolean } -const inFlightProbesByLocation = new Map>() -const noIdentityRetryAfterByLocation = new Map() +const storeIds = new WeakMap() +const inFlightProbesByLifecycle = new Map>() +const noIdentityRetryAfterByLifecycle = new Map() +const backgroundTasks = new Set() +let backgroundTaskByStore = new WeakMap() +let nextStoreId = 1 +let generation = 0 -function getRepoLocationKey(repo: Pick): string { - return `${repo.connectionId ?? 'local'}\0${repo.path}` +function getStoreId(store: RepoIdentityStore): number { + const existing = storeIds.get(store) + if (existing !== undefined) { + return existing + } + const id = nextStoreId++ + storeIds.set(store, id) + return id } -function getCurrentRepo(store: RepoIdentityStore, id: string): Repo | undefined { - return store.getRepo?.(id) ?? store.getRepos().find((repo) => repo.id === id) +function getRepoLifecycleKey(store: RepoIdentityStore, repo: Repo): string { + return [ + getStoreId(store), + repo.executionHostId ?? 'default-host', + repo.connectionId ?? 'local', + repo.path, + repo.id, + repo.addedAt + ].join('\0') } -function isSameUnenrichedRepo(snapshot: Repo, current: Repo | undefined): boolean { +function getStoreKeyPrefix(store: RepoIdentityStore): string { + return `${getStoreId(store)}\0` +} + +function pruneNoIdentityRetryLifecycles( + store: RepoIdentityStore, + liveLifecycleKeys: ReadonlySet, + now: number +): void { + const storeKeyPrefix = getStoreKeyPrefix(store) + for (const [lifecycleKey, retryAfter] of noIdentityRetryAfterByLifecycle) { + if ( + lifecycleKey.startsWith(storeKeyPrefix) && + (retryAfter <= now || !liveLifecycleKeys.has(lifecycleKey)) + ) { + noIdentityRetryAfterByLifecycle.delete(lifecycleKey) + } + } +} + +function rememberNoIdentityRetryLifecycle(lifecycleKey: string, retryAfter: number): void { + noIdentityRetryAfterByLifecycle.delete(lifecycleKey) + noIdentityRetryAfterByLifecycle.set(lifecycleKey, retryAfter) + while (noIdentityRetryAfterByLifecycle.size > MAX_REPO_REMOTE_IDENTITY_NEGATIVE_CACHE_LOCATIONS) { + const oldestLifecycleKey = noIdentityRetryAfterByLifecycle.keys().next().value + if (oldestLifecycleKey === undefined) { + break + } + noIdentityRetryAfterByLifecycle.delete(oldestLifecycleKey) + } +} + +function hasSameRepoLifecycle(snapshot: RepoLifecycle, current: Repo): boolean { return ( - !!current && - current.kind !== 'folder' && - !current.gitRemoteIdentity && + current.id === snapshot.id && + current.addedAt === snapshot.addedAt && current.path === snapshot.path && - (current.connectionId ?? null) === (snapshot.connectionId ?? null) + (current.connectionId ?? null) === (snapshot.connectionId ?? null) && + (current.executionHostId ?? null) === (snapshot.executionHostId ?? null) ) } -async function enrichRepoGitRemoteIdentity(store: RepoIdentityStore, repo: Repo): Promise { - const locationKey = getRepoLocationKey(repo) - const retryAfter = noIdentityRetryAfterByLocation.get(locationKey) ?? 0 - if (retryAfter > Date.now()) { +function getCurrentRepo(store: RepoIdentityStore, snapshot: RepoLifecycle): Repo | undefined { + return store.getRepos().find((repo) => hasSameRepoLifecycle(snapshot, repo)) +} + +function supportsGitRemoteIdentityProbe(repo: Repo, probeRuntimeHostPaths: boolean): boolean { + const executionHostId = getRepoExecutionHostId(repo) + // Why: detectGitRemoteIdentity can execute only on this machine or through + // an SSH connection; runtime-host paths need their owning runtime to probe. + return ( + repo.kind !== 'folder' && + (!!repo.connectionId || + executionHostId === LOCAL_EXECUTION_HOST_ID || + (probeRuntimeHostPaths && executionHostId.startsWith('runtime:'))) + ) +} + +function isSameUnenrichedRepo( + snapshot: Repo, + current: Repo | undefined, + probeRuntimeHostPaths: boolean +): boolean { + return ( + !!current && + hasSameRepoLifecycle(snapshot, current) && + supportsGitRemoteIdentityProbe(current, probeRuntimeHostPaths) && + !current.gitRemoteIdentity && + current.kind !== 'folder' + ) +} + +async function enrichRepoGitRemoteIdentity( + store: RepoIdentityStore, + repo: Repo, + taskGeneration: number, + retrySnapshot: ReadonlyMap, + probeRuntimeHostPaths: boolean +): Promise { + const lifecycleKey = getRepoLifecycleKey(store, repo) + const retryAfterAtPassStart = retrySnapshot.get(lifecycleKey) ?? 0 + if (retryAfterAtPassStart > Date.now()) { + // Why: a cap eviction earlier in this pass must not turn every remaining + // cached repo into a new git probe cascade. + if (noIdentityRetryAfterByLifecycle.get(lifecycleKey) === retryAfterAtPassStart) { + rememberNoIdentityRetryLifecycle(lifecycleKey, retryAfterAtPassStart) + } return false } - const inFlight = inFlightProbesByLocation.get(locationKey) + const inFlight = inFlightProbesByLifecycle.get(lifecycleKey) if (inFlight) { return inFlight } const probe = (async () => { const identity = await detectGitRemoteIdentity(repo.path, repo.connectionId) + if ( + generation !== taskGeneration || + !isSameUnenrichedRepo(repo, getCurrentRepo(store, repo), probeRuntimeHostPaths) + ) { + return false + } if (!identity) { // Why: repos without a parseable remote are common; cache misses briefly so // list calls stay cheap while still allowing recent remote changes to land. - noIdentityRetryAfterByLocation.set(locationKey, Date.now() + NO_IDENTITY_RETRY_TTL_MS) + rememberNoIdentityRetryLifecycle(lifecycleKey, Date.now() + NO_IDENTITY_RETRY_TTL_MS) return false } - noIdentityRetryAfterByLocation.delete(locationKey) - const current = getCurrentRepo(store, repo.id) - if (!isSameUnenrichedRepo(repo, current)) { - return false - } - return !!store.updateRepo(repo.id, { gitRemoteIdentity: identity }) + noIdentityRetryAfterByLifecycle.delete(lifecycleKey) + return !!store.updateRepo(repo.id, { gitRemoteIdentity: identity }, repo) })().finally(() => { - if (inFlightProbesByLocation.get(locationKey) === probe) { - inFlightProbesByLocation.delete(locationKey) + if (generation === taskGeneration && inFlightProbesByLifecycle.get(lifecycleKey) === probe) { + inFlightProbesByLifecycle.delete(lifecycleKey) } }) - inFlightProbesByLocation.set(locationKey, probe) + inFlightProbesByLifecycle.set(lifecycleKey, probe) return probe } async function enrichMissingRepoGitRemoteIdentitiesInBackground( store: RepoIdentityStore, - options: EnrichmentOptions -): Promise { - const candidates = store - .getRepos() - .filter((repo) => repo.kind !== 'folder' && !repo.gitRemoteIdentity) + request: coalescing.RepoIdentityEnrichmentRequest, + taskGeneration: number +): Promise { + const repos = store.getRepos() + const probeRuntimeHostPaths = request.probeRuntimeHostPaths + // Why: a desktop pass cannot probe runtime-owned paths, but it must not evict + // the owning runtime's TTL entry while that same lifecycle is still persisted. + const liveLifecycleKeys = new Set(repos.map((repo) => getRepoLifecycleKey(store, repo))) + pruneNoIdentityRetryLifecycles(store, liveLifecycleKeys, Date.now()) + const retrySnapshot = new Map(noIdentityRetryAfterByLifecycle) + const candidates = repos.filter( + (repo) => supportsGitRemoteIdentityProbe(repo, probeRuntimeHostPaths) && !repo.gitRemoteIdentity + ) let changed = false for (const repo of candidates) { - // Why: enrichment runs later; capture the location we probed so a mutable - // store cannot make the stale-write guard compare against changed fields. - if (await enrichRepoGitRemoteIdentity(store, { ...repo })) { + if (generation !== taskGeneration) { + return false + } + // Why: enrichment runs later; capture the lifecycle fields before awaiting + // so a removed, renamed, or ID-reused repo cannot receive stale data. + if ( + await enrichRepoGitRemoteIdentity( + store, + { ...repo }, + taskGeneration, + retrySnapshot, + probeRuntimeHostPaths + ) + ) { changed = true } } - if (changed) { - options.onChanged?.() + return changed && generation === taskGeneration +} + +async function runBackgroundTask( + store: RepoIdentityStore, + task: coalescing.RepoIdentityBackgroundTask, + initialRequest: coalescing.RepoIdentityEnrichmentRequest +): Promise { + let request = initialRequest + let changed = false + while (generation === task.generation) { + try { + changed = + (await enrichMissingRepoGitRemoteIdentitiesInBackground( + store, + request, + task.generation + )) || changed + } catch (error: unknown) { + console.error('[repo-identity] Failed to enrich git remote identities:', error) + } + if (generation !== task.generation) { + return + } + const pendingRequest = task.pendingRequest + task.pendingRequest = null + if (!pendingRequest) { + break + } + request = pendingRequest } + if (changed && generation === task.generation) { + coalescing.notifyRepoIdentityConsumers(task.onChangedByChannel) + } +} + +function startBackgroundEnrichment( + store: RepoIdentityStore, + options: EnrichmentOptions, + taskGeneration: number +): void { + const task: coalescing.RepoIdentityBackgroundTask = { + generation: taskGeneration, + promise: Promise.resolve(), + pendingRequest: null, + onChangedByChannel: new Map() + } + coalescing.rememberRepoIdentityNotification( + task.onChangedByChannel, + options.notificationChannel, + options.onChanged + ) + backgroundTaskByStore.set(store, task) + task.promise = runBackgroundTask( + store, + task, + coalescing.enrichmentRequest(options.probeRuntimeHostPaths === true) + ) + .finally(() => { + backgroundTasks.delete(task) + task.onChangedByChannel.clear() + task.pendingRequest = null + if (generation === taskGeneration && backgroundTaskByStore.get(store) === task) { + backgroundTaskByStore.delete(store) + } + }) + backgroundTasks.add(task) } export function enrichMissingRepoGitRemoteIdentities( store: RepoIdentityStore, options: EnrichmentOptions = {} ): void { - void enrichMissingRepoGitRemoteIdentitiesInBackground(store, options).catch((error: unknown) => { - console.error('[repo-identity] Failed to enrich git remote identities:', error) - }) + const existing = backgroundTaskByStore.get(store) + if (existing?.generation === generation) { + // Why: list RPCs can repeat while git is slow; retain one bounded rerun and + // one latest callback per semantic consumer instead of one task per request. + existing.pendingRequest = coalescing.mergePendingEnrichmentRequest( + existing.pendingRequest, + options.probeRuntimeHostPaths === true + ) + coalescing.rememberRepoIdentityNotification( + existing.onChangedByChannel, + options.notificationChannel, + options.onChanged + ) + return + } + startBackgroundEnrichment(store, options, generation) } export async function flushRepoGitRemoteIdentityEnrichmentForTests(): Promise { - await Promise.all(inFlightProbesByLocation.values()) + const flushGeneration = generation + while (true) { + const tasks = [...backgroundTasks] + .filter((task) => task.generation === flushGeneration) + .map((task) => task.promise) + if (tasks.length === 0) { + return + } + await Promise.all(tasks) + } } export function resetRepoGitRemoteIdentityEnrichmentForTests(): void { - inFlightProbesByLocation.clear() - noIdentityRetryAfterByLocation.clear() + generation++ + backgroundTaskByStore = new WeakMap() + inFlightProbesByLifecycle.clear() + noIdentityRetryAfterByLifecycle.clear() +} + +export function getRepoGitRemoteIdentityBackgroundTaskCountForTests(): number { + return backgroundTasks.size +} + +export function getRepoGitRemoteIdentityEnrichmentCountsForTests(): { + inFlight: number + negativeCache: number +} { + return { + inFlight: inFlightProbesByLifecycle.size, + negativeCache: noIdentityRetryAfterByLifecycle.size + } +} + +export function hasRepoGitRemoteIdentityNegativeCacheForTests(repo: Repo): boolean { + const suffix = `\0${repo.path}\0${repo.id}\0${repo.addedAt}` + return [...noIdentityRetryAfterByLifecycle.keys()].some((key) => key.endsWith(suffix)) } diff --git a/src/main/repo-git-remote-identity.test.ts b/src/main/repo-git-remote-identity.test.ts new file mode 100644 index 00000000000..4079b8f5d04 --- /dev/null +++ b/src/main/repo-git-remote-identity.test.ts @@ -0,0 +1,54 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const gitExecFileAsyncMock = vi.hoisted(() => vi.fn()) +const sshExecMock = vi.hoisted(() => vi.fn()) +const getSshGitProviderMock = vi.hoisted(() => vi.fn()) + +vi.mock('./git/runner', () => ({ + gitExecFileAsync: gitExecFileAsyncMock +})) + +vi.mock('./providers/ssh-git-dispatch', () => ({ + getSshGitProvider: getSshGitProviderMock +})) + +import { detectGitRemoteIdentity } from './repo-git-remote-identity' + +const remoteOutput = 'origin\tgit@git.company.test:team/repo.git (fetch)\n' + +describe('detectGitRemoteIdentity', () => { + beforeEach(() => { + vi.clearAllMocks() + gitExecFileAsyncMock.mockResolvedValue({ stdout: remoteOutput, stderr: '' }) + sshExecMock.mockResolvedValue({ stdout: remoteOutput, stderr: '' }) + getSshGitProviderMock.mockReturnValue({ exec: sshExecMock }) + }) + + it('bounds local git remote inspection', async () => { + await expect(detectGitRemoteIdentity('/workspace/repo')).resolves.toMatchObject({ + canonicalKey: 'git.company.test/team/repo' + }) + + expect(gitExecFileAsyncMock).toHaveBeenCalledWith(['remote', '-v'], { + cwd: '/workspace/repo', + timeout: 5000 + }) + }) + + it('bounds SSH git remote inspection on the selected provider', async () => { + await expect(detectGitRemoteIdentity('/remote/repo', 'ssh-target')).resolves.toMatchObject({ + canonicalKey: 'git.company.test/team/repo' + }) + + expect(getSshGitProviderMock).toHaveBeenCalledWith('ssh-target') + expect(sshExecMock).toHaveBeenCalledWith(['remote', '-v'], '/remote/repo', { + timeoutMs: 5000 + }) + }) + + it('returns null when the SSH provider is unavailable', async () => { + getSshGitProviderMock.mockReturnValue(undefined) + + await expect(detectGitRemoteIdentity('/remote/repo', 'missing')).resolves.toBeNull() + }) +}) diff --git a/src/main/repo-git-remote-identity.ts b/src/main/repo-git-remote-identity.ts index f5ba64a301a..8332bce3c98 100644 --- a/src/main/repo-git-remote-identity.ts +++ b/src/main/repo-git-remote-identity.ts @@ -2,14 +2,21 @@ import { deriveGitRemoteIdentity, type GitRemoteIdentity } from '../shared/git-r import { gitExecFileAsync } from './git/runner' import { getSshGitProvider } from './providers/ssh-git-dispatch' +const GIT_REMOTE_IDENTITY_TIMEOUT_MS = 5000 + export async function detectGitRemoteIdentity( repoPath: string, connectionId?: string | null ): Promise { try { const result = connectionId - ? await getSshGitProvider(connectionId)?.exec(['remote', '-v'], repoPath) - : await gitExecFileAsync(['remote', '-v'], { cwd: repoPath }) + ? await getSshGitProvider(connectionId)?.exec(['remote', '-v'], repoPath, { + timeoutMs: GIT_REMOTE_IDENTITY_TIMEOUT_MS + }) + : await gitExecFileAsync(['remote', '-v'], { + cwd: repoPath, + timeout: GIT_REMOTE_IDENTITY_TIMEOUT_MS + }) return result ? deriveGitRemoteIdentity(result.stdout) : null } catch { // Repo creation must not fail because a best-effort remote probe failed. diff --git a/src/main/repo-git-username-enrichment-subprocess.test.ts b/src/main/repo-git-username-enrichment-subprocess.test.ts new file mode 100644 index 00000000000..2128081a016 --- /dev/null +++ b/src/main/repo-git-username-enrichment-subprocess.test.ts @@ -0,0 +1,110 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import type { Repo } from '../shared/types' +import type * as RunnerModule from './git/runner' + +const gitExecFileAsyncMock = vi.hoisted(() => vi.fn()) +const ghExecFileAsyncMock = vi.hoisted(() => vi.fn()) + +vi.mock('./git/runner', async () => { + const actual = await vi.importActual('./git/runner') + return { + ...actual, + gitExecFileAsync: gitExecFileAsyncMock, + ghExecFileAsync: ghExecFileAsyncMock + } +}) + +import { + LOCAL_GIT_USERNAME_TIMEOUT_RETRY_MS, + resetGhLoginCacheForTests +} from './git/git-username' +import { + enrichRepoGitUsernames, + flushRepoGitUsernameEnrichmentForTests, + resetRepoGitUsernameEnrichmentForTests +} from './repo-git-username-enrichment' + +function makeRepo(overrides: Partial = {}): Repo { + return { + id: 'shared', + path: 'C:/repos/one', + displayName: 'One', + badgeColor: '#000', + addedAt: 1, + ...overrides + } as Repo +} + +function makeExecError(message: string, code?: string): Error { + return Object.assign(new Error(message), { stdout: '', stderr: '', code }) +} + +describe('repo git username enrichment subprocess budget', () => { + beforeEach(() => { + vi.resetAllMocks() + resetGhLoginCacheForTests() + resetRepoGitUsernameEnrichmentForTests() + gitExecFileAsyncMock.mockImplementation(async (args: string[]) => { + if (args[0] === 'remote' && args.length === 1) { + return { stdout: 'origin\n', stderr: '' } + } + if (args[0] === 'branch' && args[1] === '--show-current') { + return { stdout: '\n', stderr: '' } + } + if (args[0] === 'remote' && args[1] === 'get-url' && args[2] === 'origin') { + return { stdout: 'https://github.com/stablyai/orca.git\n', stderr: '' } + } + throw makeExecError(`missing git value: ${args.join(' ')}`) + }) + }) + + afterEach(() => { + vi.useRealTimers() + }) + + it('suppresses every git and gh subprocess until the lifecycle cooldown expires', async () => { + vi.useFakeTimers() + ghExecFileAsyncMock + .mockRejectedValueOnce(makeExecError('gh timed out', 'ETIMEDOUT')) + .mockResolvedValueOnce({ stdout: 'recovered-user\n', stderr: '' }) + const localRepo = makeRepo() + const repos = [ + makeRepo({ connectionId: 'ssh-1', executionHostId: 'ssh:ssh-1' }), + makeRepo({ executionHostId: 'runtime:environment-1' }), + localRepo + ] + const store = { + getRepos: () => repos, + setResolvedRepoGitUsername: vi.fn(() => true) + } + + enrichRepoGitUsernames(store) + await flushRepoGitUsernameEnrichmentForTests() + + // Two config reads + remote/default-base/current-branch/effective-remote probes. + expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(10) + expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(1) + + enrichRepoGitUsernames(store) + await flushRepoGitUsernameEnrichmentForTests() + + // Repeated repos:list calls and non-local provider twins do zero extra work. + expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(10) + expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(1) + + await vi.advanceTimersByTimeAsync(LOCAL_GIT_USERNAME_TIMEOUT_RETRY_MS + 1) + enrichRepoGitUsernames(store) + await flushRepoGitUsernameEnrichmentForTests() + + expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(20) + expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(2) + expect(store.setResolvedRepoGitUsername).toHaveBeenCalledWith( + 'shared', + 'recovered-user', + expect.objectContaining({ path: localRepo.path, addedAt: localRepo.addedAt }) + ) + for (const [, options] of gitExecFileAsyncMock.mock.calls) { + expect(options).toMatchObject({ cwd: localRepo.path, timeout: 5000 }) + } + }) +}) diff --git a/src/main/repo-git-username-enrichment.test.ts b/src/main/repo-git-username-enrichment.test.ts index bcecb8a2340..e5c897991ea 100644 --- a/src/main/repo-git-username-enrichment.test.ts +++ b/src/main/repo-git-username-enrichment.test.ts @@ -1,16 +1,24 @@ -import { beforeEach, describe, expect, it, vi } from 'vitest' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import type { Repo } from '../shared/types' -import type { ResolvedGitUsername } from './git/git-username' +import { + LOCAL_GIT_USERNAME_TIMEOUT_RETRY_MS, + type ResolvedGitUsername +} from './git/git-username' +import type * as GitUsernameModule from './git/git-username' const resolveLocalGitUsernameDetailedMock = vi.hoisted(() => vi.fn()) -vi.mock('./git/git-username', () => ({ +vi.mock('./git/git-username', async (importOriginal) => ({ + ...(await importOriginal()), resolveLocalGitUsernameDetailed: resolveLocalGitUsernameDetailedMock })) import { + MAX_REPO_GIT_USERNAME_ATTEMPTED_LOCATIONS, enrichRepoGitUsernames, flushRepoGitUsernameEnrichmentForTests, + getRepoGitUsernameAttemptedLocationCountForTests, + hasRepoGitUsernameAttemptedLocationForTests, resetRepoGitUsernameEnrichmentForTests } from './repo-git-username-enrichment' @@ -27,14 +35,39 @@ function makeRepo(overrides: Partial = {}): Repo { function makeStore(repos: Repo[]): { getRepos: () => Repo[] - setResolvedRepoGitUsername: ReturnType boolean>> + getRepo: (id: string) => Repo | undefined + setResolvedRepoGitUsername: ReturnType< + typeof vi.fn<(id: string, username: string, expectedLifecycle?: Repo) => boolean> + > } { return { getRepos: () => repos, - setResolvedRepoGitUsername: vi.fn<(id: string, username: string) => boolean>(() => true) + getRepo: (id) => repos.find((repo) => repo.id === id), + setResolvedRepoGitUsername: vi.fn((id: string, _username: string, expectedLifecycle?: Repo) => + repos.some( + (repo) => + repo.id === id && + (!expectedLifecycle || + (repo.path === expectedLifecycle.path && + repo.addedAt === expectedLifecycle.addedAt && + (repo.connectionId ?? null) === (expectedLifecycle.connectionId ?? null) && + (repo.executionHostId ?? null) === (expectedLifecycle.executionHostId ?? null))) + ) + ) } } +function deferred(): { + promise: Promise + resolve: (value: T) => void +} { + let resolve!: (value: T) => void + const promise = new Promise((resolvePromise) => { + resolve = resolvePromise + }) + return { promise, resolve } +} + function resolved(username: string, authoritative = true): ResolvedGitUsername { return { username, authoritative } } @@ -46,6 +79,10 @@ describe('enrichRepoGitUsernames', () => { resolveLocalGitUsernameDetailedMock.mockResolvedValue(resolved('demo-user')) }) + afterEach(() => { + vi.useRealTimers() + }) + it('resolves and persists usernames, then notifies once', async () => { const store = makeStore([makeRepo(), makeRepo({ id: 'r2', path: 'C:/repos/two' })]) const onChanged = vi.fn() @@ -54,15 +91,28 @@ describe('enrichRepoGitUsernames', () => { await flushRepoGitUsernameEnrichmentForTests() expect(resolveLocalGitUsernameDetailedMock).toHaveBeenCalledTimes(2) - expect(store.setResolvedRepoGitUsername).toHaveBeenCalledWith('r1', 'demo-user') - expect(store.setResolvedRepoGitUsername).toHaveBeenCalledWith('r2', 'demo-user') + expect(store.setResolvedRepoGitUsername).toHaveBeenCalledWith( + 'r1', + 'demo-user', + expect.objectContaining({ id: 'r1' }) + ) + expect(store.setResolvedRepoGitUsername).toHaveBeenCalledWith( + 'r2', + 'demo-user', + expect.objectContaining({ id: 'r2' }) + ) expect(onChanged).toHaveBeenCalledTimes(1) }) - it('skips folder and SSH repos', async () => { + it('skips folder, SSH, and runtime-host repos', async () => { const store = makeStore([ makeRepo({ id: 'folder', kind: 'folder' }), - makeRepo({ id: 'ssh', path: '/remote/repo', connectionId: 'conn-1' }) + makeRepo({ id: 'ssh', path: '/remote/repo', connectionId: 'conn-1' }), + makeRepo({ + id: 'runtime', + path: '/runtime/repo', + executionHostId: 'runtime:environment-1' + }) ]) enrichRepoGitUsernames(store) @@ -71,6 +121,26 @@ describe('enrichRepoGitUsernames', () => { expect(resolveLocalGitUsernameDetailedMock).not.toHaveBeenCalled() }) + it('targets the local lifecycle when an SSH repo shares its ID', async () => { + const sshRepo = makeRepo({ + id: 'shared', + path: 'C:/repos/same', + connectionId: 'ssh-target', + executionHostId: 'ssh:ssh-target' + }) + const localRepo = makeRepo({ id: 'shared', path: 'C:/repos/same', addedAt: 2 }) + const store = makeStore([sshRepo, localRepo]) + + enrichRepoGitUsernames(store) + await flushRepoGitUsernameEnrichmentForTests() + + expect(store.setResolvedRepoGitUsername).toHaveBeenCalledWith( + 'shared', + 'demo-user', + expect.objectContaining({ path: 'C:/repos/same', addedAt: 2 }) + ) + }) + it('probes each repo location at most once per session', async () => { const store = makeStore([makeRepo()]) @@ -82,16 +152,125 @@ describe('enrichRepoGitUsernames', () => { expect(resolveLocalGitUsernameDetailedMock).toHaveBeenCalledTimes(1) }) - it('keeps persisted usernames on a non-authoritative empty resolution', async () => { - resolveLocalGitUsernameDetailedMock.mockResolvedValue(resolved('', false)) + it('prunes attempted locations for removed repos', async () => { + const oldRepo = makeRepo({ id: 'old', path: 'C:/repos/old' }) + const newRepo = makeRepo({ id: 'new', path: 'C:/repos/new' }) + const repos = [oldRepo] + const store = makeStore(repos) + + enrichRepoGitUsernames(store) + await flushRepoGitUsernameEnrichmentForTests() + + expect(getRepoGitUsernameAttemptedLocationCountForTests()).toBe(1) + expect(hasRepoGitUsernameAttemptedLocationForTests(oldRepo)).toBe(true) + + repos.splice(0, repos.length, newRepo) + enrichRepoGitUsernames(store) + await flushRepoGitUsernameEnrichmentForTests() + + expect(getRepoGitUsernameAttemptedLocationCountForTests()).toBe(1) + expect(hasRepoGitUsernameAttemptedLocationForTests(oldRepo)).toBe(false) + expect(hasRepoGitUsernameAttemptedLocationForTests(newRepo)).toBe(true) + expect(resolveLocalGitUsernameDetailedMock).toHaveBeenCalledTimes(2) + }) + + it('caps attempted locations while retaining active recent repos', async () => { + const repos = Array.from( + { length: MAX_REPO_GIT_USERNAME_ATTEMPTED_LOCATIONS - 1 }, + (_, index) => makeRepo({ id: `repo-${index}`, path: `C:/repos/repo-${index}` }) + ) + const keepRepo = makeRepo({ id: 'keep', path: 'C:/repos/keep' }) + repos.push(keepRepo) + const store = makeStore(repos) + + enrichRepoGitUsernames(store) + await flushRepoGitUsernameEnrichmentForTests() + const newRepo = makeRepo({ id: 'new', path: 'C:/repos/new' }) + repos.push(newRepo) + enrichRepoGitUsernames(store) + await flushRepoGitUsernameEnrichmentForTests() + + expect(getRepoGitUsernameAttemptedLocationCountForTests()).toBe( + MAX_REPO_GIT_USERNAME_ATTEMPTED_LOCATIONS + ) + expect(hasRepoGitUsernameAttemptedLocationForTests(repos[0]!)).toBe(false) + expect(hasRepoGitUsernameAttemptedLocationForTests(keepRepo)).toBe(true) + expect(hasRepoGitUsernameAttemptedLocationForTests(newRepo)).toBe(true) + expect(resolveLocalGitUsernameDetailedMock).toHaveBeenCalledTimes( + MAX_REPO_GIT_USERNAME_ATTEMPTED_LOCATIONS + 1 + ) + }) + + it('defers the whole pipeline after a non-authoritative result, then retries', async () => { + vi.useFakeTimers() + resolveLocalGitUsernameDetailedMock + .mockResolvedValueOnce(resolved('', false)) + .mockResolvedValue(resolved('recovered-user')) const store = makeStore([makeRepo()]) const onChanged = vi.fn() + enrichRepoGitUsernames(store, { onChanged }) + await flushRepoGitUsernameEnrichmentForTests() enrichRepoGitUsernames(store, { onChanged }) await flushRepoGitUsernameEnrichmentForTests() + expect(resolveLocalGitUsernameDetailedMock).toHaveBeenCalledTimes(1) expect(store.setResolvedRepoGitUsername).not.toHaveBeenCalled() expect(onChanged).not.toHaveBeenCalled() + expect(hasRepoGitUsernameAttemptedLocationForTests(makeRepo())).toBe(true) + + await vi.advanceTimersByTimeAsync(LOCAL_GIT_USERNAME_TIMEOUT_RETRY_MS + 1) + enrichRepoGitUsernames(store, { onChanged }) + await flushRepoGitUsernameEnrichmentForTests() + + expect(resolveLocalGitUsernameDetailedMock).toHaveBeenCalledTimes(2) + expect(store.setResolvedRepoGitUsername).toHaveBeenCalledWith( + 'r1', + 'recovered-user', + expect.objectContaining({ id: 'r1' }) + ) + expect(onChanged).toHaveBeenCalledTimes(1) + + await vi.advanceTimersByTimeAsync(LOCAL_GIT_USERNAME_TIMEOUT_RETRY_MS + 1) + enrichRepoGitUsernames(store, { onChanged }) + await flushRepoGitUsernameEnrichmentForTests() + + // Authoritative success replaces the retry timestamp with a session attempt. + expect(resolveLocalGitUsernameDetailedMock).toHaveBeenCalledTimes(2) + }) + + it('scopes non-authoritative cooldowns to the exact repo lifecycle', async () => { + resolveLocalGitUsernameDetailedMock + .mockResolvedValueOnce(resolved('', false)) + .mockResolvedValue(resolved('replacement-user')) + const repos = [makeRepo()] + const store = makeStore(repos) + + enrichRepoGitUsernames(store) + await flushRepoGitUsernameEnrichmentForTests() + + const runtimeReplacement = makeRepo({ + addedAt: 2, + executionHostId: 'runtime:environment-1' + }) + repos.splice(0, repos.length, runtimeReplacement) + enrichRepoGitUsernames(store) + await flushRepoGitUsernameEnrichmentForTests() + + expect(resolveLocalGitUsernameDetailedMock).toHaveBeenCalledTimes(1) + expect(hasRepoGitUsernameAttemptedLocationForTests(makeRepo())).toBe(false) + + const localReplacement = makeRepo({ addedAt: 3 }) + repos.splice(0, repos.length, localReplacement) + enrichRepoGitUsernames(store) + await flushRepoGitUsernameEnrichmentForTests() + + expect(resolveLocalGitUsernameDetailedMock).toHaveBeenCalledTimes(2) + expect(store.setResolvedRepoGitUsername).toHaveBeenCalledWith( + 'r1', + 'replacement-user', + expect.objectContaining({ addedAt: 3 }) + ) }) it('clears stale persisted usernames on an authoritative empty resolution', async () => { @@ -104,7 +283,11 @@ describe('enrichRepoGitUsernames', () => { enrichRepoGitUsernames(store, { onChanged }) await flushRepoGitUsernameEnrichmentForTests() - expect(store.setResolvedRepoGitUsername).toHaveBeenCalledWith('r1', '') + expect(store.setResolvedRepoGitUsername).toHaveBeenCalledWith( + 'r1', + '', + expect.objectContaining({ id: 'r1' }) + ) expect(onChanged).toHaveBeenCalledTimes(1) }) @@ -138,6 +321,110 @@ describe('enrichRepoGitUsernames', () => { await flushRepoGitUsernameEnrichmentForTests() expect(resolveLocalGitUsernameDetailedMock).toHaveBeenCalledTimes(2) - expect(store.setResolvedRepoGitUsername).toHaveBeenCalledWith('r2', 'demo-user') + expect(store.setResolvedRepoGitUsername).toHaveBeenCalledWith( + 'r2', + 'demo-user', + expect.objectContaining({ id: 'r2' }) + ) + }) + + it('rejects a stale username after repo ID and path reuse, then retries the replacement', async () => { + const oldProbe = deferred() + resolveLocalGitUsernameDetailedMock.mockReturnValueOnce(oldProbe.promise) + const repos = [makeRepo()] + const store = makeStore(repos) + + enrichRepoGitUsernames(store) + const replacement = makeRepo({ addedAt: 2 }) + repos.splice(0, repos.length, replacement) + oldProbe.resolve(resolved('old-user')) + await flushRepoGitUsernameEnrichmentForTests() + + expect(store.setResolvedRepoGitUsername).not.toHaveBeenCalled() + expect(hasRepoGitUsernameAttemptedLocationForTests(replacement)).toBe(false) + + enrichRepoGitUsernames(store) + await flushRepoGitUsernameEnrichmentForTests() + + expect(resolveLocalGitUsernameDetailedMock).toHaveBeenCalledTimes(2) + expect(store.setResolvedRepoGitUsername).toHaveBeenCalledWith( + 'r1', + 'demo-user', + expect.objectContaining({ id: 'r1' }) + ) + }) + + it('does not let a reset-era result mutate the new generation', async () => { + const staleProbe = deferred() + resolveLocalGitUsernameDetailedMock.mockReturnValueOnce(staleProbe.promise) + const repo = makeRepo() + const store = makeStore([repo]) + + enrichRepoGitUsernames(store) + resetRepoGitUsernameEnrichmentForTests() + enrichRepoGitUsernames(store) + await flushRepoGitUsernameEnrichmentForTests() + + expect(store.setResolvedRepoGitUsername).toHaveBeenCalledTimes(1) + expect(store.setResolvedRepoGitUsername).toHaveBeenLastCalledWith( + 'r1', + 'demo-user', + expect.objectContaining({ id: 'r1' }) + ) + staleProbe.resolve(resolved('stale-user')) + await Promise.resolve() + await Promise.resolve() + + expect(store.setResolvedRepoGitUsername).toHaveBeenCalledTimes(1) + expect(hasRepoGitUsernameAttemptedLocationForTests(repo)).toBe(true) + }) + + it('does not probe or mutate a runtime-host replacement after reset', async () => { + const staleProbe = deferred() + resolveLocalGitUsernameDetailedMock.mockReturnValueOnce(staleProbe.promise) + const repos = [makeRepo()] + const store = makeStore(repos) + + enrichRepoGitUsernames(store) + resetRepoGitUsernameEnrichmentForTests() + const replacement = makeRepo({ addedAt: 2, executionHostId: 'runtime:environment-1' }) + repos.splice(0, repos.length, replacement) + enrichRepoGitUsernames(store) + await flushRepoGitUsernameEnrichmentForTests() + staleProbe.resolve(resolved('stale-user')) + await Promise.resolve() + await Promise.resolve() + + expect(resolveLocalGitUsernameDetailedMock).toHaveBeenCalledTimes(1) + expect(store.setResolvedRepoGitUsername).not.toHaveBeenCalled() + expect(hasRepoGitUsernameAttemptedLocationForTests(replacement)).toBe(false) + }) + + it('uses the latest pending store and callback for the queued rerun', async () => { + const firstProbe = deferred() + resolveLocalGitUsernameDetailedMock.mockReturnValueOnce(firstProbe.promise) + const firstStore = makeStore([makeRepo({ id: 'first', path: 'C:/repos/first' })]) + const droppedStore = makeStore([makeRepo({ id: 'dropped', path: 'C:/repos/dropped' })]) + const latestStore = makeStore([makeRepo({ id: 'latest', path: 'C:/repos/latest' })]) + const firstChanged = vi.fn() + const droppedChanged = vi.fn() + const latestChanged = vi.fn() + + enrichRepoGitUsernames(firstStore, { onChanged: firstChanged }) + enrichRepoGitUsernames(droppedStore, { onChanged: droppedChanged }) + enrichRepoGitUsernames(latestStore, { onChanged: latestChanged }) + firstProbe.resolve(resolved('first-user')) + await flushRepoGitUsernameEnrichmentForTests() + + expect(resolveLocalGitUsernameDetailedMock).toHaveBeenCalledTimes(2) + expect(droppedStore.setResolvedRepoGitUsername).not.toHaveBeenCalled() + expect(latestStore.setResolvedRepoGitUsername).toHaveBeenCalledWith( + 'latest', + 'demo-user', + expect.objectContaining({ id: 'latest' }) + ) + expect(firstChanged).toHaveBeenCalledTimes(1) + expect(droppedChanged).not.toHaveBeenCalled() + expect(latestChanged).toHaveBeenCalledTimes(1) }) }) diff --git a/src/main/repo-git-username-enrichment.ts b/src/main/repo-git-username-enrichment.ts index c5ae5c6d7b9..eb7726889a2 100644 --- a/src/main/repo-git-username-enrichment.ts +++ b/src/main/repo-git-username-enrichment.ts @@ -1,58 +1,214 @@ import type { Repo } from '../shared/types' -import { resolveLocalGitUsernameDetailed } from './git/git-username' +import { getRepoExecutionHostId, LOCAL_EXECUTION_HOST_ID } from '../shared/execution-host' +import { + LOCAL_GIT_USERNAME_TIMEOUT_RETRY_MS, + resolveLocalGitUsernameDetailed +} from './git/git-username' + +export const MAX_REPO_GIT_USERNAME_ATTEMPTED_LOCATIONS = 2048 type RepoUsernameStore = { getRepos(): Repo[] - setResolvedRepoGitUsername(id: string, username: string): boolean + setResolvedRepoGitUsername( + id: string, + username: string, + expectedLifecycle?: RepoLifecycle + ): boolean } +type RepoLifecycle = Pick + type EnrichmentOptions = { onChanged?: () => void } -// Why: resolution spawns git (and possibly gh) subprocesses, so run it at most -// once per repo location per app session — hydrateRepo serves the persisted -// value in between, and a relaunch picks up config changes. -const attemptedLocations = new Set() -let enrichmentInFlight: Promise | null = null -let rerunRequested = false +type EnrichmentRequest = { + store: RepoUsernameStore + options: EnrichmentOptions +} -function getRepoLocationKey(repo: Pick): string { - return `${repo.connectionId ?? 'local'}\0${repo.path}` +type InFlightEnrichment = { + generation: number + promise: Promise +} + +// null means this lifecycle completed authoritatively for the session; a +// timestamp keeps the whole git+gh pipeline quiet until a timed-out retry. +const attemptedLifecycles = new Map() +const storeIds = new WeakMap() +let nextStoreId = 1 +let generation = 0 +let enrichmentInFlight: InFlightEnrichment | null = null +let pendingRequest: EnrichmentRequest | null = null + +function getStoreId(store: RepoUsernameStore): number { + const existing = storeIds.get(store) + if (existing !== undefined) { + return existing + } + const id = nextStoreId++ + storeIds.set(store, id) + return id +} + +function getRepoLifecycleKey(store: RepoUsernameStore, repo: Repo): string { + return [ + getStoreId(store), + repo.executionHostId ?? 'default-host', + repo.connectionId ?? 'local', + repo.path, + repo.id, + repo.addedAt + ].join('\0') +} + +function getStoreKeyPrefix(store: RepoUsernameStore): string { + return `${getStoreId(store)}\0` +} + +function pruneAttemptedLifecycles( + store: RepoUsernameStore, + liveLifecycleKeys: ReadonlySet +): void { + const storeKeyPrefix = getStoreKeyPrefix(store) + for (const lifecycleKey of attemptedLifecycles.keys()) { + if (lifecycleKey.startsWith(storeKeyPrefix) && !liveLifecycleKeys.has(lifecycleKey)) { + attemptedLifecycles.delete(lifecycleKey) + } + } +} + +function rememberAttemptedLifecycle(lifecycleKey: string, retryAt: number | null = null): void { + attemptedLifecycles.delete(lifecycleKey) + attemptedLifecycles.set(lifecycleKey, retryAt) + while (attemptedLifecycles.size > MAX_REPO_GIT_USERNAME_ATTEMPTED_LOCATIONS) { + const oldestLifecycleKey = attemptedLifecycles.keys().next().value + if (oldestLifecycleKey === undefined) { + break + } + attemptedLifecycles.delete(oldestLifecycleKey) + } +} + +function isLifecycleAttemptDeferred(lifecycleKey: string, now: number): boolean { + const retryAt = attemptedLifecycles.get(lifecycleKey) + if (retryAt === undefined) { + return false + } + if (retryAt === null || retryAt > now) { + return true + } + attemptedLifecycles.delete(lifecycleKey) + return false +} + +function hasSameRepoLifecycle(snapshot: RepoLifecycle, current: Repo): boolean { + return ( + current.id === snapshot.id && + current.addedAt === snapshot.addedAt && + current.path === snapshot.path && + (current.connectionId ?? null) === (snapshot.connectionId ?? null) && + (current.executionHostId ?? null) === (snapshot.executionHostId ?? null) + ) +} + +function getCurrentRepo(store: RepoUsernameStore, snapshot: RepoLifecycle): Repo | undefined { + return store.getRepos().find((repo) => hasSameRepoLifecycle(snapshot, repo)) +} + +function supportsLocalGitUsernameProbe(repo: Repo): boolean { + // Why: runtime and SSH paths belong to another execution host; local git/gh + // must never resolve or clear credentials for those repos. + return repo.kind !== 'folder' && getRepoExecutionHostId(repo) === LOCAL_EXECUTION_HOST_ID +} + +function isSameLocalGitRepo(snapshot: Repo, current: Repo | undefined): boolean { + return ( + !!current && + hasSameRepoLifecycle(snapshot, current) && + supportsLocalGitUsernameProbe(current) && + current.kind !== 'folder' + ) } async function enrichRepoGitUsernamesInBackground( store: RepoUsernameStore, - options: EnrichmentOptions + options: EnrichmentOptions, + taskGeneration: number ): Promise { - const candidates = store.getRepos().filter( - (repo) => - repo.kind !== 'folder' && - // Why: SSH repo paths are remote; local git cannot inspect them. The - // SSH username path (getSshGitUsername) stays caller-driven. - !repo.connectionId && - !attemptedLocations.has(getRepoLocationKey(repo)) + const repos = store.getRepos() + // Why: SSH repo paths are remote; local git cannot inspect them. The SSH + // username path (getSshGitUsername) stays caller-driven. + const localGitRepos = repos.filter(supportsLocalGitUsernameProbe) + const liveLifecycleKeys = new Set(localGitRepos.map((repo) => getRepoLifecycleKey(store, repo))) + pruneAttemptedLifecycles(store, liveLifecycleKeys) + const now = Date.now() + const candidates = localGitRepos.filter( + (repo) => !isLifecycleAttemptDeferred(getRepoLifecycleKey(store, repo), now) ) let changed = false - for (const repo of candidates) { - attemptedLocations.add(getRepoLocationKey(repo)) + for (const candidate of candidates) { + if (generation !== taskGeneration) { + return + } + // Why: a background subprocess can outlive removal, rename, or ID reuse; + // snapshot the durable lifecycle fields before awaiting it. + const repo = { ...candidate } + const lifecycleKey = getRepoLifecycleKey(store, repo) + rememberAttemptedLifecycle(lifecycleKey) const { username, authoritative } = await resolveLocalGitUsernameDetailed(repo.path) - // Why: a non-authoritative '' means a probe timed out and says nothing - // about the account — keep the persisted value. An authoritative result - // (including '') is the current truth: it must also CLEAR a stale - // persisted username after the user removes github.user or logs out. - if (!authoritative && !username) { + if (generation !== taskGeneration) { + return + } + if (!isSameLocalGitRepo(repo, getCurrentRepo(store, repo))) { + // Why: the replacement lifecycle must be allowed to schedule its own probe. + attemptedLifecycles.delete(lifecycleKey) continue } - if (store.setResolvedRepoGitUsername(repo.id, username)) { + // Why: a non-authoritative '' means a probe timed out and says nothing + // about the account. An authoritative result (including '') is current truth. + if (!authoritative && !username) { + // Why: the resolver's gh cooldown alone still allows all earlier git + // probes to repeat on every repos:list; defer the entire lifecycle pipeline. + rememberAttemptedLifecycle(lifecycleKey, Date.now() + LOCAL_GIT_USERNAME_TIMEOUT_RETRY_MS) + continue + } + if (store.setResolvedRepoGitUsername(repo.id, username, repo)) { changed = true } } - if (changed) { + if (changed && generation === taskGeneration) { options.onChanged?.() } } +function startEnrichment(request: EnrichmentRequest, taskGeneration: number): void { + const inFlight: InFlightEnrichment = { + generation: taskGeneration, + promise: Promise.resolve() + } + inFlight.promise = enrichRepoGitUsernamesInBackground( + request.store, + request.options, + taskGeneration + ) + .catch((error: unknown) => { + console.error('[repo-username] Failed to enrich git usernames:', error) + }) + .finally(() => { + if (generation !== taskGeneration || enrichmentInFlight !== inFlight) { + return + } + enrichmentInFlight = null + const nextRequest = pendingRequest + pendingRequest = null + if (nextRequest) { + startEnrichment(nextRequest, taskGeneration) + } + }) + enrichmentInFlight = inFlight +} + /** * Resolve git usernames for repos that haven't been probed this session, off * the caller's critical path. Fire-and-forget by design: repos:list must stay @@ -62,34 +218,35 @@ export function enrichRepoGitUsernames( store: RepoUsernameStore, options: EnrichmentOptions = {} ): void { - if (enrichmentInFlight) { - // Why: a repo added mid-pass would otherwise be dropped until some later - // repos:list happens to fire — queue one follow-up pass instead. - rerunRequested = true + const request = { store, options } + if (enrichmentInFlight?.generation === generation) { + // Why: only one follow-up pass is useful; retain its newest store/callback + // because that request reflects the latest caller state. + pendingRequest = request return } - enrichmentInFlight = enrichRepoGitUsernamesInBackground(store, options) - .catch((error: unknown) => { - console.error('[repo-username] Failed to enrich git usernames:', error) - }) - .finally(() => { - enrichmentInFlight = null - if (rerunRequested) { - rerunRequested = false - enrichRepoGitUsernames(store, options) - } - }) + startEnrichment(request, generation) } export async function flushRepoGitUsernameEnrichmentForTests(): Promise { - // A queued rerun replaces enrichmentInFlight when the first pass settles. - while (enrichmentInFlight) { - await enrichmentInFlight + const flushGeneration = generation + while (enrichmentInFlight?.generation === flushGeneration) { + await enrichmentInFlight.promise } } export function resetRepoGitUsernameEnrichmentForTests(): void { - attemptedLocations.clear() + generation++ + attemptedLifecycles.clear() enrichmentInFlight = null - rerunRequested = false + pendingRequest = null +} + +export function getRepoGitUsernameAttemptedLocationCountForTests(): number { + return attemptedLifecycles.size +} + +export function hasRepoGitUsernameAttemptedLocationForTests(repo: Repo): boolean { + const suffix = `\0${repo.path}\0${repo.id}\0${repo.addedAt}` + return [...attemptedLifecycles.keys()].some((key) => key.endsWith(suffix)) } diff --git a/src/main/runtime/orca-runtime-repo-enrichment-authority.test.ts b/src/main/runtime/orca-runtime-repo-enrichment-authority.test.ts new file mode 100644 index 00000000000..a56460d27b3 --- /dev/null +++ b/src/main/runtime/orca-runtime-repo-enrichment-authority.test.ts @@ -0,0 +1,48 @@ +import { describe, expect, it, vi } from 'vitest' +import { OrcaRuntimeService } from './orca-runtime' + +const enrichMissingRepoGitRemoteIdentitiesMock = vi.hoisted(() => vi.fn()) + +vi.mock('../repo-git-remote-identity-enrichment', () => ({ + enrichMissingRepoGitRemoteIdentities: enrichMissingRepoGitRemoteIdentitiesMock +})) + +vi.mock('electron', () => ({ + BrowserWindow: { fromId: vi.fn(() => null) }, + webContents: { fromId: vi.fn(() => null) }, + ipcMain: { + on: vi.fn(), + removeListener: vi.fn() + }, + app: { getPath: vi.fn(() => '/tmp') } +})) + +const store = { + getRepos: vi.fn(() => []) +} + +describe('OrcaRuntimeService repo enrichment authority', () => { + it('does not grant runtime-path authority to the desktop-hosted runtime service', () => { + const desktopRuntime = new OrcaRuntimeService(store as never) + + desktopRuntime.enrichMissingRepoGitRemoteIdentities() + + expect(enrichMissingRepoGitRemoteIdentitiesMock).toHaveBeenCalledWith( + store, + expect.objectContaining({ probeRuntimeHostPaths: false }) + ) + }) + + it('grants runtime-path authority only to the serving runtime process', () => { + const servingRuntime = new OrcaRuntimeService(store as never, undefined, { + ownsPersistedRuntimeHostPaths: true + }) + + servingRuntime.enrichMissingRepoGitRemoteIdentities() + + expect(enrichMissingRepoGitRemoteIdentitiesMock).toHaveBeenCalledWith( + store, + expect.objectContaining({ probeRuntimeHostPaths: true }) + ) + }) +}) diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index 84f1954d194..cb92d51297d 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -2489,6 +2489,7 @@ export class OrcaRuntimeService { private readonly onTerminalSideEffects: ((batch: TerminalSideEffectBatch) => void) | null private readonly getAgentStatusSnapshotFn: (() => AgentStatusIpcPayload[]) | null private readonly buildAgentHookPtyEnv: (() => Record) | null + private readonly ownsPersistedRuntimeHostPaths: boolean private accountServices: RuntimeAccountServices | null = null private commitMessageAgentEnv: CommitMessageAgentEnvironmentResolvers | null = null private automationService: AutomationService | null = null @@ -2522,9 +2523,13 @@ export class OrcaRuntimeService { // managed-Codex sessions. The runtime ctor runs in BOTH window and serve. getAdditionalAiVaultCodexHomePaths?: () => readonly string[] buildAgentHookPtyEnv?: () => Record + // Why: only headless serve owns runtime-stamped filesystem paths; the + // desktop runtime service shares a Store containing other runtime hosts. + ownsPersistedRuntimeHostPaths?: boolean } ) { this.store = store + this.ownsPersistedRuntimeHostPaths = deps?.ownsPersistedRuntimeHostPaths === true if (stats) { this.stats = stats this.agentDetector = new AgentDetector(stats) @@ -11201,6 +11206,10 @@ export class OrcaRuntimeService { return } enrichMissingRepoGitRemoteIdentities(this.store, { + // Why: class identity is not authority; desktop also constructs this + // service, while only headless serve owns persisted runtime-host paths. + probeRuntimeHostPaths: this.ownsPersistedRuntimeHostPaths, + notificationChannel: 'runtime', onChanged: () => { this.invalidateResolvedWorktreeCache() this.notifyReposChanged()