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

- removing a checkout's storage unregisters it and removes its store
  pointer before deleting the slot directory, which the file lock
  backend uses for its lock file; withCheckoutSlotLock becomes
  removeCheckoutStorage, used by remove, clean, and DELETE /api/repo,
  and analyze --no-share follows the same order
- the clean --gc preview skips a slot whose index lock is held, matching
  what --force would drop

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

gitnexus/src/cli/clean.ts

@@ -40,7 +40,7 @@ import {
   reclaimAfterSlotRemoval,
   reclaimSharedStore,
   removeLegacyLocalIndex,
-  withCheckoutSlotLock,
+  removeCheckoutStorage,
   type ReclaimResult,
 } from '../storage/shared-store-lifecycle.js';
 
@@ -385,14 +385,7 @@ 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);
-          },
-          entry.path,
-        );
+        await removeCheckoutStorage(storagePath, () => unregisterRepo(entry.path), entry.path);
         console.log(t('clean.deletedRepo', { name: entry.name, storagePath }));
         reportReclaim(await reclaimAfterSlotRemoval(storagePath));
       } catch (err) {
@@ -438,14 +431,7 @@ export const cleanCommand = async (options?: {
   }
 
   try {
-    await withCheckoutSlotLock(
-      storagePath,
-      async () => {
-        await fs.rm(storagePath, { recursive: true, force: true });
-        await unregisterRepo(repo.repoPath);
-      },
-      repo.repoPath,
-    );
+    await removeCheckoutStorage(storagePath, () => unregisterRepo(repo.repoPath), repo.repoPath);
     console.log(t('common.deleted', { target: storagePath }));
     reportReclaim(await reclaimAfterSlotRemoval(storagePath));
   } catch (err) {

gitnexus/src/cli/remove.ts

@@ -31,9 +31,8 @@
 
 import {
   reclaimAfterSlotRemoval,
-  withCheckoutSlotLock,
+  removeCheckoutStorage,
 } from '../storage/shared-store-lifecycle.js';
-import fs from 'fs/promises';
 import { logger } from '../core/logger.js';
 import { cliError } from './cli-message.js';
 import { t } from './i18n/index.js';
@@ -102,14 +101,7 @@ 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);
-      },
-      entry.path,
-    );
+    await removeCheckoutStorage(storagePath, () => unregisterRepo(entry.path), entry.path);
     await reclaimAfterSlotRemoval(storagePath);
     console.log(t('remove.removed', { name: entry.name }));
     console.log(`   ${t('common.path')}:    ${entry.path}`);

gitnexus/src/core/shared-store-analyze.ts

@@ -485,8 +485,9 @@ export const leaveSharedStore = async (
   try {
     requireExclusiveIndexLock(lock, `Cannot acquire the index lock at ${previousSlot}.`);
     await registerLeftStore(repoPath, newStoragePath);
-    await fs.rm(previousSlot, { recursive: true, force: true });
     await removeSharedStorePointer(repoPath);
+    // Last: the file lock backend keeps its lock file inside this directory.
+    await fs.rm(previousSlot, { recursive: true, force: true });
   } finally {
     lock.release();
   }

gitnexus/src/server/api.ts

@@ -17,7 +17,7 @@ import { ensurePrivateSharedGraph } from '../core/shared-store-analyze.js';
 import { resolveGraphPath } from '../storage/shared-store.js';
 import {
   reclaimAfterSlotRemoval,
-  withCheckoutSlotLock,
+  removeCheckoutStorage,
 } from '../storage/shared-store-lifecycle.js';
 import express from 'express';
 import cors from 'cors';
@@ -1328,9 +1328,7 @@ export const createServer = async (port: number, host: string = '127.0.0.1') =>
         } catch {}
 
         // 1. Delete the .gitnexus index/storage directory
-        await withCheckoutSlotLock(storagePath, () =>
-          fs.rm(storagePath, { recursive: true, force: true }),
-        ).catch(() => {});
+        await removeCheckoutStorage(storagePath).catch(() => {});
         await reclaimAfterSlotRemoval(storagePath);
 
         // 2. Delete the cloned repo dir if it lives under ~/.gitnexus/repos/.

gitnexus/src/storage/shared-store-lifecycle.ts

