From 829f8d961882201637b0bc699b91dda32d05c4a9 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Thu, 25 Jun 2026 18:47:26 -0700 Subject: [PATCH] Reduce duplicate diff loading work (#6389) * Deduplicate in-flight diff reads * Clear diff dedupe for ref-moving SSH operations * Clear diff dedupe for worktree ref mutations * Document diff dedupe mutation invalidation --------- Co-authored-by: Neil --- src/main/git/status.test.ts | 257 +++++++- src/main/git/status.ts | 247 +++++--- src/main/providers/ssh-git-provider.test.ts | 274 +++++++++ src/main/providers/ssh-git-provider.ts | 235 ++++++-- src/relay/git-handler.test.ts | 407 +++++++++++++ src/relay/git-handler.ts | 631 ++++++++++++-------- src/shared/in-flight-promise-dedupe.test.ts | 83 +++ src/shared/in-flight-promise-dedupe.ts | 57 ++ 8 files changed, 1808 insertions(+), 383 deletions(-) create mode 100644 src/shared/in-flight-promise-dedupe.test.ts create mode 100644 src/shared/in-flight-promise-dedupe.ts diff --git a/src/main/git/status.test.ts b/src/main/git/status.test.ts index 65eec59dc..b91c8e06a 100644 --- a/src/main/git/status.test.ts +++ b/src/main/git/status.test.ts @@ -70,15 +70,41 @@ import { bulkUnstageFiles, clearEffectiveUpstreamStatusCacheForTests, detectConflictOperation, + getBranchDiff, discardChanges, + getCommitDiff, getBranchCompare, getCommitCompare, getDiff, getStagedCommitContext, getStatus, - isWithinWorktree + isWithinWorktree, + stageFile } from './status' +function deferredBuffer(content: string): { + promise: Promise<{ stdout: Buffer }> + resolve: () => void +} { + let resolve!: (value: { stdout: Buffer }) => void + const promise = new Promise<{ stdout: Buffer }>((innerResolve) => { + resolve = innerResolve + }) + return { + promise, + resolve: () => resolve({ stdout: Buffer.from(content) }) + } +} + +async function waitForMockCalls(mock: ReturnType, calls: number): Promise { + for (let i = 0; i < 20; i++) { + if (mock.mock.calls.length >= calls) { + return + } + await new Promise((resolve) => setTimeout(resolve, 0)) + } +} + describe('discardChanges', () => { beforeEach(() => { gitExecFileAsyncMock.mockReset() @@ -445,6 +471,235 @@ describe('getDiff', () => { mimeType: 'application/pdf' }) }) + + it('coalesces concurrent identical staged diff reads while in flight', async () => { + const leftBlob = deferredBuffer('head-content\n') + const rightBlob = deferredBuffer('index-content\n') + const pendingBuffers = [leftBlob, rightBlob] + gitExecFileAsyncBufferMock.mockImplementation(async () => pendingBuffers.shift()!.promise) + + const reads = Array.from({ length: 8 }, () => getDiff('/repo', 'src/file.ts', true)) + + await waitForMockCalls(gitExecFileAsyncBufferMock, 1) + expect(gitExecFileAsyncBufferMock).toHaveBeenCalledTimes(1) + + leftBlob.resolve() + await waitForMockCalls(gitExecFileAsyncBufferMock, 2) + rightBlob.resolve() + + const results = await Promise.all(reads) + + expect(gitExecFileAsyncBufferMock).toHaveBeenCalledTimes(2) + expect(results.every((result) => result.kind === 'text')).toBe(true) + + gitExecFileAsyncBufferMock + .mockResolvedValueOnce({ stdout: Buffer.from('fresh-head\n') }) + .mockResolvedValueOnce({ stdout: Buffer.from('fresh-index\n') }) + + await getDiff('/repo', 'src/file.ts', true) + + expect(gitExecFileAsyncBufferMock).toHaveBeenCalledTimes(4) + }) + + it('clears pending diff reads when a mutation runs', async () => { + const firstBlob = deferredBuffer('head-content\n') + const secondBlob = deferredBuffer('fresh-head-content\n') + const pendingBuffers = [firstBlob, secondBlob] + gitExecFileAsyncBufferMock.mockImplementation(async () => pendingBuffers.shift()!.promise) + readFileMock.mockResolvedValue(Buffer.from('working-tree\n')) + gitExecFileAsyncMock.mockResolvedValue({ stdout: '', stderr: '' }) + + const first = getDiff('/repo', 'src/file.ts', false) + await waitForMockCalls(gitExecFileAsyncBufferMock, 1) + + await stageFile('/repo', 'src/file.ts') + + const second = getDiff('/repo', 'src/file.ts', false) + await waitForMockCalls(gitExecFileAsyncBufferMock, 2) + + firstBlob.resolve() + secondBlob.resolve() + await Promise.all([first, second]) + + expect(gitExecFileAsyncBufferMock).toHaveBeenCalledTimes(2) + expect(gitExecFileAsyncMock).toHaveBeenCalledWith( + ['add', '--', ':(literal)src/file.ts'], + expect.objectContaining({ cwd: '/repo' }) + ) + }) + + it('coalesces concurrent identical branch and commit diff reads while in flight', async () => { + const branchLeftBlob = deferredBuffer('branch-left\n') + const branchRightBlob = deferredBuffer('branch-right\n') + const pendingBranchBuffers = [branchLeftBlob, branchRightBlob] + gitExecFileAsyncBufferMock.mockImplementation(async () => pendingBranchBuffers.shift()!.promise) + + const branchReads = Array.from({ length: 8 }, () => + getBranchDiff('/repo', { + mergeBase: 'b'.repeat(40), + headOid: 'c'.repeat(40), + filePath: 'src/file.ts', + oldPath: 'src/old-file.ts' + }) + ) + + await waitForMockCalls(gitExecFileAsyncBufferMock, 1) + branchLeftBlob.resolve() + await waitForMockCalls(gitExecFileAsyncBufferMock, 2) + branchRightBlob.resolve() + + await Promise.all(branchReads) + expect(gitExecFileAsyncBufferMock).toHaveBeenCalledTimes(2) + + gitExecFileAsyncBufferMock.mockReset() + const commitLeftBlob = deferredBuffer('commit-left\n') + const commitRightBlob = deferredBuffer('commit-right\n') + const pendingCommitBuffers = [commitLeftBlob, commitRightBlob] + gitExecFileAsyncBufferMock.mockImplementation(async () => pendingCommitBuffers.shift()!.promise) + + const commitReads = Array.from({ length: 8 }, () => + getCommitDiff('/repo', { + parentOid: 'd'.repeat(40), + commitOid: 'e'.repeat(40), + filePath: 'src/file.ts', + oldPath: 'src/old-file.ts' + }) + ) + + await waitForMockCalls(gitExecFileAsyncBufferMock, 1) + commitLeftBlob.resolve() + await waitForMockCalls(gitExecFileAsyncBufferMock, 2) + commitRightBlob.resolve() + + await Promise.all(commitReads) + expect(gitExecFileAsyncBufferMock).toHaveBeenCalledTimes(2) + }) + + it('coalesces logically identical branch and commit diff args regardless of property order', async () => { + gitExecFileAsyncBufferMock.mockResolvedValue({ stdout: Buffer.from('blob\n') }) + + await Promise.all([ + getBranchDiff('/repo', { + mergeBase: 'b'.repeat(40), + headOid: 'c'.repeat(40), + filePath: 'src/file.ts', + oldPath: 'src/old-file.ts' + }), + getBranchDiff('/repo', { + oldPath: 'src/old-file.ts', + filePath: 'src/file.ts', + headOid: 'c'.repeat(40), + mergeBase: 'b'.repeat(40) + }) + ]) + + expect(gitExecFileAsyncBufferMock).toHaveBeenCalledTimes(2) + + gitExecFileAsyncBufferMock.mockClear() + + await Promise.all([ + getCommitDiff('/repo', { + parentOid: 'd'.repeat(40), + commitOid: 'e'.repeat(40), + filePath: 'src/file.ts', + oldPath: 'src/old-file.ts' + }), + getCommitDiff('/repo', { + oldPath: 'src/old-file.ts', + filePath: 'src/file.ts', + commitOid: 'e'.repeat(40), + parentOid: 'd'.repeat(40) + }) + ]) + + expect(gitExecFileAsyncBufferMock).toHaveBeenCalledTimes(2) + }) + + it('keeps distinct diff inputs on separate in-flight reads', async () => { + gitExecFileAsyncBufferMock.mockResolvedValue({ stdout: Buffer.from('blob\n') }) + readFileMock.mockResolvedValue(Buffer.from('working-tree\n')) + + await Promise.all([ + getDiff('/repo', 'src/file.ts', false, false), + getDiff('/repo', 'src/file.ts', false, true), + getDiff('/repo', 'src/file.ts', true, false), + getDiff('/repo', 'src/file.ts', true, false, { wslDistro: 'ubuntu' }), + getDiff('/repo', 'src/file.ts', true, false, { wslDistro: 'debian' }) + ]) + + expect(gitExecFileAsyncBufferMock).toHaveBeenCalledTimes(8) + + gitExecFileAsyncBufferMock.mockReset() + gitExecFileAsyncBufferMock.mockResolvedValue({ stdout: Buffer.from('blob\n') }) + + await Promise.all([ + getBranchDiff('/repo', { + mergeBase: 'b'.repeat(40), + headOid: 'c'.repeat(40), + filePath: 'src/file.ts' + }), + getBranchDiff('/repo', { + mergeBase: 'b'.repeat(40), + headOid: 'd'.repeat(40), + filePath: 'src/file.ts' + }), + getBranchDiff('/repo', { + mergeBase: 'b'.repeat(40), + headOid: 'c'.repeat(40), + filePath: 'src/file.ts', + oldPath: 'src/old-a.ts' + }), + getBranchDiff('/repo', { + mergeBase: 'b'.repeat(40), + headOid: 'c'.repeat(40), + filePath: 'src/file.ts', + oldPath: 'src/old-b.ts' + }), + getCommitDiff('/repo', { + parentOid: 'e'.repeat(40), + commitOid: 'f'.repeat(40), + filePath: 'src/file.ts' + }), + getCommitDiff('/repo', { + parentOid: 'a'.repeat(40), + commitOid: 'f'.repeat(40), + filePath: 'src/file.ts' + }), + getCommitDiff('/repo', { + parentOid: 'e'.repeat(40), + commitOid: 'f'.repeat(40), + filePath: 'src/file.ts', + oldPath: 'src/old-a.ts' + }), + getCommitDiff('/repo', { + parentOid: 'e'.repeat(40), + commitOid: 'f'.repeat(40), + filePath: 'src/file.ts', + oldPath: 'src/old-b.ts' + }) + ]) + + expect(gitExecFileAsyncBufferMock).toHaveBeenCalledTimes(16) + }) + + it('coalesces parentless root commit diff reads without reading a left-side blob', async () => { + const rightBlob = deferredBuffer('root-content\n') + gitExecFileAsyncBufferMock.mockImplementation(async () => rightBlob.promise) + + const reads = Array.from({ length: 8 }, () => + getCommitDiff('/repo', { + parentOid: null, + commitOid: 'e'.repeat(40), + filePath: 'src/file.ts' + }) + ) + + await waitForMockCalls(gitExecFileAsyncBufferMock, 1) + rightBlob.resolve() + await Promise.all(reads) + + expect(gitExecFileAsyncBufferMock).toHaveBeenCalledTimes(1) + }) }) describe('getStatus', () => { diff --git a/src/main/git/status.ts b/src/main/git/status.ts index 1adc59a11..63917212b 100644 --- a/src/main/git/status.ts +++ b/src/main/git/status.ts @@ -45,6 +45,7 @@ import { import { resolveWorktreeAddBaseRef } from '../../shared/worktree-base-ref' import { hasWorktreeBaseCommitRef } from './worktree-base-ref-probe' import { getLargeDiffRenderLimit } from '../../shared/large-diff-render-limit' +import { InFlightPromiseDedupe, stableInFlightKey } from '../../shared/in-flight-promise-dedupe' import type { GitRuntimeOptions } from './git-runtime-options' import { gitOptionsForWorktree } from './git-runtime-options' import { parseGitRevListFirstParentOid } from '../../shared/git-rev-list-output' @@ -63,15 +64,21 @@ type EffectiveUpstreamStatusCacheEntry = { const effectiveUpstreamStatusCache = new Map() const effectiveUpstreamStatusInFlight = new Map>() const retiredEffectiveUpstreamStatusInFlight = new Map>() +const gitDiffReadDedupe = new InFlightPromiseDedupe() const effectiveUpstreamStatusWriteGeneration = new Map() const statusReadsInFlight = new Map>() +function gitRuntimeOptionsKey(options: GitRuntimeOptions): readonly unknown[] { + return [options.wslDistro ?? null] +} + // 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 { effectiveUpstreamStatusCache.clear() effectiveUpstreamStatusInFlight.clear() retiredEffectiveUpstreamStatusInFlight.clear() + gitDiffReadDedupe.clear() effectiveUpstreamStatusWriteGeneration.clear() statusReadsInFlight.clear() } @@ -101,6 +108,7 @@ export async function getStatus( worktreePath: string, options: GetStatusOptions = {} ): Promise { + gitDiffReadDedupe.clear() // Why: dedupe only concurrent identical reads; after settle, callers must // execute a fresh status read rather than observing a cached result. const cacheKey = getStatusReadKey(worktreePath, options) @@ -722,6 +730,26 @@ export async function getDiff( staged: boolean, compareAgainstHead = false, options: GitRuntimeOptions = {} +): Promise { + return gitDiffReadDedupe.run( + stableInFlightKey([ + 'diff', + worktreePath, + filePath, + staged, + compareAgainstHead, + ...gitRuntimeOptionsKey(options) + ]), + () => loadDiff(worktreePath, filePath, staged, compareAgainstHead, options) + ) +} + +async function loadDiff( + worktreePath: string, + filePath: string, + staged: boolean, + compareAgainstHead: boolean, + options: GitRuntimeOptions ): Promise { let originalContent = '' let modifiedContent = '' @@ -852,6 +880,30 @@ export async function getBranchDiff( oldPath?: string }, options: GitRuntimeOptions = {} +): Promise { + return gitDiffReadDedupe.run( + stableInFlightKey([ + 'branchDiff', + worktreePath, + args.headOid, + args.mergeBase, + args.filePath, + args.oldPath ?? null, + ...gitRuntimeOptionsKey(options) + ]), + () => loadBranchDiff(worktreePath, args, options) + ) +} + +async function loadBranchDiff( + worktreePath: string, + args: { + headOid: string + mergeBase: string + filePath: string + oldPath?: string + }, + options: GitRuntimeOptions ): Promise { try { const leftPath = args.oldPath ?? args.filePath @@ -941,6 +993,30 @@ export async function getCommitDiff( oldPath?: string }, options: GitRuntimeOptions = {} +): Promise { + return gitDiffReadDedupe.run( + stableInFlightKey([ + 'commitDiff', + worktreePath, + args.commitOid, + args.parentOid ?? null, + args.filePath, + args.oldPath ?? null, + ...gitRuntimeOptionsKey(options) + ]), + () => loadCommitDiff(worktreePath, args, options) + ) +} + +async function loadCommitDiff( + worktreePath: string, + args: { + commitOid: string + parentOid?: string | null + filePath: string + oldPath?: string + }, + options: GitRuntimeOptions ): Promise { try { const leftPath = args.oldPath ?? args.filePath @@ -1316,10 +1392,15 @@ export async function stageFile( filePath: string, options: GitRuntimeOptions = {} ): Promise { - await gitExecFileAsync( - ['add', '--', literalPathspec(filePath)], - gitOptionsForWorktree(worktreePath, options) - ) + gitDiffReadDedupe.clear() + try { + await gitExecFileAsync( + ['add', '--', literalPathspec(filePath)], + gitOptionsForWorktree(worktreePath, options) + ) + } finally { + gitDiffReadDedupe.clear() + } } /** @@ -1330,9 +1411,14 @@ export async function unstageFile( filePath: string, options: GitRuntimeOptions = {} ): Promise { - await gitExecFileAsync(['restore', '--staged', '--', literalPathspec(filePath)], { - ...gitOptionsForWorktree(worktreePath, options) - }) + gitDiffReadDedupe.clear() + try { + await gitExecFileAsync(['restore', '--staged', '--', literalPathspec(filePath)], { + ...gitOptionsForWorktree(worktreePath, options) + }) + } finally { + gitDiffReadDedupe.clear() + } } export async function getStagedCommitContext( @@ -1388,6 +1474,7 @@ export async function commitChanges( message: string, options: GitRuntimeOptions = {} ): Promise<{ success: boolean; error?: string }> { + gitDiffReadDedupe.clear() try { await gitExecFileAsync(['commit', '-m', message], gitOptionsForWorktree(worktreePath, options)) return { success: true } @@ -1409,6 +1496,8 @@ export async function commitChanges( readStringField('stdout') ?? (error instanceof Error ? error.message : 'Commit failed') return { success: false, error: errorMessage } + } finally { + gitDiffReadDedupe.clear() } } @@ -1420,35 +1509,40 @@ export async function discardChanges( filePath: string, options: GitRuntimeOptions = {} ): Promise { + gitDiffReadDedupe.clear() const resolvedWorktree = path.resolve(worktreePath) const resolvedTarget = path.resolve(worktreePath, filePath) - if (!isWithinWorktree(path, resolvedWorktree, resolvedTarget)) { - throw new Error(`Path "${filePath}" resolves outside the worktree`) - } - - let tracked = false try { - await gitExecFileAsync(['ls-files', '--error-unmatch', '--', literalPathspec(filePath)], { - ...gitOptionsForWorktree(worktreePath, options) - }) - tracked = true - } catch { - // File is not tracked by git - } + if (!isWithinWorktree(path, resolvedWorktree, resolvedTarget)) { + throw new Error(`Path "${filePath}" resolves outside the worktree`) + } - if (tracked) { - await gitExecFileAsync( - ['restore', '--worktree', '--source=HEAD', '--', literalPathspec(filePath)], - { + let tracked = false + try { + await gitExecFileAsync(['ls-files', '--error-unmatch', '--', literalPathspec(filePath)], { ...gitOptionsForWorktree(worktreePath, options) - } - ) - return - } + }) + tracked = true + } catch { + // File is not tracked by git + } - await removeSafeUntrackedDiscardTarget(worktreePath, filePath, (targetPath) => - cleanUntrackedPaths(worktreePath, [targetPath], options) - ) + if (tracked) { + await gitExecFileAsync( + ['restore', '--worktree', '--source=HEAD', '--', literalPathspec(filePath)], + { + ...gitOptionsForWorktree(worktreePath, options) + } + ) + return + } + + await removeSafeUntrackedDiscardTarget(worktreePath, filePath, (targetPath) => + cleanUntrackedPaths(worktreePath, [targetPath], options) + ) + } finally { + gitDiffReadDedupe.clear() + } } function normalizeGitPathForCompare(filePath: string): string { @@ -1517,39 +1611,46 @@ export async function bulkDiscardChanges( filePaths: string[], options: GitRuntimeOptions = {} ): Promise { + gitDiffReadDedupe.clear() if (filePaths.length === 0) { return } - const resolvedWorktree = path.resolve(worktreePath) - for (const filePath of filePaths) { - const resolvedTarget = path.resolve(worktreePath, filePath) - if (!isWithinWorktree(path, resolvedWorktree, resolvedTarget)) { - throw new Error(`Path "${filePath}" resolves outside the worktree`) - } - } - - const trackedPathSpecs = await listTrackedPathSpecs(worktreePath, filePaths, options) - const trackedPaths = filePaths.filter((filePath) => isTrackedPathSpec(filePath, trackedPathSpecs)) - const untrackedPaths = filePaths.filter( - (filePath) => !isTrackedPathSpec(filePath, trackedPathSpecs) - ) - await removeSafeUntrackedDiscardTargets( - worktreePath, - untrackedPaths, - (targetPaths) => cleanUntrackedPaths(worktreePath, targetPaths, options), - async () => { - for (let i = 0; i < trackedPaths.length; i += BULK_CHUNK_SIZE) { - const chunk = trackedPaths.slice(i, i + BULK_CHUNK_SIZE) - await gitExecFileAsync( - ['restore', '--worktree', '--source=HEAD', '--', ...chunk.map(literalPathspec)], - { - ...gitOptionsForWorktree(worktreePath, options) - } - ) + try { + const resolvedWorktree = path.resolve(worktreePath) + for (const filePath of filePaths) { + const resolvedTarget = path.resolve(worktreePath, filePath) + if (!isWithinWorktree(path, resolvedWorktree, resolvedTarget)) { + throw new Error(`Path "${filePath}" resolves outside the worktree`) } } - ) + + const trackedPathSpecs = await listTrackedPathSpecs(worktreePath, filePaths, options) + const trackedPaths = filePaths.filter((filePath) => + isTrackedPathSpec(filePath, trackedPathSpecs) + ) + const untrackedPaths = filePaths.filter( + (filePath) => !isTrackedPathSpec(filePath, trackedPathSpecs) + ) + await removeSafeUntrackedDiscardTargets( + worktreePath, + untrackedPaths, + (targetPaths) => cleanUntrackedPaths(worktreePath, targetPaths, options), + async () => { + for (let i = 0; i < trackedPaths.length; i += BULK_CHUNK_SIZE) { + const chunk = trackedPaths.slice(i, i + BULK_CHUNK_SIZE) + await gitExecFileAsync( + ['restore', '--worktree', '--source=HEAD', '--', ...chunk.map(literalPathspec)], + { + ...gitOptionsForWorktree(worktreePath, options) + } + ) + } + } + ) + } finally { + gitDiffReadDedupe.clear() + } } export function isWithinWorktree( @@ -1574,15 +1675,20 @@ export async function bulkStageFiles( filePaths: string[], options: GitRuntimeOptions = {} ): Promise { + gitDiffReadDedupe.clear() if (filePaths.length === 0) { return } - for (let i = 0; i < filePaths.length; i += BULK_CHUNK_SIZE) { - const chunk = filePaths.slice(i, i + BULK_CHUNK_SIZE) - await gitExecFileAsync( - ['add', '--', ...chunk.map(literalPathspec)], - gitOptionsForWorktree(worktreePath, options) - ) + try { + for (let i = 0; i < filePaths.length; i += BULK_CHUNK_SIZE) { + const chunk = filePaths.slice(i, i + BULK_CHUNK_SIZE) + await gitExecFileAsync( + ['add', '--', ...chunk.map(literalPathspec)], + gitOptionsForWorktree(worktreePath, options) + ) + } + } finally { + gitDiffReadDedupe.clear() } } @@ -1594,13 +1700,18 @@ export async function bulkUnstageFiles( filePaths: string[], options: GitRuntimeOptions = {} ): Promise { + gitDiffReadDedupe.clear() if (filePaths.length === 0) { return } - for (let i = 0; i < filePaths.length; i += BULK_CHUNK_SIZE) { - const chunk = filePaths.slice(i, i + BULK_CHUNK_SIZE) - await gitExecFileAsync(['restore', '--staged', '--', ...chunk.map(literalPathspec)], { - ...gitOptionsForWorktree(worktreePath, options) - }) + try { + for (let i = 0; i < filePaths.length; i += BULK_CHUNK_SIZE) { + const chunk = filePaths.slice(i, i + BULK_CHUNK_SIZE) + await gitExecFileAsync(['restore', '--staged', '--', ...chunk.map(literalPathspec)], { + ...gitOptionsForWorktree(worktreePath, options) + }) + } + } finally { + gitDiffReadDedupe.clear() } } diff --git a/src/main/providers/ssh-git-provider.test.ts b/src/main/providers/ssh-git-provider.test.ts index a6fd5911f..01736fb2d 100644 --- a/src/main/providers/ssh-git-provider.test.ts +++ b/src/main/providers/ssh-git-provider.test.ts @@ -31,6 +31,14 @@ async function waitForRequestCount(mock: ReturnType, count: number } } +function deferredValue(value: T): { promise: Promise; resolve: () => void } { + let resolve!: (value: T) => void + const promise = new Promise((innerResolve) => { + resolve = innerResolve + }) + return { promise, resolve: () => resolve(value) } +} + describe('SshGitProvider', () => { let mux: MockMultiplexer let provider: SshGitProvider @@ -828,6 +836,272 @@ describe('SshGitProvider', () => { expect(result).toEqual(diffs) }) + it('coalesces concurrent identical diff RPCs while in flight', async () => { + const diff = { + kind: 'text', + originalContent: 'old', + modifiedContent: 'new', + originalIsBinary: false, + modifiedIsBinary: false + } + const pendingDiff = deferredValue(diff) + mux.request.mockReturnValue(pendingDiff.promise) + + const reads = Array.from({ length: 8 }, () => + provider.getDiff('/home/user/repo', 'src/file.ts', false, true) + ) + + await waitForRequestCount(mux.request, 1) + expect(mux.request).toHaveBeenCalledTimes(1) + pendingDiff.resolve() + + await expect(Promise.all(reads)).resolves.toEqual(Array(8).fill(diff)) + + mux.request.mockReset() + const branchDiffs = [diff] + const pendingBranchDiff = deferredValue(branchDiffs) + mux.request.mockReturnValue(pendingBranchDiff.promise) + + const branchReads = Array.from({ length: 8 }, () => + provider.getBranchDiff('/home/user/repo', 'main', { + includePatch: true, + filePath: 'src/file.ts' + }) + ) + + await waitForRequestCount(mux.request, 1) + expect(mux.request).toHaveBeenCalledTimes(1) + pendingBranchDiff.resolve() + await expect(Promise.all(branchReads)).resolves.toEqual(Array(8).fill(branchDiffs)) + + mux.request.mockReset() + const pendingCommitDiff = deferredValue(diff) + mux.request.mockReturnValue(pendingCommitDiff.promise) + + const commitReads = Array.from({ length: 8 }, () => + provider.getCommitDiff('/home/user/repo', { + commitOid: 'c'.repeat(40), + parentOid: 'b'.repeat(40), + filePath: 'src/file.ts' + }) + ) + + await waitForRequestCount(mux.request, 1) + expect(mux.request).toHaveBeenCalledTimes(1) + pendingCommitDiff.resolve() + await expect(Promise.all(commitReads)).resolves.toEqual(Array(8).fill(diff)) + }) + + it('retries diff RPCs after an in-flight rejection settles', async () => { + const failure = new Error('transient relay failure') + mux.request.mockRejectedValueOnce(failure) + + const firstBurst = Array.from({ length: 8 }, () => + provider.getDiff('/home/user/repo', 'src/file.ts', false, true) + ) + + await expect(Promise.all(firstBurst)).rejects.toThrow('transient relay failure') + expect(mux.request).toHaveBeenCalledTimes(1) + + const diff = { + kind: 'text', + originalContent: 'old', + modifiedContent: 'new', + originalIsBinary: false, + modifiedIsBinary: false + } + mux.request.mockResolvedValueOnce(diff) + + await expect(provider.getDiff('/home/user/repo', 'src/file.ts', false, true)).resolves.toBe( + diff + ) + expect(mux.request).toHaveBeenCalledTimes(2) + }) + + it('clears pending diff RPCs when status runs', async () => { + 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({ entries: [], conflictOperation: 'unknown' }) + await provider.getStatus('/home/user/repo') + + 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 diff RPCs when a mutation runs', async () => { + 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(undefined) + await provider.stageFile('/home/user/repo', 'src/file.ts') + + 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', + originalContent: 'old', + modifiedContent: 'new', + originalIsBinary: false, + modifiedIsBinary: false + } + const pendingDiff = deferredValue([diff]) + mux.request.mockReturnValueOnce(pendingDiff.promise) + + const first = provider.getBranchDiff('/home/user/repo', 'origin/main', { + includePatch: true, + filePath: 'src/file.ts' + }) + await waitForRequestCount(mux.request, 1) + + mux.request.mockResolvedValueOnce(undefined) + await provider.fetchRemoteTrackingRef( + '/home/user/repo', + 'origin', + 'main', + 'refs/remotes/origin/main' + ) + + mux.request.mockResolvedValueOnce([diff]) + const second = provider.getBranchDiff('/home/user/repo', 'origin/main', { + includePatch: true, + filePath: 'src/file.ts' + }) + + pendingDiff.resolve() + await expect(Promise.all([first, second])).resolves.toEqual([[diff], [diff]]) + expect(mux.request).toHaveBeenCalledTimes(3) + }) + + it('coalesces logically identical branch and commit diff RPC args regardless of property order', async () => { + const diff = { + kind: 'text', + originalContent: 'old', + modifiedContent: 'new', + originalIsBinary: false, + modifiedIsBinary: false + } + mux.request.mockResolvedValue([diff]) + + await Promise.all([ + provider.getBranchDiff('/home/user/repo', 'main', { + includePatch: true, + filePath: 'src/file.ts', + oldPath: 'src/old-file.ts' + }), + provider.getBranchDiff('/home/user/repo', 'main', { + oldPath: 'src/old-file.ts', + filePath: 'src/file.ts', + includePatch: true + }) + ]) + + expect(mux.request).toHaveBeenCalledTimes(1) + + mux.request.mockReset() + mux.request.mockResolvedValue(diff) + + await Promise.all([ + provider.getCommitDiff('/home/user/repo', { + commitOid: 'c'.repeat(40), + parentOid: 'b'.repeat(40), + filePath: 'src/file.ts', + oldPath: 'src/old-file.ts' + }), + provider.getCommitDiff('/home/user/repo', { + oldPath: 'src/old-file.ts', + filePath: 'src/file.ts', + parentOid: 'b'.repeat(40), + commitOid: 'c'.repeat(40) + }) + ]) + + expect(mux.request).toHaveBeenCalledTimes(1) + }) + + it('keeps distinct diff RPC keys independent', async () => { + const diff = { + kind: 'text', + originalContent: 'old', + modifiedContent: 'new', + originalIsBinary: false, + modifiedIsBinary: false + } + mux.request.mockResolvedValue(diff) + + await Promise.all([ + provider.getDiff('/home/user/repo', 'src/file.ts', false, false), + provider.getDiff('/home/user/repo', 'src/file.ts', true, false), + provider.getDiff('/home/user/repo', 'src/file.ts', false, true), + provider.getBranchDiff('/home/user/repo', 'main', { + includePatch: true, + filePath: 'src/file.ts' + }), + provider.getBranchDiff('/home/user/repo', 'main', { + includePatch: false, + filePath: 'src/file.ts' + }), + provider.getBranchDiff('/home/user/repo', 'main', { + includePatch: true, + filePath: 'src/file.ts', + oldPath: 'src/old-file.ts' + }), + provider.getBranchDiff('/home/user/repo', 'develop', { + includePatch: true, + filePath: 'src/file.ts' + }), + provider.getCommitDiff('/home/user/repo', { + commitOid: 'c'.repeat(40), + parentOid: 'b'.repeat(40), + filePath: 'src/file.ts' + }), + provider.getCommitDiff('/home/user/repo', { + commitOid: 'c'.repeat(40), + parentOid: 'a'.repeat(40), + filePath: 'src/file.ts' + }), + provider.getCommitDiff('/home/user/repo', { + commitOid: 'c'.repeat(40), + parentOid: 'b'.repeat(40), + filePath: 'src/file.ts', + oldPath: 'src/old-file.ts' + }) + ]) + + expect(mux.request).toHaveBeenCalledTimes(10) + }) + it('listWorktrees sends git.listWorktrees request', async () => { const worktrees = [ { diff --git a/src/main/providers/ssh-git-provider.ts b/src/main/providers/ssh-git-provider.ts index 9c93c46e6..751ebc1ef 100644 --- a/src/main/providers/ssh-git-provider.ts +++ b/src/main/providers/ssh-git-provider.ts @@ -28,6 +28,7 @@ import { describeMaxBufferOverflowError, isMaxBufferOverflowError } from '../git/max-buffer-overflow' +import { InFlightPromiseDedupe, stableInFlightKey } from '../../shared/in-flight-promise-dedupe' type NonInteractiveExecQueueEntry = { started: boolean @@ -58,9 +59,22 @@ function filterUntrackedPorcelainStatus(stdout: string | undefined): string | un } export class SshGitProvider implements IGitProvider { + private readonly gitDiffReadDedupe = new InFlightPromiseDedupe() + private connectionId: string private mux: SshChannelMultiplexer private nonInteractiveExecQueues = new Map() + + 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. + this.gitDiffReadDedupe.clear() + try { + return await run() + } finally { + this.gitDiffReadDedupe.clear() + } + } private loggedWorktreeIsCleanFallback = false constructor( @@ -84,6 +98,7 @@ export class SshGitProvider implements IGitProvider { worktreePath: string, options?: GitProviderStatusOptions ): Promise { + this.gitDiffReadDedupe.clear() const includeIgnoredArgs = options?.includeIgnored ? { includeIgnored: true } : {} const upstreamCacheBypassArgs = options?.bypassEffectiveUpstreamNegativeCache ? { bypassEffectiveUpstreamNegativeCache: true } @@ -116,10 +131,13 @@ export class SshGitProvider implements IGitProvider { worktreePath: string, message: string ): Promise<{ success: boolean; error?: string }> { - return (await this.mux.request('git.commit', { - worktreePath, - message - })) as { success: boolean; error?: string } + return this.runWithDiffDedupeClear( + async () => + (await this.mux.request('git.commit', { + worktreePath, + message + })) as { success: boolean; error?: string } + ) } async getStagedCommitContext(worktreePath: string): Promise { @@ -321,36 +339,70 @@ export class SshGitProvider implements IGitProvider { staged: boolean, compareAgainstHead?: boolean ): Promise { - return (await this.mux.request('git.diff', { - worktreePath, - filePath, - staged, - compareAgainstHead - })) as GitDiffResult + return this.gitDiffReadDedupe.run( + stableInFlightKey(['diff', worktreePath, filePath, staged, compareAgainstHead]), + async () => + (await this.mux.request('git.diff', { + worktreePath, + filePath, + staged, + compareAgainstHead + })) as GitDiffResult + ) as Promise } async stageFile(worktreePath: string, filePath: string): Promise { - await this.mux.request('git.stage', { worktreePath, filePath }) + this.gitDiffReadDedupe.clear() + try { + await this.mux.request('git.stage', { worktreePath, filePath }) + } finally { + this.gitDiffReadDedupe.clear() + } } async unstageFile(worktreePath: string, filePath: string): Promise { - await this.mux.request('git.unstage', { worktreePath, filePath }) + this.gitDiffReadDedupe.clear() + try { + await this.mux.request('git.unstage', { worktreePath, filePath }) + } finally { + this.gitDiffReadDedupe.clear() + } } async bulkStageFiles(worktreePath: string, filePaths: string[]): Promise { - await this.mux.request('git.bulkStage', { worktreePath, filePaths }) + this.gitDiffReadDedupe.clear() + try { + await this.mux.request('git.bulkStage', { worktreePath, filePaths }) + } finally { + this.gitDiffReadDedupe.clear() + } } async bulkUnstageFiles(worktreePath: string, filePaths: string[]): Promise { - await this.mux.request('git.bulkUnstage', { worktreePath, filePaths }) + this.gitDiffReadDedupe.clear() + try { + await this.mux.request('git.bulkUnstage', { worktreePath, filePaths }) + } finally { + this.gitDiffReadDedupe.clear() + } } async discardChanges(worktreePath: string, filePath: string): Promise { - await this.mux.request('git.discard', { worktreePath, filePath }) + this.gitDiffReadDedupe.clear() + try { + await this.mux.request('git.discard', { worktreePath, filePath }) + } finally { + this.gitDiffReadDedupe.clear() + } } async bulkDiscardChanges(worktreePath: string, filePaths: string[]): Promise { - await this.mux.request('git.bulkDiscard', { worktreePath, filePaths }) + this.gitDiffReadDedupe.clear() + try { + await this.mux.request('git.bulkDiscard', { worktreePath, filePaths }) + } finally { + this.gitDiffReadDedupe.clear() + } } async detectConflictOperation(worktreePath: string): Promise { @@ -360,15 +412,21 @@ export class SshGitProvider implements IGitProvider { } async abortMerge(worktreePath: string): Promise { - await this.mux.request('git.abortMerge', { worktreePath }) + await this.runWithDiffDedupeClear(async () => { + await this.mux.request('git.abortMerge', { worktreePath }) + }) } async abortRebase(worktreePath: string): Promise { - await this.mux.request('git.abortRebase', { worktreePath }) + await this.runWithDiffDedupeClear(async () => { + await this.mux.request('git.abortRebase', { worktreePath }) + }) } async checkoutBranch(worktreePath: string, branch: string): Promise { - await this.mux.request('git.checkout', { worktreePath, branch }) + await this.runWithDiffDedupeClear(async () => { + await this.mux.request('git.checkout', { worktreePath, branch }) + }) } async listLocalBranches( @@ -410,41 +468,54 @@ export class SshGitProvider implements IGitProvider { pushTarget?: GitPushTarget, options: { forceWithLease?: boolean } = {} ): Promise { - await this.mux.request('git.push', { - worktreePath, - publish, - pushTarget, - ...(options.forceWithLease === true ? { forceWithLease: true } : {}) + await this.runWithDiffDedupeClear(async () => { + await this.mux.request('git.push', { + worktreePath, + publish, + pushTarget, + ...(options.forceWithLease === true ? { forceWithLease: true } : {}) + }) }) } async pullBranch(worktreePath: string, pushTarget?: GitPushTarget): Promise { - await this.mux.request('git.pull', { worktreePath, ...(pushTarget ? { pushTarget } : {}) }) + await this.runWithDiffDedupeClear(async () => { + await this.mux.request('git.pull', { worktreePath, ...(pushTarget ? { pushTarget } : {}) }) + }) } async fastForwardBranch(worktreePath: string, pushTarget?: GitPushTarget): Promise { - await this.mux.request('git.fastForward', { - worktreePath, - ...(pushTarget ? { pushTarget } : {}) + await this.runWithDiffDedupeClear(async () => { + await this.mux.request('git.fastForward', { + worktreePath, + ...(pushTarget ? { pushTarget } : {}) + }) }) } async rebaseFromBase(worktreePath: string, baseRef: string): Promise { - await this.mux.request('git.rebaseFromBase', { worktreePath, baseRef }) + await this.runWithDiffDedupeClear(async () => { + await this.mux.request('git.rebaseFromBase', { worktreePath, baseRef }) + }) } async fetchRemote(worktreePath: string, pushTarget?: GitPushTarget): Promise { - await this.mux.request('git.fetch', { worktreePath, ...(pushTarget ? { pushTarget } : {}) }) + await this.runWithDiffDedupeClear(async () => { + await this.mux.request('git.fetch', { worktreePath, ...(pushTarget ? { pushTarget } : {}) }) + }) } async syncForkDefaultBranch( worktreePath: string, expectedUpstream: GitForkSyncExpectedUpstream ): Promise { - return (await this.mux.request('git.forkSync', { - worktreePath, - ...(expectedUpstream ? { expectedUpstream } : {}) - })) as GitForkSyncResult + return this.runWithDiffDedupeClear( + async () => + (await this.mux.request('git.forkSync', { + worktreePath, + ...(expectedUpstream ? { expectedUpstream } : {}) + })) as GitForkSyncResult + ) } async fetchRemoteTrackingRef( @@ -453,11 +524,13 @@ export class SshGitProvider implements IGitProvider { branch: string, ref: string ): Promise { - await this.mux.request('git.fetchRemoteTrackingRef', { - worktreePath, - remote, - branch, - ref + await this.runWithDiffDedupeClear(async () => { + await this.mux.request('git.fetchRemoteTrackingRef', { + worktreePath, + remote, + branch, + ref + }) }) } @@ -466,10 +539,12 @@ export class SshGitProvider implements IGitProvider { remote: string, mrIid: number ): Promise { - await this.mux.request('git.fetchGitLabMergeRequestHead', { - worktreePath, - remote, - mrIid + await this.runWithDiffDedupeClear(async () => { + await this.mux.request('git.fetchGitLabMergeRequestHead', { + worktreePath, + remote, + mrIid + }) }) } @@ -478,21 +553,44 @@ export class SshGitProvider implements IGitProvider { baseRef: string, options?: { includePatch?: boolean; filePath?: string; oldPath?: string } ): Promise { - return (await this.mux.request('git.branchDiff', { - worktreePath, - baseRef, - ...options - })) as GitDiffResult[] + const keyOptions = options ?? {} + return this.gitDiffReadDedupe.run( + stableInFlightKey([ + 'branchDiff', + worktreePath, + baseRef, + keyOptions.includePatch ?? null, + keyOptions.filePath ?? null, + keyOptions.oldPath ?? null + ]), + async () => + (await this.mux.request('git.branchDiff', { + worktreePath, + baseRef, + ...options + })) as GitDiffResult[] + ) as Promise } async getCommitDiff( worktreePath: string, args: { commitOid: string; parentOid?: string | null; filePath: string; oldPath?: string } ): Promise { - return (await this.mux.request('git.commitDiff', { - worktreePath, - ...args - })) as GitDiffResult + return this.gitDiffReadDedupe.run( + stableInFlightKey([ + 'commitDiff', + worktreePath, + args.commitOid, + args.parentOid ?? null, + args.filePath, + args.oldPath ?? null + ]), + async () => + (await this.mux.request('git.commitDiff', { + worktreePath, + ...args + })) as GitDiffResult + ) as Promise } async listWorktrees( @@ -514,11 +612,13 @@ export class SshGitProvider implements IGitProvider { targetDir: string, options?: { base?: string; checkoutExistingBranch?: boolean; noCheckout?: boolean } ): Promise { - await this.mux.request('git.addWorktree', { - repoPath, - branchName, - targetDir, - ...options + await this.runWithDiffDedupeClear(async () => { + await this.mux.request('git.addWorktree', { + repoPath, + branchName, + targetDir, + ...options + }) }) } @@ -527,11 +627,14 @@ export class SshGitProvider implements IGitProvider { force?: boolean, options?: { deleteBranch?: boolean; forceBranchDelete?: boolean } ): Promise { - return ((await this.mux.request('git.removeWorktree', { - worktreePath, - force, - ...options - })) ?? {}) as RemoveWorktreeResult + return this.runWithDiffDedupeClear( + async () => + ((await this.mux.request('git.removeWorktree', { + worktreePath, + force, + ...options + })) ?? {}) as RemoveWorktreeResult + ) } async worktreeIsClean( @@ -583,11 +686,15 @@ export class SshGitProvider implements IGitProvider { ownerWorktreePath?: string checkOnly?: boolean }): Promise { - await this.mux.request('git.refreshLocalBaseRefForWorktreeCreate', args) + await this.runWithDiffDedupeClear(async () => { + await this.mux.request('git.refreshLocalBaseRefForWorktreeCreate', args) + }) } async renameCurrentBranch(worktreePath: string, newBranch: string): Promise { - await this.mux.request('git.renameCurrentBranch', { worktreePath, newBranch }) + await this.runWithDiffDedupeClear(async () => { + await this.mux.request('git.renameCurrentBranch', { worktreePath, newBranch }) + }) } async exec( diff --git a/src/relay/git-handler.test.ts b/src/relay/git-handler.test.ts index c2a8eab2b..8e1c3ba65 100644 --- a/src/relay/git-handler.test.ts +++ b/src/relay/git-handler.test.ts @@ -19,6 +19,37 @@ import { type RelayDispatcher } from './git-handler-test-setup' +type GitBufferSpyTarget = { + gitBuffer(args: string[], cwd: string): Promise +} + +type GitSpyTarget = { + git(args: string[], cwd: string): Promise<{ stdout: string; stderr: string }> +} + +function deferredRelayBuffer(content: string): { + promise: Promise + resolve: () => void +} { + let resolve!: (value: Buffer) => void + const promise = new Promise((innerResolve) => { + resolve = innerResolve + }) + return { + promise, + resolve: () => resolve(Buffer.from(content)) + } +} + +async function waitForSpyCalls(mock: ReturnType, calls: number): Promise { + for (let i = 0; i < 20; i++) { + if (mock.mock.calls.length >= calls) { + return + } + await new Promise((resolve) => setTimeout(resolve, 0)) + } +} + describe('GitHandler', () => { let dispatcher: MockDispatcher let handler: GitHandler @@ -830,6 +861,382 @@ describe('GitHandler', () => { }) describe('branchDiff', () => { + it('coalesces concurrent identical git.diff reads while in flight and reads fresh after settle', async () => { + const leftBlob = deferredRelayBuffer('left\n') + const rightBlob = deferredRelayBuffer('right\n') + const pendingBuffers = [leftBlob, rightBlob] + const gitBufferSpy = vi + .spyOn(handler as unknown as GitBufferSpyTarget, 'gitBuffer') + .mockImplementation(async () => pendingBuffers.shift()!.promise) + + const reads = Array.from({ length: 8 }, () => + dispatcher.callRequest('git.diff', { + worktreePath: tmpDir, + filePath: 'src/file.ts', + staged: true + }) + ) + + await waitForSpyCalls(gitBufferSpy, 1) + leftBlob.resolve() + await waitForSpyCalls(gitBufferSpy, 2) + rightBlob.resolve() + + await Promise.all(reads) + + expect(gitBufferSpy).toHaveBeenCalledTimes(2) + + gitBufferSpy + .mockResolvedValueOnce(Buffer.from('fresh-left\n')) + .mockResolvedValueOnce(Buffer.from('fresh-right\n')) + + await dispatcher.callRequest('git.diff', { + worktreePath: tmpDir, + filePath: 'src/file.ts', + staged: true + }) + + expect(gitBufferSpy).toHaveBeenCalledTimes(4) + }) + + it('clears pending git.diff reads when status runs', async () => { + const firstBlob = deferredRelayBuffer('left\n') + const secondBlob = deferredRelayBuffer('fresh-left\n') + const pendingBuffers = [firstBlob, secondBlob] + const gitBufferSpy = vi + .spyOn(handler as unknown as GitBufferSpyTarget, 'gitBuffer') + .mockImplementation(async () => pendingBuffers.shift()!.promise) + const gitSpy = vi + .spyOn(handler as unknown as GitSpyTarget, 'git') + .mockResolvedValue({ stdout: '', stderr: '' }) + + const first = dispatcher.callRequest('git.diff', { + worktreePath: tmpDir, + filePath: 'src/file.ts', + staged: false + }) + await waitForSpyCalls(gitBufferSpy, 1) + + await dispatcher.callRequest('git.status', { worktreePath: tmpDir }) + + const second = dispatcher.callRequest('git.diff', { + worktreePath: tmpDir, + filePath: 'src/file.ts', + staged: false + }) + await waitForSpyCalls(gitBufferSpy, 2) + + firstBlob.resolve() + secondBlob.resolve() + await Promise.all([first, second]) + + expect(gitBufferSpy).toHaveBeenCalledTimes(2) + expect(gitSpy).toHaveBeenCalled() + }) + + it('clears pending git.diff reads when a mutation runs', async () => { + const firstBlob = deferredRelayBuffer('left\n') + const secondBlob = deferredRelayBuffer('fresh-left\n') + const pendingBuffers = [firstBlob, secondBlob] + const gitBufferSpy = vi + .spyOn(handler as unknown as GitBufferSpyTarget, 'gitBuffer') + .mockImplementation(async () => pendingBuffers.shift()!.promise) + const gitSpy = vi + .spyOn(handler as unknown as GitSpyTarget, 'git') + .mockResolvedValue({ stdout: '', stderr: '' }) + + const first = dispatcher.callRequest('git.diff', { + worktreePath: tmpDir, + filePath: 'src/file.ts', + staged: false + }) + await waitForSpyCalls(gitBufferSpy, 1) + + await dispatcher.callRequest('git.stage', { worktreePath: tmpDir, filePath: 'src/file.ts' }) + + const second = dispatcher.callRequest('git.diff', { + worktreePath: tmpDir, + filePath: 'src/file.ts', + staged: false + }) + await waitForSpyCalls(gitBufferSpy, 2) + + firstBlob.resolve() + secondBlob.resolve() + await Promise.all([first, second]) + + expect(gitBufferSpy).toHaveBeenCalledTimes(2) + expect(gitSpy).toHaveBeenCalledWith(['add', '--', 'src/file.ts'], tmpDir) + }) + + it('clears pending git.diff reads when a narrow ref fetch runs', async () => { + const firstBlob = deferredRelayBuffer('left\n') + const secondBlob = deferredRelayBuffer('fresh-left\n') + const pendingBuffers = [firstBlob, secondBlob] + const gitBufferSpy = vi + .spyOn(handler as unknown as GitBufferSpyTarget, 'gitBuffer') + .mockImplementation(async () => pendingBuffers.shift()!.promise) + const gitSpy = vi + .spyOn(handler as unknown as GitSpyTarget, 'git') + .mockImplementation(async (args: string[]) => { + if (args[0] === 'remote') { + return { stdout: 'origin\n', stderr: '' } + } + return { stdout: '', stderr: '' } + }) + + const first = dispatcher.callRequest('git.diff', { + worktreePath: tmpDir, + filePath: 'src/file.ts', + staged: false + }) + await waitForSpyCalls(gitBufferSpy, 1) + + await dispatcher.callRequest('git.fetchRemoteTrackingRef', { + worktreePath: tmpDir, + remote: 'origin', + branch: 'main', + ref: 'refs/remotes/origin/main' + }) + + const second = dispatcher.callRequest('git.diff', { + worktreePath: tmpDir, + filePath: 'src/file.ts', + staged: false + }) + await waitForSpyCalls(gitBufferSpy, 2) + + firstBlob.resolve() + secondBlob.resolve() + await Promise.all([first, second]) + + expect(gitBufferSpy).toHaveBeenCalledTimes(2) + expect(gitSpy).toHaveBeenCalledWith( + ['fetch', '--no-tags', 'origin', '+refs/heads/main:refs/remotes/origin/main'], + tmpDir + ) + }) + + it('coalesces concurrent identical git.branchDiff reads while in flight', async () => { + const gitSpy = vi + .spyOn(handler as unknown as GitSpyTarget, 'git') + .mockImplementation(async (args: string[]) => { + if (args[0] === 'rev-parse' && args.includes('HEAD')) { + return { stdout: `${'c'.repeat(40)}\n`, stderr: '' } + } + if (args[0] === 'rev-parse') { + return { stdout: `${'b'.repeat(40)}\n`, stderr: '' } + } + if (args[0] === 'merge-base') { + return { stdout: `${'a'.repeat(40)}\n`, stderr: '' } + } + if (args.includes('--name-status')) { + return { stdout: 'M\tsrc/file.ts\n', stderr: '' } + } + throw new Error(`unexpected git args: ${args.join(' ')}`) + }) + const leftBlob = deferredRelayBuffer('left\n') + const rightBlob = deferredRelayBuffer('right\n') + const pendingBuffers = [leftBlob, rightBlob] + const gitBufferSpy = vi + .spyOn(handler as unknown as GitBufferSpyTarget, 'gitBuffer') + .mockImplementation(async () => pendingBuffers.shift()!.promise) + + const reads = Array.from({ length: 8 }, () => + dispatcher.callRequest('git.branchDiff', { + worktreePath: tmpDir, + baseRef: 'main', + includePatch: true, + filePath: 'src/file.ts' + }) + ) + + await waitForSpyCalls(gitBufferSpy, 1) + leftBlob.resolve() + await waitForSpyCalls(gitBufferSpy, 2) + rightBlob.resolve() + + await Promise.all(reads) + + expect(gitBufferSpy).toHaveBeenCalledTimes(2) + expect(gitSpy).toHaveBeenCalledTimes(4) + }) + + it('coalesces concurrent identical git.commitDiff reads while in flight', async () => { + const leftBlob = deferredRelayBuffer('left\n') + const rightBlob = deferredRelayBuffer('right\n') + const pendingBuffers = [leftBlob, rightBlob] + const gitBufferSpy = vi + .spyOn(handler as unknown as GitBufferSpyTarget, 'gitBuffer') + .mockImplementation(async () => pendingBuffers.shift()!.promise) + + const reads = Array.from({ length: 8 }, () => + dispatcher.callRequest('git.commitDiff', { + worktreePath: tmpDir, + commitOid: 'c'.repeat(40), + parentOid: 'b'.repeat(40), + filePath: 'src/file.ts' + }) + ) + + await waitForSpyCalls(gitBufferSpy, 1) + leftBlob.resolve() + await waitForSpyCalls(gitBufferSpy, 2) + rightBlob.resolve() + + await Promise.all(reads) + + expect(gitBufferSpy).toHaveBeenCalledTimes(2) + }) + + it('coalesces parentless root git.commitDiff reads without a left-side blob', async () => { + const rightBlob = deferredRelayBuffer('right\n') + const gitBufferSpy = vi + .spyOn(handler as unknown as GitBufferSpyTarget, 'gitBuffer') + .mockImplementation(async () => rightBlob.promise) + + const reads = Array.from({ length: 8 }, () => + dispatcher.callRequest('git.commitDiff', { + worktreePath: tmpDir, + commitOid: 'c'.repeat(40), + parentOid: null, + filePath: 'src/file.ts' + }) + ) + + await waitForSpyCalls(gitBufferSpy, 1) + rightBlob.resolve() + await Promise.all(reads) + + expect(gitBufferSpy).toHaveBeenCalledTimes(1) + }) + + it('keeps distinct relay diff keys independent', async () => { + const gitBufferSpy = vi + .spyOn(handler as unknown as GitBufferSpyTarget, 'gitBuffer') + .mockResolvedValue(Buffer.from('blob\n')) + + await Promise.all([ + dispatcher.callRequest('git.diff', { + worktreePath: tmpDir, + filePath: 'src/file.ts', + staged: true + }), + dispatcher.callRequest('git.diff', { + worktreePath: tmpDir, + filePath: 'src/file.ts', + staged: false, + compareAgainstHead: true + }) + ]) + + expect(gitBufferSpy).toHaveBeenCalledTimes(3) + + gitBufferSpy.mockClear() + const gitSpy = vi + .spyOn(handler as unknown as GitSpyTarget, 'git') + .mockImplementation(async (args: string[]) => { + if (args[0] === 'rev-parse' && args.includes('HEAD')) { + return { stdout: `${'c'.repeat(40)}\n`, stderr: '' } + } + if (args[0] === 'rev-parse' && args.includes('develop')) { + return { stdout: `${'d'.repeat(40)}\n`, stderr: '' } + } + if (args[0] === 'rev-parse') { + return { stdout: `${'b'.repeat(40)}\n`, stderr: '' } + } + if (args[0] === 'merge-base' && args.includes('d'.repeat(40))) { + return { stdout: `${'e'.repeat(40)}\n`, stderr: '' } + } + if (args[0] === 'merge-base') { + return { stdout: `${'a'.repeat(40)}\n`, stderr: '' } + } + if (args.includes('--name-status')) { + return { stdout: 'M\tsrc/file.ts\n', stderr: '' } + } + throw new Error(`unexpected git args: ${args.join(' ')}`) + }) + + await Promise.all([ + dispatcher.callRequest('git.branchDiff', { + worktreePath: tmpDir, + baseRef: 'main', + includePatch: true, + filePath: 'src/file.ts' + }), + dispatcher.callRequest('git.branchDiff', { + worktreePath: tmpDir, + baseRef: 'main', + includePatch: false, + filePath: 'src/file.ts' + }), + dispatcher.callRequest('git.branchDiff', { + worktreePath: tmpDir, + baseRef: 'develop', + includePatch: true, + filePath: 'src/file.ts' + }) + ]) + + expect(gitSpy).toHaveBeenCalledTimes(12) + expect(gitBufferSpy).toHaveBeenCalledTimes(4) + + gitBufferSpy.mockClear() + + await Promise.all([ + dispatcher.callRequest('git.commitDiff', { + worktreePath: tmpDir, + commitOid: 'c'.repeat(40), + parentOid: 'b'.repeat(40), + filePath: 'src/file.ts' + }), + dispatcher.callRequest('git.commitDiff', { + worktreePath: tmpDir, + commitOid: 'c'.repeat(40), + parentOid: 'a'.repeat(40), + filePath: 'src/file.ts' + }), + dispatcher.callRequest('git.commitDiff', { + worktreePath: tmpDir, + commitOid: 'c'.repeat(40), + parentOid: 'b'.repeat(40), + filePath: 'src/file.ts', + oldPath: 'src/old-a.ts' + }), + dispatcher.callRequest('git.commitDiff', { + worktreePath: tmpDir, + commitOid: 'c'.repeat(40), + parentOid: 'b'.repeat(40), + filePath: 'src/file.ts', + oldPath: 'src/old-b.ts' + }) + ]) + + expect(gitBufferSpy).toHaveBeenCalledTimes(8) + }) + + it('retries relay diff reads after an in-flight rejection settles', async () => { + const invalidRequest = { + worktreePath: tmpDir, + commitOid: 'not-a-full-oid', + parentOid: 'b'.repeat(40), + filePath: 'src/file.ts' + } + const first = dispatcher.callRequest('git.commitDiff', invalidRequest) + const firstBurst = [ + first, + ...Array.from({ length: 7 }, () => dispatcher.callRequest('git.commitDiff', invalidRequest)) + ] + + await expect(Promise.all(firstBurst)).rejects.toThrow( + 'commitOid must be a full git object id' + ) + + const retry = dispatcher.callRequest('git.commitDiff', invalidRequest) + expect(retry).not.toBe(first) + await expect(retry).rejects.toThrow('commitOid must be a full git object id') + }) + // Why: regression for issue #1503 on git.branchDiff. The branchCompare test // covers loadBranchChanges in git-handler.ts, but branchDiffEntries in // git-handler-ops.ts is a separate code path that also passes diff --git a/src/relay/git-handler.ts b/src/relay/git-handler.ts index 46dd750f2..e3310f969 100644 --- a/src/relay/git-handler.ts +++ b/src/relay/git-handler.ts @@ -46,6 +46,7 @@ import { } from '../shared/git-discard-path-safety' import { getGitCloneFailureMessage } from '../shared/git-clone-failure-message' import { syncForkDefaultBranch, validateGitForkSyncExpectedUpstream } from '../shared/git-fork-sync' +import { InFlightPromiseDedupe, stableInFlightKey } from '../shared/in-flight-promise-dedupe' const execFileAsync = promisify(execFile) const MAX_GIT_BUFFER = 10 * 1024 * 1024 @@ -88,6 +89,7 @@ function execFileWithStdin( export class GitHandler { private dispatcher: RelayDispatcher + private readonly gitDiffReadDedupe = new InFlightPromiseDedupe() // Why: RelayContext is accepted for protocol back-compat (see // docs/relay-fs-allowlist-removal.md) but no longer consulted on git ops. @@ -141,6 +143,17 @@ export class GitHandler { this.dispatcher.onRequest('git.isGitRepo', (p) => this.isGitRepo(p)) } + 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. + this.gitDiffReadDedupe.clear() + try { + return await run() + } finally { + this.gitDiffReadDedupe.clear() + } + } + private async git( args: string[], cwd: string, @@ -187,6 +200,7 @@ export class GitHandler { } private async getStatus(params: Record) { + this.gitDiffReadDedupe.clear() return getStatusOp(this.git.bind(this), params) } @@ -212,64 +226,110 @@ export class GitHandler { if (rel === '..' || rel.startsWith(`..${path.sep}`) || path.isAbsolute(rel)) { throw new Error(`Path "${filePath}" resolves outside the worktree`) } - return computeDiff( - this.gitBuffer.bind(this), - worktreePath, - filePath, - params.staged as boolean, - params.compareAgainstHead as boolean | undefined + return this.gitDiffReadDedupe.run( + stableInFlightKey([ + 'diff', + worktreePath, + filePath, + params.staged as boolean, + params.compareAgainstHead as boolean | undefined + ]), + () => + computeDiff( + this.gitBuffer.bind(this), + worktreePath, + filePath, + params.staged as boolean, + params.compareAgainstHead as boolean | undefined + ) ) } private async stage(params: Record) { + this.gitDiffReadDedupe.clear() const worktreePath = params.worktreePath as string const filePath = params.filePath as string - await this.git(['add', '--', filePath], worktreePath) + try { + await this.git(['add', '--', filePath], worktreePath) + } finally { + this.gitDiffReadDedupe.clear() + } } private async commit( params: Record ): Promise<{ success: boolean; error?: string }> { + this.gitDiffReadDedupe.clear() const worktreePath = params.worktreePath as string const message = params.message as string - return commitChangesRelay(this.git.bind(this), worktreePath, message) + try { + return await commitChangesRelay(this.git.bind(this), worktreePath, message) + } finally { + this.gitDiffReadDedupe.clear() + } } private async unstage(params: Record) { + this.gitDiffReadDedupe.clear() const worktreePath = params.worktreePath as string const filePath = params.filePath as string - await this.git(['restore', '--staged', '--', filePath], worktreePath) + try { + await this.git(['restore', '--staged', '--', filePath], worktreePath) + } finally { + this.gitDiffReadDedupe.clear() + } } private async bulkStage(params: Record) { + this.gitDiffReadDedupe.clear() const worktreePath = params.worktreePath as string const filePaths = params.filePaths as string[] - for (let i = 0; i < filePaths.length; i += BULK_CHUNK_SIZE) { - const chunk = filePaths.slice(i, i + BULK_CHUNK_SIZE) - await this.git(['add', '--', ...chunk], worktreePath) + try { + for (let i = 0; i < filePaths.length; i += BULK_CHUNK_SIZE) { + const chunk = filePaths.slice(i, i + BULK_CHUNK_SIZE) + await this.git(['add', '--', ...chunk], worktreePath) + } + } finally { + this.gitDiffReadDedupe.clear() } } private async bulkUnstage(params: Record) { + this.gitDiffReadDedupe.clear() const worktreePath = params.worktreePath as string const filePaths = params.filePaths as string[] - for (let i = 0; i < filePaths.length; i += BULK_CHUNK_SIZE) { - const chunk = filePaths.slice(i, i + BULK_CHUNK_SIZE) - await this.git(['restore', '--staged', '--', ...chunk], worktreePath) + try { + for (let i = 0; i < filePaths.length; i += BULK_CHUNK_SIZE) { + const chunk = filePaths.slice(i, i + BULK_CHUNK_SIZE) + await this.git(['restore', '--staged', '--', ...chunk], worktreePath) + } + } finally { + this.gitDiffReadDedupe.clear() } } private async abortMerge(params: Record) { + this.gitDiffReadDedupe.clear() const worktreePath = params.worktreePath as string - await this.git(['merge', '--abort'], worktreePath) + try { + await this.git(['merge', '--abort'], worktreePath) + } finally { + this.gitDiffReadDedupe.clear() + } } private async abortRebase(params: Record) { + this.gitDiffReadDedupe.clear() const worktreePath = params.worktreePath as string - await this.git(['rebase', '--abort'], worktreePath) + try { + await this.git(['rebase', '--abort'], worktreePath) + } finally { + this.gitDiffReadDedupe.clear() + } } private async checkout(params: Record) { + this.gitDiffReadDedupe.clear() const worktreePath = params.worktreePath as string const branch = params.branch as string // Defense-in-depth: reject option-like branch tokens (the RPC schema also @@ -279,8 +339,12 @@ export class GitHandler { if (typeof branch !== 'string' || branch.length === 0 || branch.startsWith('-')) { throw new Error('invalid_branch_name') } - await this.git(['checkout', branch, '--'], worktreePath) - return { ok: true as const, branch } + try { + await this.git(['checkout', branch, '--'], worktreePath) + return { ok: true as const, branch } + } finally { + this.gitDiffReadDedupe.clear() + } } private async localBranches(params: Record) { @@ -338,88 +402,96 @@ export class GitHandler { } private async discard(params: Record) { + this.gitDiffReadDedupe.clear() const worktreePath = params.worktreePath as string const filePath = params.filePath as string - - this.assertInWorktree(worktreePath, filePath) - - let tracked = false try { - await this.git( - ['ls-files', '--error-unmatch', '--', this.literalPathspec(filePath)], - worktreePath - ) - tracked = true - } catch { - // untracked - } + this.assertInWorktree(worktreePath, filePath) - if (tracked) { - await this.git( - ['restore', '--worktree', '--source=HEAD', '--', this.literalPathspec(filePath)], - worktreePath - ) - return - } + let tracked = false + try { + await this.git( + ['ls-files', '--error-unmatch', '--', this.literalPathspec(filePath)], + worktreePath + ) + tracked = true + } catch { + // untracked + } - await removeSafeUntrackedDiscardTarget(worktreePath, filePath, (targetPath) => - this.cleanUntrackedPaths(worktreePath, [targetPath]) - ) + if (tracked) { + await this.git( + ['restore', '--worktree', '--source=HEAD', '--', this.literalPathspec(filePath)], + worktreePath + ) + return + } + + await removeSafeUntrackedDiscardTarget(worktreePath, filePath, (targetPath) => + this.cleanUntrackedPaths(worktreePath, [targetPath]) + ) + } finally { + this.gitDiffReadDedupe.clear() + } } private async bulkDiscard(params: Record) { + this.gitDiffReadDedupe.clear() const worktreePath = params.worktreePath as string const filePaths = params.filePaths as string[] if (filePaths.length === 0) { return } + try { + for (const filePath of filePaths) { + this.assertInWorktree(worktreePath, filePath) + } - for (const filePath of filePaths) { - this.assertInWorktree(worktreePath, filePath) - } + const trackedPathSpecs: string[] = [] + for (let i = 0; i < filePaths.length; i += BULK_CHUNK_SIZE) { + const chunk = filePaths.slice(i, i + BULK_CHUNK_SIZE) + const { stdout } = await this.git( + ['ls-files', '-z', '--', ...chunk.map((p) => this.literalPathspec(p))], + worktreePath + ) + // Why: selecting a tracked directory can make `ls-files -z` return + // enough descendants for push(...split) to exceed the argument limit. + for (const trackedPathSpec of stdout.split('\0')) { + if (trackedPathSpec) { + trackedPathSpecs.push(trackedPathSpec) + } + } + } - const trackedPathSpecs: string[] = [] - for (let i = 0; i < filePaths.length; i += BULK_CHUNK_SIZE) { - const chunk = filePaths.slice(i, i + BULK_CHUNK_SIZE) - const { stdout } = await this.git( - ['ls-files', '-z', '--', ...chunk.map((p) => this.literalPathspec(p))], - worktreePath + const trackedPaths = filePaths.filter((filePath) => + this.isTrackedPathSpec(filePath, trackedPathSpecs) ) - // Why: selecting a tracked directory can make `ls-files -z` return - // enough descendants for push(...split) to exceed the argument limit. - for (const trackedPathSpec of stdout.split('\0')) { - if (trackedPathSpec) { - trackedPathSpecs.push(trackedPathSpec) + const untrackedPaths = filePaths.filter( + (filePath) => !this.isTrackedPathSpec(filePath, trackedPathSpecs) + ) + await removeSafeUntrackedDiscardTargets( + worktreePath, + untrackedPaths, + (targetPaths) => this.cleanUntrackedPaths(worktreePath, targetPaths), + async () => { + for (let i = 0; i < trackedPaths.length; i += BULK_CHUNK_SIZE) { + const chunk = trackedPaths.slice(i, i + BULK_CHUNK_SIZE) + await this.git( + [ + 'restore', + '--worktree', + '--source=HEAD', + '--', + ...chunk.map((p) => this.literalPathspec(p)) + ], + worktreePath + ) + } } - } + ) + } finally { + this.gitDiffReadDedupe.clear() } - - const trackedPaths = filePaths.filter((filePath) => - this.isTrackedPathSpec(filePath, trackedPathSpecs) - ) - const untrackedPaths = filePaths.filter( - (filePath) => !this.isTrackedPathSpec(filePath, trackedPathSpecs) - ) - await removeSafeUntrackedDiscardTargets( - worktreePath, - untrackedPaths, - (targetPaths) => this.cleanUntrackedPaths(worktreePath, targetPaths), - async () => { - for (let i = 0; i < trackedPaths.length; i += BULK_CHUNK_SIZE) { - const chunk = trackedPaths.slice(i, i + BULK_CHUNK_SIZE) - await this.git( - [ - 'restore', - '--worktree', - '--source=HEAD', - '--', - ...chunk.map((p) => this.literalPathspec(p)) - ], - worktreePath - ) - } - } - ) } private literalPathspec(filePath: string): string { @@ -524,177 +596,204 @@ export class GitHandler { } private async fetch(params: Record) { + this.gitDiffReadDedupe.clear() const worktreePath = params.worktreePath as string try { - if (params.pushTarget !== undefined) { - assertGitPushTargetShape(params.pushTarget) - const pushTarget = params.pushTarget as GitPushTarget - await this.git(['check-ref-format', '--branch', pushTarget.branchName], worktreePath) - await this.git(['fetch', '--prune', pushTarget.remoteName], worktreePath) - return + try { + if (params.pushTarget !== undefined) { + assertGitPushTargetShape(params.pushTarget) + const pushTarget = params.pushTarget as GitPushTarget + await this.git(['check-ref-format', '--branch', pushTarget.branchName], worktreePath) + await this.git(['fetch', '--prune', pushTarget.remoteName], worktreePath) + return + } + await this.git(['fetch', '--prune'], worktreePath) + } catch (error) { + // Why: mirror the local gitFetch normalization so SSH users see the same + // actionable messages instead of raw git stderr (which varies across + // versions/locales and may embed credentials). + throw new Error(normalizeGitErrorMessage(error, 'fetch')) } - await this.git(['fetch', '--prune'], worktreePath) - } catch (error) { - // Why: mirror the local gitFetch normalization so SSH users see the same - // actionable messages instead of raw git stderr (which varies across - // versions/locales and may embed credentials). - throw new Error(normalizeGitErrorMessage(error, 'fetch')) + } finally { + this.gitDiffReadDedupe.clear() } } private async forkSync(params: Record, context?: RequestContext) { - const worktreePath = params.worktreePath as string - const expectedUpstream = validateGitForkSyncExpectedUpstream(params.expectedUpstream, { - required: true + return this.runWithDiffDedupeClear(async () => { + const worktreePath = params.worktreePath as string + const expectedUpstream = validateGitForkSyncExpectedUpstream(params.expectedUpstream, { + required: true + }) + const controller = new AbortController() + const abortFromContext = () => controller.abort() + if (context?.signal?.aborted) { + controller.abort() + } else { + context?.signal?.addEventListener('abort', abortFromContext, { once: true }) + } + const timeout = setTimeout(() => controller.abort(), 60_000) + try { + return await syncForkDefaultBranch( + (args) => + this.git(args, worktreePath, { + nonInteractive: true, + signal: controller.signal + }), + { expectedUpstream } + ) + } catch (error) { + throw new Error(normalizeGitErrorMessage(error, 'push')) + } finally { + clearTimeout(timeout) + context?.signal?.removeEventListener('abort', abortFromContext) + } }) - const controller = new AbortController() - const abortFromContext = () => controller.abort() - if (context?.signal?.aborted) { - controller.abort() - } else { - context?.signal?.addEventListener('abort', abortFromContext, { once: true }) - } - const timeout = setTimeout(() => controller.abort(), 60_000) - try { - return await syncForkDefaultBranch( - (args) => - this.git(args, worktreePath, { - nonInteractive: true, - signal: controller.signal - }), - { expectedUpstream } - ) - } catch (error) { - throw new Error(normalizeGitErrorMessage(error, 'push')) - } finally { - clearTimeout(timeout) - context?.signal?.removeEventListener('abort', abortFromContext) - } } private async fetchRemoteTrackingRef(params: Record) { + this.gitDiffReadDedupe.clear() const worktreePath = params.worktreePath as string const remote = params.remote const branch = params.branch const ref = params.ref - if (typeof remote !== 'string' || typeof branch !== 'string' || typeof ref !== 'string') { - throw new Error('Invalid remote-tracking fetch request.') - } - if (remote.startsWith('-') || branch.startsWith('-')) { - throw new Error('Remote-tracking fetch inputs must not start with "-".') - } - if (ref !== `refs/remotes/${remote}/${branch}`) { - throw new Error('Remote-tracking ref does not match the requested remote and branch.') - } - try { - const { stdout } = await this.git(['remote'], worktreePath) - const remotes = stdout - .split(/\r?\n/) - .map((line) => line.trim()) - .filter(Boolean) - if (!remotes.includes(remote)) { - throw new Error(`Remote "${remote}" is not configured.`) + if (typeof remote !== 'string' || typeof branch !== 'string' || typeof ref !== 'string') { + throw new Error('Invalid remote-tracking fetch request.') } - await this.git(['check-ref-format', `refs/heads/${branch}`], worktreePath) - await this.git(['check-ref-format', ref], worktreePath) - await this.git(['fetch', '--no-tags', remote, `+refs/heads/${branch}:${ref}`], worktreePath) - } catch (error) { - // Why: create-worktree needs a write-capable fetch, but generic git.exec - // intentionally rejects fetch. This narrow RPC keeps the relay allowlist - // tight while preserving the same safe error normalization as git.fetch. - throw new Error(normalizeGitErrorMessage(error, 'fetch')) + if (remote.startsWith('-') || branch.startsWith('-')) { + throw new Error('Remote-tracking fetch inputs must not start with "-".') + } + if (ref !== `refs/remotes/${remote}/${branch}`) { + throw new Error('Remote-tracking ref does not match the requested remote and branch.') + } + + try { + const { stdout } = await this.git(['remote'], worktreePath) + const remotes = stdout + .split(/\r?\n/) + .map((line) => line.trim()) + .filter(Boolean) + if (!remotes.includes(remote)) { + throw new Error(`Remote "${remote}" is not configured.`) + } + await this.git(['check-ref-format', `refs/heads/${branch}`], worktreePath) + await this.git(['check-ref-format', ref], worktreePath) + await this.git(['fetch', '--no-tags', remote, `+refs/heads/${branch}:${ref}`], worktreePath) + } catch (error) { + // Why: create-worktree needs a write-capable fetch, but generic git.exec + // intentionally rejects fetch. This narrow RPC keeps the relay allowlist + // tight while preserving the same safe error normalization as git.fetch. + throw new Error(normalizeGitErrorMessage(error, 'fetch')) + } + } finally { + this.gitDiffReadDedupe.clear() } } private async fetchGitLabMergeRequestHead(params: Record) { + this.gitDiffReadDedupe.clear() const worktreePath = params.worktreePath as string const remote = params.remote const mrIid = params.mrIid - if (typeof remote !== 'string') { - throw new Error('Invalid GitLab merge request fetch request.') - } - if (typeof mrIid !== 'number' || !Number.isSafeInteger(mrIid) || mrIid <= 0) { - throw new Error('Invalid GitLab merge request fetch request.') - } - const mergeRequestIid = mrIid - if (remote.startsWith('-')) { - throw new Error('GitLab merge request fetch remote must not start with "-".') - } - try { - const { stdout } = await this.git(['remote'], worktreePath) - const remotes = stdout - .split(/\r?\n/) - .map((line) => line.trim()) - .filter(Boolean) - if (!remotes.includes(remote)) { - throw new Error(`Remote "${remote}" is not configured.`) + if (typeof remote !== 'string') { + throw new Error('Invalid GitLab merge request fetch request.') } - // Why: GitLab MR heads are not refs/heads/*, so the remote-tracking - // fetch RPC cannot represent fork MRs. Keep this write path MR-only. - await this.git( - ['fetch', '--no-tags', remote, `refs/merge-requests/${mergeRequestIid}/head`], - worktreePath - ) - } catch (error) { - throw new Error(normalizeGitErrorMessage(error, 'fetch')) + if (typeof mrIid !== 'number' || !Number.isSafeInteger(mrIid) || mrIid <= 0) { + throw new Error('Invalid GitLab merge request fetch request.') + } + const mergeRequestIid = mrIid + if (remote.startsWith('-')) { + throw new Error('GitLab merge request fetch remote must not start with "-".') + } + + try { + const { stdout } = await this.git(['remote'], worktreePath) + const remotes = stdout + .split(/\r?\n/) + .map((line) => line.trim()) + .filter(Boolean) + if (!remotes.includes(remote)) { + throw new Error(`Remote "${remote}" is not configured.`) + } + // Why: GitLab MR heads are not refs/heads/*, so the remote-tracking + // fetch RPC cannot represent fork MRs. Keep this write path MR-only. + await this.git( + ['fetch', '--no-tags', remote, `refs/merge-requests/${mergeRequestIid}/head`], + worktreePath + ) + } catch (error) { + throw new Error(normalizeGitErrorMessage(error, 'fetch')) + } + } finally { + this.gitDiffReadDedupe.clear() } } private async push(params: Record) { + this.gitDiffReadDedupe.clear() 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. void params.publish try { - const target = await resolveRelayPushTarget( - this.git.bind(this), - worktreePath, - params.pushTarget - ) - const args = [ - 'push', - ...(params.forceWithLease === true ? ['--force-with-lease'] : []), - '--set-upstream', - ...(target ? [target.remote, target.refspec] : ['origin', 'HEAD']) - ] - await this.git(args, worktreePath) - } catch (error) { - // Why: mirror the local gitPush normalization so SSH users see the same - // "non-fast-forward / pull first" guidance instead of raw git stderr. - throw new Error(normalizeGitErrorMessage(error, 'push')) + try { + const target = await resolveRelayPushTarget( + this.git.bind(this), + worktreePath, + params.pushTarget + ) + const args = [ + 'push', + ...(params.forceWithLease === true ? ['--force-with-lease'] : []), + '--set-upstream', + ...(target ? [target.remote, target.refspec] : ['origin', 'HEAD']) + ] + await this.git(args, worktreePath) + } catch (error) { + // Why: mirror the local gitPush normalization so SSH users see the same + // "non-fast-forward / pull first" guidance instead of raw git stderr. + throw new Error(normalizeGitErrorMessage(error, 'push')) + } + } finally { + this.gitDiffReadDedupe.clear() } } private async pullWithArgs(params: Record, pullArgs: string[]) { + this.gitDiffReadDedupe.clear() const worktreePath = params.worktreePath as string try { - if (params.pushTarget !== undefined) { - assertGitPushTargetShape(params.pushTarget) - const pushTarget = params.pushTarget as GitPushTarget - await this.git(['check-ref-format', '--branch', pushTarget.branchName], worktreePath) - await this.git( - ['pull', ...pullArgs, pushTarget.remoteName, pushTarget.branchName], - worktreePath - ) - return + try { + if (params.pushTarget !== undefined) { + assertGitPushTargetShape(params.pushTarget) + const pushTarget = params.pushTarget as GitPushTarget + await this.git(['check-ref-format', '--branch', pushTarget.branchName], worktreePath) + await this.git( + ['pull', ...pullArgs, pushTarget.remoteName, pushTarget.branchName], + worktreePath + ) + return + } + const upstream = await resolveEffectiveGitUpstream((args) => this.git(args, worktreePath)) + if (upstream && !upstream.isConfiguredUpstream) { + // Why: legacy Orca branches may still track origin/main while pushes + // target origin/. Pull the same effective branch the UI reports. + await this.git( + ['pull', ...pullArgs, upstream.remoteName, upstream.branchName], + worktreePath + ) + return + } + await this.git(['pull', ...pullArgs], worktreePath) + } catch (error) { + // Why: mirror the local gitPull normalization so SSH users see the same + // actionable messages instead of raw git stderr. + throw new Error(normalizeGitErrorMessage(error, 'pull')) } - const upstream = await resolveEffectiveGitUpstream((args) => this.git(args, worktreePath)) - if (upstream && !upstream.isConfiguredUpstream) { - // Why: legacy Orca branches may still track origin/main while pushes - // target origin/. Pull the same effective branch the UI reports. - await this.git( - ['pull', ...pullArgs, upstream.remoteName, upstream.branchName], - worktreePath - ) - return - } - await this.git(['pull', ...pullArgs], worktreePath) - } catch (error) { - // Why: mirror the local gitPull normalization so SSH users see the same - // actionable messages instead of raw git stderr. - throw new Error(normalizeGitErrorMessage(error, 'pull')) + } finally { + this.gitDiffReadDedupe.clear() } } @@ -709,16 +808,21 @@ export class GitHandler { } private async rebaseFromBase(params: Record) { + this.gitDiffReadDedupe.clear() const worktreePath = params.worktreePath as string const baseRef = params.baseRef as string try { - const source = await resolveGitRemoteRebaseSource( - ((args) => this.git(args, worktreePath)) as GitCommandRunner, - baseRef - ) - await this.git(['pull', '--rebase', source.remoteName, source.branchName], worktreePath) - } catch (error) { - throw new Error(normalizeGitErrorMessage(error, 'pull')) + try { + const source = await resolveGitRemoteRebaseSource( + ((args) => this.git(args, worktreePath)) as GitCommandRunner, + baseRef + ) + await this.git(['pull', '--rebase', source.remoteName, source.branchName], worktreePath) + } catch (error) { + throw new Error(normalizeGitErrorMessage(error, 'pull')) + } + } finally { + this.gitDiffReadDedupe.clear() } } @@ -728,27 +832,50 @@ export class GitHandler { if (baseRef.startsWith('-')) { throw new Error('Base ref must not start with "-"') } - return branchDiffEntries( - this.git.bind(this), - this.gitBuffer.bind(this), - worktreePath, - baseRef, - { - includePatch: params.includePatch as boolean | undefined, - filePath: params.filePath as string | undefined, - oldPath: params.oldPath as string | undefined - } + const options = { + includePatch: params.includePatch as boolean | undefined, + filePath: params.filePath as string | undefined, + oldPath: params.oldPath as string | undefined + } + return this.gitDiffReadDedupe.run( + stableInFlightKey([ + 'branchDiff', + worktreePath, + baseRef, + options.includePatch ?? null, + options.filePath ?? null, + options.oldPath ?? null + ]), + () => + branchDiffEntries( + this.git.bind(this), + this.gitBuffer.bind(this), + worktreePath, + baseRef, + options + ) ) } private async commitDiff(params: Record) { const worktreePath = params.worktreePath as string - return commitDiffEntry(this.gitBuffer.bind(this), worktreePath, { + const args = { commitOid: params.commitOid as string, parentOid: params.parentOid as string | null | undefined, filePath: params.filePath as string, oldPath: params.oldPath as string | undefined - }) + } + return this.gitDiffReadDedupe.run( + stableInFlightKey([ + 'commitDiff', + worktreePath, + args.commitOid, + args.parentOid ?? null, + args.filePath, + args.oldPath ?? null + ]), + () => commitDiffEntry(this.gitBuffer.bind(this), worktreePath, args) + ) } private async exec(params: Record, context?: RequestContext) { @@ -841,22 +968,24 @@ export class GitHandler { } private async renameCurrentBranch(params: Record) { - const worktreePath = params.worktreePath - const newBranch = params.newBranch - if (typeof worktreePath !== 'string' || typeof newBranch !== 'string') { - throw new Error('Invalid branch rename request.') - } - if (newBranch.startsWith('-')) { - throw new Error('Branch name must not start with "-".') - } - try { - // Why: generic git.exec intentionally blocks destructive branch flags. - // This narrow RPC permits only the already-checked current-branch rename. - await this.git(['check-ref-format', '--branch', newBranch], worktreePath) - await this.git(['branch', '-m', newBranch], worktreePath) - } catch (error) { - throw new Error(normalizeGitErrorMessage(error)) - } + return this.runWithDiffDedupeClear(async () => { + const worktreePath = params.worktreePath + const newBranch = params.newBranch + if (typeof worktreePath !== 'string' || typeof newBranch !== 'string') { + throw new Error('Invalid branch rename request.') + } + if (newBranch.startsWith('-')) { + throw new Error('Branch name must not start with "-".') + } + try { + // Why: generic git.exec intentionally blocks destructive branch flags. + // This narrow RPC permits only the already-checked current-branch rename. + await this.git(['check-ref-format', '--branch', newBranch], worktreePath) + await this.git(['branch', '-m', newBranch], worktreePath) + } catch (error) { + throw new Error(normalizeGitErrorMessage(error)) + } + }) } private async isGitRepo(params: Record) { @@ -891,11 +1020,11 @@ export class GitHandler { } private async addWorktree(params: Record) { - return addWorktreeOp(this.git.bind(this), params) + return this.runWithDiffDedupeClear(() => addWorktreeOp(this.git.bind(this), params)) } private async removeWorktree(params: Record) { - return removeWorktreeOp(this.git.bind(this), params) + return this.runWithDiffDedupeClear(() => removeWorktreeOp(this.git.bind(this), params)) } private async worktreeIsClean(params: Record) { @@ -903,6 +1032,8 @@ export class GitHandler { } private async refreshLocalBaseRefForWorktreeCreate(params: Record) { - return refreshLocalBaseRefForWorktreeCreateOp(this.git.bind(this), params) + return this.runWithDiffDedupeClear(() => + refreshLocalBaseRefForWorktreeCreateOp(this.git.bind(this), params) + ) } } diff --git a/src/shared/in-flight-promise-dedupe.test.ts b/src/shared/in-flight-promise-dedupe.test.ts new file mode 100644 index 000000000..fcc1005a5 --- /dev/null +++ b/src/shared/in-flight-promise-dedupe.test.ts @@ -0,0 +1,83 @@ +import { describe, expect, it, vi } from 'vitest' +import { InFlightPromiseDedupe, stableInFlightKey } from './in-flight-promise-dedupe' + +describe('InFlightPromiseDedupe', () => { + it('coalesces only while in flight and retries after rejection', async () => { + const dedupe = new InFlightPromiseDedupe() + const load = vi + .fn<() => Promise>() + .mockRejectedValueOnce(new Error('transient failure')) + .mockResolvedValueOnce('fresh') + + const key = stableInFlightKey(['diff', '/repo', 'src/file.ts', true]) + const first = dedupe.run(key, load) + const second = dedupe.run(key, load) + + expect(first).toBe(second) + await expect(first).rejects.toThrow('transient failure') + expect(load).toHaveBeenCalledTimes(1) + + await expect(dedupe.run(key, load)).resolves.toBe('fresh') + expect(load).toHaveBeenCalledTimes(2) + }) + + it('uses exact keys for distinct input parts', async () => { + const dedupe = new InFlightPromiseDedupe() + const load = vi.fn(async () => 'value') + + await Promise.all([ + dedupe.run(stableInFlightKey(['diff', '/repo', 'src/file.ts', true]), load), + dedupe.run(stableInFlightKey(['diff', '/repo', 'src/file.ts', false]), load) + ]) + + expect(load).toHaveBeenCalledTimes(2) + }) + + it('clears entries after synchronous loader failures', async () => { + const dedupe = new InFlightPromiseDedupe() + const load = vi + .fn<() => Promise | string>() + .mockImplementationOnce(() => { + throw new Error('sync failure') + }) + .mockResolvedValueOnce('fresh') + + const key = stableInFlightKey(['diff', '/repo', 'src/file.ts']) + + await expect(dedupe.run(key, () => Promise.resolve(load()))).rejects.toThrow('sync failure') + await expect(dedupe.run(key, () => Promise.resolve(load()))).resolves.toBe('fresh') + expect(load).toHaveBeenCalledTimes(2) + }) + + it('clear drops pending entries so later calls start fresh work', async () => { + const dedupe = new InFlightPromiseDedupe() + const load = vi.fn<() => Promise>() + load.mockReturnValueOnce(new Promise(() => undefined)).mockResolvedValueOnce('fresh') + + const key = stableInFlightKey(['diff', '/repo', 'src/file.ts']) + void dedupe.run(key, load) + dedupe.clear() + + await expect(dedupe.run(key, load)).resolves.toBe('fresh') + expect(load).toHaveBeenCalledTimes(2) + }) + + it('expires hung entries so retries can start fresh work', async () => { + vi.useFakeTimers() + try { + const dedupe = new InFlightPromiseDedupe(5) + const load = vi.fn<() => Promise>() + load.mockReturnValueOnce(new Promise(() => undefined)).mockResolvedValueOnce('fresh') + + const key = stableInFlightKey(['diff', '/repo', 'src/file.ts']) + void dedupe.run(key, load) + + vi.advanceTimersByTime(5) + + await expect(dedupe.run(key, load)).resolves.toBe('fresh') + expect(load).toHaveBeenCalledTimes(2) + } finally { + vi.useRealTimers() + } + }) +}) diff --git a/src/shared/in-flight-promise-dedupe.ts b/src/shared/in-flight-promise-dedupe.ts new file mode 100644 index 000000000..0ad90f372 --- /dev/null +++ b/src/shared/in-flight-promise-dedupe.ts @@ -0,0 +1,57 @@ +export class InFlightPromiseDedupe { + private readonly entries = new Map< + string, + { promise: Promise; timeout: ReturnType | null } + >() + + constructor(private readonly maxInFlightMs = 30_000) {} + + run(key: string, load: () => Promise): Promise { + const existing = this.entries.get(key) + if (existing) { + return existing.promise + } + + // Why: this is in-flight coalescing only; the next read after settle must + // observe fresh git state instead of a cached diff. + const promise = Promise.resolve() + .then(load) + .finally(() => { + const entry = this.entries.get(key) + if (entry?.promise === promise) { + if (entry.timeout) { + clearTimeout(entry.timeout) + } + this.entries.delete(key) + } + }) + const entry = { + promise, + // Why: renderer diff rows already time out hung loads; drop matching + // in-flight entries too so retry can issue fresh git work. + timeout: + this.maxInFlightMs > 0 + ? setTimeout(() => { + if (this.entries.get(key)?.promise === promise) { + this.entries.delete(key) + } + }, this.maxInFlightMs) + : null + } + this.entries.set(key, entry) + return promise + } + + clear(): void { + for (const entry of this.entries.values()) { + if (entry.timeout) { + clearTimeout(entry.timeout) + } + } + this.entries.clear() + } +} + +export function stableInFlightKey(parts: readonly unknown[]): string { + return JSON.stringify(parts) +}