diff --git a/src/main/diagnostics/main-thread-churn-probe.test.ts b/src/main/diagnostics/main-thread-churn-probe.test.ts index 62ede5c99c8..bb87989d56d 100644 --- a/src/main/diagnostics/main-thread-churn-probe.test.ts +++ b/src/main/diagnostics/main-thread-churn-probe.test.ts @@ -1,10 +1,19 @@ import { afterEach, describe, expect, it, vi } from 'vitest' + +const { writeStartupDiagnosticLineMock } = vi.hoisted(() => ({ + writeStartupDiagnosticLineMock: vi.fn() +})) +vi.mock('../startup/startup-diagnostics', () => ({ + writeStartupDiagnosticLine: writeStartupDiagnosticLineMock +})) + import { MAIN_THREAD_DIAGNOSTICS_ENV, classifySubprocessCommand, drainSubprocessSpawnStats, isMainThreadDiagnosticsEnabled, - recordSubprocessSpawn + recordSubprocessSpawn, + startMainThreadChurnProbe } from './main-thread-churn-probe' afterEach(() => { @@ -87,3 +96,24 @@ describe('recordSubprocessSpawn', () => { expect(drainSubprocessSpawnStats()).toEqual({}) }) }) + +describe('startMainThreadChurnProbe', () => { + // Counters nothing reads are counters nobody can act on: the probe report is what makes + // "the diff cache never hit" visible outside a test run. + it('folds caller-contributed counters into the report line', async () => { + vi.stubEnv(MAIN_THREAD_DIAGNOSTICS_ENV, '1') + writeStartupDiagnosticLineMock.mockClear() + vi.useFakeTimers() + try { + startMainThreadChurnProbe({ extraStats: () => ({ diffCache: { hits: 3, misses: 1 } }) }) + await vi.advanceTimersByTimeAsync(5_100) + } finally { + vi.useRealTimers() + } + + const line = String(writeStartupDiagnosticLineMock.mock.calls.at(-1)?.[0] ?? '') + expect(JSON.parse(line.replace('[main-thread] ', ''))).toMatchObject({ + diffCache: { hits: 3, misses: 1 } + }) + }) +}) diff --git a/src/main/diagnostics/main-thread-churn-probe.ts b/src/main/diagnostics/main-thread-churn-probe.ts index 1d70999bc14..af6877e408e 100644 --- a/src/main/diagnostics/main-thread-churn-probe.ts +++ b/src/main/diagnostics/main-thread-churn-probe.ts @@ -136,14 +136,20 @@ export function writeMainThreadDiagnosticMarker(marker: string): void { ) } +export type MainThreadChurnProbeOptions = { + /** Extra counters folded into each report line, sampled once per window. */ + extraStats?: () => Record +} + /** * Long-running main-process jank probe for benchmarks and field diagnosis of * issue #7576. Every 5s emits one `[main-thread] {json}` stderr line with the - * window's worst event-loop stall, stall counts over 50/250ms, and drained - * subprocess spawn stats. Unlike the startup stall probe this never stops: - * the churn it measures (git status polling, updater retries) is steady-state. + * window's worst event-loop stall, stall counts over 50/250ms, drained subprocess + * spawn stats, and any counters the caller contributes. Unlike the startup stall + * probe this never stops: the churn it measures (git status polling, updater + * retries) is steady-state. */ -export function startMainThreadChurnProbe(): void { +export function startMainThreadChurnProbe(options: MainThreadChurnProbeOptions = {}): void { if (!isMainThreadDiagnosticsEnabled()) { return } @@ -176,7 +182,8 @@ export function startMainThreadChurnProbe(): void { gapsOver50Ms, gapsOver250Ms, spawnCount: Object.values(spawns).reduce((sum, s) => sum + s.count, 0), - spawns + spawns, + ...options.extraStats?.() } windowMaxGapMs = 0 gapsOver50Ms = 0 diff --git a/src/main/git/command-runner/git-command-resolution.ts b/src/main/git/command-runner/git-command-resolution.ts index 9c53658c159..ddde988b933 100644 --- a/src/main/git/command-runner/git-command-resolution.ts +++ b/src/main/git/command-runner/git-command-resolution.ts @@ -1,10 +1,14 @@ +import { waitForPromiseWithSignal } from '../../../shared/abort-signal-reason' +import { withTimeout } from '../../../shared/promise-timeout-fallback' import { parseWslPath } from '../../wsl' import { isWslDirectGitReadCommand } from '../wsl-direct-git-read-commands' import { disableWslGitReadEnvironment, getWslGitReadEnvironment, invalidateWslGitReadEnvironment, - peekWslGitReadEnvironment + isWslGitReadEnvironmentSettled, + peekWslGitReadEnvironment, + WSL_GIT_READ_ENVIRONMENT_WAIT_MS } from '../wsl-git-read-environment' import { usesHostGitForWslLinkedWorktree } from '../wsl-linked-worktree-git-routing' import { resolveCommand, type ResolvedCommand } from './wsl-command-resolution' @@ -27,23 +31,39 @@ export function resolveGitCommand( // Why: WSL Git resolves a Windows-authored linked-worktree pointer relative to cwd. return { binary: 'git', args, cwd: options.cwd, wsl: null, wslMode: null } } - if (!forceLoginShell && shouldAttemptWslDirectGit(args, options)) { - const distro = wslDistroForCommand(options.cwd, options.wslDistro) - const environment = distro ? peekWslGitReadEnvironment(distro) : undefined + const distro = directWslGitReadDistro(args, options, forceLoginShell) + if (distro) { + const environment = peekWslGitReadEnvironment(distro) if (environment) { - return resolveCommand('git', args, options.cwd, options.wslDistro, { + return resolveCommand('git', args, options.cwd, distro, { wslGitReadEnvironment: environment, env: options.env, terminationBarrier: options.terminationBarrier }) } - if (distro) { - void getWslGitReadEnvironment(distro) - } + void getWslGitReadEnvironment(distro) } return resolveGitCommandWithoutProbe(args, options, captureLoginShellOutput) } +/** + * The distro this command could run shell-free in, or null when it can't. + * + * Why cwd counts: a `\\wsl.localhost\\...` worktree names its distro in + * the path, so requiring the caller to have resolved a WSL project runtime first + * left every diff read on the login shell — one rc run per `git show`. + */ +function directWslGitReadDistro( + args: string[], + options: GitExecOptions, + forceLoginShell: boolean +): string | null { + if (forceLoginShell || !shouldAttemptWslDirectGit(args, options)) { + return null + } + return wslDistroForCommand(options.cwd, options.wslDistro) +} + function shouldAttemptWslDirectGit(args: string[], options: GitExecOptions): boolean { return Boolean( process.platform === 'win32' && @@ -55,8 +75,36 @@ function shouldAttemptWslDirectGit(args: string[], options: GitExecOptions): boo !Object.entries(options.env ?? {}).some( ([key, value]) => key.startsWith('GIT_') && key !== 'GIT_OPTIONAL_LOCKS' && value !== process.env[key] - ) && - options.wslDistro + ) + ) +} + +/** + * Give a read the chance to take the shell-free route instead of silently + * falling back while the probe is still resolving. + * + * The probe is one `wsl.exe` call, shared per distro and reused by every later + * command, so waiting costs at most once. Bounded because a cold or wedged + * distro must not hold a read behind it — past the bound the login-shell route + * runs exactly as it did before. + */ +export function pendingWslDirectGitReadEnvironment( + args: string[], + options: GitExecOptions +): Promise | null { + // Why null rather than a resolved promise: every non-WSL git call goes through here, and + // awaiting even an already-settled promise would push the spawn into a later microtask. + // A settled probe — including a distro whose direct route was permanently disabled — has + // nothing left to wait for, and neither does a read that is already aborted. + const distro = directWslGitReadDistro(args, options, false) + if (!distro || isWslGitReadEnvironmentSettled(distro) || options.signal?.aborted) { + return null + } + // withTimeout also absorbs rejection, so a probe failure can never become a read failure. + return withTimeout( + waitForPromiseWithSignal(getWslGitReadEnvironment(distro), options.signal), + WSL_GIT_READ_ENVIRONMENT_WAIT_MS, + null ) } diff --git a/src/main/git/command-runner/git-exec-file.ts b/src/main/git/command-runner/git-exec-file.ts index c2815c4bb4c..d9762030813 100644 --- a/src/main/git/command-runner/git-exec-file.ts +++ b/src/main/git/command-runner/git-exec-file.ts @@ -13,6 +13,7 @@ import { resolveCommand, type ResolvedCommand } from './wsl-command-resolution' import type { GitExecOptions } from './git-exec-options' import { execFileCapture, execFileCaptureToTermination } from './exec-file-capture' import { + pendingWslDirectGitReadEnvironment, directWslGitExitCode, disableDirectWslGitAfterSuccessfulFallback, invalidateMissingDirectWslGit, @@ -39,6 +40,10 @@ async function gitExecFileAsyncUnlocked( signal: options.signal }) } + const readEnvironmentReady = pendingWslDirectGitReadEnvironment(args, options) + if (readEnvironmentReady) { + await readEnvironmentReady + } let resolved = resolveGitCommand(args, options, false, options.captureWslLoginShellOutput) const environmentReady = prepareWindowsHostGitEnvironment( resolved, @@ -127,11 +132,15 @@ export function gitExecFileAsync( */ export async function gitExecFileAsyncBuffer( args: string[], - options: { cwd: string; maxBuffer?: number; wslDistro?: string } + options: { cwd: string; maxBuffer?: number; wslDistro?: string; preferWslDirectGit?: boolean } ): Promise<{ stdout: Buffer }> { if (isWslLinkedWorktreeGitRoutingCandidate(options.cwd, options.wslDistro)) { await prepareWslLinkedWorktreeGitRouting(options.cwd, options.wslDistro) } + const readEnvironmentReady = pendingWslDirectGitReadEnvironment(args, options) + if (readEnvironmentReady) { + await readEnvironmentReady + } // `git show` is a read, so this normally runs with no shell at all. The fence // still matters for the login-shell fallback: these are raw blob bytes going // straight to the diff/blob viewer, where a banner becomes file content. diff --git a/src/main/git/command-runner/git-stream-stdout.ts b/src/main/git/command-runner/git-stream-stdout.ts index 318be527c41..7f33308d747 100644 --- a/src/main/git/command-runner/git-stream-stdout.ts +++ b/src/main/git/command-runner/git-stream-stdout.ts @@ -11,6 +11,7 @@ import { killSpawnedCommandTree } from './spawned-command-tree-kill' import type { ResolvedCommand } from './wsl-command-resolution' import { DEFAULT_GIT_MAX_BUFFER, type GitExecOptions } from './git-exec-options' import { + pendingWslDirectGitReadEnvironment, directWslGitExitCode, disableDirectWslGitAfterSuccessfulFallback, invalidateMissingDirectWslGit, @@ -64,6 +65,10 @@ export async function gitStreamStdout( ...(options.preferWslDirectGit ? { preferWslDirectGit: true } : {}), ...(options.signal ? { signal: options.signal } : {}) } + const readEnvironmentReady = pendingWslDirectGitReadEnvironment(args, gitOptions) + if (readEnvironmentReady) { + await readEnvironmentReady + } let resolved = resolveGitCommand(args, gitOptions) const environmentReady = prepareWindowsHostGitEnvironment( resolved, diff --git a/src/main/git/git-runtime-options.ts b/src/main/git/git-runtime-options.ts index caa52bd02b4..edc3bd4b30c 100644 --- a/src/main/git/git-runtime-options.ts +++ b/src/main/git/git-runtime-options.ts @@ -14,7 +14,13 @@ export function gitOptionsForWorktree( } } -export function gitStatusReadOptionsForWorktree( +/** + * Options for a git invocation that only reads. Opting in explicitly keeps the + * shell-free WSL route from depending on `wsl-direct-git-read-commands` + * classifying the argv, which is a heuristic these call sites already know the + * answer to. + */ +export function gitReadOptionsForWorktree( cwd: string, options: GitRuntimeOptions = {} ): { diff --git a/src/main/git/runner-wsl-direct-read.test.ts b/src/main/git/runner-wsl-direct-read.test.ts index aa5bd5dbc0d..9cc43223c76 100644 --- a/src/main/git/runner-wsl-direct-read.test.ts +++ b/src/main/git/runner-wsl-direct-read.test.ts @@ -17,11 +17,14 @@ vi.mock('../observability/instrumentation', () => ({ })) vi.mock('../diagnostics/main-thread-churn-probe', () => ({ recordSubprocessSpawn: vi.fn() })) +import { pendingWslDirectGitReadEnvironment } from './command-runner/git-command-resolution' import { gitExecFileAsync, gitSpawn, gitStreamStdout } from './runner' import { + disableWslGitReadEnvironment, getWslGitReadEnvironment, resetWslGitReadEnvironmentForTests, - seedWslGitReadEnvironmentForTests + seedWslGitReadEnvironmentForTests, + WSL_GIT_READ_ENVIRONMENT_WAIT_MS } from './wsl-git-read-environment' import { prepareWslLinkedWorktreeGitRouting, @@ -349,7 +352,9 @@ describe('WSL direct Git reads', () => { }) }) - it('leaves override-less WSL UNC routing on its existing non-login shell', async () => { + // A UNC worktree names its distro in the path, so a read there needs no resolved WSL project + // runtime to skip the shell -- requiring one is what kept every diff read on the login shell. + it('takes the direct read route for an override-less WSL UNC cwd', async () => { await withPlatform('win32', async () => { seedWslGitReadEnvironmentForTests(DISTRO, LOGIN_ENVIRONMENT) succeedExecFile() @@ -359,7 +364,132 @@ describe('WSL direct Git reads', () => { preferWslDirectGit: true }) - expect(execFileMock.mock.calls[0]?.[1]?.slice(3, 5)).toEqual(['bash', '-c']) + const resolved = execFileMock.mock.calls[0]?.[1] ?? [] + expect(resolved).toContain('--exec') + expect(resolved).toContain(`PATH=${LOGIN_ENVIRONMENT.path}`) + expect(resolved).not.toContain('bash') + }) + }) + + // The classifier already recognizes `show`, so the blob reads behind every diff take the + // direct route from the cwd alone -- no caller-supplied distro, no explicit opt-in. + it('takes the direct read route for an unclassified-caller blob read on a UNC cwd', async () => { + await withPlatform('win32', async () => { + seedWslGitReadEnvironmentForTests(DISTRO, LOGIN_ENVIRONMENT) + succeedExecFile() + + await gitExecFileAsync(['show', ':src/file.ts'], { + cwd: String.raw`\\wsl.localhost\Ubuntu\repo` + }) + + expect(execFileMock.mock.calls[0]?.[1]).toContain('--exec') + }) + }) + + // Kicking the probe off and resolving without it left the reads issued before it + // answered on the login shell -- the slow route, chosen by nothing but timing. + it('waits for a cold probe so the first read already skips the shell', async () => { + await withPlatform('win32', async () => { + execFileMock.mockImplementation((_command, args, _options, callback) => { + const child = createMockChild() + if (String(args).includes('_orca_git_path')) { + setTimeout( + () => callback?.(null, fencedProbeStdout(args, LOGIN_ENVIRONMENT_FIELDS), ''), + 0 + ) + } else { + queueMicrotask(() => callback?.(null, 'ok', '')) + } + return child + }) + + await gitExecFileAsync(['show', ':src/file.ts'], { + cwd: String.raw`\\wsl.localhost\Ubuntu\repo` + }) + + expect(execFileMock.mock.calls.at(-1)?.[1]).toContain('--exec') + expect(execFileMock.mock.calls.at(-1)?.[1]).toContain(`PATH=${LOGIN_ENVIRONMENT.path}`) + }) + }) + + it('stops waiting for a wedged probe and reads through the shell route', async () => { + await withPlatform('win32', async () => { + vi.useFakeTimers() + try { + execFileMock.mockImplementation((_command, args, _options, callback) => { + const child = createMockChild() + // The probe never answers; only the git command itself does. + if (!String(args).includes('_orca_git_path')) { + queueMicrotask(() => callback?.(null, 'ok', '')) + } + return child + }) + + const pending = gitExecFileAsync(['show', ':src/file.ts'], { + cwd: String.raw`\\wsl.localhost\Ubuntu\repo` + }) + await vi.advanceTimersByTimeAsync(WSL_GIT_READ_ENVIRONMENT_WAIT_MS) + await pending + + // Exactly the routing this read had before the wait existed: an unanswered + // probe must cost the bound and nothing else. + const resolved = execFileMock.mock.calls.at(-1)?.[1] ?? [] + expect(resolved.slice(3, 5)).toEqual(['bash', '-c']) + expect(resolved).not.toContain('/usr/bin/env') + } finally { + vi.useRealTimers() + } + }) + }) + + // Once the route is disabled the answer is already known, so paying for a timer and two + // extra microtask hops on every later read is pure overhead on the host this PR targets. + it('stops deferring reads once the direct route is disabled for the distro', async () => { + await withPlatform('win32', async () => { + seedWslGitReadEnvironmentForTests(DISTRO, LOGIN_ENVIRONMENT) + disableWslGitReadEnvironment(DISTRO) + + expect( + pendingWslDirectGitReadEnvironment(['show', ':src/file.ts'], { + cwd: String.raw`\\wsl.localhost\Ubuntu\repo` + }) + ).toBeNull() + }) + }) + + it('never waits on the probe for an already-aborted read', async () => { + await withPlatform('win32', async () => { + const controller = new AbortController() + controller.abort() + + expect( + pendingWslDirectGitReadEnvironment(['show', ':src/file.ts'], { + cwd: String.raw`\\wsl.localhost\Ubuntu\repo`, + signal: controller.signal + }) + ).toBeNull() + expect(execFileMock).not.toHaveBeenCalled() + }) + }) + + it('stops waiting for a cold probe as soon as the read aborts', async () => { + await withPlatform('win32', async () => { + vi.useFakeTimers() + try { + // The probe never answers, so only the abort can end the wait. + execFileMock.mockImplementation(() => createMockChild()) + const controller = new AbortController() + + const pending = pendingWslDirectGitReadEnvironment(['show', ':src/file.ts'], { + cwd: String.raw`\\wsl.localhost\Ubuntu\repo`, + signal: controller.signal + }) + controller.abort() + + await expect(pending).resolves.toBeNull() + } finally { + vi.useRealTimers() + } }) }) diff --git a/src/main/git/settled-diff-cache-bounds.test.ts b/src/main/git/settled-diff-cache-bounds.test.ts new file mode 100644 index 00000000000..8d25bba40de --- /dev/null +++ b/src/main/git/settled-diff-cache-bounds.test.ts @@ -0,0 +1,154 @@ +import { describe, expect, it } from 'vitest' +import type { GitDiffResult } from '../../shared/git-diff-compare-types' +import { + MAX_SETTLED_DIFF_CACHE_ENTRIES, + MAX_SETTLED_DIFF_CACHE_RESULT_CHARACTERS, + MAX_SETTLED_DIFF_CACHE_TOTAL_CHARACTERS, + SettledDiffCache +} from './source-control/settled-diff-cache' +import type { WorktreeDiffStamp } from './source-control/worktree-diff-stamp' + +function settledStamp(value: string): WorktreeDiffStamp { + // Old enough that the racy-write margin is satisfied. + return { value, newestMtimeMs: Date.now() - 60_000, capturedAtMs: Date.now() } +} + +function diffOfSize(characters: number): GitDiffResult { + return { + kind: 'text', + originalContent: '', + modifiedContent: 'x'.repeat(characters), + originalIsBinary: false, + modifiedIsBinary: false + } +} + +describe('SettledDiffCache bounds', () => { + it('evicts the least recently used entry past the entry cap', () => { + const cache = new SettledDiffCache() + for (let index = 0; index <= MAX_SETTLED_DIFF_CACHE_ENTRIES; index += 1) { + cache.set(`key-${index}`, settledStamp(`stamp-${index}`), diffOfSize(1), cache.beginRead()) + } + + expect(cache.stats().entries).toBe(MAX_SETTLED_DIFF_CACHE_ENTRIES) + expect(cache.get('key-0', settledStamp('stamp-0'))).toBeUndefined() + expect( + cache.get( + `key-${MAX_SETTLED_DIFF_CACHE_ENTRIES}`, + settledStamp(`stamp-${MAX_SETTLED_DIFF_CACHE_ENTRIES}`) + ) + ).toBeDefined() + }) + + it('keeps a re-read entry alive by refreshing its LRU position', () => { + const cache = new SettledDiffCache() + cache.set('hot', settledStamp('hot-stamp'), diffOfSize(1), cache.beginRead()) + for (let index = 0; index < MAX_SETTLED_DIFF_CACHE_ENTRIES; index += 1) { + cache.get('hot', settledStamp('hot-stamp')) + cache.set(`cold-${index}`, settledStamp(`cold-${index}`), diffOfSize(1), cache.beginRead()) + } + + expect(cache.get('hot', settledStamp('hot-stamp'))).toBeDefined() + }) + + // One entry can legitimately hold megabytes, so an entry count alone bounds nothing. + it('holds total retained content under the character budget', () => { + const cache = new SettledDiffCache() + const chunk = MAX_SETTLED_DIFF_CACHE_RESULT_CHARACTERS + const needed = Math.ceil(MAX_SETTLED_DIFF_CACHE_TOTAL_CHARACTERS / chunk) + 2 + + for (let index = 0; index < needed; index += 1) { + cache.set( + `key-${index}`, + settledStamp(`stamp-${index}`), + diffOfSize(chunk), + cache.beginRead() + ) + } + + expect(cache.stats().retainedCharacters).toBeLessThanOrEqual( + MAX_SETTLED_DIFF_CACHE_TOTAL_CHARACTERS + ) + }) + + it('declines a single result larger than the per-entry cap', () => { + const cache = new SettledDiffCache() + + cache.set( + 'huge', + settledStamp('huge-stamp'), + diffOfSize(MAX_SETTLED_DIFF_CACHE_RESULT_CHARACTERS + 1), + cache.beginRead() + ) + + expect(cache.stats().entries).toBe(0) + expect(cache.stats().retainedCharacters).toBe(0) + }) + + it('releases retained characters when a key is overwritten', () => { + const cache = new SettledDiffCache() + cache.set('key', settledStamp('first'), diffOfSize(1_000), cache.beginRead()) + cache.set('key', settledStamp('second'), diffOfSize(10), cache.beginRead()) + + expect(cache.stats().entries).toBe(1) + expect(cache.stats().retainedCharacters).toBe(10) + expect(cache.get('key', settledStamp('first'))).toBeUndefined() + expect(cache.get('key', settledStamp('second'))).toBeDefined() + }) + + it('drops everything and refuses in-flight stores after a clear', () => { + const cache = new SettledDiffCache() + const readGeneration = cache.beginRead() + cache.clear() + cache.set('key', settledStamp('stamp'), diffOfSize(1), readGeneration) + + expect(cache.stats().entries).toBe(0) + expect(cache.stats().invalidatedDuringRead).toBe(1) + }) +}) + +// Why (#15036 review): the margin compares this host's clock to mtimes the WSL guest +// wrote. A guest running ahead refuses every store for as long as the skew lasts, and +// without its own counter that is indistinguishable from a repo nobody has touched. +describe('settled diff cache clock skew', () => { + const result: GitDiffResult = diffOfSize(10) + + it('counts a future-dated mtime separately from an ordinary racy write', () => { + const cache = new SettledDiffCache() + const now = Date.now() + + // Honestly fresh: written just now, margin not yet satisfied. + cache.set( + 'fresh', + { value: 'a', newestMtimeMs: now, capturedAtMs: now }, + result, + cache.beginRead() + ) + // Skewed: mtime in this host's future, which no local write can produce. + cache.set( + 'skewed', + { value: 'b', newestMtimeMs: now + 60_000, capturedAtMs: now }, + result, + cache.beginRead() + ) + + const stats = cache.stats() + expect(stats.racyWrites).toBe(2) + expect(stats.clockSkewedWrites).toBe(1) + expect(stats.stores).toBe(0) + }) + + it('does not flag a stamp whose components were all absent', () => { + const cache = new SettledDiffCache() + const now = Date.now() + + cache.set( + 'absent', + { value: 'c', newestMtimeMs: Number.NEGATIVE_INFINITY, capturedAtMs: now }, + result, + cache.beginRead() + ) + + expect(cache.stats().clockSkewedWrites).toBe(0) + }) +}) diff --git a/src/main/git/source-control/effective-upstream-status-probe.ts b/src/main/git/source-control/effective-upstream-status-probe.ts index bea3dc93d79..1fd15cda486 100644 --- a/src/main/git/source-control/effective-upstream-status-probe.ts +++ b/src/main/git/source-control/effective-upstream-status-probe.ts @@ -6,7 +6,7 @@ import { } from '../../../shared/git-effective-upstream' import { createGitConfigSnapshotRunner } from '../../../shared/git-config-snapshot-runner' import type { GitRuntimeOptions } from '../git-runtime-options' -import { gitStatusReadOptionsForWorktree } from '../git-runtime-options' +import { gitReadOptionsForWorktree } from '../git-runtime-options' import { gitExecFileAsync } from '../runner' import { MAX_EFFECTIVE_UPSTREAM_NEGATIVE_CACHE_ENTRIES, @@ -90,7 +90,7 @@ async function probeOrRevalidateEffectiveUpstreamStatus( } else if (cached) { try { const status = await getGitUpstreamStatusForUpstreamName( - (args) => gitExecFileAsync(args, gitStatusReadOptionsForWorktree(worktreePath, options)), + (args) => gitExecFileAsync(args, gitReadOptionsForWorktree(worktreePath, options)), cached.upstreamName ) return { status, probedSameNameOriginRef: false } @@ -127,7 +127,7 @@ async function probeEffectiveUpstreamStatus( ): Promise<{ status: GitUpstreamStatus; probedSameNameOriginRef: boolean }> { let probedSameNameOriginRef = false const snapshotRunner = createGitConfigSnapshotRunner((args) => - gitExecFileAsync(args, gitStatusReadOptionsForWorktree(worktreePath, options)) + gitExecFileAsync(args, gitReadOptionsForWorktree(worktreePath, options)) ) const status = await getEffectiveGitUpstreamStatus((args) => { if (args[0] === 'rev-parse' && args.includes(`refs/remotes/origin/${branchName}`)) { diff --git a/src/main/git/source-control/file-diff.ts b/src/main/git/source-control/file-diff.ts index a8fe78a6270..5595f0f6804 100644 --- a/src/main/git/source-control/file-diff.ts +++ b/src/main/git/source-control/file-diff.ts @@ -3,7 +3,8 @@ import type { GitDiffResult } from '../../../shared/git-diff-compare-types' import { stableInFlightKey } from '../../../shared/in-flight-promise-dedupe' import type { GitRuntimeOptions } from '../git-runtime-options' import { gitRuntimeOptionsKey } from './git-runtime-options-cache-key' -import { gitDiffReadDedupe } from './git-read-cache-invalidation' +import { gitDiffReadDedupe, settledDiffCache } from './git-read-cache-invalidation' +import { readWorktreeDiffStamp } from './worktree-diff-stamp' import { buildDiffResult } from './diff-result' import { readGitBlobAtIndexPath, @@ -33,27 +34,74 @@ export async function getDiff( compareAgainstHead = false, options: GitRuntimeOptions = {} ): Promise { - // Why: register the dedupe synchronously (before any await) so concurrent identical reads coalesce. - return gitDiffReadDedupe.run( - stableInFlightKey([ - 'diff', + const readKey = stableInFlightKey([ + 'diff', + worktreePath, + filePath, + staged, + compareAgainstHead, + ...gitRuntimeOptionsKey(options) + ]) + // Why: register the dedupe synchronously (before any await) so concurrent identical reads + // coalesce — including on the settled-cache lookup, which is itself I/O. + return gitDiffReadDedupe.run(readKey, () => + loadDiffThroughSettledCache( + readKey, worktreePath, filePath, staged, compareAgainstHead, - ...gitRuntimeOptionsKey(options) - ]), - () => loadDiff(worktreePath, filePath, staged, compareAgainstHead, options) + options + ) ) } +/** + * Serve a settled diff when the git state it was built from is provably + * unchanged, otherwise read and — only if the read proved everything it touched + * — record it under the stamp taken *before* the read. + * + * Stamping first is what makes staleness impossible: anything that moves during + * or after the read leaves the stored stamp behind, so the next lookup misses. + */ +async function loadDiffThroughSettledCache( + readKey: string, + worktreePath: string, + filePath: string, + staged: boolean, + compareAgainstHead: boolean, + options: GitRuntimeOptions +): Promise { + // Why before the stamp read: the stamp is itself several awaited stats, and a mutation that + // lands entirely inside that window would otherwise leave the fence covering only the git read. + const readGeneration = settledDiffCache.beginRead() + // A staged diff compares HEAD to the index, so the working tree is not one of its inputs. + const stamp = await readWorktreeDiffStamp(worktreePath, filePath, !staged) + const cached = settledDiffCache.get(readKey, stamp) + if (cached) { + return cached + } + const loaded = await loadDiff(worktreePath, filePath, staged, compareAgainstHead, options) + if (loaded.reusable) { + settledDiffCache.set(readKey, stamp, loaded.result, readGeneration) + } + return loaded.result +} + +/** + * `reusable` is false when the result cannot be proven to describe the stamped + * state: a submodule route, whose inputs live in another repo and are stamped by + * that repo's own read, or a blob read that failed rather than proving absence. + */ +type LoadedDiff = { result: GitDiffResult; reusable: boolean } + async function loadDiff( worktreePath: string, filePath: string, staged: boolean, compareAgainstHead: boolean, options: GitRuntimeOptions -): Promise { +): Promise { // Why: gitlink paths can't be read as blobs, so route submodule diffs explicitly (root → pointer, inner → recurse). const submodulePaths = await listSubmodulePaths(worktreePath, options) if (submodulePaths.length > 0) { @@ -63,13 +111,15 @@ async function loadDiff( const submoduleWorktreePath = resolveSubmoduleWorktreePath(worktreePath, matchedSubmodule) const normalizedFilePath = filePath.replace(/\\/g, '/').replace(/\/+$/, '') if (normalizedFilePath === matchedSubmodule) { - return buildSubmodulePointerDiff( - worktreePath, - matchedSubmodule, - staged, - compareAgainstHead, - options, - submoduleWorktreePath + return notReusable( + await buildSubmodulePointerDiff( + worktreePath, + matchedSubmodule, + staged, + compareAgainstHead, + options, + submoduleWorktreePath + ) ) } const innerPath = normalizedFilePath.slice(matchedSubmodule.length + 1) @@ -82,15 +132,20 @@ async function loadDiff( : await readWorkingSubmoduleHead(submoduleWorktreePath, options) // Why: a moved gitlink with a clean submodule worktree means the change is committed — diff the two commits. if (fromOid && toOid && fromOid !== toOid) { - return buildSubmoduleInnerCommitRangeDiff( - submoduleWorktreePath, - innerPath, - fromOid, - toOid, - options + return notReusable( + await buildSubmoduleInnerCommitRangeDiff( + submoduleWorktreePath, + innerPath, + fromOid, + toOid, + options + ) ) } - return getDiff(submoduleWorktreePath, innerPath, staged, compareAgainstHead, options) + // The inner read stamps and caches against the submodule's own repo state. + return notReusable( + await getDiff(submoduleWorktreePath, innerPath, staged, compareAgainstHead, options) + ) } } @@ -99,6 +154,7 @@ async function loadDiff( let originalIsBinary = false let modifiedIsBinary = false let modifiedDeleted = false + let readFailed = false try { if (staged) { @@ -113,6 +169,7 @@ async function loadDiff( modifiedContent = rightBlob.content modifiedIsBinary = rightBlob.isBinary modifiedDeleted = !rightBlob.exists + readFailed = leftBlob.failed === true || rightBlob.failed === true } else { // The left chain (index→HEAD) is sequential within itself, but the working // tree read is a plain fs read that does not depend on it. @@ -127,9 +184,11 @@ async function loadDiff( modifiedContent = workingTreeBlob.content modifiedIsBinary = workingTreeBlob.isBinary modifiedDeleted = !workingTreeBlob.exists + readFailed = leftBlob.failed === true || workingTreeBlob.failed === true } } catch { // Fallback + readFailed = true } const result = buildDiffResult( @@ -141,7 +200,11 @@ async function loadDiff( ) // Why: mark a proven deletion so previewers don't mistake a read failure's empty side for one. if (result.kind === 'binary' && modifiedDeleted) { - return { ...result, modifiedDeleted: true } + return { result: { ...result, modifiedDeleted: true }, reusable: !readFailed } } - return result + return { result, reusable: !readFailed } +} + +function notReusable(result: GitDiffResult): LoadedDiff { + return { result, reusable: false } } diff --git a/src/main/git/source-control/git-blob-read.ts b/src/main/git/source-control/git-blob-read.ts index e9e9182939c..f25a80eedbb 100644 --- a/src/main/git/source-control/git-blob-read.ts +++ b/src/main/git/source-control/git-blob-read.ts @@ -2,7 +2,7 @@ import { readFile, stat } from 'node:fs/promises' import * as path from 'node:path' import { isBinaryBuffer } from '../../../shared/binary-buffer' import type { GitRuntimeOptions } from '../git-runtime-options' -import { gitOptionsForWorktree } from '../git-runtime-options' +import { gitReadOptionsForWorktree } from '../git-runtime-options' import { gitExecFileAsyncBuffer } from '../runner' import { isMaxBufferOverflowError } from '../max-buffer-overflow' import { MAX_GIT_SHOW_BYTES } from './git-show-max-bytes' @@ -12,6 +12,21 @@ export type GitBlobReadResult = { content: string isBinary: boolean exists: boolean + /** + * The read did not complete: the blob is neither known-present nor proven + * absent. Callers must not persist a diff built on one, because the empty side + * it produces is indistinguishable from a genuinely new file. + */ + failed?: boolean +} + +/** + * Tell "Git ran and said the path is not there" apart from "the read never got + * an answer". Git exits 128 for a missing path in a tree or the index; a WSL + * relay that never reached Git exits with anything else, or with a spawn errno. + */ +function isProvenAbsentError(error: unknown): boolean { + return (error as { code?: unknown } | null)?.code === 128 } export async function readUnstagedLeftBlob( @@ -24,7 +39,9 @@ export async function readUnstagedLeftBlob( return indexBlob } - return readGitBlobAtOidPath(worktreePath, 'HEAD', filePath, options) + const headBlob = await readGitBlobAtOidPath(worktreePath, 'HEAD', filePath, options) + // Why: if the index read never got an answer, falling back to HEAD is a guess, not a proof. + return indexBlob.failed ? { ...headBlob, failed: true } : headBlob } export async function readGitBlobAtIndexPath( @@ -36,7 +53,7 @@ export async function readGitBlobAtIndexPath( const gitPath = filePath.replace(/\\/g, '/') try { const { stdout } = await gitExecFileAsyncBuffer(['show', `:${gitPath}`], { - ...gitOptionsForWorktree(worktreePath, options), + ...gitReadOptionsForWorktree(worktreePath, options), maxBuffer: MAX_GIT_SHOW_BYTES }) @@ -45,7 +62,7 @@ export async function readGitBlobAtIndexPath( if (isMaxBufferOverflowError(error)) { return { content: '', isBinary: true, exists: true } } - return { content: '', isBinary: false, exists: false } + return { content: '', isBinary: false, exists: false, failed: !isProvenAbsentError(error) } } } @@ -61,7 +78,7 @@ export async function readGitBlobAtOidPath( const { stdout } = await gitExecFileAsyncBuffer( ['show', '--end-of-options', `${oid}:${gitPath}`], { - ...gitOptionsForWorktree(worktreePath, options), + ...gitReadOptionsForWorktree(worktreePath, options), maxBuffer: MAX_GIT_SHOW_BYTES } ) @@ -71,7 +88,7 @@ export async function readGitBlobAtOidPath( if (isMaxBufferOverflowError(error)) { return { content: '', isBinary: true, exists: true } } - return { content: '', isBinary: false, exists: false } + return { content: '', isBinary: false, exists: false, failed: !isProvenAbsentError(error) } } } @@ -81,11 +98,8 @@ export async function readWorkingTreeFile(filePath: string): Promise { + try { + await access(target) + return true + } catch { + return false + } +} + export async function abortMerge( worktreePath: string, options: GitRuntimeOptions = {} diff --git a/src/main/git/source-control/git-read-cache-invalidation.ts b/src/main/git/source-control/git-read-cache-invalidation.ts index 7d4da0cc8ed..13abe4706a8 100644 --- a/src/main/git/source-control/git-read-cache-invalidation.ts +++ b/src/main/git/source-control/git-read-cache-invalidation.ts @@ -7,15 +7,20 @@ import { GitStatusReadLeaseOwner } from '../git-status-read-lease-owner' import { invalidateGitUpstreamStatusReads } from '../upstream' import { clearSubmodulePathsCache } from './submodule-paths' import { resolvedUpstreamNameCache } from './resolved-upstream-name-cache' +import { SettledDiffCache } from './settled-diff-cache' export const gitDiffReadDedupe = new InFlightPromiseDedupe() +/** Settled diff results, valid only while their stamped git state holds. */ +export const settledDiffCache = new SettledDiffCache() + export const statusReadLeaseOwner = new GitStatusReadLeaseOwner() // Why: clear every in-flight git read cache; clearing only some would let a post-mutation // getStatus() join a pre-mutation read and publish it as current. export function invalidateGitReadCaches(): void { gitDiffReadDedupe.clear() + settledDiffCache.clear() statusReadLeaseOwner.invalidate() invalidateGitBranchLineTotalInFlight() invalidateGitUpstreamStatusReads() diff --git a/src/main/git/source-control/settled-diff-cache.ts b/src/main/git/source-control/settled-diff-cache.ts new file mode 100644 index 00000000000..5ffd7f03e76 --- /dev/null +++ b/src/main/git/source-control/settled-diff-cache.ts @@ -0,0 +1,155 @@ +import { BoundedMap } from '../../../shared/bounded-map' +import type { GitDiffResult } from '../../../shared/git-diff-compare-types' +import { + canProveUnchangedByStamp, + isDiffStampClockSkewed, + type WorktreeDiffStamp +} from './worktree-diff-stamp' + +/** + * Diff results that survive their read, guarded by a stamp of the git state they + * were built from. + * + * Why this is not a TTL: nothing here expires on a clock, so no window exists in + * which a stale diff can be served. An entry is returned only when a freshly + * taken stamp equals the one captured *before* the read that produced it, which + * means no input moved from then until now. Everything else — an unprovable + * stamp, a write too recent for its mtime to be conclusive, a read that failed + * partway, a mutation that ran while the read was in flight — declines to cache + * rather than risk it. + */ + +// A diff result carries whole file contents on both sides, so an entry count alone bounds +// nothing: the real budget is characters, and one entry can legitimately be huge. +export const MAX_SETTLED_DIFF_CACHE_ENTRIES = 32 +export const MAX_SETTLED_DIFF_CACHE_RESULT_CHARACTERS = 1_000_000 +export const MAX_SETTLED_DIFF_CACHE_TOTAL_CHARACTERS = 8_000_000 + +export type SettledDiffCacheStats = { + hits: number + /** Stamp was taken but no entry matched it. */ + misses: number + /** No stamp could be taken, so the read could never be cached. */ + unprovable: number + stores: number + /** Store declined because a write was too recent for its mtime to be conclusive. */ + racyWrites: number + /** + * Subset of `racyWrites` where a component's mtime was in this host's future, so + * the clocks disagree and the refusal will persist until they converge. A nonzero + * count here means the cache is off for a reason no amount of idling will fix. + */ + clockSkewedWrites: number + /** Store declined because a mutation invalidated the cache while the read ran. */ + invalidatedDuringRead: number + entries: number + retainedCharacters: number +} + +type CacheEntry = { stamp: string; result: GitDiffResult; characters: number } + +export class SettledDiffCache { + private readonly entries = new BoundedMap({ + maxEntries: MAX_SETTLED_DIFF_CACHE_ENTRIES, + maxBytes: MAX_SETTLED_DIFF_CACHE_TOTAL_CHARACTERS, + maxEntryBytes: MAX_SETTLED_DIFF_CACHE_RESULT_CHARACTERS, + sizeOf: (entry) => entry.characters + }) + private generation = 0 + private hits = 0 + private misses = 0 + private unprovable = 0 + private stores = 0 + private racyWrites = 0 + private clockSkewedWrites = 0 + private invalidatedDuringRead = 0 + + /** + * Take before starting a read; hand back to `set`. A mutation that lands while + * the read is in flight bumps the generation, and the store is refused — the + * result describes pre-mutation state and must not outlive it. + */ + beginRead(): number { + return this.generation + } + + get(key: string, stamp: WorktreeDiffStamp | null): GitDiffResult | undefined { + if (!stamp) { + this.unprovable += 1 + return undefined + } + // BoundedMap.get() already refreshes the LRU position. + const entry = this.entries.get(key) + if (!entry || entry.stamp !== stamp.value) { + this.misses += 1 + return undefined + } + this.hits += 1 + return entry.result + } + + set( + key: string, + stamp: WorktreeDiffStamp | null, + result: GitDiffResult, + readGeneration: number + ): void { + if (!stamp) { + return + } + if (readGeneration !== this.generation) { + this.invalidatedDuringRead += 1 + return + } + if (!canProveUnchangedByStamp(stamp)) { + this.racyWrites += 1 + if (isDiffStampClockSkewed(stamp)) { + this.clockSkewedWrites += 1 + } + return + } + const characters = resultCharacterCount(result) + if (this.entries.set(key, { stamp: stamp.value, result, characters })) { + this.stores += 1 + } + } + + clear(): void { + this.entries.clear() + // Why: bump so a read that started pre-mutation can't repopulate the invalidated cache. + this.generation += 1 + } + + /** + * Why exposed: a stamp that can never match — an inode or mtime the filesystem + * reports unstably — looks exactly like having no cache at all. These counters + * are what tells a miss storm apart from a cold start. + */ + stats(): SettledDiffCacheStats { + return { + hits: this.hits, + misses: this.misses, + unprovable: this.unprovable, + stores: this.stores, + racyWrites: this.racyWrites, + clockSkewedWrites: this.clockSkewedWrites, + invalidatedDuringRead: this.invalidatedDuringRead, + entries: this.entries.size, + retainedCharacters: this.entries.retainedBytes + } + } + + resetStatsForTests(): void { + this.hits = 0 + this.misses = 0 + this.unprovable = 0 + this.stores = 0 + this.racyWrites = 0 + this.clockSkewedWrites = 0 + this.invalidatedDuringRead = 0 + } +} + +function resultCharacterCount(result: GitDiffResult): number { + return result.originalContent.length + result.modifiedContent.length +} diff --git a/src/main/git/source-control/status-branch-line-total-input.ts b/src/main/git/source-control/status-branch-line-total-input.ts index d2ac795bb55..5603d55bd4c 100644 --- a/src/main/git/source-control/status-branch-line-total-input.ts +++ b/src/main/git/source-control/status-branch-line-total-input.ts @@ -6,7 +6,7 @@ import { type GitBranchLineTotal } from '../../../shared/git-branch-line-total' import type { GitRuntimeOptions } from '../git-runtime-options' -import { gitStatusReadOptionsForWorktree } from '../git-runtime-options' +import { gitReadOptionsForWorktree } from '../git-runtime-options' import { gitExecFileAsync, gitOptionalLocksDisabledEnv } from '../runner' import type { GetStatusOptions } from './get-status-options' @@ -36,7 +36,7 @@ export function createBranchLineTotalInput( .map((entry) => entry.path), runDiffNumstat: (args, signal) => gitExecFileAsync(args, { - ...gitStatusReadOptionsForWorktree(worktreePath, options), + ...gitReadOptionsForWorktree(worktreePath, options), // Why: after the spread, so the shared lease signal wins over this caller's own. signal, env: gitOptionalLocksDisabledEnv(), diff --git a/src/main/git/source-control/status-line-stats.ts b/src/main/git/source-control/status-line-stats.ts index 660c9ea018e..32e229cdbbd 100644 --- a/src/main/git/source-control/status-line-stats.ts +++ b/src/main/git/source-control/status-line-stats.ts @@ -6,7 +6,7 @@ import { type GitLineStats } from '../../../shared/git-uncommitted-line-stats' import type { GitRuntimeOptions } from '../git-runtime-options' -import { gitStatusReadOptionsForWorktree } from '../git-runtime-options' +import { gitReadOptionsForWorktree } from '../git-runtime-options' import { gitExecFileAsync, gitOptionalLocksDisabledEnv } from '../runner' async function runNumstat( @@ -26,7 +26,7 @@ async function runNumstat( '-M' ], { - ...gitStatusReadOptionsForWorktree(worktreePath, options), + ...gitReadOptionsForWorktree(worktreePath, options), env: gitOptionalLocksDisabledEnv() } ) diff --git a/src/main/git/source-control/status-read.ts b/src/main/git/source-control/status-read.ts index 898b46e1ba9..7893cabde15 100644 --- a/src/main/git/source-control/status-read.ts +++ b/src/main/git/source-control/status-read.ts @@ -15,7 +15,7 @@ import { import { gitOptionalLocksDisabledEnv, gitStreamStdout } from '../runner' import { findExistingWorktreeSymlinkPaths } from '../worktree-symlink-detection' import type { GetStatusOptions } from './get-status-options' -import { gitDiffReadDedupe, statusReadLeaseOwner } from './git-read-cache-invalidation' +import { statusReadLeaseOwner } from './git-read-cache-invalidation' import { detectConflictOperation } from './git-conflict-operation' import { parseUnmergedEntry } from './status-conflict-entries' import { getEffectiveUpstreamStatusCacheKey } from './effective-upstream-status-cache' @@ -37,8 +37,10 @@ export async function getStatus( worktreePath: string, options: GetStatusOptions = {} ): Promise { - gitDiffReadDedupe.clear() - // Why: dedupe only concurrent identical reads; after settle, callers must run a fresh read. + // Why nothing is cleared here: a status poll is a read. Dropping the in-flight diff entry + // mid-read only made a concurrent identical request start duplicate git work, and the + // settled diff cache is keyed on stamped git state, which a read cannot change anyway. + // Mutations invalidate both, through invalidateGitReadCaches. const cacheKey = getStatusReadKey(worktreePath, options) return statusReadLeaseOwner.lease(cacheKey, options.signal, (sharedSignal) => runGetStatus(worktreePath, { ...options, signal: sharedSignal }) diff --git a/src/main/git/source-control/worktree-diff-stamp.ts b/src/main/git/source-control/worktree-diff-stamp.ts new file mode 100644 index 00000000000..f24130d274b --- /dev/null +++ b/src/main/git/source-control/worktree-diff-stamp.ts @@ -0,0 +1,226 @@ +import { readFile, stat } from 'node:fs/promises' +import * as path from 'node:path' +import { resolveGitDir } from './resolve-git-dir' + +/** + * Subprocess-free proof of every input one working-tree file diff is built from: + * the HEAD tree, the index, `.gitmodules` (which decides submodule routing) and + * the working-tree file itself. + * + * Two equal stamps mean `git show HEAD:`, `git show :` and the + * working-tree bytes would still return what they returned when the stamp was + * taken, so a caller may reuse a settled diff instead of respawning Git — on a + * WSL/UNC worktree that trades two `wsl.exe` spawns for a handful of 9p stats. + * + * `null` means "cannot prove unchanged" — not a repo, an unreadable layout, or a + * filesystem that reports no usable mtime — and callers must not cache. + */ +export type WorktreeDiffStamp = { + /** Opaque; compare for equality only. */ + value: string + /** Newest mtime any component reported, or -Infinity when every one was absent. */ + newestMtimeMs: number + capturedAtMs: number +} + +/** + * How long after a component's mtime the stamp has to be taken before a later + * write is guaranteed to move that mtime. + * + * Why 2s: FAT/exFAT truncate mtime to a 2s bucket, the coarsest granularity a + * repo can realistically sit on. Below the margin a second write inside the same + * bucket would leave the stamp identical, so the read stays uncached instead. + */ +export const DIFF_STAMP_RACY_WRITE_MARGIN_MS = 2_000 + +const MISSING = '-' +const ABSENT: StampComponent = { text: MISSING, mtimeMs: Number.NEGATIVE_INFINITY } + +type StampComponent = { text: string; mtimeMs: number } + +/** + * A write that lands in the same timestamp bucket as the stamp is invisible, so + * only a stamp taken a full bucket after its newest component can be trusted. + * + * The margin compares two clocks: `capturedAtMs` is this host's, while the mtimes + * come from whatever wrote the files. On a `\\wsl.localhost` worktree the guest + * sets them, and a guest running ahead pushes every recently-touched file past the + * margin — silently, and for as long as the skew lasts. `isDiffStampClockSkewed` + * separates that from an honestly-too-fresh file so the caller can count it. + */ +export function canProveUnchangedByStamp(stamp: WorktreeDiffStamp): boolean { + return stamp.capturedAtMs - stamp.newestMtimeMs >= DIFF_STAMP_RACY_WRITE_MARGIN_MS +} + +/** + * True when a component's mtime is in this host's future, which no local write can + * produce — so the margin above is measuring skew, not freshness, and will keep + * refusing to store until the clocks converge. + */ +export function isDiffStampClockSkewed(stamp: WorktreeDiffStamp): boolean { + return Number.isFinite(stamp.newestMtimeMs) && stamp.newestMtimeMs > stamp.capturedAtMs +} + +/** + * Stamp the inputs of one file diff. Pass `includeWorkingTree: false` for a + * staged diff, which compares HEAD to the index and never reads the working tree. + */ +export async function readWorktreeDiffStamp( + worktreePath: string, + filePath: string, + includeWorkingTree: boolean +): Promise { + const capturedAtMs = Date.now() + try { + const gitDir = await resolveGitDir(worktreePath) + const [head, index, gitmodules, workingTree] = await Promise.all([ + readHeadComponent(gitDir), + // Over-invalidates on purpose: git run outside Orca (a terminal `git status`/`git add`) + // can refresh a stat-dirty index and rewrite this file without changing a single blob, + // which costs one re-read. That is the safe direction — do not "fix" it by dropping the + // index from the stamp, because `git add` then becomes invisible and the cache serves a + // pre-staging diff. + readFileStampComponent(path.join(gitDir, 'index')), + readFileStampComponent(path.join(worktreePath, '.gitmodules')), + includeWorkingTree + ? readWorkingTreeComponent(path.join(worktreePath, filePath)) + : Promise.resolve(ABSENT) + ]) + if (!head) { + return null + } + const components = [head, index, gitmodules, workingTree] + return { + // Why JSON: a path or a ref can contain any separator character, and an ambiguous + // join is a stamp collision — two different states that compare equal. + value: JSON.stringify([ + worktreePath, + filePath, + ...components.map((component) => component.text) + ]), + newestMtimeMs: Math.max(...components.map((component) => component.mtimeMs)), + capturedAtMs + } + } catch { + return null + } +} + +/** + * Identify the commit HEAD resolves to. Reading the tip is what makes a plain + * commit visible: it rewrites `refs/heads/` and leaves HEAD untouched. + * + * A loose tip is recorded by content, so it needs no mtime margin. Only when no + * loose ref exists at all — packed refs, the reftable backend, or an unborn + * branch — does this fall back to the coarser stamps of the files a packed tip + * moves, and the recorded "no loose ref" marker still catches the ref appearing. + */ +async function readHeadComponent(gitDir: string): Promise { + const [head, commonDirEntry] = await Promise.all([ + readTrimmedFile(path.join(gitDir, 'HEAD')), + readTrimmedFile(path.join(gitDir, 'commondir')) + ]) + if (!head) { + return null + } + const commonDir = commonDirEntry ? path.resolve(gitDir, commonDirEntry) : gitDir + const refName = head.match(/^ref:\s*(.+?)\s*$/)?.[1] + if (!refName || !isSafeRefName(refName)) { + // Detached HEAD already holds the object id; an unrecognized HEAD is covered by its own text. + return { text: head, mtimeMs: Number.NEGATIVE_INFINITY } + } + // Per-worktree refs (`refs/bisect`, `refs/worktree`) live beside the checkout; branches are shared. + const [perWorktreeTip, sharedTip] = await Promise.all([ + readTrimmedFile(path.join(gitDir, refName)), + commonDir === gitDir ? Promise.resolve(null) : readTrimmedFile(path.join(commonDir, refName)) + ]) + const looseTip = perWorktreeTip ?? sharedTip + if (looseTip) { + // A loose ref shadows any packed entry, so its bytes settle the tip on their own. + return { + text: JSON.stringify([head, 'loose', perWorktreeTip ? 'worktree' : 'common', looseTip]), + mtimeMs: Number.NEGATIVE_INFINITY + } + } + const [packedRefs, reftable] = await Promise.all([ + readFileStampComponent(path.join(commonDir, 'packed-refs')), + readFileStampComponent(path.join(commonDir, 'reftable')) + ]) + return { + text: JSON.stringify([head, 'packed', packedRefs.text, reftable.text]), + mtimeMs: Math.max(packedRefs.mtimeMs, reftable.mtimeMs) + } +} + +/** Keep a hand-edited HEAD from steering the stamp outside the repo's ref store. */ +function isSafeRefName(refName: string): boolean { + const segments = refName.split(/[\\/]/) + return ( + segments[0] === 'refs' && + segments.length > 1 && + segments.every((segment) => segment.length > 0 && segment !== '.' && segment !== '..') && + !path.isAbsolute(refName) + ) +} + +async function readTrimmedFile(filePath: string): Promise { + try { + const trimmed = (await readFile(filePath, 'utf-8')).trim() + return trimmed.length > 0 ? trimmed : null + } catch (error) { + if (isMissingEntryError(error)) { + return null + } + throw error + } +} + +async function readFileStampComponent(filePath: string): Promise { + try { + const stats = await stat(filePath) + return { text: `${requireMtimeMs(stats.mtimeMs)}:${stats.size}`, mtimeMs: stats.mtimeMs } + } catch (error) { + if (isMissingEntryError(error)) { + return ABSENT + } + throw error + } +} + +async function readWorkingTreeComponent(filePath: string): Promise { + try { + const stats = await stat(filePath) + // Why ino is optional: an atomic-rename save can keep both the size and the mtime bucket, + // so a changed inode is extra proof — but Windows reports 0 for it on the network + // redirector behind `\\wsl.localhost`, and requiring an unstable 0 to match would make + // the stamp never hit on exactly the host this cache exists for. Fold it in only when + // the filesystem gives a real one. + const inode = isUsableInode(stats.ino) ? String(stats.ino) : MISSING + return { + text: `${requireMtimeMs(stats.mtimeMs)}:${stats.size}:${inode}`, + mtimeMs: stats.mtimeMs + } + } catch (error) { + if (isMissingEntryError(error)) { + return ABSENT + } + throw error + } +} + +function isUsableInode(ino: unknown): boolean { + return typeof ino === 'number' && Number.isFinite(ino) && ino !== 0 +} + +/** A filesystem (or a stub) that reports no usable mtime cannot prove anything unchanged. */ +function requireMtimeMs(mtimeMs: unknown): number { + if (typeof mtimeMs !== 'number' || !Number.isFinite(mtimeMs)) { + throw new Error('stat reported no usable mtime') + } + return mtimeMs +} + +function isMissingEntryError(error: unknown): boolean { + const code = (error as NodeJS.ErrnoException | null)?.code + return code === 'ENOENT' || code === 'ENOTDIR' +} diff --git a/src/main/git/status-conflict-operations.test.ts b/src/main/git/status-conflict-operations.test.ts index e99c43532f4..5b61932578f 100644 --- a/src/main/git/status-conflict-operations.test.ts +++ b/src/main/git/status-conflict-operations.test.ts @@ -15,7 +15,7 @@ const { readFileMock, statMock, rmMock, - existsSyncMock + accessMock } = vi.hoisted(() => ({ gitExecFileAsyncMock: vi.fn(), gitExecFileAsyncBufferMock: vi.fn(), @@ -25,7 +25,7 @@ const { readFileMock: vi.fn(), statMock: vi.fn(), rmMock: vi.fn(), - existsSyncMock: vi.fn() + accessMock: vi.fn() })) vi.mock('./runner', () => @@ -37,13 +37,16 @@ vi.mock('./runner', () => ) vi.mock('fs/promises', () => - createFsPromisesModuleMock({ lstatMock, realpathMock, readFileMock, statMock, rmMock }) + createFsPromisesModuleMock({ + lstatMock, + realpathMock, + readFileMock, + statMock, + rmMock, + accessMock + }) ) -vi.mock('fs', () => ({ - existsSync: existsSyncMock -})) - vi.mock('../../shared/node-bounded-file-reader', async (importOriginal) => createBoundedFileReaderModuleMock(await importOriginal(), { readFileMock, @@ -83,32 +86,65 @@ describe('abortRebase', () => { describe('detectConflictOperation', () => { beforeEach(() => { readFileMock.mockReset() - existsSyncMock.mockReset() + accessMock.mockReset() }) it('ignores a stale REBASE_HEAD when no rebase directory exists', async () => { readFileMock.mockResolvedValue('gitdir: /repo/.git/worktrees/feature\n') - existsSyncMock.mockImplementation((target: string) => { - if (target.endsWith('MERGE_HEAD')) { - return false - } - if (target.endsWith('CHERRY_PICK_HEAD')) { - return false - } - if (target.endsWith('rebase-merge')) { - return false - } - if (target.endsWith('rebase-apply')) { - return false - } + accessMock.mockImplementation(async (target: string) => { + // Only REBASE_HEAD is present: the marker git leaves behind after a rebase finishes. if (target.endsWith('REBASE_HEAD')) { - return true + return undefined } - return false + throw Object.assign(new Error(`ENOENT: ${target}`), { code: 'ENOENT' }) }) const result = await detectConflictOperation('/repo') expect(result).toBe('unknown') }) + + it.each([ + ['MERGE_HEAD', 'merge'], + ['rebase-merge', 'rebase'], + ['rebase-apply', 'rebase'], + ['CHERRY_PICK_HEAD', 'cherry-pick'] + ])('reports %s as %s', async (marker, expected) => { + readFileMock.mockResolvedValue('gitdir: /repo/.git/worktrees/feature\n') + accessMock.mockImplementation(async (target: string) => { + if (target.endsWith(marker)) { + return undefined + } + throw Object.assign(new Error(`ENOENT: ${target}`), { code: 'ENOENT' }) + }) + + await expect(detectConflictOperation('/repo')).resolves.toBe(expected) + }) + + // The four markers are independent, so serializing them costs four round trips + // on a UNC git dir for something one wave answers. + it('probes every marker concurrently', async () => { + readFileMock.mockResolvedValue('gitdir: /repo/.git/worktrees/feature\n') + let concurrent = 0 + let peakConcurrent = 0 + accessMock.mockImplementation(async () => { + concurrent += 1 + peakConcurrent = Math.max(peakConcurrent, concurrent) + await Promise.resolve() + concurrent -= 1 + throw Object.assign(new Error('ENOENT'), { code: 'ENOENT' }) + }) + + await detectConflictOperation('/repo') + + expect(accessMock).toHaveBeenCalledTimes(4) + expect(peakConcurrent).toBe(4) + }) + + it('reads as unknown when the git dir cannot be reached at all', async () => { + readFileMock.mockRejectedValue(Object.assign(new Error('EIO'), { code: 'EIO' })) + accessMock.mockRejectedValue(Object.assign(new Error('EIO'), { code: 'EIO' })) + + await expect(detectConflictOperation('/repo')).resolves.toBe('unknown') + }) }) diff --git a/src/main/git/status-diff-settled-cache.test.ts b/src/main/git/status-diff-settled-cache.test.ts new file mode 100644 index 00000000000..1e5cf2231b1 --- /dev/null +++ b/src/main/git/status-diff-settled-cache.test.ts @@ -0,0 +1,331 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import type * as BoundedFileReader from '../../shared/node-bounded-file-reader' +import { createBoundedFileReaderModuleMock, createGitRunnerModuleMock } from './status-test-harness' + +const { + gitExecFileAsyncMock, + gitExecFileAsyncBufferMock, + gitStreamOptionsMock, + lstatMock, + realpathMock, + rmMock, + existsSyncMock +} = vi.hoisted(() => ({ + gitExecFileAsyncMock: vi.fn(), + gitExecFileAsyncBufferMock: vi.fn(), + gitStreamOptionsMock: vi.fn(), + lstatMock: vi.fn(), + realpathMock: vi.fn(), + rmMock: vi.fn(), + existsSyncMock: vi.fn() +})) + +/** + * A tiny in-memory filesystem, because every assertion here is about what the + * cache does when one specific path's mtime or bytes move. Sequenced + * `mockResolvedValueOnce` stacks cannot express that: the stamp and the diff + * read touch overlapping paths in an order the test should not have to know. + */ +type FakeFile = { content: Buffer; mtimeMs: number; ino: number } + +const { files } = vi.hoisted(() => ({ files: new Map() })) + +const { readFileMock, statMock, accessMock } = vi.hoisted(() => { + const missing = (target: string): NodeJS.ErrnoException => + Object.assign(new Error(`ENOENT: ${target}`), { code: 'ENOENT' }) + return { + readFileMock: vi.fn(async (target: string, encoding?: BufferEncoding) => { + const file = files.get(target) + if (!file) { + throw missing(target) + } + return encoding ? file.content.toString(encoding) : file.content + }), + statMock: vi.fn(async (target: string) => { + const file = files.get(target) + if (!file) { + throw missing(target) + } + return { + isFile: () => true, + size: file.content.byteLength, + mtimeMs: file.mtimeMs, + ino: file.ino + } + }), + accessMock: vi.fn(async (target: string) => { + if (!files.has(target)) { + throw missing(target) + } + }) + } +}) + +vi.mock('./runner', () => + createGitRunnerModuleMock({ + gitExecFileAsyncMock, + gitExecFileAsyncBufferMock, + gitStreamOptionsMock + }) +) + +vi.mock('fs/promises', () => ({ + lstat: lstatMock, + realpath: realpathMock, + readFile: readFileMock, + stat: statMock, + rm: rmMock, + access: accessMock +})) + +vi.mock('fs', () => ({ existsSync: existsSyncMock })) + +vi.mock('../../shared/node-bounded-file-reader', async (importOriginal) => + createBoundedFileReaderModuleMock(await importOriginal(), { + readFileMock, + statMock + }) +) + +import { getDiff, getStatus, invalidateGitReadCaches, stageFile } from './status' +import { settledDiffCache } from './source-control/git-read-cache-invalidation' + +const REPO = '/repo' +const FILE = 'src/file.ts' +const WORKING_TREE_PATH = `${REPO}/${FILE}` +const HEAD_PATH = `${REPO}/.git/HEAD` +const REF_PATH = `${REPO}/.git/refs/heads/main` +const INDEX_PATH = `${REPO}/.git/index` +const GITMODULES_PATH = `${REPO}/.gitmodules` + +// Old enough that a further write is guaranteed to move the mtime, which is what +// lets the cache store at all. +const SETTLED_MTIME_MS = Date.now() - 60_000 +let nextInode = 100 + +function writeFile(target: string, content: string, mtimeMs = SETTLED_MTIME_MS): void { + files.set(target, { content: Buffer.from(content), mtimeMs, ino: (nextInode += 1) }) +} + +function blobReadCount(): number { + return gitExecFileAsyncBufferMock.mock.calls.length +} + +function seedRepo(): void { + files.clear() + // No `.git` file entry: `.git` is a directory, so reading it as a pointer misses. + writeFile(HEAD_PATH, 'ref: refs/heads/main\n') + writeFile(REF_PATH, `${'a'.repeat(40)}\n`) + writeFile(INDEX_PATH, 'index-bytes') + writeFile(WORKING_TREE_PATH, 'working-tree-content') +} + +describe('settled diff cache', () => { + beforeEach(() => { + gitExecFileAsyncMock.mockReset() + gitExecFileAsyncBufferMock.mockReset() + gitStreamOptionsMock.mockReset() + readFileMock.mockClear() + statMock.mockClear() + accessMock.mockClear() + existsSyncMock.mockReset() + invalidateGitReadCaches() + settledDiffCache.resetStatsForTests() + seedRepo() + // `.gitmodules` is absent, so submodule routing resolves to "no submodules". + gitExecFileAsyncMock.mockResolvedValue({ stdout: '', stderr: '' }) + gitExecFileAsyncBufferMock.mockResolvedValue({ stdout: Buffer.from('index-content\n') }) + }) + + it('serves the second read of an unchanged file without respawning git', async () => { + const first = await getDiff(REPO, FILE, false) + const spawnsAfterFirst = blobReadCount() + + const second = await getDiff(REPO, FILE, false) + + expect(spawnsAfterFirst).toBeGreaterThan(0) + expect(blobReadCount()).toBe(spawnsAfterFirst) + expect(second).toEqual(first) + expect(settledDiffCache.stats().hits).toBe(1) + }) + + // The four invalidation axes, one per diff input. Each proves the stale result is + // never served, which matters more than any of the hits above. + it.each([ + [ + 'the working tree file is edited', + () => writeFile(WORKING_TREE_PATH, 'edited-in-another-editor') + ], + ['the index is rewritten by git add', () => writeFile(INDEX_PATH, 'index-bytes-after-add')], + ['HEAD moves to a new commit', () => writeFile(REF_PATH, `${'b'.repeat(40)}\n`)], + ['HEAD is detached onto another commit', () => writeFile(HEAD_PATH, `${'c'.repeat(40)}\n`)], + ['.gitmodules appears', () => writeFile(GITMODULES_PATH, '[submodule "vendor"]\n')] + ])('re-reads after %s', async (_name, mutate) => { + await getDiff(REPO, FILE, false) + const spawnsAfterFirst = blobReadCount() + + mutate() + gitExecFileAsyncBufferMock.mockResolvedValue({ stdout: Buffer.from('fresh-index-content\n') }) + const second = await getDiff(REPO, FILE, false) + + expect(blobReadCount()).toBeGreaterThan(spawnsAfterFirst) + expect(second).toMatchObject({ originalContent: 'fresh-index-content\n' }) + }) + + it('re-reads after a mutation runs through the shared invalidation point', async () => { + await getDiff(REPO, FILE, false) + const spawnsAfterFirst = blobReadCount() + + await stageFile(REPO, FILE) + await getDiff(REPO, FILE, false) + + expect(blobReadCount()).toBeGreaterThan(spawnsAfterFirst) + }) + + // The dangerous ordering: the read observed pre-mutation state, so storing its + // result after the mutation would pin a diff that was already wrong. + it('refuses to store a result for a read that a mutation overtook', async () => { + let releaseBlob = (): void => {} + const blocked = new Promise<{ stdout: Buffer }>((resolve) => { + releaseBlob = () => resolve({ stdout: Buffer.from('pre-mutation\n') }) + }) + gitExecFileAsyncBufferMock.mockReturnValue(blocked) + + const inFlight = getDiff(REPO, FILE, false) + await vi.waitFor(() => expect(blobReadCount()).toBeGreaterThan(0)) + invalidateGitReadCaches() + releaseBlob() + await inFlight + + expect(settledDiffCache.stats().invalidatedDuringRead).toBe(1) + expect(settledDiffCache.stats().entries).toBe(0) + + const spawnsAfterFirst = blobReadCount() + gitExecFileAsyncBufferMock.mockResolvedValue({ stdout: Buffer.from('post-mutation\n') }) + const second = await getDiff(REPO, FILE, false) + + expect(blobReadCount()).toBeGreaterThan(spawnsAfterFirst) + expect(second).toMatchObject({ originalContent: 'post-mutation\n' }) + }) + + // The stamp is itself several awaited stats, so a mutation can begin and end entirely + // inside it — leaving a stamp torn across the mutation that no later stamp can match. + it('refuses to store a result for a mutation that landed inside the stamp read', async () => { + const baseStat = statMock.getMockImplementation() + if (!baseStat) { + throw new Error('the fake filesystem lost its stat implementation') + } + let invalidated = false + statMock.mockImplementation(async (target: string) => { + if (!invalidated && target === INDEX_PATH) { + invalidated = true + invalidateGitReadCaches() + } + return baseStat(target) + }) + try { + await getDiff(REPO, FILE, false) + } finally { + statMock.mockImplementation(baseStat) + } + + expect(invalidated).toBe(true) + expect(settledDiffCache.stats().invalidatedDuringRead).toBe(1) + expect(settledDiffCache.stats().entries).toBe(0) + }) + + // A write inside the mtime granularity window could be overwritten again without + // moving the timestamp, so that read is not allowed to become a cache entry. + it('refuses to store a diff of a file written moments ago', async () => { + writeFile(WORKING_TREE_PATH, 'just-saved', Date.now()) + + await getDiff(REPO, FILE, false) + const spawnsAfterFirst = blobReadCount() + await getDiff(REPO, FILE, false) + + expect(blobReadCount()).toBeGreaterThan(spawnsAfterFirst) + expect(settledDiffCache.stats().racyWrites).toBeGreaterThan(0) + expect(settledDiffCache.stats().entries).toBe(0) + }) + + // A folder workspace, or any path that is not a git checkout, cannot be stamped. + it('never caches when the repo layout cannot be stamped', async () => { + files.delete(HEAD_PATH) + + await getDiff(REPO, FILE, false) + const spawnsAfterFirst = blobReadCount() + await getDiff(REPO, FILE, false) + + expect(blobReadCount()).toBeGreaterThan(spawnsAfterFirst) + expect(settledDiffCache.stats().unprovable).toBeGreaterThan(0) + expect(settledDiffCache.stats().entries).toBe(0) + }) + + // A WSL relay that never reached git returns the same empty left side as a new + // file does, so persisting it would pin a wrong diff until something else moved. + it('refuses to store a diff whose blob read failed rather than proved absence', async () => { + gitExecFileAsyncBufferMock.mockRejectedValue( + Object.assign(new Error('wsl.exe failed'), { code: 1 }) + ) + + await getDiff(REPO, FILE, false) + const spawnsAfterFirst = blobReadCount() + await getDiff(REPO, FILE, false) + + expect(blobReadCount()).toBeGreaterThan(spawnsAfterFirst) + expect(settledDiffCache.stats().entries).toBe(0) + }) + + it('caches a new file whose absence from the index git actually reported', async () => { + gitExecFileAsyncBufferMock.mockRejectedValue( + Object.assign(new Error("fatal: path 'src/file.ts' does not exist"), { code: 128 }) + ) + + await getDiff(REPO, FILE, false) + const spawnsAfterFirst = blobReadCount() + await getDiff(REPO, FILE, false) + + expect(blobReadCount()).toBe(spawnsAfterFirst) + expect(settledDiffCache.stats().hits).toBe(1) + }) + + it('keeps staged and unstaged diffs of one file in separate entries', async () => { + await getDiff(REPO, FILE, false) + const spawnsAfterUnstaged = blobReadCount() + + await getDiff(REPO, FILE, true) + + expect(blobReadCount()).toBeGreaterThan(spawnsAfterUnstaged) + }) + + // A staged diff compares HEAD to the index, so a working-tree edit must not evict it. + it('keeps a staged diff across a working-tree edit', async () => { + await getDiff(REPO, FILE, true) + const spawnsAfterFirst = blobReadCount() + + writeFile(WORKING_TREE_PATH, 'edited-after-staging') + await getDiff(REPO, FILE, true) + + expect(blobReadCount()).toBe(spawnsAfterFirst) + }) + + it('does not let a status poll drop the in-flight diff read', async () => { + let releaseBlob = (): void => {} + const blocked = new Promise<{ stdout: Buffer }>((resolve) => { + releaseBlob = () => resolve({ stdout: Buffer.from('index-content\n') }) + }) + gitExecFileAsyncBufferMock.mockReturnValue(blocked) + + const first = getDiff(REPO, FILE, false) + await vi.waitFor(() => expect(blobReadCount()).toBeGreaterThan(0)) + const spawnsBeforePoll = blobReadCount() + + await getStatus(REPO) + const second = getDiff(REPO, FILE, false) + releaseBlob() + await Promise.all([first, second]) + + // Why exactly equal: the second read must join the first, not start its own spawns. + expect(blobReadCount()).toBe(spawnsBeforePoll) + }) +}) diff --git a/src/main/git/status-diff.test.ts b/src/main/git/status-diff.test.ts index df59b1b9a3f..d66c64e7a89 100644 --- a/src/main/git/status-diff.test.ts +++ b/src/main/git/status-diff.test.ts @@ -102,7 +102,8 @@ describe('getDiff', () => { expect(gitExecFileAsyncBufferMock).toHaveBeenCalledWith(['show', ':src/file.ts'], { cwd: '/repo', - maxBuffer: 10 * 1024 * 1024 + maxBuffer: 10 * 1024 * 1024, + preferWslDirectGit: true }) expect(readFileMock).toHaveBeenCalledWith(path.join('/repo', 'src/file.ts')) expect(result).toEqual({ @@ -122,7 +123,8 @@ describe('getDiff', () => { expect(gitExecFileAsyncBufferMock).toHaveBeenCalledWith(['show', ':src/file.ts'], { cwd: '/repo', - maxBuffer: 10 * 1024 * 1024 + maxBuffer: 10 * 1024 * 1024, + preferWslDirectGit: true }) }) @@ -139,7 +141,8 @@ describe('getDiff', () => { ['show', '--end-of-options', 'HEAD:src/file.ts'], { cwd: '/repo', - maxBuffer: 10 * 1024 * 1024 + maxBuffer: 10 * 1024 * 1024, + preferWslDirectGit: true } ) expect(result.originalContent).toBe('head-content\n') @@ -158,15 +161,19 @@ describe('getDiff', () => { }) it('does not read oversized working-tree files into memory', async () => { + const workingTreePath = path.join('/repo', 'dist/large.log') gitExecFileAsyncBufferMock.mockResolvedValueOnce({ stdout: Buffer.from('index-content\n') }) - statMock.mockResolvedValueOnce({ - isFile: () => true, - size: 10 * 1024 * 1024 + 1 - }) + // Why by path: the diff also stats git-dir entries to stamp its inputs, so a + // one-shot queue would hand the oversized size to whichever stat ran first. + statMock.mockImplementation(async (target: string) => + target === workingTreePath + ? { isFile: () => true, size: 10 * 1024 * 1024 + 1 } + : { isFile: () => true, size: 12 } + ) const result = await getDiff('/repo', 'dist/large.log', false) - expect(readFileMock).not.toHaveBeenCalled() + expect(readFileMock).not.toHaveBeenCalledWith(workingTreePath) expect(result.kind).toBe('binary') expect(result.modifiedIsBinary).toBe(true) expect(result.modifiedContent).toBe('') @@ -259,8 +266,14 @@ describe('getDiff', () => { it('flags a deleted image so previewers can fall back to the original bytes', async () => { const pngBuffer = Buffer.from([0x89, 0x50, 0x4e, 0x47, 0x00]) + const workingTreePath = path.join('/repo', 'assets/deleted.png') gitExecFileAsyncBufferMock.mockResolvedValueOnce({ stdout: pngBuffer }) - statMock.mockRejectedValueOnce(Object.assign(new Error('missing'), { code: 'ENOENT' })) + statMock.mockImplementation(async (target: string) => { + if (target === workingTreePath) { + throw Object.assign(new Error('missing'), { code: 'ENOENT' }) + } + return { isFile: () => true, size: 12 } + }) const result = await getDiff('/repo', 'assets/deleted.png', false) @@ -275,9 +288,15 @@ describe('getDiff', () => { it('does not treat an unreadable working-tree image as a deletion', async () => { const pngBuffer = Buffer.from([0x89, 0x50, 0x4e, 0x47, 0x00]) + const workingTreePath = path.join('/repo', 'assets/unreadable.png') gitExecFileAsyncBufferMock.mockResolvedValueOnce({ stdout: pngBuffer }) - statMock.mockResolvedValueOnce({ isFile: () => true, size: 5 }) - readFileMock.mockRejectedValueOnce(new Error('EIO')) + statMock.mockImplementation(async () => ({ isFile: () => true, size: 5 })) + readFileMock.mockImplementation(async (target: string) => { + if (target === workingTreePath) { + throw new Error('EIO') + } + return Buffer.from('') + }) const result = await getDiff('/repo', 'assets/unreadable.png', false) diff --git a/src/main/git/status-submodule.test.ts b/src/main/git/status-submodule.test.ts index 138d2874b19..b67a4eca9a0 100644 --- a/src/main/git/status-submodule.test.ts +++ b/src/main/git/status-submodule.test.ts @@ -123,11 +123,11 @@ describe('submodule diff routing', () => { expect(gitExecFileAsyncBufferMock).toHaveBeenCalledWith( ['show', '--end-of-options', `${OLD_OID}:lib/main.dart`], - { cwd: SUBMODULE, maxBuffer: 10 * 1024 * 1024 } + { cwd: SUBMODULE, maxBuffer: 10 * 1024 * 1024, preferWslDirectGit: true } ) expect(gitExecFileAsyncBufferMock).toHaveBeenCalledWith( ['show', '--end-of-options', `${NEW_OID}:lib/main.dart`], - { cwd: SUBMODULE, maxBuffer: 10 * 1024 * 1024 } + { cwd: SUBMODULE, maxBuffer: 10 * 1024 * 1024, preferWslDirectGit: true } ) expect(result.kind).toBe('text') expect(result.originalContent).toBe('v1\n') @@ -167,11 +167,11 @@ describe('submodule diff routing', () => { expect(gitExecFileAsyncBufferMock).toHaveBeenCalledWith( ['show', '--end-of-options', `${OLD_OID}:lib/main.dart`], - { cwd: SUBMODULE, maxBuffer: 10 * 1024 * 1024 } + { cwd: SUBMODULE, maxBuffer: 10 * 1024 * 1024, preferWslDirectGit: true } ) expect(gitExecFileAsyncBufferMock).toHaveBeenCalledWith( ['show', '--end-of-options', `${NEW_OID}:lib/main.dart`], - { cwd: SUBMODULE, maxBuffer: 10 * 1024 * 1024 } + { cwd: SUBMODULE, maxBuffer: 10 * 1024 * 1024, preferWslDirectGit: true } ) expect(result.kind).toBe('text') expect(result.originalContent).toBe('v1\n') @@ -202,7 +202,8 @@ describe('submodule diff routing', () => { expect(gitExecFileAsyncBufferMock).toHaveBeenCalledWith(['show', ':lib/main.dart'], { cwd: SUBMODULE, - maxBuffer: 10 * 1024 * 1024 + maxBuffer: 10 * 1024 * 1024, + preferWslDirectGit: true }) expect(readFileMock).toHaveBeenCalledWith(path.join(SUBMODULE, 'lib/main.dart')) expect(result.kind).toBe('text') diff --git a/src/main/git/status-test-harness.ts b/src/main/git/status-test-harness.ts index eda7e2f5014..99925f90363 100644 --- a/src/main/git/status-test-harness.ts +++ b/src/main/git/status-test-harness.ts @@ -15,6 +15,8 @@ export type FsPromisesMocks = { readFileMock: MockFn statMock: MockFn rmMock: MockFn + /** Optional: defaults to "nothing exists", which is what most git-read tests assume. */ + accessMock?: MockFn } export function createGitRunnerModuleMock(mocks: GitRunnerMocks): Record { @@ -49,7 +51,12 @@ export function createFsPromisesModuleMock(mocks: FsPromisesMocks): Record { + throw Object.assign(new Error(`ENOENT: ${target}`), { code: 'ENOENT' }) + }) } } diff --git a/src/main/git/status.test.ts b/src/main/git/status.test.ts index e658e773743..9663becb622 100644 --- a/src/main/git/status.test.ts +++ b/src/main/git/status.test.ts @@ -15,6 +15,7 @@ const { readFileMock, statMock, rmMock, + accessMock, existsSyncMock } = vi.hoisted(() => ({ gitExecFileAsyncMock: vi.fn(), @@ -25,6 +26,7 @@ const { readFileMock: vi.fn(), statMock: vi.fn(), rmMock: vi.fn(), + accessMock: vi.fn(), existsSyncMock: vi.fn() })) @@ -37,9 +39,17 @@ vi.mock('./runner', () => ) vi.mock('fs/promises', () => - createFsPromisesModuleMock({ lstatMock, realpathMock, readFileMock, statMock, rmMock }) + createFsPromisesModuleMock({ + lstatMock, + realpathMock, + readFileMock, + statMock, + rmMock, + accessMock + }) ) +// Why still here: unmerged-entry parsing probes the working tree through node:fs directly. vi.mock('fs', () => ({ existsSync: existsSyncMock })) @@ -62,6 +72,8 @@ describe('getStatus', () => { lstatMock.mockReset() readFileMock.mockReset() existsSyncMock.mockReset() + accessMock.mockReset() + accessMock.mockRejectedValue(Object.assign(new Error('ENOENT'), { code: 'ENOENT' })) // Why: untracked line counting stats a file before reading it; any // under-limit size routes the read to readFileMock. statMock.mockReset() @@ -75,7 +87,12 @@ describe('getStatus', () => { it('parses unmerged porcelain v2 entries into unresolved conflict rows', async () => { readFileMock.mockResolvedValue('gitdir: /repo/.git/worktrees/feature\n') - existsSyncMock.mockImplementation((target: string) => target.endsWith('MERGE_HEAD')) + accessMock.mockImplementation(async (target: string) => { + if (target.endsWith('MERGE_HEAD')) { + return undefined + } + throw Object.assign(new Error('ENOENT'), { code: 'ENOENT' }) + }) gitExecFileAsyncMock.mockResolvedValueOnce({ stdout: 'u UU N... 100644 100644 100644 100644 aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb cccccccccccccccccccccccccccccccccccccccc src/app.ts\n' @@ -97,7 +114,6 @@ describe('getStatus', () => { it('maps deleted conflicts to deleted when the working tree file is absent', async () => { readFileMock.mockResolvedValue('gitdir: /repo/.git/worktrees/feature\n') - existsSyncMock.mockReturnValue(false) gitExecFileAsyncMock.mockResolvedValueOnce({ stdout: 'u UD N... 100644 100644 000000 100644 aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb cccccccccccccccccccccccccccccccccccccccc src/deleted.ts\n' @@ -132,7 +148,6 @@ describe('getStatus', () => { it('passes core.quotePath=false and round-trips UTF-8 paths', async () => { readFileMock.mockResolvedValue('gitdir: /repo/.git/worktrees/feature\n') - existsSyncMock.mockReturnValue(false) gitExecFileAsyncMock.mockResolvedValueOnce({ stdout: '1 .M N... 100644 100644 100644 ce013625030ba8dba906f756967f9e9ca394464a ce013625030ba8dba906f756967f9e9ca394464a docs/日本語/sample.md\n' @@ -159,7 +174,6 @@ describe('getStatus', () => { it('preserves porcelain v2 submodule dirtiness flags on status rows', async () => { readFileMock.mockResolvedValue('gitdir: /repo/.git/worktrees/feature\n') - existsSyncMock.mockReturnValue(false) gitExecFileAsyncMock.mockResolvedValueOnce({ stdout: '1 AM S..U 000000 160000 160000 0000000000000000000000000000000000000000 7844cb64e631f17a9ca5b548f3500ef7cecd2f17 nested-repo\n' @@ -185,7 +199,6 @@ describe('getStatus', () => { it('omits ignored files by default and parses them when requested', async () => { readFileMock.mockResolvedValue('gitdir: /repo/.git/worktrees/feature\n') - existsSyncMock.mockReturnValue(false) gitExecFileAsyncMock.mockResolvedValueOnce({ stdout: '! dist/\n! generated/file.js\n' }) @@ -206,7 +219,6 @@ describe('getStatus', () => { it('parses branch identity from porcelain v2 branch headers', async () => { readFileMock.mockResolvedValue('gitdir: /repo/.git/worktrees/feature\n') - existsSyncMock.mockReturnValue(false) gitExecFileAsyncMock.mockResolvedValueOnce({ stdout: '# branch.oid abcdef1234567890\n# branch.head feature/prompts\n1 .M N... 100644 100644 100644 ce013625030ba8dba906f756967f9e9ca394464a ce013625030ba8dba906f756967f9e9ca394464a src/app.ts\n' @@ -222,7 +234,6 @@ describe('getStatus', () => { it('folds upstream ahead/behind from porcelain v2 into the status result', async () => { readFileMock.mockResolvedValue('gitdir: /repo/.git/worktrees/feature\n') - existsSyncMock.mockReturnValue(false) gitExecFileAsyncMock.mockResolvedValueOnce({ stdout: '# branch.oid abcdef1234567890\n# branch.head feature/prompts\n# branch.upstream origin/feature/prompts\n# branch.ab +2 -3\n' @@ -241,7 +252,6 @@ describe('getStatus', () => { it('reports no upstream from porcelain v2 status when no same-name origin branch exists', async () => { readFileMock.mockResolvedValue('gitdir: /repo/.git/worktrees/feature\n') - existsSyncMock.mockReturnValue(false) gitExecFileAsyncMock.mockImplementation((args: string[]) => { if (args[0] === '-c' && args.includes('status')) { return Promise.resolve({ @@ -274,7 +284,6 @@ describe('getStatus', () => { it('uses same-name origin branch status for legacy base-tracking worktrees', async () => { readFileMock.mockResolvedValue('gitdir: /repo/.git/worktrees/feature\n') - existsSyncMock.mockReturnValue(false) gitExecFileAsyncMock .mockResolvedValueOnce({ stdout: @@ -297,7 +306,6 @@ describe('getStatus', () => { it('omits --ignored and ignoredPaths when includeIgnored is not requested', async () => { readFileMock.mockResolvedValue('gitdir: /repo/.git/worktrees/feature\n') - existsSyncMock.mockReturnValue(false) gitExecFileAsyncMock.mockResolvedValueOnce({ stdout: '' }) const result = await getStatus('/repo') @@ -315,7 +323,6 @@ describe('getStatus', () => { it('parses ! porcelain v2 records into ignoredPaths when includeIgnored is true', async () => { readFileMock.mockResolvedValue('gitdir: /repo/.git/worktrees/feature\n') - existsSyncMock.mockReturnValue(false) gitExecFileAsyncMock.mockResolvedValueOnce({ stdout: '! dist/\n! .env\n! coverage/\n' }) @@ -337,7 +344,6 @@ describe('getStatus', () => { it('attaches per-area line counts from staged and unstaged numstat', async () => { readFileMock.mockResolvedValue('gitdir: /repo/.git/worktrees/feature\n') - existsSyncMock.mockReturnValue(false) gitExecFileAsyncMock.mockImplementation((args: string[]) => { if (args.includes('status')) { return Promise.resolve({ @@ -364,7 +370,6 @@ describe('getStatus', () => { it('reuses unchanged line stats only when the safety hint is present', async () => { readFileMock.mockResolvedValue('gitdir: /repo/.git/worktrees/feature\n') - existsSyncMock.mockReturnValue(false) gitExecFileAsyncMock.mockImplementation((args: string[]) => { if (args.includes('status')) { return Promise.resolve({ @@ -392,7 +397,6 @@ describe('getStatus', () => { it('recomputes after a scan whose numstat failed instead of pinning missing counts', async () => { readFileMock.mockResolvedValue('gitdir: /repo/.git/worktrees/feature\n') - existsSyncMock.mockReturnValue(false) let failNumstat = true gitExecFileAsyncMock.mockImplementation((args: string[]) => { if (args.includes('status')) { @@ -422,7 +426,6 @@ describe('getStatus', () => { it('invalidates safety reuse for a new head and for known mutations', async () => { readFileMock.mockResolvedValue('gitdir: /repo/.git/worktrees/feature\n') - existsSyncMock.mockReturnValue(false) let head = 'head-1' gitExecFileAsyncMock.mockImplementation((args: string[]) => { if (args.includes('status')) { @@ -450,7 +453,6 @@ describe('getStatus', () => { it('isolates line-stat reuse between WSL distributions', async () => { readFileMock.mockResolvedValue('gitdir: /repo/.git/worktrees/feature\n') - existsSyncMock.mockReturnValue(false) gitExecFileAsyncMock.mockImplementation((args: string[]) => { if (args.includes('status')) { return Promise.resolve({ @@ -474,7 +476,6 @@ describe('getStatus', () => { it('attaches numstat counts for literal paths containing rename markers', async () => { readFileMock.mockResolvedValue('gitdir: /repo/.git/worktrees/feature\n') - existsSyncMock.mockReturnValue(false) gitExecFileAsyncMock.mockImplementation((args: string[]) => { if (args.includes('status')) { return Promise.resolve({ @@ -503,7 +504,6 @@ describe('getStatus', () => { it('attaches staged rename counts to the new path', async () => { readFileMock.mockResolvedValue('gitdir: /repo/.git/worktrees/feature\n') - existsSyncMock.mockReturnValue(false) gitExecFileAsyncMock.mockImplementation((args: string[]) => { if (args.includes('status')) { return Promise.resolve({ @@ -531,7 +531,6 @@ describe('getStatus', () => { }) it('counts untracked file contents as additions', async () => { - existsSyncMock.mockReturnValue(false) lstatMock.mockResolvedValue({ size: 14, mtimeMs: 1, @@ -555,7 +554,6 @@ describe('getStatus', () => { it('leaves binary working-tree changes without counts', async () => { readFileMock.mockResolvedValue('gitdir: /repo/.git/worktrees/feature\n') - existsSyncMock.mockReturnValue(false) gitExecFileAsyncMock.mockImplementation((args: string[]) => { if (args.includes('status')) { return Promise.resolve({ @@ -578,7 +576,6 @@ describe('getStatus', () => { it('skips numstat entirely for a clean working tree', async () => { readFileMock.mockResolvedValue('gitdir: /repo/.git/worktrees/feature\n') - existsSyncMock.mockReturnValue(false) gitExecFileAsyncMock.mockResolvedValueOnce({ stdout: '' }) await getStatus('/repo') @@ -588,7 +585,6 @@ describe('getStatus', () => { it('truncates and flags didHitLimit when entries exceed the limit', async () => { readFileMock.mockResolvedValue('gitdir: /repo/.git/worktrees/feature\n') - existsSyncMock.mockReturnValue(false) const stdout = `${Array.from({ length: 25 }, (_, i) => `? file${i}.txt`).join('\n')}\n` gitExecFileAsyncMock.mockReset() gitExecFileAsyncMock.mockResolvedValue({ stdout: '' }) @@ -651,7 +647,6 @@ describe('getStatus', () => { it('does not flag didHitLimit for a normal repo under the limit', async () => { readFileMock.mockResolvedValue('gitdir: /repo/.git/worktrees/feature\n') - existsSyncMock.mockReturnValue(false) gitExecFileAsyncMock.mockReset() gitExecFileAsyncMock.mockResolvedValue({ stdout: '' }) gitExecFileAsyncMock.mockResolvedValueOnce({ stdout: '? a.txt\n? b.txt\n' }) diff --git a/src/main/git/wsl-git-read-environment.ts b/src/main/git/wsl-git-read-environment.ts index 7c809453498..e75efee30d4 100644 --- a/src/main/git/wsl-git-read-environment.ts +++ b/src/main/git/wsl-git-read-environment.ts @@ -7,10 +7,18 @@ import { export type WslGitReadEnvironment = { gitPath: string; home: string; path: string } const PROBE_TIMEOUT_MS = 10_000 +/** + * How long a read may wait for a cold probe before taking the login shell. + * Short enough that a wedged distro cannot stall the panel, long enough that a + * healthy one resolves and every later read runs shell-free. + */ +export const WSL_GIT_READ_ENVIRONMENT_WAIT_MS = 1_500 const PROBE_MAX_BUFFER = 64 * 1024 const TRANSIENT_PROBE_RETRY_MS = 30_000 const environmentByDistro = new Map>() -const resolvedEnvironmentByDistro = new Map() +// Why the null entries matter: a settled "no direct route" answer is what lets a read skip the +// bounded probe wait entirely instead of racing an already-decided promise on every call. +const settledEnvironmentByDistro = new Map() const transientRetryAfterByDistro = new Map() type ProbeOutcome = @@ -76,6 +84,7 @@ export function getWslGitReadEnvironment(distro: string): Promise= retryAfter) { environmentByDistro.delete(distro) + settledEnvironmentByDistro.delete(distro) transientRetryAfterByDistro.delete(distro) } let environment = environmentByDistro.get(distro) @@ -85,10 +94,11 @@ export function getWslGitReadEnvironment(distro: string): Promise ({ diffCache: settledDiffCache.stats() }) }) function focusExistingWindow(): void { focusExistingMainWindow({