From 482f8aa4b43ff15a58f3de79eb16b3464393a07e Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Thu, 3 Sep 2026 03:56:40 -0700 Subject: [PATCH] perf(worktrees): classify each worktree once, defer the SSH meta index, unserialise conflict probes Three redundancies on the worktree-catalog and git-status read paths: - buildDetectedGitWorktrees ran mergeWorktree + toDetectedWorktree twice for every visible row. Discovery backfill returns the same meta object when it wrote nothing, and both builders are pure over it, so skip the second pass on identity. - The SSH worktree-meta index parsed every worktree id on the host, then threw it away whenever the provider was connected. Build it lazily, memoised. - Unmerged `u` records were resolved one fs.access at a time. Resolve the prefix the cap can reach with 8-way concurrency, keyed by record index so Git's output order and error precedence are unchanged. --- src/main/git/source-control/status-read.ts | 30 ++- ...tected-provider-listing-meta-index.test.ts | 131 ++++++++++ .../listing/detected-provider-listing.ts | 15 +- .../detected-worktree-classification.test.ts | 234 ++++++++++++++++++ .../listing/ssh-worktree-fallback.ts | 15 +- .../listing/worktree-discovery-metadata.ts | 3 +- .../git-status-conflict-entries.test.ts | 212 ++++++++++++++++ src/shared/git-status-conflict-entries.ts | 44 ++++ 8 files changed, 669 insertions(+), 15 deletions(-) create mode 100644 src/main/ipc/worktrees/listing/detected-provider-listing-meta-index.test.ts create mode 100644 src/main/ipc/worktrees/listing/detected-worktree-classification.test.ts create mode 100644 src/shared/git-status-conflict-entries.test.ts diff --git a/src/main/git/source-control/status-read.ts b/src/main/git/source-control/status-read.ts index 276bf26e646..11779d8af3a 100644 --- a/src/main/git/source-control/status-read.ts +++ b/src/main/git/source-control/status-read.ts @@ -18,7 +18,10 @@ import { findExistingWorktreeSymlinkPaths } from '../worktree-symlink-detection' import type { GetStatusOptions } from './get-status-options' import { statusReadLeaseOwner } from './git-read-cache-invalidation' import { detectConflictOperation } from './git-conflict-operation' -import { parseUnmergedEntry } from '../../../shared/git-status-conflict-entries' +import { + parseUnmergedEntry, + resolveUnmergedStatusRecords +} from '../../../shared/git-status-conflict-entries' import { getEffectiveUpstreamStatusCacheKey } from './effective-upstream-status-cache' import { getShortBranchName, @@ -176,16 +179,37 @@ async function runGetStatus( // Why: git runs in the distro and answers in its namespace; the working-tree probes below run here. const hostWorktreePath = resolveWorktreeHostPath(worktreePath, options) ?? worktreePath + // Why: a record only pushes one entry, so the cap can never break before index `limit`; prefetching + // exactly that prefix keeps the probe count identical to a serial read on a truncated status. + const resolvableUnmergedEnd = didHitLimit + ? Math.min(parser.statusRecords.length, limit) + : parser.statusRecords.length + // Why: skip the prefetch entirely for the conflict-free poll so the common path allocates nothing extra. + const resolvedUnmerged = + parser.unmergedLines.length > 0 + ? await resolveUnmergedStatusRecords( + hostWorktreePath, + parser.statusRecords, + resolvableUnmergedEnd + ) + : undefined + // Why: resolve deferred conflicts in Git's output order so the cap cannot hide // an early conflict behind ordinary rows that appeared later in the stream. - for (const record of parser.statusRecords) { + for (const [index, record] of parser.statusRecords.entries()) { if (didHitLimit && entries.length >= limit) { break } if (record.type === 'entry') { entries.push(record.entry) } else { - const unmergedEntry = await parseUnmergedEntry(hostWorktreePath, record.line) + const prefetched = resolvedUnmerged?.[index] + if (prefetched?.ok === false) { + throw prefetched.error + } + const unmergedEntry = prefetched + ? prefetched.entry + : await parseUnmergedEntry(hostWorktreePath, record.line) if (unmergedEntry) { entries.push(unmergedEntry) } diff --git a/src/main/ipc/worktrees/listing/detected-provider-listing-meta-index.test.ts b/src/main/ipc/worktrees/listing/detected-provider-listing-meta-index.test.ts new file mode 100644 index 00000000000..4bf55e97492 --- /dev/null +++ b/src/main/ipc/worktrees/listing/detected-provider-listing-meta-index.test.ts @@ -0,0 +1,131 @@ +/** + * The SSH worktree-meta index is only ever read via `metaIndex.get(repo.id)` on the disconnected + * fallbacks, so a connected listing must not pay `parseWorktreeId` over the whole host snapshot. + */ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import type { Repo } from '../../../../shared/repo-types' +import type { Store } from '../../../persistence/loading-store/store' +import type * as SshWorktreeFallbackModule from './ssh-worktree-fallback' + +const { getSshGitProviderMock, indexBuildSpy } = vi.hoisted(() => ({ + getSshGitProviderMock: vi.fn(), + indexBuildSpy: vi.fn() +})) + +vi.mock('../../../providers/ssh-git-dispatch', () => ({ + getSshGitProvider: getSshGitProviderMock, + requireSshGitProvider: getSshGitProviderMock, + getSshGitProviderGeneration: () => 1 +})) + +vi.mock('./ssh-worktree-fallback', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + // Both builders are counted: the point is that NO index is built on the connected path. + createSshWorktreeMetaIndex: (...args: Parameters) => { + indexBuildSpy('all-hosts', ...args) + return actual.createSshWorktreeMetaIndex(...args) + }, + createSshWorktreeMetaIndexForRepo: ( + ...args: Parameters + ) => { + indexBuildSpy('repo-scoped', ...args) + return actual.createSshWorktreeMetaIndexForRepo(...args) + } + } +}) + +const { listDetectedWorktreesForCapturedRepo } = await import('./detected-provider-listing') + +const repo = { + id: 'repo-1', + path: '/home/user/repo', + displayName: 'repo', + connectionId: 'conn-1' +} as Repo + +const worktreeId = `${repo.id}::/home/user/feature` + +function createStore(): Store { + const rows: Record = { + [worktreeId]: { instanceId: 'instance-1' }, + // Other repos' rows share the host snapshot; only this repo's bucket is ever read back. + 'repo-2::/home/user/other': { instanceId: 'instance-2' } + } + return { + getRepos: () => [repo], + getRepo: () => repo, + getSettings: () => ({}), + getProjectHostSetups: () => [], + getAllWorktreeLineage: () => ({}), + getAllWorktreeMeta: () => rows, + getWorktreeMeta: (id: string) => rows[id], + setWorktreeMeta: vi.fn() + } as unknown as Store +} + +describe('SSH worktree meta index construction', () => { + beforeEach(() => { + indexBuildSpy.mockClear() + getSshGitProviderMock.mockReset() + }) + + it('does not build the index when the provider answers', async () => { + const provider = { + listWorktrees: vi.fn().mockResolvedValue([ + { path: repo.path, head: 'a', branch: 'main', isBare: false, isMainWorktree: true }, + { + path: '/home/user/feature', + head: 'b', + branch: 'feature', + isBare: false, + isMainWorktree: false + } + ]) + } + + const result = await listDetectedWorktreesForCapturedRepo( + createStore(), + repo, + () => true, + provider as never + ) + + expect(result).toMatchObject({ authoritative: true, source: 'git' }) + expect(indexBuildSpy).not.toHaveBeenCalled() + }) + + it('builds the index once when no provider is available', async () => { + const result = await listDetectedWorktreesForCapturedRepo( + createStore(), + repo, + () => true, + undefined + ) + + expect(result).toMatchObject({ authoritative: false, source: 'metadata-fallback' }) + expect(indexBuildSpy).toHaveBeenCalledTimes(1) + expect(indexBuildSpy).toHaveBeenCalledWith('all-hosts', expect.anything()) + expect( + (result as { worktrees: { id: string }[] }).worktrees.map((worktree) => worktree.id) + ).toEqual([worktreeId]) + }) + + it('builds the index once when the provider listing fails', async () => { + const provider = { listWorktrees: vi.fn().mockRejectedValue(new Error('relay down')) } + + const result = await listDetectedWorktreesForCapturedRepo( + createStore(), + repo, + () => true, + provider as never + ) + + expect(result).toMatchObject({ authoritative: false, source: 'metadata-fallback' }) + expect(indexBuildSpy).toHaveBeenCalledTimes(1) + expect( + (result as { worktrees: { id: string }[] }).worktrees.map((worktree) => worktree.id) + ).toEqual([worktreeId]) + }) +}) diff --git a/src/main/ipc/worktrees/listing/detected-provider-listing.ts b/src/main/ipc/worktrees/listing/detected-provider-listing.ts index 51a0618ba66..25c08d262fb 100644 --- a/src/main/ipc/worktrees/listing/detected-provider-listing.ts +++ b/src/main/ipc/worktrees/listing/detected-provider-listing.ts @@ -10,7 +10,8 @@ import type { ListDesktopLineageForHostArgs } from '../../../../shared/host-line import { buildDetectedGitWorktrees, createSshWorktreeMetaIndex, - listDisconnectedSshWorktrees + listDisconnectedSshWorktrees, + type SshWorktreeMetaIndex } from './ssh-worktree-fallback' import { buildDisconnectedDetectedWorktrees, @@ -42,9 +43,11 @@ export async function listDetectedWorktreesForCapturedRepo( const allMeta = isFolderRepo(repo) ? undefined : readAllWorktreeMetaForHost(store, getRepoExecutionHostId(repo)) - const sshWorktreeMetaIndex = repo.connectionId - ? createSshWorktreeMetaIndex(Object.entries(allMeta ?? {})) - : new Map() + // Why: only the disconnected fallbacks read this, so keep parseWorktreeId over the whole host snapshot + // off the connected path entirely. + let cachedSshWorktreeMetaIndex: SshWorktreeMetaIndex | undefined + const sshWorktreeMetaIndex = (): SshWorktreeMetaIndex => + (cachedSshWorktreeMetaIndex ??= createSshWorktreeMetaIndex(Object.entries(allMeta ?? {}))) try { let gitWorktrees: GitWorktreeInfo[] @@ -86,7 +89,7 @@ export async function listDetectedWorktreesForCapturedRepo( if (!isCurrent()) { return null } - const worktrees = listDisconnectedSshWorktrees(store, repo, sshWorktreeMetaIndex) + const worktrees = listDisconnectedSshWorktrees(store, repo, sshWorktreeMetaIndex()) return { repoId: repo.id, authoritative: false, @@ -158,7 +161,7 @@ export async function listDetectedWorktreesForCapturedRepo( err ) if (repo.connectionId) { - const worktrees = listDisconnectedSshWorktrees(store, repo, sshWorktreeMetaIndex) + const worktrees = listDisconnectedSshWorktrees(store, repo, sshWorktreeMetaIndex()) return { repoId: repo.id, authoritative: false, diff --git a/src/main/ipc/worktrees/listing/detected-worktree-classification.test.ts b/src/main/ipc/worktrees/listing/detected-worktree-classification.test.ts new file mode 100644 index 00000000000..6a25d696c9b --- /dev/null +++ b/src/main/ipc/worktrees/listing/detected-worktree-classification.test.ts @@ -0,0 +1,234 @@ +/** + * Guards the single-classification contract of `buildDetectedGitWorktrees`: every visible worktree + * used to be run through `mergeWorktree` + `toDetectedWorktree` twice per catalog pass. + */ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import type { Repo } from '../../../../shared/repo-types' +import type { Store } from '../../../persistence/loading-store/store' +import type { WorktreeMeta } from '../../../../shared/worktree/meta-types' +import type { GitWorktreeInfo } from '../../../../shared/worktree/types' +import type * as NodeCryptoModule from 'node:crypto' +import type * as OwnershipModule from '../../../../shared/worktree/ownership' + +const { toDetectedWorktreeSpy } = vi.hoisted(() => ({ toDetectedWorktreeSpy: vi.fn() })) + +vi.mock('../../../../shared/worktree/ownership', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + toDetectedWorktree: (args: Parameters[0]) => { + toDetectedWorktreeSpy(args) + return actual.toDetectedWorktree(args) + } + } +}) + +vi.mock('node:crypto', async (importOriginal) => ({ + ...(await importOriginal()), + randomUUID: () => 'fixed-instance-id' +})) + +const { buildDetectedGitWorktrees } = await import('./ssh-worktree-fallback') +const { getProjectHostSetupWorktreeMeta } = + await import('../../../../shared/project-host-setup-lookup') +const { mergeWorktree } = await import('../../worktree-logic') +const { resolveWorktreeMetaWithDiscoveryBackfill } = await import('./worktree-discovery-metadata') +const ownership = await import('../../../../shared/worktree/ownership') +const { projectResolvedWorktreeLineage } = + await import('../../../../shared/resolved-worktree-lineage') +const { createWorktreeVisibilitySourceMatcher, resolveCustomWorktreeVisibilitySources } = + await import('../../../../shared/worktree/visibility-sources') +const { resolveConfiguredWorktreeBasePaths } = + await import('../../../../shared/worktree/configured-worktree-base-path') +const { dedupeWorktreesByPath } = await import('../../worktree-path-comparison') +const { readWorktreeMetaForHost } = + await import('../../../persistence/host-qualified-worktree-meta') +const { getRepoOwnedWorktreeMeta } = await import('../../../worktree-metadata-ownership') +const { getRepoExecutionHostId } = await import('../../../../shared/execution-host') + +const repo: Repo = { + id: 'repo-1', + path: '/workspace/repo', + displayName: 'repo', + badgeColor: '#000', + addedAt: 0 +} as Repo + +const ownershipMeta = getProjectHostSetupWorktreeMeta([], repo) + +function gitWorktree(path: string): GitWorktreeInfo { + return { + path, + head: 'abc123', + branch: 'refs/heads/feature', + isBare: false, + isMainWorktree: false + } +} + +/** Fully settled metadata: discovery backfill has nothing to write, so it hands the same object back. */ +function settledMeta(overrides: Partial = {}): WorktreeMeta { + return { + ...ownershipMeta, + instanceId: 'instance-settled', + orcaCreatedAt: 1, + lastActivityAt: 5, + ...overrides + } as WorktreeMeta +} + +function createStore(meta: Record, repos: Repo[] = [repo]) { + const rows = { ...meta } + return { + getRepos: () => repos, + getSettings: () => ({ workspaceDir: '/workspace', nestWorkspaces: true }), + getProjectHostSetups: () => [], + getAllWorktreeLineage: () => ({}), + getAllWorktreeMeta: () => rows, + getWorktreeMeta: (id: string) => rows[id], + getWorktreeMetaForHost: (id: string, hostId: string) => + rows[id]?.hostId === hostId ? rows[id] : undefined, + getAllWorktreeMetaForHost: () => rows, + setWorktreeMeta: (id: string, patch: Partial) => { + rows[id] = { ...rows[id], ...patch } as WorktreeMeta + return rows[id] + }, + setWorktreeMetaForHost: (id: string, hostId: string, patch: Partial) => { + rows[id] = { ...rows[id], ...patch, hostId } as WorktreeMeta + return rows[id] + } + } as unknown as Store +} + +/** The pre-change implementation, verbatim, as the equivalence oracle. */ +function buildDetectedGitWorktreesTwoPass( + store: Store, + target: Repo, + gitWorktrees: GitWorktreeInfo[], + allMetaOverride?: Record +) { + const settings = store.getSettings() + const knownOrcaLayouts = ownership.buildKnownOrcaWorkspaceLayouts(settings, target) + const isLegacyRepoForVisibility = ownership.isLegacyRepoForExternalWorktreeVisibility(target) + const liveWorktrees = dedupeWorktreesByPath(gitWorktrees.filter((info) => !info.prunable)) + const worktreeVisibilitySourceMatcher = createWorktreeVisibilitySourceMatcher( + [target.path, ...liveWorktrees.map((worktree) => worktree.path)], + resolveCustomWorktreeVisibilitySources(target, settings.worktreeVisibilityDefaults), + resolveConfiguredWorktreeBasePaths(target) + ) + const allMeta = allMetaOverride ?? store.getAllWorktreeMeta?.() + const repoOwnerCount = store.getRepos().filter((candidate) => candidate.id === target.id).length + const detectedRows = liveWorktrees.map((info) => { + const worktreeId = `${target.id}::${info.path}` + const legacyMeta = store.getWorktreeMeta?.(worktreeId) + const metaById = allMeta ?? (legacyMeta ? { [worktreeId]: legacyMeta } : {}) + let meta = + readWorktreeMetaForHost(store, worktreeId, getRepoExecutionHostId(target)) ?? + getRepoOwnedWorktreeMeta(target, worktreeId, metaById, repoOwnerCount) + const worktree = mergeWorktree(target.id, info, meta, target.displayName) + const detected = ownership.toDetectedWorktree({ + repo: target, + worktree, + meta, + settings, + knownOrcaLayouts, + isLegacyRepoForVisibility, + worktreeVisibilitySourceMatcher + }) + if (!detected.visible) { + return detected + } + meta = resolveWorktreeMetaWithDiscoveryBackfill( + store, + target, + worktreeId, + allMeta, + repoOwnerCount + ) + return ownership.toDetectedWorktree({ + repo: target, + worktree: mergeWorktree(target.id, info, meta, target.displayName), + meta, + settings, + knownOrcaLayouts, + isLegacyRepoForVisibility, + worktreeVisibilitySourceMatcher + }) + }) + return projectResolvedWorktreeLineage(detectedRows, store.getAllWorktreeLineage?.() ?? {}) +} + +describe('buildDetectedGitWorktrees classification passes', () => { + beforeEach(() => { + toDetectedWorktreeSpy.mockClear() + // Discovery backfill stamps lastActivityAt from the clock; freeze it so equivalence is deterministic. + vi.spyOn(Date, 'now').mockReturnValue(1_700_000_000_000) + }) + + it('classifies each visible worktree once per catalog pass, not twice', () => { + const paths = ['/workspace/one', '/workspace/two', '/workspace/three'] + const meta = Object.fromEntries( + paths.map((path) => [`${repo.id}::${path}`, settledMeta({ displayName: path })]) + ) + const store = createStore(meta) + + const detected = buildDetectedGitWorktrees(store, repo, paths.map(gitWorktree), meta) + + expect(detected).toHaveLength(3) + expect(detected.every((row) => row.visible)).toBe(true) + expect(toDetectedWorktreeSpy).toHaveBeenCalledTimes(paths.length) + }) + + it('reads the locator-keyed metadata row only when no host snapshot is available', () => { + const worktreeId = `${repo.id}::/workspace/one` + const meta = { [worktreeId]: settledMeta() } + const store = createStore(meta) + const legacyReads = vi.spyOn(store, 'getWorktreeMeta') + + buildDetectedGitWorktrees(store, repo, [gitWorktree('/workspace/one')], meta) + expect(legacyReads).not.toHaveBeenCalled() + + // Partial stores (compatibility shapes) expose no snapshot, so the locator-keyed lookup must still run. + const partialStore = createStore(meta) as Partial + delete partialStore.getAllWorktreeMeta + delete partialStore.getAllWorktreeMetaForHost + delete partialStore.getWorktreeMetaForHost + const partialLegacyReads = vi.spyOn(partialStore as Store, 'getWorktreeMeta') + + const rows = buildDetectedGitWorktrees( + partialStore as Store, + repo, + [gitWorktree('/workspace/one')], + undefined + ) + expect(partialLegacyReads).toHaveBeenCalledWith(worktreeId) + expect(rows[0]).toMatchObject({ id: worktreeId, lastActivityAt: 5 }) + }) + + it.each([ + ['settled metadata', () => settledMeta()], + ['metadata needing discovery backfill', () => ({ orcaCreatedAt: 1 }) as WorktreeMeta], + ['no metadata at all', () => undefined] + ])('emits a catalog deep-equal to the two-pass build for %s', (_label, makeMeta) => { + const worktreeId = `${repo.id}::/workspace/one` + const seed = makeMeta() + const build = (fn: typeof buildDetectedGitWorktrees) => + fn( + createStore(seed ? { [worktreeId]: seed } : {}), + repo, + [gitWorktree('/workspace/one'), gitWorktree('/workspace/hidden-external')], + seed ? { [worktreeId]: seed } : {} + ) + + expect(build(buildDetectedGitWorktrees)).toEqual(build(buildDetectedGitWorktreesTwoPass)) + }) + + it('emits a catalog deep-equal to the two-pass build for a folder-style listing with no host snapshot', () => { + const worktreeId = `${repo.id}::/workspace/one` + const seed = settledMeta() + const build = (fn: typeof buildDetectedGitWorktrees) => + fn(createStore({ [worktreeId]: seed }), repo, [gitWorktree('/workspace/one')], undefined) + + expect(build(buildDetectedGitWorktrees)).toEqual(build(buildDetectedGitWorktreesTwoPass)) + }) +}) diff --git a/src/main/ipc/worktrees/listing/ssh-worktree-fallback.ts b/src/main/ipc/worktrees/listing/ssh-worktree-fallback.ts index 7ec2cac1fc9..5c734d8bcd9 100644 --- a/src/main/ipc/worktrees/listing/ssh-worktree-fallback.ts +++ b/src/main/ipc/worktrees/listing/ssh-worktree-fallback.ts @@ -155,9 +155,10 @@ export function buildDetectedGitWorktrees( const repoOwnerCount = store.getRepos().filter((candidate) => candidate.id === repo.id).length const detected = liveWorktrees.map((gitWorktree) => { const worktreeId = `${repo.id}::${gitWorktree.path}` - const legacyMeta = store.getWorktreeMeta?.(worktreeId) + // Why: the locator-keyed row is only a stand-in for a missing host snapshot, so don't read it when we have one. + const legacyMeta = allMeta === undefined ? store.getWorktreeMeta?.(worktreeId) : undefined const metaById = allMeta ?? (legacyMeta ? { [worktreeId]: legacyMeta } : {}) - let meta = + const meta = readWorktreeMetaForHost(store, worktreeId, getRepoExecutionHostId(repo)) ?? getRepoOwnedWorktreeMeta(repo, worktreeId, metaById, repoOwnerCount) const worktree = mergeWorktree(repo.id, gitWorktree, meta, repo.displayName) @@ -174,17 +175,21 @@ export function buildDetectedGitWorktrees( return detected } - meta = resolveWorktreeMetaWithDiscoveryBackfill( + const backfilledMeta = resolveWorktreeMetaWithDiscoveryBackfill( store, repo, worktreeId, allMeta, repoOwnerCount ) + // Why: backfill hands back the same object when it wrote nothing, and both builders are pure over it. + if (backfilledMeta === meta) { + return detected + } return toDetectedWorktree({ repo, - worktree: mergeWorktree(repo.id, gitWorktree, meta, repo.displayName), - meta, + worktree: mergeWorktree(repo.id, gitWorktree, backfilledMeta, repo.displayName), + meta: backfilledMeta, settings, knownOrcaLayouts, isLegacyRepoForVisibility, diff --git a/src/main/ipc/worktrees/listing/worktree-discovery-metadata.ts b/src/main/ipc/worktrees/listing/worktree-discovery-metadata.ts index 6af4d45c3cd..b17677ddfcc 100644 --- a/src/main/ipc/worktrees/listing/worktree-discovery-metadata.ts +++ b/src/main/ipc/worktrees/listing/worktree-discovery-metadata.ts @@ -40,8 +40,9 @@ export function resolveWorktreeMetaWithDiscoveryBackfill( repoOwnerCount = store.getRepos().filter((candidate) => candidate.id === repo.id).length ): WorktreeMeta { const executionHostId = getRepoExecutionHostId(repo) - const legacyMeta = store.getWorktreeMeta?.(worktreeId) const allMeta = allMetaOverride ?? store.getAllWorktreeMeta?.() + // Why: the locator-keyed row is only a stand-in for a missing snapshot, so don't read it when we have one. + const legacyMeta = allMeta === undefined ? store.getWorktreeMeta?.(worktreeId) : undefined const existing = readWorktreeMetaForHost(store, worktreeId, executionHostId) ?? getRepoOwnedWorktreeMeta( diff --git a/src/shared/git-status-conflict-entries.test.ts b/src/shared/git-status-conflict-entries.test.ts new file mode 100644 index 00000000000..35f8e4685c5 --- /dev/null +++ b/src/shared/git-status-conflict-entries.test.ts @@ -0,0 +1,212 @@ +/** + * `u` records for asymmetric conflict kinds each cost an `fs.access`. Resolving them concurrently + * must not move a row, change how many probes run, or change which error wins. + */ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import type { StatusPorcelainRecord } from './git-status-porcelain-parser' +import type { GitStatusEntry } from './git-status-types' +import type * as NodeFsPromisesModule from 'node:fs/promises' + +const { accessMock } = vi.hoisted(() => ({ accessMock: vi.fn() })) + +vi.mock('node:fs/promises', async (importOriginal) => ({ + ...(await importOriginal()), + access: accessMock +})) + +/** + * `parseUnmergedEntry` only rethrows when reading `error.code` itself throws, so this is how a + * record is made to reject rather than degrade to 'modified'. + */ +function rejectionEscapingTheErrnoGuard(failure: Error): unknown { + return new Proxy( + {}, + { + get: () => { + throw failure + } + } + ) +} + +const { parseUnmergedEntry, resolveUnmergedStatusRecords } = + await import('./git-status-conflict-entries') + +const WORKTREE = '/repo' + +function unmergedLine(xy: string, filePath: string): string { + return `u ${xy} N... 100644 100644 100644 100644 aaa bbb ccc ${filePath}` +} + +function entryRecord(path: string): StatusPorcelainRecord { + return { type: 'entry', entry: { path, status: 'modified', area: 'unstaged' } } +} + +/** Mixes both conflict families: UU/AA return without I/O, AU/UA/DU/UD each probe the filesystem. */ +const MIXED_RECORDS: StatusPorcelainRecord[] = [ + { type: 'unmerged', line: unmergedLine('UU', 'both-modified.ts') }, + entryRecord('plain-one.ts'), + { type: 'unmerged', line: unmergedLine('AU', 'added-by-us.ts') }, + { type: 'unmerged', line: unmergedLine('AA', 'both-added.ts') }, + { type: 'unmerged', line: unmergedLine('UA', 'added-by-them.ts') }, + entryRecord('plain-two.ts'), + { type: 'unmerged', line: unmergedLine('DU', 'deleted-by-us.ts') }, + { type: 'unmerged', line: unmergedLine('UD', 'deleted-by-them.ts') }, + { type: 'unmerged', line: unmergedLine('160000', 'submodule-ignored.ts') } +] + +/** The pre-change consumption: one `await` per record, in Git's output order. */ +async function collectSerially( + records: readonly StatusPorcelainRecord[] +): Promise<(GitStatusEntry | null)[]> { + const collected: (GitStatusEntry | null)[] = [] + for (const record of records) { + collected.push( + record.type === 'entry' ? record.entry : await parseUnmergedEntry(WORKTREE, record.line) + ) + } + return collected +} + +async function collectConcurrently( + records: readonly StatusPorcelainRecord[] +): Promise<(GitStatusEntry | null)[]> { + const resolved = await resolveUnmergedStatusRecords(WORKTREE, records, records.length) + return records.map((record, index) => { + if (record.type === 'entry') { + return record.entry + } + const settled = resolved[index] + if (settled?.ok === false) { + throw settled.error + } + return settled?.entry ?? null + }) +} + +describe('resolveUnmergedStatusRecords', () => { + beforeEach(() => { + accessMock.mockReset() + }) + + it('produces the same rows, in the same order, as the serial read', async () => { + accessMock.mockImplementation((target: string) => + target.includes('deleted-by') + ? Promise.reject(Object.assign(new Error('x'), { code: 'ENOENT' })) + : Promise.resolve(undefined) + ) + + const serial = await collectSerially(MIXED_RECORDS) + const concurrent = await collectConcurrently(MIXED_RECORDS) + + expect(concurrent).toEqual(serial) + expect(concurrent.map((entry) => entry?.path ?? null)).toEqual([ + 'both-modified.ts', + 'plain-one.ts', + 'added-by-us.ts', + 'both-added.ts', + 'added-by-them.ts', + 'plain-two.ts', + 'deleted-by-us.ts', + 'deleted-by-them.ts', + null + ]) + expect(concurrent.map((entry) => entry?.status ?? null)).toEqual([ + 'modified', + 'modified', + 'modified', + 'modified', + 'modified', + 'modified', + 'deleted', + 'deleted', + null + ]) + }) + + it('runs the same number of filesystem probes as the serial read', async () => { + accessMock.mockResolvedValue(undefined) + await collectSerially(MIXED_RECORDS) + const serialProbes = accessMock.mock.calls.length + + accessMock.mockClear() + await collectConcurrently(MIXED_RECORDS) + + // Only the four asymmetric kinds probe; UU/AA/submodule never touch the filesystem. + expect(serialProbes).toBe(4) + expect(accessMock.mock.calls.length).toBe(serialProbes) + }) + + it('never exceeds the concurrency bound', async () => { + let inFlight = 0 + let peak = 0 + accessMock.mockImplementation(async () => { + inFlight += 1 + peak = Math.max(peak, inFlight) + await new Promise((resolve) => setTimeout(resolve, 0)) + inFlight -= 1 + }) + const records = Array.from({ length: 50 }, (_, index) => ({ + type: 'unmerged' as const, + line: unmergedLine('AU', `conflict-${index}.ts`) + })) + + await resolveUnmergedStatusRecords(WORKTREE, records, records.length) + + expect(peak).toBeGreaterThan(1) + expect(peak).toBeLessThanOrEqual(8) + }) + + it('resolves only the requested prefix, leaving later records for the caller', async () => { + accessMock.mockResolvedValue(undefined) + + const resolved = await resolveUnmergedStatusRecords(WORKTREE, MIXED_RECORDS, 4) + + expect(resolved[0]).toEqual({ + ok: true, + entry: expect.objectContaining({ path: 'both-modified.ts' }) + }) + expect(resolved[1]).toBeUndefined() + expect(resolved[6]).toBeUndefined() + expect(accessMock.mock.calls.length).toBe(1) + }) + + it('surfaces the earliest failing record, exactly as the serial read did', async () => { + const firstFailure = new Error('first failure') + const secondFailure = new Error('second failure') + accessMock.mockImplementation((target: string) => { + if (target.endsWith('added-by-them.ts')) { + return Promise.reject(rejectionEscapingTheErrnoGuard(firstFailure)) + } + if (target.endsWith('deleted-by-us.ts')) { + return Promise.reject(rejectionEscapingTheErrnoGuard(secondFailure)) + } + return Promise.resolve(undefined) + }) + + await expect(collectSerially(MIXED_RECORDS)).rejects.toBe(firstFailure) + await expect(collectConcurrently(MIXED_RECORDS)).rejects.toBe(firstFailure) + }) + + it('does not surface a failure the serial read would never have reached', async () => { + const lateFailure = new Error('late failure') + accessMock.mockImplementation((target: string) => + target.endsWith('deleted-by-them.ts') + ? Promise.reject(rejectionEscapingTheErrnoGuard(lateFailure)) + : Promise.resolve(undefined) + ) + + // The caller stops before that record, so the captured rejection is simply never replayed. + const resolved = await resolveUnmergedStatusRecords( + WORKTREE, + MIXED_RECORDS, + MIXED_RECORDS.length + ) + const consumedPrefix = MIXED_RECORDS.slice(0, 7).map((record, index) => + record.type === 'entry' ? record.entry : resolved[index] + ) + + expect(consumedPrefix).not.toContainEqual({ ok: false, error: lateFailure }) + expect(resolved[7]).toEqual({ ok: false, error: lateFailure }) + }) +}) diff --git a/src/shared/git-status-conflict-entries.ts b/src/shared/git-status-conflict-entries.ts index a50d389e2bb..8071d8292d0 100644 --- a/src/shared/git-status-conflict-entries.ts +++ b/src/shared/git-status-conflict-entries.ts @@ -1,8 +1,52 @@ import { access } from 'node:fs/promises' import * as path from 'node:path' import type { GitConflictKind, GitFileStatus, GitStatusEntry } from './git-status-types' +import type { StatusPorcelainRecord } from './git-status-porcelain-parser' import { decodeGitCQuotedPath } from './git-cquoted-path' +const UNMERGED_ENTRY_RESOLVE_CONCURRENCY = 8 + +/** A settled `parseUnmergedEntry` result, replayed at the record index it belongs to. */ +export type ResolvedUnmergedEntry = + | { ok: true; entry: GitStatusEntry | null } + | { ok: false; error: unknown } + +/** + * Resolve the deferred `u` records in `records[0, end)` with bounded concurrency, keyed by record + * index. Asymmetric conflict kinds each cost an `fs.access`, which is a 9p/network round trip on a + * WSL or remote worktree, and a big rebase makes hundreds of them — serialising those stalls every + * status poll for the life of the conflict. + * + * Ordering is untouched: results stay at their own index, so the caller still consumes Git's output + * order, and a rejection is replayed only if the caller actually reaches that record. + */ +export async function resolveUnmergedStatusRecords( + worktreePath: string, + records: readonly StatusPorcelainRecord[], + end: number +): Promise<(ResolvedUnmergedEntry | undefined)[]> { + const resolved: (ResolvedUnmergedEntry | undefined)[] = [] + let nextIndex = 0 + const resolveNext = async (): Promise => { + while (nextIndex < end) { + const index = nextIndex + nextIndex += 1 + const record = records[index] + if (record?.type !== 'unmerged') { + continue + } + try { + resolved[index] = { ok: true, entry: await parseUnmergedEntry(worktreePath, record.line) } + } catch (error) { + resolved[index] = { ok: false, error } + } + } + } + const workerCount = Math.min(UNMERGED_ENTRY_RESOLVE_CONCURRENCY, end) + await Promise.all(Array.from({ length: workerCount }, () => resolveNext())) + return resolved +} + export async function parseUnmergedEntry( worktreePath: string, line: string