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

- analyze --no-share leaves the shared store even when routed to a
  branch sub-index (gate on sharedStore, not placement.branch)
- clean --gc aborts a store whose member listing is unreadable instead
  of treating it as empty, and skips non-directory entries under stores/
- reusing an existing commit graph no longer fails a finished analyze
  when the redundant private graph cannot be wiped
- a store pointer holding JSON null, a number, a string, or an array is
  treated as invalid
- clean, remove, and DELETE /api/repo hold the checkout slot's index
  lock while deleting the slot
- the server analyze launcher waits on the checkout slot the worker
  actually writes
- sync the Factory hook copy; isolate hook tests from storage overrides;
  use a junction for the Windows worktree alias; the relative
  GITNEXUS_HOME test now sets a relative home

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

gitnexus-factory-plugin/hooks/registry-query.cjs

@@ -293,6 +293,37 @@ function resolveEntryStoragePath(entry) {
   return path.resolve(path.join(entry.path, GITNEXUS_DIR));
 }
 
+// A single path segment: `..repo-<hash>` is a legal slot name, `..` is not.
+function isDirectChild(parent, child) {
+  const rel = path.relative(parent, child);
+  return rel !== '' && rel !== '..' && !path.isAbsolute(rel) && !rel.includes(path.sep);
+}
+
+// Mirror gitnexus/src/storage/shared-store.ts resolveGraphPath (#3352): a
+// shared-store checkout slot may read a commit graph in the same store
+// instead of owning <slot>/lbug. Any other recorded value is ignored.
+function resolveGraphPath(storagePath, metadata) {
+  const own = path.join(storagePath, LBUG_DIRECTORY);
+  const storesRoot = path.resolve(
+    process.env.GITNEXUS_HOME || path.join(os.homedir(), '.gitnexus'),
+    'stores',
+  );
+  const slot = path.resolve(storagePath);
+  const checkoutsDir = path.dirname(slot);
+  const root = path.dirname(checkoutsDir);
+  if (path.basename(checkoutsDir) !== 'checkouts') return own;
+  if (!isDirectChild(checkoutsDir, slot) || !isDirectChild(storesRoot, root)) return own;
+  const recorded = metadata && metadata.graphPath;
+  if (typeof recorded !== 'string' || !path.isAbsolute(recorded)) return own;
+  const graph = path.resolve(recorded);
+  // Only a published `<commit>-<featureKey>` dir, never `.publish-*` staging.
+  const valid =
+    path.basename(graph) === LBUG_DIRECTORY &&
+    isDirectChild(path.join(root, 'commits'), path.dirname(graph)) &&
+    /^[0-9a-f]{7,64}-[0-9a-f]{8,64}$/.test(path.basename(path.dirname(graph)));
+  return valid ? graph : own;
+}
+
 function hasLocalIndexSignal(storagePath) {
   try {
     return (
@@ -382,7 +413,9 @@ function findRegisteredRepo(cwd) {
       best = {
         path: entry.path,
         storagePath,
-        lbugPath: path.join(indexDir, LBUG_DIRECTORY),
+        lbugPath: branchIsIndexed
+          ? path.join(indexDir, LBUG_DIRECTORY)
+          : resolveGraphPath(storagePath, ownershipMetadata),
         metadata: branchIsIndexed ? readIndexMetadata(indexDir) : ownershipMetadata,
       };
     }

gitnexus/src/cli/clean.ts

@@ -41,6 +41,7 @@ import {
   reclaimSharedStore,
   removeLegacyLocalIndex,
   removeSharedStorePointer,
+  withCheckoutSlotLock,
   type ReclaimResult,
 } from '../storage/shared-store-lifecycle.js';
 
@@ -180,6 +181,8 @@ 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;
     // Without --force this is a preview: same selection, nothing deleted.
     const result = await reclaimSharedStore(root, { gc: true, dryRun: !force });
     console.log(
@@ -374,8 +377,10 @@ export const cleanCommand = async (options?: {
     for (const entry of entries) {
       try {
         const storagePath = await requireDeletableStoragePath(entry);
-        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);
+        });
         console.log(t('clean.deletedRepo', { name: entry.name, storagePath }));
         const reclaim = await reclaimAfterSlotRemoval(storagePath);
         if (reclaim) await removeSharedStorePointer(entry.path);
@@ -423,8 +428,10 @@ export const cleanCommand = async (options?: {
   }
 
   try {
-    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);
+    });
     console.log(t('common.deleted', { target: storagePath }));
     const reclaim = await reclaimAfterSlotRemoval(storagePath);
     if (reclaim) await removeSharedStorePointer(repo.repoPath);

gitnexus/src/cli/remove.ts

@@ -32,6 +32,7 @@
 import {
   reclaimAfterSlotRemoval,
   removeSharedStorePointer,
+  withCheckoutSlotLock,
 } from '../storage/shared-store-lifecycle.js';
 import fs from 'fs/promises';
 import { logger } from '../core/logger.js';
@@ -102,8 +103,10 @@ 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 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);
+    });
     if (await reclaimAfterSlotRemoval(storagePath)) await removeSharedStorePointer(entry.path);
     console.log(t('remove.removed', { name: entry.name }));
     console.log(`   ${t('common.path')}:    ${entry.path}`);

gitnexus/src/core/run-analyze.ts

@@ -1328,7 +1328,9 @@ export async function runFullAnalysis(
       );
       if (flatShared) {
         await publishSharedGraph(flatShared, repoPath, writeTarget.currentCommit, log);
-      } else if (!writeTarget.placement.branch) {
+      } else if (!writeTarget.sharedStore) {
+        // Also for a `--branch` run routed to a local branch sub-slot: the
+        // checkout still leaves the store.
         // Leaving a store (`--no-share`, or sharing turned off): the up-to-date
         // path does not re-register, so point the registry at the new storage
         // before the old slot goes away.

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

@@ -345,7 +345,16 @@ export const publishSharedGraph = async (
       const targetGraph = path.join(target, LBUG_DIRECTORY);
       let published = await exists(targetGraph);
       if (published) {
-        await wipeLbugDbFiles(own);
+        try {
+          await wipeLbugDbFiles(own);
+        } catch (err) {
+          // An open reader (Windows) can block the delete. The analysis
+          // already succeeded; keep the private graph and try next run.
+          published = false;
+          log(
+            `Shared store: could not drop the private graph (${(err as Error).message}); keeping it.`,
+          );
+        }
       } else if ((await exists(own)) && (await inspectLbugSidecars(own)).kind === 'clean') {
         await fs.mkdir(layout.commitsDir, { recursive: true });
         const staging = path.join(layout.commitsDir, `.publish-${randomUUID()}`);
@@ -375,9 +384,14 @@ export const publishSharedGraph = async (
       delete meta.graphPath;
       await saveMeta(slot, meta);
     }
-    const reclaimed = await reclaimSharedStoreLocked(layout.root);
-    if (reclaimed.removed.length > 0) {
-      log(`Shared store: removed ${reclaimed.removed.length} commit graph(s) no checkout uses.`);
+    // Best effort: an unreadable store must not fail a finished analysis.
+    try {
+      const reclaimed = await reclaimSharedStoreLocked(layout.root);
+      if (reclaimed.removed.length > 0) {
+        log(`Shared store: removed ${reclaimed.removed.length} commit graph(s) no checkout uses.`);
+      }
+    } catch (err) {
+      log(`Shared store: skipped cleanup (${(err as Error).message}).`);
     }
   });
 

gitnexus/src/server/analyze-launch.ts

@@ -10,7 +10,11 @@
  * path is resolved relative to `import.meta.url`.
  */
 
-import { resolveGraphPath, storeRootOfCheckoutSlot } from '../storage/shared-store.js';
+import {
+  resolveGraphPath,
+  resolveSharedStore,
+  storeRootOfCheckoutSlot,
+} from '../storage/shared-store.js';
 import path from 'path';
 import { existsSync, statSync } from 'node:fs';
 import { fork } from 'child_process';
@@ -338,7 +342,10 @@ export function createLaunchAnalysisWorker(deps: LaunchDeps) {
           const settle = msg.result.alreadyUpToDate
             ? Promise.resolve(true)
             : waitForSettledIndex(
-                analyzeLockKey,
+                // A worktree's first analyze writes a shared-store slot the
+                // launcher's pre-run lookup could not see yet (#3352); the
+                // worker picks it with this same resolver.
+                resolveSharedStore(msg.result.repoPath)?.checkoutSlot ?? analyzeLockKey,
                 jobStartMs,
                 opts.branch,
                 msg.result.isPrimaryBranch,

gitnexus/src/server/api.ts

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

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

@@ -62,6 +62,17 @@ export interface ReclaimResult {
 
 const listDir = (dir: string): Promise<string[]> => fs.readdir(dir).catch(() => [] as string[]);
 
+/**
+ * Listing for reclaim decisions: only a missing directory is empty. Any other
+ * read error aborts, because treating an unreadable `checkouts/` as "no
+ * members" would delete every commit graph as unreferenced.
+ */
+const listDirStrict = (dir: string): Promise<string[]> =>
+  fs.readdir(dir).catch((err: NodeJS.ErrnoException) => {
+    if (err.code === 'ENOENT') return [] as string[];
+    throw err;
+  });
+
 /**
  * Member slots that no registry entry uses any more: the checkout directory is
  * gone, or its entry moved elsewhere (`--no-share`, sharing turned off). The
@@ -104,7 +115,7 @@ export const reclaimSharedStoreLocked = async (
   const checkoutsDir = path.join(storeRoot, 'checkouts');
   const commitsDir = path.join(storeRoot, 'commits');
   const referenced = new Set<string>();
-  let slots = (await listDir(checkoutsDir)).map((name) => path.join(checkoutsDir, name));
+  let slots = (await listDirStrict(checkoutsDir)).map((name) => path.join(checkoutsDir, name));
   if (opts.gc) {
     const orphans = await orphanMembers(slots);
     for (const slot of [...orphans]) {
@@ -140,7 +151,7 @@ export const reclaimSharedStoreLocked = async (
     if (graphPath) referenced.add(path.dirname(path.resolve(graphPath)));
   }
 
-  for (const name of await listDir(commitsDir)) {
+  for (const name of await listDirStrict(commitsDir)) {
     const dir = path.join(commitsDir, name);
     if (referenced.has(dir)) continue;
     if (opts.dryRun) {
@@ -158,7 +169,7 @@ export const reclaimSharedStoreLocked = async (
   }
 
   const isEmpty = async (): Promise<boolean> =>
-    (await listDir(checkoutsDir)).length + (await listDir(commitsDir)).length === 0;
+    (await listDirStrict(checkoutsDir)).length + (await listDirStrict(commitsDir)).length === 0;
   if (!opts.dryRun && (await isEmpty())) {
     // Also hold the cache lock (publish -> cache, the only nesting order) so
     // a member saving caches cannot lose them, then re-check: a new member's
@@ -296,3 +307,24 @@ export const removeLegacyLocalIndex = async (
   }
   return legacy;
 };
+
+/**
+ * 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.
+ */
+export const withCheckoutSlotLock = async <T>(
+  storagePath: string,
+  fn: () => Promise<T>,
+): 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();
+  } finally {
+    lock.release();
+  }
+};

gitnexus/src/storage/shared-store.ts

@@ -292,6 +292,7 @@ export const readSharedStorePointer = (checkoutPath: string): string | null => {
   } catch {
     return null;
   }
+  if (!pointer || typeof pointer !== 'object' || Array.isArray(pointer)) return null;
   const { checkoutSlot: recorded, storeKey } = pointer;
   if (typeof recorded !== 'string' || !path.isAbsolute(recorded)) return null;
   if (typeof storeKey !== 'string') return null;

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

@@ -13,6 +13,7 @@ import {
   reclaimAfterSlotRemoval,
   reclaimSharedStore,
 } from '../../src/storage/shared-store-lifecycle.js';
+import { getGlobalDir } from '../../src/storage/global-dir.js';
 import { createTempDir } from '../helpers/test-db.js';
 
 // These suites exercise sharing; an inherited opt-out would silently disable it.
@@ -144,6 +145,13 @@ describe('shared store clean (#3352)', () => {
     expect(await commitDirs(layout)).toHaveLength(1);
   }, 240_000);
 
+  it('clean --gc skips stray files in the stores directory', async () => {
+    await analyze(wtA);
+    await fs.writeFile(path.join(tmpHome.dbPath, 'stores', '.DS_Store'), 'x');
+    const logs = await cleanIn(main, { gc: true, force: true });
+    expect(logs.join('\n')).toMatch(/Shared store .*: dropped 0 checkout/);
+  }, 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');
@@ -256,12 +264,41 @@ describe('reclaimSharedStore', () => {
   });
 
   it('counts references correctly when GITNEXUS_HOME is relative', async () => {
-    const referenced = await commitGraph('ddddddd-4444444444444444');
-    await member('wt-000000000000', { graphPath: path.join(referenced, 'lbug') });
-    const relativeRoot = path.relative(process.cwd(), layout().root);
-    const result = await reclaimSharedStore(relativeRoot);
-    expect(result.removed).toEqual([]);
-    expect(existsSync(referenced)).toBe(true);
+    const absoluteHome = process.env.GITNEXUS_HOME as string;
+    process.env.GITNEXUS_HOME = path.relative(process.cwd(), absoluteHome);
+    try {
+      const referenced = await commitGraph('ddddddd-4444444444444444');
+      await member('wt-000000000000', { graphPath: path.resolve(referenced, 'lbug') });
+      // The root exactly as `clean --gc` builds it: relative under this home.
+      const gcRoot = path.join(getGlobalDir(), 'stores', layout().key);
+      expect(path.isAbsolute(gcRoot)).toBe(false);
+      const result = await reclaimSharedStore(gcRoot);
+      expect(result.removed).toEqual([]);
+      expect(existsSync(referenced)).toBe(true);
+    } finally {
+      process.env.GITNEXUS_HOME = absoluteHome;
+    }
+  });
+
+  it('aborts instead of deleting graphs when the member list cannot be read', async () => {
+    const graph = await commitGraph('eeeeeee-5555555555555555');
+    await member('wt-000000000000', { graphPath: path.join(graph, 'lbug') });
+    const realReaddir = fs.readdir;
+    const readdir = vi.spyOn(fs, 'readdir').mockImplementation((async (
+      target: string,
+      ...rest: unknown[]
+    ) => {
+      if (String(target) === layout().checkoutsDir) {
+        throw Object.assign(new Error('denied'), { code: 'EACCES' });
+      }
+      return (realReaddir as (...a: unknown[]) => Promise<unknown>)(target, ...rest);
+    }) as typeof fs.readdir);
+    try {
+      await expect(reclaimSharedStore(layout().root, { gc: true })).rejects.toThrow(/denied/);
+    } finally {
+      readdir.mockRestore();
+    }
+    expect(existsSync(graph)).toBe(true);
   });
 
   it('reports a graph it cannot delete instead of failing', async () => {

gitnexus/test/unit/hooks-shared-store.test.ts

@@ -29,6 +29,15 @@ const HOOK_COPIES = [
     'hooks',
     'registry-query.cjs',
   ),
+  path.resolve(
+    __dirname,
+    '..',
+    '..',
+    '..',
+    'gitnexus-factory-plugin',
+    'hooks',
+    'registry-query.cjs',
+  ),
 ];
 
 type HookRepo = { storagePath: string; lbugPath: string } | null;
@@ -42,6 +51,9 @@ describe('registry-query shared store graph (#3352)', () => {
   let slot: string;
   let commitGraph: string;
   const savedHome = process.env.GITNEXUS_HOME;
+  // Either storage override takes precedence over the registry row in the hook.
+  const savedStoragePath = process.env.GITNEXUS_STORAGE_PATH;
+  const savedStorageRoot = process.env.GITNEXUS_STORAGE_ROOT;
 
   const writeSlot = (meta: Record<string, unknown>) => {
     fs.mkdirSync(slot, { recursive: true });
@@ -70,11 +82,17 @@ describe('registry-query shared store graph (#3352)', () => {
       ]),
     );
     process.env.GITNEXUS_HOME = home;
+    delete process.env.GITNEXUS_STORAGE_PATH;
+    delete process.env.GITNEXUS_STORAGE_ROOT;
   });
 
   afterEach(() => {
     if (savedHome === undefined) delete process.env.GITNEXUS_HOME;
     else process.env.GITNEXUS_HOME = savedHome;
+    if (savedStoragePath === undefined) delete process.env.GITNEXUS_STORAGE_PATH;
+    else process.env.GITNEXUS_STORAGE_PATH = savedStoragePath;
+    if (savedStorageRoot === undefined) delete process.env.GITNEXUS_STORAGE_ROOT;
+    else process.env.GITNEXUS_STORAGE_ROOT = savedStorageRoot;
     fs.rmSync(tmp, { recursive: true, force: true });
   });
 

gitnexus/test/unit/storage/shared-store.test.ts

@@ -6,6 +6,7 @@ import { afterEach, beforeEach, describe, expect, it } from 'vitest';
 import {
   commitGraphDir,
   isSharedStoreDisabled,
+  readSharedStorePointer,
   resolveGraphPath,
   resolveSharedStore,
   resolveSharedStoreKey,
@@ -171,7 +172,7 @@ describe('sharedStoreLayout', () => {
   it('maps a symlinked spelling of a worktree to the same slot', async () => {
     const { wts } = await makeRepo(['wt']);
     const link = path.join(await makeTempDir('gn-shared-link-'), 'alias');
-    await fs.symlink(wts[0], link);
+    await fs.symlink(wts[0], link, process.platform === 'win32' ? 'junction' : 'dir');
     expect(layoutOf(link).checkoutSlot).toBe(layoutOf(wts[0]).checkoutSlot);
   });
 
@@ -383,3 +384,15 @@ describe('bare repositories (#3352 review)', () => {
     expect(resolveSharedStoreKey(bare, cleanEnv())).toBeNull();
   });
 });
+
+describe('readSharedStorePointer malformed content (#3352 review)', () => {
+  it.each(['null', '42', '"text"', '[]'])(
+    'returns null for a pointer whose JSON is %s',
+    async (body) => {
+      const dir = await makeTempDir('gn-shared-ptr-');
+      await fs.mkdir(path.join(dir, '.gitnexus'));
+      await fs.writeFile(path.join(dir, '.gitnexus', 'store.json'), body);
+      expect(readSharedStorePointer(dir)).toBeNull();
+    },
+  );
+});