diff --git a/src/main/git-limiter.test.ts b/src/main/git-limiter.test.ts new file mode 100644 index 00000000..7102acff --- /dev/null +++ b/src/main/git-limiter.test.ts @@ -0,0 +1,129 @@ +import { describe, it, expect, beforeEach } from 'vitest' +import { + runGitRead, + gitLimiterStats, + resetGitLimiter, + MAX_CONCURRENT_GIT_READS +} from './git-limiter' + +/** A promise plus its resolver, so a test can hold a git read open. */ +function deferred(): { promise: Promise; resolve: () => void } { + let resolve!: () => void + const promise = new Promise((r) => { + resolve = r + }) + return { promise, resolve } +} + +/** Let the microtask queue drain so pending acquires settle. */ +const tick = (): Promise => new Promise((r) => setImmediate(r)) + +describe('git-limiter', () => { + beforeEach(() => { + resetGitLimiter() + }) + + it('runs up to the cap concurrently', async () => { + const gates = Array.from({ length: MAX_CONCURRENT_GIT_READS }, deferred) + let started = 0 + const runs = gates.map((g) => + runGitRead('interactive', async () => { + started++ + await g.promise + }) + ) + await tick() + + expect(started).toBe(MAX_CONCURRENT_GIT_READS) + gates.forEach((g) => g.resolve()) + await Promise.all(runs) + }) + + it('queues work past the cap', async () => { + const gates = Array.from({ length: MAX_CONCURRENT_GIT_READS }, deferred) + let started = 0 + const runs = gates.map((g) => + runGitRead('interactive', async () => { + started++ + await g.promise + }) + ) + const extra = runGitRead('interactive', async () => { + started++ + }) + await tick() + + expect(started).toBe(MAX_CONCURRENT_GIT_READS) + expect(gitLimiterStats().interactiveQueued).toBe(1) + + gates.forEach((g) => g.resolve()) + await Promise.all([...runs, extra]) + expect(started).toBe(MAX_CONCURRENT_GIT_READS + 1) + }) + + it('dequeues interactive work ahead of bulk work already waiting', async () => { + const gates = Array.from({ length: MAX_CONCURRENT_GIT_READS }, deferred) + const blockers = gates.map((g) => runGitRead('interactive', () => g.promise)) + await tick() + + const order: string[] = [] + // The queued items hold their own permit open, so freeing one permit at a + // time makes the dequeue order directly observable. + const bulkGate = deferred() + const interactiveGate = deferred() + // Bulk enqueues first, interactive second — priority must still win. + const bulk = runGitRead('bulk', async () => { + order.push('bulk') + await bulkGate.promise + }) + const interactive = runGitRead('interactive', async () => { + order.push('interactive') + await interactiveGate.promise + }) + await tick() + expect(order).toEqual([]) + + gates[0].resolve() + await tick() + expect(order).toEqual(['interactive']) + + gates[1].resolve() + await tick() + expect(order).toEqual(['interactive', 'bulk']) + + gates.slice(2).forEach((g) => g.resolve()) + bulkGate.resolve() + interactiveGate.resolve() + await Promise.all([...blockers, bulk, interactive]) + }) + + it('releases the permit when the operation throws', async () => { + await expect( + runGitRead('interactive', async () => { + throw new Error('git exploded') + }) + ).rejects.toThrow('git exploded') + + expect(gitLimiterStats().active).toBe(0) + + // A subsequent read still gets a permit rather than hanging. + await expect(runGitRead('interactive', async () => 'ok')).resolves.toBe('ok') + }) + + it('drains bulk work once interactive work is done', async () => { + const done: number[] = [] + const all = Array.from({ length: MAX_CONCURRENT_GIT_READS * 3 }, (_, i) => + runGitRead(i % 2 === 0 ? 'bulk' : 'interactive', async () => { + done.push(i) + }) + ) + await Promise.all(all) + + expect(done).toHaveLength(MAX_CONCURRENT_GIT_READS * 3) + expect(gitLimiterStats()).toEqual({ + active: 0, + interactiveQueued: 0, + bulkQueued: 0 + }) + }) +}) diff --git a/src/main/git-limiter.ts b/src/main/git-limiter.ts new file mode 100644 index 00000000..a435c244 --- /dev/null +++ b/src/main/git-limiter.ts @@ -0,0 +1,121 @@ +// Concurrency gate for read-only git subprocesses. +// +// Every panel read here is I/O-bound, not CPU-bound: a cold `git status +// --porcelain` in a large monorepo stats thousands of files at ~35% CPU while +// the main process sits idle. That makes the interesting number *not* total +// throughput but how long any one read waits behind the others. +// +// Measured on the reference monorepo (18 worktrees, `git status --porcelain` +// in each, warm cache), varying only the concurrency cap: +// +// cap=1 total 1626ms p50 49ms max 463ms +// cap=4 total 741ms p50 77ms max 495ms +// cap=8 total 718ms p50 155ms max 534ms +// cap=16 total 732ms p50 274ms max 619ms +// cap=64 total 766ms p50 220ms max 621ms +// +// Total wall time plateaus at cap=4 — past that, extra parallelism buys no +// throughput and only inflates per-call latency (p50 77ms → 274ms), because +// each read now shares the disk with 15 others instead of 3. So the cap is +// close to free, which is what makes the second half of this module possible. +// +// The second half is priority. A cap alone doesn't help an interactive read +// that lands behind a 66-worktree bulk sweep — it still waits for the queue to +// drain. Interactive work is dequeued ahead of bulk work, so a background scan +// yields to a panel the user is actually looking at. Strict priority is safe +// here because interactive load is inherently finite and short-lived (a bounded +// set of mounted panels, each firing a handful of reads per switch or per 30s +// poll), so bulk always drains once the burst passes. +// +// Writes deliberately do NOT go through this gate. Merges, fetches, and +// `worktree add` are user-initiated, rare, and long — queueing them behind a +// background sweep would be strictly worse, and they call read helpers +// internally, which under a shared cap is a deadlock waiting to happen. +// +// Cap re-checked against the thing that actually matters — one worktree +// switch, which issues ~15-20 reads, measured on the same monorepo both idle +// and while a full dirty sweep runs: +// +// cap quiet mean quiet p50 contended mean contended max +// 2 1881ms 1784ms 1876ms 2884ms +// 4 1675ms 1614ms 1702ms 3449ms +// 6 1623ms 1548ms 1677ms 3724ms +// 8 1625ms 1559ms 1770ms 3936ms +// +// 4 and 6 are within run-to-run noise of each other on the mean (repeat runs +// at cap=4 landed between 1602ms and 1675ms), and the tail under contention +// gets monotonically worse as the cap rises. 4 is chosen for that tail. + +export type GitPriority = 'interactive' | 'bulk' + +/** The throughput plateau from the table above. */ +export const MAX_CONCURRENT_GIT_READS = 4 + +interface Waiter { + resolve: () => void +} + +const queues: Record = { + interactive: [], + bulk: [] +} + +let active = 0 + +function next(): void { + const waiter = queues.interactive.shift() ?? queues.bulk.shift() + if (!waiter) return + active++ + waiter.resolve() +} + +function acquire(priority: GitPriority): Promise { + if (active < MAX_CONCURRENT_GIT_READS) { + active++ + return Promise.resolve() + } + return new Promise((resolve) => { + queues[priority].push({ resolve }) + }) +} + +function release(): void { + active-- + next() +} + +/** Run a read-only git operation under the concurrency gate. The permit is + * held only for the duration of `fn`, so a caller that runs several reads in + * sequence takes and returns a permit per read rather than holding one across + * the whole sequence — that's what keeps nested helpers deadlock-free. */ +export async function runGitRead( + priority: GitPriority, + fn: () => Promise +): Promise { + await acquire(priority) + try { + return await fn() + } finally { + release() + } +} + +/** Test-only: observable queue depth. */ +export function gitLimiterStats(): { + active: number + interactiveQueued: number + bulkQueued: number +} { + return { + active, + interactiveQueued: queues.interactive.length, + bulkQueued: queues.bulk.length + } +} + +/** Test-only: drop all state between cases. */ +export function resetGitLimiter(): void { + queues.interactive.length = 0 + queues.bulk.length = 0 + active = 0 +} diff --git a/src/main/index.ts b/src/main/index.ts index 69b367dc..ec906668 100644 --- a/src/main/index.ts +++ b/src/main/index.ts @@ -1674,8 +1674,8 @@ function registerIpcHandlers(): void { } ) - transport.onRequest('worktree:isDirty', async (_ctx, path: string) => { - const git = await isWorktreeDirty(path) + transport.onRequest('worktree:isDirty', async (_ctx, path: string, opts?: { bulk?: boolean }) => { + const git = await isWorktreeDirty(path, opts?.bulk ? 'bulk' : 'interactive') const scratchpad = hasScratchpadNote(store.getSnapshot().state.scratchpad, path) return { git, scratchpad } }) diff --git a/src/main/worktree-main-status-cache.test.ts b/src/main/worktree-main-status-cache.test.ts new file mode 100644 index 00000000..0c362b70 --- /dev/null +++ b/src/main/worktree-main-status-cache.test.ts @@ -0,0 +1,110 @@ +import { describe, it, expect, beforeEach, afterEach } from 'vitest' +/* These spawn real git. Under a full parallel suite run a handful of spawns + * can blow past vitest's 5s default, so every case here sets its own budget — + * a timeout in this file would otherwise read as a caching regression. */ +import { execFileSync } from 'child_process' +import { mkdtempSync, mkdirSync, rmSync, writeFileSync } from 'fs' +import { tmpdir } from 'os' +import { join } from 'path' +import { getMainWorktreeStatus, invalidateMainWorktreeStatus } from './worktree' + +function git(cwd: string, args: string[]): string { + return execFileSync('git', args, { + cwd, + env: { + ...process.env, + GIT_AUTHOR_NAME: 't', + GIT_AUTHOR_EMAIL: 't@t', + GIT_COMMITTER_NAME: 't', + GIT_COMMITTER_EMAIL: 't@t' + } + }).toString() +} + +const TIMEOUT = 60_000 + +describe('getMainWorktreeStatus caching', () => { + let repo: string + + beforeEach(() => { + invalidateMainWorktreeStatus() + repo = mkdtempSync(join(tmpdir(), 'harness-mainstatus-')) + git(repo, ['init', '-q', '-b', 'main']) + git(repo, ['config', 'commit.gpgsign', 'false']) + writeFileSync(join(repo, 'f.txt'), 'base\n') + git(repo, ['add', 'f.txt']) + git(repo, ['commit', '-q', '-m', 'base']) + }) + + afterEach(() => { + invalidateMainWorktreeStatus() + rmSync(repo, { recursive: true, force: true }) + }) + + // The switch-time shape: the panel asks directly while worktree:previewMerge + // asks internally, at the same instant. One underlying read, not two. + it('collapses concurrent callers onto a single read', async () => { + const [a, b] = await Promise.all([ + getMainWorktreeStatus(repo), + getMainWorktreeStatus(repo) + ]) + expect(a).toBe(b) + }, TIMEOUT) + + it('serves a later caller from cache within the TTL', async () => { + const first = await getMainWorktreeStatus(repo) + expect(await getMainWorktreeStatus(repo)).toBe(first) + }, TIMEOUT) + + it('re-reads after an explicit invalidation', async () => { + const first = await getMainWorktreeStatus(repo) + invalidateMainWorktreeStatus(repo) + const second = await getMainWorktreeStatus(repo) + expect(second).not.toBe(first) + expect(second).toEqual(first) + }, TIMEOUT) + + it('re-reads when forced, and picks up a change the cache would have hidden', async () => { + const clean = await getMainWorktreeStatus(repo) + expect(clean.isDirty).toBe(false) + expect(clean.ready).toBe(true) + + writeFileSync(join(repo, 'f.txt'), 'dirty\n') + // Unforced within the TTL still reports the stale answer — that's the + // trade the TTL makes, and why the merge gate forces. + expect((await getMainWorktreeStatus(repo)).isDirty).toBe(false) + + const forced = await getMainWorktreeStatus(repo, { force: true }) + expect(forced.isDirty).toBe(true) + expect(forced.ready).toBe(false) + }, TIMEOUT) + + it("keys by repo, so a second repo is not served the first one's answer", async () => { + const other = mkdtempSync(join(tmpdir(), 'harness-mainstatus-b-')) + try { + // `master` rather than an arbitrary name: getLocalBaseBranch only + // recognises main/master, and falls back to the literal 'main' for + // anything else — which would mask a key collision instead of exposing it. + git(other, ['init', '-q', '-b', 'master']) + git(other, ['config', 'commit.gpgsign', 'false']) + writeFileSync(join(other, 'g.txt'), 'x\n') + git(other, ['add', 'g.txt']) + git(other, ['commit', '-q', '-m', 'x']) + + const a = await getMainWorktreeStatus(repo) + const b = await getMainWorktreeStatus(other) + expect(a.path).not.toBe(b.path) + expect(a.baseBranch).toBe('main') + expect(b.baseBranch).toBe('master') + } finally { + rmSync(other, { recursive: true, force: true }) + } + }, TIMEOUT) + + it('does not cache a failure', async () => { + const missing = join(tmpdir(), 'harness-mainstatus-does-not-exist') + await expect(getMainWorktreeStatus(missing)).rejects.toThrow() + // A cached rejection here would poison the repo for the whole TTL. + await expect(getMainWorktreeStatus(missing)).rejects.toThrow() + }, TIMEOUT) +}) diff --git a/src/main/worktree.ts b/src/main/worktree.ts index b6e064cc..9c61b55a 100644 --- a/src/main/worktree.ts +++ b/src/main/worktree.ts @@ -17,6 +17,7 @@ import { perfLog } from './perf-log' import { resolveUserShell, loginShellCommandArgs } from './user-shell' import { detectInProgressOp } from './git-ops-state' import { cachedGitRead } from './git-poll-cache' +import { runGitRead, type GitPriority } from './git-limiter' import type { Worktree } from '../shared/state/worktrees' const execFileAsync = promisify(execFile) @@ -33,15 +34,46 @@ function readOnlyGitEnv(): NodeJS.ProcessEnv { return { ...process.env, GIT_OPTIONAL_LOCKS: '0' } } +/** Every read-only git spawn goes through here so the concurrency gate in + * git-limiter.ts sees all of them. Gating at the leaf exec — rather than around + * a whole helper — is what keeps callers that issue several reads in sequence + * (getMainWorktreeStatus, resolveDefaultBaseRef) from holding a permit while + * waiting for one, which would deadlock at the cap. Writes bypass this on + * purpose; see the module comment in git-limiter.ts. */ +function execGitRead( + args: string[], + opts: ExecOpts, + priority: GitPriority = 'interactive' +): Promise<{ stdout: string }> { + return gatedExec(args, opts, priority) +} + +/** Shared body of execGitRead/tracedExec. `execMs` is measured *inside* the + * gate so it stays comparable with pre-limiter `[git-op]` lines — time spent + * queueing is reported separately as `waitMs` rather than folded into exec. */ +async function gatedExec( + args: string[], + opts: ExecOpts, + priority: GitPriority +): Promise<{ stdout: string; execMs: number; waitMs: number }> { + const queued = performance.now() + return runGitRead(priority, async () => { + const started = performance.now() + const { stdout } = await execFileAsync('git', args, { env: readOnlyGitEnv(), ...opts }) + return { + stdout: typeof stdout === 'string' ? stdout : stdout.toString(), + execMs: performance.now() - started, + waitMs: started - queued + } + }) +} + async function tracedExec( args: string[], opts: ExecOpts ): Promise<{ stdout: string; execMs: number; outputBytes: number }> { - const t0 = performance.now() - const { stdout } = await execFileAsync('git', args, { env: readOnlyGitEnv(), ...opts }) - const execMs = performance.now() - t0 - const text = typeof stdout === 'string' ? stdout : stdout.toString() - return { stdout: text, execMs, outputBytes: text.length } + const { stdout, execMs } = await gatedExec(args, opts, 'interactive') + return { stdout, execMs, outputBytes: stdout.length } } // Alias so existing imports of WorktreeInfo keep working; the canonical @@ -116,7 +148,7 @@ export function parseWorktreeListPorcelain( } export async function listWorktrees(repoRoot: string): Promise { - const { stdout } = await execFileAsync('git', ['worktree', 'list', '--porcelain'], { + const { stdout } = await execGitRead(['worktree', 'list', '--porcelain'], { cwd: repoRoot }) @@ -182,7 +214,7 @@ export async function fetchPullRequestRef( /** True if a local branch with this name already exists in the repo. */ export async function localBranchExists(repoRoot: string, branchName: string): Promise { try { - await execFileAsync('git', ['rev-parse', '--verify', '--quiet', `refs/heads/${branchName}`], { + await execGitRead(['rev-parse', '--verify', '--quiet', `refs/heads/${branchName}`], { cwd: repoRoot }) return true @@ -196,8 +228,7 @@ export async function listBranches(repoRoot: string): Promise { // excluded — hundreds of remote branches make the picker UI unusable. Users // who need a remote ref can type it into the Ref tab on the New worktree // screen. - const { stdout } = await execFileAsync( - 'git', + const { stdout } = await execGitRead( ['branch', '--format=%(refname:short)'], { cwd: repoRoot } ) @@ -364,13 +395,17 @@ export async function continueWorktree( return { worktree: updated, stashReapplied, stashConflict } } -/** Check if a worktree has uncommitted changes */ -export async function isWorktreeDirty(path: string): Promise { +/** Check if a worktree has uncommitted changes. + * + * `priority` exists for the Cleanup modal, which asks this of every worktree at + * once. At 'bulk' that sweep queues behind anything the user is actually + * looking at instead of burying it. */ +export async function isWorktreeDirty( + path: string, + priority: GitPriority = 'interactive' +): Promise { try { - const { stdout } = await execFileAsync('git', ['status', '--porcelain'], { - cwd: path, - env: readOnlyGitEnv() - }) + const { stdout } = await execGitRead(['status', '--porcelain'], { cwd: path }, priority) return stdout.trim().length > 0 } catch { return false @@ -411,8 +446,7 @@ export async function getDefaultBaseRef(worktreePath: string): Promise { async function resolveDefaultBaseRef(worktreePath: string): Promise { try { - const { stdout } = await execFileAsync( - 'git', + const { stdout } = await execGitRead( ['symbolic-ref', '--short', 'refs/remotes/origin/HEAD'], { cwd: worktreePath } ) @@ -421,7 +455,7 @@ async function resolveDefaultBaseRef(worktreePath: string): Promise { } catch {} for (const candidate of ['origin/main', 'origin/master', 'main', 'master']) { try { - await execFileAsync('git', ['rev-parse', '--verify', candidate], { cwd: worktreePath }) + await execGitRead(['rev-parse', '--verify', candidate], { cwd: worktreePath }) return candidate } catch {} } @@ -476,15 +510,14 @@ async function getUnpushedHashes(worktreePath: string): Promise | nu if (!branch) return null const remoteRef = `refs/remotes/origin/${branch}` try { - await execFileAsync('git', ['rev-parse', '--verify', '--quiet', remoteRef], { + await execGitRead(['rev-parse', '--verify', '--quiet', remoteRef], { cwd: worktreePath }) } catch { return null } try { - const { stdout } = await execFileAsync( - 'git', + const { stdout } = await execGitRead( ['log', `origin/${branch}..HEAD`, '--pretty=format:%H', '--max-count=500'], { cwd: worktreePath } ) @@ -807,8 +840,7 @@ export async function getCommitMeta( try { const sep = '\x1f' const end = '\x1e' - const { stdout: meta } = await execFileAsync( - 'git', + const { stdout: meta } = await execGitRead( ['show', '-s', `--pretty=format:%H${sep}%h${sep}%an${sep}%ae${sep}%aI${sep}%s${sep}%b${end}`, hash], { cwd: worktreePath } ) @@ -828,8 +860,7 @@ export async function getCommitDiff( const meta = await getCommitMeta(worktreePath, hash) if (!meta) return null try { - const { stdout: diff } = await execFileAsync( - 'git', + const { stdout: diff } = await execGitRead( ['show', '--no-color', '--pretty=format:', hash], { cwd: worktreePath, maxBuffer: 32 * 1024 * 1024 } ) @@ -845,8 +876,7 @@ export async function getCommitDiff( * resolve; the cap keeps the payload bounded on large repos. */ export async function listRecentCommitShas(worktreePath: string): Promise { try { - const { stdout } = await execFileAsync( - 'git', + const { stdout } = await execGitRead( ['rev-list', '--all', '--max-count=10000'], { cwd: worktreePath, maxBuffer: 16 * 1024 * 1024 } ) @@ -1116,7 +1146,7 @@ async function getFileAtRef( filePath: string ): Promise { try { - const { stdout } = await execFileAsync('git', ['show', `${ref}:${filePath}`], { + const { stdout } = await execGitRead(['show', `${ref}:${filePath}`], { cwd: worktreePath, maxBuffer: 16 * 1024 * 1024 }) @@ -1128,7 +1158,7 @@ async function getFileAtRef( async function getMergeBase(worktreePath: string, ref: string): Promise { try { - const { stdout } = await execFileAsync('git', ['merge-base', ref, 'HEAD'], { + const { stdout } = await execGitRead(['merge-base', ref, 'HEAD'], { cwd: worktreePath }) return stdout.trim() || null @@ -1247,7 +1277,7 @@ async function getLocalBaseBranch(repoRoot: string): Promise { /** Get the current branch of a worktree, or empty string if detached. */ export async function getCurrentBranch(worktreePath: string): Promise { try { - const { stdout } = await execFileAsync('git', ['symbolic-ref', '--short', 'HEAD'], { + const { stdout } = await execGitRead(['symbolic-ref', '--short', 'HEAD'], { cwd: worktreePath }) return stdout.trim() @@ -1287,8 +1317,7 @@ export async function renameWorktreeBranch( return { ok: true, oldBranch, branch: newBranch, renamed: false } } - const upstream = await execFileAsync( - 'git', + const upstream = await execGitRead( ['rev-parse', '--abbrev-ref', '--symbolic-full-name', '@{upstream}'], { cwd: worktreePath } ) @@ -1318,8 +1347,71 @@ export async function renameWorktreeBranch( return { ok: true, oldBranch, branch: newBranch, renamed: true } } +// Main-worktree status is asked for far more often than it changes. It's keyed +// by repoRoot, not worktree, so switching between two worktrees of the same +// repo recomputes an identical answer — and a single switch asks for it twice +// over, because `worktree:previewMerge` needs it internally while the panel +// requests it directly in parallel. Each miss is four sequential git spawns, +// one of which is a full `git status` on the main checkout. +// +// An in-flight entry is what collapses the concurrent pair; the TTL is what +// covers flipping back and forth between worktrees. It's deliberately short — +// this answer gates the "main isn't ready" fixup button, so it should track +// reality at human timescale — and any Ness-side mutation of main busts it +// outright rather than waiting the TTL out. +// +// The TTL runs from when the read *settles*, not from when it was dispatched. +// Dispatch-time would be self-defeating on exactly the repos this exists for: +// where the four spawns take longer than the TTL, every entry would land +// already expired and the cache would never serve anyone. +const MAIN_STATUS_TTL_MS = 3000 +interface MainStatusEntry { + /** null while the read is still in flight — such an entry is always fresh, + * which is what makes concurrent callers join rather than each spawn git. */ + settledAt: number | null + promise: Promise +} +const mainStatusCache = new Map() + +export function invalidateMainWorktreeStatus(repoRoot?: string): void { + if (repoRoot === undefined) mainStatusCache.clear() + else mainStatusCache.delete(repoRoot) +} + /** Report status of the main worktree for a local merge. */ -export async function getMainWorktreeStatus(repoRoot: string): Promise { +export async function getMainWorktreeStatus( + repoRoot: string, + opts: { force?: boolean } = {} +): Promise { + const hit = mainStatusCache.get(repoRoot) + if ( + !opts.force && + hit && + (hit.settledAt === null || Date.now() - hit.settledAt < MAIN_STATUS_TTL_MS) + ) { + return hit.promise + } + + const entry: MainStatusEntry = { + settledAt: null, + promise: readMainWorktreeStatus(repoRoot).then( + (value) => { + if (mainStatusCache.get(repoRoot) === entry) entry.settledAt = Date.now() + return value + }, + (err) => { + // Never let a failure stick around for the TTL — the next caller + // should get a real attempt, not a cached rejection. + if (mainStatusCache.get(repoRoot) === entry) mainStatusCache.delete(repoRoot) + throw err + } + ) + } + mainStatusCache.set(repoRoot, entry) + return entry.promise +} + +async function readMainWorktreeStatus(repoRoot: string): Promise { const t0 = performance.now() let walledExec = 0 let cumExec = 0 @@ -1376,7 +1468,7 @@ export async function prepareMainForMerge(repoRoot: string): Promise { - const status = await getMainWorktreeStatus(repoRoot) + // Forced: this read is the gate on whether merging is safe at all, so it + // must reflect the repo right now rather than up to a TTL ago. + const status = await getMainWorktreeStatus(repoRoot, { force: true }) if (!status.ready) { throw new Error( `Main worktree is not ready: ${status.isDirty ? 'has uncommitted changes' : `on ${status.currentBranch || 'detached HEAD'}, not ${status.baseBranch}`}` @@ -1459,6 +1553,11 @@ export async function mergeWorktreeLocally( ) } throw new Error(`Merge failed and was aborted: ${detail}`) + } finally { + // Main moved either way — a landed merge, or the abort/reset on the + // failure path. Dropped after the fact rather than before, so a read + // racing the merge can't repopulate the entry with mid-merge state. + invalidateMainWorktreeStatus(repoRoot) } return { @@ -1486,8 +1585,7 @@ export async function previewMergeConflicts( baseBranch: string ): Promise { try { - await execFileAsync( - 'git', + await execGitRead( ['merge-tree', '--write-tree', '--name-only', baseBranch, sourceBranch], { cwd: repoRoot } ) @@ -1517,7 +1615,7 @@ export async function previewMergeConflicts( /** Resolve a branch ref to its current SHA, or null if it doesn't exist. */ export async function getBranchSha(repoRoot: string, branch: string): Promise { try { - const { stdout } = await execFileAsync('git', ['rev-parse', '--verify', `refs/heads/${branch}`], { + const { stdout } = await execGitRead(['rev-parse', '--verify', `refs/heads/${branch}`], { cwd: repoRoot }) return stdout.trim() || null @@ -1533,7 +1631,7 @@ export async function isBranchAncestorOfBase( base: string ): Promise { try { - await execFileAsync('git', ['merge-base', '--is-ancestor', branch, base], { cwd: repoRoot }) + await execGitRead(['merge-base', '--is-ancestor', branch, base], { cwd: repoRoot }) return true } catch { return false @@ -1548,8 +1646,7 @@ export async function getBranchDiffStats( try { const base = await getDefaultBaseRef(worktreePath) if (!base || base === 'HEAD') return { added: 0, removed: 0, files: 0 } - const { stdout } = await execFileAsync( - 'git', + const { stdout } = await execGitRead( ['diff', '--numstat', `${base}...HEAD`], { cwd: worktreePath, maxBuffer: 8 * 1024 * 1024 } ) diff --git a/src/renderer/build-backend.ts b/src/renderer/build-backend.ts index f1344d44..b4637626 100644 --- a/src/renderer/build-backend.ts +++ b/src/renderer/build-backend.ts @@ -134,7 +134,8 @@ export function buildBackend( newBranchName: string, baseBranch?: string ) => req('worktree:continue', repoRoot, worktreePath, newBranchName, baseBranch), - isWorktreeDirty: (path: string) => req('worktree:isDirty', path), + isWorktreeDirty: (path: string, opts?: { bulk?: boolean }) => + req('worktree:isDirty', path, opts), removeWorktree: ( repoRoot: string, path: string, diff --git a/src/renderer/components/Cleanup.tsx b/src/renderer/components/Cleanup.tsx index 20b3e863..72bcdcdf 100644 --- a/src/renderer/components/Cleanup.tsx +++ b/src/renderer/components/Cleanup.tsx @@ -53,6 +53,9 @@ interface Candidate { prState: PRStatus['state'] | null merged: boolean dirty: boolean + /** False until the dirty sweep has actually answered for this worktree. + * `dirty: false` alone can't be trusted while the scan is in flight. */ + dirtyKnown: boolean } export function Cleanup({ @@ -69,7 +72,9 @@ export function Cleanup({ const [repoFilter, setRepoFilter] = useState(null) // null = all repos const [mergedOnly, setMergedOnly] = useState(false) const [includeDirty, setIncludeDirty] = useState(false) + // Absent key = not scanned yet. Distinct from `false` (scanned, clean). const [dirtyMap, setDirtyMap] = useState>({}) + const [scanningDirty, setScanningDirty] = useState(true) const [activityLastTs, setActivityLastTs] = useState>({}) const [selected, setSelected] = useState>({}) // Paths the user has explicitly clicked; their selection state must survive @@ -93,20 +98,45 @@ export function Cleanup({ return [...set].sort() }, [eligible]) + // The dirty sweep asks git about every eligible worktree, which on a large + // set is the most expensive thing this modal does. Two things keep it off + // everything else's back: it runs at 'bulk' priority so the main process + // dequeues it behind whatever panel the user is looking at, and its answers + // land one at a time instead of behind a single Promise.all barrier, so the + // list is usable long before the last worktree reports in. + // + // A path missing from dirtyMap means "not scanned yet", which is distinct + // from "clean" — see the selection defaults below, which must not offer up + // an unscanned worktree for deletion. + useEffect(() => { + let cancelled = false + setScanningDirty(true) + let outstanding = eligible.length + if (outstanding === 0) setScanningDirty(false) + for (const w of eligible) { + void backend + .isWorktreeDirty(w.path, { bulk: true }) + .then((d) => { + if (cancelled) return + setDirtyMap((prev) => ({ ...prev, [w.path]: d.git || d.scratchpad })) + }) + .catch(() => {}) + .finally(() => { + if (cancelled) return + if (--outstanding === 0) setScanningDirty(false) + }) + } + return () => { + cancelled = true + } + }, [eligible, backend]) + useEffect(() => { let cancelled = false ;(async () => { setLoading(true) try { - const [log, dirtyResults] = await Promise.all([ - backend.getActivityLog(), - Promise.all( - eligible.map(async (w) => { - const d = await backend.isWorktreeDirty(w.path) - return [w.path, d.git || d.scratchpad] as const - }) - ) - ]) + const log = await backend.getActivityLog() if (cancelled) return const lastTs: Record = {} for (const [path, rec] of Object.entries(log as ActivityLog)) { @@ -125,9 +155,6 @@ export function Cleanup({ if (ts !== undefined) lastTs[path] = ts } setActivityLastTs(lastTs) - const dmap: Record = {} - for (const [path, d] of dirtyResults) dmap[path] = d - setDirtyMap(dmap) } finally { if (!cancelled) setLoading(false) } @@ -159,7 +186,8 @@ export function Cleanup({ lastActiveMs: lastMs, prState: pr?.state ?? null, merged, - dirty + dirty, + dirtyKnown: dirtyMap[w.path] !== undefined }) } out.sort((a, b) => { @@ -182,7 +210,11 @@ export function Cleanup({ } for (const c of candidates) { if (touched[c.worktree.path]) continue - next[c.worktree.path] = includeDirty ? true : !c.dirty + // `=== false` rather than `!c.dirty`: while the sweep is still running + // an unscanned worktree reads as not-dirty, and defaulting those to + // selected would arm the delete button over worktrees that may well + // have uncommitted work. They tick on as each one is proven clean. + next[c.worktree.path] = includeDirty ? true : c.dirtyKnown && !c.dirty } return next }) @@ -379,8 +411,16 @@ export function Cleanup({ {!loading && candidates.length > 0 && ( <>
-
- {candidates.length} match{candidates.length === 1 ? '' : 'es'} · {selectedPaths.length} selected +
+ + {candidates.length} match{candidates.length === 1 ? '' : 'es'} · {selectedPaths.length} selected + + {scanningDirty && ( + + + checking for uncommitted changes… + + )}