From 2cdee6d4a52e138bb03a4a4eebb5c32daf125390 Mon Sep 17 00:00:00 2001 From: Gergo Magyar Date: Thu, 24 Sep 2026 19:07:05 +0000 Subject: [PATCH] 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) --- .../hooks/registry-query.cjs | 35 ++++++++++++- gitnexus/src/cli/clean.ts | 15 ++++-- gitnexus/src/cli/remove.ts | 7 ++- gitnexus/src/core/run-analyze.ts | 4 +- gitnexus/src/core/shared-store-analyze.ts | 22 +++++++-- gitnexus/src/server/analyze-launch.ts | 11 ++++- gitnexus/src/server/api.ts | 9 +++- .../src/storage/shared-store-lifecycle.ts | 38 ++++++++++++-- gitnexus/src/storage/shared-store.ts | 1 + .../integration/shared-store-clean.test.ts | 49 ++++++++++++++++--- gitnexus/test/unit/hooks-shared-store.test.ts | 18 +++++++ .../test/unit/storage/shared-store.test.ts | 15 +++++- 12 files changed, 198 insertions(+), 26 deletions(-) diff --git a/gitnexus-factory-plugin/hooks/registry-query.cjs b/gitnexus-factory-plugin/hooks/registry-query.cjs index 5a126d67f..0607602db 100644 --- a/gitnexus-factory-plugin/hooks/registry-query.cjs +++ b/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-` 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 /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 `-` 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, }; } diff --git a/gitnexus/src/cli/clean.ts b/gitnexus/src/cli/clean.ts index 48649d93c..ed8429490 100644 --- a/gitnexus/src/cli/clean.ts +++ b/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 => { } 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); diff --git a/gitnexus/src/cli/remove.ts b/gitnexus/src/cli/remove.ts index d21ae9491..4f3ce8c94 100644 --- a/gitnexus/src/cli/remove.ts +++ b/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}`); diff --git a/gitnexus/src/core/run-analyze.ts b/gitnexus/src/core/run-analyze.ts index b8c4e2014..f72476d85 100644 --- a/gitnexus/src/core/run-analyze.ts +++ b/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. diff --git a/gitnexus/src/core/shared-store-analyze.ts b/gitnexus/src/core/shared-store-analyze.ts index a27c5f298..acd9cd29a 100644 --- a/gitnexus/src/core/shared-store-analyze.ts +++ b/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}).`); } }); diff --git a/gitnexus/src/server/analyze-launch.ts b/gitnexus/src/server/analyze-launch.ts index ffa46be8d..04023e985 100644 --- a/gitnexus/src/server/analyze-launch.ts +++ b/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, diff --git a/gitnexus/src/server/api.ts b/gitnexus/src/server/api.ts index 8c5faf71b..07eeb3057 100644 --- a/gitnexus/src/server/api.ts +++ b/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/. diff --git a/gitnexus/src/storage/shared-store-lifecycle.ts b/gitnexus/src/storage/shared-store-lifecycle.ts index 5172aff27..53f01b98c 100644 --- a/gitnexus/src/storage/shared-store-lifecycle.ts +++ b/gitnexus/src/storage/shared-store-lifecycle.ts @@ -62,6 +62,17 @@ export interface ReclaimResult { const listDir = (dir: string): Promise => 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 => + 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(); - 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 => - (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 ( + storagePath: string, + fn: () => Promise, +): Promise => { + 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(); + } +}; diff --git a/gitnexus/src/storage/shared-store.ts b/gitnexus/src/storage/shared-store.ts index 9fd30cd86..2d7893824 100644 --- a/gitnexus/src/storage/shared-store.ts +++ b/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; diff --git a/gitnexus/test/integration/shared-store-clean.test.ts b/gitnexus/test/integration/shared-store-clean.test.ts index a9cc5b32a..f7ef0b705 100644 --- a/gitnexus/test/integration/shared-store-clean.test.ts +++ b/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)(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 () => { diff --git a/gitnexus/test/unit/hooks-shared-store.test.ts b/gitnexus/test/unit/hooks-shared-store.test.ts index 609c19efe..186c72d9a 100644 --- a/gitnexus/test/unit/hooks-shared-store.test.ts +++ b/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) => { 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 }); }); diff --git a/gitnexus/test/unit/storage/shared-store.test.ts b/gitnexus/test/unit/storage/shared-store.test.ts index 939dad3a1..0ca91f9d6 100644 --- a/gitnexus/test/unit/storage/shared-store.test.ts +++ b/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(); + }, + ); +});