From e7975a6fb7d5d775b87902efae6f574c02c61b23 Mon Sep 17 00:00:00 2001 From: Gergo Magyar Date: Thu, 24 Sep 2026 20:21:45 +0000 Subject: [PATCH] fix(storage): address seventh review round on the shared store (#3374) - clean --gc aborts when the registry cannot be read instead of treating every member as orphaned; with no registry file it collects only members whose checkout directory is gone - seeding from a repository-local index re-reads its metadata under the index lock, so the copied graph and the saved metadata match - clean --gc skips a store another collector removed meanwhile, and reports "no shared stores" when stores/ holds only stray files Co-Authored-By: Claude Opus 5.5 (1M context) --- gitnexus/src/cli/clean.ts | 20 ++++++--- gitnexus/src/core/shared-store-analyze.ts | 16 +++++-- .../src/storage/shared-store-lifecycle.ts | 17 +++++--- .../integration/shared-store-clean.test.ts | 43 +++++++++++++++++++ 4 files changed, 80 insertions(+), 16 deletions(-) diff --git a/gitnexus/src/cli/clean.ts b/gitnexus/src/cli/clean.ts index 10c2e8b03..46dd55cff 100644 --- a/gitnexus/src/cli/clean.ts +++ b/gitnexus/src/cli/clean.ts @@ -174,15 +174,23 @@ const collectSharedStores = async (force: boolean): Promise => { if (err.code === 'ENOENT') return [] as string[]; throw err; }); - if (names.length === 0) { + // Stray files (`.DS_Store`) are not stores, and a symlink is not followed: + // reclaim deletes under the root it is given. A store another collector + // removed meanwhile is simply gone. + const roots: string[] = []; + for (const name of names) { + const root = path.join(storesDir, name); + const stat = await fs.lstat(root).catch((err: NodeJS.ErrnoException) => { + if (err.code === 'ENOENT') return null; + throw err; + }); + if (stat?.isDirectory()) roots.push(root); + } + if (roots.length === 0) { console.log(t('clean.gc.none')); return; } - for (const name of names) { - const root = path.join(storesDir, name); - // 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; + for (const root of roots) { // Without --force this is a preview: same selection, nothing deleted. const result = await reclaimSharedStore(root, { gc: true, dryRun: !force }); console.log( diff --git a/gitnexus/src/core/shared-store-analyze.ts b/gitnexus/src/core/shared-store-analyze.ts index a8e3fde4b..1e433abff 100644 --- a/gitnexus/src/core/shared-store-analyze.ts +++ b/gitnexus/src/core/shared-store-analyze.ts @@ -182,18 +182,26 @@ const seedFromLocalIndex = async ( log: Log, ): Promise => { const sourceGraph = path.join(source, LBUG_DIRECTORY); - const meta = await loadMeta(source); - if (!meta || meta.incrementalInProgress || !meta.lastCommit) return false; - if (!(await exists(sourceGraph))) return false; - if (commitDistanceToHead(repoPath, meta.lastCommit) === null) return false; + const usable = (m: RepoMeta | null): m is RepoMeta => + !!m && + !m.incrementalInProgress && + !!m.lastCommit && + commitDistanceToHead(repoPath, m.lastCommit) !== null; + if (!usable(await loadMeta(source)) || !(await exists(sourceGraph))) return false; let lock; try { lock = await acquireIndexLock(source, { timeoutMs: 2_000 }); } catch { return false; // another analyze is writing it; seed from scratch instead } + let meta: RepoMeta; try { if (lock.lockFree || (await inspectLbugSidecars(sourceGraph)).kind !== 'clean') return false; + // Re-read under the lock: an analyze that finished while this waited + // rewrote both, and the copied graph must match the saved metadata. + const locked = await loadMeta(source); + if (!usable(locked)) return false; + meta = locked; await fs.mkdir(slot, { recursive: true }); try { await cloneGraphFile(sourceGraph, path.join(slot, LBUG_DIRECTORY)); diff --git a/gitnexus/src/storage/shared-store-lifecycle.ts b/gitnexus/src/storage/shared-store-lifecycle.ts index e7a05ad0c..29b045dc4 100644 --- a/gitnexus/src/storage/shared-store-lifecycle.ts +++ b/gitnexus/src/storage/shared-store-lifecycle.ts @@ -16,7 +16,7 @@ import { acquireIndexLock, requireExclusiveIndexLock, type IndexLockHandle } fro import { canonicalizePath, findRegistryEntryByRepoPath, - readRegistry, + readRegistryStrictIfPresent, registryPathEquals, } from './repo-manager.js'; import { loadMeta } from './repo-meta.js'; @@ -78,17 +78,22 @@ const listDirStrict = (dir: string): Promise => * gone, or its entry moved elsewhere (`--no-share`, sharing turned off). The * registry is the membership record for opted-in clones and for a main * checkout whose last worktree was removed, so identity alone cannot decide. - * A slot with no attributable `repoPath` is never collected. + * A slot with no attributable `repoPath` is never collected. An unreadable + * registry aborts; without a registry file only slots whose checkout is gone + * are collected. */ const orphanMembers = async (slots: string[]): Promise> => { - const entries = await readRegistry(); + const entries = await readRegistryStrictIfPresent(); const orphans = new Set(); for (const slot of slots) { const meta = await loadMeta(slot); if (!meta?.repoPath) continue; - const entry = existsSync(meta.repoPath) - ? findRegistryEntryByRepoPath(entries, meta.repoPath) - : undefined; + if (!existsSync(meta.repoPath)) { + orphans.add(slot); + continue; + } + if (!entries) continue; + const entry = findRegistryEntryByRepoPath(entries, meta.repoPath); if ( !entry || !registryPathEquals(canonicalizePath(entry.storagePath), canonicalizePath(slot)) diff --git a/gitnexus/test/integration/shared-store-clean.test.ts b/gitnexus/test/integration/shared-store-clean.test.ts index 00cef0c54..aa57509cc 100644 --- a/gitnexus/test/integration/shared-store-clean.test.ts +++ b/gitnexus/test/integration/shared-store-clean.test.ts @@ -170,6 +170,32 @@ describe('shared store clean (#3352)', () => { expect(existsSync(path.join(decoy, 'commits', 'ddddddd-4444444444444444'))).toBe(true); }, 240_000); + it('clean --gc reports no stores when stores/ holds only stray files', async () => { + await fs.mkdir(path.join(tmpHome.dbPath, 'stores'), { recursive: true }); + await fs.writeFile(path.join(tmpHome.dbPath, 'stores', '.DS_Store'), 'x'); + const logs = await cleanIn(main, { gc: true }); + expect(logs).toContain('No shared stores to collect.'); + }); + + it('clean --gc skips a store removed by a concurrent collector', async () => { + const gone = path.join(tmpHome.dbPath, 'stores', 'repo-0123456789ab'); + await fs.mkdir(gone, { recursive: true }); + const realLstat = fs.lstat; + const lstat = vi.spyOn(fs, 'lstat').mockImplementation((async ( + target: string, + ...rest: unknown[] + ) => { + if (String(target) === gone) throw Object.assign(new Error('gone'), { code: 'ENOENT' }); + return (realLstat as (...a: unknown[]) => Promise)(target, ...rest); + }) as typeof fs.lstat); + try { + const logs = await cleanIn(main, { gc: true, force: true }); + expect(logs).toContain('No shared stores to collect.'); + } finally { + lstat.mockRestore(); + } + }); + 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'); @@ -340,6 +366,23 @@ describe('reclaimSharedStore', () => { expect(existsSync(path.join(dir, 'store.json'))).toBe(false); }); + it('aborts instead of collecting members when the registry cannot be read', async () => { + const slot = await member('wt-000000000000', { repoPath: tmpHome.dbPath }); + await fs.writeFile(path.join(tmpHome.dbPath, 'registry.json'), '{not json'); + await expect(reclaimSharedStore(layout().root, { gc: true })).rejects.toThrow(); + expect(existsSync(slot)).toBe(true); + }); + + it('without a registry file collects only members whose checkout is gone', async () => { + const live = await member('wt-000000000000', { repoPath: tmpHome.dbPath }); + const dead = await member('wt-111111111111', { + repoPath: path.join(tmpHome.dbPath, 'deleted-worktree'), + }); + await reclaimSharedStore(layout().root, { gc: true }); + expect(existsSync(live)).toBe(true); + expect(existsSync(dead)).toBe(false); + }); + it('reports a graph it cannot delete instead of failing', async () => { const orphan = await commitGraph('ccccccc-3333333333333333'); await member('wt-000000000000', {});