@@ -124,13 +124,10 @@ export const reclaimSharedStoreLocked = async (
   if (opts.gc) {
     const orphans = await orphanMembers(slots);
     for (const slot of [...orphans]) {
-      if (opts.dryRun) {
-        result.droppedMembers.push(slot);
-        continue;
-      }
       // An analyze holds its slot's index lock until it has registered the
       // checkout, so a slot that is seeded but not yet registered is busy,
-      // not orphaned. Judge and delete only a slot whose lock is free.
+      // not orphaned. Judge (and, unless previewing, delete) only a slot
+      // whose lock is free, so the preview matches what --force would do.
       let lock: IndexLockHandle;
       try {
         lock = await acquireIndexLock(slot, { timeoutMs: 1 });
@@ -143,7 +140,7 @@ export const reclaimSharedStoreLocked = async (
           orphans.delete(slot);
           continue;
         }
-        await fs.rm(slot, { recursive: true, force: true });
+        if (!opts.dryRun) await fs.rm(slot, { recursive: true, force: true });
         result.droppedMembers.push(slot);
       } finally {
         lock.release();
@@ -319,25 +316,31 @@ 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, 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.
+ * Delete a checkout's index storage and run `unregister`. For a shared-store
+ * checkout slot this happens under the slot's index lock, which an analyze
+ * holds until it has registered the checkout and written its pointer, so it
+ * cannot re-register a removed slot, write into it, or have its new pointer
+ * deleted. There `unregister` and removing `checkoutPath`'s pointer run
+ * first and the slot directory goes last: the file lock backend keeps its
+ * lock file inside that directory, so nothing may depend on the lock after
+ * it is deleted. Other storage is deleted, then unregistered, as before.
  */
-export const withCheckoutSlotLock = async <T>(
+export const removeCheckoutStorage = async (
   storagePath: string,
-  fn: () => Promise<T>,
+  unregister: () => Promise<void> = async () => {},
   checkoutPath?: string,
-): Promise<T> => {
-  if (!storeRootOfCheckoutSlot(storagePath)) return fn();
+): Promise<void> => {
+  if (!storeRootOfCheckoutSlot(storagePath)) {
+    await fs.rm(storagePath, { recursive: true, force: true });
+    await unregister();
+    return;
+  }
   const lock = await acquireIndexLock(storagePath);
   try {
     requireExclusiveIndexLock(lock, `Cannot acquire the index lock at ${storagePath}.`);
-    const result = await fn();
+    await unregister();
     if (checkoutPath) await removeSharedStorePointer(checkoutPath);
-    return result;
+    await fs.rm(storagePath, { recursive: true, force: true });
   } finally {
     lock.release();
   }

gitnexus/test/integration/shared-store-clean.test.ts

@@ -1,5 +1,5 @@
 import { execFileSync } from 'child_process';
-import { existsSync } from 'fs';
+import { existsSync, readFileSync } from 'fs';
 import fs from 'fs/promises';
 import path from 'path';
 import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it, vi } from 'vitest';
@@ -138,6 +138,32 @@ describe('shared store clean (#3352)', () => {
     expect(existsSync(layoutOf(wtA).root)).toBe(false);
   }, 240_000);
 
+  it('deletes the slot directory last, after unregistering and removing the pointer', async () => {
+    await analyze(wtA);
+    const slot = layoutOf(wtA).checkoutSlot;
+    const pointer = path.join(wtA, '.gitnexus', 'store.json');
+    const registry = path.join(tmpHome.dbPath, 'registry.json');
+    expect(readFileSync(registry, 'utf-8')).toContain(JSON.stringify(wtA).slice(1, -1));
+    const seen: { pointer: boolean; registered: boolean }[] = [];
+    const realRm = fs.rm;
+    const rm = vi.spyOn(fs, 'rm').mockImplementation(async (target, options) => {
+      if (String(target) === slot) {
+        seen.push({
+          pointer: existsSync(pointer),
+          registered: readFileSync(registry, 'utf-8').includes(JSON.stringify(wtA).slice(1, -1)),
+        });
+      }
+      return realRm(target, options);
+    });
+    try {
+      await cleanIn(wtA, { force: true });
+    } finally {
+      rm.mockRestore();
+    }
+    expect(seen).toEqual([{ pointer: false, registered: false }]);
+    expect(existsSync(slot)).toBe(false);
+  }, 240_000);
+
   it('previews without --force and deletes nothing', async () => {
     await analyze(wtA);
     const layout = layoutOf(wtA);
@@ -307,6 +333,21 @@ describe('reclaimSharedStore', () => {
     expect(after.droppedMembers).toEqual([slot]);
   });
 
+  it('clean --gc preview does not count a slot whose index lock is held', async () => {
+    const slot = await member('busy-000000000000', { repoPath: '/nonexistent/checkout' });
+    const { acquireIndexLock } = await import('../../src/storage/index-lock.js');
+    const lock = await acquireIndexLock(slot);
+    try {
+      const preview = await reclaimSharedStore(layout().root, { gc: true, dryRun: true });
+      expect(preview.droppedMembers).toEqual([]);
+    } finally {
+      lock.release();
+    }
+    const after = await reclaimSharedStore(layout().root, { gc: true, dryRun: true });
+    expect(after.droppedMembers).toEqual([slot]);
+    expect(existsSync(slot)).toBe(true);
+  });
+
   it('counts references correctly when GITNEXUS_HOME is relative', async () => {
     const absoluteHome = process.env.GITNEXUS_HOME as string;
     process.env.GITNEXUS_HOME = path.relative(process.cwd(), absoluteHome);