fix(storage): address sixth review round on the shared store (#3374)

- clean, remove, and analyze --no-share remove the checkout's store
  pointer while still holding the slot's index lock, so an analyze that
  takes the lock next cannot have its new pointer deleted
- clean --gc does not follow a symlink under stores/ (lstat)
- removing a store pointer keeps the checkout's .gitnexus directory when
  it cannot be listed, instead of treating it as empty

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

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<void> => {
   }
   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:');
   }

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}`);

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}.`);
 };

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<void> => {
   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 <T>(
   storagePath: string,
   fn: () => Promise<T>,
+  checkoutPath?: string,
 ): Promise<T> => {
   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();
   }

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<unknown>)(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', {});