From 67a866a470e214ea329fe055d806e96b9232f69a Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Tue, 22 Sep 2026 16:23:12 -0700 Subject: [PATCH 01/13] fix(git): keep merge-tree's throwaway trees out of the real object store The PR conflict summary and the squash-merged branch cleanup both run `git merge-tree --write-tree` only to read its output, but every divergent run left unreachable loose objects behind. Run it with a per-call scratch object directory (inside objects/, alternates pointing at the real store) that is deleted afterwards, on native, WSL and SSH relay hosts alike. --- .../git/local-git-object-quarantine.test.ts | 52 +++++++ src/main/git/local-git-object-quarantine.ts | 79 ++++++++++ ...ge-tree-object-quarantine-real-git.test.ts | 144 ++++++++++++++++++ src/main/git/worktree-branch-removal.ts | 19 ++- .../client-merge-queue-auto-merge.test.ts | 3 + .../github/client-pr-conflict-summary.test.ts | 3 + src/main/github/client-test-mocks.ts | 13 ++ src/main/github/conflict-summary.test.ts | 26 +++- src/main/github/conflict-summary.ts | 35 ++++- src/relay/git-handler-branch-cleanup.ts | 19 ++- src/relay/git-handler-operation-context.ts | 1 + src/relay/git-handler-ops.ts | 2 + src/relay/git-handler.ts | 3 + ...lay-git-object-quarantine-real-git.test.ts | 55 +++++++ src/relay/relay-git-object-quarantine.ts | 29 ++++ src/shared/git-branch-cleanup.test.ts | 7 + src/shared/git-branch-cleanup.ts | 10 +- ...object-quarantine-real-git.test-fixture.ts | 93 +++++++++++ src/shared/git-object-quarantine.test.ts | 140 +++++++++++++++++ src/shared/git-object-quarantine.ts | 90 +++++++++++ 20 files changed, 796 insertions(+), 27 deletions(-) create mode 100644 src/main/git/local-git-object-quarantine.test.ts create mode 100644 src/main/git/local-git-object-quarantine.ts create mode 100644 src/main/git/merge-tree-object-quarantine-real-git.test.ts create mode 100644 src/relay/relay-git-object-quarantine-real-git.test.ts create mode 100644 src/relay/relay-git-object-quarantine.ts create mode 100644 src/shared/git-object-quarantine-real-git.test-fixture.ts create mode 100644 src/shared/git-object-quarantine.test.ts create mode 100644 src/shared/git-object-quarantine.ts diff --git a/src/main/git/local-git-object-quarantine.test.ts b/src/main/git/local-git-object-quarantine.test.ts new file mode 100644 index 000000000000..64b8c318f6b3 --- /dev/null +++ b/src/main/git/local-git-object-quarantine.test.ts @@ -0,0 +1,52 @@ +import { describe, expect, it } from 'vitest' +import { + localGitObjectQuarantineProcessEnv, + localGitObjectsDirectory +} from './local-git-object-quarantine' + +const QUARANTINE = { + GIT_OBJECT_DIRECTORY: '/home/me/repo/.git/objects/tmp_objdir-orca-merge-tree-x', + GIT_ALTERNATE_OBJECT_DIRECTORIES: '/home/me/repo/.git/objects' +} + +describe('localGitObjectQuarantineProcessEnv', () => { + it('forwards both variables through WSLENV on Windows, keeping existing entries', () => { + const env = localGitObjectQuarantineProcessEnv(QUARANTINE, 'win32', { + PATH: 'C:\\bin', + WSLENV: 'ORCA_X/u' + }) + + expect(env).toMatchObject({ PATH: 'C:\\bin', ...QUARANTINE }) + expect(env.WSLENV?.split(':')).toEqual([ + 'ORCA_X/u', + 'GIT_OBJECT_DIRECTORY', + 'GIT_ALTERNATE_OBJECT_DIRECTORIES' + ]) + }) + + it('leaves WSLENV alone off Windows', () => { + const env = localGitObjectQuarantineProcessEnv(QUARANTINE, 'darwin', { PATH: '/usr/bin' }) + + expect(env).toEqual({ PATH: '/usr/bin', ...QUARANTINE }) + }) +}) + +describe('localGitObjectsDirectory', () => { + it('translates a WSL Git common dir into a path the Windows host can open', () => { + expect(localGitObjectsDirectory('/home/me/repo/.git', 'Ubuntu')).toEqual({ + hostPath: '\\\\wsl.localhost\\Ubuntu\\home\\me\\repo\\.git\\objects', + gitPath: '/home/me/repo/.git/objects' + }) + }) + + it('uses one spelling for native Git', () => { + expect(localGitObjectsDirectory('/Users/me/repo/.git', undefined)).toEqual({ + hostPath: '/Users/me/repo/.git/objects', + gitPath: '/Users/me/repo/.git/objects' + }) + expect(localGitObjectsDirectory('C:\\repo\\.git', undefined)).toEqual({ + hostPath: 'C:\\repo\\.git\\objects', + gitPath: 'C:\\repo\\.git\\objects' + }) + }) +}) diff --git a/src/main/git/local-git-object-quarantine.ts b/src/main/git/local-git-object-quarantine.ts new file mode 100644 index 000000000000..ada47b69fc1f --- /dev/null +++ b/src/main/git/local-git-object-quarantine.ts @@ -0,0 +1,79 @@ +import { mkdtemp, rm } from 'node:fs/promises' +import { posix, win32 } from 'node:path' +import { isWindowsAbsolutePathLike } from '../../shared/cross-platform-path' +import type { GitAdmissionTier } from '../../shared/rpc-contract/git-admission-tier-params' +import { + createGitObjectQuarantine, + type GitObjectQuarantineEnv, + type GitObjectsDirectory +} from '../../shared/git-object-quarantine' +import { addWslEnvKeys } from '../../shared/wsl-env' +import { isWslUncPath, toWindowsWslPath } from '../../shared/wsl-paths' +import { parseWslPath } from '../wsl' +import { usesHostGitForWslLinkedWorktree } from './wsl-linked-worktree-git-routing' +import { readRepoCommonDirFromGit } from './worktree-list-reader' + +export type LocalGitObjectQuarantine = { + /** `env` is a full process env for `gitExecFileAsync`, or undefined to run unquarantined. */ + run: (command: (env: NodeJS.ProcessEnv | undefined) => Promise) => Promise +} + +/** `objects/` in Git's spelling and, for WSL Git, translated to one the main process can open. */ +export function localGitObjectsDirectory( + commonDir: string, + wslDistro: string | undefined +): GitObjectsDirectory { + const hostCommonDir = + wslDistro && !isWslUncPath(commonDir) && !isWindowsAbsolutePathLike(commonDir) + ? toWindowsWslPath(commonDir, wslDistro) + : commonDir + // Decided by path syntax, not by platform: a Windows main process drives WSL Git. + const pathApiFor = (value: string): typeof posix => + isWindowsAbsolutePathLike(value) ? win32 : posix + return { + hostPath: pathApiFor(hostCommonDir).join(hostCommonDir, 'objects'), + gitPath: pathApiFor(commonDir).join(commonDir, 'objects') + } +} + +export function localGitObjectQuarantineProcessEnv( + quarantine: GitObjectQuarantineEnv, + platform: NodeJS.Platform = process.platform, + baseEnv: NodeJS.ProcessEnv = process.env +): NodeJS.ProcessEnv { + const env: NodeJS.ProcessEnv = { ...baseEnv, ...quarantine } + if (platform === 'win32') { + // Why: wsl.exe forwards only WSLENV-named vars; the values are already Linux paths, so no `/p`. + addWslEnvKeys(env, ['GIT_OBJECT_DIRECTORY', 'GIT_ALTERNATE_OBJECT_DIRECTORIES']) + } + return env +} + +/** Scratch object store for throwaway Git writes on the local (native or WSL) host. */ +export function createLocalGitObjectQuarantine( + repoPath: string, + options: { wslDistro?: string; signal?: AbortSignal; admissionTier?: GitAdmissionTier } = {} +): LocalGitObjectQuarantine { + const wslDistro = parseWslPath(repoPath)?.distro ?? options.wslDistro + const quarantine = createGitObjectQuarantine({ + async resolveObjectsDirectory() { + const commonDir = await readRepoCommonDirFromGit(repoPath, { + ...(options.wslDistro ? { wslDistro: options.wslDistro } : {}), + ...(options.signal ? { signal: options.signal } : {}), + ...(options.admissionTier ? { admissionTier: options.admissionTier } : {}) + }) + // Why after the read: that first Git call is what settles the WSL linked-worktree route, + // and Windows Git on that route may not share the spelling resolved above. + if (!commonDir || usesHostGitForWslLinkedWorktree(repoPath, options.wslDistro)) { + return undefined + } + return localGitObjectsDirectory(commonDir, wslDistro) + }, + makeTempDirectory: (prefix) => mkdtemp(prefix), + removeDirectory: (hostPath) => rm(hostPath, { recursive: true, force: true }) + }) + return { + run: (command) => + quarantine.run((env) => command(env ? localGitObjectQuarantineProcessEnv(env) : undefined)) + } +} diff --git a/src/main/git/merge-tree-object-quarantine-real-git.test.ts b/src/main/git/merge-tree-object-quarantine-real-git.test.ts new file mode 100644 index 000000000000..4f8de73dd1c4 --- /dev/null +++ b/src/main/git/merge-tree-object-quarantine-real-git.test.ts @@ -0,0 +1,144 @@ +import { existsSync } from 'node:fs' +import { afterEach, beforeEach, describe, expect, it } from 'vitest' +import { + createDivergentRepoFixture, + type DivergentRepoFixture +} from '../../shared/git-object-quarantine-real-git.test-fixture' +import { + __resetPRConflictSummaryCachesForTests, + getPRConflictSummary +} from '../github/conflict-summary' +import { createLocalGitObjectQuarantine } from './local-git-object-quarantine' +import { gitExecFileAsync } from './runner' +import { deleteBranchAfterWorktreeRemoval } from './worktree-branch-removal' + +// Why: every divergent `merge-tree --write-tree` used to leave unreachable loose objects in the +// real store; enough of them keeps Git's auto-gc re-running forever on large repositories. +describe('merge-tree runs against a scratch object store (real Git)', () => { + let fixture: DivergentRepoFixture + + beforeEach(() => { + __resetPRConflictSummaryCachesForTests() + fixture = createDivergentRepoFixture() + }) + + afterEach(() => { + fixture.dispose() + }) + + function unquarantinedConflictFiles(cwd: string, base: string, head: string): string[] { + const mergeBase = fixture.git(cwd, 'merge-base', head, base).trim() + let stdout: string + try { + stdout = fixture.git( + cwd, + 'merge-tree', + '--write-tree', + '--name-only', + '-z', + '--no-messages', + '--merge-base', + mergeBase, + head, + base + ) + } catch (error) { + // A conflicted merge exits 1 but still prints the file list. + stdout = error instanceof Error && 'stdout' in error ? String(error.stdout) : '' + } + return stdout.split('\0').filter(Boolean).slice(1) + } + + it.each([ + ['the main worktree', (f: DivergentRepoFixture) => f.repoPath], + ['a linked worktree', (f: DivergentRepoFixture) => f.linkedPath] + ])('conflict summary from %s writes no loose objects', async (_label, pickCwd) => { + const cwd = pickCwd(fixture) + const base = fixture.git(cwd, 'rev-parse', 'main').trim() + const head = fixture.git(cwd, 'rev-parse', 'feature-conflict').trim() + const before = fixture.looseObjectCount() + + const summary = await getPRConflictSummary(cwd, 'main', base, head) + + expect(fixture.looseObjectCount()).toBe(before) + expect(fixture.scratchDirectories()).toEqual([]) + expect(summary?.files).toEqual(['shared.txt']) + // The unquarantined run proves the fixture really writes objects and the output matches. + expect(unquarantinedConflictFiles(cwd, base, head)).toEqual(summary?.files) + expect(fixture.looseObjectCount()).toBeGreaterThan(before) + }) + + it.each([ + ['the main worktree', (f: DivergentRepoFixture) => f.repoPath], + ['a linked worktree', (f: DivergentRepoFixture) => f.linkedPath] + ])('branch cleanup from %s writes no loose objects', async (_label, pickCwd) => { + const cwd = pickCwd(fixture) + fixture.git(cwd, 'config', 'branch.feature-extra.base', 'refs/heads/main') + fixture.git(cwd, 'config', 'branch.feature-squashed.base', 'refs/heads/main') + const extraHead = fixture.git(cwd, 'rev-parse', 'feature-extra').trim() + const squashedHead = fixture.git(cwd, 'rev-parse', 'feature-squashed').trim() + const before = fixture.looseObjectCount() + + const extra = await deleteBranchAfterWorktreeRemoval(cwd, 'feature-extra', extraHead, {}) + const squashed = await deleteBranchAfterWorktreeRemoval( + cwd, + 'feature-squashed', + squashedHead, + {} + ) + + expect(fixture.looseObjectCount()).toBe(before) + expect(fixture.scratchDirectories()).toEqual([]) + expect(extra).toEqual({ preservedBranch: { branchName: 'feature-extra', head: extraHead } }) + expect(squashed).toEqual({}) + // Same verdicts as an unquarantined merge-tree, which does write objects. + const mainTree = fixture.git(cwd, 'rev-parse', 'main^{tree}').trim() + const mergeTree = (branch: string): string => + fixture.git(cwd, 'merge-tree', '--write-tree', 'main', branch).trim() + expect(mergeTree('feature-extra')).not.toBe(mainTree) + expect(mergeTree(squashedHead)).toBe(mainTree) + expect(fixture.looseObjectCount()).toBeGreaterThan(before) + }) + + it('removes the scratch directory when the Git command fails', async () => { + const quarantine = createLocalGitObjectQuarantine(fixture.linkedPath) + let scratch: string | undefined + + await expect( + quarantine.run(async (env) => { + scratch = env?.GIT_OBJECT_DIRECTORY + expect(scratch && existsSync(scratch)).toBe(true) + return gitExecFileAsync(['merge-tree', '--write-tree', 'main', 'no-such-branch'], { + cwd: fixture.linkedPath, + env + }) + }) + ).rejects.toThrow() + + expect(scratch).toBeDefined() + expect(existsSync(scratch ?? '')).toBe(false) + expect(fixture.scratchDirectories()).toEqual([]) + }) + + it('removes the scratch directory when the command is aborted', async () => { + const quarantine = createLocalGitObjectQuarantine(fixture.repoPath) + const controller = new AbortController() + let scratch: string | undefined + + await expect( + quarantine.run(async (env) => { + scratch = env?.GIT_OBJECT_DIRECTORY + const running = gitExecFileAsync(['merge-tree', '--write-tree', 'main', 'feature-extra'], { + cwd: fixture.repoPath, + env, + signal: controller.signal + }) + controller.abort() + return running + }) + ).rejects.toThrow() + + expect(scratch).toBeDefined() + expect(existsSync(scratch ?? '')).toBe(false) + }) +}) diff --git a/src/main/git/worktree-branch-removal.ts b/src/main/git/worktree-branch-removal.ts index dc09311596c0..26326eb93a0d 100644 --- a/src/main/git/worktree-branch-removal.ts +++ b/src/main/git/worktree-branch-removal.ts @@ -1,11 +1,13 @@ import { branchHasNoUnmergedChangesWithLazyTargetRefresh, - getBranchCleanupTargetRefs + getBranchCleanupTargetRefs, + type GitBranchCleanupExecOptions } from '../../shared/git-branch-cleanup' import type { RemoveWorktreeResult } from '../../shared/worktree/create-types' import { withLocalGitCapabilityCacheForExecution } from './git-capability-state' import { withRepoRefMaintenancePaused } from './local-repo-ref-maintenance' import { gitExecFileAsync } from './runner' +import { createLocalGitObjectQuarantine } from './local-git-object-quarantine' import { parseWorktreeList } from '../../shared/git-worktree-porcelain-parser' import { isBranchCheckedOutInWorktreeError } from '../../shared/git-branch-delete-refusal' import type { GitWorktreeExecOptions, RemoveWorktreeOptions } from './worktree-operation-options' @@ -107,11 +109,16 @@ async function deleteAlreadyMergedBranchAfterSafeDeleteFailure( branchHead: string, options: GitWorktreeExecOptions = {} ): Promise { - const runGit = (args: string[], execOptions?: { stdin?: string }) => - gitExecFileAsync(args, { - ...gitExecOptions(repoPath, options), - ...(execOptions?.stdin !== undefined ? { stdin: execOptions.stdin } : {}) - }) + const quarantine = createLocalGitObjectQuarantine(repoPath, options) + const runGit = (args: string[], execOptions?: GitBranchCleanupExecOptions) => { + const run = (env: NodeJS.ProcessEnv | undefined) => + gitExecFileAsync(args, { + ...gitExecOptions(repoPath, options), + ...(execOptions?.stdin !== undefined ? { stdin: execOptions.stdin } : {}), + ...(env ? { env } : {}) + }) + return execOptions?.discardWrittenObjects ? quarantine.run(run) : run(undefined) + } const targetRefs = await getBranchCleanupTargetRefs(runGit, branchName) // Why: squash merges rewrite commit IDs, so `branch -d` rejects already-merged branches; delete only when Git proves no unmerged tree changes. const hasNoUnmergedChanges = await withLocalGitCapabilityCacheForExecution( diff --git a/src/main/github/client-merge-queue-auto-merge.test.ts b/src/main/github/client-merge-queue-auto-merge.test.ts index d099408d022f..3a7745f4da8b 100644 --- a/src/main/github/client-merge-queue-auto-merge.test.ts +++ b/src/main/github/client-merge-queue-auto-merge.test.ts @@ -9,6 +9,9 @@ const { clientMocks, moduleMocks } = await vi.hoisted(async () => { vi.mock('./gh-utils', () => moduleMocks.ghUtilsModuleMock(clientMocks)) vi.mock('../git/runner', () => moduleMocks.gitRunnerModuleMock(clientMocks)) +vi.mock('../git/local-git-object-quarantine', () => + moduleMocks.localGitObjectQuarantineModuleMock() +) vi.mock('../providers/ssh-git-dispatch', () => moduleMocks.sshGitDispatchModuleMock(clientMocks)) vi.mock('./local-git-config-signature', () => moduleMocks.localGitConfigSignatureModuleMock(clientMocks) diff --git a/src/main/github/client-pr-conflict-summary.test.ts b/src/main/github/client-pr-conflict-summary.test.ts index a71939112743..f4ec8592f445 100644 --- a/src/main/github/client-pr-conflict-summary.test.ts +++ b/src/main/github/client-pr-conflict-summary.test.ts @@ -9,6 +9,9 @@ const { clientMocks, moduleMocks } = await vi.hoisted(async () => { vi.mock('./gh-utils', () => moduleMocks.ghUtilsModuleMock(clientMocks)) vi.mock('../git/runner', () => moduleMocks.gitRunnerModuleMock(clientMocks)) +vi.mock('../git/local-git-object-quarantine', () => + moduleMocks.localGitObjectQuarantineModuleMock() +) vi.mock('../providers/ssh-git-dispatch', () => moduleMocks.sshGitDispatchModuleMock(clientMocks)) vi.mock('./local-git-config-signature', () => moduleMocks.localGitConfigSignatureModuleMock(clientMocks) diff --git a/src/main/github/client-test-mocks.ts b/src/main/github/client-test-mocks.ts index 15fe0f362144..1412d485b913 100644 --- a/src/main/github/client-test-mocks.ts +++ b/src/main/github/client-test-mocks.ts @@ -131,6 +131,19 @@ export function gitRunnerModuleMock(mocks: GitHubClientMocks): GitRunnerModuleMo } } +export type LocalGitObjectQuarantineModuleMock = { + createLocalGitObjectQuarantine: () => { + run: (command: (env: undefined) => Promise) => Promise + } +} + +// Why: client tests script git calls in order; the scratch object store adds a common-dir read. +export function localGitObjectQuarantineModuleMock(): LocalGitObjectQuarantineModuleMock { + return { + createLocalGitObjectQuarantine: () => ({ run: (command) => command(undefined) }) + } +} + export type SshGitDispatchModuleMock = { getSshGitProviderGeneration: Mock getSshGitProvider: Mock diff --git a/src/main/github/conflict-summary.test.ts b/src/main/github/conflict-summary.test.ts index 36d96407301b..aac9b63c9a39 100644 --- a/src/main/github/conflict-summary.test.ts +++ b/src/main/github/conflict-summary.test.ts @@ -17,17 +17,26 @@ import { __resetPRConflictSummaryCachesForTests, getPRConflictSummary } from './ type GitResult = { stdout: string } type GitHandler = (argv: string[]) => Promise +// Why its own key: the merge-tree object quarantine reads the common dir once per derivation. +const COMMON_DIR_COMMAND = 'rev-parse --git-common-dir' + const defaultHandlers: Record = { fetch: async () => ({ stdout: '' }), 'rev-parse': async () => ({ stdout: 'base-tip-1\n' }), 'merge-base': async () => ({ stdout: 'merge-base-1\n' }), 'rev-list': async () => ({ stdout: '3\n' }), - 'merge-tree': async () => ({ stdout: 'tree-oid\u0000src/conflict.ts\u0000' }) + 'merge-tree': async () => ({ stdout: 'tree-oid\u0000src/conflict.ts\u0000' }), + [COMMON_DIR_COMMAND]: async () => ({ stdout: '.git\n' }) +} + +function commandKey(argv: string[]): string { + return argv[0] === 'rev-parse' && argv.includes('--git-common-dir') ? COMMON_DIR_COMMAND : argv[0] } function mockGitDispatch(overrides: Record = {}): void { gitExecFileAsyncMock.mockImplementation((argv: string[]) => { - const handler = overrides[argv[0]] ?? defaultHandlers[argv[0]] + const key = commandKey(argv) + const handler = overrides[key] ?? defaultHandlers[key] if (!handler) { return Promise.reject(new Error(`unexpected git command: ${argv.join(' ')}`)) } @@ -40,7 +49,7 @@ function spawnCount(command?: string): number { if (!command) { return calls.length } - return calls.filter(([argv]) => Array.isArray(argv) && argv[0] === command).length + return calls.filter(([argv]) => Array.isArray(argv) && commandKey(argv) === command).length } const expectedSummary = { @@ -78,13 +87,14 @@ describe('getPRConflictSummary caching', () => { const first = await deriveSummary() expect(first).toEqual(expectedSummary) expect(spawnCount('fetch')).toBe(1) - expect(spawnCount()).toBe(5) + expect(spawnCount(COMMON_DIR_COMMAND)).toBe(1) + expect(spawnCount()).toBe(6) const second = await deriveSummary() const third = await deriveSummary() expect(second).toEqual(expectedSummary) expect(third).toEqual(expectedSummary) - expect(spawnCount()).toBe(5) + expect(spawnCount()).toBe(6) }) it('re-derives when headRefOid changes without re-fetching inside the throttle window', async () => { @@ -151,7 +161,7 @@ describe('getPRConflictSummary caching', () => { expect(first).toEqual(expectedSummary) expect(second).toEqual(expectedSummary) - expect(spawnCount()).toBe(5) + expect(spawnCount()).toBe(6) }) it('dedupes overlapping derivations after resolving the same live base tip', async () => { @@ -264,7 +274,7 @@ describe('getPRConflictSummary caching', () => { const repeat = await deriveSummary() expect(repeat).toEqual(expectedSummary) expect(spawnCount('fetch')).toBe(1) - expect(spawnCount()).toBe(5) + expect(spawnCount()).toBe(6) }) it("falls back to GitHub's baseRefOid when fetch and remote-tracking refs are unavailable", async () => { @@ -306,7 +316,7 @@ describe('getPRConflictSummary caching', () => { admissionTier: 'background' }) - expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(5) + expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(6) for (const [, options] of gitExecFileAsyncMock.mock.calls) { expect(options).toEqual( expect.objectContaining({ diff --git a/src/main/github/conflict-summary.ts b/src/main/github/conflict-summary.ts index 470f85733676..a1f23385cb26 100644 --- a/src/main/github/conflict-summary.ts +++ b/src/main/github/conflict-summary.ts @@ -5,6 +5,10 @@ import { } from '../../shared/git-merge-tree-capability' import { gitExecFileAsync } from '../git/runner' import { gitOptionsForWorktree, type GitRuntimeOptions } from '../git/git-runtime-options' +import { + createLocalGitObjectQuarantine, + type LocalGitObjectQuarantine +} from '../git/local-git-object-quarantine' import { clearGitCapabilityStateForTests, withLocalGitCapabilityCacheForExecution @@ -234,6 +238,8 @@ async function loadConflictingFiles( headOid, baseOid ] + // Why: only the file list is read; the merged tree it writes would pile up as loose objects. + const quarantine = createLocalGitObjectQuarantine(repoPath, localGitOptions) return withLocalGitCapabilityCacheForExecution( { cwd: repoPath, wslDistro: localGitOptions.wslDistro }, @@ -245,9 +251,12 @@ async function loadConflictingFiles( 'merge-tree-merge-base', async () => { try { - const result = await gitExecFileAsync(modernArgs, { - ...gitOptionsForWorktree(repoPath, localGitOptions) - }) + const result = await quarantine.run((env) => + gitExecFileAsync(modernArgs, { + ...gitOptionsForWorktree(repoPath, localGitOptions), + ...(env ? { env } : {}) + }) + ) return parseMergeTreeNameOnlyOutput(result.stdout) } catch (error) { if (isUnsupportedMergeTreeWriteTreeError(error)) { @@ -262,7 +271,13 @@ async function loadConflictingFiles( throw error } }, - () => loadConflictingFilesWithLegacyMergeTree(repoPath, legacyArgs, localGitOptions), + () => + loadConflictingFilesWithLegacyMergeTree( + repoPath, + legacyArgs, + localGitOptions, + quarantine + ), isUnsupportedMergeTreeMergeBaseError ), async () => { @@ -278,12 +293,16 @@ async function loadConflictingFiles( async function loadConflictingFilesWithLegacyMergeTree( repoPath: string, legacyArgs: string[], - localGitOptions: LocalGitExecOptions + localGitOptions: LocalGitExecOptions, + quarantine: LocalGitObjectQuarantine ): Promise { try { - const result = await gitExecFileAsync(legacyArgs, { - ...gitOptionsForWorktree(repoPath, localGitOptions) - }) + const result = await quarantine.run((env) => + gitExecFileAsync(legacyArgs, { + ...gitOptionsForWorktree(repoPath, localGitOptions), + ...(env ? { env } : {}) + }) + ) return parseMergeTreeNameOnlyOutput(result.stdout) } catch (fallbackError) { const fallbackStdout = getGitErrorOutput(fallbackError, 'stdout') diff --git a/src/relay/git-handler-branch-cleanup.ts b/src/relay/git-handler-branch-cleanup.ts index 3bde9cfcdaf7..bc7681cb91eb 100644 --- a/src/relay/git-handler-branch-cleanup.ts +++ b/src/relay/git-handler-branch-cleanup.ts @@ -1,10 +1,12 @@ import { branchHasNoUnmergedChangesWithLazyTargetRefresh, - getBranchCleanupTargetRefs + getBranchCleanupTargetRefs, + type GitBranchCleanupExecOptions } from '../shared/git-branch-cleanup' import type { GitCapabilityCache } from '../shared/git-capability-cache' import type { GitExec } from './git-handler-ops' import { parseWorktreeList } from '../shared/git-worktree-porcelain-parser' +import { createRelayGitObjectQuarantine } from './relay-git-object-quarantine' export async function deleteAlreadyMergedRelayBranchAfterSafeDeleteFailure( git: GitExec, @@ -13,8 +15,19 @@ export async function deleteAlreadyMergedRelayBranchAfterSafeDeleteFailure( branchHead: string, capabilities: GitCapabilityCache ): Promise { - const runGit = (args: string[], options?: { stdin?: string }) => - options ? git(args, repoPath, options) : git(args, repoPath) + const quarantine = createRelayGitObjectQuarantine(git, repoPath) + const runGit = (args: string[], options?: GitBranchCleanupExecOptions) => { + const run = (env: Record | undefined) => { + if (options?.stdin === undefined && !env) { + return git(args, repoPath) + } + return git(args, repoPath, { + ...(options?.stdin !== undefined ? { stdin: options.stdin } : {}), + ...(env ? { env } : {}) + }) + } + return options?.discardWrittenObjects ? quarantine.run(run) : run(undefined) + } const targetRefs = await getBranchCleanupTargetRefs(runGit, branchName) // Why: SSH worktrees hit the same squash-merge shape as local worktrees. // Git's no-op merge proof lets us clean up only branches whose changes diff --git a/src/relay/git-handler-operation-context.ts b/src/relay/git-handler-operation-context.ts index 85d2d9080975..5cf8a03c51b9 100644 --- a/src/relay/git-handler-operation-context.ts +++ b/src/relay/git-handler-operation-context.ts @@ -14,6 +14,7 @@ export type GitHandlerCommandOptions = { stdin?: string timeout?: number terminationBarrier?: boolean + env?: Record } export type GitHandlerCommandResult = { stdout: string; stderr: string } diff --git a/src/relay/git-handler-ops.ts b/src/relay/git-handler-ops.ts index ba5c9e596999..f7ac7eb9e07e 100644 --- a/src/relay/git-handler-ops.ts +++ b/src/relay/git-handler-ops.ts @@ -22,6 +22,8 @@ export type GitExec = ( signal?: AbortSignal stdin?: string timeout?: number + /** Merged over the relay's Git env for this one command. */ + env?: Record } ) => Promise<{ stdout: string; stderr: string }> diff --git a/src/relay/git-handler.ts b/src/relay/git-handler.ts index 66bbd6d3cd1b..e884756aa659 100644 --- a/src/relay/git-handler.ts +++ b/src/relay/git-handler.ts @@ -164,6 +164,9 @@ export class GitHandler { if (opts?.disableOptionalLocks) { env.GIT_OPTIONAL_LOCKS = '0' } + if (opts?.env) { + Object.assign(env, opts.env) + } const execOptions = { cwd: expandedCwd, env, diff --git a/src/relay/relay-git-object-quarantine-real-git.test.ts b/src/relay/relay-git-object-quarantine-real-git.test.ts new file mode 100644 index 000000000000..92a66dd25993 --- /dev/null +++ b/src/relay/relay-git-object-quarantine-real-git.test.ts @@ -0,0 +1,55 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { + createDivergentRepoFixture, + type DivergentRepoFixture +} from '../shared/git-object-quarantine-real-git.test-fixture' +import type { GitHandler } from './git-handler' +import { createGitHandlerRelay } from './git-handler-test-harness' +import type { MockDispatcher } from './git-handler-test-setup' + +// Why: the SSH relay runs the same merge-tree proof on the remote host's own object store. +describe('relay branch cleanup runs merge-tree against a scratch object store (real Git)', () => { + let fixture: DivergentRepoFixture + let dispatcher: MockDispatcher + let handler: GitHandler + + beforeEach(() => { + vi.spyOn(console, 'warn').mockImplementation(() => {}) + fixture = createDivergentRepoFixture() + ;({ dispatcher, handler } = createGitHandlerRelay()) + }) + + afterEach(() => { + handler.dispose() + fixture.dispose() + vi.restoreAllMocks() + }) + + function removeWorktreeFor(branch: string): Promise { + fixture.git(fixture.repoPath, 'config', `branch.${branch}.base`, 'refs/heads/main') + const worktreePath = fixture.addWorktree(`wt-${branch}`, branch) + return dispatcher.callRequest('git.removeWorktree', { worktreePath }) + } + + it('deletes a squash-merged branch without writing loose objects', async () => { + const before = fixture.looseObjectCount() + + await expect(removeWorktreeFor('feature-squashed')).resolves.toEqual({}) + + expect(fixture.looseObjectCount()).toBe(before) + expect(fixture.scratchDirectories()).toEqual([]) + expect(fixture.git(fixture.repoPath, 'branch', '--list', 'feature-squashed')).toBe('') + }) + + it('preserves a branch with unmerged changes without writing loose objects', async () => { + const head = fixture.git(fixture.repoPath, 'rev-parse', 'feature-extra').trim() + const before = fixture.looseObjectCount() + + await expect(removeWorktreeFor('feature-extra')).resolves.toEqual({ + preservedBranch: { branchName: 'feature-extra', head } + }) + + expect(fixture.looseObjectCount()).toBe(before) + expect(fixture.scratchDirectories()).toEqual([]) + }) +}) diff --git a/src/relay/relay-git-object-quarantine.ts b/src/relay/relay-git-object-quarantine.ts new file mode 100644 index 000000000000..ffa66486cfc9 --- /dev/null +++ b/src/relay/relay-git-object-quarantine.ts @@ -0,0 +1,29 @@ +import { mkdtemp, rm } from 'node:fs/promises' +import { resolve } from 'node:path' +import { + createGitObjectQuarantine, + type GitObjectQuarantine +} from '../shared/git-object-quarantine' +import { expandTilde } from './context' +import type { GitExec } from './git-handler-ops' + +/** Scratch object store for throwaway Git writes; the relay and Git share one host and spelling. */ +export function createRelayGitObjectQuarantine( + git: GitExec, + repoPath: string +): GitObjectQuarantine { + return createGitObjectQuarantine({ + async resolveObjectsDirectory() { + const { stdout } = await git(['rev-parse', '--git-common-dir'], repoPath) + const commonDir = stdout.replace(/\r?\n$/, '') + if (!commonDir) { + return undefined + } + // Why: Git 2.25 may print the common dir relative to the command's cwd. + const objects = resolve(expandTilde(repoPath), commonDir, 'objects') + return { hostPath: objects, gitPath: objects } + }, + makeTempDirectory: (prefix) => mkdtemp(prefix), + removeDirectory: (hostPath) => rm(hostPath, { recursive: true, force: true }) + }) +} diff --git a/src/shared/git-branch-cleanup.test.ts b/src/shared/git-branch-cleanup.test.ts index ab59cdb8bb88..33a4ed663058 100644 --- a/src/shared/git-branch-cleanup.test.ts +++ b/src/shared/git-branch-cleanup.test.ts @@ -107,6 +107,13 @@ describe('branchHasNoUnmergedChangesOnAnyTarget', () => { expect(runGit).toHaveBeenCalledWith(['patch-id', '--stable'], { stdin: 'branch-diff' }) expect(runGit).toHaveBeenCalledWith(['patch-id', '--stable'], { stdin: 'squash-diff' }) expect(runGit).not.toHaveBeenCalledWith(['cherry', '-v', 'target', 'refs/heads/feature/test']) + // Only merge-tree's throwaway tree writes go to a scratch object store. + const discarding = vi + .mocked(runGit) + .mock.calls.filter(([, options]) => options?.discardWrittenObjects) + .map(([args]) => args[0]) + expect(discarding.length).toBeGreaterThan(0) + expect(new Set(discarding)).toEqual(new Set(['merge-tree'])) }) it('preserves a branch with merge commits when no target squash commit matches', async () => { diff --git a/src/shared/git-branch-cleanup.ts b/src/shared/git-branch-cleanup.ts index 6766bb97d343..685b57444846 100644 --- a/src/shared/git-branch-cleanup.ts +++ b/src/shared/git-branch-cleanup.ts @@ -1,9 +1,15 @@ import type { GitCapabilityCache } from './git-capability-cache' import { isUnsupportedMergeTreeWriteTreeError } from './git-merge-tree-capability' +export type GitBranchCleanupExecOptions = { + stdin?: string + /** The command's object writes are throwaway; run it against a scratch object store. */ + discardWrittenObjects?: boolean +} + export type GitBranchCleanupExec = ( argv: string[], - options?: { stdin?: string } + options?: GitBranchCleanupExecOptions ) => Promise<{ stdout: string }> const SQUASH_PATCH_SCAN_LIMIT = 200 @@ -117,7 +123,7 @@ async function branchMergesWithoutTreeChanges( try { return await capabilities.runWithFallback( 'merge-tree-write-tree', - async () => (await runGit(args)).stdout.trim() || null, + async () => (await runGit(args, { discardWrittenObjects: true })).stdout.trim() || null, async () => null, isUnsupportedMergeTreeWriteTreeError ) diff --git a/src/shared/git-object-quarantine-real-git.test-fixture.ts b/src/shared/git-object-quarantine-real-git.test-fixture.ts new file mode 100644 index 000000000000..f340776885be --- /dev/null +++ b/src/shared/git-object-quarantine-real-git.test-fixture.ts @@ -0,0 +1,93 @@ +import { execFileSync } from 'node:child_process' +import { mkdtempSync, readdirSync, realpathSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { GIT_OBJECT_QUARANTINE_DIR_PREFIX } from './git-object-quarantine' + +/** + * A repository where `merge-tree --write-tree` has real work to do: + * - `feature-conflict` conflicts with `main` on `shared.txt` (checked out in a linked worktree); + * - `feature-extra` adds a file `main` lacks, so merging it produces a brand-new tree; + * - `feature-squashed` adds a file `main` later received through a separate (squash) commit. + */ +export type DivergentRepoFixture = { + repoPath: string + linkedPath: string + commonDir: string + git: (cwd: string, ...args: string[]) => string + /** Files under the real object store's `objects/xx` fan-out directories. */ + looseObjectCount: () => number + scratchDirectories: () => string[] + addWorktree: (name: string, branch: string) => string + dispose: () => void +} + +export function createDivergentRepoFixture(): DivergentRepoFixture { + const root = realpathSync(mkdtempSync(join(tmpdir(), 'orca-merge-tree-quarantine-'))) + const repoPath = join(root, 'repo') + const git = (cwd: string, ...args: string[]): string => + execFileSync('git', args, { cwd, encoding: 'utf8' }) + const write = (name: string, content: string): void => { + writeFileSync(join(repoPath, name), content) + } + const commit = (message: string): void => { + git(repoPath, 'add', '-A') + git(repoPath, 'commit', '--quiet', '-m', message) + } + + execFileSync('git', ['init', '--quiet', repoPath]) + git(repoPath, 'config', 'user.name', 'Orca Test') + git(repoPath, 'config', 'user.email', 'orca@example.test') + git(repoPath, 'config', 'commit.gpgSign', 'false') + git(repoPath, 'config', 'core.hooksPath', '.git/no-hooks') + git(repoPath, 'config', 'gc.auto', '0') + git(repoPath, 'checkout', '--quiet', '-b', 'main') + write('shared.txt', 'base\n') + commit('base') + + git(repoPath, 'checkout', '--quiet', '-b', 'feature-conflict') + write('shared.txt', 'feature\n') + write('conflict-only.txt', 'feature only\n') + commit('feature conflict') + + git(repoPath, 'checkout', '--quiet', '-b', 'feature-extra', 'main') + write('extra.txt', 'extra\n') + commit('feature extra') + + git(repoPath, 'checkout', '--quiet', '-b', 'feature-squashed', 'main') + write('squashed.txt', 'squashed\n') + commit('feature squashed') + + git(repoPath, 'checkout', '--quiet', 'main') + write('shared.txt', 'main\n') + write('main-only.txt', 'main only\n') + commit('main moves on') + write('squashed.txt', 'squashed\n') + commit('squash merge of feature-squashed') + + const linkedPath = join(root, 'linked') + git(repoPath, 'worktree', 'add', '--quiet', linkedPath, 'feature-conflict') + // Why: start from a packed store so every loose object is one the code under test wrote. + git(repoPath, 'gc', '--quiet', '--prune=now') + + const commonDir = join(repoPath, '.git') + const objectsDir = join(commonDir, 'objects') + return { + repoPath, + linkedPath, + commonDir, + git, + looseObjectCount: () => + readdirSync(objectsDir) + .filter((entry) => /^[0-9a-f]{2}$/.test(entry)) + .reduce((count, entry) => count + readdirSync(join(objectsDir, entry)).length, 0), + scratchDirectories: () => + readdirSync(objectsDir).filter((entry) => entry.startsWith(GIT_OBJECT_QUARANTINE_DIR_PREFIX)), + addWorktree: (name, branch) => { + const worktreePath = join(root, name) + git(repoPath, 'worktree', 'add', '--quiet', worktreePath, branch) + return worktreePath + }, + dispose: () => rmSync(root, { recursive: true, force: true }) + } +} diff --git a/src/shared/git-object-quarantine.test.ts b/src/shared/git-object-quarantine.test.ts new file mode 100644 index 000000000000..916db8d31092 --- /dev/null +++ b/src/shared/git-object-quarantine.test.ts @@ -0,0 +1,140 @@ +import { describe, expect, it, vi } from 'vitest' +import { + createGitObjectQuarantine, + GIT_OBJECT_QUARANTINE_DIR_PREFIX, + type GitObjectQuarantineHost, + type GitObjectsDirectory +} from './git-object-quarantine' + +function fakeHost( + objects: GitObjectsDirectory | undefined, + overrides: Partial = {} +) { + return { + resolveObjectsDirectory: vi.fn(async () => objects), + makeTempDirectory: vi.fn(async (prefix: string) => `${prefix}AbC123`), + removeDirectory: vi.fn(async () => {}), + ...overrides + } +} + +describe('createGitObjectQuarantine', () => { + it('points Git at a scratch dir inside objects/ and removes it afterwards', async () => { + const host = fakeHost({ hostPath: '/repo/.git/objects', gitPath: '/repo/.git/objects' }) + const command = vi.fn(async () => 'ok') + + await expect(createGitObjectQuarantine(host).run(command)).resolves.toBe('ok') + + const scratch = `/repo/.git/objects/${GIT_OBJECT_QUARANTINE_DIR_PREFIX}AbC123` + expect(command).toHaveBeenCalledWith({ + GIT_OBJECT_DIRECTORY: scratch, + GIT_ALTERNATE_OBJECT_DIRECTORIES: '/repo/.git/objects' + }) + expect(host.removeDirectory).toHaveBeenCalledWith(scratch) + }) + + it('removes the scratch dir when the command fails', async () => { + const host = fakeHost({ hostPath: '/r/objects', gitPath: '/r/objects' }) + const failure = new Error('merge-tree failed') + + await expect( + createGitObjectQuarantine(host).run(async () => { + throw failure + }) + ).rejects.toBe(failure) + expect(host.removeDirectory).toHaveBeenCalledTimes(1) + }) + + it('keeps the command result when the scratch dir cannot be removed', async () => { + const host = fakeHost( + { hostPath: '/r/objects', gitPath: '/r/objects' }, + { + removeDirectory: vi.fn(async () => { + throw new Error('EBUSY') + }) + } + ) + + await expect(createGitObjectQuarantine(host).run(async () => 'tree')).resolves.toBe('tree') + }) + + it.each([ + ['the objects dir is unknown', fakeHost(undefined)], + [ + 'resolving the objects dir fails', + fakeHost(undefined, { + resolveObjectsDirectory: vi.fn(async () => { + throw new Error('x') + }) + }) + ], + [ + 'the scratch dir cannot be created', + fakeHost( + { hostPath: '/r/objects', gitPath: '/r/objects' }, + { + makeTempDirectory: vi.fn(async () => { + throw new Error('EACCES') + }) + } + ) + ] + ])('runs the command unquarantined when %s', async (_label, host) => { + const command = vi.fn(async () => 'ok') + + await expect(createGitObjectQuarantine(host).run(command)).resolves.toBe('ok') + + expect(command).toHaveBeenCalledWith(undefined) + expect(host.removeDirectory).not.toHaveBeenCalled() + }) + + it('resolves the objects dir once and gives every run its own scratch dir', async () => { + let next = 0 + const host = fakeHost( + { hostPath: '/r/objects', gitPath: '/r/objects' }, + { makeTempDirectory: vi.fn(async (prefix: string) => `${prefix}${++next}`) } + ) + const quarantine = createGitObjectQuarantine(host) + const seen: (string | undefined)[] = [] + + await quarantine.run(async (env) => seen.push(env?.GIT_OBJECT_DIRECTORY)) + await quarantine.run(async (env) => seen.push(env?.GIT_OBJECT_DIRECTORY)) + + expect(host.resolveObjectsDirectory).toHaveBeenCalledTimes(1) + expect(new Set(seen).size).toBe(2) + }) + + it('creates the scratch dir in host spelling but hands WSL Git its Linux spelling', async () => { + const host = fakeHost({ + hostPath: '\\\\wsl.localhost\\Ubuntu\\home\\me\\repo\\.git\\objects', + gitPath: '/home/me/repo/.git/objects' + }) + const command = vi.fn(async () => 'ok') + + await createGitObjectQuarantine(host).run(command) + + expect(host.makeTempDirectory).toHaveBeenCalledWith( + `\\\\wsl.localhost\\Ubuntu\\home\\me\\repo\\.git\\objects\\${GIT_OBJECT_QUARANTINE_DIR_PREFIX}` + ) + expect(command).toHaveBeenCalledWith({ + GIT_OBJECT_DIRECTORY: `/home/me/repo/.git/objects/${GIT_OBJECT_QUARANTINE_DIR_PREFIX}AbC123`, + GIT_ALTERNATE_OBJECT_DIRECTORIES: '/home/me/repo/.git/objects' + }) + }) + + it.each([ + ['/plain/objects', '/plain/objects'], + ['/odd:dir/objects', '"/odd:dir/objects"'], + ['/quo"te\\dir/objects', '"/quo\\"te\\\\dir/objects"'], + ['C:\\repo\\.git\\objects', 'C:\\repo\\.git\\objects'] + ])('spells alternates for %s so Git reads one entry', async (gitPath, expected) => { + const host = fakeHost({ hostPath: gitPath, gitPath }) + const command = vi.fn(async () => 'ok') + + await createGitObjectQuarantine(host).run(command) + + expect(command).toHaveBeenCalledWith( + expect.objectContaining({ GIT_ALTERNATE_OBJECT_DIRECTORIES: expected }) + ) + }) +}) diff --git a/src/shared/git-object-quarantine.ts b/src/shared/git-object-quarantine.ts new file mode 100644 index 000000000000..3a656d437634 --- /dev/null +++ b/src/shared/git-object-quarantine.ts @@ -0,0 +1,90 @@ +import { posix, win32 } from 'node:path' +import { isWindowsAbsolutePathLike } from './cross-platform-path' + +/** + * Runs a Git command whose object writes are throwaway (`merge-tree --write-tree` + * only wants its stdout) against a scratch object directory, so nothing it + * writes lands in the repository's real object store as unreachable loose + * objects. Reads still see every real object through the alternates variable. + */ + +export type GitObjectQuarantineEnv = { + GIT_OBJECT_DIRECTORY: string + GIT_ALTERNATE_OBJECT_DIRECTORIES: string +} + +export type GitObjectsDirectory = { + /** How the process creating and deleting the scratch dir spells `objects/`. */ + hostPath: string + /** How the Git child spells the same directory (differs for WSL). */ + gitPath: string +} + +export type GitObjectQuarantineHost = { + resolveObjectsDirectory: () => Promise + /** mkdtemp semantics: `prefix` plus a unique suffix, returns the created path. */ + makeTempDirectory: (prefix: string) => Promise + removeDirectory: (hostPath: string) => Promise +} + +export type GitObjectQuarantine = { + run: (command: (env: GitObjectQuarantineEnv | undefined) => Promise) => Promise +} + +// Why inside `objects/` with a `tmp_objdir` prefix: that is where Git puts its own +// quarantine dirs, it is on the Git host for native, WSL and SSH alike, and +// `git prune` removes stale `tmp_*` entries if a crash ever strands one. +export const GIT_OBJECT_QUARANTINE_DIR_PREFIX = 'tmp_objdir-orca-merge-tree-' + +// Decided by path syntax, not by platform: a Windows main process drives WSL Git. +function pathApiFor(value: string): typeof posix { + return isWindowsAbsolutePathLike(value) ? win32 : posix +} + +// Why: Git splits this variable on `:` (`;` on Windows) and C-unquotes a leading `"`. +function alternateObjectDirectoriesValue(gitPath: string): string { + if (isWindowsAbsolutePathLike(gitPath) || !/[:"\\]/.test(gitPath)) { + return gitPath + } + return `"${gitPath.replace(/[\\"]/g, (char) => `\\${char}`)}"` +} + +/** Resolves the objects dir once per quarantine; each run gets its own scratch dir. */ +export function createGitObjectQuarantine(host: GitObjectQuarantineHost): GitObjectQuarantine { + let objectsDirectory: Promise | undefined + const resolveObjectsDirectory = (): Promise => { + objectsDirectory ??= host.resolveObjectsDirectory().catch(() => undefined) + return objectsDirectory + } + + return { + async run(command) { + const objects = await resolveObjectsDirectory() + let scratchHostPath: string | undefined + if (objects) { + try { + scratchHostPath = await host.makeTempDirectory( + pathApiFor(objects.hostPath).join(objects.hostPath, GIT_OBJECT_QUARANTINE_DIR_PREFIX) + ) + } catch { + // Why: bookkeeping must not block the user's action; run unquarantined. + scratchHostPath = undefined + } + } + if (!objects || !scratchHostPath) { + return command(undefined) + } + try { + return await command({ + GIT_OBJECT_DIRECTORY: pathApiFor(objects.gitPath).join( + objects.gitPath, + pathApiFor(scratchHostPath).basename(scratchHostPath) + ), + GIT_ALTERNATE_OBJECT_DIRECTORIES: alternateObjectDirectoriesValue(objects.gitPath) + }) + } finally { + await host.removeDirectory(scratchHostPath).catch(() => {}) + } + } + } +} From 33958927f19e023fdc113288fcff660691b3b990 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Mon, 28 Sep 2026 23:17:47 -0700 Subject: [PATCH 02/13] fix(git): keep partial-clone downloads and clean up stranded scratch stores - A partial clone fetches missing blobs during merge-tree; those packs landed in the scratch store and were deleted, so every check re-downloaded them. Move fetched packs (index last) into the real store before removing the scratch dir. - Sweep this app's own scratch dirs older than an hour, once per objects dir per process, so a crash or failed delete cannot strand one forever in repos where gc never runs. - Remove with Windows transient-lock retries; the shared core now owns all filesystem work and host adapters only resolve the objects dir. --- src/main/git/local-git-object-quarantine.ts | 38 ++-- ...ge-tree-object-quarantine-real-git.test.ts | 22 ++ ...lay-git-object-quarantine-real-git.test.ts | 2 +- src/relay/relay-git-object-quarantine.ts | 23 +- ...object-quarantine-real-git.test-fixture.ts | 53 ++++- src/shared/git-object-quarantine.test.ts | 203 +++++++++++------- src/shared/git-object-quarantine.ts | 110 +++++++--- 7 files changed, 301 insertions(+), 150 deletions(-) diff --git a/src/main/git/local-git-object-quarantine.ts b/src/main/git/local-git-object-quarantine.ts index ada47b69fc1f..48849d2103b1 100644 --- a/src/main/git/local-git-object-quarantine.ts +++ b/src/main/git/local-git-object-quarantine.ts @@ -1,9 +1,8 @@ -import { mkdtemp, rm } from 'node:fs/promises' -import { posix, win32 } from 'node:path' import { isWindowsAbsolutePathLike } from '../../shared/cross-platform-path' import type { GitAdmissionTier } from '../../shared/rpc-contract/git-admission-tier-params' import { createGitObjectQuarantine, + pathApiForGitPath, type GitObjectQuarantineEnv, type GitObjectsDirectory } from '../../shared/git-object-quarantine' @@ -27,12 +26,9 @@ export function localGitObjectsDirectory( wslDistro && !isWslUncPath(commonDir) && !isWindowsAbsolutePathLike(commonDir) ? toWindowsWslPath(commonDir, wslDistro) : commonDir - // Decided by path syntax, not by platform: a Windows main process drives WSL Git. - const pathApiFor = (value: string): typeof posix => - isWindowsAbsolutePathLike(value) ? win32 : posix return { - hostPath: pathApiFor(hostCommonDir).join(hostCommonDir, 'objects'), - gitPath: pathApiFor(commonDir).join(commonDir, 'objects') + hostPath: pathApiForGitPath(hostCommonDir).join(hostCommonDir, 'objects'), + gitPath: pathApiForGitPath(commonDir).join(commonDir, 'objects') } } @@ -55,22 +51,18 @@ export function createLocalGitObjectQuarantine( options: { wslDistro?: string; signal?: AbortSignal; admissionTier?: GitAdmissionTier } = {} ): LocalGitObjectQuarantine { const wslDistro = parseWslPath(repoPath)?.distro ?? options.wslDistro - const quarantine = createGitObjectQuarantine({ - async resolveObjectsDirectory() { - const commonDir = await readRepoCommonDirFromGit(repoPath, { - ...(options.wslDistro ? { wslDistro: options.wslDistro } : {}), - ...(options.signal ? { signal: options.signal } : {}), - ...(options.admissionTier ? { admissionTier: options.admissionTier } : {}) - }) - // Why after the read: that first Git call is what settles the WSL linked-worktree route, - // and Windows Git on that route may not share the spelling resolved above. - if (!commonDir || usesHostGitForWslLinkedWorktree(repoPath, options.wslDistro)) { - return undefined - } - return localGitObjectsDirectory(commonDir, wslDistro) - }, - makeTempDirectory: (prefix) => mkdtemp(prefix), - removeDirectory: (hostPath) => rm(hostPath, { recursive: true, force: true }) + const quarantine = createGitObjectQuarantine(async () => { + const commonDir = await readRepoCommonDirFromGit(repoPath, { + ...(options.wslDistro ? { wslDistro: options.wslDistro } : {}), + ...(options.signal ? { signal: options.signal } : {}), + ...(options.admissionTier ? { admissionTier: options.admissionTier } : {}) + }) + // Why after the read: that first Git call is what settles the WSL linked-worktree route, + // and Windows Git on that route may not share the spelling resolved above. + if (!commonDir || usesHostGitForWslLinkedWorktree(repoPath, options.wslDistro)) { + return undefined + } + return localGitObjectsDirectory(commonDir, wslDistro) }) return { run: (command) => diff --git a/src/main/git/merge-tree-object-quarantine-real-git.test.ts b/src/main/git/merge-tree-object-quarantine-real-git.test.ts index 4f8de73dd1c4..0cc3ade038c5 100644 --- a/src/main/git/merge-tree-object-quarantine-real-git.test.ts +++ b/src/main/git/merge-tree-object-quarantine-real-git.test.ts @@ -100,6 +100,28 @@ describe('merge-tree runs against a scratch object store (real Git)', () => { expect(fixture.looseObjectCount()).toBeGreaterThan(before) }) + it('keeps file contents a partial clone downloads, so the next check does not fetch them again', async () => { + const clone = fixture.createPartialClone() + const git = (...args: string[]): string => fixture.git(clone.clonePath, ...args).trim() + const base = git('rev-parse', 'origin/main') + const head = git('rev-parse', 'origin/feature-conflict') + const packsBefore = clone.packCount() + const looseBefore = clone.looseObjectCount() + + const first = await getPRConflictSummary(clone.clonePath, 'main', base, head) + const packsAfterFirst = clone.packCount() + __resetPRConflictSummaryCachesForTests() + const second = await getPRConflictSummary(clone.clonePath, 'main', base, head) + + expect(first?.files).toEqual(['shared.txt']) + expect(second?.files).toEqual(['shared.txt']) + // The first check fetched the missing blobs into the real store; the second found them there. + expect(packsAfterFirst).toBeGreaterThan(packsBefore) + expect(clone.packCount()).toBe(packsAfterFirst) + expect(clone.looseObjectCount()).toBe(looseBefore) + expect(clone.scratchDirectories()).toEqual([]) + }) + it('removes the scratch directory when the Git command fails', async () => { const quarantine = createLocalGitObjectQuarantine(fixture.linkedPath) let scratch: string | undefined diff --git a/src/relay/relay-git-object-quarantine-real-git.test.ts b/src/relay/relay-git-object-quarantine-real-git.test.ts index 92a66dd25993..078c30d01073 100644 --- a/src/relay/relay-git-object-quarantine-real-git.test.ts +++ b/src/relay/relay-git-object-quarantine-real-git.test.ts @@ -31,7 +31,7 @@ describe('relay branch cleanup runs merge-tree against a scratch object store (r return dispatcher.callRequest('git.removeWorktree', { worktreePath }) } - it('deletes a squash-merged branch without writing loose objects', async () => { + it('still deletes a squash-merged branch through the scratch store', async () => { const before = fixture.looseObjectCount() await expect(removeWorktreeFor('feature-squashed')).resolves.toEqual({}) diff --git a/src/relay/relay-git-object-quarantine.ts b/src/relay/relay-git-object-quarantine.ts index ffa66486cfc9..1d828d94a0ba 100644 --- a/src/relay/relay-git-object-quarantine.ts +++ b/src/relay/relay-git-object-quarantine.ts @@ -1,4 +1,3 @@ -import { mkdtemp, rm } from 'node:fs/promises' import { resolve } from 'node:path' import { createGitObjectQuarantine, @@ -12,18 +11,14 @@ export function createRelayGitObjectQuarantine( git: GitExec, repoPath: string ): GitObjectQuarantine { - return createGitObjectQuarantine({ - async resolveObjectsDirectory() { - const { stdout } = await git(['rev-parse', '--git-common-dir'], repoPath) - const commonDir = stdout.replace(/\r?\n$/, '') - if (!commonDir) { - return undefined - } - // Why: Git 2.25 may print the common dir relative to the command's cwd. - const objects = resolve(expandTilde(repoPath), commonDir, 'objects') - return { hostPath: objects, gitPath: objects } - }, - makeTempDirectory: (prefix) => mkdtemp(prefix), - removeDirectory: (hostPath) => rm(hostPath, { recursive: true, force: true }) + return createGitObjectQuarantine(async () => { + const { stdout } = await git(['rev-parse', '--git-common-dir'], repoPath) + const commonDir = stdout.replace(/\r?\n$/, '') + if (!commonDir) { + return undefined + } + // Why: Git 2.25 may print the common dir relative to the command's cwd. + const objects = resolve(expandTilde(repoPath), commonDir, 'objects') + return { hostPath: objects, gitPath: objects } }) } diff --git a/src/shared/git-object-quarantine-real-git.test-fixture.ts b/src/shared/git-object-quarantine-real-git.test-fixture.ts index f340776885be..e6c0d7c5df80 100644 --- a/src/shared/git-object-quarantine-real-git.test-fixture.ts +++ b/src/shared/git-object-quarantine-real-git.test-fixture.ts @@ -2,6 +2,7 @@ import { execFileSync } from 'node:child_process' import { mkdtempSync, readdirSync, realpathSync, rmSync, writeFileSync } from 'node:fs' import { tmpdir } from 'node:os' import { join } from 'node:path' +import { pathToFileURL } from 'node:url' import { GIT_OBJECT_QUARANTINE_DIR_PREFIX } from './git-object-quarantine' /** @@ -19,9 +20,31 @@ export type DivergentRepoFixture = { looseObjectCount: () => number scratchDirectories: () => string[] addWorktree: (name: string, branch: string) => string + /** A blob-less clone of the repo, so merge-tree must fetch file contents on demand. */ + createPartialClone: () => PartialCloneFixture dispose: () => void } +export type PartialCloneFixture = { + clonePath: string + looseObjectCount: () => number + /** Packs in the clone's real `objects/pack`, each named by its `.idx`. */ + packCount: () => number + scratchDirectories: () => string[] +} + +function looseObjectCountIn(objectsDir: string): number { + return readdirSync(objectsDir) + .filter((entry) => /^[0-9a-f]{2}$/.test(entry)) + .reduce((count, entry) => count + readdirSync(join(objectsDir, entry)).length, 0) +} + +function scratchDirectoriesIn(objectsDir: string): string[] { + return readdirSync(objectsDir).filter((entry) => + entry.startsWith(GIT_OBJECT_QUARANTINE_DIR_PREFIX) + ) +} + export function createDivergentRepoFixture(): DivergentRepoFixture { const root = realpathSync(mkdtempSync(join(tmpdir(), 'orca-merge-tree-quarantine-'))) const repoPath = join(root, 'repo') @@ -77,17 +100,35 @@ export function createDivergentRepoFixture(): DivergentRepoFixture { linkedPath, commonDir, git, - looseObjectCount: () => - readdirSync(objectsDir) - .filter((entry) => /^[0-9a-f]{2}$/.test(entry)) - .reduce((count, entry) => count + readdirSync(join(objectsDir, entry)).length, 0), - scratchDirectories: () => - readdirSync(objectsDir).filter((entry) => entry.startsWith(GIT_OBJECT_QUARANTINE_DIR_PREFIX)), + looseObjectCount: () => looseObjectCountIn(objectsDir), + scratchDirectories: () => scratchDirectoriesIn(objectsDir), addWorktree: (name, branch) => { const worktreePath = join(root, name) git(repoPath, 'worktree', 'add', '--quiet', worktreePath, branch) return worktreePath }, + createPartialClone: () => { + git(repoPath, 'config', 'uploadpack.allowFilter', 'true') + const clonePath = join(root, 'partial') + git( + root, + 'clone', + '--quiet', + '--no-checkout', + '--filter=blob:none', + pathToFileURL(repoPath).href, + clonePath + ) + const cloneObjectsDir = join(clonePath, '.git', 'objects') + return { + clonePath, + looseObjectCount: () => looseObjectCountIn(cloneObjectsDir), + packCount: () => + readdirSync(join(cloneObjectsDir, 'pack')).filter((entry) => entry.endsWith('.idx')) + .length, + scratchDirectories: () => scratchDirectoriesIn(cloneObjectsDir) + } + }, dispose: () => rmSync(root, { recursive: true, force: true }) } } diff --git a/src/shared/git-object-quarantine.test.ts b/src/shared/git-object-quarantine.test.ts index 916db8d31092..1746f17e6069 100644 --- a/src/shared/git-object-quarantine.test.ts +++ b/src/shared/git-object-quarantine.test.ts @@ -1,125 +1,120 @@ -import { describe, expect, it, vi } from 'vitest' import { + existsSync, + mkdirSync, + mkdtempSync, + readdirSync, + rmSync, + utimesSync, + writeFileSync +} from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { + _resetGitObjectQuarantineSweepForTests, createGitObjectQuarantine, GIT_OBJECT_QUARANTINE_DIR_PREFIX, - type GitObjectQuarantineHost, - type GitObjectsDirectory + STALE_GIT_OBJECT_QUARANTINE_AGE_MS, + type GitObjectQuarantineEnv } from './git-object-quarantine' -function fakeHost( - objects: GitObjectsDirectory | undefined, - overrides: Partial = {} -) { - return { - resolveObjectsDirectory: vi.fn(async () => objects), - makeTempDirectory: vi.fn(async (prefix: string) => `${prefix}AbC123`), - removeDirectory: vi.fn(async () => {}), - ...overrides - } -} - describe('createGitObjectQuarantine', () => { + let root: string + let objects: string + + beforeEach(() => { + _resetGitObjectQuarantineSweepForTests() + root = mkdtempSync(join(tmpdir(), 'orca-object-quarantine-')) + objects = join(root, 'objects') + mkdirSync(join(objects, 'pack'), { recursive: true }) + }) + + afterEach(() => { + rmSync(root, { recursive: true, force: true }) + }) + + const resolveNative = () => async () => ({ hostPath: objects, gitPath: objects }) + const scratchDirs = (): string[] => + readdirSync(objects).filter((entry) => entry.startsWith(GIT_OBJECT_QUARANTINE_DIR_PREFIX)) + it('points Git at a scratch dir inside objects/ and removes it afterwards', async () => { - const host = fakeHost({ hostPath: '/repo/.git/objects', gitPath: '/repo/.git/objects' }) - const command = vi.fn(async () => 'ok') + let seen: GitObjectQuarantineEnv | undefined - await expect(createGitObjectQuarantine(host).run(command)).resolves.toBe('ok') + await expect( + createGitObjectQuarantine(resolveNative()).run(async (env) => { + seen = env + expect(scratchDirs()).toHaveLength(1) + return 'ok' + }) + ).resolves.toBe('ok') - const scratch = `/repo/.git/objects/${GIT_OBJECT_QUARANTINE_DIR_PREFIX}AbC123` - expect(command).toHaveBeenCalledWith({ - GIT_OBJECT_DIRECTORY: scratch, - GIT_ALTERNATE_OBJECT_DIRECTORIES: '/repo/.git/objects' + expect(seen).toEqual({ + GIT_OBJECT_DIRECTORY: expect.stringContaining( + join(objects, GIT_OBJECT_QUARANTINE_DIR_PREFIX) + ), + GIT_ALTERNATE_OBJECT_DIRECTORIES: objects }) - expect(host.removeDirectory).toHaveBeenCalledWith(scratch) + expect(scratchDirs()).toEqual([]) }) it('removes the scratch dir when the command fails', async () => { - const host = fakeHost({ hostPath: '/r/objects', gitPath: '/r/objects' }) const failure = new Error('merge-tree failed') await expect( - createGitObjectQuarantine(host).run(async () => { + createGitObjectQuarantine(resolveNative()).run(async () => { throw failure }) ).rejects.toBe(failure) - expect(host.removeDirectory).toHaveBeenCalledTimes(1) - }) - - it('keeps the command result when the scratch dir cannot be removed', async () => { - const host = fakeHost( - { hostPath: '/r/objects', gitPath: '/r/objects' }, - { - removeDirectory: vi.fn(async () => { - throw new Error('EBUSY') - }) - } - ) - - await expect(createGitObjectQuarantine(host).run(async () => 'tree')).resolves.toBe('tree') + expect(scratchDirs()).toEqual([]) }) it.each([ - ['the objects dir is unknown', fakeHost(undefined)], + ['the objects dir is unknown', async () => undefined], [ 'resolving the objects dir fails', - fakeHost(undefined, { - resolveObjectsDirectory: vi.fn(async () => { - throw new Error('x') - }) - }) + async () => { + throw new Error('x') + } ], [ 'the scratch dir cannot be created', - fakeHost( - { hostPath: '/r/objects', gitPath: '/r/objects' }, - { - makeTempDirectory: vi.fn(async () => { - throw new Error('EACCES') - }) - } - ) + async () => ({ hostPath: '/no/such/objects', gitPath: '/no/such/objects' }) ] - ])('runs the command unquarantined when %s', async (_label, host) => { + ])('runs the command unquarantined when %s', async (_label, resolve) => { const command = vi.fn(async () => 'ok') - await expect(createGitObjectQuarantine(host).run(command)).resolves.toBe('ok') + await expect(createGitObjectQuarantine(resolve).run(command)).resolves.toBe('ok') expect(command).toHaveBeenCalledWith(undefined) - expect(host.removeDirectory).not.toHaveBeenCalled() }) it('resolves the objects dir once and gives every run its own scratch dir', async () => { - let next = 0 - const host = fakeHost( - { hostPath: '/r/objects', gitPath: '/r/objects' }, - { makeTempDirectory: vi.fn(async (prefix: string) => `${prefix}${++next}`) } - ) - const quarantine = createGitObjectQuarantine(host) + const resolve = vi.fn(resolveNative()) + const quarantine = createGitObjectQuarantine(resolve) const seen: (string | undefined)[] = [] await quarantine.run(async (env) => seen.push(env?.GIT_OBJECT_DIRECTORY)) await quarantine.run(async (env) => seen.push(env?.GIT_OBJECT_DIRECTORY)) - expect(host.resolveObjectsDirectory).toHaveBeenCalledTimes(1) + expect(resolve).toHaveBeenCalledTimes(1) expect(new Set(seen).size).toBe(2) }) it('creates the scratch dir in host spelling but hands WSL Git its Linux spelling', async () => { - const host = fakeHost({ - hostPath: '\\\\wsl.localhost\\Ubuntu\\home\\me\\repo\\.git\\objects', + let seen: GitObjectQuarantineEnv | undefined + + await createGitObjectQuarantine(async () => ({ + hostPath: objects, gitPath: '/home/me/repo/.git/objects' + })).run(async (env) => { + seen = env + expect(scratchDirs()).toHaveLength(1) }) - const command = vi.fn(async () => 'ok') - - await createGitObjectQuarantine(host).run(command) - expect(host.makeTempDirectory).toHaveBeenCalledWith( - `\\\\wsl.localhost\\Ubuntu\\home\\me\\repo\\.git\\objects\\${GIT_OBJECT_QUARANTINE_DIR_PREFIX}` + expect(seen?.GIT_OBJECT_DIRECTORY).toMatch( + new RegExp(`^/home/me/repo/\\.git/objects/${GIT_OBJECT_QUARANTINE_DIR_PREFIX}\\w+$`) ) - expect(command).toHaveBeenCalledWith({ - GIT_OBJECT_DIRECTORY: `/home/me/repo/.git/objects/${GIT_OBJECT_QUARANTINE_DIR_PREFIX}AbC123`, - GIT_ALTERNATE_OBJECT_DIRECTORIES: '/home/me/repo/.git/objects' - }) + expect(seen?.GIT_ALTERNATE_OBJECT_DIRECTORIES).toBe('/home/me/repo/.git/objects') }) it.each([ @@ -128,13 +123,61 @@ describe('createGitObjectQuarantine', () => { ['/quo"te\\dir/objects', '"/quo\\"te\\\\dir/objects"'], ['C:\\repo\\.git\\objects', 'C:\\repo\\.git\\objects'] ])('spells alternates for %s so Git reads one entry', async (gitPath, expected) => { - const host = fakeHost({ hostPath: gitPath, gitPath }) - const command = vi.fn(async () => 'ok') + let seen: GitObjectQuarantineEnv | undefined - await createGitObjectQuarantine(host).run(command) - - expect(command).toHaveBeenCalledWith( - expect.objectContaining({ GIT_ALTERNATE_OBJECT_DIRECTORIES: expected }) + await createGitObjectQuarantine(async () => ({ hostPath: objects, gitPath })).run( + async (env) => { + seen = env + } ) + + expect(seen?.GIT_ALTERNATE_OBJECT_DIRECTORIES).toBe(expected) + }) + + it('moves packs Git fetched into the scratch dir to the real store, index included', async () => { + await createGitObjectQuarantine(resolveNative()).run(async (env) => { + const scratchPack = join(env?.GIT_OBJECT_DIRECTORY ?? '', 'pack') + mkdirSync(scratchPack) + for (const file of ['pack-abc1.pack', 'pack-abc1.idx', 'pack-abc1.promisor']) { + writeFileSync(join(scratchPack, file), file) + } + // A pack still being written has no index yet; Git could not use it. + writeFileSync(join(scratchPack, 'pack-def2.pack'), 'partial') + writeFileSync(join(scratchPack, 'tmp_pack_XYZ'), 'partial') + mkdirSync(join(env?.GIT_OBJECT_DIRECTORY ?? '', 'ab')) + writeFileSync(join(env?.GIT_OBJECT_DIRECTORY ?? '', 'ab', 'cdef'), 'merge-tree output') + }) + + expect(readdirSync(join(objects, 'pack')).sort()).toEqual([ + 'pack-abc1.idx', + 'pack-abc1.pack', + 'pack-abc1.promisor' + ]) + expect(existsSync(join(objects, 'ab'))).toBe(false) + expect(scratchDirs()).toEqual([]) + }) + + it('removes scratch dirs a crash stranded, once per objects dir, leaving recent ones and Git’s own', async () => { + const stale = join(objects, `${GIT_OBJECT_QUARANTINE_DIR_PREFIX}stale`) + const recent = join(objects, `${GIT_OBJECT_QUARANTINE_DIR_PREFIX}recent`) + const gitOwn = join(objects, 'tmp_objdir-incoming-old') + for (const dir of [stale, recent, gitOwn]) { + mkdirSync(dir) + } + const old = (Date.now() - STALE_GIT_OBJECT_QUARANTINE_AGE_MS - 60_000) / 1000 + utimesSync(stale, old, old) + utimesSync(gitOwn, old, old) + + const quarantine = createGitObjectQuarantine(resolveNative()) + await quarantine.run(async () => {}) + + expect(existsSync(stale)).toBe(false) + expect(existsSync(recent)).toBe(true) + expect(existsSync(gitOwn)).toBe(true) + + mkdirSync(stale) + utimesSync(stale, old, old) + await createGitObjectQuarantine(resolveNative()).run(async () => {}) + expect(existsSync(stale)).toBe(true) }) }) diff --git a/src/shared/git-object-quarantine.ts b/src/shared/git-object-quarantine.ts index 3a656d437634..119988c74064 100644 --- a/src/shared/git-object-quarantine.ts +++ b/src/shared/git-object-quarantine.ts @@ -1,5 +1,7 @@ +import { mkdtemp, readdir, rename, stat } from 'node:fs/promises' import { posix, win32 } from 'node:path' import { isWindowsAbsolutePathLike } from './cross-platform-path' +import { removeTree } from './windows-transient-lock-removal' /** * Runs a Git command whose object writes are throwaway (`merge-tree --write-tree` @@ -14,30 +16,29 @@ export type GitObjectQuarantineEnv = { } export type GitObjectsDirectory = { - /** How the process creating and deleting the scratch dir spells `objects/`. */ + /** How this process spells `objects/` when creating and deleting the scratch dir. */ hostPath: string /** How the Git child spells the same directory (differs for WSL). */ gitPath: string } -export type GitObjectQuarantineHost = { - resolveObjectsDirectory: () => Promise - /** mkdtemp semantics: `prefix` plus a unique suffix, returns the created path. */ - makeTempDirectory: (prefix: string) => Promise - removeDirectory: (hostPath: string) => Promise -} - export type GitObjectQuarantine = { run: (command: (env: GitObjectQuarantineEnv | undefined) => Promise) => Promise } // Why inside `objects/` with a `tmp_objdir` prefix: that is where Git puts its own // quarantine dirs, it is on the Git host for native, WSL and SSH alike, and -// `git prune` removes stale `tmp_*` entries if a crash ever strands one. +// `git gc` expires stale `tmp_*` entries too. export const GIT_OBJECT_QUARANTINE_DIR_PREFIX = 'tmp_objdir-orca-merge-tree-' -// Decided by path syntax, not by platform: a Windows main process drives WSL Git. -function pathApiFor(value: string): typeof posix { +// Why an hour: a run lives at most as long as a Git read (minutes), so anything older was +// stranded by a crash or a failed delete, including repos where `git gc` never runs. +export const STALE_GIT_OBJECT_QUARANTINE_AGE_MS = 60 * 60 * 1000 + +const sweptObjectsDirectories = new Set() + +/** Decided by path syntax, not by platform: a Windows main process drives WSL Git. */ +export function pathApiForGitPath(value: string): typeof posix { return isWindowsAbsolutePathLike(value) ? win32 : posix } @@ -49,42 +50,99 @@ function alternateObjectDirectoriesValue(gitPath: string): string { return `"${gitPath.replace(/[\\"]/g, (char) => `\\${char}`)}"` } +async function sweepStaleScratchDirectories(objectsHostPath: string): Promise { + if (sweptObjectsDirectories.has(objectsHostPath)) { + return + } + sweptObjectsDirectories.add(objectsHostPath) + const path = pathApiForGitPath(objectsHostPath) + const entries = await readdir(objectsHostPath).catch(() => []) + const cutoff = Date.now() - STALE_GIT_OBJECT_QUARANTINE_AGE_MS + for (const entry of entries) { + if (!entry.startsWith(GIT_OBJECT_QUARANTINE_DIR_PREFIX)) { + continue + } + const scratch = path.join(objectsHostPath, entry) + const modified = await stat(scratch).then( + (stats) => stats.mtimeMs, + () => undefined + ) + if (modified !== undefined && modified < cutoff) { + await removeTree(scratch).catch(() => {}) + } + } +} + +/** + * A partial clone fetches missing blobs on demand, and Git files that download + * as a pack in the scratch dir. Keep those packs so the next check does not + * download the same blobs again; merge-tree's own writes are loose objects. + */ +async function keepFetchedPacks(scratchHostPath: string, objectsHostPath: string): Promise { + const path = pathApiForGitPath(objectsHostPath) + const scratchPackDir = path.join(scratchHostPath, 'pack') + const entries = await readdir(scratchPackDir).catch(() => []) + const packNames = entries + .filter((entry) => /^pack-[0-9a-f]+\.pack$/.test(entry)) + .map((entry) => entry.slice(0, -'.pack'.length)) + for (const packName of packNames) { + const files = entries.filter((entry) => entry.startsWith(`${packName}.`)) + if (!files.includes(`${packName}.idx`)) { + continue + } + // Why `.idx` last: Git finds a pack through its index, so everything it names must already be in place. + const ordered = [...files.filter((file) => !file.endsWith('.idx')), `${packName}.idx`] + try { + for (const file of ordered) { + await rename(path.join(scratchPackDir, file), path.join(objectsHostPath, 'pack', file)) + } + } catch { + // Why: a pack without its index is invisible to Git; the next lookup fetches it again. + } + } +} + /** Resolves the objects dir once per quarantine; each run gets its own scratch dir. */ -export function createGitObjectQuarantine(host: GitObjectQuarantineHost): GitObjectQuarantine { +export function createGitObjectQuarantine( + resolveObjectsDirectory: () => Promise +): GitObjectQuarantine { let objectsDirectory: Promise | undefined - const resolveObjectsDirectory = (): Promise => { - objectsDirectory ??= host.resolveObjectsDirectory().catch(() => undefined) + const resolveOnce = (): Promise => { + objectsDirectory ??= resolveObjectsDirectory().catch(() => undefined) return objectsDirectory } return { async run(command) { - const objects = await resolveObjectsDirectory() + const objects = await resolveOnce() let scratchHostPath: string | undefined if (objects) { - try { - scratchHostPath = await host.makeTempDirectory( - pathApiFor(objects.hostPath).join(objects.hostPath, GIT_OBJECT_QUARANTINE_DIR_PREFIX) - ) - } catch { - // Why: bookkeeping must not block the user's action; run unquarantined. - scratchHostPath = undefined - } + await sweepStaleScratchDirectories(objects.hostPath) + const path = pathApiForGitPath(objects.hostPath) + scratchHostPath = await mkdtemp( + path.join(objects.hostPath, GIT_OBJECT_QUARANTINE_DIR_PREFIX) + ).catch(() => undefined) } if (!objects || !scratchHostPath) { + // Why: bookkeeping must not block the user's action; run unquarantined. return command(undefined) } try { return await command({ - GIT_OBJECT_DIRECTORY: pathApiFor(objects.gitPath).join( + GIT_OBJECT_DIRECTORY: pathApiForGitPath(objects.gitPath).join( objects.gitPath, - pathApiFor(scratchHostPath).basename(scratchHostPath) + pathApiForGitPath(scratchHostPath).basename(scratchHostPath) ), GIT_ALTERNATE_OBJECT_DIRECTORIES: alternateObjectDirectoriesValue(objects.gitPath) }) } finally { - await host.removeDirectory(scratchHostPath).catch(() => {}) + await keepFetchedPacks(scratchHostPath, objects.hostPath) + await removeTree(scratchHostPath).catch(() => {}) } } } } + +export function _resetGitObjectQuarantineSweepForTests(): void { + sweptObjectsDirectories.clear() +} From 0f3ed37e90fdd7e44ed19161b5b77e82ec2ca036 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Tue, 29 Sep 2026 00:18:57 -0700 Subject: [PATCH 03/13] fix(git): quote Windows alternates paths containing ';' and log scratch cleanup failures --- src/shared/git-object-quarantine.test.ts | 3 ++- src/shared/git-object-quarantine.ts | 17 +++++++++++++---- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/src/shared/git-object-quarantine.test.ts b/src/shared/git-object-quarantine.test.ts index 1746f17e6069..1b60a5b31d14 100644 --- a/src/shared/git-object-quarantine.test.ts +++ b/src/shared/git-object-quarantine.test.ts @@ -121,7 +121,8 @@ describe('createGitObjectQuarantine', () => { ['/plain/objects', '/plain/objects'], ['/odd:dir/objects', '"/odd:dir/objects"'], ['/quo"te\\dir/objects', '"/quo\\"te\\\\dir/objects"'], - ['C:\\repo\\.git\\objects', 'C:\\repo\\.git\\objects'] + ['C:\\repo\\.git\\objects', 'C:\\repo\\.git\\objects'], + ['C:\\a;b\\.git\\objects', '"C:\\\\a;b\\\\.git\\\\objects"'] ])('spells alternates for %s so Git reads one entry', async (gitPath, expected) => { let seen: GitObjectQuarantineEnv | undefined diff --git a/src/shared/git-object-quarantine.ts b/src/shared/git-object-quarantine.ts index 119988c74064..c9cb60d5a17b 100644 --- a/src/shared/git-object-quarantine.ts +++ b/src/shared/git-object-quarantine.ts @@ -42,9 +42,10 @@ export function pathApiForGitPath(value: string): typeof posix { return isWindowsAbsolutePathLike(value) ? win32 : posix } -// Why: Git splits this variable on `:` (`;` on Windows) and C-unquotes a leading `"`. +// Why: Git splits this variable on `:` (`;` for Git for Windows) and C-unquotes a leading `"`. function alternateObjectDirectoriesValue(gitPath: string): string { - if (isWindowsAbsolutePathLike(gitPath) || !/[:"\\]/.test(gitPath)) { + const needsQuoting = isWindowsAbsolutePathLike(gitPath) ? /[;"]/ : /[:"\\]/ + if (!needsQuoting.test(gitPath)) { return gitPath } return `"${gitPath.replace(/[\\"]/g, (char) => `\\${char}`)}"` @@ -96,8 +97,9 @@ async function keepFetchedPacks(scratchHostPath: string, objectsHostPath: string for (const file of ordered) { await rename(path.join(scratchPackDir, file), path.join(objectsHostPath, 'pack', file)) } - } catch { + } catch (error) { // Why: a pack without its index is invisible to Git; the next lookup fetches it again. + console.warn('[git-object-quarantine] could not keep a fetched pack', packName, error) } } } @@ -137,7 +139,14 @@ export function createGitObjectQuarantine( }) } finally { await keepFetchedPacks(scratchHostPath, objects.hostPath) - await removeTree(scratchHostPath).catch(() => {}) + await removeTree(scratchHostPath).catch((error: unknown) => { + // Why: the stale sweep removes it on a later run; the check's result still stands. + console.warn( + '[git-object-quarantine] could not remove scratch dir', + scratchHostPath, + error + ) + }) } } } From ed3be7d234db04f523a1c4339939063631bc89b3 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Wed, 30 Sep 2026 10:46:17 -0700 Subject: [PATCH 04/13] fix(git): sweep a merge-tree scratch dir only when its owner is provably gone Another Orca process on the same repo (a second instance, dev and release builds) could delete a scratch object dir still in use once its mtime passed an hour. Each scratch dir now records its owner (pid, hostname, platform, Linux pid namespace); the sweep removes it only when that owner shares this pid space and has exited, or, with no checkable owner, at Git's own two-week tmp_ expiry. Also: - A fetched pack whose later rename fails is rolled back, so the real store never keeps an index-less pack; a pack already installed is left alone. - WSL Git gets the /mnt/ spelling of a drive-path common dir (Git < 2.31 prints it relative, resolved against the C:\ repo path). - conflict-summary tests no longer create scratch dirs in the cwd. --- .../git/local-git-object-quarantine.test.ts | 11 ++ src/main/git/local-git-object-quarantine.ts | 8 +- src/main/github/conflict-summary.test.ts | 8 + src/shared/git-object-quarantine.test.ts | 180 ++++++++++++++++-- src/shared/git-object-quarantine.ts | 145 ++++++++++++-- 5 files changed, 317 insertions(+), 35 deletions(-) diff --git a/src/main/git/local-git-object-quarantine.test.ts b/src/main/git/local-git-object-quarantine.test.ts index 64b8c318f6b3..9272659d48b3 100644 --- a/src/main/git/local-git-object-quarantine.test.ts +++ b/src/main/git/local-git-object-quarantine.test.ts @@ -39,6 +39,17 @@ describe('localGitObjectsDirectory', () => { }) }) + it('hands WSL Git the drvfs spelling of a drive-path common dir (Git < 2.31 prints it relative)', () => { + expect(localGitObjectsDirectory('C:\\repo\\.git', 'Ubuntu')).toEqual({ + hostPath: 'C:\\repo\\.git\\objects', + gitPath: '/mnt/c/repo/.git/objects' + }) + expect(localGitObjectsDirectory('/mnt/c/repo/.git', 'Ubuntu')).toEqual({ + hostPath: 'C:\\repo\\.git\\objects', + gitPath: '/mnt/c/repo/.git/objects' + }) + }) + it('uses one spelling for native Git', () => { expect(localGitObjectsDirectory('/Users/me/repo/.git', undefined)).toEqual({ hostPath: '/Users/me/repo/.git/objects', diff --git a/src/main/git/local-git-object-quarantine.ts b/src/main/git/local-git-object-quarantine.ts index 48849d2103b1..f0addb82ff83 100644 --- a/src/main/git/local-git-object-quarantine.ts +++ b/src/main/git/local-git-object-quarantine.ts @@ -7,7 +7,7 @@ import { type GitObjectsDirectory } from '../../shared/git-object-quarantine' import { addWslEnvKeys } from '../../shared/wsl-env' -import { isWslUncPath, toWindowsWslPath } from '../../shared/wsl-paths' +import { toLinuxPath, toWindowsWslPath } from '../../shared/wsl-paths' import { parseWslPath } from '../wsl' import { usesHostGitForWslLinkedWorktree } from './wsl-linked-worktree-git-routing' import { readRepoCommonDirFromGit } from './worktree-list-reader' @@ -23,12 +23,14 @@ export function localGitObjectsDirectory( wslDistro: string | undefined ): GitObjectsDirectory { const hostCommonDir = - wslDistro && !isWslUncPath(commonDir) && !isWindowsAbsolutePathLike(commonDir) + wslDistro && !isWindowsAbsolutePathLike(commonDir) ? toWindowsWslPath(commonDir, wslDistro) : commonDir + // Why: Git < 2.31 prints a relative common dir, resolved against a `C:\` repo path; WSL Git needs `/mnt/c`. + const gitCommonDir = wslDistro ? toLinuxPath(commonDir) : commonDir return { hostPath: pathApiForGitPath(hostCommonDir).join(hostCommonDir, 'objects'), - gitPath: pathApiForGitPath(commonDir).join(commonDir, 'objects') + gitPath: pathApiForGitPath(gitCommonDir).join(gitCommonDir, 'objects') } } diff --git a/src/main/github/conflict-summary.test.ts b/src/main/github/conflict-summary.test.ts index aac9b63c9a39..3010959eec46 100644 --- a/src/main/github/conflict-summary.test.ts +++ b/src/main/github/conflict-summary.test.ts @@ -1,3 +1,4 @@ +import type * as NodeFsPromises from 'node:fs/promises' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' // Why these tests exist: the conflict-summary derivation used to re-run a @@ -10,6 +11,13 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' const gitExecFileAsyncMock = vi.hoisted(() => vi.fn()) vi.mock('../git/runner', () => ({ gitExecFileAsync: gitExecFileAsyncMock })) +// Why: off Windows a WSL objects path is relative, so a real scratch dir would land in the cwd. +vi.mock('node:fs/promises', async (importOriginal) => ({ + ...(await importOriginal()), + mkdtemp: async () => { + throw new Error('no object store in this test') + } +})) import { CONFLICT_SUMMARY_BASE_FETCH_WINDOW_MS } from './conflict-summary-cache' import { __resetPRConflictSummaryCachesForTests, getPRConflictSummary } from './conflict-summary' diff --git a/src/shared/git-object-quarantine.test.ts b/src/shared/git-object-quarantine.test.ts index 1b60a5b31d14..6f3a84831b29 100644 --- a/src/shared/git-object-quarantine.test.ts +++ b/src/shared/git-object-quarantine.test.ts @@ -3,21 +3,45 @@ import { mkdirSync, mkdtempSync, readdirSync, + readFileSync, rmSync, utimesSync, writeFileSync } from 'node:fs' -import { tmpdir } from 'node:os' -import { join } from 'node:path' +import type * as NodeFsPromises from 'node:fs/promises' +import { hostname, tmpdir } from 'node:os' +import { basename, join } from 'node:path' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { _resetGitObjectQuarantineSweepForTests, createGitObjectQuarantine, GIT_OBJECT_QUARANTINE_DIR_PREFIX, + GIT_OBJECT_QUARANTINE_OWNER_FILE, + gitObjectQuarantineOwner, STALE_GIT_OBJECT_QUARANTINE_AGE_MS, - type GitObjectQuarantineEnv + UNOWNED_GIT_OBJECT_QUARANTINE_AGE_MS, + type GitObjectQuarantineEnv, + type GitObjectQuarantineOwner } from './git-object-quarantine' +const { failedRenameTargets } = vi.hoisted(() => ({ failedRenameTargets: new Set() })) + +vi.mock('node:fs/promises', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + rename: async (from: string, to: string) => { + if (failedRenameTargets.has(basename(to))) { + throw Object.assign(new Error('EBUSY: resource busy or locked'), { code: 'EBUSY' }) + } + return actual.rename(from, to) + } + } +}) + +const EXITED_PID = 2_000_000_001 +const OTHER_USER_PID = 2_000_000_002 + describe('createGitObjectQuarantine', () => { let root: string let objects: string @@ -30,6 +54,8 @@ describe('createGitObjectQuarantine', () => { }) afterEach(() => { + failedRenameTargets.clear() + vi.restoreAllMocks() rmSync(root, { recursive: true, force: true }) }) @@ -158,27 +184,141 @@ describe('createGitObjectQuarantine', () => { expect(scratchDirs()).toEqual([]) }) - it('removes scratch dirs a crash stranded, once per objects dir, leaving recent ones and Git’s own', async () => { - const stale = join(objects, `${GIT_OBJECT_QUARANTINE_DIR_PREFIX}stale`) - const recent = join(objects, `${GIT_OBJECT_QUARANTINE_DIR_PREFIX}recent`) - const gitOwn = join(objects, 'tmp_objdir-incoming-old') - for (const dir of [stale, recent, gitOwn]) { + it('rolls back a pack whose index could not be moved, keeping the other packs', async () => { + failedRenameTargets.add('pack-abc1.idx') + + await createGitObjectQuarantine(resolveNative()).run(async (env) => { + const scratchPack = join(env?.GIT_OBJECT_DIRECTORY ?? '', 'pack') + mkdirSync(scratchPack) + for (const file of ['pack-abc1.pack', 'pack-abc1.idx', 'pack-def2.pack', 'pack-def2.idx']) { + writeFileSync(join(scratchPack, file), file) + } + }) + + expect(readdirSync(join(objects, 'pack')).sort()).toEqual(['pack-def2.idx', 'pack-def2.pack']) + expect(scratchDirs()).toEqual([]) + }) + + it('leaves a pack the real store already has untouched', async () => { + writeFileSync(join(objects, 'pack', 'pack-abc1.pack'), 'installed') + writeFileSync(join(objects, 'pack', 'pack-abc1.idx'), 'installed') + + await createGitObjectQuarantine(resolveNative()).run(async (env) => { + const scratchPack = join(env?.GIT_OBJECT_DIRECTORY ?? '', 'pack') + mkdirSync(scratchPack) + writeFileSync(join(scratchPack, 'pack-abc1.pack'), 'refetched') + writeFileSync(join(scratchPack, 'pack-abc1.idx'), 'refetched') + }) + + expect(readFileSync(join(objects, 'pack', 'pack-abc1.pack'), 'utf8')).toBe('installed') + expect(readFileSync(join(objects, 'pack', 'pack-abc1.idx'), 'utf8')).toBe('installed') + }) + + it('records this process as the owner of its scratch dir', async () => { + let owner: unknown + + await createGitObjectQuarantine(resolveNative()).run(async (env) => { + owner = JSON.parse( + readFileSync( + join(env?.GIT_OBJECT_DIRECTORY ?? '', GIT_OBJECT_QUARANTINE_OWNER_FILE), + 'utf8' + ) + ) + }) + + expect(owner).toMatchObject({ + pid: process.pid, + hostname: hostname(), + platform: process.platform + }) + }) + + describe('stale scratch sweep', () => { + const HOUR = 60 * 60 * 1000 + const self = (): GitObjectQuarantineOwner => gitObjectQuarantineOwner() + const scratch = (name: string): string => + join(objects, `${GIT_OBJECT_QUARANTINE_DIR_PREFIX}${name}`) + const makeScratch = ( + name: string, + ageMs: number, + owner?: GitObjectQuarantineOwner | string + ) => { + const dir = scratch(name) mkdirSync(dir) + if (owner !== undefined) { + writeFileSync( + join(dir, GIT_OBJECT_QUARANTINE_OWNER_FILE), + typeof owner === 'string' ? owner : JSON.stringify(owner) + ) + } + const modified = (Date.now() - ageMs) / 1000 + utimesSync(dir, modified, modified) + return dir } - const old = (Date.now() - STALE_GIT_OBJECT_QUARANTINE_AGE_MS - 60_000) / 1000 - utimesSync(stale, old, old) - utimesSync(gitOwn, old, old) - const quarantine = createGitObjectQuarantine(resolveNative()) - await quarantine.run(async () => {}) + beforeEach(() => { + vi.spyOn(process, 'kill').mockImplementation((pid) => { + const code = pid === EXITED_PID ? 'ESRCH' : pid === OTHER_USER_PID ? 'EPERM' : undefined + if (code) { + throw Object.assign(new Error(`kill ${code}`), { code }) + } + return true + }) + }) + + const exited = (): GitObjectQuarantineOwner => ({ ...self(), pid: EXITED_PID }) + const elsewhere = (): GitObjectQuarantineOwner => ({ ...self(), hostname: `not-${hostname()}` }) + // Why the other platform: a WSL distro reports the Windows hostname but has its own pids. + const otherPidSpace = (): GitObjectQuarantineOwner => ({ + ...exited(), + platform: process.platform === 'win32' ? 'linux' : 'win32' + }) + const pastGitExpiry = UNOWNED_GIT_OBJECT_QUARANTINE_AGE_MS + HOUR + + it.each<[string, boolean, number, () => GitObjectQuarantineOwner | string | undefined]>([ + ['its owner on this host has exited', true, 2 * HOUR, exited], + ['its owner is this process', false, 2 * HOUR, self], + [ + 'its owner runs as another user', + false, + 2 * HOUR, + () => ({ ...self(), pid: OTHER_USER_PID }) + ], + ['its owner exited under an hour ago', false, STALE_GIT_OBJECT_QUARANTINE_AGE_MS / 2, exited], + [ + 'its owner is on another host', + false, + 2 * 24 * HOUR, + () => ({ ...elsewhere(), pid: EXITED_PID }) + ], + ['its owner shares the hostname but not the pid space', false, 2 * 24 * HOUR, otherPidSpace], + ['its owner is on another host and Git would expire it', true, pastGitExpiry, elsewhere], + ['it has no owner record', false, 2 * 24 * HOUR, () => undefined], + ['its owner record is unreadable', false, 2 * 24 * HOUR, () => '{"pid":'], + ['it has no owner record and Git would expire it', true, pastGitExpiry, () => undefined] + ])('when %s, removes it: %s', async (_label, removed, ageMs, owner) => { + const dir = makeScratch('candidate', ageMs, owner()) - expect(existsSync(stale)).toBe(false) - expect(existsSync(recent)).toBe(true) - expect(existsSync(gitOwn)).toBe(true) + await createGitObjectQuarantine(resolveNative()).run(async () => {}) - mkdirSync(stale) - utimesSync(stale, old, old) - await createGitObjectQuarantine(resolveNative()).run(async () => {}) - expect(existsSync(stale)).toBe(true) + expect(existsSync(dir)).toBe(!removed) + }) + + it('sweeps once per objects dir and never touches Git’s own quarantine dirs', async () => { + const gitOwn = join(objects, 'tmp_objdir-incoming-old') + mkdirSync(gitOwn) + const old = (Date.now() - UNOWNED_GIT_OBJECT_QUARANTINE_AGE_MS - HOUR) / 1000 + utimesSync(gitOwn, old, old) + const stranded = makeScratch('stranded', 2 * HOUR, exited()) + + await createGitObjectQuarantine(resolveNative()).run(async () => {}) + + expect(existsSync(stranded)).toBe(false) + expect(existsSync(gitOwn)).toBe(true) + + makeScratch('stranded', 2 * HOUR, exited()) + await createGitObjectQuarantine(resolveNative()).run(async () => {}) + expect(existsSync(stranded)).toBe(true) + }) }) }) diff --git a/src/shared/git-object-quarantine.ts b/src/shared/git-object-quarantine.ts index c9cb60d5a17b..8f3f65ebe82b 100644 --- a/src/shared/git-object-quarantine.ts +++ b/src/shared/git-object-quarantine.ts @@ -1,4 +1,6 @@ -import { mkdtemp, readdir, rename, stat } from 'node:fs/promises' +import { readlinkSync } from 'node:fs' +import { lstat, mkdtemp, readFile, readdir, rename, rm, stat, writeFile } from 'node:fs/promises' +import { hostname } from 'node:os' import { posix, win32 } from 'node:path' import { isWindowsAbsolutePathLike } from './cross-platform-path' import { removeTree } from './windows-transient-lock-removal' @@ -31,11 +33,27 @@ export type GitObjectQuarantine = { // `git gc` expires stale `tmp_*` entries too. export const GIT_OBJECT_QUARANTINE_DIR_PREFIX = 'tmp_objdir-orca-merge-tree-' -// Why an hour: a run lives at most as long as a Git read (minutes), so anything older was -// stranded by a crash or a failed delete, including repos where `git gc` never runs. +// Why a floor even for a dead owner: it covers pid spaces the owner record cannot tell apart. export const STALE_GIT_OBJECT_QUARANTINE_AGE_MS = 60 * 60 * 1000 +// Why two weeks: `git gc` expires `objects/tmp_*` dirs at that age (gc.pruneExpire); with no +// owner this process can check, only Git's own abandonment rule is safe. +export const UNOWNED_GIT_OBJECT_QUARANTINE_AGE_MS = 14 * 24 * 60 * 60 * 1000 + +// Why: Orca processes sharing a repo (a second instance, dev and release builds) sweep each +// other's scratch dirs; only a provably dead owner makes one stale, as Git decides for gc.pid. +export const GIT_OBJECT_QUARANTINE_OWNER_FILE = 'orca-owner.json' + +export type GitObjectQuarantineOwner = { + pid: number + hostname: string + platform: string + /** Linux only: a pid is meaningful only inside the pid namespace that issued it. */ + pidNamespace?: string +} + const sweptObjectsDirectories = new Set() +let currentOwner: GitObjectQuarantineOwner | undefined /** Decided by path syntax, not by platform: a Windows main process drives WSL Git. */ export function pathApiForGitPath(value: string): typeof posix { @@ -51,6 +69,92 @@ function alternateObjectDirectoriesValue(gitPath: string): string { return `"${gitPath.replace(/[\\"]/g, (char) => `\\${char}`)}"` } +export function gitObjectQuarantineOwner(): GitObjectQuarantineOwner { + if (!currentOwner) { + let pidNamespace: string | undefined + if (process.platform === 'linux') { + try { + pidNamespace = readlinkSync('/proc/self/ns/pid') + } catch {} + } + currentOwner = { + pid: process.pid, + // Why platform too: a WSL distro takes the Windows hostname but has its own pids. + hostname: hostname(), + platform: process.platform, + ...(pidNamespace ? { pidNamespace } : {}) + } + } + return currentOwner +} + +async function readScratchOwner(scratch: string): Promise { + const path = pathApiForGitPath(scratch) + try { + const parsed: unknown = JSON.parse( + await readFile(path.join(scratch, GIT_OBJECT_QUARANTINE_OWNER_FILE), 'utf8') + ) + if ( + typeof parsed === 'object' && + parsed !== null && + 'pid' in parsed && + typeof parsed.pid === 'number' && + Number.isSafeInteger(parsed.pid) && + parsed.pid > 0 && + 'hostname' in parsed && + typeof parsed.hostname === 'string' && + 'platform' in parsed && + typeof parsed.platform === 'string' + ) { + const pidNamespace = + 'pidNamespace' in parsed && typeof parsed.pidNamespace === 'string' + ? parsed.pidNamespace + : undefined + return { + pid: parsed.pid, + hostname: parsed.hostname, + platform: parsed.platform, + ...(pidNamespace ? { pidNamespace } : {}) + } + } + } catch {} + return undefined +} + +function sharesThisPidSpace(owner: GitObjectQuarantineOwner): boolean { + const self = gitObjectQuarantineOwner() + return ( + owner.hostname === self.hostname && + owner.platform === self.platform && + owner.pidNamespace === self.pidNamespace + ) +} + +// Why only ESRCH: EPERM means the pid exists under another user, so a reused pid keeps the dir. +function isProcessAlive(pid: number): boolean { + try { + process.kill(pid, 0) + return true + } catch (error) { + return !(error instanceof Error && 'code' in error && error.code === 'ESRCH') + } +} + +async function isStaleScratchDirectory(scratch: string, now: number): Promise { + const modified = await stat(scratch).then( + (stats) => stats.mtimeMs, + () => undefined + ) + if (modified === undefined || now - modified < STALE_GIT_OBJECT_QUARANTINE_AGE_MS) { + return false + } + const owner = await readScratchOwner(scratch) + if (owner && sharesThisPidSpace(owner)) { + return !isProcessAlive(owner.pid) + } + return now - modified >= UNOWNED_GIT_OBJECT_QUARANTINE_AGE_MS +} + async function sweepStaleScratchDirectories(objectsHostPath: string): Promise { if (sweptObjectsDirectories.has(objectsHostPath)) { return @@ -58,17 +162,13 @@ async function sweepStaleScratchDirectories(objectsHostPath: string): Promise []) - const cutoff = Date.now() - STALE_GIT_OBJECT_QUARANTINE_AGE_MS + const now = Date.now() for (const entry of entries) { if (!entry.startsWith(GIT_OBJECT_QUARANTINE_DIR_PREFIX)) { continue } const scratch = path.join(objectsHostPath, entry) - const modified = await stat(scratch).then( - (stats) => stats.mtimeMs, - () => undefined - ) - if (modified !== undefined && modified < cutoff) { + if (await isStaleScratchDirectory(scratch, now)) { await removeTree(scratch).catch(() => {}) } } @@ -93,12 +193,26 @@ async function keepFetchedPacks(scratchHostPath: string, objectsHostPath: string } // Why `.idx` last: Git finds a pack through its index, so everything it names must already be in place. const ordered = [...files.filter((file) => !file.endsWith('.idx')), `${packName}.idx`] + const installed: string[] = [] try { for (const file of ordered) { - await rename(path.join(scratchPackDir, file), path.join(objectsHostPath, 'pack', file)) + const target = path.join(objectsHostPath, 'pack', file) + // Why skip: pack names are content hashes, and replacing a live pack's file could strand its index. + const exists = await lstat(target).then( + () => true, + () => false + ) + if (exists) { + continue + } + await rename(path.join(scratchPackDir, file), target) + installed.push(target) } } catch (error) { - // Why: a pack without its index is invisible to Git; the next lookup fetches it again. + // Why undo: a pack without its index is garbage Git cannot see; the next lookup fetches it again. + for (const target of installed) { + await rm(target, { force: true }).catch(() => {}) + } console.warn('[git-object-quarantine] could not keep a fetched pack', packName, error) } } @@ -124,6 +238,13 @@ export function createGitObjectQuarantine( scratchHostPath = await mkdtemp( path.join(objects.hostPath, GIT_OBJECT_QUARANTINE_DIR_PREFIX) ).catch(() => undefined) + if (scratchHostPath) { + // Why no fallback on failure: an ownerless dir is kept until Git's own two-week expiry. + await writeFile( + path.join(scratchHostPath, GIT_OBJECT_QUARANTINE_OWNER_FILE), + JSON.stringify(gitObjectQuarantineOwner()) + ).catch(() => {}) + } } if (!objects || !scratchHostPath) { // Why: bookkeeping must not block the user's action; run unquarantined. @@ -140,7 +261,7 @@ export function createGitObjectQuarantine( } finally { await keepFetchedPacks(scratchHostPath, objects.hostPath) await removeTree(scratchHostPath).catch((error: unknown) => { - // Why: the stale sweep removes it on a later run; the check's result still stands. + // Why: a later process sweeps it once this one has exited; the check's result still stands. console.warn( '[git-object-quarantine] could not remove scratch dir', scratchHostPath, From d3c2df63ffa88c25c3421b04077c779358974506 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Wed, 30 Sep 2026 10:51:25 -0700 Subject: [PATCH 05/13] fix(git): cap every merge-tree scratch dir at two weeks, live-looking owner included A reused pid held by a long-lived process could keep a leftover scratch dir forever in a repo where git gc never runs. The two-week age (git gc's own tmp_ expiry) now removes any scratch dir; a younger one is still swept only when its owner shares this pid space and has exited. --- src/shared/git-object-quarantine.test.ts | 7 ++++--- src/shared/git-object-quarantine.ts | 17 +++++++++-------- 2 files changed, 13 insertions(+), 11 deletions(-) diff --git a/src/shared/git-object-quarantine.test.ts b/src/shared/git-object-quarantine.test.ts index 6f3a84831b29..3f23f929f145 100644 --- a/src/shared/git-object-quarantine.test.ts +++ b/src/shared/git-object-quarantine.test.ts @@ -19,7 +19,7 @@ import { GIT_OBJECT_QUARANTINE_OWNER_FILE, gitObjectQuarantineOwner, STALE_GIT_OBJECT_QUARANTINE_AGE_MS, - UNOWNED_GIT_OBJECT_QUARANTINE_AGE_MS, + MAX_GIT_OBJECT_QUARANTINE_AGE_MS, type GitObjectQuarantineEnv, type GitObjectQuarantineOwner } from './git-object-quarantine' @@ -273,11 +273,12 @@ describe('createGitObjectQuarantine', () => { ...exited(), platform: process.platform === 'win32' ? 'linux' : 'win32' }) - const pastGitExpiry = UNOWNED_GIT_OBJECT_QUARANTINE_AGE_MS + HOUR + const pastGitExpiry = MAX_GIT_OBJECT_QUARANTINE_AGE_MS + HOUR it.each<[string, boolean, number, () => GitObjectQuarantineOwner | string | undefined]>([ ['its owner on this host has exited', true, 2 * HOUR, exited], ['its owner is this process', false, 2 * HOUR, self], + ['its owner on this host looks alive but it is 15 days old', true, 15 * 24 * HOUR, self], [ 'its owner runs as another user', false, @@ -307,7 +308,7 @@ describe('createGitObjectQuarantine', () => { it('sweeps once per objects dir and never touches Git’s own quarantine dirs', async () => { const gitOwn = join(objects, 'tmp_objdir-incoming-old') mkdirSync(gitOwn) - const old = (Date.now() - UNOWNED_GIT_OBJECT_QUARANTINE_AGE_MS - HOUR) / 1000 + const old = (Date.now() - MAX_GIT_OBJECT_QUARANTINE_AGE_MS - HOUR) / 1000 utimesSync(gitOwn, old, old) const stranded = makeScratch('stranded', 2 * HOUR, exited()) diff --git a/src/shared/git-object-quarantine.ts b/src/shared/git-object-quarantine.ts index 8f3f65ebe82b..3c69ffa8ede4 100644 --- a/src/shared/git-object-quarantine.ts +++ b/src/shared/git-object-quarantine.ts @@ -36,9 +36,10 @@ export const GIT_OBJECT_QUARANTINE_DIR_PREFIX = 'tmp_objdir-orca-merge-tree-' // Why a floor even for a dead owner: it covers pid spaces the owner record cannot tell apart. export const STALE_GIT_OBJECT_QUARANTINE_AGE_MS = 60 * 60 * 1000 -// Why two weeks: `git gc` expires `objects/tmp_*` dirs at that age (gc.pruneExpire); with no -// owner this process can check, only Git's own abandonment rule is safe. -export const UNOWNED_GIT_OBJECT_QUARANTINE_AGE_MS = 14 * 24 * 60 * 60 * 1000 +// Why a hard cap for every dir, live-looking owner included: a reused pid (a daemon, a shell) +// would otherwise keep a leftover forever in a gc-off repo. Two weeks is `git gc`'s own expiry +// for `objects/tmp_*` (gc.pruneExpire), and no merge-tree runs that long. +export const MAX_GIT_OBJECT_QUARANTINE_AGE_MS = 14 * 24 * 60 * 60 * 1000 // Why: Orca processes sharing a repo (a second instance, dev and release builds) sweep each // other's scratch dirs; only a provably dead owner makes one stale, as Git decides for gc.pid. @@ -148,11 +149,11 @@ async function isStaleScratchDirectory(scratch: string, now: number): Promise= MAX_GIT_OBJECT_QUARANTINE_AGE_MS) { + return true } - return now - modified >= UNOWNED_GIT_OBJECT_QUARANTINE_AGE_MS + const owner = await readScratchOwner(scratch) + return owner !== undefined && sharesThisPidSpace(owner) && !isProcessAlive(owner.pid) } async function sweepStaleScratchDirectories(objectsHostPath: string): Promise { @@ -239,7 +240,7 @@ export function createGitObjectQuarantine( path.join(objects.hostPath, GIT_OBJECT_QUARANTINE_DIR_PREFIX) ).catch(() => undefined) if (scratchHostPath) { - // Why no fallback on failure: an ownerless dir is kept until Git's own two-week expiry. + // Why no fallback on failure: an ownerless dir is kept until the two-week cap. await writeFile( path.join(scratchHostPath, GIT_OBJECT_QUARANTINE_OWNER_FILE), JSON.stringify(gitObjectQuarantineOwner()) From 558f13e0cd8863cf9a5b82f47340232f002963b6 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Wed, 30 Sep 2026 11:09:14 -0700 Subject: [PATCH 06/13] fix(git): sweep merge-tree scratch dirs by age alone, off the check's path - The stale sweep now removes any scratch dir at least two weeks old, Git's own expiry for its objects/tmp_* dirs; no merge-tree runs that long. The owner file and pid-space probe are gone, and with them the cases where a sweeper misjudged another process's pid. - The sweep no longer delays the user's check: it starts without being awaited and can only touch dirs far older than the current run's. - A fetched pack's .keep never moves into the real store, so a transient one from an aborted lazy fetch cannot stop gc from repacking that pack. - Tests: the legacy merge-tree fallback gets the scratch env; the local quarantine's WSL decisions (distro from a UNC path, drvfs spelling, host-Git linked-worktree skip, WSLENV hand-off). --- .../git/local-git-object-quarantine.test.ts | 83 ++++++++- src/main/github/conflict-summary.test.ts | 53 +++++- src/shared/git-object-quarantine.test.ts | 155 +++++++---------- src/shared/git-object-quarantine.ts | 157 ++++-------------- 4 files changed, 219 insertions(+), 229 deletions(-) diff --git a/src/main/git/local-git-object-quarantine.test.ts b/src/main/git/local-git-object-quarantine.test.ts index 9272659d48b3..0e24bb38e54d 100644 --- a/src/main/git/local-git-object-quarantine.test.ts +++ b/src/main/git/local-git-object-quarantine.test.ts @@ -1,8 +1,42 @@ -import { describe, expect, it } from 'vitest' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import type * as GitObjectQuarantineModule from '../../shared/git-object-quarantine' +import type { GitObjectQuarantine, GitObjectsDirectory } from '../../shared/git-object-quarantine' import { + createLocalGitObjectQuarantine, localGitObjectQuarantineProcessEnv, localGitObjectsDirectory } from './local-git-object-quarantine' +import { + resetWslLinkedWorktreeGitRoutingForTests, + seedWslLinkedWorktreeGitRoutingForTests +} from './wsl-linked-worktree-git-routing' + +const { readRepoCommonDirFromGitMock } = vi.hoisted(() => ({ + readRepoCommonDirFromGitMock: vi.fn() +})) + +vi.mock('./worktree-list-reader', () => ({ + readRepoCommonDirFromGit: readRepoCommonDirFromGitMock +})) +vi.mock('../../shared/git-object-quarantine', async (importOriginal) => ({ + ...(await importOriginal()), + // Why: hands the resolved objects dir straight to the command; these tests pin routing, not disk. + createGitObjectQuarantine: ( + resolve: () => Promise + ): GitObjectQuarantine => ({ + run: async (command) => { + const objects = await resolve() + return command( + objects + ? { + GIT_OBJECT_DIRECTORY: `${objects.gitPath}/scratch`, + GIT_ALTERNATE_OBJECT_DIRECTORIES: objects.gitPath + } + : undefined + ) + } + }) +})) const QUARANTINE = { GIT_OBJECT_DIRECTORY: '/home/me/repo/.git/objects/tmp_objdir-orca-merge-tree-x', @@ -61,3 +95,50 @@ describe('localGitObjectsDirectory', () => { }) }) }) + +describe('createLocalGitObjectQuarantine on a Windows host', () => { + const originalPlatform = process.platform + + beforeEach(() => { + Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) + resetWslLinkedWorktreeGitRoutingForTests() + readRepoCommonDirFromGitMock.mockReset() + }) + + afterEach(() => { + Object.defineProperty(process, 'platform', { configurable: true, value: originalPlatform }) + resetWslLinkedWorktreeGitRoutingForTests() + }) + + const envSeenBy = (repoPath: string, options: { wslDistro?: string } = {}) => + createLocalGitObjectQuarantine(repoPath, options).run(async (env) => env) + + it('takes the distro from a WSL repo path and hands WSL Git the variables through WSLENV', async () => { + readRepoCommonDirFromGitMock.mockResolvedValue('/home/me/repo/.git') + + const env = await envSeenBy('\\\\wsl.localhost\\Ubuntu\\home\\me\\repo') + + expect(env).toMatchObject({ + GIT_OBJECT_DIRECTORY: '/home/me/repo/.git/objects/scratch', + GIT_ALTERNATE_OBJECT_DIRECTORIES: '/home/me/repo/.git/objects' + }) + expect(env?.WSLENV?.split(':')).toEqual( + expect.arrayContaining(['GIT_OBJECT_DIRECTORY', 'GIT_ALTERNATE_OBJECT_DIRECTORIES']) + ) + }) + + it('hands WSL Git the drvfs spelling for a drive-path repo', async () => { + readRepoCommonDirFromGitMock.mockResolvedValue('C:\\repo\\.git') + + const env = await envSeenBy('C:\\repo', { wslDistro: 'Ubuntu' }) + + expect(env?.GIT_OBJECT_DIRECTORY).toBe('/mnt/c/repo/.git/objects/scratch') + }) + + it('runs unquarantined when a WSL-authored linked worktree routes to Windows Git', async () => { + readRepoCommonDirFromGitMock.mockResolvedValue('/mnt/c/repo/.git') + seedWslLinkedWorktreeGitRoutingForTests('C:\\wt') + + await expect(envSeenBy('C:\\wt', { wslDistro: 'Ubuntu' })).resolves.toBeUndefined() + }) +}) diff --git a/src/main/github/conflict-summary.test.ts b/src/main/github/conflict-summary.test.ts index 3010959eec46..1da118da8610 100644 --- a/src/main/github/conflict-summary.test.ts +++ b/src/main/github/conflict-summary.test.ts @@ -1,4 +1,7 @@ +import { mkdirSync, mkdtempSync, readdirSync, realpathSync, rmSync } from 'node:fs' import type * as NodeFsPromises from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { isAbsolute, join } from 'node:path' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' // Why these tests exist: the conflict-summary derivation used to re-run a @@ -12,12 +15,18 @@ const gitExecFileAsyncMock = vi.hoisted(() => vi.fn()) vi.mock('../git/runner', () => ({ gitExecFileAsync: gitExecFileAsyncMock })) // Why: off Windows a WSL objects path is relative, so a real scratch dir would land in the cwd. -vi.mock('node:fs/promises', async (importOriginal) => ({ - ...(await importOriginal()), - mkdtemp: async () => { - throw new Error('no object store in this test') +vi.mock('node:fs/promises', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + mkdtemp: async (prefix: string) => { + if (!isAbsolute(prefix)) { + throw new Error('no object store in this test') + } + return actual.mkdtemp(prefix) + } } -})) +}) import { CONFLICT_SUMMARY_BASE_FETCH_WINDOW_MS } from './conflict-summary-cache' import { __resetPRConflictSummaryCachesForTests, getPRConflictSummary } from './conflict-summary' @@ -383,4 +392,38 @@ describe('getPRConflictSummary caching', () => { expect(spawnCount('merge-base')).toBe(2) expect(spawnCount('merge-tree')).toBe(1) }) + + it('runs the legacy merge-tree fallback against the scratch object store too', async () => { + const commonDir = join( + realpathSync(mkdtempSync(join(tmpdir(), 'orca-conflict-summary-'))), + '.git' + ) + mkdirSync(join(commonDir, 'objects'), { recursive: true }) + try { + mockGitDispatch({ + [COMMON_DIR_COMMAND]: async () => ({ stdout: `${commonDir}\n` }), + 'merge-tree': async (argv) => { + if (argv.includes('--merge-base')) { + throw Object.assign(new Error("error: unknown option `merge-base'"), { stderr: '' }) + } + return { stdout: 'tree-oid\u0000src/conflict.ts\u0000' } + } + }) + + await expect(deriveSummary()).resolves.toEqual(expectedSummary) + + const legacy = gitExecFileAsyncMock.mock.calls.find( + ([argv]) => argv[0] === 'merge-tree' && !argv.includes('--merge-base') + ) + expect(legacy?.[1]?.env).toMatchObject({ + GIT_OBJECT_DIRECTORY: expect.stringContaining( + join(commonDir, 'objects', 'tmp_objdir-orca-merge-tree-') + ), + GIT_ALTERNATE_OBJECT_DIRECTORIES: join(commonDir, 'objects') + }) + expect(readdirSync(join(commonDir, 'objects'))).toEqual([]) + } finally { + rmSync(join(commonDir, '..'), { recursive: true, force: true }) + } + }) }) diff --git a/src/shared/git-object-quarantine.test.ts b/src/shared/git-object-quarantine.test.ts index 3f23f929f145..d84d9ca8e7bb 100644 --- a/src/shared/git-object-quarantine.test.ts +++ b/src/shared/git-object-quarantine.test.ts @@ -9,22 +9,23 @@ import { writeFileSync } from 'node:fs' import type * as NodeFsPromises from 'node:fs/promises' -import { hostname, tmpdir } from 'node:os' +import { tmpdir } from 'node:os' import { basename, join } from 'node:path' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { _resetGitObjectQuarantineSweepForTests, + _settleGitObjectQuarantineSweepsForTests, createGitObjectQuarantine, GIT_OBJECT_QUARANTINE_DIR_PREFIX, - GIT_OBJECT_QUARANTINE_OWNER_FILE, - gitObjectQuarantineOwner, STALE_GIT_OBJECT_QUARANTINE_AGE_MS, - MAX_GIT_OBJECT_QUARANTINE_AGE_MS, - type GitObjectQuarantineEnv, - type GitObjectQuarantineOwner + type GitObjectQuarantineEnv } from './git-object-quarantine' -const { failedRenameTargets } = vi.hoisted(() => ({ failedRenameTargets: new Set() })) +const { failedRenameTargets, objectsReaddirGate } = vi.hoisted(() => { + // Why: holds the stale sweep's `objects/` listing so a test can show the command does not wait for it. + const objectsReaddirGate: { current: Promise | undefined } = { current: undefined } + return { failedRenameTargets: new Set(), objectsReaddirGate } +}) vi.mock('node:fs/promises', async (importOriginal) => { const actual = await importOriginal() @@ -35,13 +36,16 @@ vi.mock('node:fs/promises', async (importOriginal) => { throw Object.assign(new Error('EBUSY: resource busy or locked'), { code: 'EBUSY' }) } return actual.rename(from, to) + }, + readdir: async (path: string) => { + if (basename(path) === 'objects') { + await objectsReaddirGate.current + } + return actual.readdir(path) } } }) -const EXITED_PID = 2_000_000_001 -const OTHER_USER_PID = 2_000_000_002 - describe('createGitObjectQuarantine', () => { let root: string let objects: string @@ -53,7 +57,8 @@ describe('createGitObjectQuarantine', () => { mkdirSync(join(objects, 'pack'), { recursive: true }) }) - afterEach(() => { + afterEach(async () => { + await _settleGitObjectQuarantineSweepsForTests() failedRenameTargets.clear() vi.restoreAllMocks() rmSync(root, { recursive: true, force: true }) @@ -214,112 +219,70 @@ describe('createGitObjectQuarantine', () => { expect(readFileSync(join(objects, 'pack', 'pack-abc1.idx'), 'utf8')).toBe('installed') }) - it('records this process as the owner of its scratch dir', async () => { - let owner: unknown - + it('never moves a fetched pack’s `.keep` into the real store', async () => { await createGitObjectQuarantine(resolveNative()).run(async (env) => { - owner = JSON.parse( - readFileSync( - join(env?.GIT_OBJECT_DIRECTORY ?? '', GIT_OBJECT_QUARANTINE_OWNER_FILE), - 'utf8' - ) - ) + const scratchPack = join(env?.GIT_OBJECT_DIRECTORY ?? '', 'pack') + mkdirSync(scratchPack) + for (const file of ['pack-abc1.pack', 'pack-abc1.idx', 'pack-abc1.keep']) { + writeFileSync(join(scratchPack, file), file) + } }) - expect(owner).toMatchObject({ - pid: process.pid, - hostname: hostname(), - platform: process.platform - }) + expect(readdirSync(join(objects, 'pack')).sort()).toEqual(['pack-abc1.idx', 'pack-abc1.pack']) }) describe('stale scratch sweep', () => { - const HOUR = 60 * 60 * 1000 - const self = (): GitObjectQuarantineOwner => gitObjectQuarantineOwner() - const scratch = (name: string): string => - join(objects, `${GIT_OBJECT_QUARANTINE_DIR_PREFIX}${name}`) - const makeScratch = ( - name: string, - ageMs: number, - owner?: GitObjectQuarantineOwner | string - ) => { - const dir = scratch(name) + const DAY = 24 * 60 * 60 * 1000 + const makeDir = (name: string, ageMs: number): string => { + const dir = join(objects, name) mkdirSync(dir) - if (owner !== undefined) { - writeFileSync( - join(dir, GIT_OBJECT_QUARANTINE_OWNER_FILE), - typeof owner === 'string' ? owner : JSON.stringify(owner) - ) - } const modified = (Date.now() - ageMs) / 1000 utimesSync(dir, modified, modified) return dir } - beforeEach(() => { - vi.spyOn(process, 'kill').mockImplementation((pid) => { - const code = pid === EXITED_PID ? 'ESRCH' : pid === OTHER_USER_PID ? 'EPERM' : undefined - if (code) { - throw Object.assign(new Error(`kill ${code}`), { code }) - } - return true - }) - }) - - const exited = (): GitObjectQuarantineOwner => ({ ...self(), pid: EXITED_PID }) - const elsewhere = (): GitObjectQuarantineOwner => ({ ...self(), hostname: `not-${hostname()}` }) - // Why the other platform: a WSL distro reports the Windows hostname but has its own pids. - const otherPidSpace = (): GitObjectQuarantineOwner => ({ - ...exited(), - platform: process.platform === 'win32' ? 'linux' : 'win32' - }) - const pastGitExpiry = MAX_GIT_OBJECT_QUARANTINE_AGE_MS + HOUR - - it.each<[string, boolean, number, () => GitObjectQuarantineOwner | string | undefined]>([ - ['its owner on this host has exited', true, 2 * HOUR, exited], - ['its owner is this process', false, 2 * HOUR, self], - ['its owner on this host looks alive but it is 15 days old', true, 15 * 24 * HOUR, self], - [ - 'its owner runs as another user', - false, - 2 * HOUR, - () => ({ ...self(), pid: OTHER_USER_PID }) - ], - ['its owner exited under an hour ago', false, STALE_GIT_OBJECT_QUARANTINE_AGE_MS / 2, exited], - [ - 'its owner is on another host', - false, - 2 * 24 * HOUR, - () => ({ ...elsewhere(), pid: EXITED_PID }) - ], - ['its owner shares the hostname but not the pid space', false, 2 * 24 * HOUR, otherPidSpace], - ['its owner is on another host and Git would expire it', true, pastGitExpiry, elsewhere], - ['it has no owner record', false, 2 * 24 * HOUR, () => undefined], - ['its owner record is unreadable', false, 2 * 24 * HOUR, () => '{"pid":'], - ['it has no owner record and Git would expire it', true, pastGitExpiry, () => undefined] - ])('when %s, removes it: %s', async (_label, removed, ageMs, owner) => { - const dir = makeScratch('candidate', ageMs, owner()) + it('removes scratch dirs past Git’s own two-week expiry and keeps younger ones', async () => { + expect(STALE_GIT_OBJECT_QUARANTINE_AGE_MS).toBe(14 * DAY) + const expired = makeDir(`${GIT_OBJECT_QUARANTINE_DIR_PREFIX}old`, 15 * DAY) + const young = makeDir(`${GIT_OBJECT_QUARANTINE_DIR_PREFIX}young`, 13 * DAY) + const gitOwn = makeDir('tmp_objdir-incoming-old', 15 * DAY) await createGitObjectQuarantine(resolveNative()).run(async () => {}) + await _settleGitObjectQuarantineSweepsForTests() - expect(existsSync(dir)).toBe(!removed) + expect(existsSync(expired)).toBe(false) + expect(existsSync(young)).toBe(true) + expect(existsSync(gitOwn)).toBe(true) }) - it('sweeps once per objects dir and never touches Git’s own quarantine dirs', async () => { - const gitOwn = join(objects, 'tmp_objdir-incoming-old') - mkdirSync(gitOwn) - const old = (Date.now() - MAX_GIT_OBJECT_QUARANTINE_AGE_MS - HOUR) / 1000 - utimesSync(gitOwn, old, old) - const stranded = makeScratch('stranded', 2 * HOUR, exited()) + it('sweeps once per objects dir', async () => { + await createGitObjectQuarantine(resolveNative()).run(async () => {}) + await _settleGitObjectQuarantineSweepsForTests() + const expired = makeDir(`${GIT_OBJECT_QUARANTINE_DIR_PREFIX}old`, 15 * DAY) await createGitObjectQuarantine(resolveNative()).run(async () => {}) + await _settleGitObjectQuarantineSweepsForTests() - expect(existsSync(stranded)).toBe(false) - expect(existsSync(gitOwn)).toBe(true) + expect(existsSync(expired)).toBe(true) + }) - makeScratch('stranded', 2 * HOUR, exited()) - await createGitObjectQuarantine(resolveNative()).run(async () => {}) - expect(existsSync(stranded)).toBe(true) + it('does not hold the command back while the sweep runs', async () => { + let releaseSweep = (): void => {} + objectsReaddirGate.current = new Promise((resolve) => { + releaseSweep = resolve + }) + let commandRan = false + try { + const running = createGitObjectQuarantine(resolveNative()).run(async () => { + commandRan = true + }) + await vi.waitFor(() => expect(commandRan).toBe(true)) + releaseSweep() + await running + } finally { + releaseSweep() + objectsReaddirGate.current = undefined + } }) }) }) diff --git a/src/shared/git-object-quarantine.ts b/src/shared/git-object-quarantine.ts index 3c69ffa8ede4..3122eaaf0450 100644 --- a/src/shared/git-object-quarantine.ts +++ b/src/shared/git-object-quarantine.ts @@ -1,6 +1,4 @@ -import { readlinkSync } from 'node:fs' -import { lstat, mkdtemp, readFile, readdir, rename, rm, stat, writeFile } from 'node:fs/promises' -import { hostname } from 'node:os' +import { lstat, mkdtemp, readdir, rename, rm, stat } from 'node:fs/promises' import { posix, win32 } from 'node:path' import { isWindowsAbsolutePathLike } from './cross-platform-path' import { removeTree } from './windows-transient-lock-removal' @@ -33,28 +31,10 @@ export type GitObjectQuarantine = { // `git gc` expires stale `tmp_*` entries too. export const GIT_OBJECT_QUARANTINE_DIR_PREFIX = 'tmp_objdir-orca-merge-tree-' -// Why a floor even for a dead owner: it covers pid spaces the owner record cannot tell apart. -export const STALE_GIT_OBJECT_QUARANTINE_AGE_MS = 60 * 60 * 1000 +// Why two weeks: `git gc` expires its own `objects/tmp_*` dirs at that age, and no merge-tree runs that long. +export const STALE_GIT_OBJECT_QUARANTINE_AGE_MS = 14 * 24 * 60 * 60 * 1000 -// Why a hard cap for every dir, live-looking owner included: a reused pid (a daemon, a shell) -// would otherwise keep a leftover forever in a gc-off repo. Two weeks is `git gc`'s own expiry -// for `objects/tmp_*` (gc.pruneExpire), and no merge-tree runs that long. -export const MAX_GIT_OBJECT_QUARANTINE_AGE_MS = 14 * 24 * 60 * 60 * 1000 - -// Why: Orca processes sharing a repo (a second instance, dev and release builds) sweep each -// other's scratch dirs; only a provably dead owner makes one stale, as Git decides for gc.pid. -export const GIT_OBJECT_QUARANTINE_OWNER_FILE = 'orca-owner.json' - -export type GitObjectQuarantineOwner = { - pid: number - hostname: string - platform: string - /** Linux only: a pid is meaningful only inside the pid namespace that issued it. */ - pidNamespace?: string -} - -const sweptObjectsDirectories = new Set() -let currentOwner: GitObjectQuarantineOwner | undefined +const sweepsByObjectsDirectory = new Map>() /** Decided by path syntax, not by platform: a Windows main process drives WSL Git. */ export function pathApiForGitPath(value: string): typeof posix { @@ -70,111 +50,35 @@ function alternateObjectDirectoriesValue(gitPath: string): string { return `"${gitPath.replace(/[\\"]/g, (char) => `\\${char}`)}"` } -export function gitObjectQuarantineOwner(): GitObjectQuarantineOwner { - if (!currentOwner) { - let pidNamespace: string | undefined - if (process.platform === 'linux') { - try { - pidNamespace = readlinkSync('/proc/self/ns/pid') - } catch {} - } - currentOwner = { - pid: process.pid, - // Why platform too: a WSL distro takes the Windows hostname but has its own pids. - hostname: hostname(), - platform: process.platform, - ...(pidNamespace ? { pidNamespace } : {}) - } - } - return currentOwner -} - -async function readScratchOwner(scratch: string): Promise { - const path = pathApiForGitPath(scratch) - try { - const parsed: unknown = JSON.parse( - await readFile(path.join(scratch, GIT_OBJECT_QUARANTINE_OWNER_FILE), 'utf8') - ) - if ( - typeof parsed === 'object' && - parsed !== null && - 'pid' in parsed && - typeof parsed.pid === 'number' && - Number.isSafeInteger(parsed.pid) && - parsed.pid > 0 && - 'hostname' in parsed && - typeof parsed.hostname === 'string' && - 'platform' in parsed && - typeof parsed.platform === 'string' - ) { - const pidNamespace = - 'pidNamespace' in parsed && typeof parsed.pidNamespace === 'string' - ? parsed.pidNamespace - : undefined - return { - pid: parsed.pid, - hostname: parsed.hostname, - platform: parsed.platform, - ...(pidNamespace ? { pidNamespace } : {}) - } - } - } catch {} - return undefined -} - -function sharesThisPidSpace(owner: GitObjectQuarantineOwner): boolean { - const self = gitObjectQuarantineOwner() - return ( - owner.hostname === self.hostname && - owner.platform === self.platform && - owner.pidNamespace === self.pidNamespace - ) -} - -// Why only ESRCH: EPERM means the pid exists under another user, so a reused pid keeps the dir. -function isProcessAlive(pid: number): boolean { - try { - process.kill(pid, 0) - return true - } catch (error) { - return !(error instanceof Error && 'code' in error && error.code === 'ESRCH') - } -} - -async function isStaleScratchDirectory(scratch: string, now: number): Promise { - const modified = await stat(scratch).then( - (stats) => stats.mtimeMs, - () => undefined - ) - if (modified === undefined || now - modified < STALE_GIT_OBJECT_QUARANTINE_AGE_MS) { - return false - } - if (now - modified >= MAX_GIT_OBJECT_QUARANTINE_AGE_MS) { - return true - } - const owner = await readScratchOwner(scratch) - return owner !== undefined && sharesThisPidSpace(owner) && !isProcessAlive(owner.pid) -} - async function sweepStaleScratchDirectories(objectsHostPath: string): Promise { - if (sweptObjectsDirectories.has(objectsHostPath)) { - return - } - sweptObjectsDirectories.add(objectsHostPath) const path = pathApiForGitPath(objectsHostPath) const entries = await readdir(objectsHostPath).catch(() => []) - const now = Date.now() + const cutoff = Date.now() - STALE_GIT_OBJECT_QUARANTINE_AGE_MS for (const entry of entries) { if (!entry.startsWith(GIT_OBJECT_QUARANTINE_DIR_PREFIX)) { continue } const scratch = path.join(objectsHostPath, entry) - if (await isStaleScratchDirectory(scratch, now)) { + const modified = await stat(scratch).then( + (stats) => stats.mtimeMs, + () => undefined + ) + if (modified !== undefined && modified < cutoff) { await removeTree(scratch).catch(() => {}) } } } +// Why not awaited: cleanup must not delay the user's check, and it only removes dirs far older than this run's. +function startSweepOnce(objectsHostPath: string): void { + if (!sweepsByObjectsDirectory.has(objectsHostPath)) { + sweepsByObjectsDirectory.set( + objectsHostPath, + sweepStaleScratchDirectories(objectsHostPath).catch(() => {}) + ) + } +} + /** * A partial clone fetches missing blobs on demand, and Git files that download * as a pack in the scratch dir. Keep those packs so the next check does not @@ -192,8 +96,10 @@ async function keepFetchedPacks(scratchHostPath: string, objectsHostPath: string if (!files.includes(`${packName}.idx`)) { continue } + // Why no `.keep`: an aborted lazy fetch can leave its transient one, and gc never repacks a kept pack. + const kept = files.filter((file) => !file.endsWith('.keep')) // Why `.idx` last: Git finds a pack through its index, so everything it names must already be in place. - const ordered = [...files.filter((file) => !file.endsWith('.idx')), `${packName}.idx`] + const ordered = [...kept.filter((file) => !file.endsWith('.idx')), `${packName}.idx`] const installed: string[] = [] try { for (const file of ordered) { @@ -234,18 +140,11 @@ export function createGitObjectQuarantine( const objects = await resolveOnce() let scratchHostPath: string | undefined if (objects) { - await sweepStaleScratchDirectories(objects.hostPath) + startSweepOnce(objects.hostPath) const path = pathApiForGitPath(objects.hostPath) scratchHostPath = await mkdtemp( path.join(objects.hostPath, GIT_OBJECT_QUARANTINE_DIR_PREFIX) ).catch(() => undefined) - if (scratchHostPath) { - // Why no fallback on failure: an ownerless dir is kept until the two-week cap. - await writeFile( - path.join(scratchHostPath, GIT_OBJECT_QUARANTINE_OWNER_FILE), - JSON.stringify(gitObjectQuarantineOwner()) - ).catch(() => {}) - } } if (!objects || !scratchHostPath) { // Why: bookkeeping must not block the user's action; run unquarantined. @@ -262,7 +161,7 @@ export function createGitObjectQuarantine( } finally { await keepFetchedPacks(scratchHostPath, objects.hostPath) await removeTree(scratchHostPath).catch((error: unknown) => { - // Why: a later process sweeps it once this one has exited; the check's result still stands. + // Why: the stale sweep removes it later; the check's result still stands. console.warn( '[git-object-quarantine] could not remove scratch dir', scratchHostPath, @@ -275,5 +174,9 @@ export function createGitObjectQuarantine( } export function _resetGitObjectQuarantineSweepForTests(): void { - sweptObjectsDirectories.clear() + sweepsByObjectsDirectory.clear() +} + +export async function _settleGitObjectQuarantineSweepsForTests(): Promise { + await Promise.all(sweepsByObjectsDirectory.values()) } From 3da84cf5f3f3a25d33c9ee5c9d9deaf06ba9204e Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Thu, 1 Oct 2026 00:42:24 -0700 Subject: [PATCH 07/13] fix(git): let quarantined merge-tree see exactly the object stores Git sees without it The scratch object store must not change what merge-tree can read, or the conflict list and the squash-merged branch cleanup differ from main: - An inherited GIT_ALTERNATE_OBJECT_DIRECTORIES (from Orca's or the SSH relay's environment) was replaced; it is now kept and the real store appended after it, as Git's own temporary object stores do. - An inherited GIT_OBJECT_DIRECTORY is the store Git reads and writes, so the command now runs unquarantined. On the WSL route a Windows value only counts if WSLENV forwards it, and then the quarantine is skipped too. - The real store's own info/alternates entries are listed before it, so an alternates chain is linked at the same depth as without the quarantine (a 6-deep chain no longer hits Git's nesting limit). An alternates file that cannot be read or holds a quoted entry runs unquarantined. --- .../git/local-git-object-quarantine.test.ts | 34 +++++ src/main/git/local-git-object-quarantine.ts | 25 +++- ...ge-tree-object-quarantine-real-git.test.ts | 118 +++++++++++++++++- src/relay/relay-git-object-quarantine.test.ts | 42 +++++++ src/relay/relay-git-object-quarantine.ts | 8 +- src/shared/git-object-quarantine.test.ts | 75 +++++++++++ src/shared/git-object-quarantine.ts | 95 ++++++++++++-- 7 files changed, 384 insertions(+), 13 deletions(-) create mode 100644 src/relay/relay-git-object-quarantine.test.ts diff --git a/src/main/git/local-git-object-quarantine.test.ts b/src/main/git/local-git-object-quarantine.test.ts index 0e24bb38e54d..6ee05e5b6bee 100644 --- a/src/main/git/local-git-object-quarantine.test.ts +++ b/src/main/git/local-git-object-quarantine.test.ts @@ -3,6 +3,7 @@ import type * as GitObjectQuarantineModule from '../../shared/git-object-quarant import type { GitObjectQuarantine, GitObjectsDirectory } from '../../shared/git-object-quarantine' import { createLocalGitObjectQuarantine, + inheritedLocalObjectStore, localGitObjectQuarantineProcessEnv, localGitObjectsDirectory } from './local-git-object-quarantine' @@ -142,3 +143,36 @@ describe('createLocalGitObjectQuarantine on a Windows host', () => { await expect(envSeenBy('C:\\wt', { wslDistro: 'Ubuntu' })).resolves.toBeUndefined() }) }) + +describe('inheritedLocalObjectStore', () => { + const ALTERNATES = { GIT_ALTERNATE_OBJECT_DIRECTORIES: '/shared/objects' } + + it('passes native Git’s inherited alternates on and refuses an inherited object dir', () => { + expect(inheritedLocalObjectStore(undefined, ALTERNATES, 'darwin')).toEqual({ + inheritedAlternates: '/shared/objects' + }) + expect( + inheritedLocalObjectStore(undefined, { GIT_OBJECT_DIRECTORY: 'C:\\objects' }, 'win32') + ).toBeUndefined() + }) + + it('ignores Windows values WSLENV does not forward, since WSL Git never saw them', () => { + expect(inheritedLocalObjectStore('Ubuntu', ALTERNATES, 'win32')).toEqual({}) + expect( + inheritedLocalObjectStore('Ubuntu', { GIT_OBJECT_DIRECTORY: 'C:\\objects' }, 'win32') + ).toEqual({}) + }) + + it.each(['GIT_ALTERNATE_OBJECT_DIRECTORIES', 'GIT_OBJECT_DIRECTORY/p'])( + 'runs unquarantined when WSLENV forwards %s to WSL Git', + (token) => { + const env = { + GIT_ALTERNATE_OBJECT_DIRECTORIES: 'C:\\shared', + GIT_OBJECT_DIRECTORY: 'C:\\objects', + WSLENV: `ORCA_X/u:${token}` + } + + expect(inheritedLocalObjectStore('Ubuntu', env, 'win32')).toBeUndefined() + } + ) +}) diff --git a/src/main/git/local-git-object-quarantine.ts b/src/main/git/local-git-object-quarantine.ts index f0addb82ff83..69b9fcdbc445 100644 --- a/src/main/git/local-git-object-quarantine.ts +++ b/src/main/git/local-git-object-quarantine.ts @@ -2,6 +2,7 @@ import { isWindowsAbsolutePathLike } from '../../shared/cross-platform-path' import type { GitAdmissionTier } from '../../shared/rpc-contract/git-admission-tier-params' import { createGitObjectQuarantine, + inheritedObjectStore, pathApiForGitPath, type GitObjectQuarantineEnv, type GitObjectsDirectory @@ -47,6 +48,24 @@ export function localGitObjectQuarantineProcessEnv( return env } +const OBJECT_STORE_VARIABLES = ['GIT_OBJECT_DIRECTORY', 'GIT_ALTERNATE_OBJECT_DIRECTORIES'] + +/** The object-store variables local Git would inherit, or undefined when they cannot be mirrored. */ +export function inheritedLocalObjectStore( + wslDistro: string | undefined, + env: NodeJS.ProcessEnv = process.env, + platform: NodeJS.Platform = process.platform +): Pick | undefined { + if (platform !== 'win32' || !wslDistro) { + return inheritedObjectStore(env) + } + // Why: wsl.exe forwards only what WSLENV names, maybe path-translated (`/p`); skip rather than guess. + const forwarded = new Set((env.WSLENV ?? '').split(':').map((token) => token.split('/')[0])) + return OBJECT_STORE_VARIABLES.some((key) => forwarded.has(key) && env[key] !== undefined) + ? undefined + : {} +} + /** Scratch object store for throwaway Git writes on the local (native or WSL) host. */ export function createLocalGitObjectQuarantine( repoPath: string, @@ -54,6 +73,10 @@ export function createLocalGitObjectQuarantine( ): LocalGitObjectQuarantine { const wslDistro = parseWslPath(repoPath)?.distro ?? options.wslDistro const quarantine = createGitObjectQuarantine(async () => { + const inherited = inheritedLocalObjectStore(wslDistro) + if (!inherited) { + return undefined + } const commonDir = await readRepoCommonDirFromGit(repoPath, { ...(options.wslDistro ? { wslDistro: options.wslDistro } : {}), ...(options.signal ? { signal: options.signal } : {}), @@ -64,7 +87,7 @@ export function createLocalGitObjectQuarantine( if (!commonDir || usesHostGitForWslLinkedWorktree(repoPath, options.wslDistro)) { return undefined } - return localGitObjectsDirectory(commonDir, wslDistro) + return { ...localGitObjectsDirectory(commonDir, wslDistro), ...inherited } }) return { run: (command) => diff --git a/src/main/git/merge-tree-object-quarantine-real-git.test.ts b/src/main/git/merge-tree-object-quarantine-real-git.test.ts index 0cc3ade038c5..629f9e69a117 100644 --- a/src/main/git/merge-tree-object-quarantine-real-git.test.ts +++ b/src/main/git/merge-tree-object-quarantine-real-git.test.ts @@ -1,5 +1,6 @@ -import { existsSync } from 'node:fs' -import { afterEach, beforeEach, describe, expect, it } from 'vitest' +import { existsSync, readdirSync, writeFileSync } from 'node:fs' +import { dirname, join } from 'node:path' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { createDivergentRepoFixture, type DivergentRepoFixture @@ -164,3 +165,116 @@ describe('merge-tree runs against a scratch object store (real Git)', () => { expect(existsSync(scratch ?? '')).toBe(false) }) }) + +// Why: the quarantine must leave Git seeing exactly the object stores it sees without it. +describe('merge-tree quarantine keeps the object stores Git would otherwise see (real Git)', () => { + let fixture: DivergentRepoFixture + + beforeEach(() => { + __resetPRConflictSummaryCachesForTests() + fixture = createDivergentRepoFixture() + }) + + afterEach(() => { + vi.unstubAllEnvs() + fixture.dispose() + }) + + const BRANCHES = ['main', 'feature-conflict', 'feature-extra', 'feature-squashed'] + const fixtureHead = (branch: string): string => + fixture.git(fixture.repoPath, 'rev-parse', branch).trim() + + function emptyRepo(name: string): string { + const repo = join(dirname(fixture.repoPath), name) + fixture.git(dirname(fixture.repoPath), 'init', '--quiet', repo) + return repo + } + + /** Points `repo`'s branches at the fixture's commits; their objects must already be reachable. */ + function addFixtureBranches(repo: string): void { + for (const branch of BRANCHES) { + fixture.git(repo, 'update-ref', `refs/heads/${branch}`, fixtureHead(branch)) + } + fixture.git(repo, 'symbolic-ref', 'HEAD', 'refs/heads/main') + fixture.git(repo, 'config', 'branch.feature-extra.base', 'refs/heads/main') + fixture.git(repo, 'config', 'branch.feature-squashed.base', 'refs/heads/main') + } + + const looseObjectsIn = (repo: string): number => { + const objects = join(repo, '.git', 'objects') + return readdirSync(objects) + .filter((entry) => /^[0-9a-f]{2}$/.test(entry)) + .reduce((count, entry) => count + readdirSync(join(objects, entry)).length, 0) + } + + async function expectUnquarantinedVerdicts(repo: string): Promise { + const summary = await getPRConflictSummary( + repo, + 'main', + fixtureHead('main'), + fixtureHead('feature-conflict') + ) + const extra = await deleteBranchAfterWorktreeRemoval( + repo, + 'feature-extra', + fixtureHead('feature-extra'), + {} + ) + const squashed = await deleteBranchAfterWorktreeRemoval( + repo, + 'feature-squashed', + fixtureHead('feature-squashed'), + {} + ) + + expect(summary?.files).toEqual(['shared.txt']) + expect(extra).toEqual({ + preservedBranch: { branchName: 'feature-extra', head: fixtureHead('feature-extra') } + }) + expect(squashed).toEqual({}) + expect(fixture.git(repo, 'branch', '--list', 'feature-squashed')).toBe('') + } + + it('reads objects reachable only through inherited GIT_ALTERNATE_OBJECT_DIRECTORIES', async () => { + const repo = emptyRepo('reached-by-env-alternates') + vi.stubEnv('GIT_ALTERNATE_OBJECT_DIRECTORIES', join(fixture.commonDir, 'objects')) + addFixtureBranches(repo) + + await expectUnquarantinedVerdicts(repo) + + expect(looseObjectsIn(repo)).toBe(0) + }) + + it('leaves an inherited GIT_OBJECT_DIRECTORY in charge, writing where Git would', async () => { + const repo = emptyRepo('inherited-object-dir') + vi.stubEnv('GIT_OBJECT_DIRECTORY', join(fixture.commonDir, 'objects')) + addFixtureBranches(repo) + const before = fixture.looseObjectCount() + + await expectUnquarantinedVerdicts(repo) + + // Unquarantined, Git writes the merge results into the store it was handed. + expect(fixture.looseObjectCount()).toBeGreaterThan(before) + expect(fixture.scratchDirectories()).toEqual([]) + }) + + it('reads through an info/alternates chain at the deepest nesting Git accepts', async () => { + let linked = join(fixture.commonDir, 'objects') + let repo = '' + for (let depth = 1; depth <= 6; depth++) { + repo = emptyRepo(`alternates-chain-${depth}`) + writeFileSync(join(repo, '.git', 'objects', 'info', 'alternates'), `${linked}\n`) + linked = join(repo, '.git', 'objects') + } + addFixtureBranches(repo) + + await expectUnquarantinedVerdicts(repo) + + expect(looseObjectsIn(repo)).toBe(0) + // Without the quarantine Git reaches the bottom of this chain too, and no deeper. + const squashed = fixtureHead('feature-squashed') + expect(fixture.git(repo, 'merge-tree', '--write-tree', 'main', squashed).trim()).toBe( + fixture.git(repo, 'rev-parse', 'main^{tree}').trim() + ) + }) +}) diff --git a/src/relay/relay-git-object-quarantine.test.ts b/src/relay/relay-git-object-quarantine.test.ts new file mode 100644 index 000000000000..e966df70ab09 --- /dev/null +++ b/src/relay/relay-git-object-quarantine.test.ts @@ -0,0 +1,42 @@ +import { mkdirSync, mkdtempSync, realpathSync, rmSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { _settleGitObjectQuarantineSweepsForTests } from '../shared/git-object-quarantine' +import type { GitExec } from './git-handler-ops' +import { createRelayGitObjectQuarantine } from './relay-git-object-quarantine' + +describe('createRelayGitObjectQuarantine', () => { + let root: string + let git: GitExec + + beforeEach(() => { + root = realpathSync(mkdtempSync(join(tmpdir(), 'orca-relay-quarantine-'))) + mkdirSync(join(root, '.git', 'objects'), { recursive: true }) + git = vi.fn(async () => ({ stdout: '.git\n', stderr: '' })) + }) + + afterEach(async () => { + await _settleGitObjectQuarantineSweepsForTests() + vi.unstubAllEnvs() + rmSync(root, { recursive: true, force: true }) + }) + + const envSeenByGit = () => createRelayGitObjectQuarantine(git, root).run(async (env) => env) + + it('appends the real store to alternates the relay process hands Git', async () => { + vi.stubEnv('GIT_ALTERNATE_OBJECT_DIRECTORIES', '/shared/objects') + + const env = await envSeenByGit() + + expect(env?.GIT_ALTERNATE_OBJECT_DIRECTORIES).toBe( + `/shared/objects:${join(root, '.git', 'objects')}` + ) + }) + + it('runs unquarantined when the relay process hands Git its own object dir', async () => { + vi.stubEnv('GIT_OBJECT_DIRECTORY', '/elsewhere/objects') + + await expect(envSeenByGit()).resolves.toBeUndefined() + }) +}) diff --git a/src/relay/relay-git-object-quarantine.ts b/src/relay/relay-git-object-quarantine.ts index 1d828d94a0ba..74279bb99daa 100644 --- a/src/relay/relay-git-object-quarantine.ts +++ b/src/relay/relay-git-object-quarantine.ts @@ -1,6 +1,7 @@ import { resolve } from 'node:path' import { createGitObjectQuarantine, + inheritedObjectStore, type GitObjectQuarantine } from '../shared/git-object-quarantine' import { expandTilde } from './context' @@ -12,6 +13,11 @@ export function createRelayGitObjectQuarantine( repoPath: string ): GitObjectQuarantine { return createGitObjectQuarantine(async () => { + // Why the relay's own env: relay Git inherits it unchanged (buildRelayGitEnv). + const inherited = inheritedObjectStore(process.env) + if (!inherited) { + return undefined + } const { stdout } = await git(['rev-parse', '--git-common-dir'], repoPath) const commonDir = stdout.replace(/\r?\n$/, '') if (!commonDir) { @@ -19,6 +25,6 @@ export function createRelayGitObjectQuarantine( } // Why: Git 2.25 may print the common dir relative to the command's cwd. const objects = resolve(expandTilde(repoPath), commonDir, 'objects') - return { hostPath: objects, gitPath: objects } + return { hostPath: objects, gitPath: objects, ...inherited } }) } diff --git a/src/shared/git-object-quarantine.test.ts b/src/shared/git-object-quarantine.test.ts index d84d9ca8e7bb..083e9236f991 100644 --- a/src/shared/git-object-quarantine.test.ts +++ b/src/shared/git-object-quarantine.test.ts @@ -17,6 +17,7 @@ import { _settleGitObjectQuarantineSweepsForTests, createGitObjectQuarantine, GIT_OBJECT_QUARANTINE_DIR_PREFIX, + inheritedObjectStore, STALE_GIT_OBJECT_QUARANTINE_AGE_MS, type GitObjectQuarantineEnv } from './git-object-quarantine' @@ -166,6 +167,67 @@ describe('createGitObjectQuarantine', () => { expect(seen?.GIT_ALTERNATE_OBJECT_DIRECTORIES).toBe(expected) }) + it.each([ + ['/repo/.git/objects', '/shared/objects:/more', '/shared/objects:/more:/repo/.git/objects'], + ['C:\\repo\\.git\\objects', 'D:\\shared', 'D:\\shared;C:\\repo\\.git\\objects'] + ])( + 'appends the real store to inherited alternates for %s, as Git’s own temporary stores do', + async (gitPath, inheritedAlternates, expected) => { + let seen: GitObjectQuarantineEnv | undefined + + await createGitObjectQuarantine(async () => ({ + hostPath: objects, + gitPath, + inheritedAlternates + })).run(async (env) => { + seen = env + }) + + expect(seen?.GIT_ALTERNATE_OBJECT_DIRECTORIES).toBe(expected) + } + ) + + it('lists the real store’s own alternates before it, resolved the way Git resolves them', async () => { + mkdirSync(join(objects, 'info')) + writeFileSync( + join(objects, 'info', 'alternates'), + '# a comment\n\n/abs/one/objects\n../../sibling/.git/objects\n/odd:dir/objects\n' + ) + let seen: GitObjectQuarantineEnv | undefined + + await createGitObjectQuarantine(async () => ({ + hostPath: objects, + gitPath: '/repo/.git/objects', + inheritedAlternates: '/inherited' + })).run(async (env) => { + seen = env + }) + + expect(seen?.GIT_ALTERNATE_OBJECT_DIRECTORIES).toBe( + [ + '/inherited', + '/abs/one/objects', + '/repo/.git/objects/../../sibling/.git/objects', + '"/odd:dir/objects"', + '/repo/.git/objects' + ].join(':') + ) + }) + + it.each([ + ['holds a C-quoted entry', (info: string) => writeFileSync(join(info, 'alternates'), '"/q"\n')], + ['cannot be read', (info: string) => mkdirSync(join(info, 'alternates'))] + ])('runs unquarantined when the real store’s alternates file %s', async (_label, prepare) => { + mkdirSync(join(objects, 'info')) + prepare(join(objects, 'info')) + const command = vi.fn(async () => 'ok') + + await expect(createGitObjectQuarantine(resolveNative()).run(command)).resolves.toBe('ok') + + expect(command).toHaveBeenCalledWith(undefined) + expect(scratchDirs()).toEqual([]) + }) + it('moves packs Git fetched into the scratch dir to the real store, index included', async () => { await createGitObjectQuarantine(resolveNative()).run(async (env) => { const scratchPack = join(env?.GIT_OBJECT_DIRECTORY ?? '', 'pack') @@ -286,3 +348,16 @@ describe('createGitObjectQuarantine', () => { }) }) }) + +describe('inheritedObjectStore', () => { + it('passes inherited alternates on, so the quarantine can append to them', () => { + expect(inheritedObjectStore({ GIT_ALTERNATE_OBJECT_DIRECTORIES: '/shared' })).toEqual({ + inheritedAlternates: '/shared' + }) + expect(inheritedObjectStore({})).toEqual({}) + }) + + it('refuses an inherited object dir, which is the store Git reads and writes', () => { + expect(inheritedObjectStore({ GIT_OBJECT_DIRECTORY: '/elsewhere/objects' })).toBeUndefined() + }) +}) diff --git a/src/shared/git-object-quarantine.ts b/src/shared/git-object-quarantine.ts index 3122eaaf0450..86a952948f36 100644 --- a/src/shared/git-object-quarantine.ts +++ b/src/shared/git-object-quarantine.ts @@ -1,4 +1,4 @@ -import { lstat, mkdtemp, readdir, rename, rm, stat } from 'node:fs/promises' +import { lstat, mkdtemp, readdir, readFile, rename, rm, stat } from 'node:fs/promises' import { posix, win32 } from 'node:path' import { isWindowsAbsolutePathLike } from './cross-platform-path' import { removeTree } from './windows-transient-lock-removal' @@ -20,6 +20,8 @@ export type GitObjectsDirectory = { hostPath: string /** How the Git child spells the same directory (differs for WSL). */ gitPath: string + /** GIT_ALTERNATE_OBJECT_DIRECTORIES as Git would inherit it unquarantined. */ + inheritedAlternates?: string } export type GitObjectQuarantine = { @@ -42,12 +44,86 @@ export function pathApiForGitPath(value: string): typeof posix { } // Why: Git splits this variable on `:` (`;` for Git for Windows) and C-unquotes a leading `"`. -function alternateObjectDirectoriesValue(gitPath: string): string { - const needsQuoting = isWindowsAbsolutePathLike(gitPath) ? /[;"]/ : /[:"\\]/ - if (!needsQuoting.test(gitPath)) { - return gitPath +function quoteAlternate(path: string, windowsGit: boolean): string { + const needsQuoting = windowsGit ? /[;"]/ : /[:"\\]/ + if (!needsQuoting.test(path)) { + return path } - return `"${gitPath.replace(/[\\"]/g, (char) => `\\${char}`)}"` + return `"${path.replace(/[\\"]/g, (char) => `\\${char}`)}"` +} + +/** + * Which inherited object-store variables a quarantine can mirror, as `GitObjectsDirectory` fields; + * undefined when it cannot: an inherited GIT_OBJECT_DIRECTORY is the store Git reads and writes. + */ +export function inheritedObjectStore( + env: Record +): Pick | undefined { + if (env.GIT_OBJECT_DIRECTORY !== undefined) { + return undefined + } + const alternates = env.GIT_ALTERNATE_OBJECT_DIRECTORIES + return alternates ? { inheritedAlternates: alternates } : {} +} + +function isMissingFileError(error: unknown): boolean { + return ( + error instanceof Error && + 'code' in error && + (error.code === 'ENOENT' || error.code === 'ENOTDIR') + ) +} + +/** The real store's `info/alternates` entries as Git resolves them, or undefined when they cannot be mirrored. */ +async function readRealStoreAlternates( + objects: GitObjectsDirectory +): Promise { + let text: string + try { + const bytes = await readFile( + pathApiForGitPath(objects.hostPath).join(objects.hostPath, 'info', 'alternates') + ) + text = new TextDecoder('utf-8', { fatal: true, ignoreBOM: true }).decode(bytes) + } catch (error) { + return isMissingFileError(error) ? [] : undefined + } + const windowsGit = isWindowsAbsolutePathLike(objects.gitPath) + const entries: string[] = [] + for (const line of text.split('\n')) { + if (!line || line.startsWith('#')) { + continue + } + // Why: Git C-unquotes such a line; running unquarantined beats reimplementing that. + if (line.startsWith('"')) { + return undefined + } + const absolute = windowsGit + ? isWindowsAbsolutePathLike(line) || /^[\\/]/.test(line) + : line.startsWith('/') + // Why not normalized: Git appends a relative entry to the store's path and then resolves symlinks. + entries.push(absolute ? line : `${objects.gitPath}/${line}`) + } + return entries +} + +/** + * The alternates that let a quarantined Git see exactly what it sees unquarantined, or undefined. + * Inherited value first, as Git's own temporary object dirs append. The real store's own alternates + * go before the real store: Git links an alternate's alternates one level deeper, and listing them + * directly keeps a chain at the nesting depth Git allows without the quarantine. + */ +async function quarantineAlternates(objects: GitObjectsDirectory): Promise { + const realAlternates = await readRealStoreAlternates(objects) + if (!realAlternates) { + return undefined + } + const windowsGit = isWindowsAbsolutePathLike(objects.gitPath) + const entries = [...realAlternates, objects.gitPath].map((entry) => + quoteAlternate(entry, windowsGit) + ) + return [...(objects.inheritedAlternates ? [objects.inheritedAlternates] : []), ...entries].join( + windowsGit ? ';' : ':' + ) } async function sweepStaleScratchDirectories(objectsHostPath: string): Promise { @@ -138,15 +214,16 @@ export function createGitObjectQuarantine( return { async run(command) { const objects = await resolveOnce() + const alternates = objects ? await quarantineAlternates(objects) : undefined let scratchHostPath: string | undefined - if (objects) { + if (objects && alternates !== undefined) { startSweepOnce(objects.hostPath) const path = pathApiForGitPath(objects.hostPath) scratchHostPath = await mkdtemp( path.join(objects.hostPath, GIT_OBJECT_QUARANTINE_DIR_PREFIX) ).catch(() => undefined) } - if (!objects || !scratchHostPath) { + if (!objects || alternates === undefined || !scratchHostPath) { // Why: bookkeeping must not block the user's action; run unquarantined. return command(undefined) } @@ -156,7 +233,7 @@ export function createGitObjectQuarantine( objects.gitPath, pathApiForGitPath(scratchHostPath).basename(scratchHostPath) ), - GIT_ALTERNATE_OBJECT_DIRECTORIES: alternateObjectDirectoriesValue(objects.gitPath) + GIT_ALTERNATE_OBJECT_DIRECTORIES: alternates }) } finally { await keepFetchedPacks(scratchHostPath, objects.hostPath) From 3a96c33b017d588e521c881e0dc3526c2f25b204 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Thu, 1 Oct 2026 01:28:20 -0700 Subject: [PATCH 08/13] fix(git): run merge-tree unquarantined when the repo's store has its own alternates Mirroring Git's alternates resolution could not match it in every shape: a path back to the real store (mutual alternates, or an inherited alternates value naming the repo itself) still linked its chain one level deeper, and a NUL byte in info/alternates broke the spawn. When objects/info/alternates exists, or cannot be checked, the command now runs exactly as before. Repos cloned with --reference or --shared keep writing throwaway objects, as they always have. Inherited alternates are still appended to, and an inherited GIT_OBJECT_DIRECTORY still skips the quarantine. --- .../git/local-git-object-quarantine.test.ts | 7 ++ ...ge-tree-object-quarantine-real-git.test.ts | 8 +-- src/shared/git-object-quarantine.test.ts | 67 ++++++++++--------- src/shared/git-object-quarantine.ts | 63 +++++------------ 4 files changed, 61 insertions(+), 84 deletions(-) diff --git a/src/main/git/local-git-object-quarantine.test.ts b/src/main/git/local-git-object-quarantine.test.ts index 6ee05e5b6bee..f6101899e8ad 100644 --- a/src/main/git/local-git-object-quarantine.test.ts +++ b/src/main/git/local-git-object-quarantine.test.ts @@ -158,6 +158,13 @@ describe('inheritedLocalObjectStore', () => { it('ignores Windows values WSLENV does not forward, since WSL Git never saw them', () => { expect(inheritedLocalObjectStore('Ubuntu', ALTERNATES, 'win32')).toEqual({}) + expect( + inheritedLocalObjectStore( + 'Ubuntu', + { WSLENV: 'GIT_OBJECT_DIRECTORY/p:GIT_ALTERNATE_OBJECT_DIRECTORIES' }, + 'win32' + ) + ).toEqual({}) expect( inheritedLocalObjectStore('Ubuntu', { GIT_OBJECT_DIRECTORY: 'C:\\objects' }, 'win32') ).toEqual({}) diff --git a/src/main/git/merge-tree-object-quarantine-real-git.test.ts b/src/main/git/merge-tree-object-quarantine-real-git.test.ts index 629f9e69a117..07d583e6967c 100644 --- a/src/main/git/merge-tree-object-quarantine-real-git.test.ts +++ b/src/main/git/merge-tree-object-quarantine-real-git.test.ts @@ -270,11 +270,7 @@ describe('merge-tree quarantine keeps the object stores Git would otherwise see await expectUnquarantinedVerdicts(repo) - expect(looseObjectsIn(repo)).toBe(0) - // Without the quarantine Git reaches the bottom of this chain too, and no deeper. - const squashed = fixtureHead('feature-squashed') - expect(fixture.git(repo, 'merge-tree', '--write-tree', 'main', squashed).trim()).toBe( - fixture.git(repo, 'rev-parse', 'main^{tree}').trim() - ) + // A store with its own alternates runs unquarantined, so Git writes where it always has. + expect(looseObjectsIn(repo)).toBeGreaterThan(0) }) }) diff --git a/src/shared/git-object-quarantine.test.ts b/src/shared/git-object-quarantine.test.ts index 083e9236f991..35be08ca15cc 100644 --- a/src/shared/git-object-quarantine.test.ts +++ b/src/shared/git-object-quarantine.test.ts @@ -1,4 +1,5 @@ import { + chmodSync, existsSync, mkdirSync, mkdtempSync, @@ -169,7 +170,12 @@ describe('createGitObjectQuarantine', () => { it.each([ ['/repo/.git/objects', '/shared/objects:/more', '/shared/objects:/more:/repo/.git/objects'], - ['C:\\repo\\.git\\objects', 'D:\\shared', 'D:\\shared;C:\\repo\\.git\\objects'] + ['C:\\repo\\.git\\objects', 'D:\\shared', 'D:\\shared;C:\\repo\\.git\\objects'], + [ + '\\\\server\\share\\repo\\.git\\objects', + '\\\\server\\shared', + '\\\\server\\shared;\\\\server\\share\\repo\\.git\\objects' + ] ])( 'appends the real store to inherited alternates for %s, as Git’s own temporary stores do', async (gitPath, inheritedAlternates, expected) => { @@ -187,45 +193,40 @@ describe('createGitObjectQuarantine', () => { } ) - it('lists the real store’s own alternates before it, resolved the way Git resolves them', async () => { - mkdirSync(join(objects, 'info')) - writeFileSync( - join(objects, 'info', 'alternates'), - '# a comment\n\n/abs/one/objects\n../../sibling/.git/objects\n/odd:dir/objects\n' - ) - let seen: GitObjectQuarantineEnv | undefined + it.each([ + ['has an alternates file', (info: string) => writeFileSync(join(info, 'alternates'), '')], + [ + 'has an alternates file this process cannot inspect', + (info: string) => { + writeFileSync(join(info, 'alternates'), '/elsewhere/objects\n') + chmodSync(info, 0o000) + } + ] + ])('runs unquarantined when the real store %s', async (_label, prepare) => { + const info = join(objects, 'info') + mkdirSync(info) + prepare(info) + const command = vi.fn(async () => 'ok') - await createGitObjectQuarantine(async () => ({ - hostPath: objects, - gitPath: '/repo/.git/objects', - inheritedAlternates: '/inherited' - })).run(async (env) => { - seen = env - }) + try { + await expect(createGitObjectQuarantine(resolveNative()).run(command)).resolves.toBe('ok') + } finally { + chmodSync(info, 0o755) + } - expect(seen?.GIT_ALTERNATE_OBJECT_DIRECTORIES).toBe( - [ - '/inherited', - '/abs/one/objects', - '/repo/.git/objects/../../sibling/.git/objects', - '"/odd:dir/objects"', - '/repo/.git/objects' - ].join(':') - ) + expect(command).toHaveBeenCalledWith(undefined) + expect(scratchDirs()).toEqual([]) }) - it.each([ - ['holds a C-quoted entry', (info: string) => writeFileSync(join(info, 'alternates'), '"/q"\n')], - ['cannot be read', (info: string) => mkdirSync(join(info, 'alternates'))] - ])('runs unquarantined when the real store’s alternates file %s', async (_label, prepare) => { - mkdirSync(join(objects, 'info')) - prepare(join(objects, 'info')) + it('quarantines a store whose info/ holds no alternates file', async () => { + writeFileSync(join(objects, 'info'), 'not a directory') const command = vi.fn(async () => 'ok') - await expect(createGitObjectQuarantine(resolveNative()).run(command)).resolves.toBe('ok') + await createGitObjectQuarantine(resolveNative()).run(command) - expect(command).toHaveBeenCalledWith(undefined) - expect(scratchDirs()).toEqual([]) + expect(command).toHaveBeenCalledWith( + expect.objectContaining({ GIT_ALTERNATE_OBJECT_DIRECTORIES: objects }) + ) }) it('moves packs Git fetched into the scratch dir to the real store, index included', async () => { diff --git a/src/shared/git-object-quarantine.ts b/src/shared/git-object-quarantine.ts index 86a952948f36..80b779424cdf 100644 --- a/src/shared/git-object-quarantine.ts +++ b/src/shared/git-object-quarantine.ts @@ -1,4 +1,4 @@ -import { lstat, mkdtemp, readdir, readFile, rename, rm, stat } from 'node:fs/promises' +import { lstat, mkdtemp, readdir, rename, rm, stat } from 'node:fs/promises' import { posix, win32 } from 'node:path' import { isWindowsAbsolutePathLike } from './cross-platform-path' import { removeTree } from './windows-transient-lock-removal' @@ -74,56 +74,29 @@ function isMissingFileError(error: unknown): boolean { ) } -/** The real store's `info/alternates` entries as Git resolves them, or undefined when they cannot be mirrored. */ -async function readRealStoreAlternates( - objects: GitObjectsDirectory -): Promise { - let text: string - try { - const bytes = await readFile( - pathApiForGitPath(objects.hostPath).join(objects.hostPath, 'info', 'alternates') - ) - text = new TextDecoder('utf-8', { fatal: true, ignoreBOM: true }).decode(bytes) - } catch (error) { - return isMissingFileError(error) ? [] : undefined - } - const windowsGit = isWindowsAbsolutePathLike(objects.gitPath) - const entries: string[] = [] - for (const line of text.split('\n')) { - if (!line || line.startsWith('#')) { - continue - } - // Why: Git C-unquotes such a line; running unquarantined beats reimplementing that. - if (line.startsWith('"')) { - return undefined - } - const absolute = windowsGit - ? isWindowsAbsolutePathLike(line) || /^[\\/]/.test(line) - : line.startsWith('/') - // Why not normalized: Git appends a relative entry to the store's path and then resolves symlinks. - entries.push(absolute ? line : `${objects.gitPath}/${line}`) - } - return entries -} - /** - * The alternates that let a quarantined Git see exactly what it sees unquarantined, or undefined. - * Inherited value first, as Git's own temporary object dirs append. The real store's own alternates - * go before the real store: Git links an alternate's alternates one level deeper, and listing them - * directly keeps a chain at the nesting depth Git allows without the quarantine. + * The alternates for a quarantined run, or undefined to run unquarantined. Inherited value first, + * as Git's own temporary object dirs append. A real store with its own alternates is not + * quarantined: as an alternate instead of the primary, its chain would link one level deeper. */ async function quarantineAlternates(objects: GitObjectsDirectory): Promise { - const realAlternates = await readRealStoreAlternates(objects) - if (!realAlternates) { + const alternatesFile = pathApiForGitPath(objects.hostPath).join( + objects.hostPath, + 'info', + 'alternates' + ) + const hasAlternatesFile = await stat(alternatesFile).then( + () => true, + (error: unknown) => !isMissingFileError(error) + ) + if (hasAlternatesFile) { return undefined } const windowsGit = isWindowsAbsolutePathLike(objects.gitPath) - const entries = [...realAlternates, objects.gitPath].map((entry) => - quoteAlternate(entry, windowsGit) - ) - return [...(objects.inheritedAlternates ? [objects.inheritedAlternates] : []), ...entries].join( - windowsGit ? ';' : ':' - ) + const realStore = quoteAlternate(objects.gitPath, windowsGit) + return objects.inheritedAlternates + ? `${objects.inheritedAlternates}${windowsGit ? ';' : ':'}${realStore}` + : realStore } async function sweepStaleScratchDirectories(objectsHostPath: string): Promise { From 14c3c5039957efc233adde1ccb16de5d06b29b29 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Fri, 2 Oct 2026 14:18:33 -0700 Subject: [PATCH 09/13] refactor(git): drop the stale-folder sweep and pack-move rollback from merge-tree's scratch store A scratch folder left by a crash is a tmp_* dir inside objects/, which Git's own gc prunes after gc.pruneExpire, as it does Git's own leftovers, so Orca's 14-day sweep and its test hooks go. Packs a partial clone fetched into the scratch store are moved in Git's migration order: pack- files only, index last, existing names left in place, stopping at the first failed rename with no rollback. A failed move is logged and never stops the scratch dir's removal or the result. Adds a real-Git test that a partial clone lists its conflicts offline once an earlier check downloaded the contents. Co-Authored-By: Claude --- ...ge-tree-object-quarantine-real-git.test.ts | 16 +++ src/relay/relay-git-object-quarantine.test.ts | 4 +- src/shared/git-object-quarantine.test.ts | 133 +++++------------- src/shared/git-object-quarantine.ts | 104 ++++---------- 4 files changed, 74 insertions(+), 183 deletions(-) diff --git a/src/main/git/merge-tree-object-quarantine-real-git.test.ts b/src/main/git/merge-tree-object-quarantine-real-git.test.ts index 07d583e6967c..b233dc9a2ef4 100644 --- a/src/main/git/merge-tree-object-quarantine-real-git.test.ts +++ b/src/main/git/merge-tree-object-quarantine-real-git.test.ts @@ -123,6 +123,22 @@ describe('merge-tree runs against a scratch object store (real Git)', () => { expect(clone.scratchDirectories()).toEqual([]) }) + it('lists a partial clone’s conflicts offline once an earlier check downloaded the contents', async () => { + const clone = fixture.createPartialClone() + const git = (...args: string[]): string => fixture.git(clone.clonePath, ...args).trim() + const base = git('rev-parse', 'origin/main') + const head = git('rev-parse', 'origin/feature-conflict') + + const online = await getPRConflictSummary(clone.clonePath, 'main', base, head) + git('remote', 'set-url', 'origin', join(dirname(clone.clonePath), 'unreachable-origin')) + __resetPRConflictSummaryCachesForTests() + const offline = await getPRConflictSummary(clone.clonePath, 'main', base, head) + + expect(online?.files).toEqual(['shared.txt']) + expect(offline?.files).toEqual(['shared.txt']) + expect(clone.scratchDirectories()).toEqual([]) + }) + it('removes the scratch directory when the Git command fails', async () => { const quarantine = createLocalGitObjectQuarantine(fixture.linkedPath) let scratch: string | undefined diff --git a/src/relay/relay-git-object-quarantine.test.ts b/src/relay/relay-git-object-quarantine.test.ts index e966df70ab09..c7a7a16baa4a 100644 --- a/src/relay/relay-git-object-quarantine.test.ts +++ b/src/relay/relay-git-object-quarantine.test.ts @@ -2,7 +2,6 @@ import { mkdirSync, mkdtempSync, realpathSync, rmSync } from 'node:fs' import { tmpdir } from 'node:os' import { join } from 'node:path' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' -import { _settleGitObjectQuarantineSweepsForTests } from '../shared/git-object-quarantine' import type { GitExec } from './git-handler-ops' import { createRelayGitObjectQuarantine } from './relay-git-object-quarantine' @@ -16,8 +15,7 @@ describe('createRelayGitObjectQuarantine', () => { git = vi.fn(async () => ({ stdout: '.git\n', stderr: '' })) }) - afterEach(async () => { - await _settleGitObjectQuarantineSweepsForTests() + afterEach(() => { vi.unstubAllEnvs() rmSync(root, { recursive: true, force: true }) }) diff --git a/src/shared/git-object-quarantine.test.ts b/src/shared/git-object-quarantine.test.ts index 35be08ca15cc..bccacc5dfaaa 100644 --- a/src/shared/git-object-quarantine.test.ts +++ b/src/shared/git-object-quarantine.test.ts @@ -6,7 +6,6 @@ import { readdirSync, readFileSync, rmSync, - utimesSync, writeFileSync } from 'node:fs' import type * as NodeFsPromises from 'node:fs/promises' @@ -14,19 +13,15 @@ import { tmpdir } from 'node:os' import { basename, join } from 'node:path' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { - _resetGitObjectQuarantineSweepForTests, - _settleGitObjectQuarantineSweepsForTests, createGitObjectQuarantine, GIT_OBJECT_QUARANTINE_DIR_PREFIX, inheritedObjectStore, - STALE_GIT_OBJECT_QUARANTINE_AGE_MS, type GitObjectQuarantineEnv } from './git-object-quarantine' -const { failedRenameTargets, objectsReaddirGate } = vi.hoisted(() => { - // Why: holds the stale sweep's `objects/` listing so a test can show the command does not wait for it. - const objectsReaddirGate: { current: Promise | undefined } = { current: undefined } - return { failedRenameTargets: new Set(), objectsReaddirGate } +const { failedRenameTargets, renamedTargets } = vi.hoisted(() => { + const renamedTargets: string[] = [] + return { failedRenameTargets: new Set(), renamedTargets } }) vi.mock('node:fs/promises', async (importOriginal) => { @@ -37,13 +32,8 @@ vi.mock('node:fs/promises', async (importOriginal) => { if (failedRenameTargets.has(basename(to))) { throw Object.assign(new Error('EBUSY: resource busy or locked'), { code: 'EBUSY' }) } - return actual.rename(from, to) - }, - readdir: async (path: string) => { - if (basename(path) === 'objects') { - await objectsReaddirGate.current - } - return actual.readdir(path) + await actual.rename(from, to) + renamedTargets.push(basename(to)) } } }) @@ -53,15 +43,14 @@ describe('createGitObjectQuarantine', () => { let objects: string beforeEach(() => { - _resetGitObjectQuarantineSweepForTests() root = mkdtempSync(join(tmpdir(), 'orca-object-quarantine-')) objects = join(root, 'objects') mkdirSync(join(objects, 'pack'), { recursive: true }) }) - afterEach(async () => { - await _settleGitObjectQuarantineSweepsForTests() + afterEach(() => { failedRenameTargets.clear() + renamedTargets.length = 0 vi.restoreAllMocks() rmSync(root, { recursive: true, force: true }) }) @@ -229,14 +218,14 @@ describe('createGitObjectQuarantine', () => { ) }) - it('moves packs Git fetched into the scratch dir to the real store, index included', async () => { + it('moves packs Git fetched into the scratch dir to the real store, index last', async () => { await createGitObjectQuarantine(resolveNative()).run(async (env) => { const scratchPack = join(env?.GIT_OBJECT_DIRECTORY ?? '', 'pack') mkdirSync(scratchPack) - for (const file of ['pack-abc1.pack', 'pack-abc1.idx', 'pack-abc1.promisor']) { + for (const file of ['pack-abc1.idx', 'pack-abc1.pack', 'pack-abc1.promisor']) { writeFileSync(join(scratchPack, file), file) } - // A pack still being written has no index yet; Git could not use it. + // A pack without its index is invisible to Git, so moving it is harmless. writeFileSync(join(scratchPack, 'pack-def2.pack'), 'partial') writeFileSync(join(scratchPack, 'tmp_pack_XYZ'), 'partial') mkdirSync(join(env?.GIT_OBJECT_DIRECTORY ?? '', 'ab')) @@ -246,27 +235,14 @@ describe('createGitObjectQuarantine', () => { expect(readdirSync(join(objects, 'pack')).sort()).toEqual([ 'pack-abc1.idx', 'pack-abc1.pack', - 'pack-abc1.promisor' + 'pack-abc1.promisor', + 'pack-def2.pack' ]) + expect(renamedTargets.at(-1)).toBe('pack-abc1.idx') expect(existsSync(join(objects, 'ab'))).toBe(false) expect(scratchDirs()).toEqual([]) }) - it('rolls back a pack whose index could not be moved, keeping the other packs', async () => { - failedRenameTargets.add('pack-abc1.idx') - - await createGitObjectQuarantine(resolveNative()).run(async (env) => { - const scratchPack = join(env?.GIT_OBJECT_DIRECTORY ?? '', 'pack') - mkdirSync(scratchPack) - for (const file of ['pack-abc1.pack', 'pack-abc1.idx', 'pack-def2.pack', 'pack-def2.idx']) { - writeFileSync(join(scratchPack, file), file) - } - }) - - expect(readdirSync(join(objects, 'pack')).sort()).toEqual(['pack-def2.idx', 'pack-def2.pack']) - expect(scratchDirs()).toEqual([]) - }) - it('leaves a pack the real store already has untouched', async () => { writeFileSync(join(objects, 'pack', 'pack-abc1.pack'), 'installed') writeFileSync(join(objects, 'pack', 'pack-abc1.idx'), 'installed') @@ -282,71 +258,28 @@ describe('createGitObjectQuarantine', () => { expect(readFileSync(join(objects, 'pack', 'pack-abc1.idx'), 'utf8')).toBe('installed') }) - it('never moves a fetched pack’s `.keep` into the real store', async () => { - await createGitObjectQuarantine(resolveNative()).run(async (env) => { - const scratchPack = join(env?.GIT_OBJECT_DIRECTORY ?? '', 'pack') - mkdirSync(scratchPack) - for (const file of ['pack-abc1.pack', 'pack-abc1.idx', 'pack-abc1.keep']) { - writeFileSync(join(scratchPack, file), file) - } - }) - - expect(readdirSync(join(objects, 'pack')).sort()).toEqual(['pack-abc1.idx', 'pack-abc1.pack']) - }) + it('stops at a failed pack move, still removing the scratch dir and returning the result', async () => { + failedRenameTargets.add('pack-abc1.pack') + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) - describe('stale scratch sweep', () => { - const DAY = 24 * 60 * 60 * 1000 - const makeDir = (name: string, ageMs: number): string => { - const dir = join(objects, name) - mkdirSync(dir) - const modified = (Date.now() - ageMs) / 1000 - utimesSync(dir, modified, modified) - return dir - } - - it('removes scratch dirs past Git’s own two-week expiry and keeps younger ones', async () => { - expect(STALE_GIT_OBJECT_QUARANTINE_AGE_MS).toBe(14 * DAY) - const expired = makeDir(`${GIT_OBJECT_QUARANTINE_DIR_PREFIX}old`, 15 * DAY) - const young = makeDir(`${GIT_OBJECT_QUARANTINE_DIR_PREFIX}young`, 13 * DAY) - const gitOwn = makeDir('tmp_objdir-incoming-old', 15 * DAY) - - await createGitObjectQuarantine(resolveNative()).run(async () => {}) - await _settleGitObjectQuarantineSweepsForTests() - - expect(existsSync(expired)).toBe(false) - expect(existsSync(young)).toBe(true) - expect(existsSync(gitOwn)).toBe(true) - }) - - it('sweeps once per objects dir', async () => { - await createGitObjectQuarantine(resolveNative()).run(async () => {}) - await _settleGitObjectQuarantineSweepsForTests() - const expired = makeDir(`${GIT_OBJECT_QUARANTINE_DIR_PREFIX}old`, 15 * DAY) - - await createGitObjectQuarantine(resolveNative()).run(async () => {}) - await _settleGitObjectQuarantineSweepsForTests() - - expect(existsSync(expired)).toBe(true) - }) - - it('does not hold the command back while the sweep runs', async () => { - let releaseSweep = (): void => {} - objectsReaddirGate.current = new Promise((resolve) => { - releaseSweep = resolve + await expect( + createGitObjectQuarantine(resolveNative()).run(async (env) => { + const scratchPack = join(env?.GIT_OBJECT_DIRECTORY ?? '', 'pack') + mkdirSync(scratchPack) + for (const file of ['pack-abc1.idx', 'pack-abc1.pack']) { + writeFileSync(join(scratchPack, file), file) + } + return 'ok' }) - let commandRan = false - try { - const running = createGitObjectQuarantine(resolveNative()).run(async () => { - commandRan = true - }) - await vi.waitFor(() => expect(commandRan).toBe(true)) - releaseSweep() - await running - } finally { - releaseSweep() - objectsReaddirGate.current = undefined - } - }) + ).resolves.toBe('ok') + + // The index never lands without its pack. + expect(readdirSync(join(objects, 'pack'))).toEqual([]) + expect(scratchDirs()).toEqual([]) + expect(warn).toHaveBeenCalledWith( + '[git-object-quarantine] could not keep a fetched pack', + expect.any(Error) + ) }) }) diff --git a/src/shared/git-object-quarantine.ts b/src/shared/git-object-quarantine.ts index 80b779424cdf..f7183b0432d0 100644 --- a/src/shared/git-object-quarantine.ts +++ b/src/shared/git-object-quarantine.ts @@ -1,4 +1,4 @@ -import { lstat, mkdtemp, readdir, rename, rm, stat } from 'node:fs/promises' +import { lstat, mkdtemp, readdir, rename, stat } from 'node:fs/promises' import { posix, win32 } from 'node:path' import { isWindowsAbsolutePathLike } from './cross-platform-path' import { removeTree } from './windows-transient-lock-removal' @@ -33,11 +33,6 @@ export type GitObjectQuarantine = { // `git gc` expires stale `tmp_*` entries too. export const GIT_OBJECT_QUARANTINE_DIR_PREFIX = 'tmp_objdir-orca-merge-tree-' -// Why two weeks: `git gc` expires its own `objects/tmp_*` dirs at that age, and no merge-tree runs that long. -export const STALE_GIT_OBJECT_QUARANTINE_AGE_MS = 14 * 24 * 60 * 60 * 1000 - -const sweepsByObjectsDirectory = new Map>() - /** Decided by path syntax, not by platform: a Windows main process drives WSL Git. */ export function pathApiForGitPath(value: string): typeof posix { return isWindowsAbsolutePathLike(value) ? win32 : posix @@ -99,35 +94,6 @@ async function quarantineAlternates(objects: GitObjectsDirectory): Promise { - const path = pathApiForGitPath(objectsHostPath) - const entries = await readdir(objectsHostPath).catch(() => []) - const cutoff = Date.now() - STALE_GIT_OBJECT_QUARANTINE_AGE_MS - for (const entry of entries) { - if (!entry.startsWith(GIT_OBJECT_QUARANTINE_DIR_PREFIX)) { - continue - } - const scratch = path.join(objectsHostPath, entry) - const modified = await stat(scratch).then( - (stats) => stats.mtimeMs, - () => undefined - ) - if (modified !== undefined && modified < cutoff) { - await removeTree(scratch).catch(() => {}) - } - } -} - -// Why not awaited: cleanup must not delay the user's check, and it only removes dirs far older than this run's. -function startSweepOnce(objectsHostPath: string): void { - if (!sweepsByObjectsDirectory.has(objectsHostPath)) { - sweepsByObjectsDirectory.set( - objectsHostPath, - sweepStaleScratchDirectories(objectsHostPath).catch(() => {}) - ) - } -} - /** * A partial clone fetches missing blobs on demand, and Git files that download * as a pack in the scratch dir. Keep those packs so the next check does not @@ -136,40 +102,24 @@ function startSweepOnce(objectsHostPath: string): void { async function keepFetchedPacks(scratchHostPath: string, objectsHostPath: string): Promise { const path = pathApiForGitPath(objectsHostPath) const scratchPackDir = path.join(scratchHostPath, 'pack') - const entries = await readdir(scratchPackDir).catch(() => []) - const packNames = entries - .filter((entry) => /^pack-[0-9a-f]+\.pack$/.test(entry)) - .map((entry) => entry.slice(0, -'.pack'.length)) - for (const packName of packNames) { - const files = entries.filter((entry) => entry.startsWith(`${packName}.`)) - if (!files.includes(`${packName}.idx`)) { - continue - } - // Why no `.keep`: an aborted lazy fetch can leave its transient one, and gc never repacks a kept pack. - const kept = files.filter((file) => !file.endsWith('.keep')) - // Why `.idx` last: Git finds a pack through its index, so everything it names must already be in place. - const ordered = [...kept.filter((file) => !file.endsWith('.idx')), `${packName}.idx`] - const installed: string[] = [] - try { - for (const file of ordered) { - const target = path.join(objectsHostPath, 'pack', file) - // Why skip: pack names are content hashes, and replacing a live pack's file could strand its index. - const exists = await lstat(target).then( - () => true, - () => false - ) - if (exists) { - continue - } - await rename(path.join(scratchPackDir, file), target) - installed.push(target) - } - } catch (error) { - // Why undo: a pack without its index is garbage Git cannot see; the next lookup fetches it again. - for (const target of installed) { - await rm(target, { force: true }).catch(() => {}) - } - console.warn('[git-object-quarantine] could not keep a fetched pack', packName, error) + // Why `pack-` only: Git's in-progress `tmp_pack_*` files are not usable packs. + const files = (await readdir(scratchPackDir).catch(() => [])).filter((file) => + file.startsWith('pack-') + ) + // Why `.idx` last: Git finds a pack through its index (Git's own migration order). + const ordered = [ + ...files.filter((file) => !file.endsWith('.idx')), + ...files.filter((file) => file.endsWith('.idx')) + ] + for (const file of ordered) { + const target = path.join(objectsHostPath, 'pack', file) + // Why skip: pack names are content hashes, and Git also leaves an existing file in place. + const exists = await lstat(target).then( + () => true, + () => false + ) + if (!exists) { + await rename(path.join(scratchPackDir, file), target) } } } @@ -190,7 +140,6 @@ export function createGitObjectQuarantine( const alternates = objects ? await quarantineAlternates(objects) : undefined let scratchHostPath: string | undefined if (objects && alternates !== undefined) { - startSweepOnce(objects.hostPath) const path = pathApiForGitPath(objects.hostPath) scratchHostPath = await mkdtemp( path.join(objects.hostPath, GIT_OBJECT_QUARANTINE_DIR_PREFIX) @@ -209,9 +158,12 @@ export function createGitObjectQuarantine( GIT_ALTERNATE_OBJECT_DIRECTORIES: alternates }) } finally { - await keepFetchedPacks(scratchHostPath, objects.hostPath) + // Why stop at the first failure: an index must never be installed after its pack failed. + await keepFetchedPacks(scratchHostPath, objects.hostPath).catch((error: unknown) => { + console.warn('[git-object-quarantine] could not keep a fetched pack', error) + }) await removeTree(scratchHostPath).catch((error: unknown) => { - // Why: the stale sweep removes it later; the check's result still stands. + // Why: `git gc` prunes leftover `tmp_*` dirs; the check's result still stands. console.warn( '[git-object-quarantine] could not remove scratch dir', scratchHostPath, @@ -222,11 +174,3 @@ export function createGitObjectQuarantine( } } } - -export function _resetGitObjectQuarantineSweepForTests(): void { - sweepsByObjectsDirectory.clear() -} - -export async function _settleGitObjectQuarantineSweepsForTests(): Promise { - await Promise.all(sweepsByObjectsDirectory.values()) -} From 2fae54e5d808b8c89b8ef9f13d7940f54ba7ef2c Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Fri, 2 Oct 2026 16:14:17 -0700 Subject: [PATCH 10/13] fix(git): quarantine merge-tree only once the host is known to support --write-tree Git before 2.35 cannot prune a leftover scratch folder under objects/tmp_*, so a crash in the moment one exists makes every later `git gc` warn and stops automatic cleanup. On Git before 2.38 merge-tree --write-tree is rejected, yet the first check per host per session still made and removed a folder around it. Branch cleanup and the PR conflict list now use the scratch object store only when that host's capability cache already knows merge-tree --write-tree works. Until then the command runs exactly as it does on main: on a Git that rejects it no folder is ever created, and every Git that does get one is 2.38+, whose gc prunes it. Also reword two comments in the quarantine module. Co-Authored-By: Claude --- ...ge-tree-object-quarantine-real-git.test.ts | 53 +++++++++++- src/main/github/conflict-summary.test.ts | 84 +++++++++++++++++-- src/main/github/conflict-summary.ts | 49 +++++------ ...lay-git-object-quarantine-real-git.test.ts | 7 ++ src/shared/git-binary-compatibility.test.ts | 47 ++++++++++- src/shared/git-branch-cleanup.test.ts | 50 +++++++++++ src/shared/git-branch-cleanup.ts | 6 +- src/shared/git-object-quarantine.ts | 8 +- 8 files changed, 257 insertions(+), 47 deletions(-) diff --git a/src/main/git/merge-tree-object-quarantine-real-git.test.ts b/src/main/git/merge-tree-object-quarantine-real-git.test.ts index b233dc9a2ef4..bca3b91168a9 100644 --- a/src/main/git/merge-tree-object-quarantine-real-git.test.ts +++ b/src/main/git/merge-tree-object-quarantine-real-git.test.ts @@ -1,6 +1,8 @@ import { existsSync, readdirSync, writeFileSync } from 'node:fs' -import { dirname, join } from 'node:path' +import type * as NodeFsPromises from 'node:fs/promises' +import { basename, dirname, join } from 'node:path' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { GIT_OBJECT_QUARANTINE_DIR_PREFIX } from '../../shared/git-object-quarantine' import { createDivergentRepoFixture, type DivergentRepoFixture @@ -9,10 +11,33 @@ import { __resetPRConflictSummaryCachesForTests, getPRConflictSummary } from '../github/conflict-summary' +import { __resetPRConflictSummaryDerivationCachesForTests } from '../github/conflict-summary-cache' +import { getLocalGitCapabilityCache } from './git-capability-state' import { createLocalGitObjectQuarantine } from './local-git-object-quarantine' import { gitExecFileAsync } from './runner' import { deleteBranchAfterWorktreeRemoval } from './worktree-branch-removal' +const scratchFoldersMade = vi.hoisted((): string[] => []) + +vi.mock('node:fs/promises', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + mkdtemp: async (prefix: string) => { + const made = await actual.mkdtemp(prefix) + if (basename(made).startsWith(GIT_OBJECT_QUARANTINE_DIR_PREFIX)) { + scratchFoldersMade.push(made) + } + return made + } + } +}) + +/** Records what a first successful check teaches the host; the cold path has its own test. */ +function rememberMergeTreeWritesTrees(): void { + getLocalGitCapabilityCache().rememberSupported('merge-tree-write-tree') +} + // Why: every divergent `merge-tree --write-tree` used to leave unreachable loose objects in the // real store; enough of them keeps Git's auto-gc re-running forever on large repositories. describe('merge-tree runs against a scratch object store (real Git)', () => { @@ -20,6 +45,8 @@ describe('merge-tree runs against a scratch object store (real Git)', () => { beforeEach(() => { __resetPRConflictSummaryCachesForTests() + rememberMergeTreeWritesTrees() + scratchFoldersMade.length = 0 fixture = createDivergentRepoFixture() }) @@ -111,7 +138,7 @@ describe('merge-tree runs against a scratch object store (real Git)', () => { const first = await getPRConflictSummary(clone.clonePath, 'main', base, head) const packsAfterFirst = clone.packCount() - __resetPRConflictSummaryCachesForTests() + __resetPRConflictSummaryDerivationCachesForTests() const second = await getPRConflictSummary(clone.clonePath, 'main', base, head) expect(first?.files).toEqual(['shared.txt']) @@ -120,6 +147,7 @@ describe('merge-tree runs against a scratch object store (real Git)', () => { expect(packsAfterFirst).toBeGreaterThan(packsBefore) expect(clone.packCount()).toBe(packsAfterFirst) expect(clone.looseObjectCount()).toBe(looseBefore) + expect(scratchFoldersMade).toHaveLength(2) expect(clone.scratchDirectories()).toEqual([]) }) @@ -131,14 +159,31 @@ describe('merge-tree runs against a scratch object store (real Git)', () => { const online = await getPRConflictSummary(clone.clonePath, 'main', base, head) git('remote', 'set-url', 'origin', join(dirname(clone.clonePath), 'unreachable-origin')) - __resetPRConflictSummaryCachesForTests() + __resetPRConflictSummaryDerivationCachesForTests() const offline = await getPRConflictSummary(clone.clonePath, 'main', base, head) expect(online?.files).toEqual(['shared.txt']) expect(offline?.files).toEqual(['shared.txt']) + expect(scratchFoldersMade).toHaveLength(2) expect(clone.scratchDirectories()).toEqual([]) }) + it('makes no scratch folder until a check shows this Git writes trees, then one per check', async () => { + __resetPRConflictSummaryCachesForTests() + const base = fixture.git(fixture.repoPath, 'rev-parse', 'main').trim() + const head = fixture.git(fixture.repoPath, 'rev-parse', 'feature-conflict').trim() + + const first = await getPRConflictSummary(fixture.repoPath, 'main', base, head) + expect(scratchFoldersMade).toEqual([]) + __resetPRConflictSummaryDerivationCachesForTests() + const second = await getPRConflictSummary(fixture.repoPath, 'main', base, head) + + expect(first?.files).toEqual(['shared.txt']) + expect(second?.files).toEqual(['shared.txt']) + expect(scratchFoldersMade).toHaveLength(1) + expect(fixture.scratchDirectories()).toEqual([]) + }) + it('removes the scratch directory when the Git command fails', async () => { const quarantine = createLocalGitObjectQuarantine(fixture.linkedPath) let scratch: string | undefined @@ -188,6 +233,8 @@ describe('merge-tree quarantine keeps the object stores Git would otherwise see beforeEach(() => { __resetPRConflictSummaryCachesForTests() + rememberMergeTreeWritesTrees() + scratchFoldersMade.length = 0 fixture = createDivergentRepoFixture() }) diff --git a/src/main/github/conflict-summary.test.ts b/src/main/github/conflict-summary.test.ts index 1da118da8610..a65d2b6e1df7 100644 --- a/src/main/github/conflict-summary.test.ts +++ b/src/main/github/conflict-summary.test.ts @@ -12,6 +12,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' // throttle window. const gitExecFileAsyncMock = vi.hoisted(() => vi.fn()) +const scratchFolderPrefixes = vi.hoisted((): string[] => []) vi.mock('../git/runner', () => ({ gitExecFileAsync: gitExecFileAsyncMock })) // Why: off Windows a WSL objects path is relative, so a real scratch dir would land in the cwd. @@ -20,6 +21,7 @@ vi.mock('node:fs/promises', async (importOriginal) => { return { ...actual, mkdtemp: async (prefix: string) => { + scratchFolderPrefixes.push(prefix) if (!isAbsolute(prefix)) { throw new Error('no object store in this test') } @@ -28,13 +30,15 @@ vi.mock('node:fs/promises', async (importOriginal) => { } }) +import { GIT_CAPABILITY_RETRY_INTERVAL_MS } from '../../shared/git-capability-cache' +import { getLocalGitCapabilityCache } from '../git/git-capability-state' import { CONFLICT_SUMMARY_BASE_FETCH_WINDOW_MS } from './conflict-summary-cache' import { __resetPRConflictSummaryCachesForTests, getPRConflictSummary } from './conflict-summary' type GitResult = { stdout: string } type GitHandler = (argv: string[]) => Promise -// Why its own key: the merge-tree object quarantine reads the common dir once per derivation. +// Why its own key: once merge-tree is known to work, its quarantine reads the common dir per derivation. const COMMON_DIR_COMMAND = 'rev-parse --git-common-dir' const defaultHandlers: Record = { @@ -91,6 +95,7 @@ describe('getPRConflictSummary caching', () => { vi.useFakeTimers() vi.setSystemTime(1_750_000_000_000) gitExecFileAsyncMock.mockReset() + scratchFolderPrefixes.length = 0 __resetPRConflictSummaryCachesForTests() }) @@ -104,14 +109,13 @@ describe('getPRConflictSummary caching', () => { const first = await deriveSummary() expect(first).toEqual(expectedSummary) expect(spawnCount('fetch')).toBe(1) - expect(spawnCount(COMMON_DIR_COMMAND)).toBe(1) - expect(spawnCount()).toBe(6) + expect(spawnCount()).toBe(5) const second = await deriveSummary() const third = await deriveSummary() expect(second).toEqual(expectedSummary) expect(third).toEqual(expectedSummary) - expect(spawnCount()).toBe(6) + expect(spawnCount()).toBe(5) }) it('re-derives when headRefOid changes without re-fetching inside the throttle window', async () => { @@ -178,7 +182,7 @@ describe('getPRConflictSummary caching', () => { expect(first).toEqual(expectedSummary) expect(second).toEqual(expectedSummary) - expect(spawnCount()).toBe(6) + expect(spawnCount()).toBe(5) }) it('dedupes overlapping derivations after resolving the same live base tip', async () => { @@ -291,7 +295,7 @@ describe('getPRConflictSummary caching', () => { const repeat = await deriveSummary() expect(repeat).toEqual(expectedSummary) expect(spawnCount('fetch')).toBe(1) - expect(spawnCount()).toBe(6) + expect(spawnCount()).toBe(5) }) it("falls back to GitHub's baseRefOid when fetch and remote-tracking refs are unavailable", async () => { @@ -327,12 +331,15 @@ describe('getPRConflictSummary caching', () => { it('preserves background admission across the complete WSL derivation chain', async () => { mockGitDispatch() + // Known support adds the quarantine's common-dir read to the chain. + getLocalGitCapabilityCache({ wslDistro: 'Ubuntu' }).rememberSupported('merge-tree-write-tree') await getPRConflictSummary('/repo-root', 'main', 'github-base-oid', 'head-oid-1', { wslDistro: 'Ubuntu', admissionTier: 'background' }) + expect(spawnCount(COMMON_DIR_COMMAND)).toBe(1) expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(6) for (const [, options] of gitExecFileAsyncMock.mock.calls) { expect(options).toEqual( @@ -393,6 +400,60 @@ describe('getPRConflictSummary caching', () => { expect(spawnCount('merge-tree')).toBe(1) }) + it('makes no scratch folder for a Git that rejects merge-tree --write-tree, retries included', async () => { + // Git 2.25's exact rejection of both argument shapes. + const rejected = Object.assign(new Error('Command failed: git merge-tree --write-tree'), { + code: 129, + stdout: '', + stderr: 'usage: git merge-tree \n' + }) + mockGitDispatch({ 'merge-tree': () => Promise.reject(rejected) }) + + await expect(deriveSummary('head-oid-1')).resolves.toBeUndefined() + vi.setSystemTime(1_750_000_000_000 + GIT_CAPABILITY_RETRY_INTERVAL_MS + 1_000) + await expect(deriveSummary('head-oid-2')).resolves.toBeUndefined() + + expect(spawnCount('merge-tree')).toBe(2) + expect(spawnCount(COMMON_DIR_COMMAND)).toBe(0) + expect(scratchFolderPrefixes).toEqual([]) + expect(gitExecFileAsyncMock.mock.calls.filter(([, options]) => options?.env)).toEqual([]) + }) + + it('runs the first merge-tree as Git always has, then the next ones against a scratch store', async () => { + const commonDir = join( + realpathSync(mkdtempSync(join(tmpdir(), 'orca-conflict-summary-'))), + '.git' + ) + mkdirSync(join(commonDir, 'objects'), { recursive: true }) + try { + mockGitDispatch({ [COMMON_DIR_COMMAND]: async () => ({ stdout: `${commonDir}\n` }) }) + + await expect(deriveSummary('head-oid-1')).resolves.toEqual(expectedSummary) + expect(spawnCount(COMMON_DIR_COMMAND)).toBe(0) + expect(scratchFolderPrefixes).toEqual([]) + await expect(deriveSummary('head-oid-2')).resolves.toEqual(expectedSummary) + + const envs = gitExecFileAsyncMock.mock.calls + .filter(([argv]) => argv[0] === 'merge-tree') + .map(([argv, options]) => [argv.includes('--merge-base'), options?.env]) + expect(envs).toEqual([ + [true, undefined], + [ + true, + expect.objectContaining({ + GIT_OBJECT_DIRECTORY: expect.stringContaining( + join(commonDir, 'objects', 'tmp_objdir-orca-merge-tree-') + ) + }) + ] + ]) + expect(scratchFolderPrefixes).toHaveLength(1) + expect(readdirSync(join(commonDir, 'objects'))).toEqual([]) + } finally { + rmSync(join(commonDir, '..'), { recursive: true, force: true }) + } + }) + it('runs the legacy merge-tree fallback against the scratch object store too', async () => { const commonDir = join( realpathSync(mkdtempSync(join(tmpdir(), 'orca-conflict-summary-'))), @@ -410,17 +471,22 @@ describe('getPRConflictSummary caching', () => { } }) - await expect(deriveSummary()).resolves.toEqual(expectedSummary) + // The first check learns that this Git writes trees; the next one is quarantined. + await expect(deriveSummary('head-oid-1')).resolves.toEqual(expectedSummary) + await expect(deriveSummary('head-oid-2')).resolves.toEqual(expectedSummary) - const legacy = gitExecFileAsyncMock.mock.calls.find( + const legacy = gitExecFileAsyncMock.mock.calls.filter( ([argv]) => argv[0] === 'merge-tree' && !argv.includes('--merge-base') ) - expect(legacy?.[1]?.env).toMatchObject({ + expect(legacy).toHaveLength(2) + expect(legacy[0]?.[1]?.env).toBeUndefined() + expect(legacy[1]?.[1]?.env).toMatchObject({ GIT_OBJECT_DIRECTORY: expect.stringContaining( join(commonDir, 'objects', 'tmp_objdir-orca-merge-tree-') ), GIT_ALTERNATE_OBJECT_DIRECTORIES: join(commonDir, 'objects') }) + expect(spawnCount('merge-tree')).toBe(3) expect(readdirSync(join(commonDir, 'objects'))).toEqual([]) } finally { rmSync(join(commonDir, '..'), { recursive: true, force: true }) diff --git a/src/main/github/conflict-summary.ts b/src/main/github/conflict-summary.ts index a1f23385cb26..1065c67d24c5 100644 --- a/src/main/github/conflict-summary.ts +++ b/src/main/github/conflict-summary.ts @@ -5,10 +5,7 @@ import { } from '../../shared/git-merge-tree-capability' import { gitExecFileAsync } from '../git/runner' import { gitOptionsForWorktree, type GitRuntimeOptions } from '../git/git-runtime-options' -import { - createLocalGitObjectQuarantine, - type LocalGitObjectQuarantine -} from '../git/local-git-object-quarantine' +import { createLocalGitObjectQuarantine } from '../git/local-git-object-quarantine' import { clearGitCapabilityStateForTests, withLocalGitCapabilityCacheForExecution @@ -243,20 +240,26 @@ async function loadConflictingFiles( return withLocalGitCapabilityCacheForExecution( { cwd: repoPath, wslDistro: localGitOptions.wslDistro }, - (capabilities) => - capabilities.runWithFallback( + (capabilities) => { + const runMergeTree = (args: string[]): Promise<{ stdout: string }> => { + const run = (env: NodeJS.ProcessEnv | undefined) => + gitExecFileAsync(args, { + ...gitOptionsForWorktree(repoPath, localGitOptions), + ...(env ? { env } : {}) + }) + // Why only once known supported: old Git would still get a scratch folder, and Git < 2.35 cannot prune a leftover one. + return capabilities.isKnownSupported('merge-tree-write-tree') + ? quarantine.run(run) + : run(undefined) + } + return capabilities.runWithFallback( 'merge-tree-write-tree', () => capabilities.runWithFallback( 'merge-tree-merge-base', async () => { try { - const result = await quarantine.run((env) => - gitExecFileAsync(modernArgs, { - ...gitOptionsForWorktree(repoPath, localGitOptions), - ...(env ? { env } : {}) - }) - ) + const result = await runMergeTree(modernArgs) return parseMergeTreeNameOnlyOutput(result.stdout) } catch (error) { if (isUnsupportedMergeTreeWriteTreeError(error)) { @@ -271,13 +274,7 @@ async function loadConflictingFiles( throw error } }, - () => - loadConflictingFilesWithLegacyMergeTree( - repoPath, - legacyArgs, - localGitOptions, - quarantine - ), + () => loadConflictingFilesWithLegacyMergeTree(runMergeTree, legacyArgs), isUnsupportedMergeTreeMergeBaseError ), async () => { @@ -287,22 +284,16 @@ async function loadConflictingFiles( }, isUnsupportedMergeTreeWriteTreeError ) + } ) } async function loadConflictingFilesWithLegacyMergeTree( - repoPath: string, - legacyArgs: string[], - localGitOptions: LocalGitExecOptions, - quarantine: LocalGitObjectQuarantine + runMergeTree: (args: string[]) => Promise<{ stdout: string }>, + legacyArgs: string[] ): Promise { try { - const result = await quarantine.run((env) => - gitExecFileAsync(legacyArgs, { - ...gitOptionsForWorktree(repoPath, localGitOptions), - ...(env ? { env } : {}) - }) - ) + const result = await runMergeTree(legacyArgs) return parseMergeTreeNameOnlyOutput(result.stdout) } catch (fallbackError) { const fallbackStdout = getGitErrorOutput(fallbackError, 'stdout') diff --git a/src/relay/relay-git-object-quarantine-real-git.test.ts b/src/relay/relay-git-object-quarantine-real-git.test.ts index 078c30d01073..63847d1bd0c5 100644 --- a/src/relay/relay-git-object-quarantine-real-git.test.ts +++ b/src/relay/relay-git-object-quarantine-real-git.test.ts @@ -31,7 +31,13 @@ describe('relay branch cleanup runs merge-tree against a scratch object store (r return dispatcher.callRequest('git.removeWorktree', { worktreePath }) } + // Why: the relay quarantines only once a check has shown its Git writes trees; that first check runs as Git always has. + async function learnMergeTreeSupportThrough(branch: string): Promise { + await removeWorktreeFor(branch) + } + it('still deletes a squash-merged branch through the scratch store', async () => { + await learnMergeTreeSupportThrough('feature-extra') const before = fixture.looseObjectCount() await expect(removeWorktreeFor('feature-squashed')).resolves.toEqual({}) @@ -43,6 +49,7 @@ describe('relay branch cleanup runs merge-tree against a scratch object store (r it('preserves a branch with unmerged changes without writing loose objects', async () => { const head = fixture.git(fixture.repoPath, 'rev-parse', 'feature-extra').trim() + await learnMergeTreeSupportThrough('feature-squashed') const before = fixture.looseObjectCount() await expect(removeWorktreeFor('feature-extra')).resolves.toEqual({ diff --git a/src/shared/git-binary-compatibility.test.ts b/src/shared/git-binary-compatibility.test.ts index 8575f0fddd29..346520abc9af 100644 --- a/src/shared/git-binary-compatibility.test.ts +++ b/src/shared/git-binary-compatibility.test.ts @@ -1,5 +1,5 @@ import { execFile } from 'node:child_process' -import { mkdir, mkdtemp, readFile, rm, writeFile } from 'node:fs/promises' +import { mkdir, mkdtemp, readdir, readFile, rm, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { dirname, join } from 'node:path' import { promisify } from 'node:util' @@ -9,6 +9,15 @@ import { isUnsupportedMergeTreeWriteTreeError } from './git-merge-tree-capability' import { isBranchCheckedOutInWorktreeError } from './git-branch-delete-refusal' +import { + branchHasNoUnmergedChangesOnAnyTarget, + type GitBranchCleanupExec +} from './git-branch-cleanup' +import { GitCapabilityCache } from './git-capability-cache' +import { + createGitObjectQuarantine, + GIT_OBJECT_QUARANTINE_DIR_PREFIX +} from './git-object-quarantine' import { isForEachRefExcludeUnsupportedError } from './git-ref-command-capabilities' import { isNoWriteFetchHeadUnsupportedError } from './git-fetch-head-capability' import { @@ -380,6 +389,42 @@ describeBinaryCompatibility('real Git binary compatibility', () => { } }) + // Why pin this: a scratch folder made around a Git that rejects --write-tree can outlive a crash, + // and Git before 2.35 never prunes it; only a Git known to write trees gets one. + it('makes a merge-tree scratch object store only once this Git is known to write trees', async () => { + const branch = 'compat-cleanup-merged' + await runGit(['branch', branch, 'HEAD']) + const objectsPath = join(repoPath, '.git', 'objects') + const quarantine = createGitObjectQuarantine(async () => ({ + hostPath: objectsPath, + gitPath: image ? '/repo/.git/objects' : objectsPath + })) + let scratchFolders = 0 + const runCleanupGit: GitBranchCleanupExec = (args, options) => + options?.discardWrittenObjects + ? quarantine.run((env) => { + scratchFolders += env ? 1 : 0 + return runGit(args, env) + }) + : runGit(args) + const capabilities = new GitCapabilityCache() + const check = () => + branchHasNoUnmergedChangesOnAnyTarget(runCleanupGit, branch, ['HEAD'], capabilities) + + try { + await expect(check()).resolves.toBe(supports(2, 38)) + await expect(check()).resolves.toBe(supports(2, 38)) + expect(scratchFolders).toBe(supports(2, 38) ? 1 : 0) + expect( + (await readdir(objectsPath)).filter((entry) => + entry.startsWith(GIT_OBJECT_QUARANTINE_DIR_PREFIX) + ) + ).toEqual([]) + } finally { + await runGit(['branch', '-D', branch]) + } + }) + it('supports exact show-ref probes', async () => { const head = (await runGit(['rev-parse', 'HEAD'])).stdout.trim() const originRef = 'refs/remotes/origin/compat-exact' diff --git a/src/shared/git-branch-cleanup.test.ts b/src/shared/git-branch-cleanup.test.ts index 33a4ed663058..97e69c262f1e 100644 --- a/src/shared/git-branch-cleanup.test.ts +++ b/src/shared/git-branch-cleanup.test.ts @@ -116,6 +116,56 @@ describe('branchHasNoUnmergedChangesOnAnyTarget', () => { expect(new Set(discarding)).toEqual(new Set(['merge-tree'])) }) + it('runs the first merge-tree as Git always has, then the next ones against a scratch store', async () => { + const runGit = baseProofResponses() + + await branchHasNoUnmergedChangesOnAnyTarget( + runGit, + 'feature/test', + ['refs/remotes/origin/main'], + new GitCapabilityCache() + ) + + const mergeTrees = vi + .mocked(runGit) + .mock.calls.filter(([args]) => args[0] === 'merge-tree') + .map(([args, options]) => [args[2], options?.discardWrittenObjects]) + expect(mergeTrees).toEqual([ + ['target', false], + ['squash', true] + ]) + }) + + it('never asks for a scratch store from a Git that rejects merge-tree --write-tree', async () => { + // Git 2.25's exact rejection. + const rejected = Object.assign(new Error('Command failed: git merge-tree --write-tree'), { + code: 129, + stdout: '', + stderr: 'usage: git merge-tree \n' + }) + const runGit = baseProofResponses({ + 'merge-tree --write-tree target refs/heads/feature/test': rejected, + 'merge-tree --write-tree squash refs/heads/feature/test': rejected + }) + const capabilities = new GitCapabilityCache() + const check = () => + branchHasNoUnmergedChangesOnAnyTarget( + runGit, + 'feature/test', + ['refs/remotes/origin/main'], + capabilities + ) + + await expect(check()).resolves.toBe(false) + await expect(check()).resolves.toBe(false) + + const mergeTrees = vi + .mocked(runGit) + .mock.calls.filter(([args]) => args[0] === 'merge-tree') + .map(([, options]) => options?.discardWrittenObjects) + expect(mergeTrees).toEqual([false]) + }) + it('preserves a branch with merge commits when no target squash commit matches', async () => { const runGit = baseProofResponses({ squashPatchId: 'other-patch squash\n' }) diff --git a/src/shared/git-branch-cleanup.ts b/src/shared/git-branch-cleanup.ts index 685b57444846..4729186e5c45 100644 --- a/src/shared/git-branch-cleanup.ts +++ b/src/shared/git-branch-cleanup.ts @@ -123,7 +123,11 @@ async function branchMergesWithoutTreeChanges( try { return await capabilities.runWithFallback( 'merge-tree-write-tree', - async () => (await runGit(args, { discardWrittenObjects: true })).stdout.trim() || null, + async () => { + // Why only once known supported: old Git would still get a scratch folder, and Git < 2.35 cannot prune a leftover one. + const discardWrittenObjects = capabilities.isKnownSupported('merge-tree-write-tree') + return (await runGit(args, { discardWrittenObjects })).stdout.trim() || null + }, async () => null, isUnsupportedMergeTreeWriteTreeError ) diff --git a/src/shared/git-object-quarantine.ts b/src/shared/git-object-quarantine.ts index f7183b0432d0..082594493965 100644 --- a/src/shared/git-object-quarantine.ts +++ b/src/shared/git-object-quarantine.ts @@ -30,7 +30,7 @@ export type GitObjectQuarantine = { // Why inside `objects/` with a `tmp_objdir` prefix: that is where Git puts its own // quarantine dirs, it is on the Git host for native, WSL and SSH alike, and -// `git gc` expires stale `tmp_*` entries too. +// `git gc` (2.35+) expires stale `tmp_*` dirs too. export const GIT_OBJECT_QUARANTINE_DIR_PREFIX = 'tmp_objdir-orca-merge-tree-' /** Decided by path syntax, not by platform: a Windows main process drives WSL Git. */ @@ -106,7 +106,7 @@ async function keepFetchedPacks(scratchHostPath: string, objectsHostPath: string const files = (await readdir(scratchPackDir).catch(() => [])).filter((file) => file.startsWith('pack-') ) - // Why `.idx` last: Git finds a pack through its index (Git's own migration order). + // Why `.idx` last: Git finds a pack through its index, so the rest must be in place first. const ordered = [ ...files.filter((file) => !file.endsWith('.idx')), ...files.filter((file) => file.endsWith('.idx')) @@ -119,6 +119,7 @@ async function keepFetchedPacks(scratchHostPath: string, objectsHostPath: string () => false ) if (!exists) { + // Why no per-file catch: stopping at the first failure keeps an index from landing without its pack. await rename(path.join(scratchPackDir, file), target) } } @@ -158,12 +159,11 @@ export function createGitObjectQuarantine( GIT_ALTERNATE_OBJECT_DIRECTORIES: alternates }) } finally { - // Why stop at the first failure: an index must never be installed after its pack failed. await keepFetchedPacks(scratchHostPath, objects.hostPath).catch((error: unknown) => { console.warn('[git-object-quarantine] could not keep a fetched pack', error) }) await removeTree(scratchHostPath).catch((error: unknown) => { - // Why: `git gc` prunes leftover `tmp_*` dirs; the check's result still stands. + // Why: callers quarantine only Git that writes trees (2.38+), whose `git gc` prunes leftover `tmp_*` dirs. console.warn( '[git-object-quarantine] could not remove scratch dir', scratchHostPath, From 5bc549e8cffb33c714396495ff8a6582db61faf3 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Fri, 2 Oct 2026 16:46:09 -0700 Subject: [PATCH 11/13] fix(git): learn merge-tree support from a conflicting branch-cleanup check Branch cleanup quarantines merge-tree only once the host's capability cache knows --write-tree works. A conflicting merge-tree exits 1, so the check threw and support was never recorded; on a host whose only checks conflict (likely over SSH, where the relay has no conflict summary to teach its cache) every check kept writing loose objects into the real store. A failure whose stdout starts with a tree ID proves --write-tree ran (old Git that rejects the option prints nothing there), so it now resolves to the same "not merged" result and lets the cache record support. Every other failure is rethrown unchanged. Also shorten two comments and soften a third. Co-Authored-By: Claude --- src/main/github/conflict-summary.ts | 2 +- ...lay-git-object-quarantine-real-git.test.ts | 42 ++++++++++++++ src/shared/git-branch-cleanup.test.ts | 56 +++++++++++++++++++ src/shared/git-branch-cleanup.ts | 21 ++++++- src/shared/git-object-quarantine.ts | 2 +- 5 files changed, 119 insertions(+), 4 deletions(-) diff --git a/src/main/github/conflict-summary.ts b/src/main/github/conflict-summary.ts index 1065c67d24c5..ff19a692f5e3 100644 --- a/src/main/github/conflict-summary.ts +++ b/src/main/github/conflict-summary.ts @@ -247,7 +247,7 @@ async function loadConflictingFiles( ...gitOptionsForWorktree(repoPath, localGitOptions), ...(env ? { env } : {}) }) - // Why only once known supported: old Git would still get a scratch folder, and Git < 2.35 cannot prune a leftover one. + // Why only once supported: an old Git still gets the folder, and Git < 2.35 can't prune a leftover. return capabilities.isKnownSupported('merge-tree-write-tree') ? quarantine.run(run) : run(undefined) diff --git a/src/relay/relay-git-object-quarantine-real-git.test.ts b/src/relay/relay-git-object-quarantine-real-git.test.ts index 63847d1bd0c5..e6f14078f251 100644 --- a/src/relay/relay-git-object-quarantine-real-git.test.ts +++ b/src/relay/relay-git-object-quarantine-real-git.test.ts @@ -1,4 +1,7 @@ +import type * as NodeFsPromises from 'node:fs/promises' +import { basename } from 'node:path' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { GIT_OBJECT_QUARANTINE_DIR_PREFIX } from '../shared/git-object-quarantine' import { createDivergentRepoFixture, type DivergentRepoFixture @@ -7,6 +10,22 @@ import type { GitHandler } from './git-handler' import { createGitHandlerRelay } from './git-handler-test-harness' import type { MockDispatcher } from './git-handler-test-setup' +const scratchFoldersMade = vi.hoisted((): string[] => []) + +vi.mock('node:fs/promises', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + mkdtemp: async (prefix: string) => { + const made = await actual.mkdtemp(prefix) + if (basename(made).startsWith(GIT_OBJECT_QUARANTINE_DIR_PREFIX)) { + scratchFoldersMade.push(made) + } + return made + } + } +}) + // Why: the SSH relay runs the same merge-tree proof on the remote host's own object store. describe('relay branch cleanup runs merge-tree against a scratch object store (real Git)', () => { let fixture: DivergentRepoFixture @@ -15,6 +34,7 @@ describe('relay branch cleanup runs merge-tree against a scratch object store (r beforeEach(() => { vi.spyOn(console, 'warn').mockImplementation(() => {}) + scratchFoldersMade.length = 0 fixture = createDivergentRepoFixture() ;({ dispatcher, handler } = createGitHandlerRelay()) }) @@ -59,4 +79,26 @@ describe('relay branch cleanup runs merge-tree against a scratch object store (r expect(fixture.looseObjectCount()).toBe(before) expect(fixture.scratchDirectories()).toEqual([]) }) + + it('learns from a conflicting check that its Git writes trees, so the next one is quarantined', async () => { + const head = fixture.git(fixture.repoPath, 'rev-parse', 'feature-conflict').trim() + const preserved = { preservedBranch: { branchName: 'feature-conflict', head } } + // No saved base, so HEAD (main) is the only target and each removal runs merge-tree once. + const removeConflicting = (worktreePath: string) => + dispatcher.callRequest('git.removeWorktree', { worktreePath }) + + const initial = fixture.looseObjectCount() + await expect(removeConflicting(fixture.linkedPath)).resolves.toEqual(preserved) + // The first check ran as Git always has: no scratch folder, so its conflict wrote loose objects. + expect(scratchFoldersMade).toEqual([]) + const before = fixture.looseObjectCount() + expect(before).toBeGreaterThan(initial) + + const again = fixture.addWorktree('wt-feature-conflict', 'feature-conflict') + await expect(removeConflicting(again)).resolves.toEqual(preserved) + + expect(scratchFoldersMade).toHaveLength(1) + expect(fixture.looseObjectCount()).toBe(before) + expect(fixture.scratchDirectories()).toEqual([]) + }) }) diff --git a/src/shared/git-branch-cleanup.test.ts b/src/shared/git-branch-cleanup.test.ts index 97e69c262f1e..1cb948189f4a 100644 --- a/src/shared/git-branch-cleanup.test.ts +++ b/src/shared/git-branch-cleanup.test.ts @@ -166,6 +166,62 @@ describe('branchHasNoUnmergedChangesOnAnyTarget', () => { expect(mergeTrees).toEqual([false]) }) + describe('a merge-tree that exits 1', () => { + function exitOne(stdout: string): Error { + return Object.assign(new Error('Command failed: git merge-tree --write-tree'), { + code: 1, + stdout, + stderr: '' + }) + } + + async function scratchStoreRequestsOverTwoChecks(failure: Error): Promise { + // No branch-only merge commits, so each check runs merge-tree exactly once. + const runGit = baseProofResponses({ + 'merge-tree --write-tree target refs/heads/feature/test': failure, + 'rev-list --right-only --merges --count target...refs/heads/feature/test': '0\n' + }) + const capabilities = new GitCapabilityCache() + const check = () => + branchHasNoUnmergedChangesOnAnyTarget( + runGit, + 'feature/test', + ['refs/remotes/origin/main'], + capabilities + ) + + await expect(check()).resolves.toBe(false) + await expect(check()).resolves.toBe(false) + + return vi + .mocked(runGit) + .mock.calls.filter(([args]) => args[0] === 'merge-tree') + .map(([, options]) => options?.discardWrittenObjects) + } + + it.each([ + ['SHA-1', 'a'.repeat(40)], + ['SHA-256', 'b'.repeat(64)] + ])( + 'learns support from a conflict (%s tree ID), so the next check asks for a scratch store', + async (_label, treeId) => { + const conflict = exitOne(`${treeId}\n100644 ${'c'.repeat(40)} 1\tshared.txt\n\n`) + + await expect(scratchStoreRequestsOverTwoChecks(conflict)).resolves.toEqual([false, true]) + } + ) + + it.each([ + ['empty stdout', ''], + ['stdout that is not a tree ID', 'merged-tree\n'] + ])('does not record support from %s', async (_label, stdout) => { + await expect(scratchStoreRequestsOverTwoChecks(exitOne(stdout))).resolves.toEqual([ + false, + false + ]) + }) + }) + it('preserves a branch with merge commits when no target squash commit matches', async () => { const runGit = baseProofResponses({ squashPatchId: 'other-patch squash\n' }) diff --git a/src/shared/git-branch-cleanup.ts b/src/shared/git-branch-cleanup.ts index 4729186e5c45..3a9d7375b0dc 100644 --- a/src/shared/git-branch-cleanup.ts +++ b/src/shared/git-branch-cleanup.ts @@ -112,6 +112,15 @@ async function hasBranchOnlyMergeCommits( return Number(stdout ?? 0) > 0 } +/** `merge-tree --write-tree` exits 1 on a conflict after printing the merged tree's ID first. */ +function printedMergedTreeId(error: unknown): boolean { + if (typeof error !== 'object' || error === null || !('stdout' in error)) { + return false + } + const firstLine = typeof error.stdout === 'string' ? error.stdout.split('\n', 1)[0].trim() : '' + return /^(?:[0-9a-f]{40}|[0-9a-f]{64})$/.test(firstLine) +} + async function branchMergesWithoutTreeChanges( runGit: GitBranchCleanupExec, targetOid: string, @@ -124,9 +133,17 @@ async function branchMergesWithoutTreeChanges( return await capabilities.runWithFallback( 'merge-tree-write-tree', async () => { - // Why only once known supported: old Git would still get a scratch folder, and Git < 2.35 cannot prune a leftover one. + // Why only once supported: an old Git still gets the folder, and Git < 2.35 can't prune a leftover. const discardWrittenObjects = capabilities.isKnownSupported('merge-tree-write-tree') - return (await runGit(args, { discardWrittenObjects })).stdout.trim() || null + try { + return (await runGit(args, { discardWrittenObjects })).stdout.trim() || null + } catch (error) { + // Why: a conflict still proves --write-tree ran; returning lets the cache record it. + if (printedMergedTreeId(error)) { + return null + } + throw error + } }, async () => null, isUnsupportedMergeTreeWriteTreeError diff --git a/src/shared/git-object-quarantine.ts b/src/shared/git-object-quarantine.ts index 082594493965..e3cd608e9b46 100644 --- a/src/shared/git-object-quarantine.ts +++ b/src/shared/git-object-quarantine.ts @@ -163,7 +163,7 @@ export function createGitObjectQuarantine( console.warn('[git-object-quarantine] could not keep a fetched pack', error) }) await removeTree(scratchHostPath).catch((error: unknown) => { - // Why: callers quarantine only Git that writes trees (2.38+), whose `git gc` prunes leftover `tmp_*` dirs. + // Why: quarantined only once this host's Git ran --write-tree (2.38+), whose gc prunes `tmp_*`. console.warn( '[git-object-quarantine] could not remove scratch dir', scratchHostPath, From eb5e4508b1f6c6f8fb00c60851602f21ee72b44a Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Thu, 15 Oct 2026 02:24:15 -0700 Subject: [PATCH 12/13] fix(git): put WSL Git's scratch object store on its command line A WSL login profile that set GIT_ALTERNATE_OBJECT_DIRECTORIES replaced the WSLENV-forwarded value, so a quarantined merge-tree lost the real store and failed where main succeeds. The WSL runner now passes both variables as command-line assignments after the profile, appending to its alternates. --- .../command-runner/git-command-resolution.ts | 1 + ...sl-command-resolution-object-store.test.ts | 44 +++++++++++++++++++ .../command-runner/wsl-command-resolution.ts | 24 +++++++++- .../git/local-git-object-quarantine.test.ts | 33 +------------- src/main/git/local-git-object-quarantine.ts | 18 +------- 5 files changed, 71 insertions(+), 49 deletions(-) create mode 100644 src/main/git/command-runner/wsl-command-resolution-object-store.test.ts diff --git a/src/main/git/command-runner/git-command-resolution.ts b/src/main/git/command-runner/git-command-resolution.ts index 750034dc0369..a70dd1368040 100644 --- a/src/main/git/command-runner/git-command-resolution.ts +++ b/src/main/git/command-runner/git-command-resolution.ts @@ -124,6 +124,7 @@ export function resolveGitCommandWithoutProbe( return resolveCommand('git', wslGitArgs(args, options), options.cwd, options.wslDistro, { useWslLoginShell: Boolean(options.wslDistro), captureLoginShellOutput, + env: options.env, terminationBarrier: options.terminationBarrier }) } diff --git a/src/main/git/command-runner/wsl-command-resolution-object-store.test.ts b/src/main/git/command-runner/wsl-command-resolution-object-store.test.ts new file mode 100644 index 000000000000..9585a2e88745 --- /dev/null +++ b/src/main/git/command-runner/wsl-command-resolution-object-store.test.ts @@ -0,0 +1,44 @@ +import { runProcess } from '@orca/process-host' +import { afterEach, beforeEach, describe, expect, it } from 'vitest' +import { resolveCommand } from './wsl-command-resolution' + +const originalPlatform = process.platform +const QUARANTINE = { + GIT_OBJECT_DIRECTORY: '/home/me/repo/.git/objects/tmp_objdir-orca-merge-tree-x', + GIT_ALTERNATE_OBJECT_DIRECTORIES: '/home/me/repo/.git/objects' +} + +/** What WSL Git would see when a login profile has set `profileAlternates` before the payload runs. */ +async function alternatesSeenByWslGit(profileAlternates: string | undefined): Promise { + const resolved = resolveCommand('git', [], undefined, 'Ubuntu', { + env: { ...process.env, ...QUARANTINE }, + wslShellCommand: 'printenv GIT_ALTERNATE_OBJECT_DIRECTORIES' + }) + const payload = resolved.args.at(-1) ?? '' + const profile = + profileAlternates === undefined + ? '' + : `export GIT_ALTERNATE_OBJECT_DIRECTORIES='${profileAlternates}'; ` + const result = await runProcess({ program: 'sh', args: ['-c', `${profile}${payload}`] }) + expect(result.code).toBe(0) + return result.stdout.trim() +} + +// Why pin this: a WSL login profile that sets GIT_ALTERNATE_OBJECT_DIRECTORIES used to replace a +// WSLENV-forwarded value, so a quarantined merge-tree lost the real object store and failed. +describe.skipIf(originalPlatform === 'win32')('WSL Git object-store variables', () => { + beforeEach(() => { + Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) + }) + + afterEach(() => { + Object.defineProperty(process, 'platform', { configurable: true, value: originalPlatform }) + }) + + it('reach WSL Git after the login profile, appending to its alternates', async () => { + await expect(alternatesSeenByWslGit(undefined)).resolves.toBe('/home/me/repo/.git/objects') + await expect(alternatesSeenByWslGit('/shared/objects')).resolves.toBe( + '/shared/objects:/home/me/repo/.git/objects' + ) + }) +}) diff --git a/src/main/git/command-runner/wsl-command-resolution.ts b/src/main/git/command-runner/wsl-command-resolution.ts index cb5d9de41f43..eebf7f10ebdc 100644 --- a/src/main/git/command-runner/wsl-command-resolution.ts +++ b/src/main/git/command-runner/wsl-command-resolution.ts @@ -23,6 +23,25 @@ const GIT_OUTPUT_LOCALE_ENV_ARGS = Object.entries(UNTRANSLATED_GIT_OUTPUT_ENV).m ([key, value]) => `${key}=${value}` ) +// Why on the command string: a login profile runs first and could replace WSLENV-forwarded values; +// alternates append to whatever the profile set, as Git's own scratch object stores do. +function wslGitObjectStorePrefix(env: NodeJS.ProcessEnv | undefined): string { + const objectDirectory = env?.GIT_OBJECT_DIRECTORY + const alternates = env?.GIT_ALTERNATE_OBJECT_DIRECTORIES + // Why compare with process.env: only a quarantine's own values, never a Windows-side inherited one. + if ( + objectDirectory === undefined || + alternates === undefined || + objectDirectory === process.env.GIT_OBJECT_DIRECTORY + ) { + return '' + } + return ( + `GIT_OBJECT_DIRECTORY=${quotePosixShell(objectDirectory)} ` + + `GIT_ALTERNATE_OBJECT_DIRECTORIES="\${GIT_ALTERNATE_OBJECT_DIRECTORIES:+$GIT_ALTERNATE_OBJECT_DIRECTORIES:}"${quotePosixShell(alternates)} ` + ) +} + export type ResolvedCommand = { binary: string args: string[] @@ -92,7 +111,10 @@ export function resolveCommand( const translatedArgs = translateArgsForWsl(args) // Why: env on wsl.exe stays Windows-side (WSLENV forwards only named vars), so the locale must ride the command string (issue #7808). - const localePrefix = command === 'git' ? `${GIT_OUTPUT_LOCALE_SHELL_PREFIX} ` : '' + const localePrefix = + command === 'git' + ? `${GIT_OUTPUT_LOCALE_SHELL_PREFIX} ${wslGitObjectStorePrefix(options.env)}` + : '' const escapedCommand = options.wslShellCommand ?? quotePosixShell(command) // Why: shell-escape each arg to prevent word splitting / glob expansion inside the bash -c string. const escapedArgs = translatedArgs.map(quotePosixShell) diff --git a/src/main/git/local-git-object-quarantine.test.ts b/src/main/git/local-git-object-quarantine.test.ts index f6101899e8ad..419342d2ddc7 100644 --- a/src/main/git/local-git-object-quarantine.test.ts +++ b/src/main/git/local-git-object-quarantine.test.ts @@ -4,7 +4,6 @@ import type { GitObjectQuarantine, GitObjectsDirectory } from '../../shared/git- import { createLocalGitObjectQuarantine, inheritedLocalObjectStore, - localGitObjectQuarantineProcessEnv, localGitObjectsDirectory } from './local-git-object-quarantine' import { @@ -39,33 +38,6 @@ vi.mock('../../shared/git-object-quarantine', async (importOriginal) => ({ }) })) -const QUARANTINE = { - GIT_OBJECT_DIRECTORY: '/home/me/repo/.git/objects/tmp_objdir-orca-merge-tree-x', - GIT_ALTERNATE_OBJECT_DIRECTORIES: '/home/me/repo/.git/objects' -} - -describe('localGitObjectQuarantineProcessEnv', () => { - it('forwards both variables through WSLENV on Windows, keeping existing entries', () => { - const env = localGitObjectQuarantineProcessEnv(QUARANTINE, 'win32', { - PATH: 'C:\\bin', - WSLENV: 'ORCA_X/u' - }) - - expect(env).toMatchObject({ PATH: 'C:\\bin', ...QUARANTINE }) - expect(env.WSLENV?.split(':')).toEqual([ - 'ORCA_X/u', - 'GIT_OBJECT_DIRECTORY', - 'GIT_ALTERNATE_OBJECT_DIRECTORIES' - ]) - }) - - it('leaves WSLENV alone off Windows', () => { - const env = localGitObjectQuarantineProcessEnv(QUARANTINE, 'darwin', { PATH: '/usr/bin' }) - - expect(env).toEqual({ PATH: '/usr/bin', ...QUARANTINE }) - }) -}) - describe('localGitObjectsDirectory', () => { it('translates a WSL Git common dir into a path the Windows host can open', () => { expect(localGitObjectsDirectory('/home/me/repo/.git', 'Ubuntu')).toEqual({ @@ -114,7 +86,7 @@ describe('createLocalGitObjectQuarantine on a Windows host', () => { const envSeenBy = (repoPath: string, options: { wslDistro?: string } = {}) => createLocalGitObjectQuarantine(repoPath, options).run(async (env) => env) - it('takes the distro from a WSL repo path and hands WSL Git the variables through WSLENV', async () => { + it('takes the distro from a WSL repo path', async () => { readRepoCommonDirFromGitMock.mockResolvedValue('/home/me/repo/.git') const env = await envSeenBy('\\\\wsl.localhost\\Ubuntu\\home\\me\\repo') @@ -123,9 +95,6 @@ describe('createLocalGitObjectQuarantine on a Windows host', () => { GIT_OBJECT_DIRECTORY: '/home/me/repo/.git/objects/scratch', GIT_ALTERNATE_OBJECT_DIRECTORIES: '/home/me/repo/.git/objects' }) - expect(env?.WSLENV?.split(':')).toEqual( - expect.arrayContaining(['GIT_OBJECT_DIRECTORY', 'GIT_ALTERNATE_OBJECT_DIRECTORIES']) - ) }) it('hands WSL Git the drvfs spelling for a drive-path repo', async () => { diff --git a/src/main/git/local-git-object-quarantine.ts b/src/main/git/local-git-object-quarantine.ts index 69b9fcdbc445..068709b9f774 100644 --- a/src/main/git/local-git-object-quarantine.ts +++ b/src/main/git/local-git-object-quarantine.ts @@ -4,10 +4,8 @@ import { createGitObjectQuarantine, inheritedObjectStore, pathApiForGitPath, - type GitObjectQuarantineEnv, type GitObjectsDirectory } from '../../shared/git-object-quarantine' -import { addWslEnvKeys } from '../../shared/wsl-env' import { toLinuxPath, toWindowsWslPath } from '../../shared/wsl-paths' import { parseWslPath } from '../wsl' import { usesHostGitForWslLinkedWorktree } from './wsl-linked-worktree-git-routing' @@ -35,19 +33,6 @@ export function localGitObjectsDirectory( } } -export function localGitObjectQuarantineProcessEnv( - quarantine: GitObjectQuarantineEnv, - platform: NodeJS.Platform = process.platform, - baseEnv: NodeJS.ProcessEnv = process.env -): NodeJS.ProcessEnv { - const env: NodeJS.ProcessEnv = { ...baseEnv, ...quarantine } - if (platform === 'win32') { - // Why: wsl.exe forwards only WSLENV-named vars; the values are already Linux paths, so no `/p`. - addWslEnvKeys(env, ['GIT_OBJECT_DIRECTORY', 'GIT_ALTERNATE_OBJECT_DIRECTORIES']) - } - return env -} - const OBJECT_STORE_VARIABLES = ['GIT_OBJECT_DIRECTORY', 'GIT_ALTERNATE_OBJECT_DIRECTORIES'] /** The object-store variables local Git would inherit, or undefined when they cannot be mirrored. */ @@ -91,6 +76,7 @@ export function createLocalGitObjectQuarantine( }) return { run: (command) => - quarantine.run((env) => command(env ? localGitObjectQuarantineProcessEnv(env) : undefined)) + // Why no WSLENV: the WSL runner puts both variables on WSL Git's command line. + quarantine.run((env) => command(env ? { ...process.env, ...env } : undefined)) } } From 240e830e81916b0262b7d5fd4ca99f8f6bdd8efe Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Thu, 15 Oct 2026 03:05:10 -0700 Subject: [PATCH 13/13] test(git): drop quarantine tests the real-Git and compatibility tests already pin The cold-capability gating, scratch-folder lifecycle and WSL path spelling are covered end to end by merge-tree-object-quarantine-real-git.test.ts, the real-binary compatibility contract (Git 2.25/2.38/2.49) and the localGitObjectsDirectory tests, so the unit copies only mirrored them. --- .../git/local-git-object-quarantine.test.ts | 19 ----- src/main/github/conflict-summary.test.ts | 35 -------- src/shared/git-branch-cleanup.test.ts | 50 ----------- src/shared/git-object-quarantine.test.ts | 85 ------------------- 4 files changed, 189 deletions(-) diff --git a/src/main/git/local-git-object-quarantine.test.ts b/src/main/git/local-git-object-quarantine.test.ts index 419342d2ddc7..0ef6758a0dbe 100644 --- a/src/main/git/local-git-object-quarantine.test.ts +++ b/src/main/git/local-git-object-quarantine.test.ts @@ -86,25 +86,6 @@ describe('createLocalGitObjectQuarantine on a Windows host', () => { const envSeenBy = (repoPath: string, options: { wslDistro?: string } = {}) => createLocalGitObjectQuarantine(repoPath, options).run(async (env) => env) - it('takes the distro from a WSL repo path', async () => { - readRepoCommonDirFromGitMock.mockResolvedValue('/home/me/repo/.git') - - const env = await envSeenBy('\\\\wsl.localhost\\Ubuntu\\home\\me\\repo') - - expect(env).toMatchObject({ - GIT_OBJECT_DIRECTORY: '/home/me/repo/.git/objects/scratch', - GIT_ALTERNATE_OBJECT_DIRECTORIES: '/home/me/repo/.git/objects' - }) - }) - - it('hands WSL Git the drvfs spelling for a drive-path repo', async () => { - readRepoCommonDirFromGitMock.mockResolvedValue('C:\\repo\\.git') - - const env = await envSeenBy('C:\\repo', { wslDistro: 'Ubuntu' }) - - expect(env?.GIT_OBJECT_DIRECTORY).toBe('/mnt/c/repo/.git/objects/scratch') - }) - it('runs unquarantined when a WSL-authored linked worktree routes to Windows Git', async () => { readRepoCommonDirFromGitMock.mockResolvedValue('/mnt/c/repo/.git') seedWslLinkedWorktreeGitRoutingForTests('C:\\wt') diff --git a/src/main/github/conflict-summary.test.ts b/src/main/github/conflict-summary.test.ts index a65d2b6e1df7..bbe212820b81 100644 --- a/src/main/github/conflict-summary.test.ts +++ b/src/main/github/conflict-summary.test.ts @@ -419,41 +419,6 @@ describe('getPRConflictSummary caching', () => { expect(gitExecFileAsyncMock.mock.calls.filter(([, options]) => options?.env)).toEqual([]) }) - it('runs the first merge-tree as Git always has, then the next ones against a scratch store', async () => { - const commonDir = join( - realpathSync(mkdtempSync(join(tmpdir(), 'orca-conflict-summary-'))), - '.git' - ) - mkdirSync(join(commonDir, 'objects'), { recursive: true }) - try { - mockGitDispatch({ [COMMON_DIR_COMMAND]: async () => ({ stdout: `${commonDir}\n` }) }) - - await expect(deriveSummary('head-oid-1')).resolves.toEqual(expectedSummary) - expect(spawnCount(COMMON_DIR_COMMAND)).toBe(0) - expect(scratchFolderPrefixes).toEqual([]) - await expect(deriveSummary('head-oid-2')).resolves.toEqual(expectedSummary) - - const envs = gitExecFileAsyncMock.mock.calls - .filter(([argv]) => argv[0] === 'merge-tree') - .map(([argv, options]) => [argv.includes('--merge-base'), options?.env]) - expect(envs).toEqual([ - [true, undefined], - [ - true, - expect.objectContaining({ - GIT_OBJECT_DIRECTORY: expect.stringContaining( - join(commonDir, 'objects', 'tmp_objdir-orca-merge-tree-') - ) - }) - ] - ]) - expect(scratchFolderPrefixes).toHaveLength(1) - expect(readdirSync(join(commonDir, 'objects'))).toEqual([]) - } finally { - rmSync(join(commonDir, '..'), { recursive: true, force: true }) - } - }) - it('runs the legacy merge-tree fallback against the scratch object store too', async () => { const commonDir = join( realpathSync(mkdtempSync(join(tmpdir(), 'orca-conflict-summary-'))), diff --git a/src/shared/git-branch-cleanup.test.ts b/src/shared/git-branch-cleanup.test.ts index 1cb948189f4a..5a35ba63a97c 100644 --- a/src/shared/git-branch-cleanup.test.ts +++ b/src/shared/git-branch-cleanup.test.ts @@ -116,56 +116,6 @@ describe('branchHasNoUnmergedChangesOnAnyTarget', () => { expect(new Set(discarding)).toEqual(new Set(['merge-tree'])) }) - it('runs the first merge-tree as Git always has, then the next ones against a scratch store', async () => { - const runGit = baseProofResponses() - - await branchHasNoUnmergedChangesOnAnyTarget( - runGit, - 'feature/test', - ['refs/remotes/origin/main'], - new GitCapabilityCache() - ) - - const mergeTrees = vi - .mocked(runGit) - .mock.calls.filter(([args]) => args[0] === 'merge-tree') - .map(([args, options]) => [args[2], options?.discardWrittenObjects]) - expect(mergeTrees).toEqual([ - ['target', false], - ['squash', true] - ]) - }) - - it('never asks for a scratch store from a Git that rejects merge-tree --write-tree', async () => { - // Git 2.25's exact rejection. - const rejected = Object.assign(new Error('Command failed: git merge-tree --write-tree'), { - code: 129, - stdout: '', - stderr: 'usage: git merge-tree \n' - }) - const runGit = baseProofResponses({ - 'merge-tree --write-tree target refs/heads/feature/test': rejected, - 'merge-tree --write-tree squash refs/heads/feature/test': rejected - }) - const capabilities = new GitCapabilityCache() - const check = () => - branchHasNoUnmergedChangesOnAnyTarget( - runGit, - 'feature/test', - ['refs/remotes/origin/main'], - capabilities - ) - - await expect(check()).resolves.toBe(false) - await expect(check()).resolves.toBe(false) - - const mergeTrees = vi - .mocked(runGit) - .mock.calls.filter(([args]) => args[0] === 'merge-tree') - .map(([, options]) => options?.discardWrittenObjects) - expect(mergeTrees).toEqual([false]) - }) - describe('a merge-tree that exits 1', () => { function exitOne(stdout: string): Error { return Object.assign(new Error('Command failed: git merge-tree --write-tree'), { diff --git a/src/shared/git-object-quarantine.test.ts b/src/shared/git-object-quarantine.test.ts index bccacc5dfaaa..ae07ae0012cc 100644 --- a/src/shared/git-object-quarantine.test.ts +++ b/src/shared/git-object-quarantine.test.ts @@ -15,7 +15,6 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { createGitObjectQuarantine, GIT_OBJECT_QUARANTINE_DIR_PREFIX, - inheritedObjectStore, type GitObjectQuarantineEnv } from './git-object-quarantine' @@ -59,37 +58,6 @@ describe('createGitObjectQuarantine', () => { const scratchDirs = (): string[] => readdirSync(objects).filter((entry) => entry.startsWith(GIT_OBJECT_QUARANTINE_DIR_PREFIX)) - it('points Git at a scratch dir inside objects/ and removes it afterwards', async () => { - let seen: GitObjectQuarantineEnv | undefined - - await expect( - createGitObjectQuarantine(resolveNative()).run(async (env) => { - seen = env - expect(scratchDirs()).toHaveLength(1) - return 'ok' - }) - ).resolves.toBe('ok') - - expect(seen).toEqual({ - GIT_OBJECT_DIRECTORY: expect.stringContaining( - join(objects, GIT_OBJECT_QUARANTINE_DIR_PREFIX) - ), - GIT_ALTERNATE_OBJECT_DIRECTORIES: objects - }) - expect(scratchDirs()).toEqual([]) - }) - - it('removes the scratch dir when the command fails', async () => { - const failure = new Error('merge-tree failed') - - await expect( - createGitObjectQuarantine(resolveNative()).run(async () => { - throw failure - }) - ).rejects.toBe(failure) - expect(scratchDirs()).toEqual([]) - }) - it.each([ ['the objects dir is unknown', async () => undefined], [ @@ -110,35 +78,6 @@ describe('createGitObjectQuarantine', () => { expect(command).toHaveBeenCalledWith(undefined) }) - it('resolves the objects dir once and gives every run its own scratch dir', async () => { - const resolve = vi.fn(resolveNative()) - const quarantine = createGitObjectQuarantine(resolve) - const seen: (string | undefined)[] = [] - - await quarantine.run(async (env) => seen.push(env?.GIT_OBJECT_DIRECTORY)) - await quarantine.run(async (env) => seen.push(env?.GIT_OBJECT_DIRECTORY)) - - expect(resolve).toHaveBeenCalledTimes(1) - expect(new Set(seen).size).toBe(2) - }) - - it('creates the scratch dir in host spelling but hands WSL Git its Linux spelling', async () => { - let seen: GitObjectQuarantineEnv | undefined - - await createGitObjectQuarantine(async () => ({ - hostPath: objects, - gitPath: '/home/me/repo/.git/objects' - })).run(async (env) => { - seen = env - expect(scratchDirs()).toHaveLength(1) - }) - - expect(seen?.GIT_OBJECT_DIRECTORY).toMatch( - new RegExp(`^/home/me/repo/\\.git/objects/${GIT_OBJECT_QUARANTINE_DIR_PREFIX}\\w+$`) - ) - expect(seen?.GIT_ALTERNATE_OBJECT_DIRECTORIES).toBe('/home/me/repo/.git/objects') - }) - it.each([ ['/plain/objects', '/plain/objects'], ['/odd:dir/objects', '"/odd:dir/objects"'], @@ -207,17 +146,6 @@ describe('createGitObjectQuarantine', () => { expect(scratchDirs()).toEqual([]) }) - it('quarantines a store whose info/ holds no alternates file', async () => { - writeFileSync(join(objects, 'info'), 'not a directory') - const command = vi.fn(async () => 'ok') - - await createGitObjectQuarantine(resolveNative()).run(command) - - expect(command).toHaveBeenCalledWith( - expect.objectContaining({ GIT_ALTERNATE_OBJECT_DIRECTORIES: objects }) - ) - }) - it('moves packs Git fetched into the scratch dir to the real store, index last', async () => { await createGitObjectQuarantine(resolveNative()).run(async (env) => { const scratchPack = join(env?.GIT_OBJECT_DIRECTORY ?? '', 'pack') @@ -282,16 +210,3 @@ describe('createGitObjectQuarantine', () => { ) }) }) - -describe('inheritedObjectStore', () => { - it('passes inherited alternates on, so the quarantine can append to them', () => { - expect(inheritedObjectStore({ GIT_ALTERNATE_OBJECT_DIRECTORIES: '/shared' })).toEqual({ - inheritedAlternates: '/shared' - }) - expect(inheritedObjectStore({})).toEqual({}) - }) - - it('refuses an inherited object dir, which is the store Git reads and writes', () => { - expect(inheritedObjectStore({ GIT_OBJECT_DIRECTORY: '/elsewhere/objects' })).toBeUndefined() - }) -})