diff --git a/gitnexus/src/cli/clean.ts b/gitnexus/src/cli/clean.ts index ed8429490..10c2e8b03 100644 --- a/gitnexus/src/cli/clean.ts +++ b/gitnexus/src/cli/clean.ts @@ -40,7 +40,6 @@ import { reclaimAfterSlotRemoval, reclaimSharedStore, removeLegacyLocalIndex, - removeSharedStorePointer, withCheckoutSlotLock, type ReclaimResult, } from '../storage/shared-store-lifecycle.js'; @@ -181,8 +180,9 @@ const collectSharedStores = async (force: boolean): Promise => { } for (const name of names) { const root = path.join(storesDir, name); - // Stray files (`.DS_Store`) are not stores. - if (!(await fs.stat(root)).isDirectory()) continue; + // Stray files (`.DS_Store`) are not stores, and a symlink is not followed: + // reclaim deletes under the root it is given. + if (!(await fs.lstat(root)).isDirectory()) continue; // Without --force this is a preview: same selection, nothing deleted. const result = await reclaimSharedStore(root, { gc: true, dryRun: !force }); console.log( @@ -377,14 +377,16 @@ export const cleanCommand = async (options?: { for (const entry of entries) { try { const storagePath = await requireDeletableStoragePath(entry); - await withCheckoutSlotLock(storagePath, async () => { - await fs.rm(storagePath, { recursive: true, force: true }); - await unregisterRepo(entry.path); - }); + await withCheckoutSlotLock( + storagePath, + async () => { + await fs.rm(storagePath, { recursive: true, force: true }); + await unregisterRepo(entry.path); + }, + entry.path, + ); console.log(t('clean.deletedRepo', { name: entry.name, storagePath })); - const reclaim = await reclaimAfterSlotRemoval(storagePath); - if (reclaim) await removeSharedStorePointer(entry.path); - reportReclaim(reclaim); + reportReclaim(await reclaimAfterSlotRemoval(storagePath)); } catch (err) { if (err instanceof StorageDeletionError) { logger.error(`Refusing to clean ${entry.name}: ${err.message}`); @@ -428,14 +430,16 @@ export const cleanCommand = async (options?: { } try { - await withCheckoutSlotLock(storagePath, async () => { - await fs.rm(storagePath, { recursive: true, force: true }); - await unregisterRepo(repo.repoPath); - }); + await withCheckoutSlotLock( + storagePath, + async () => { + await fs.rm(storagePath, { recursive: true, force: true }); + await unregisterRepo(repo.repoPath); + }, + repo.repoPath, + ); console.log(t('common.deleted', { target: storagePath })); - const reclaim = await reclaimAfterSlotRemoval(storagePath); - if (reclaim) await removeSharedStorePointer(repo.repoPath); - reportReclaim(reclaim); + reportReclaim(await reclaimAfterSlotRemoval(storagePath)); } catch (err) { logger.error({ err }, 'Failed to delete:'); } diff --git a/gitnexus/src/cli/remove.ts b/gitnexus/src/cli/remove.ts index 4f3ce8c94..060e2ef93 100644 --- a/gitnexus/src/cli/remove.ts +++ b/gitnexus/src/cli/remove.ts @@ -31,7 +31,6 @@ import { reclaimAfterSlotRemoval, - removeSharedStorePointer, withCheckoutSlotLock, } from '../storage/shared-store-lifecycle.js'; import fs from 'fs/promises'; @@ -103,11 +102,15 @@ export const removeCommand = async (target: string, options?: { force?: boolean // orphaned — `listRegisteredRepos({ validate: true })` prunes those on // next read, so the failure is self-healing. try { - await withCheckoutSlotLock(storagePath, async () => { - await fs.rm(storagePath, { recursive: true, force: true }); - await unregisterRepo(entry.path); - }); - if (await reclaimAfterSlotRemoval(storagePath)) await removeSharedStorePointer(entry.path); + await withCheckoutSlotLock( + storagePath, + async () => { + await fs.rm(storagePath, { recursive: true, force: true }); + await unregisterRepo(entry.path); + }, + entry.path, + ); + await reclaimAfterSlotRemoval(storagePath); console.log(t('remove.removed', { name: entry.name })); console.log(` ${t('common.path')}: ${entry.path}`); console.log(` ${t('common.storage')}: ${entry.storagePath}`); diff --git a/gitnexus/src/core/shared-store-analyze.ts b/gitnexus/src/core/shared-store-analyze.ts index acd9cd29a..a8e3fde4b 100644 --- a/gitnexus/src/core/shared-store-analyze.ts +++ b/gitnexus/src/core/shared-store-analyze.ts @@ -478,10 +478,10 @@ export const leaveSharedStore = async ( requireExclusiveIndexLock(lock, `Cannot acquire the index lock at ${previousSlot}.`); await registerLeftStore(repoPath, newStoragePath); await fs.rm(previousSlot, { recursive: true, force: true }); + await removeSharedStorePointer(repoPath); } finally { lock.release(); } - await removeSharedStorePointer(repoPath); await reclaimAfterSlotRemoval(previousSlot); log(`Shared store: left ${previousSlot}.`); }; diff --git a/gitnexus/src/storage/shared-store-lifecycle.ts b/gitnexus/src/storage/shared-store-lifecycle.ts index 53f01b98c..e7a05ad0c 100644 --- a/gitnexus/src/storage/shared-store-lifecycle.ts +++ b/gitnexus/src/storage/shared-store-lifecycle.ts @@ -252,12 +252,17 @@ export const writeSharedStorePointer = async ( await fs.writeFile(path.join(dir, '.gitignore'), '*\n', { flag: 'wx' }).catch(() => {}); }; -/** Remove the pointer file (the directory stays if it holds anything else). */ +/** + * Remove the pointer file. The directory stays if it holds anything else, or + * if it cannot be listed (its contents are then unknown). + */ export const removeSharedStorePointer = async (checkoutPath: string): Promise => { const dir = path.join(checkoutPath, GITNEXUS_DIR); await fs.rm(path.join(dir, SHARED_STORE_POINTER), { force: true }); - const rest = await listDir(dir); - if (rest.every((name) => POINTER_DIR_KEEP.has(name))) { + const rest = await fs + .readdir(dir) + .catch((err: NodeJS.ErrnoException) => (err.code === 'ENOENT' ? [] : null)); + if (rest?.every((name) => POINTER_DIR_KEEP.has(name))) { await fs.rm(dir, { recursive: true, force: true }); } }; @@ -310,20 +315,24 @@ export const removeLegacyLocalIndex = async ( /** * Run `fn` (delete a slot, unregister its checkout) under the slot's index - * lock when `storagePath` is a shared-store checkout slot. An analyze holds - * that lock until it has registered the checkout, so it cannot re-register a - * slot this removes or write into it afterwards. Other storage runs `fn` - * directly, as before. + * lock when `storagePath` is a shared-store checkout slot, then remove + * `checkoutPath`'s store pointer before releasing it. An analyze holds that + * lock until it has registered the checkout and written its pointer, so it + * cannot re-register a slot this removes, write into it, or have its new + * pointer deleted afterwards. Other storage runs `fn` directly, as before. */ export const withCheckoutSlotLock = async ( storagePath: string, fn: () => Promise, + checkoutPath?: string, ): Promise => { if (!storeRootOfCheckoutSlot(storagePath)) return fn(); const lock = await acquireIndexLock(storagePath); try { requireExclusiveIndexLock(lock, `Cannot acquire the index lock at ${storagePath}.`); - return await fn(); + const result = await fn(); + if (checkoutPath) await removeSharedStorePointer(checkoutPath); + return result; } finally { lock.release(); } diff --git a/gitnexus/test/integration/shared-store-clean.test.ts b/gitnexus/test/integration/shared-store-clean.test.ts index f7ef0b705..00cef0c54 100644 --- a/gitnexus/test/integration/shared-store-clean.test.ts +++ b/gitnexus/test/integration/shared-store-clean.test.ts @@ -12,6 +12,7 @@ import { import { reclaimAfterSlotRemoval, reclaimSharedStore, + removeSharedStorePointer, } from '../../src/storage/shared-store-lifecycle.js'; import { getGlobalDir } from '../../src/storage/global-dir.js'; import { createTempDir } from '../helpers/test-db.js'; @@ -152,6 +153,23 @@ describe('shared store clean (#3352)', () => { expect(logs.join('\n')).toMatch(/Shared store .*: dropped 0 checkout/); }, 240_000); + it('clean --gc does not follow a symlink in the stores directory', async () => { + const decoy = path.join(tmpRepo.dbPath, 'decoy'); + await fs.mkdir(path.join(decoy, 'checkouts'), { recursive: true }); + await fs.mkdir(path.join(decoy, 'commits', 'ddddddd-4444444444444444'), { recursive: true }); + const stores = path.join(tmpHome.dbPath, 'stores'); + await fs.mkdir(stores, { recursive: true }); + await fs.symlink( + decoy, + path.join(stores, 'repo-0123456789ab'), + process.platform === 'win32' ? 'junction' : 'dir', + ); + + await cleanIn(main, { gc: true, force: true }); + + expect(existsSync(path.join(decoy, 'commits', 'ddddddd-4444444444444444'))).toBe(true); + }, 240_000); + it('clean --gc without --force previews and deletes nothing', async () => { await analyze(wtA); await fs.writeFile(path.join(wtB, 'b.ts'), 'export function beta() { return 2; }\n'); @@ -301,6 +319,27 @@ describe('reclaimSharedStore', () => { expect(existsSync(graph)).toBe(true); }); + it('keeps a checkout .gitnexus directory it cannot list', async () => { + const dir = path.join(tmpHome.dbPath, 'checkout', '.gitnexus'); + await fs.mkdir(dir, { recursive: true }); + await fs.writeFile(path.join(dir, 'store.json'), '{}'); + const realReaddir = fs.readdir; + const readdir = vi.spyOn(fs, 'readdir').mockImplementation((async ( + target: string, + ...rest: unknown[] + ) => { + if (String(target) === dir) throw Object.assign(new Error('denied'), { code: 'EACCES' }); + return (realReaddir as (...a: unknown[]) => Promise)(target, ...rest); + }) as typeof fs.readdir); + try { + await removeSharedStorePointer(path.dirname(dir)); + } finally { + readdir.mockRestore(); + } + expect(existsSync(dir)).toBe(true); + expect(existsSync(path.join(dir, 'store.json'))).toBe(false); + }); + it('reports a graph it cannot delete instead of failing', async () => { const orphan = await commitGraph('ccccccc-3333333333333333'); await member('wt-000000000000', {});