From fee20a78778fdec128f2d26d35db7c8ab5cf601d Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Thu, 23 Jul 2026 14:53:47 -0700 Subject: [PATCH] fix(worktrees): dereference symlinked .worktreeinclude entries + cache APFS volume probe MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two issues found by review + perf audit of the copy path: - Correctness (HIGH): a listed entry that is itself a gitignored symlink was copied AS a symlink (fs.cp dereference:false), and the darwin APFS branch was skipped for all symlink sources. Editing the worktree's copy then wrote through to the shared/primary target — inverting copy-mode's 'each worktree owns its files' guarantee, and escaping the worktree entirely if the link pointed outside it. Now resolve realpath for a top-level symlink in copy mode so we copy content; nested symlinks inside a copied dir stay as-is (cp -R semantics). - Perf: assertSameApfsVolume ran df+diskutil per copied path (4 subprocesses each), so an N-entry include spawned ~4N short-lived processes on the macOS create hot path, all re-probing one volume. Add a per-materialization device-keyed cache: one probe per distinct volume (4N -> ~4). Tests: symlinked-file and symlinked-dir dereference regressions (no leak to primary); APFS volume probed once regardless of copied-path count. --- src/main/ipc/worktree-apfs-clone.ts | 35 ++++++++++++++--- src/main/ipc/worktree-symlinks.test.ts | 53 ++++++++++++++++++++++++++ src/main/ipc/worktree-symlinks.ts | 28 ++++++++++---- 3 files changed, 103 insertions(+), 13 deletions(-) diff --git a/src/main/ipc/worktree-apfs-clone.ts b/src/main/ipc/worktree-apfs-clone.ts index 69587b88de1..2d3b13573ab 100644 --- a/src/main/ipc/worktree-apfs-clone.ts +++ b/src/main/ipc/worktree-apfs-clone.ts @@ -29,6 +29,12 @@ type DarwinFilesystemInfo = { filesystemName: string } +/** Per-materialization cache keyed by `stat().dev`. Copying N `.worktreeinclude` + * paths would otherwise re-run df+diskutil per path (4 subprocesses each) even + * though source and worktree almost always share one volume; caching collapses + * that to one probe per distinct volume. */ +export type DarwinFilesystemCache = Map> + export class ApfsCloneUnavailableError extends Error { constructor(message: string) { super(message) @@ -72,14 +78,32 @@ async function getDarwinFilesystemInfo( } } +async function getCachedDarwinFilesystemInfo( + path: string, + deps: ApfsCloneDeps, + cache: DarwinFilesystemCache +): Promise { + const deviceId = (await stat(path)).dev + const cached = cache.get(deviceId) + if (cached) { + return cached + } + // Why: cache the pending (or rejected) probe so every path on this volume + // reuses one df+diskutil pair instead of respawning them per copy. + const pending = getDarwinFilesystemInfo(path, deps) + cache.set(deviceId, pending) + return pending +} + async function assertSameApfsVolume( source: string, target: string, - deps: ApfsCloneDeps + deps: ApfsCloneDeps, + cache: DarwinFilesystemCache ): Promise { const [sourceInfo, targetInfo] = await Promise.all([ - getDarwinFilesystemInfo(source, deps), - getDarwinFilesystemInfo(dirname(target), deps) + getCachedDarwinFilesystemInfo(source, deps, cache), + getCachedDarwinFilesystemInfo(dirname(target), deps, cache) ]) if ( sourceInfo.device !== targetInfo.device || @@ -151,10 +175,11 @@ export async function cloneWorktreePathWithApfs( source: string, target: string, sourceIsDirectory: boolean, - deps: ApfsCloneDeps = defaultApfsCloneDeps + deps: ApfsCloneDeps = defaultApfsCloneDeps, + filesystemCache: DarwinFilesystemCache = new Map() ): Promise { await mkdir(dirname(target), { recursive: true }) - await assertSameApfsVolume(source, target, deps) + await assertSameApfsVolume(source, target, deps, filesystemCache) // Why: Node's COPYFILE_FICLONE_FORCE returns ENOSYS on macOS in our runtime, // while Darwin's cp exposes APFS clonefile via -c. Preflight the volume so // cp's non-APFS full-copy fallback cannot surprise users. diff --git a/src/main/ipc/worktree-symlinks.test.ts b/src/main/ipc/worktree-symlinks.test.ts index 148404ebe4c..865c3f17b11 100644 --- a/src/main/ipc/worktree-symlinks.test.ts +++ b/src/main/ipc/worktree-symlinks.test.ts @@ -447,6 +447,31 @@ describe('createWorktreeCopiedPaths', () => { expect(readFileSync(join(worktree, 'apps', 'web', '.env'), 'utf8')).toBe('A=1') }) + // Finding 1 regression: a symlinked include entry must become an independent + // copy, not a symlink, or worktree edits would leak back into the shared target. + posixIt('dereferences a symlinked file entry so edits do not leak to the primary', async () => { + writeFileSync(join(primary, '.env.shared'), 'SECRET=1\n') + symlinkSync(join(primary, '.env.shared'), join(primary, '.env')) + + await createWorktreeCopiedPaths(primary, worktree, ['.env'], { platform: 'linux' }) + + expect(lstatSync(join(worktree, '.env')).isSymbolicLink()).toBe(false) + writeFileSync(join(worktree, '.env'), 'SECRET=2\n') + expect(readFileSync(join(primary, '.env.shared'), 'utf8')).toBe('SECRET=1\n') + }) + + posixIt('dereferences a symlinked directory entry into an independent copy', async () => { + mkdirSync(join(primary, '.cache-real')) + writeFileSync(join(primary, '.cache-real', 'f'), 'ORIG\n') + symlinkSync(join(primary, '.cache-real'), join(primary, '.cache'), 'dir') + + await createWorktreeCopiedPaths(primary, worktree, ['.cache'], { platform: 'linux' }) + + expect(lstatSync(join(worktree, '.cache')).isSymbolicLink()).toBe(false) + writeFileSync(join(worktree, '.cache', 'f'), 'CHANGED\n') + expect(readFileSync(join(primary, '.cache-real', 'f'), 'utf8')).toBe('ORIG\n') + }) + it('preserves a pre-existing target in the worktree (no clobber)', async () => { writeFileSync(join(primary, '.env'), 'SECRET=1\n') writeFileSync(join(worktree, '.env'), 'MINE=1\n') @@ -502,6 +527,34 @@ describe('createWorktreeCopiedPaths', () => { ) expect(readFileSync(join(worktree, '.env'), 'utf8')).toBe('CLONED=1\n') }) + + // Perf: the df+diskutil volume probe must not scale with the number of copied + // paths — one probe per distinct volume, cached across the materialization. + it('probes each APFS volume once regardless of how many paths are copied', async () => { + for (const name of ['.env', '.env.local', 'config.json', 'secrets.json']) { + writeFileSync(join(primary, name), `${name}\n`) + } + const deps = createApfsCloneDeps({ onCp: () => {} }) + + await createWorktreeCopiedPaths( + primary, + worktree, + ['.env', '.env.local', 'config.json', 'secrets.json'], + { platform: 'darwin', apfsCloneDeps: deps } + ) + + const execFileAsyncMock = vi.mocked(deps.execFileAsync) + const dfCalls = execFileAsyncMock.mock.calls.filter(([file]) => file === '/bin/df').length + const diskutilCalls = execFileAsyncMock.mock.calls.filter( + ([file]) => file === '/usr/sbin/diskutil' + ).length + // 4 paths would be 8 df + 8 diskutil un-cached; source+worktree share one + // tmp volume, so caching collapses this to a single probe pair. + expect(dfCalls).toBeLessThanOrEqual(2) + expect(diskutilCalls).toBeLessThanOrEqual(2) + // The copies themselves still happen per path. + expect(execFileAsyncMock.mock.calls.filter(([file]) => file === '/bin/cp')).toHaveLength(4) + }) }) describe('removeWorktreeSymlinks', () => { diff --git a/src/main/ipc/worktree-symlinks.ts b/src/main/ipc/worktree-symlinks.ts index af6307afc8a..fb81cc37df4 100644 --- a/src/main/ipc/worktree-symlinks.ts +++ b/src/main/ipc/worktree-symlinks.ts @@ -1,11 +1,12 @@ -import { symlink, mkdir, stat, lstat, unlink, cp } from 'node:fs/promises' +import { symlink, mkdir, stat, lstat, unlink, cp, realpath } from 'node:fs/promises' import { dirname, isAbsolute, resolve } from 'node:path' import { ApfsCloneUnavailableError, cloneWorktreePathWithApfs, defaultApfsCloneDeps, WorktreeLinkedPathTargetExistsError, - type ApfsCloneDeps + type ApfsCloneDeps, + type DarwinFilesystemCache } from './worktree-apfs-clone' type WorktreeLinkedPathOptions = { @@ -69,9 +70,15 @@ async function createWorktreeLinkedPath( sourceIsDirectory: boolean, sourceIsSymbolicLink: boolean, mode: WorktreeMaterializeMode, - options: WorktreeLinkedPathOptions + options: WorktreeLinkedPathOptions, + apfsFilesystemCache: DarwinFilesystemCache ): Promise { - if (options.platform === 'darwin' && !sourceIsSymbolicLink) { + // Why: copy mode promises each worktree an independent copy; copying the + // symlink itself would recreate a link to the shared target, so edits in the + // worktree would leak back into the primary checkout (or escape it entirely if + // the link points outside). Resolve the real source so we copy content. + const copySource = mode === 'copy' && sourceIsSymbolicLink ? await realpath(source) : source + if (options.platform === 'darwin' && (!sourceIsSymbolicLink || mode === 'copy')) { try { const cloneWorktreePath = options.cloneWorktreePath ?? @@ -80,9 +87,10 @@ async function createWorktreeLinkedPath( cloneSource, cloneTarget, cloneSourceIsDirectory, - options.apfsCloneDeps ?? defaultApfsCloneDeps + options.apfsCloneDeps ?? defaultApfsCloneDeps, + apfsFilesystemCache )) - await cloneWorktreePath(source, target, sourceIsDirectory) + await cloneWorktreePath(copySource, target, sourceIsDirectory) return } catch (error) { if (error instanceof WorktreeLinkedPathTargetExistsError) { @@ -97,7 +105,7 @@ async function createWorktreeLinkedPath( } } if (mode === 'copy') { - await copyWorktreePath(source, target) + await copyWorktreePath(copySource, target) return } await symlinkWorktreePath(source, target, sourceIsDirectory) @@ -122,6 +130,9 @@ async function materializeWorktreePaths( options: WorktreeLinkedPathOptions = {} ): Promise { const effectiveOptions = { platform: process.platform, ...options } + // Why: one df+diskutil probe per distinct volume for the whole materialization, + // not per copied path — see DarwinFilesystemCache. + const apfsFilesystemCache: DarwinFilesystemCache = new Map() for (const rawPath of paths) { const safePath = getSafeRelativePath(rawPath) @@ -159,7 +170,8 @@ async function materializeWorktreePaths( sourceIsDirectory, sourceIsSymbolicLink, mode, - effectiveOptions + effectiveOptions, + apfsFilesystemCache ) } catch (error) { console.error(