diff --git a/src/main/git/repo-default-remote.test.ts b/src/main/git/repo-default-remote.test.ts index 947cb90f233..197ad5a3fbc 100644 --- a/src/main/git/repo-default-remote.test.ts +++ b/src/main/git/repo-default-remote.test.ts @@ -71,4 +71,42 @@ describe('getDefaultRemote', () => { 'Failed to resolve default remote for repo.' ) }) + + it('keeps configured branch precedence when a remote list is already available', async () => { + gitExecFileAsyncMock.mockImplementation(async (argv: string[]) => { + if (argv[0] === 'for-each-ref') { + return { stdout: 'refs/remotes/origin/HEAD\0refs/remotes/origin/main\n' } + } + if (argv[0] === 'config') { + return { stdout: 'upstream\n' } + } + throw new Error('unexpected command') + }) + + await expect(getDefaultRemote('/repo', {}, ['origin', 'upstream'])).resolves.toBe('upstream') + expect(gitExecFileAsyncMock.mock.calls.some(([args]) => args[0] === 'remote')).toBe(false) + }) + + it.each([ + [['origin', 'upstream'], 'origin'], + [['company'], 'company'] + ])('reuses known remotes %j when no default ref resolves', async (remotes, expected) => { + gitExecFileAsyncMock.mockRejectedValue(new Error('missing ref')) + + await expect(getDefaultRemote('/repo', {}, remotes)).resolves.toBe(expected) + expect(gitExecFileAsyncMock.mock.calls.some(([args]) => args[0] === 'remote')).toBe(false) + }) + + it.each([ + [[], 'Repo has no configured git remotes.'], + [ + ['upstream', 'fork'], + 'Repo has multiple remotes (upstream, fork) and no default is configured. Set branch..remote.' + ] + ])('preserves errors for known remotes %j', async (remotes, message) => { + gitExecFileAsyncMock.mockRejectedValue(new Error('missing ref')) + + await expect(getDefaultRemote('/repo', {}, remotes)).rejects.toThrow(message) + expect(gitExecFileAsyncMock.mock.calls.some(([args]) => args[0] === 'remote')).toBe(false) + }) }) diff --git a/src/main/git/repo.ts b/src/main/git/repo.ts index 392b5651818..da64c013efb 100644 --- a/src/main/git/repo.ts +++ b/src/main/git/repo.ts @@ -125,7 +125,8 @@ export async function getRemoteCount(path: string): Promise { /** Resolve the configured push remote without assuming a provider. */ export async function getDefaultRemote( path: string, - options: LocalGitExecOptions = {} + options: LocalGitExecOptions = {}, + knownRemoteNames?: readonly string[] ): Promise { const defaultRef = await getDefaultBaseRefAsync(path, options) const defaultBranch = defaultRef @@ -150,11 +151,14 @@ export async function getDefaultRemote( } try { - const { stdout } = await gitExecFileAsync(['remote'], gitExecOptions(path, options)) - const remotes = stdout - .split('\n') - .map((line) => line.trim()) - .filter(Boolean) + let remotes = knownRemoteNames + if (remotes === undefined) { + const { stdout } = await gitExecFileAsync(['remote'], gitExecOptions(path, options)) + remotes = stdout + .split('\n') + .map((line) => line.trim()) + .filter(Boolean) + } if (remotes.includes('origin')) { return 'origin' } diff --git a/src/main/git/worktree-list-reader-cancellation.test.ts b/src/main/git/worktree-list-reader-cancellation.test.ts new file mode 100644 index 00000000000..d5a46730e5d --- /dev/null +++ b/src/main/git/worktree-list-reader-cancellation.test.ts @@ -0,0 +1,156 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import type * as FsPromises from 'node:fs/promises' +import type { GitWorktreeInfo } from '../../shared/worktree/types' +import type { gitExecFileAsync } from './runner' + +const { statProbe, gitExec } = vi.hoisted(() => ({ + statProbe: vi.fn<(worktreePath: string) => Promise>(), + gitExec: vi.fn() +})) + +vi.mock('node:fs/promises', async (importOriginal) => ({ + ...(await importOriginal()), + stat: statProbe +})) +vi.mock('./runner', () => ({ + gitExecFileAsync: gitExec, + translateWslOutputPaths: (output: string) => output +})) +vi.mock('../../shared/git-worktree-admin', () => ({ + annotateWorktreeLocksFromAdmin: async (_repoPath: string, rows: GitWorktreeInfo[]) => rows +})) + +import { clearGitCapabilityStateForTests } from './git-capability-state' +import { readWorktreeList } from './worktree-list-reader' + +function porcelainRow(worktreePath: string, markers: string[] = []): string { + return [ + `worktree ${worktreePath}`, + `HEAD ${'a'.repeat(40)}`, + 'branch refs/heads/main', + ...markers, + '', + '' + ].join('\n') +} + +let porcelain = '' +function nextTurn(): Promise { + return new Promise((resolve) => setImmediate(resolve)) +} + +beforeEach(() => { + clearGitCapabilityStateForTests() + statProbe.mockReset() + gitExec.mockReset() + porcelain = + porcelainRow('/repo') + + Array.from({ length: 32 }, (_, index) => porcelainRow(`/repo/task-${index}`)).join('') + gitExec.mockImplementation(async (args) => { + if (args.includes('-z')) { + throw Object.assign(new Error("unknown switch `z'"), { stderr: "error: unknown switch `z'" }) + } + return { stdout: porcelain, stderr: '' } + }) +}) + +describe('old-Git native worktree listing cancellation', () => { + it('rejects before pending existence probes finish and prevents further probes', async () => { + const controller = new AbortController() + const reason = new Error('Listing closed') + const releases: (() => void)[] = [] + statProbe.mockImplementation(() => new Promise((resolve) => releases.push(resolve))) + const outcome = vi.fn<(result: unknown) => void>() + const observed = readWorktreeList('/repo', { signal: controller.signal }).then( + (result) => outcome(result), + (error: unknown) => outcome(error) + ) + try { + await nextTurn() + expect(gitExec.mock.calls.map(([args]) => args)).toEqual([ + ['worktree', 'list', '--porcelain', '-z'], + ['worktree', 'list', '--porcelain'] + ]) + expect(statProbe).toHaveBeenCalledTimes(8) + controller.abort(reason) + await nextTurn() + expect(outcome).toHaveBeenCalledExactlyOnceWith(reason) + expect(statProbe).toHaveBeenCalledTimes(8) + } finally { + releases.splice(0).forEach((release) => release()) + } + await observed + await nextTurn() + expect(outcome).toHaveBeenCalledExactlyOnceWith(reason) + expect(statProbe).toHaveBeenCalledTimes(8) + }) + + it('starts no existence probes when the request is already aborted', async () => { + const controller = new AbortController() + const reason = new Error('Already closed') + controller.abort(reason) + await expect(readWorktreeList('/repo', { signal: controller.signal })).rejects.toBe(reason) + await nextTurn() + expect(statProbe).not.toHaveBeenCalled() + }) + + it('handles synchronous cancellation by the first existence probe without an unhandled rejection', async () => { + const controller = new AbortController() + const reason = new Error('First probe closed the request') + const releases: (() => void)[] = [] + const unhandled: unknown[] = [] + const onUnhandled = (error: unknown): void => { + unhandled.push(error) + } + process.on('unhandledRejection', onUnhandled) + statProbe.mockImplementation(() => { + controller.abort(reason) + return new Promise((resolve) => releases.push(resolve)) + }) + try { + await expect(readWorktreeList('/repo', { signal: controller.signal })).rejects.toBe(reason) + expect(statProbe).toHaveBeenCalledTimes(1) + releases.splice(0).forEach((release) => release()) + await nextTurn() + expect(statProbe).toHaveBeenCalledTimes(1) + expect(unhandled).toEqual([]) + } finally { + releases.splice(0).forEach((release) => release()) + process.off('unhandledRejection', onUnhandled) + } + }) + + it('keeps lock and prunable protections and treats only ENOENT as absence', async () => { + porcelain = [ + porcelainRow('/repo'), + porcelainRow('/repo/bare', ['bare']), + porcelainRow('/repo/locked', ['locked agent session']), + porcelainRow('/repo/prunable', ['prunable missing directory']), + porcelainRow('/repo/missing'), + porcelainRow('/repo/denied'), + porcelainRow('/repo/live') + ].join('') + statProbe.mockImplementation(async (worktreePath) => { + if (worktreePath.endsWith('/missing')) { + throw Object.assign(new Error('missing'), { code: 'ENOENT' }) + } + if (worktreePath.endsWith('/denied')) { + throw Object.assign(new Error('denied'), { code: 'EACCES' }) + } + }) + const result = await readWorktreeList('/repo') + expect(statProbe.mock.calls.map(([worktreePath]) => worktreePath)).toEqual([ + '/repo/missing', + '/repo/denied', + '/repo/live' + ]) + expect(result.filter((row) => row.prunable).map((row) => row.path)).toEqual([ + '/repo/prunable', + '/repo/missing' + ]) + expect(result.find((row) => row.path === '/repo/locked')).toMatchObject({ + locked: true, + lockReason: 'agent session' + }) + }) +}) diff --git a/src/main/git/worktree-list-reader.ts b/src/main/git/worktree-list-reader.ts index ba75f31d837..d6bf5c2b2a3 100644 --- a/src/main/git/worktree-list-reader.ts +++ b/src/main/git/worktree-list-reader.ts @@ -1,3 +1,4 @@ +import { throwIfSignalAborted, waitForPromiseWithSignal } from '../../shared/abort-signal-reason' import { annotateWorktreeLocksFromAdmin } from '../../shared/git-worktree-admin' import { stat } from 'node:fs/promises' import type { GitWorktreeInfo } from '../../shared/worktree/types' @@ -263,6 +264,7 @@ async function annotatePrunableByExistence( async function probeNext(): Promise { while (nextIndex < worktrees.length) { + throwIfSignalAborted(options.signal) const index = nextIndex nextIndex += 1 const worktree = worktrees[index] @@ -287,7 +289,11 @@ async function annotatePrunableByExistence( } const workerCount = Math.min(PRUNABLE_EXISTENCE_PROBE_CONCURRENCY, worktrees.length) - await Promise.all(Array.from({ length: workerCount }, () => probeNext())) + await waitForPromiseWithSignal( + Promise.all(Array.from({ length: workerCount }, () => probeNext())), + options.signal + ) + throwIfSignalAborted(options.signal) return annotated } diff --git a/src/main/git/worktree-listing-sparse-cancellation.test.ts b/src/main/git/worktree-listing-sparse-cancellation.test.ts new file mode 100644 index 00000000000..7749ae99cda --- /dev/null +++ b/src/main/git/worktree-listing-sparse-cancellation.test.ts @@ -0,0 +1,140 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import type { GitWorktreeInfo } from '../../shared/worktree/types' +import type { GitWorktreeExecOptions } from './worktree-operation-options' + +const { sparseProbe } = vi.hoisted(() => ({ + sparseProbe: + vi.fn< + (repoPath: string, worktreePath: string, options?: GitWorktreeExecOptions) => Promise + >() +})) + +vi.mock('./worktree-sparse-checkout-cache', () => ({ + detectSparseCheckoutCached: sparseProbe +})) +vi.mock('./worktree-list-reader', () => ({ + readCheckedOutBranchRef: vi.fn(), + readRepoCommonDirFromGit: vi.fn(), + readRepoLocation: vi.fn(), + readTranslatedWorktreeGraph: vi.fn(), + readWorktreeHeadOid: vi.fn(), + readWorktreeList: vi.fn() +})) + +import { annotateSparseCheckoutStatus } from './worktree-listing' + +function listedWorktree(index: number): GitWorktreeInfo { + return { + path: `/repo/task-${index}`, + head: 'a'.repeat(40), + branch: `refs/heads/task-${index}`, + isBare: false, + isMainWorktree: false + } +} + +function nextTurn(): Promise { + return new Promise((resolve) => setImmediate(resolve)) +} + +beforeEach(() => { + sparseProbe.mockReset() +}) + +describe('sparse worktree listing cancellation', () => { + it('rejects while eight probes are pending and claims no more rows after they settle', async () => { + const rows = Array.from({ length: 32 }, (_, index) => listedWorktree(index)) + const controller = new AbortController() + const reason = new Error('Listing closed') + const releases: (() => void)[] = [] + sparseProbe.mockImplementation( + () => new Promise((resolve) => releases.push(() => resolve(true))) + ) + const outcome = vi.fn<(result: unknown) => void>() + const observed = annotateSparseCheckoutStatus('/repo', rows, { + signal: controller.signal, + wslDistro: 'Ubuntu' + }).then( + (result) => outcome(result), + (error: unknown) => outcome(error) + ) + + try { + await nextTurn() + expect(sparseProbe).toHaveBeenCalledTimes(8) + expect(sparseProbe).toHaveBeenCalledWith('/repo', rows[0]?.path, { + signal: controller.signal, + wslDistro: 'Ubuntu' + }) + controller.abort(reason) + await nextTurn() + expect(outcome).toHaveBeenCalledExactlyOnceWith(reason) + expect(sparseProbe).toHaveBeenCalledTimes(8) + } finally { + releases.splice(0).forEach((release) => release()) + } + await observed + await nextTurn() + expect(sparseProbe).toHaveBeenCalledTimes(8) + expect(outcome).toHaveBeenCalledExactlyOnceWith(reason) + expect(rows.every((row) => row.isSparse === undefined)).toBe(true) + }) + + it('starts no probes for a pre-aborted request, including an empty listing', async () => { + const controller = new AbortController() + const reason = new Error('Already closed') + controller.abort(reason) + for (const rows of [[listedWorktree(0)], []]) { + await expect( + annotateSparseCheckoutStatus('/repo', rows, { signal: controller.signal }) + ).rejects.toBe(reason) + } + await nextTurn() + expect(sparseProbe).not.toHaveBeenCalled() + }) + + it('observes worker rejections when the first probe synchronously aborts the request', async () => { + const controller = new AbortController() + const reason = new Error('First probe closed the request') + const releases: (() => void)[] = [] + const unhandled: unknown[] = [] + const onUnhandled = (error: unknown): void => { + unhandled.push(error) + } + process.on('unhandledRejection', onUnhandled) + sparseProbe.mockImplementation(() => { + controller.abort(reason) + return new Promise((resolve) => releases.push(() => resolve(true))) + }) + try { + await expect( + annotateSparseCheckoutStatus( + '/repo', + Array.from({ length: 32 }, (_, index) => listedWorktree(index)), + { signal: controller.signal } + ) + ).rejects.toBe(reason) + expect(sparseProbe).toHaveBeenCalledTimes(1) + releases.splice(0).forEach((release) => release()) + await nextTurn() + expect(sparseProbe).toHaveBeenCalledTimes(1) + expect(unhandled).toEqual([]) + } finally { + releases.splice(0).forEach((release) => release()) + process.off('unhandledRejection', onUnhandled) + } + }) + + it('preserves existing sparse and bare rows during a successful listing', async () => { + const rows = [ + { ...listedWorktree(0), isBare: true }, + { ...listedWorktree(1), isSparse: true }, + listedWorktree(2) + ] + sparseProbe.mockResolvedValue(true) + const result = await annotateSparseCheckoutStatus('/repo', rows) + expect(sparseProbe).toHaveBeenCalledExactlyOnceWith('/repo', rows[2]?.path, {}) + expect(result).toEqual([rows[0], rows[1], { ...rows[2], isSparse: true }]) + expect(rows[2]?.isSparse).toBeUndefined() + }) +}) diff --git a/src/main/git/worktree-listing.ts b/src/main/git/worktree-listing.ts index e902a89a957..5dd1a4e29c7 100644 --- a/src/main/git/worktree-listing.ts +++ b/src/main/git/worktree-listing.ts @@ -1,3 +1,4 @@ +import { throwIfSignalAborted, waitForPromiseWithSignal } from '../../shared/abort-signal-reason' import { readFile, realpath, stat } from 'node:fs/promises' import { join, posix } from 'node:path' import { isDefinitiveAbsence } from '../../shared/definitive-filesystem-absence' @@ -128,6 +129,7 @@ export async function annotateSparseCheckoutStatus( async function detectNext(): Promise { while (nextIndex < worktrees.length) { + throwIfSignalAborted(options.signal) const index = nextIndex nextIndex += 1 const worktree = worktrees[index] @@ -135,6 +137,7 @@ export async function annotateSparseCheckoutStatus( continue } const isSparse = await detectSparseCheckoutCached(repoPath, worktree.path, options) + throwIfSignalAborted(options.signal) if (isSparse) { annotated[index] = { ...worktree, isSparse } } @@ -143,7 +146,11 @@ export async function annotateSparseCheckoutStatus( // Why: cap concurrency so status-poll refreshes don't fan out many sparse-checkout filesystem probes at once. const workerCount = Math.min(SPARSE_CHECKOUT_DETECTION_CONCURRENCY, worktrees.length) - await Promise.all(Array.from({ length: workerCount }, () => detectNext())) + await waitForPromiseWithSignal( + Promise.all(Array.from({ length: workerCount }, () => detectNext())), + options.signal + ) + throwIfSignalAborted(options.signal) return annotated } diff --git a/src/main/github/review-head-remote.test.ts b/src/main/github/review-head-remote.test.ts index 90e4203f14e..1af2bec6cf1 100644 --- a/src/main/github/review-head-remote.test.ts +++ b/src/main/github/review-head-remote.test.ts @@ -98,7 +98,7 @@ describe('resolveGitHubReviewHeadRemote', () => { expect(remote).toBe('origin') expect(getGitHubApiRepositoryForRemoteMock).not.toHaveBeenCalled() - expect(getDefaultRemoteMock).toHaveBeenCalledWith('/repo', { wslDistro: 'Ubuntu' }) + expect(getDefaultRemoteMock).toHaveBeenCalledWith('/repo', { wslDistro: 'Ubuntu' }, ['origin']) }) it('prefers origin over other remotes on SSH repos when no identity resolves', async () => { diff --git a/src/main/github/review-head-remote.ts b/src/main/github/review-head-remote.ts index eabd04f86cc..e5b204812ee 100644 --- a/src/main/github/review-head-remote.ts +++ b/src/main/github/review-head-remote.ts @@ -49,5 +49,5 @@ export async function resolveGitHubReviewHeadRemote(args: { if (args.connectionId) { return pickPreferredGitRemote(remotes) } - return getDefaultRemote(args.repoPath, args.localGitOptions ?? {}) + return getDefaultRemote(args.repoPath, args.localGitOptions ?? {}, remotes) } diff --git a/src/main/ipc/worktrees-wsl-runtime-routing.test.ts b/src/main/ipc/worktrees-wsl-runtime-routing.test.ts index 029ffc6171a..3877cc12030 100644 --- a/src/main/ipc/worktrees-wsl-runtime-routing.test.ts +++ b/src/main/ipc/worktrees-wsl-runtime-routing.test.ts @@ -462,7 +462,11 @@ describe('registerWorktreeHandlers', () => { ['rev-parse', '--verify', 'origin/feature/add-feature'], { cwd: '/workspace/repo', wslDistro: 'Ubuntu' } ) - expect(getDefaultRemoteMock).toHaveBeenCalledWith('/workspace/repo', { wslDistro: 'Ubuntu' }) + expect(getDefaultRemoteMock).toHaveBeenCalledWith( + '/workspace/repo', + { wslDistro: 'Ubuntu' }, + [] + ) expect(result).toMatchObject({ baseBranch: 'def456', headSha: 'def456', diff --git a/src/relay/git-handler-worktree-existence-cancellation.test.ts b/src/relay/git-handler-worktree-existence-cancellation.test.ts new file mode 100644 index 00000000000..1cf07e20913 --- /dev/null +++ b/src/relay/git-handler-worktree-existence-cancellation.test.ts @@ -0,0 +1,108 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import type * as FsPromises from 'node:fs/promises' +import type { GitWorktreeInfo } from '../shared/worktree/types' + +const { statProbe } = vi.hoisted(() => ({ + statProbe: vi.fn<(worktreePath: string) => Promise>() +})) +vi.mock('node:fs/promises', async (importOriginal) => ({ + ...(await importOriginal()), + stat: statProbe +})) +vi.mock('../shared/git-worktree-admin', () => ({ + annotateWorktreeLocksFromAdmin: vi.fn() +})) + +import { annotatePrunableWorktreesByExistence } from './git-handler-worktree-list' + +function listedWorktree(index: number): GitWorktreeInfo { + return { + path: `/remote/repo/task-${index}`, + head: 'a'.repeat(40), + branch: `refs/heads/task-${index}`, + isBare: false, + isMainWorktree: false + } +} + +function nextTurn(): Promise { + return new Promise((resolve) => setImmediate(resolve)) +} + +beforeEach(() => { + statProbe.mockReset() +}) + +describe('relay worktree existence cancellation', () => { + it('rejects before eight pending stats finish and starts no more after they settle', async () => { + const rows = Array.from({ length: 32 }, (_, index) => listedWorktree(index)) + const controller = new AbortController() + const reason = new Error('Remote listing closed') + const releases: (() => void)[] = [] + statProbe.mockImplementation(() => new Promise((resolve) => releases.push(resolve))) + const outcome = vi.fn<(result: unknown) => void>() + const observed = annotatePrunableWorktreesByExistence(rows, controller.signal).then( + (result) => outcome(result), + (error: unknown) => outcome(error) + ) + try { + await nextTurn() + expect(statProbe).toHaveBeenCalledTimes(8) + controller.abort(reason) + await nextTurn() + expect(outcome).toHaveBeenCalledExactlyOnceWith(reason) + expect(statProbe).toHaveBeenCalledTimes(8) + } finally { + releases.splice(0).forEach((release) => release()) + } + await observed + await nextTurn() + expect(statProbe).toHaveBeenCalledTimes(8) + expect(outcome).toHaveBeenCalledExactlyOnceWith(reason) + expect(rows.every((row) => row.prunable === undefined)).toBe(true) + }) + + it('starts no stats for an already-aborted request, including an empty catalog', async () => { + const controller = new AbortController() + const reason = new Error('Already closed') + controller.abort(reason) + for (const rows of [[listedWorktree(0)], []]) { + await expect(annotatePrunableWorktreesByExistence(rows, controller.signal)).rejects.toBe( + reason + ) + } + await nextTurn() + expect(statProbe).not.toHaveBeenCalled() + }) + + it('handles synchronous abort in the first stat without abandoning rejected workers', async () => { + const controller = new AbortController() + const reason = new Error('First probe closed the request') + const releases: (() => void)[] = [] + const unhandled: unknown[] = [] + const onUnhandled = (error: unknown): void => { + unhandled.push(error) + } + process.on('unhandledRejection', onUnhandled) + statProbe.mockImplementation(() => { + controller.abort(reason) + return new Promise((resolve) => releases.push(resolve)) + }) + try { + await expect( + annotatePrunableWorktreesByExistence( + Array.from({ length: 32 }, (_, index) => listedWorktree(index)), + controller.signal + ) + ).rejects.toBe(reason) + expect(statProbe).toHaveBeenCalledTimes(1) + releases.splice(0).forEach((release) => release()) + await nextTurn() + expect(statProbe).toHaveBeenCalledTimes(1) + expect(unhandled).toEqual([]) + } finally { + releases.splice(0).forEach((release) => release()) + process.off('unhandledRejection', onUnhandled) + } + }) +}) diff --git a/src/relay/git-handler-worktree-list-cancellation.test.ts b/src/relay/git-handler-worktree-list-cancellation.test.ts new file mode 100644 index 00000000000..9297e3984c8 --- /dev/null +++ b/src/relay/git-handler-worktree-list-cancellation.test.ts @@ -0,0 +1,209 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import type * as FsPromises from 'node:fs/promises' +import type { GitWorktreeInfo } from '../shared/worktree/types' +import type { GitHandlerOperationHost } from './git-handler-operation-context' +import { createGitHandlerRelay } from './git-handler-test-harness' + +const { statProbe, annotateLocks } = vi.hoisted(() => ({ + statProbe: vi.fn<(worktreePath: string) => Promise>(), + annotateLocks: + vi.fn< + ( + repoPath: string, + rows: GitWorktreeInfo[], + options?: { signal?: AbortSignal } + ) => Promise + >() +})) +vi.mock('node:fs/promises', async (importOriginal) => ({ + ...(await importOriginal()), + stat: statProbe +})) +vi.mock('../shared/git-worktree-admin', () => ({ + annotateWorktreeLocksFromAdmin: annotateLocks +})) + +function porcelainRow(worktreePath: string): string { + return `worktree ${worktreePath}\nHEAD ${'a'.repeat(40)}\nbranch refs/heads/main\n\n` +} + +function unsupportedZError(): Error { + return Object.assign(new Error('git usage error'), { + code: 129, + stderr: 'usage: git worktree list []\n' + }) +} + +function unsupportedPathFormatError(): Error { + return Object.assign(new Error('unsupported path format'), { + stderr: 'error: unknown option `path-format=absolute`\n' + }) +} + +function nextTurn(): Promise { + return new Promise((resolve) => setImmediate(resolve)) +} + +let relay: ReturnType + +beforeEach(() => { + relay = createGitHandlerRelay() + statProbe.mockReset() + annotateLocks.mockReset().mockImplementation(async (_repoPath, rows) => rows) +}) +afterEach(() => relay.handler.dispose()) + +describe('relay worktree listing request cancellation', () => { + it('forwards the dispatcher signal through the old-Git existence fallback', async () => { + const controller = new AbortController() + const reason = new Error('Remote request closed') + const porcelain = + porcelainRow('/remote/repo') + + Array.from({ length: 32 }, (_, index) => porcelainRow(`/remote/repo/task-${index}`)).join('') + const git = vi.fn(async (args) => { + if (args.includes('-z')) { + throw unsupportedZError() + } + return { stdout: porcelain, stderr: '' } + }) + Object.assign(relay.handler, { git }) + const releases: (() => void)[] = [] + statProbe.mockImplementation(() => new Promise((resolve) => releases.push(resolve))) + const outcome = vi.fn<(result: unknown) => void>() + const observed = relay.dispatcher + .callRequest( + 'git.listWorktrees', + { repoPath: '/remote/repo' }, + { isStale: () => false, signal: controller.signal } + ) + .then( + (result) => outcome(result), + (error: unknown) => outcome(error) + ) + try { + await nextTurn() + expect(statProbe).toHaveBeenCalledTimes(8) + expect(git.mock.calls.map(([, , options]) => options?.signal)).toEqual([ + controller.signal, + controller.signal + ]) + expect(annotateLocks).toHaveBeenCalledWith('/remote/repo', expect.any(Array), { + signal: controller.signal + }) + controller.abort(reason) + await nextTurn() + expect(outcome).toHaveBeenCalledExactlyOnceWith(reason) + } finally { + releases.splice(0).forEach((release) => release()) + } + await observed + await nextTurn() + expect(statProbe).toHaveBeenCalledTimes(8) + expect(outcome).toHaveBeenCalledExactlyOnceWith(reason) + }) + + it.each(['preferred', 'fallback'] as const)( + 'preserves cancellation from the %s normalization command', + async (mode) => { + const controller = new AbortController() + const reason = new Error('Normalization canceled') + const git = vi.fn(async (args, _cwd, options) => { + expect(options?.signal).toBe(controller.signal) + if (args[0] === 'worktree') { + return { stdout: porcelainRow('/remote/git-store'), stderr: '' } + } + if (mode === 'fallback' && args.includes('--path-format=absolute')) { + throw unsupportedPathFormatError() + } + controller.abort(reason) + throw reason + }) + Object.assign(relay.handler, { git }) + await expect( + relay.dispatcher.callRequest( + 'git.listWorktrees', + { repoPath: '/remote/repo' }, + { isStale: () => false, signal: controller.signal } + ) + ).rejects.toBe(reason) + const locationCommands = git.mock.calls + .filter(([args]) => args[0] === 'rev-parse') + .map(([args]) => args) + expect(locationCommands).toEqual( + mode === 'preferred' + ? [ + [ + 'rev-parse', + '--path-format=absolute', + '--show-toplevel', + '--git-common-dir', + '--git-dir' + ] + ] + : [ + [ + 'rev-parse', + '--path-format=absolute', + '--show-toplevel', + '--git-common-dir', + '--git-dir' + ], + ['rev-parse', '--show-toplevel', '--git-common-dir', '--git-dir'] + ] + ) + expect(statProbe).not.toHaveBeenCalled() + } + ) + + it.each(['preferred', 'fallback'] as const)( + 'keeps separate-git-dir normalization working with the %s command', + async (mode) => { + const controller = new AbortController() + const git = vi.fn(async (args, _cwd, options) => { + expect(options?.signal).toBe(controller.signal) + if (args[0] === 'worktree') { + return { stdout: porcelainRow('/remote/git-store'), stderr: '' } + } + if (mode === 'fallback' && args.includes('--path-format=absolute')) { + throw unsupportedPathFormatError() + } + return { stdout: '/remote/repo\n/remote/git-store\n/remote/git-store\n', stderr: '' } + }) + Object.assign(relay.handler, { git }) + await expect( + relay.dispatcher.callRequest( + 'git.listWorktrees', + { repoPath: '/remote/repo' }, + { isStale: () => false, signal: controller.signal } + ) + ).resolves.toEqual([ + expect.objectContaining({ + path: '/remote/repo', + isMainWorktree: true + }) + ]) + expect(statProbe).not.toHaveBeenCalled() + } + ) + + it('rejects a pre-aborted dispatcher request before normalization or existence probes', async () => { + const controller = new AbortController() + const reason = new Error('Already closed') + controller.abort(reason) + const git = vi.fn(async () => ({ + stdout: porcelainRow('/remote/git-store'), + stderr: '' + })) + Object.assign(relay.handler, { git }) + await expect( + relay.dispatcher.callRequest( + 'git.listWorktrees', + { repoPath: '/remote/repo' }, + { isStale: () => false, signal: controller.signal } + ) + ).rejects.toBe(reason) + expect(git).toHaveBeenCalledOnce() + expect(annotateLocks).not.toHaveBeenCalled() + expect(statProbe).not.toHaveBeenCalled() + }) +}) diff --git a/src/relay/git-handler-worktree-list.ts b/src/relay/git-handler-worktree-list.ts index fe784be8d4e..53c034d1593 100644 --- a/src/relay/git-handler-worktree-list.ts +++ b/src/relay/git-handler-worktree-list.ts @@ -1,3 +1,4 @@ +import { throwIfSignalAborted, waitForPromiseWithSignal } from '../shared/abort-signal-reason' import { annotateWorktreeLocksFromAdmin } from '../shared/git-worktree-admin' import { expandTilde } from './context' import { stat } from 'node:fs/promises' @@ -46,21 +47,20 @@ const PRUNABLE_EXISTENCE_PROBE_CONCURRENCY = 8 * harmless backstop. The relay owns the filesystem, so a plain stat is * authoritative. */ export async function annotatePrunableWorktreesByExistence( - worktrees: GitWorktreeInfo[] + worktrees: GitWorktreeInfo[], + signal?: AbortSignal ): Promise { const annotated = [...worktrees] let nextIndex = 0 async function probeNext(): Promise { while (nextIndex < worktrees.length) { + throwIfSignalAborted(signal) const index = nextIndex nextIndex += 1 const worktree = worktrees[index] const worktreePath = worktree?.path ?? '' - // Git only marks linked worktrees prunable, and never locked ones (a - // lock shields the registration even when the directory is missing). The - // Older Git locks are annotated from the host admin directory. A missing main - // worktree is surfaced by the repo-level failure paths. + // Locks protect missing linked worktrees; repository failures own the main row. if ( !worktreePath || worktree.isMainWorktree === true || @@ -73,7 +73,7 @@ export async function annotatePrunableWorktreesByExistence( try { await stat(worktreePath) } catch (err) { - if ((err as NodeJS.ErrnoException | undefined)?.code === 'ENOENT') { + if (typeof err === 'object' && err !== null && 'code' in err && err.code === 'ENOENT') { annotated[index] = { ...worktree, prunable: true } } } @@ -81,7 +81,11 @@ export async function annotatePrunableWorktreesByExistence( } const workerCount = Math.min(PRUNABLE_EXISTENCE_PROBE_CONCURRENCY, worktrees.length) - await Promise.all(Array.from({ length: workerCount }, () => probeNext())) + await waitForPromiseWithSignal( + Promise.all(Array.from({ length: workerCount }, () => probeNext())), + signal + ) + throwIfSignalAborted(signal) return annotated } diff --git a/src/relay/git-handler-worktree-operations.ts b/src/relay/git-handler-worktree-operations.ts index 51b57c742d2..80948e5f608 100644 --- a/src/relay/git-handler-worktree-operations.ts +++ b/src/relay/git-handler-worktree-operations.ts @@ -1,3 +1,4 @@ +import { throwIfSignalAborted } from '../shared/abort-signal-reason' import { annotateWorktreeLocksFromAdmin } from '../shared/git-worktree-admin' import * as path from 'node:path' import type { RequestContext } from './dispatcher' @@ -67,7 +68,10 @@ export class GitHandlerWorktreeOperations extends GitHandlerOperationContext { } } - private async readRepoLocation(repoPath: string): Promise { + private async readRepoLocation( + repoPath: string, + signal?: AbortSignal + ): Promise { try { return await this.gitCapabilities.runWithFallback( 'rev-parse-path-format', @@ -80,7 +84,8 @@ export class GitHandlerWorktreeOperations extends GitHandlerOperationContext { '--git-common-dir', '--git-dir' ], - repoPath + repoPath, + { signal } ) if (hasUnsupportedRevParsePathFormatEcho(stdout)) { // Why: old Git echoes the unknown option and exits zero; remember the signal though the paths still parse. @@ -91,21 +96,25 @@ export class GitHandlerWorktreeOperations extends GitHandlerOperationContext { async () => { const { stdout } = await this.git( ['rev-parse', '--show-toplevel', '--git-common-dir', '--git-dir'], - repoPath + repoPath, + { signal } ) return parseRelayRepoLocation(repoPath, stdout) }, isUnsupportedRevParsePathFormatError ) } catch { + throwIfSignalAborted(signal) return undefined } } private async normalizeMainWorktreePath( repoPath: string, - worktrees: GitWorktreeInfo[] + worktrees: GitWorktreeInfo[], + signal?: AbortSignal ): Promise { + throwIfSignalAborted(signal) const mainIndex = worktrees.findIndex((worktree) => worktree.isMainWorktree === true) const mainWorktree = worktrees[mainIndex] const mainPath = mainWorktree?.path ?? '' @@ -115,7 +124,8 @@ export class GitHandlerWorktreeOperations extends GitHandlerOperationContext { return worktrees } - const location = await this.readRepoLocation(resolvedRepoPath) + const location = await this.readRepoLocation(resolvedRepoPath, signal) + throwIfSignalAborted(signal) if (!location) { return worktrees } @@ -144,7 +154,8 @@ export class GitHandlerWorktreeOperations extends GitHandlerOperationContext { }) return this.normalizeMainWorktreePath( repoPath, - parseWorktreeList(stdout, { nulDelimited: true }) + parseWorktreeList(stdout, { nulDelimited: true }), + context?.signal ) }, async () => { @@ -154,12 +165,17 @@ export class GitHandlerWorktreeOperations extends GitHandlerOperationContext { const { stdout } = await this.git(['worktree', 'list', '--porcelain'], repoPath, { signal: context?.signal }) - const normalized = await this.normalizeMainWorktreePath(repoPath, parseWorktreeList(stdout)) + const normalized = await this.normalizeMainWorktreePath( + repoPath, + parseWorktreeList(stdout), + context?.signal + ) // Why: Git <2.31 emits no `prunable` annotation, so probe each linked worktree's existence instead of trusting stale registrations (issue #8389). return annotatePrunableWorktreesByExistence( await annotateWorktreeLocksFromAdmin(expandTilde(repoPath), normalized, { signal: context?.signal - }) + }), + context?.signal ) }, isUnsupportedWorktreeListZError diff --git a/src/shared/abort-signal-reason.test.ts b/src/shared/abort-signal-reason.test.ts new file mode 100644 index 00000000000..d2d16ff65ac --- /dev/null +++ b/src/shared/abort-signal-reason.test.ts @@ -0,0 +1,14 @@ +import { expect, it } from 'vitest' +import { waitForPromiseWithSignal } from './abort-signal-reason' + +it('observes work that rejects after its caller has already canceled', async () => { + const controller = new AbortController() + const reason = new Error('Caller canceled') + controller.abort(reason) + const work = new Promise((_resolve, reject) => { + queueMicrotask(() => reject(new Error('Late filesystem failure'))) + }) + + await expect(waitForPromiseWithSignal(work, controller.signal)).rejects.toBe(reason) + await new Promise((resolve) => setImmediate(resolve)) +}) diff --git a/src/shared/abort-signal-reason.ts b/src/shared/abort-signal-reason.ts index e1e365d761b..06eaf2cf36e 100644 --- a/src/shared/abort-signal-reason.ts +++ b/src/shared/abort-signal-reason.ts @@ -16,6 +16,8 @@ export function waitForPromiseWithSignal(promise: Promise, signal?: AbortS return promise } if (signal.aborted) { + // Observe abandoned work so its later rejection stays handled. + void promise.catch(() => undefined) return Promise.reject(abortSignalReason(signal)) } return new Promise((resolve, reject) => { diff --git a/src/shared/git-fork-sync.test.ts b/src/shared/git-fork-sync.test.ts index 5fb6abd81ed..ad8512c8e52 100644 --- a/src/shared/git-fork-sync.test.ts +++ b/src/shared/git-fork-sync.test.ts @@ -16,6 +16,10 @@ function createRunner(overrides: { originExists?: boolean upstreamExists?: boolean aheadBehind?: string + equalTips?: boolean + ancestryError?: boolean + commitOutput?: string + countError?: boolean }): { runGit: GitForkSyncRunner; calls: string[][] } { const calls: string[][] = [] const runGit = vi.fn(async (args: string[]) => { @@ -42,14 +46,22 @@ function createRunner(overrides: { throw new Error('missing upstream branch') } return { - stdout: ref.includes('upstream') - ? '2222222222222222222222222222222222222222\n' - : '1111111111111111111111111111111111111111\n' + stdout: + overrides.commitOutput ?? + (ref.includes('upstream') && !overrides.equalTips + ? '2222222222222222222222222222222222222222\n' + : '1111111111111111111111111111111111111111\n') } } if (args[0] === 'rev-list') { + if (overrides.countError) { + throw new Error('invalid revision') + } return { stdout: overrides.aheadBehind ?? '0\t2\n' } } + if (args[0] === 'merge-base' && overrides.ancestryError) { + throw new Error('missing parent object') + } return { stdout: '' } }) return { runGit, calls } @@ -106,6 +118,48 @@ describe('syncForkDefaultBranch', () => { expect(flattenedCommands(calls)).not.toContain('push origin') }) + it('skips history counts for equal verified tips while retaining fetches and ancestry validation', async () => { + const { runGit, calls } = createRunner({ equalTips: true }) + + await expect(syncForkDefaultBranch(runGit)).resolves.toMatchObject({ + status: 'up-to-date', + ahead: 0, + behind: 0 + }) + expect(calls.filter((args) => args[0] === 'fetch')).toHaveLength(2) + expect(calls.filter((args) => args[0] === 'rev-parse')).toHaveLength(2) + expect(calls.some((args) => args[0] === 'rev-list' || args[0] === 'push')).toBe(false) + expect(calls).toContainEqual([ + 'merge-base', + '--is-ancestor', + '1111111111111111111111111111111111111111', + '1111111111111111111111111111111111111111' + ]) + }) + + it('still blocks equal tips when ancestry cannot be verified', async () => { + const { runGit, calls } = createRunner({ equalTips: true, ancestryError: true }) + + await expect(syncForkDefaultBranch(runGit)).resolves.toMatchObject({ + status: 'blocked', + reason: 'diverged', + ahead: 0, + behind: 0 + }) + expect(calls.some((args) => args[0] === 'push')).toBe(false) + }) + + it('keeps the count query error when a wrapper returns identical non-object output', async () => { + const { runGit, calls } = createRunner({ + commitOutput: 'wrapper banner\n', + countError: true + }) + + await expect(syncForkDefaultBranch(runGit)).rejects.toThrow('invalid revision') + expect(calls.some((args) => args[0] === 'rev-list')).toBe(true) + expect(calls.some((args) => args[0] === 'push')).toBe(false) + }) + it('scans newline-heavy remote and default-branch output without line-array splitting', async () => { const splitSpy = vi.spyOn(String.prototype, 'split') const { runGit } = createRunner({ diff --git a/src/shared/git-fork-sync.ts b/src/shared/git-fork-sync.ts index 4dc0e688860..9a802a54268 100644 --- a/src/shared/git-fork-sync.ts +++ b/src/shared/git-fork-sync.ts @@ -278,9 +278,13 @@ export async function syncForkDefaultBranch( return { ...resultWithBranch, status: 'blocked', reason: 'missing-origin-branch' } } - const counts = parseAheadBehind( - (await runGit(['rev-list', '--left-right', '--count', `${originOid}...${upstreamOid}`])).stdout - ) + const counts = + originOid === upstreamOid && /^(?:[0-9a-fA-F]{40}|[0-9a-fA-F]{64})$/.test(originOid) + ? { ahead: 0, behind: 0 } + : parseAheadBehind( + (await runGit(['rev-list', '--left-right', '--count', `${originOid}...${upstreamOid}`])) + .stdout + ) if (counts.ahead > 0 || !(await isAncestor(runGit, originOid, upstreamOid))) { return { ...resultWithBranch, ...counts, status: 'blocked', reason: 'diverged' }