mirror of
https://github.com/stablyai/orca.git
synced 2026-10-19 00:03:20 +00:00
fix(worktrees): dereference symlinked .worktreeinclude entries + cache APFS volume probe
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.
This commit is contained in:
@@ -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<number, Promise<DarwinFilesystemInfo>>
|
||||
|
||||
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<DarwinFilesystemInfo> {
|
||||
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<void> {
|
||||
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<void> {
|
||||
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.
|
||||
|
||||
@@ -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', () => {
|
||||
|
||||
@@ -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<void> {
|
||||
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<void> {
|
||||
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(
|
||||
|
||||
Reference in New Issue
Block a user