From acb35ee649c055fedae4d240323a0a9ee2129ae2 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sat, 11 Jul 2026 01:30:19 -0700 Subject: [PATCH] Fix git submodule path cache retention (#7687) --- src/main/git/checkout.ts | 5 +- src/main/git/remote.ts | 35 +-- src/main/git/remove-worktree.test.ts | 3 +- .../git/status-submodule-path-cache.test.ts | 231 ++++++++++++++++++ src/main/git/status.ts | 118 +++++++-- src/main/git/worktree.ts | 28 ++- src/main/ipc/repos-remote.test.ts | 53 +++- src/main/ipc/repos.ts | 3 +- src/main/providers/ssh-git-provider.test.ts | 36 +++ src/main/providers/ssh-git-provider.ts | 13 +- src/main/runtime/orca-runtime.test.ts | 41 ++++ src/main/runtime/orca-runtime.ts | 15 +- ...ndler-submodule-cache-invalidation.test.ts | 74 ++++++ src/relay/git-handler-submodule-ops.test.ts | 76 ++++++ src/relay/git-handler-submodule-ops.ts | 76 +++++- src/relay/git-handler.test.ts | 4 + src/relay/git-handler.ts | 101 ++++---- src/shared/git-exec-mutation.ts | 7 + 18 files changed, 807 insertions(+), 112 deletions(-) create mode 100644 src/main/git/status-submodule-path-cache.test.ts create mode 100644 src/relay/git-handler-submodule-cache-invalidation.test.ts create mode 100644 src/shared/git-exec-mutation.ts diff --git a/src/main/git/checkout.ts b/src/main/git/checkout.ts index 5e8152af8..a7b610f93 100644 --- a/src/main/git/checkout.ts +++ b/src/main/git/checkout.ts @@ -1,6 +1,7 @@ import type { GitRuntimeOptions } from './git-runtime-options' import { gitOptionsForWorktree } from './git-runtime-options' import { gitExecFileAsync } from './runner' +import { runWithGitReadCacheInvalidation } from './status' /** * Reject branch names git would parse as an option (`-`/`--…`) or that aren't a @@ -27,7 +28,9 @@ export async function checkoutBranch( options: GitRuntimeOptions = {} ): Promise { assertValidBranchName(branch) - await gitExecFileAsync(['checkout', branch, '--'], gitOptionsForWorktree(worktreePath, options)) + await runWithGitReadCacheInvalidation(() => + gitExecFileAsync(['checkout', branch, '--'], gitOptionsForWorktree(worktreePath, options)) + ) } /** diff --git a/src/main/git/remote.ts b/src/main/git/remote.ts index d2b1c0b5d..ab7fba775 100644 --- a/src/main/git/remote.ts +++ b/src/main/git/remote.ts @@ -7,6 +7,7 @@ import type { GitRuntimeOptions } from './git-runtime-options' import { gitOptionsForWorktree } from './git-runtime-options' import { validateGitPushTarget } from './push-target-validation' import { gitExecFileAsync } from './runner' +import { runWithGitReadCacheInvalidation } from './status' async function getConfiguredPushTarget( worktreePath: string, @@ -249,7 +250,9 @@ export async function gitPull( // Why: plain `git pull` uses the user's configured pull strategy (merge by // default) so diverged branches reconcile instead of erroring out. Conflicts // surface through the existing conflict-resolution flow. - await gitPullWithArgs(worktreePath, [], pushTarget, options) + await runWithGitReadCacheInvalidation(() => + gitPullWithArgs(worktreePath, [], pushTarget, options) + ) } export async function gitFastForward( @@ -257,7 +260,9 @@ export async function gitFastForward( pushTarget?: GitPushTarget, options: GitRuntimeOptions = {} ): Promise { - await gitPullWithArgs(worktreePath, ['--ff-only'], pushTarget, options) + await runWithGitReadCacheInvalidation(() => + gitPullWithArgs(worktreePath, ['--ff-only'], pushTarget, options) + ) } export async function gitPullRebaseFromBase( @@ -265,18 +270,20 @@ export async function gitPullRebaseFromBase( baseRef: string, options: GitRuntimeOptions = {} ): Promise { - try { - const source = await resolveGitRemoteRebaseSource( - (args) => gitExecFileAsync(args, gitOptionsForWorktree(worktreePath, options)), - baseRef - ) - await gitExecFileAsync( - ['pull', '--rebase', source.remoteName, source.branchName], - gitOptionsForWorktree(worktreePath, options) - ) - } catch (error) { - throw new Error(normalizeGitErrorMessage(error, 'pull')) - } + await runWithGitReadCacheInvalidation(async () => { + try { + const source = await resolveGitRemoteRebaseSource( + (args) => gitExecFileAsync(args, gitOptionsForWorktree(worktreePath, options)), + baseRef + ) + await gitExecFileAsync( + ['pull', '--rebase', source.remoteName, source.branchName], + gitOptionsForWorktree(worktreePath, options) + ) + } catch (error) { + throw new Error(normalizeGitErrorMessage(error, 'pull')) + } + }) } export async function gitFetch( diff --git a/src/main/git/remove-worktree.test.ts b/src/main/git/remove-worktree.test.ts index 5c3a07693..4330a3be9 100644 --- a/src/main/git/remove-worktree.test.ts +++ b/src/main/git/remove-worktree.test.ts @@ -24,7 +24,8 @@ vi.mock('./runner', () => ({ })) vi.mock('./status', () => ({ - resolveGitDir: resolveGitDirMock + resolveGitDir: resolveGitDirMock, + runWithGitReadCacheInvalidation: (run: () => Promise) => run() })) vi.mock('fs/promises', async () => { diff --git a/src/main/git/status-submodule-path-cache.test.ts b/src/main/git/status-submodule-path-cache.test.ts new file mode 100644 index 000000000..18b4ba693 --- /dev/null +++ b/src/main/git/status-submodule-path-cache.test.ts @@ -0,0 +1,231 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +const { gitExecFileAsyncMock, gitExecFileAsyncBufferMock, gitStreamStdoutMock } = vi.hoisted( + () => ({ + gitExecFileAsyncMock: vi.fn(), + gitExecFileAsyncBufferMock: vi.fn(), + gitStreamStdoutMock: vi.fn() + }) +) + +vi.mock('./runner', () => ({ + gitExecFileAsync: gitExecFileAsyncMock, + gitExecFileAsyncBuffer: gitExecFileAsyncBufferMock, + gitStreamStdout: gitStreamStdoutMock, + gitOptionalLocksDisabledEnv: (env: NodeJS.ProcessEnv = process.env) => ({ + ...env, + GIT_OPTIONAL_LOCKS: '0' + }) +})) + +import { + MAX_SUBMODULE_PATHS_CACHE_ENTRIES, + abortMerge, + abortRebase, + clearSubmodulePathsCacheForTests, + getSubmodulePathsCacheCountForTests, + listSubmodulePaths +} from './status' +import { checkoutBranch } from './checkout' +import { gitPull, gitPullRebaseFromBase } from './remote' +import { addWorktree, removeWorktree } from './worktree' + +describe('submodule path cache', () => { + beforeEach(() => { + clearSubmodulePathsCacheForTests() + gitExecFileAsyncMock.mockReset() + gitExecFileAsyncBufferMock.mockReset() + gitStreamStdoutMock.mockReset() + gitExecFileAsyncMock.mockImplementation((_args: string[], options?: { cwd?: string }) => + Promise.resolve({ + stdout: `submodule.lib.path ${String(options?.cwd ?? 'repo').replace(/^.*[/\\\\]/, '')}-lib\n` + }) + ) + }) + + afterEach(() => { + vi.useRealTimers() + clearSubmodulePathsCacheForTests() + }) + + it('prunes expired entries even when later reads use different worktrees', async () => { + vi.useFakeTimers() + vi.setSystemTime(0) + + await listSubmodulePaths('/repo-a') + await listSubmodulePaths('/repo-b') + + expect(getSubmodulePathsCacheCountForTests()).toBe(2) + + vi.setSystemTime(5_001) + await expect(listSubmodulePaths('/repo-c')).resolves.toEqual(['repo-c-lib']) + + expect(getSubmodulePathsCacheCountForTests()).toBe(1) + expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(3) + }) + + it('stays bounded through prolonged worktree churn and keeps recently reused worktrees', async () => { + vi.useFakeTimers() + vi.setSystemTime(0) + + for (let wave = 0; wave < 4; wave += 1) { + for (let i = 0; i < MAX_SUBMODULE_PATHS_CACHE_ENTRIES; i += 1) { + await listSubmodulePaths(`/wave-${wave}-repo-${i}`) + } + expect(getSubmodulePathsCacheCountForTests()).toBe(MAX_SUBMODULE_PATHS_CACHE_ENTRIES) + vi.advanceTimersByTime(5_001) + } + + await expect(listSubmodulePaths('/retained-repo')).resolves.toEqual(['retained-repo-lib']) + for (let i = 0; i < MAX_SUBMODULE_PATHS_CACHE_ENTRIES - 1; i += 1) { + await listSubmodulePaths(`/final-repo-${i}`) + } + await expect(listSubmodulePaths('/retained-repo')).resolves.toEqual(['retained-repo-lib']) + await listSubmodulePaths(`/final-repo-${MAX_SUBMODULE_PATHS_CACHE_ENTRIES - 1}`) + await listSubmodulePaths('/overflow-repo') + + expect(getSubmodulePathsCacheCountForTests()).toBe(MAX_SUBMODULE_PATHS_CACHE_ENTRIES) + + const callsBeforeRetainedRead = gitExecFileAsyncMock.mock.calls.length + await expect(listSubmodulePaths('/retained-repo')).resolves.toEqual(['retained-repo-lib']) + expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(callsBeforeRetainedRead) + + await expect(listSubmodulePaths('/final-repo-0')).resolves.toEqual(['final-repo-0-lib']) + expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(callsBeforeRetainedRead + 1) + }) + + it('does not let a pre-invalidation read repopulate the cache', async () => { + let resolveOldRead: ((value: { stdout: string }) => void) | undefined + gitExecFileAsyncMock.mockImplementationOnce( + () => + new Promise<{ stdout: string }>((resolve) => { + resolveOldRead = resolve + }) + ) + + const oldRead = listSubmodulePaths('/repo') + expect(resolveOldRead).toBeTypeOf('function') + clearSubmodulePathsCacheForTests() + resolveOldRead?.({ stdout: 'submodule.lib.path old-lib\n' }) + + await expect(oldRead).resolves.toEqual(['old-lib']) + expect(getSubmodulePathsCacheCountForTests()).toBe(0) + await expect(listSubmodulePaths('/repo')).resolves.toEqual(['repo-lib']) + expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(2) + }) + + it('does not reuse another branch\'s submodule paths after local or WSL checkout', async () => { + let modulePath = 'main-lib' + gitExecFileAsyncMock.mockImplementation((args: string[]) => { + if (args[0] === 'checkout') { + modulePath = 'feature-lib' + return Promise.resolve({ stdout: '' }) + } + return Promise.resolve({ stdout: `submodule.lib.path ${modulePath}\n` }) + }) + const runtime = { wslDistro: 'Ubuntu' } + + await expect(listSubmodulePaths('/repo', runtime)).resolves.toEqual(['main-lib']) + await checkoutBranch('/repo', 'feature', runtime) + await expect(listSubmodulePaths('/repo', runtime)).resolves.toEqual(['feature-lib']) + + const configReads = gitExecFileAsyncMock.mock.calls.filter( + ([args]) => args[0] === 'config' && args.includes('.gitmodules') + ) + expect(configReads).toHaveLength(2) + expect(gitExecFileAsyncMock).toHaveBeenCalledWith(['checkout', 'feature', '--'], { + cwd: '/repo', + wslDistro: 'Ubuntu' + }) + }) + + it.each([ + ['pull', () => gitPull('/repo')], + ['rebase', () => gitPullRebaseFromBase('/repo', 'origin/main')] + ])('invalidates submodule paths around local %s', async (_name, mutate) => { + let modulePath = 'old-lib' + gitExecFileAsyncMock.mockImplementation((args: string[]) => { + if (args[0] === 'remote') { + return Promise.resolve({ stdout: 'origin\n' }) + } + if (args[0] === 'pull') { + modulePath = 'fresh-lib' + return Promise.resolve({ stdout: '' }) + } + if (args[0] === 'config' && args.includes('.gitmodules')) { + return Promise.resolve({ stdout: `submodule.lib.path ${modulePath}\n` }) + } + return Promise.resolve({ stdout: '' }) + }) + + await expect(listSubmodulePaths('/repo')).resolves.toEqual(['old-lib']) + await mutate() + await expect(listSubmodulePaths('/repo')).resolves.toEqual(['fresh-lib']) + + const configReads = gitExecFileAsyncMock.mock.calls.filter( + ([args]) => args[0] === 'config' && args.includes('.gitmodules') + ) + expect(configReads).toHaveLength(2) + }) + + it.each([ + ['merge', () => abortMerge('/repo', { wslDistro: 'Ubuntu' })], + ['rebase', () => abortRebase('/repo', { wslDistro: 'Ubuntu' })] + ])('drops a cached empty .gitmodules result when %s abort restores it', async (_name, abort) => { + let modulePath = '' + gitExecFileAsyncMock.mockImplementation((args: string[]) => { + if (args[1] === '--abort') { + modulePath = 'restored-lib' + return Promise.resolve({ stdout: '' }) + } + return Promise.resolve({ + stdout: modulePath ? `submodule.lib.path ${modulePath}\n` : '' + }) + }) + + await expect(listSubmodulePaths('/repo', { wslDistro: 'Ubuntu' })).resolves.toEqual([]) + await abort() + await expect(listSubmodulePaths('/repo', { wslDistro: 'Ubuntu' })).resolves.toEqual([ + 'restored-lib' + ]) + + const configReads = gitExecFileAsyncMock.mock.calls.filter( + ([args]) => args[0] === 'config' && args.includes('.gitmodules') + ) + expect(configReads).toHaveLength(2) + }) + + it('drops a same-path negative cache when a local or WSL worktree is recreated', async () => { + let recreated = false + gitExecFileAsyncMock.mockImplementation((args: string[]) => { + if (args[0] === 'worktree' && args[1] === 'add') { + recreated = true + } + if (args[0] === 'config' && args.includes('.gitmodules')) { + return Promise.resolve({ + stdout: recreated ? 'submodule.lib.path recreated-lib\n' : '' + }) + } + return Promise.resolve({ stdout: '' }) + }) + const runtime = { wslDistro: 'Ubuntu' } + + await expect(listSubmodulePaths('/repo-feature', runtime)).resolves.toEqual([]) + await removeWorktree('/repo', '/repo-feature', true, { + ...runtime, + knownRemovedWorktree: { branch: '', head: '', locked: false } + }) + await addWorktree('/repo', '/repo-feature', 'feature', undefined, false, false, { + ...runtime, + checkoutExistingBranch: true + }) + await expect(listSubmodulePaths('/repo-feature', runtime)).resolves.toEqual([ + 'recreated-lib' + ]) + + const configReads = gitExecFileAsyncMock.mock.calls.filter( + ([args]) => args[0] === 'config' && args.includes('.gitmodules') + ) + expect(configReads).toHaveLength(2) + }) +}) diff --git a/src/main/git/status.ts b/src/main/git/status.ts index 6ec73ef8a..ddbdd1b2c 100644 --- a/src/main/git/status.ts +++ b/src/main/git/status.ts @@ -64,8 +64,10 @@ type EffectiveUpstreamStatusCacheEntry = { } const SUBMODULE_PATHS_CACHE_TTL_MS = 5_000 +export const MAX_SUBMODULE_PATHS_CACHE_ENTRIES = 512 type SubmodulePathsCacheEntry = { paths: string[]; expiresAt: number } const submodulePathsCache = new Map() +let submodulePathsCacheGeneration = 0 // Why: the effective-upstream resolution chain (symbolic-ref + rev-parse ×2-3 // + config snapshot) costs 4-5 subprocess spawns and only changes when branch @@ -92,15 +94,37 @@ const statusReadsInFlight = new Map>() // Why: a mutation invalidates both in-flight diff reads and in-flight status // coalescing; clearing only the diff dedupe would let a post-mutation // getStatus() join a pre-mutation read and return stale entries. -function clearGitReadInvalidationState(): void { +export function invalidateGitReadCaches(): void { gitDiffReadDedupe.clear() statusReadsInFlight.clear() - submodulePathsCache.clear() + clearSubmodulePathsCache() resolvedUpstreamNameCache.clear() } +export async function runWithGitReadCacheInvalidation(run: () => Promise): Promise { + invalidateGitReadCaches() + try { + return await run() + } finally { + // Why: a read that started during the mutation can be stale too, so the + // post-mutation boundary retires both pre-existing and overlapping reads. + invalidateGitReadCaches() + } +} + export function clearSubmodulePathsCacheForTests(): void { + clearSubmodulePathsCache() +} + +function clearSubmodulePathsCache(): void { submodulePathsCache.clear() + // Why: a pre-mutation .gitmodules read must not repopulate the cache after + // the mutation invalidated it. + submodulePathsCacheGeneration += 1 +} + +export function getSubmodulePathsCacheCountForTests(): number { + return submodulePathsCache.size } function gitRuntimeOptionsKey(options: GitRuntimeOptions): readonly unknown[] { @@ -113,6 +137,44 @@ function getSubmodulePathsCacheKey(worktreePath: string, options: GitRuntimeOpti return [worktreePath, ...gitRuntimeOptionsKey(options)].join('\0') } +function pruneExpiredSubmodulePathsCache(now: number): void { + for (const [cacheKey, entry] of submodulePathsCache) { + if (entry.expiresAt <= now) { + submodulePathsCache.delete(cacheKey) + } + } +} + +function trimSubmodulePathsCache(): void { + while (submodulePathsCache.size > MAX_SUBMODULE_PATHS_CACHE_ENTRIES) { + const oldestKey = submodulePathsCache.keys().next().value + if (oldestKey === undefined) { + break + } + submodulePathsCache.delete(oldestKey) + } +} + +function getCachedSubmodulePaths(cacheKey: string, now: number): string[] | null { + const cached = submodulePathsCache.get(cacheKey) + if (!cached) { + return null + } + if (cached.expiresAt <= now) { + submodulePathsCache.delete(cacheKey) + return null + } + submodulePathsCache.delete(cacheKey) + submodulePathsCache.set(cacheKey, cached) + return cached.paths +} + +function rememberSubmodulePaths(cacheKey: string, paths: string[], now: number): void { + submodulePathsCache.delete(cacheKey) + submodulePathsCache.set(cacheKey, { paths, expiresAt: now + SUBMODULE_PATHS_CACHE_TTL_MS }) + trimSubmodulePathsCache() +} + // Why: status tests reuse this reset hook, so every cross-call memoization layer // must reset together even though the historical name mentions upstream only. export function clearEffectiveUpstreamStatusCacheForTests(): void { @@ -120,7 +182,7 @@ export function clearEffectiveUpstreamStatusCacheForTests(): void { effectiveUpstreamStatusInFlight.clear() retiredEffectiveUpstreamStatusInFlight.clear() effectiveUpstreamStatusWriteGeneration.clear() - clearGitReadInvalidationState() + invalidateGitReadCaches() } export function getEffectiveUpstreamStatusCacheCountForTests(): number { @@ -908,14 +970,18 @@ export async function abortMerge( worktreePath: string, options: GitRuntimeOptions = {} ): Promise { - await gitExecFileAsync(['merge', '--abort'], gitOptionsForWorktree(worktreePath, options)) + await runWithGitReadCacheInvalidation(() => + gitExecFileAsync(['merge', '--abort'], gitOptionsForWorktree(worktreePath, options)) + ) } export async function abortRebase( worktreePath: string, options: GitRuntimeOptions = {} ): Promise { - await gitExecFileAsync(['rebase', '--abort'], gitOptionsForWorktree(worktreePath, options)) + await runWithGitReadCacheInvalidation(() => + gitExecFileAsync(['rebase', '--abort'], gitOptionsForWorktree(worktreePath, options)) + ) } export async function resolveGitDir(worktreePath: string): Promise { @@ -945,10 +1011,14 @@ export async function listSubmodulePaths( ): Promise { const now = Date.now() const cacheKey = getSubmodulePathsCacheKey(worktreePath, options) - const cached = submodulePathsCache.get(cacheKey) - if (cached && cached.expiresAt > now) { - return cached.paths + const cached = getCachedSubmodulePaths(cacheKey, now) + if (cached) { + return cached } + // Why: prune on misses so removed worktrees do not accumulate while hot + // cache hits stay O(1). + pruneExpiredSubmodulePathsCache(now) + const cacheGeneration = submodulePathsCacheGeneration let paths: string[] = [] try { const { stdout } = await gitExecFileAsync( @@ -971,7 +1041,9 @@ export async function listSubmodulePaths( // No .gitmodules (or git config failure) — treat as a repo without submodules. paths = [] } - submodulePathsCache.set(cacheKey, { paths, expiresAt: now + SUBMODULE_PATHS_CACHE_TTL_MS }) + if (cacheGeneration === submodulePathsCacheGeneration) { + rememberSubmodulePaths(cacheKey, paths, Date.now()) + } return paths } @@ -1834,14 +1906,14 @@ export async function stageFile( filePath: string, options: GitRuntimeOptions = {} ): Promise { - clearGitReadInvalidationState() + invalidateGitReadCaches() try { await gitExecFileAsync( ['add', '--', literalPathspec(filePath, options)], gitOptionsForWorktree(worktreePath, options) ) } finally { - clearGitReadInvalidationState() + invalidateGitReadCaches() } } @@ -1853,13 +1925,13 @@ export async function unstageFile( filePath: string, options: GitRuntimeOptions = {} ): Promise { - clearGitReadInvalidationState() + invalidateGitReadCaches() try { await gitExecFileAsync(['restore', '--staged', '--', literalPathspec(filePath, options)], { ...gitOptionsForWorktree(worktreePath, options) }) } finally { - clearGitReadInvalidationState() + invalidateGitReadCaches() } } @@ -1916,7 +1988,7 @@ export async function commitChanges( message: string, options: GitRuntimeOptions = {} ): Promise<{ success: boolean; error?: string }> { - clearGitReadInvalidationState() + invalidateGitReadCaches() try { await gitExecFileAsync(['commit', '-m', message], gitOptionsForWorktree(worktreePath, options)) return { success: true } @@ -1939,7 +2011,7 @@ export async function commitChanges( (error instanceof Error ? error.message : 'Commit failed') return { success: false, error: errorMessage } } finally { - clearGitReadInvalidationState() + invalidateGitReadCaches() } } @@ -1951,7 +2023,7 @@ export async function discardChanges( filePath: string, options: GitRuntimeOptions = {} ): Promise { - clearGitReadInvalidationState() + invalidateGitReadCaches() const resolvedWorktree = path.resolve(worktreePath) const resolvedTarget = path.resolve(worktreePath, filePath) try { @@ -1986,7 +2058,7 @@ export async function discardChanges( cleanUntrackedPaths(worktreePath, [targetPath], options) ) } finally { - clearGitReadInvalidationState() + invalidateGitReadCaches() } } @@ -2061,7 +2133,7 @@ export async function bulkDiscardChanges( filePaths: string[], options: GitRuntimeOptions = {} ): Promise { - clearGitReadInvalidationState() + invalidateGitReadCaches() if (filePaths.length === 0) { return } @@ -2105,7 +2177,7 @@ export async function bulkDiscardChanges( } ) } finally { - clearGitReadInvalidationState() + invalidateGitReadCaches() } } @@ -2131,7 +2203,7 @@ export async function bulkStageFiles( filePaths: string[], options: GitRuntimeOptions = {} ): Promise { - clearGitReadInvalidationState() + invalidateGitReadCaches() if (filePaths.length === 0) { return } @@ -2144,7 +2216,7 @@ export async function bulkStageFiles( ) } } finally { - clearGitReadInvalidationState() + invalidateGitReadCaches() } } @@ -2156,7 +2228,7 @@ export async function bulkUnstageFiles( filePaths: string[], options: GitRuntimeOptions = {} ): Promise { - clearGitReadInvalidationState() + invalidateGitReadCaches() if (filePaths.length === 0) { return } @@ -2176,6 +2248,6 @@ export async function bulkUnstageFiles( ) } } finally { - clearGitReadInvalidationState() + invalidateGitReadCaches() } } diff --git a/src/main/git/worktree.ts b/src/main/git/worktree.ts index 6923858f3..37c017c04 100644 --- a/src/main/git/worktree.ts +++ b/src/main/git/worktree.ts @@ -24,7 +24,7 @@ import { } from '../../shared/git-worktree-command-capabilities' import { getLocalGitCapabilityCache } from './git-capability-state' import { gitExecFileAsync, translateWslOutputPaths } from './runner' -import { resolveGitDir } from './status' +import { resolveGitDir, runWithGitReadCacheInvalidation } from './status' import { hasWorktreeBaseCommitRef } from './worktree-base-ref-probe' export type AddWorktreeResult = { @@ -838,14 +838,16 @@ export async function addWorktree( options: AddWorktreeOptions = {} ): Promise { try { - return await performAddWorktree( - repoPath, - worktreePath, - branch, - baseBranch, - refreshLocalBaseRef, - noCheckout, - options + return await runWithGitReadCacheInvalidation(() => + performAddWorktree( + repoPath, + worktreePath, + branch, + baseBranch, + refreshLocalBaseRef, + noCheckout, + options + ) ) } finally { bumpWorktreeScanGeneration(repoPath) @@ -1056,7 +1058,9 @@ export async function moveWorktree( newPath: string ): Promise { try { - await gitExecFileAsync(['worktree', 'move', oldPath, newPath], { cwd: repoPath }) + await runWithGitReadCacheInvalidation(() => + gitExecFileAsync(['worktree', 'move', oldPath, newPath], { cwd: repoPath }) + ) } finally { bumpWorktreeScanGeneration(repoPath) } @@ -1076,7 +1080,9 @@ export async function removeWorktree( options: RemoveWorktreeOptions = {} ): Promise { try { - return await performRemoveWorktree(repoPath, worktreePath, force, options) + return await runWithGitReadCacheInvalidation(() => + performRemoveWorktree(repoPath, worktreePath, force, options) + ) } finally { bumpWorktreeScanGeneration(repoPath) } diff --git a/src/main/ipc/repos-remote.test.ts b/src/main/ipc/repos-remote.test.ts index 7a23522ac..8b5cb94e5 100644 --- a/src/main/ipc/repos-remote.test.ts +++ b/src/main/ipc/repos-remote.test.ts @@ -20,6 +20,7 @@ const { mockGitProvider, mockFilesystemProvider, mockMultiplexer, + gitExecFileAsyncMock, gitSpawnMock, listWorktreeGraphMock, invalidateAuthorizedRootsCacheMock, @@ -71,6 +72,7 @@ const { notify: vi.fn() }, gitSpawnMock: vi.fn(), + gitExecFileAsyncMock: vi.fn(), listWorktreeGraphMock: vi.fn(), invalidateAuthorizedRootsCacheMock: vi.fn(), prepareLocalWorktreeRootForRepoMock: vi.fn() @@ -103,8 +105,14 @@ vi.mock('../git/repo', async () => { }) vi.mock('../git/runner', () => ({ - gitExecFileAsync: vi.fn(), - gitSpawn: gitSpawnMock + gitExecFileAsync: gitExecFileAsyncMock, + gitExecFileAsyncBuffer: vi.fn(), + gitStreamStdout: vi.fn(), + gitSpawn: gitSpawnMock, + gitOptionalLocksDisabledEnv: (env: NodeJS.ProcessEnv = process.env) => ({ + ...env, + GIT_OPTIONAL_LOCKS: '0' + }) })) vi.mock('../git/worktree', () => ({ @@ -147,6 +155,10 @@ vi.mock('./ssh', () => ({ })) import { registerRepoHandlers } from './repos' +import { + clearSubmodulePathsCacheForTests, + listSubmodulePaths +} from '../git/status' beforeEach(() => { clearGitCapabilityStateForTests() @@ -1011,6 +1023,8 @@ describe('repos:addRemote', () => { mockMultiplexer.request.mockReset() mockMultiplexer.notify.mockReset() gitSpawnMock.mockReset() + gitExecFileAsyncMock.mockReset().mockResolvedValue({ stdout: '', stderr: '' }) + clearSubmodulePathsCacheForTests() prepareLocalWorktreeRootForRepoMock.mockReset().mockResolvedValue(undefined) gitSpawnMock.mockImplementation(() => { const proc = new EventEmitter() as EventEmitter & { stderr: EventEmitter } @@ -1969,6 +1983,41 @@ describe('repos:add + repos:clone', () => { expect(result).toHaveProperty('externalWorktreeVisibility', 'hide') }) + it('drops a same-path negative submodule cache before a local clone', async () => { + const destination = await createTempRoot() + const clonePath = join(destination, 'orca') + let cloned = false + gitExecFileAsyncMock.mockImplementation((args: string[]) => + Promise.resolve({ + stdout: + args[0] === 'config' && args.includes('.gitmodules') && cloned + ? 'submodule.lib.path vendor/lib\n' + : '', + stderr: '' + }) + ) + gitSpawnMock.mockImplementationOnce(() => { + const proc = createMockCloneProcess() + queueMicrotask(() => { + cloned = true + proc.emit('close', 0, null) + }) + return proc + }) + + await expect(listSubmodulePaths(clonePath)).resolves.toEqual([]) + await handlers.get('repos:clone')!(null, { + url: 'https://example.com/orca.git', + destination + }) + await expect(listSubmodulePaths(clonePath)).resolves.toEqual(['vendor/lib']) + + const configReads = gitExecFileAsyncMock.mock.calls.filter( + ([args]) => args[0] === 'config' && args.includes('.gitmodules') + ) + expect(configReads).toHaveLength(2) + }) + it('preserves existing badgeColor when repos:clone upgrades folder->git after dedupe', async () => { const destination = await createTempRoot() const clonePath = join(destination, 'orca') diff --git a/src/main/ipc/repos.ts b/src/main/ipc/repos.ts index 57225c63c..d1b182108 100644 --- a/src/main/ipc/repos.ts +++ b/src/main/ipc/repos.ts @@ -96,6 +96,7 @@ import { } from '../project-groups/folder-workspace-path-status' import { getGitCloneFailureMessage } from '../../shared/git-clone-failure-message' import { prepareLocalWorktreeRootForRepo } from '../worktree-root-preparation' +import { runWithGitReadCacheInvalidation } from '../git/status' // Why: `method` answers "which entry point did the user take?", not "what did // they add?" — so the IPC the renderer invoked IS the method. We never send @@ -982,7 +983,7 @@ async function runWithClonePathLock(clonePathKey: string, task: () => Promise try { await previous - return await task() + return await runWithGitReadCacheInvalidation(task) } finally { release() if (cloneInFlightByPath.get(clonePathKey) === tail) { diff --git a/src/main/providers/ssh-git-provider.test.ts b/src/main/providers/ssh-git-provider.test.ts index 91c781bf4..eb8383a3f 100644 --- a/src/main/providers/ssh-git-provider.test.ts +++ b/src/main/providers/ssh-git-provider.test.ts @@ -1054,6 +1054,42 @@ describe('SshGitProvider', () => { expect(mux.request).toHaveBeenCalledTimes(3) }) + it.each([ + [ + 'clone', + (provider: SshGitProvider) => + provider.clone(['clone', '--', 'https://example.com/repo.git', 'repo'], '/projects') + ], + [ + 'mutating git.exec', + (provider: SshGitProvider) => + provider.exec(['commit', '--allow-empty', '-m', 'initialize'], '/home/user/repo') + ] + ])('clears pending diff RPCs when %s runs', async (_name, mutate) => { + const diff = { + kind: 'text', + originalContent: 'old', + modifiedContent: 'new', + originalIsBinary: false, + modifiedIsBinary: false + } + const pendingDiff = deferredValue(diff) + mux.request.mockReturnValueOnce(pendingDiff.promise) + + const first = provider.getDiff('/home/user/repo', 'src/file.ts', false, true) + await waitForRequestCount(mux.request, 1) + + mux.request.mockResolvedValueOnce({ stdout: '', stderr: '' }) + await mutate(provider) + + mux.request.mockResolvedValueOnce(diff) + const second = provider.getDiff('/home/user/repo', 'src/file.ts', false, true) + + pendingDiff.resolve() + await expect(Promise.all([first, second])).resolves.toEqual([diff, diff]) + expect(mux.request).toHaveBeenCalledTimes(3) + }) + it('clears pending branch diff RPCs when a ref-moving provider operation runs', async () => { const diff = { kind: 'text', diff --git a/src/main/providers/ssh-git-provider.ts b/src/main/providers/ssh-git-provider.ts index 76fb16390..4bca707a0 100644 --- a/src/main/providers/ssh-git-provider.ts +++ b/src/main/providers/ssh-git-provider.ts @@ -31,6 +31,7 @@ import { isMaxBufferOverflowError } from '../git/max-buffer-overflow' import { InFlightPromiseDedupe, stableInFlightKey } from '../../shared/in-flight-promise-dedupe' +import { gitExecMutatesRepository } from '../../shared/git-exec-mutation' type NonInteractiveExecQueueEntry = { started: boolean @@ -761,9 +762,13 @@ export class SshGitProvider implements IGitProvider { cwd: string, options?: { signal?: AbortSignal; timeoutMs?: number } ): Promise<{ stdout: string; stderr: string }> { - const result = options - ? await requestGitStreamable(this.mux, 'git.exec', { args, cwd }, options) - : await requestGitStreamable(this.mux, 'git.exec', { args, cwd }) + const run = () => + options + ? requestGitStreamable(this.mux, 'git.exec', { args, cwd }, options) + : requestGitStreamable(this.mux, 'git.exec', { args, cwd }) + const result = gitExecMutatesRepository(args) + ? await this.runWithDiffDedupeClear(run) + : await run() return result as { stdout: string stderr: string @@ -779,6 +784,7 @@ export class SshGitProvider implements IGitProvider { onProgress?: (progress: { phase: string; percent: number }) => void } ): Promise<{ stdout: string; stderr: string }> { + this.gitDiffReadDedupe.clear() const progressId = `clone-${Date.now()}-${Math.random().toString(36).slice(2)}` const unsubscribe = options?.onProgress ? this.mux.onNotificationByMethod('git.cloneProgress', (params) => { @@ -811,6 +817,7 @@ export class SshGitProvider implements IGitProvider { throw error } finally { unsubscribe?.() + this.gitDiffReadDedupe.clear() } } diff --git a/src/main/runtime/orca-runtime.test.ts b/src/main/runtime/orca-runtime.test.ts index 681eedc9a..1f0fb5900 100644 --- a/src/main/runtime/orca-runtime.test.ts +++ b/src/main/runtime/orca-runtime.test.ts @@ -30,6 +30,10 @@ import { removeWorktree } from '../git/worktree' import * as gitRunner from '../git/runner' +import { + clearSubmodulePathsCacheForTests, + listSubmodulePaths +} from '../git/status' import { createSetupRunnerScript, getEffectiveHooks, @@ -6182,6 +6186,43 @@ describe('OrcaRuntimeService', () => { } }) + it('drops a same-path negative submodule cache before runtime cloneRepo', async () => { + const spawnSpy = vi.spyOn(gitRunner, 'gitSpawn') + const destination = await mkdtemp(join(tmpdir(), 'orca-runtime-reclone-')) + const clonePath = join(destination, 'reclone') + const added: Record[] = [] + const cloneStore = { + ...store, + getRepos: () => [...added] as never, + addRepo: (repo: Record) => added.push(repo), + getRepo: (id: string) => added.find((repo) => repo.id === id) as never + } + spawnSpy.mockImplementation(() => { + const proc = new EventEmitter() as EventEmitter & { stderr: EventEmitter } + proc.stderr = new EventEmitter() + queueMicrotask(() => { + void mkdir(clonePath, { recursive: true }) + .then(() => + writeFile(join(clonePath, '.gitmodules'), '[submodule "lib"]\n\tpath = vendor/lib\n') + ) + .then(() => proc.emit('close', 0, null)) + }) + return proc as never + }) + const runtime = new OrcaRuntimeService(cloneStore as never) + + try { + clearSubmodulePathsCacheForTests() + await expect(listSubmodulePaths(clonePath)).resolves.toEqual([]) + await runtime.cloneRepo('https://example.com/reclone.git', destination) + await expect(listSubmodulePaths(clonePath)).resolves.toEqual(['vendor/lib']) + } finally { + clearSubmodulePathsCacheForTests() + spawnSpy.mockRestore() + await rm(destination, { recursive: true, force: true }) + } + }) + it('preserves existing badgeColor on runtime cloneRepo folder->git dedupe upgrade', async () => { const spawnSpy = vi.spyOn(gitRunner, 'gitSpawn') spawnSpy.mockImplementation(() => { diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index 63eae9a06..e183aff46 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -53,6 +53,7 @@ import { buildAgentPromptPasteBytes } from '../../shared/agent-prompt-injection' import { gitExecFileAsync, gitSpawn } from '../git/runner' +import { runWithGitReadCacheInvalidation } from '../git/status' import { cleanupClaimedCloneTarget, claimCloneTarget, @@ -11470,12 +11471,14 @@ export class OrcaRuntimeService { try { await previous - return await this.cloneRepoAfterPathLock( - trimmedUrl, - trimmedDestination, - clonePath, - clonePathKey, - executionHostId + return await runWithGitReadCacheInvalidation(() => + this.cloneRepoAfterPathLock( + trimmedUrl, + trimmedDestination, + clonePath, + clonePathKey, + executionHostId + ) ) } finally { release() diff --git a/src/relay/git-handler-submodule-cache-invalidation.test.ts b/src/relay/git-handler-submodule-cache-invalidation.test.ts new file mode 100644 index 000000000..57b6a5d22 --- /dev/null +++ b/src/relay/git-handler-submodule-cache-invalidation.test.ts @@ -0,0 +1,74 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { RelayContext } from './context' +import { GitHandler } from './git-handler' +import { + createMockDispatcher, + type MockDispatcher, + type RelayDispatcher +} from './git-handler-test-setup' + +type GitHandlerSpies = { + git(args: string[], cwd: string): Promise<{ stdout: string; stderr: string }> + gitBuffer(args: string[], cwd: string): Promise + spawnClone( + args: string[], + cwd: string, + progressId: string + ): Promise<{ stdout: string; stderr: string }> +} + +describe('GitHandler submodule cache invalidation', () => { + let dispatcher: MockDispatcher + let handler: GitHandler + let target: GitHandlerSpies + + beforeEach(() => { + dispatcher = createMockDispatcher() + handler = new GitHandler(dispatcher as unknown as RelayDispatcher, new RelayContext()) + target = handler as unknown as GitHandlerSpies + vi.spyOn(target, 'git').mockResolvedValue({ stdout: '', stderr: '' }) + vi.spyOn(target, 'gitBuffer').mockResolvedValue(Buffer.from('content\n')) + }) + + afterEach(() => { + handler.dispose() + }) + + it.each([ + { + name: 'clone', + mutate: async () => { + vi.spyOn(target, 'spawnClone').mockResolvedValue({ stdout: '', stderr: '' }) + await dispatcher.callRequest('git.clone', { + args: ['clone', '--', 'https://example.com/repo.git', 'repo'], + cwd: '/projects', + progressId: 'clone-test' + }) + } + }, + { + name: 'mutating git.exec', + mutate: () => + dispatcher.callRequest('git.exec', { + args: ['commit', '--allow-empty', '-m', 'initialize'], + cwd: '/repo' + }) + } + ])('clears a cached empty .gitmodules result around $name', async ({ mutate }) => { + const diffRequest = { + worktreePath: '/repo', + filePath: 'src/file.ts', + staged: false + } + + await dispatcher.callRequest('git.diff', diffRequest) + await mutate() + await dispatcher.callRequest('git.diff', diffRequest) + + const gitSpy = vi.mocked(target.git) + const submodulePathReads = gitSpy.mock.calls.filter( + ([args]) => args[0] === 'config' && args.includes('.gitmodules') + ) + expect(submodulePathReads).toHaveLength(2) + }) +}) diff --git a/src/relay/git-handler-submodule-ops.test.ts b/src/relay/git-handler-submodule-ops.test.ts index ed80b5bc3..154810976 100644 --- a/src/relay/git-handler-submodule-ops.test.ts +++ b/src/relay/git-handler-submodule-ops.test.ts @@ -2,8 +2,11 @@ import { describe, expect, it } from 'vitest' import * as path from 'node:path' import type { GitExec } from './git-handler-ops' import { + MAX_SUBMODULE_PATHS_CACHE_ENTRIES, SUBMODULE_PATHS_CACHE_TTL_MS, + clearSubmodulePathsCache, createSubmodulePathsCache, + getSubmodulePathsCacheCount, listSubmodulePathsCached, resolveSubmoduleWorktreePath } from './git-handler-submodule-ops' @@ -56,6 +59,18 @@ describe('listSubmodulePathsCached', () => { expect(calls()).toBe(2) }) + it('prunes expired entries when a different remote worktree misses', async () => { + const { git } = gitmodulesExec(['vendor/lib']) + const cache = createSubmodulePathsCache() + + await listSubmodulePathsCached(git, '/repo-a', cache, 0) + await listSubmodulePathsCached(git, '/repo-b', cache, 0) + expect(getSubmodulePathsCacheCount(cache)).toBe(2) + + await listSubmodulePathsCached(git, '/repo-c', cache, SUBMODULE_PATHS_CACHE_TTL_MS + 1) + expect(getSubmodulePathsCacheCount(cache)).toBe(1) + }) + it('caches an empty result so a submodule-free repo is not re-read', async () => { let calls = 0 const git: GitExec = async () => { @@ -71,6 +86,67 @@ describe('listSubmodulePathsCached', () => { expect(second).toEqual([]) expect(calls).toBe(1) }) + + it('stays bounded through prolonged remote-worktree churn and retains recent entries', async () => { + const { git, calls } = gitmodulesExec(['vendor/lib']) + const cache = createSubmodulePathsCache() + let now = 0 + + for (let wave = 0; wave < 4; wave += 1) { + for (let i = 0; i < MAX_SUBMODULE_PATHS_CACHE_ENTRIES; i += 1) { + await listSubmodulePathsCached(git, `/wave-${wave}-repo-${i}`, cache, now) + } + expect(getSubmodulePathsCacheCount(cache)).toBe(MAX_SUBMODULE_PATHS_CACHE_ENTRIES) + now += SUBMODULE_PATHS_CACHE_TTL_MS + 1 + } + + await listSubmodulePathsCached(git, '/retained-repo', cache, now) + for (let i = 0; i < MAX_SUBMODULE_PATHS_CACHE_ENTRIES - 1; i += 1) { + await listSubmodulePathsCached(git, `/final-repo-${i}`, cache, now) + } + await listSubmodulePathsCached(git, '/retained-repo', cache, now) + await listSubmodulePathsCached( + git, + `/final-repo-${MAX_SUBMODULE_PATHS_CACHE_ENTRIES - 1}`, + cache, + now + ) + await listSubmodulePathsCached(git, '/overflow-repo', cache, now) + + expect(getSubmodulePathsCacheCount(cache)).toBe(MAX_SUBMODULE_PATHS_CACHE_ENTRIES) + const callsBeforeReads = calls() + await listSubmodulePathsCached(git, '/retained-repo', cache, now) + expect(calls()).toBe(callsBeforeReads) + await listSubmodulePathsCached(git, '/final-repo-0', cache, now) + expect(calls()).toBe(callsBeforeReads + 1) + }) + + it('does not let a pre-mutation SSH read repopulate the cache', async () => { + let resolveOldRead: ((value: { stdout: string; stderr: string }) => void) | undefined + let calls = 0 + const git: GitExec = () => { + calls += 1 + if (calls > 1) { + return Promise.resolve({ stdout: 'submodule.lib.path fresh-lib\n', stderr: '' }) + } + return new Promise((resolve) => { + resolveOldRead = resolve + }) + } + const cache = createSubmodulePathsCache() + + const oldRead = listSubmodulePathsCached(git, '/repo', cache, 1_000) + expect(resolveOldRead).toBeTypeOf('function') + clearSubmodulePathsCache(cache) + resolveOldRead?.({ stdout: 'submodule.lib.path old-lib\n', stderr: '' }) + + await expect(oldRead).resolves.toEqual(['old-lib']) + expect(getSubmodulePathsCacheCount(cache)).toBe(0) + await expect(listSubmodulePathsCached(git, '/repo', cache, 1_001)).resolves.toEqual([ + 'fresh-lib' + ]) + expect(calls).toBe(2) + }) }) describe('resolveSubmoduleWorktreePath', () => { diff --git a/src/relay/git-handler-submodule-ops.ts b/src/relay/git-handler-submodule-ops.ts index 9403fa96f..0409ba611 100644 --- a/src/relay/git-handler-submodule-ops.ts +++ b/src/relay/git-handler-submodule-ops.ts @@ -19,11 +19,69 @@ import { readBlobAtOid, type GitBufferExec, type GitExec } from './git-handler-o * doesn't re-read `.gitmodules` over the (possibly high-latency) SSH link. */ export const SUBMODULE_PATHS_CACHE_TTL_MS = 5_000 +export const MAX_SUBMODULE_PATHS_CACHE_ENTRIES = 512 type SubmodulePathsCacheEntry = { paths: string[]; expiresAt: number } -export type SubmodulePathsCache = Map +export type SubmodulePathsCache = { + entries: Map + generation: number +} export function createSubmodulePathsCache(): SubmodulePathsCache { - return new Map() + return { entries: new Map(), generation: 0 } +} + +export function clearSubmodulePathsCache(cache: SubmodulePathsCache): void { + cache.entries.clear() + // Why: a pre-mutation SSH read must not restore stale .gitmodules paths + // after the mutation invalidated them. + cache.generation += 1 +} + +export function getSubmodulePathsCacheCount(cache: SubmodulePathsCache): number { + return cache.entries.size +} + +function getCachedSubmodulePaths( + cache: SubmodulePathsCache, + worktreePath: string, + now: number +): string[] | null { + const cached = cache.entries.get(worktreePath) + if (!cached) { + return null + } + if (cached.expiresAt <= now) { + cache.entries.delete(worktreePath) + return null + } + cache.entries.delete(worktreePath) + cache.entries.set(worktreePath, cached) + return cached.paths +} + +function pruneExpiredSubmodulePaths(cache: SubmodulePathsCache, now: number): void { + for (const [worktreePath, entry] of cache.entries) { + if (entry.expiresAt <= now) { + cache.entries.delete(worktreePath) + } + } +} + +function rememberSubmodulePaths( + cache: SubmodulePathsCache, + worktreePath: string, + paths: string[], + now: number +): void { + cache.entries.delete(worktreePath) + cache.entries.set(worktreePath, { paths, expiresAt: now + SUBMODULE_PATHS_CACHE_TTL_MS }) + while (cache.entries.size > MAX_SUBMODULE_PATHS_CACHE_ENTRIES) { + const oldestPath = cache.entries.keys().next().value + if (oldestPath === undefined) { + break + } + cache.entries.delete(oldestPath) + } } /** @@ -38,12 +96,18 @@ export async function listSubmodulePathsCached( cache: SubmodulePathsCache, now: number = Date.now() ): Promise { - const cached = cache.get(worktreePath) - if (cached && cached.expiresAt > now) { - return cached.paths + const cached = getCachedSubmodulePaths(cache, worktreePath, now) + if (cached) { + return cached } + // Why: prune on misses so disconnected worktrees cannot accumulate while + // repeated SSH diff clicks keep their O(1) cache-hit path. + pruneExpiredSubmodulePaths(cache, now) + const cacheGeneration = cache.generation const paths = await listSubmodulePaths(git, worktreePath) - cache.set(worktreePath, { paths, expiresAt: now + SUBMODULE_PATHS_CACHE_TTL_MS }) + if (cacheGeneration === cache.generation) { + rememberSubmodulePaths(cache, worktreePath, paths, now) + } return paths } diff --git a/src/relay/git-handler.test.ts b/src/relay/git-handler.test.ts index ba8f631d8..1ae68d3ce 100644 --- a/src/relay/git-handler.test.ts +++ b/src/relay/git-handler.test.ts @@ -1193,6 +1193,10 @@ describe('GitHandler', () => { expect(gitBufferSpy).toHaveBeenCalledTimes(2) expect(gitSpy).toHaveBeenCalledWith(['add', '--', 'src/file.ts'], tmpDir) + const submodulePathReads = gitSpy.mock.calls.filter( + ([args]) => args[0] === 'config' && args.includes('.gitmodules') + ) + expect(submodulePathReads).toHaveLength(2) }) it('clears pending git.diff reads when a narrow ref fetch runs', async () => { diff --git a/src/relay/git-handler.ts b/src/relay/git-handler.ts index 67e78821d..d21dcf3cc 100644 --- a/src/relay/git-handler.ts +++ b/src/relay/git-handler.ts @@ -22,6 +22,7 @@ import { buildSubmoduleInnerCommitRangeDiff, computeSubmodulePointerDiff, computeSubmoduleRangeEntries, + clearSubmodulePathsCache, createSubmodulePathsCache, findContainingSubmodule, listSubmodulePathsCached, @@ -39,6 +40,7 @@ import { } from './git-handler-worktree-ops' import { forceDeletePreservedRelayBranch } from './git-handler-branch-cleanup' import { refreshLocalBaseRefForWorktreeCreateOp } from './git-handler-local-base-ref-refresh' +import { gitExecMutatesRepository } from '../shared/git-exec-mutation' import { detectConflictOperation, getStatusOp } from './git-handler-status-ops' import { checkIgnoredPathsOp } from './git-handler-check-ignore' import { resolveRelayPushTarget } from './git-handler-push-target' @@ -182,6 +184,7 @@ export class GitHandler { dispose(): void { this.responseStreams.disposeAll() + this.clearGitMutationReadCaches() } private registerHandlers(): void { @@ -272,14 +275,19 @@ export class GitHandler { return this.responseStreams.startStream(payload, this.dispatcher, context) } - private async runWithDiffDedupeClear(run: () => Promise): Promise { - // Why: git mutations can stale both existing and concurrently-started diff reads. - // Clear before and after so later reads never join pre-mutation work. + private clearGitMutationReadCaches(): void { this.gitDiffReadDedupe.clear() + clearSubmodulePathsCache(this.submodulePathsCache) + } + + private async runWithGitReadCacheClear(run: () => Promise): Promise { + // Why: git mutations can stale existing and concurrently-started diff and + // .gitmodules reads. Clear before and after so later reads cannot join them. + this.clearGitMutationReadCaches() try { return await run() } finally { - this.gitDiffReadDedupe.clear() + this.clearGitMutationReadCaches() } } @@ -476,42 +484,42 @@ export class GitHandler { } private async stage(params: Record) { - this.gitDiffReadDedupe.clear() + this.clearGitMutationReadCaches() const worktreePath = params.worktreePath as string const filePath = params.filePath as string try { await this.git(['add', '--', filePath], worktreePath) } finally { - this.gitDiffReadDedupe.clear() + this.clearGitMutationReadCaches() } } private async commit( params: Record ): Promise<{ success: boolean; error?: string }> { - this.gitDiffReadDedupe.clear() + this.clearGitMutationReadCaches() const worktreePath = params.worktreePath as string const message = params.message as string try { return await commitChangesRelay(this.git.bind(this), worktreePath, message) } finally { - this.gitDiffReadDedupe.clear() + this.clearGitMutationReadCaches() } } private async unstage(params: Record) { - this.gitDiffReadDedupe.clear() + this.clearGitMutationReadCaches() const worktreePath = params.worktreePath as string const filePath = params.filePath as string try { await this.git(['restore', '--staged', '--', filePath], worktreePath) } finally { - this.gitDiffReadDedupe.clear() + this.clearGitMutationReadCaches() } } private async bulkStage(params: Record) { - this.gitDiffReadDedupe.clear() + this.clearGitMutationReadCaches() const worktreePath = params.worktreePath as string const filePaths = params.filePaths as string[] try { @@ -520,12 +528,12 @@ export class GitHandler { await this.git(['add', '--', ...chunk], worktreePath) } } finally { - this.gitDiffReadDedupe.clear() + this.clearGitMutationReadCaches() } } private async bulkUnstage(params: Record) { - this.gitDiffReadDedupe.clear() + this.clearGitMutationReadCaches() const worktreePath = params.worktreePath as string const filePaths = params.filePaths as string[] try { @@ -534,32 +542,32 @@ export class GitHandler { await this.git(['restore', '--staged', '--', ...chunk], worktreePath) } } finally { - this.gitDiffReadDedupe.clear() + this.clearGitMutationReadCaches() } } private async abortMerge(params: Record) { - this.gitDiffReadDedupe.clear() + this.clearGitMutationReadCaches() const worktreePath = params.worktreePath as string try { await this.git(['merge', '--abort'], worktreePath) } finally { - this.gitDiffReadDedupe.clear() + this.clearGitMutationReadCaches() } } private async abortRebase(params: Record) { - this.gitDiffReadDedupe.clear() + this.clearGitMutationReadCaches() const worktreePath = params.worktreePath as string try { await this.git(['rebase', '--abort'], worktreePath) } finally { - this.gitDiffReadDedupe.clear() + this.clearGitMutationReadCaches() } } private async checkout(params: Record) { - this.gitDiffReadDedupe.clear() + this.clearGitMutationReadCaches() const worktreePath = params.worktreePath as string const branch = params.branch as string // Defense-in-depth: reject option-like branch tokens (the RPC schema also @@ -573,7 +581,7 @@ export class GitHandler { await this.git(['checkout', branch, '--'], worktreePath) return { ok: true as const, branch } } finally { - this.gitDiffReadDedupe.clear() + this.clearGitMutationReadCaches() } } @@ -632,7 +640,7 @@ export class GitHandler { } private async discard(params: Record) { - this.gitDiffReadDedupe.clear() + this.clearGitMutationReadCaches() const worktreePath = params.worktreePath as string const filePath = params.filePath as string try { @@ -661,12 +669,12 @@ export class GitHandler { this.cleanUntrackedPaths(worktreePath, [targetPath]) ) } finally { - this.gitDiffReadDedupe.clear() + this.clearGitMutationReadCaches() } } private async bulkDiscard(params: Record) { - this.gitDiffReadDedupe.clear() + this.clearGitMutationReadCaches() const worktreePath = params.worktreePath as string const filePaths = params.filePaths as string[] if (filePaths.length === 0) { @@ -720,7 +728,7 @@ export class GitHandler { } ) } finally { - this.gitDiffReadDedupe.clear() + this.clearGitMutationReadCaches() } } @@ -826,7 +834,7 @@ export class GitHandler { } private async fetch(params: Record) { - this.gitDiffReadDedupe.clear() + this.clearGitMutationReadCaches() const worktreePath = params.worktreePath as string try { try { @@ -845,12 +853,12 @@ export class GitHandler { throw new Error(normalizeGitErrorMessage(error, 'fetch')) } } finally { - this.gitDiffReadDedupe.clear() + this.clearGitMutationReadCaches() } } private async forkSync(params: Record, context?: RequestContext) { - return this.runWithDiffDedupeClear(async () => { + return this.runWithGitReadCacheClear(async () => { const worktreePath = params.worktreePath as string const expectedUpstream = validateGitForkSyncExpectedUpstream(params.expectedUpstream, { required: true @@ -882,7 +890,7 @@ export class GitHandler { } private async fetchRemoteTrackingRef(params: Record) { - this.gitDiffReadDedupe.clear() + this.clearGitMutationReadCaches() const worktreePath = params.worktreePath as string const remote = params.remote const branch = params.branch @@ -930,12 +938,12 @@ export class GitHandler { throw new Error(normalizeGitErrorMessage(error, 'fetch')) } } finally { - this.gitDiffReadDedupe.clear() + this.clearGitMutationReadCaches() } } private async fetchGitLabMergeRequestHead(params: Record) { - this.gitDiffReadDedupe.clear() + this.clearGitMutationReadCaches() const worktreePath = params.worktreePath as string const remote = params.remote const mrIid = params.mrIid @@ -970,12 +978,12 @@ export class GitHandler { throw new Error(normalizeGitErrorMessage(error, 'fetch')) } } finally { - this.gitDiffReadDedupe.clear() + this.clearGitMutationReadCaches() } } private async push(params: Record) { - this.gitDiffReadDedupe.clear() + this.clearGitMutationReadCaches() const worktreePath = params.worktreePath as string // Why: mirror src/main/git/remote.ts. Push to a configured upstream when // present so SSH worktrees with non-origin targets do not get repointed. @@ -1000,12 +1008,12 @@ export class GitHandler { throw new Error(normalizeGitErrorMessage(error, 'push')) } } finally { - this.gitDiffReadDedupe.clear() + this.clearGitMutationReadCaches() } } private async pullWithArgs(params: Record, pullArgs: string[]) { - this.gitDiffReadDedupe.clear() + this.clearGitMutationReadCaches() const worktreePath = params.worktreePath as string try { try { @@ -1036,7 +1044,7 @@ export class GitHandler { throw new Error(normalizeGitErrorMessage(error, 'pull')) } } finally { - this.gitDiffReadDedupe.clear() + this.clearGitMutationReadCaches() } } @@ -1051,7 +1059,7 @@ export class GitHandler { } private async rebaseFromBase(params: Record) { - this.gitDiffReadDedupe.clear() + this.clearGitMutationReadCaches() const worktreePath = params.worktreePath as string const baseRef = params.baseRef as string try { @@ -1065,7 +1073,7 @@ export class GitHandler { throw new Error(normalizeGitErrorMessage(error, 'pull')) } } finally { - this.gitDiffReadDedupe.clear() + this.clearGitMutationReadCaches() } } @@ -1128,7 +1136,10 @@ export class GitHandler { const cwd = params.cwd as string validateGitExecArgs(args) - const { stdout, stderr } = await this.git(args, cwd, { signal: context?.signal }) + const run = () => this.git(args, cwd, { signal: context?.signal }) + const { stdout, stderr } = gitExecMutatesRepository(args) + ? await this.runWithGitReadCacheClear(run) + : await run() return this.maybeStreamResponse({ stdout, stderr }, params, context) } @@ -1143,7 +1154,9 @@ export class GitHandler { if (args[0] !== 'clone') { throw new Error('git.clone only supports clone commands.') } - return await this.spawnClone(args, cwd, progressId, context) + return await this.runWithGitReadCacheClear(() => + this.spawnClone(args, cwd, progressId, context) + ) } private async spawnClone( @@ -1213,7 +1226,7 @@ export class GitHandler { } private async renameCurrentBranch(params: Record) { - return this.runWithDiffDedupeClear(async () => { + return this.runWithGitReadCacheClear(async () => { const worktreePath = params.worktreePath const newBranch = params.newBranch if (typeof worktreePath !== 'string' || typeof newBranch !== 'string') { @@ -1250,7 +1263,7 @@ export class GitHandler { if (!repoPath || repoPath.includes('\0') || expectedHead.includes('\0')) { throw new Error('Invalid preserved branch force-delete request.') } - return this.runWithDiffDedupeClear(() => + return this.runWithGitReadCacheClear(() => forceDeletePreservedRelayBranch(this.git.bind(this), repoPath, branchName, expectedHead) ) } @@ -1359,11 +1372,11 @@ export class GitHandler { } private async addWorktree(params: Record) { - return this.runWithDiffDedupeClear(() => addWorktreeOp(this.git.bind(this), params)) + return this.runWithGitReadCacheClear(() => addWorktreeOp(this.git.bind(this), params)) } private async removeWorktree(params: Record) { - return this.runWithDiffDedupeClear(() => + return this.runWithGitReadCacheClear(() => removeWorktreeOp(this.git.bind(this), params, this.gitCapabilities) ) } @@ -1373,7 +1386,7 @@ export class GitHandler { } private async refreshLocalBaseRefForWorktreeCreate(params: Record) { - return this.runWithDiffDedupeClear(() => + return this.runWithGitReadCacheClear(() => refreshLocalBaseRefForWorktreeCreateOp(this.git.bind(this), params, this.gitCapabilities) ) } diff --git a/src/shared/git-exec-mutation.ts b/src/shared/git-exec-mutation.ts new file mode 100644 index 000000000..78412f04f --- /dev/null +++ b/src/shared/git-exec-mutation.ts @@ -0,0 +1,7 @@ +const MUTATING_GIT_EXEC_SUBCOMMANDS = new Set(['clone', 'commit', 'init']) + +// Why: relay git.exec permits these narrow write shapes alongside read-only +// probes, so cache invalidation must distinguish them before dispatch. +export function gitExecMutatesRepository(args: readonly string[]): boolean { + return MUTATING_GIT_EXEC_SUBCOMMANDS.has(args[0] ?? '') +}