From d746675c2881c7304cd60f13ed5a71eb5acc06e3 Mon Sep 17 00:00:00 2001 From: Gergo Magyar Date: Thu, 24 Sep 2026 17:38:31 +0000 Subject: [PATCH] fix(storage): address second review round on the shared store (#3374) - Reject --no-share in a linked worktree before taking any lock or indexing anything. - clean --all and remove also delete each shared checkout's pointer. - Delete an empty store only while also holding its cache lock, and re-check emptiness under it. - A store pointer is trusted only when the slot's own metadata names this checkout; the editable pointer file just says where to look. - Device names with an extension are prefixed on Windows only, so POSIX slot names stay stable. - Test fixtures use the gitnexus-test- prefix the stale-sidecar sweep recognizes; help text and the private-graph label are accurate. Co-Authored-By: Claude Opus 5.5 (1M context) --- gitnexus/src/cli/clean.ts | 4 ++- gitnexus/src/cli/i18n/zh-CN.ts | 2 +- gitnexus/src/cli/index.ts | 2 +- gitnexus/src/cli/remove.ts | 7 +++-- gitnexus/src/core/run-analyze.ts | 7 +++++ .../src/storage/shared-store-lifecycle.ts | 27 ++++++++++++------- gitnexus/src/storage/shared-store.ts | 26 ++++++++++++++---- gitnexus/src/storage/storage-slot.ts | 18 +++++++++---- .../integration/shared-store-adoption.test.ts | 21 +++++++++++++-- .../integration/shared-store-analyze.test.ts | 8 +++--- .../integration/shared-store-cache.test.ts | 6 ++--- .../integration/shared-store-clean.test.ts | 17 +++++++++--- .../shared-store-clone-optin.test.ts | 10 +++++-- .../integration/shared-store-seed.test.ts | 6 ++--- .../test/unit/storage/shared-store.test.ts | 13 ++++++--- 15 files changed, 128 insertions(+), 46 deletions(-) diff --git a/gitnexus/src/cli/clean.ts b/gitnexus/src/cli/clean.ts index e9cc747ae..98b011a34 100644 --- a/gitnexus/src/cli/clean.ts +++ b/gitnexus/src/cli/clean.ts @@ -372,7 +372,9 @@ export const cleanCommand = async (options?: { await fs.rm(storagePath, { recursive: true, force: true }); await unregisterRepo(entry.path); console.log(t('clean.deletedRepo', { name: entry.name, storagePath })); - reportReclaim(await reclaimAfterSlotRemoval(storagePath)); + const reclaim = await reclaimAfterSlotRemoval(storagePath); + if (reclaim) await removeSharedStorePointer(entry.path); + reportReclaim(reclaim); } catch (err) { if (err instanceof StorageDeletionError) { logger.error(`Refusing to clean ${entry.name}: ${err.message}`); diff --git a/gitnexus/src/cli/i18n/zh-CN.ts b/gitnexus/src/cli/i18n/zh-CN.ts index 60d1c654a..4a96cdd9f 100644 --- a/gitnexus/src/cli/i18n/zh-CN.ts +++ b/gitnexus/src/cli/i18n/zh-CN.ts @@ -30,7 +30,7 @@ export const zhCN = { 'list.processes': '流程', 'list.unknown': 'unknown', 'status.sharedStoreShared': '共享索引：存储 {{key}}，提交 {{commit}} 的共享图', - 'status.sharedStorePrivate': '共享索引：存储 {{key}}，私有图（有本地更改）', + 'status.sharedStorePrivate': '共享索引：存储 {{key}}，私有图（有本地更改或固定分支索引）', 'status.legacyLocalIndex': '残留的本地索引：{{path}}（{{size}}）；使用 `gitnexus clean --local-index --force` 删除', 'status.notGitRepo': '当前目录不是 git 仓库。', diff --git a/gitnexus/src/cli/index.ts b/gitnexus/src/cli/index.ts index 21cbb016a..f1b42f9ec 100644 --- a/gitnexus/src/cli/index.ts +++ b/gitnexus/src/cli/index.ts @@ -358,7 +358,7 @@ program .option('--stale', 'Reclaim leftover branch indexes that are not a live local head') .option( '--gc', - 'Drop shared-store checkouts whose worktree is gone and delete commit graphs nothing references', + 'Drop shared-store checkouts no registry entry uses and delete commit graphs nothing references', ) .option( '--local-index', diff --git a/gitnexus/src/cli/remove.ts b/gitnexus/src/cli/remove.ts index 697200a62..d21ae9491 100644 --- a/gitnexus/src/cli/remove.ts +++ b/gitnexus/src/cli/remove.ts @@ -29,7 +29,10 @@ * here there is no pipeline, so no conflation.) */ -import { reclaimAfterSlotRemoval } from '../storage/shared-store-lifecycle.js'; +import { + reclaimAfterSlotRemoval, + removeSharedStorePointer, +} from '../storage/shared-store-lifecycle.js'; import fs from 'fs/promises'; import { logger } from '../core/logger.js'; import { cliError } from './cli-message.js'; @@ -101,7 +104,7 @@ export const removeCommand = async (target: string, options?: { force?: boolean try { await fs.rm(storagePath, { recursive: true, force: true }); await unregisterRepo(entry.path); - await reclaimAfterSlotRemoval(storagePath); + if (await reclaimAfterSlotRemoval(storagePath)) await removeSharedStorePointer(entry.path); console.log(t('remove.removed', { name: entry.name })); console.log(` ${t('common.path')}: ${entry.path}`); console.log(` ${t('common.storage')}: ${entry.storagePath}`); diff --git a/gitnexus/src/core/run-analyze.ts b/gitnexus/src/core/run-analyze.ts index 9bd1b1f25..b8c4e2014 100644 --- a/gitnexus/src/core/run-analyze.ts +++ b/gitnexus/src/core/run-analyze.ts @@ -1137,6 +1137,13 @@ async function resolveWriteTarget(repoPath: string, options: AnalyzeOptions): Pr const storageRequirements = options.force ? ANALYZE_FORCE_STORAGE_REQUIREMENTS : ANALYZE_STORAGE_REQUIREMENTS; + if (options.noShare && resolveSharedStore(repoPath)) { + // Fail before any lock or indexing; only an opted-in clone can leave. + throw new Error( + '--no-share: linked worktrees always use the shared index store. ' + + 'Set GITNEXUS_SHARED_STORE=off to index every checkout into its own .gitnexus.', + ); + } const sharingOff = options.noShare || isSharedStoreDisabled(); const sharedStore = sharingOff ? undefined diff --git a/gitnexus/src/storage/shared-store-lifecycle.ts b/gitnexus/src/storage/shared-store-lifecycle.ts index 9102cd82d..6913f1a24 100644 --- a/gitnexus/src/storage/shared-store-lifecycle.ts +++ b/gitnexus/src/storage/shared-store-lifecycle.ts @@ -132,16 +132,23 @@ export const reclaimSharedStoreLocked = async ( } } - const remaining = (await listDir(checkoutsDir)).length + (await listDir(commitsDir)).length; - if (remaining === 0 && !opts.dryRun) { - // The lock directory lives inside the store; removing it while held is - // safe on POSIX and is retried on the next reclaim elsewhere. - await fs - .rm(storeRoot, { recursive: true, force: true }) - .then(() => { - result.storeRemoved = true; - }) - .catch(() => {}); + const isEmpty = async (): Promise => + (await listDir(checkoutsDir)).length + (await listDir(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 + // slot may have appeared while waiting. The lock directories live inside + // the store; removing them while held is safe on POSIX and is retried on + // the next reclaim elsewhere. + await withStoreLock({ root: storeRoot }, 'cache', async () => { + if (!(await isEmpty())) return; + await fs + .rm(storeRoot, { recursive: true, force: true }) + .then(() => { + result.storeRemoved = true; + }) + .catch(() => {}); + }); } return result; }; diff --git a/gitnexus/src/storage/shared-store.ts b/gitnexus/src/storage/shared-store.ts index d02276c39..613aadbc5 100644 --- a/gitnexus/src/storage/shared-store.ts +++ b/gitnexus/src/storage/shared-store.ts @@ -282,15 +282,31 @@ export const readSharedStorePointer = (checkoutPath: string): string | null => { const pointerPath = path.resolve(root, GITNEXUS_DIR, SHARED_STORE_POINTER); const pointerRel = path.relative(root, pointerPath); if (pointerRel.startsWith('..') || path.isAbsolute(pointerRel)) return null; - let recorded: unknown; + let pointer: { checkoutSlot?: unknown; storeKey?: unknown }; try { - recorded = (JSON.parse(fs.readFileSync(pointerPath, 'utf-8')) as { checkoutSlot?: unknown }) - .checkoutSlot; + pointer = JSON.parse(fs.readFileSync(pointerPath, 'utf-8')) as typeof pointer; } catch { return null; } + const { checkoutSlot: recorded, storeKey } = pointer; if (typeof recorded !== 'string' || !path.isAbsolute(recorded)) return null; + if (typeof storeKey !== 'string') return null; const slot = path.resolve(recorded); - if (!storeRootOfCheckoutSlot(slot)) return null; - return path.basename(slot) === slotName(checkoutPath) ? slot : null; + const storeRoot = storeRootOfCheckoutSlot(slot); + if (!storeRoot || path.basename(storeRoot) !== storeKey) return null; + if (slot !== sharedStoreLayout(storeKey, checkoutPath).checkoutSlot) return null; + // The pointer file is editable, so its fields only say where to look. The + // binding is the slot's own metadata, written by analyze for this checkout: + // a slot in another store for this path exists only if this checkout was + // really a member there. + const metaPath = path.resolve(slot, INDEX_METADATA_FILE); + const metaRel = path.relative(slot, metaPath); + if (metaRel.startsWith('..') || path.isAbsolute(metaRel)) return null; + let owner: unknown; + try { + owner = (JSON.parse(fs.readFileSync(metaPath, 'utf-8')) as { repoPath?: unknown }).repoPath; + } catch { + return null; + } + return typeof owner === 'string' && slotName(owner) === slotName(checkoutPath) ? slot : null; }; diff --git a/gitnexus/src/storage/storage-slot.ts b/gitnexus/src/storage/storage-slot.ts index 0fbfe8318..5bc8640ce 100644 --- a/gitnexus/src/storage/storage-slot.ts +++ b/gitnexus/src/storage/storage-slot.ts @@ -14,7 +14,11 @@ export const STORAGE_ROOT_ENV = 'GITNEXUS_STORAGE_ROOT'; const STORAGE_SLOT_HASH_LENGTH = 12; -const sanitizeSlotBasename = (value: string): string => { +/** Exported for tests; production callers use {@link slotNameForCanonicalPath}. */ +export const sanitizeSlotBasename = ( + value: string, + platform: NodeJS.Platform = process.platform, +): string => { // Linear: a quantified `/[. ]+$/` on attacker-controlled basenames is // js/polynomial-redos (CodeQL #1056). Cap first, then walk the tail once. const sanitized = value.replace(/[\u0000-\u001f<>:"/\\|?*]/g, '-').slice(0, 80); @@ -25,10 +29,14 @@ const sanitizeSlotBasename = (value: string): string => { end--; } const candidate = sanitized.slice(0, end) || 'repository'; - // Windows reserves device names with any extension too (`CON.txt`). - return /^(con|prn|aux|nul|com[1-9]|lpt[1-9])(\..*)?$/i.test(candidate) - ? `repository-${candidate}` - : candidate; + // Exact device names were always prefixed. Windows also reserves them with + // an extension (`CON.txt`); apply that only there, so existing POSIX slot + // names stay stable. + const reserved = + platform === 'win32' + ? /^(con|prn|aux|nul|com[1-9]|lpt[1-9])(\..*)?$/i + : /^(con|prn|aux|nul|com[1-9]|lpt[1-9])$/i; + return reserved.test(candidate) ? `repository-${candidate}` : candidate; }; /** diff --git a/gitnexus/test/integration/shared-store-adoption.test.ts b/gitnexus/test/integration/shared-store-adoption.test.ts index 31e0532ef..c1390eeb5 100644 --- a/gitnexus/test/integration/shared-store-adoption.test.ts +++ b/gitnexus/test/integration/shared-store-adoption.test.ts @@ -71,8 +71,8 @@ describe('shared store adoption and reporting (#3352)', () => { beforeEach(async () => { savedCwd = process.cwd(); - tmpHome = await createTempDir('gitnexus-adopt-home-'); - tmpRepo = await createTempDir('gitnexus-adopt-repo-'); + tmpHome = await createTempDir('gitnexus-test-adopt-home-'); + tmpRepo = await createTempDir('gitnexus-test-adopt-repo-'); savedHome = process.env.GITNEXUS_HOME; savedSwitch = process.env[SHARED_STORE_ENV]; delete process.env[SHARED_STORE_ENV]; @@ -238,6 +238,23 @@ describe('shared store adoption and reporting (#3352)', () => { const other = layoutOf(main).checkoutSlot; await fs.writeFile(pointer, JSON.stringify({ version: 1, checkoutSlot: other })); expect(readSharedStorePointer(wt)).toBeNull(); + const ownSlot = layoutOf(wt).checkoutSlot; + const otherStoreSlot = path.join( + path.dirname(path.dirname(path.dirname(ownSlot))), + 'other-000000000000', + 'checkouts', + path.basename(ownSlot), + ); + await fs.writeFile( + pointer, + JSON.stringify({ version: 1, storeKey: 'other-000000000000', checkoutSlot: otherStoreSlot }), + ); + expect(readSharedStorePointer(wt)).toBeNull(); + await fs.writeFile( + pointer, + JSON.stringify({ version: 1, storeKey: layoutOf(wt).key, checkoutSlot: otherStoreSlot }), + ); + expect(readSharedStorePointer(wt)).toBeNull(); await fs.writeFile(pointer, JSON.stringify({ version: 1, checkoutSlot: '/etc' })); expect(readSharedStorePointer(wt)).toBeNull(); await fs.writeFile(pointer, 'not json'); diff --git a/gitnexus/test/integration/shared-store-analyze.test.ts b/gitnexus/test/integration/shared-store-analyze.test.ts index 63aa23a8e..73867212e 100644 --- a/gitnexus/test/integration/shared-store-analyze.test.ts +++ b/gitnexus/test/integration/shared-store-analyze.test.ts @@ -61,8 +61,8 @@ describe('shared sibling store analyze (#3352)', () => { let wtB: string; beforeEach(async () => { - tmpHome = await createTempDir('gitnexus-shared-home-'); - tmpRepo = await createTempDir('gitnexus-shared-repo-'); + tmpHome = await createTempDir('gitnexus-test-shared-home-'); + tmpRepo = await createTempDir('gitnexus-test-shared-repo-'); savedHome = process.env.GITNEXUS_HOME; process.env.GITNEXUS_HOME = tmpHome.dbPath; @@ -259,8 +259,8 @@ describe('publishSharedGraph race (#3352)', () => { let savedHome: string | undefined; beforeEach(async () => { - tmpHome = await createTempDir('gitnexus-shared-race-home-'); - tmpRepo = await createTempDir('gitnexus-shared-race-repo-'); + tmpHome = await createTempDir('gitnexus-test-shared-race-home-'); + tmpRepo = await createTempDir('gitnexus-test-shared-race-repo-'); savedHome = process.env.GITNEXUS_HOME; process.env.GITNEXUS_HOME = tmpHome.dbPath; }); diff --git a/gitnexus/test/integration/shared-store-cache.test.ts b/gitnexus/test/integration/shared-store-cache.test.ts index 3311f3e9f..6415eb383 100644 --- a/gitnexus/test/integration/shared-store-cache.test.ts +++ b/gitnexus/test/integration/shared-store-cache.test.ts @@ -54,8 +54,8 @@ describe('shared store caches (#3352)', () => { let wtB: string; beforeEach(async () => { - tmpHome = await createTempDir('gitnexus-cache-home-'); - tmpRepo = await createTempDir('gitnexus-cache-repo-'); + tmpHome = await createTempDir('gitnexus-test-cache-home-'); + tmpRepo = await createTempDir('gitnexus-test-cache-repo-'); savedHome = process.env.GITNEXUS_HOME; process.env.GITNEXUS_HOME = tmpHome.dbPath; const root = await fs.realpath(tmpRepo.dbPath); @@ -119,7 +119,7 @@ describe('withStoreLock', () => { let savedHome: string | undefined; beforeEach(async () => { - tmpHome = await createTempDir('gitnexus-store-lock-home-'); + tmpHome = await createTempDir('gitnexus-test-store-lock-home-'); savedHome = process.env.GITNEXUS_HOME; process.env.GITNEXUS_HOME = tmpHome.dbPath; }); diff --git a/gitnexus/test/integration/shared-store-clean.test.ts b/gitnexus/test/integration/shared-store-clean.test.ts index cd68a01be..8c2e9ccc9 100644 --- a/gitnexus/test/integration/shared-store-clean.test.ts +++ b/gitnexus/test/integration/shared-store-clean.test.ts @@ -76,8 +76,8 @@ describe('shared store clean (#3352)', () => { beforeEach(async () => { savedCwd = process.cwd(); - tmpHome = await createTempDir('gitnexus-clean-home-'); - tmpRepo = await createTempDir('gitnexus-clean-repo-'); + tmpHome = await createTempDir('gitnexus-test-clean-home-'); + tmpRepo = await createTempDir('gitnexus-test-clean-repo-'); savedHome = process.env.GITNEXUS_HOME; process.env.GITNEXUS_HOME = tmpHome.dbPath; const root = await fs.realpath(tmpRepo.dbPath); @@ -125,6 +125,17 @@ describe('shared store clean (#3352)', () => { expect(logs.join('\n')).toMatch(/removed 1 commit graph/); }, 240_000); + it('clean --all --force removes each shared checkout pointer with its slot', async () => { + await analyze(wtA); + await analyze(wtB); + expect(existsSync(path.join(wtA, '.gitnexus', 'store.json'))).toBe(true); + await cleanIn(wtA, { all: true, force: true }); + for (const wt of [wtA, wtB]) { + expect(existsSync(path.join(wt, '.gitnexus', 'store.json'))).toBe(false); + } + expect(existsSync(layoutOf(wtA).root)).toBe(false); + }, 240_000); + it('previews without --force and deletes nothing', async () => { await analyze(wtA); const layout = layoutOf(wtA); @@ -172,7 +183,7 @@ describe('reclaimSharedStore', () => { let savedHome: string | undefined; beforeEach(async () => { - tmpHome = await createTempDir('gitnexus-reclaim-home-'); + tmpHome = await createTempDir('gitnexus-test-reclaim-home-'); savedHome = process.env.GITNEXUS_HOME; process.env.GITNEXUS_HOME = tmpHome.dbPath; }); diff --git a/gitnexus/test/integration/shared-store-clone-optin.test.ts b/gitnexus/test/integration/shared-store-clone-optin.test.ts index c82755f86..07ac6ba78 100644 --- a/gitnexus/test/integration/shared-store-clone-optin.test.ts +++ b/gitnexus/test/integration/shared-store-clone-optin.test.ts @@ -51,8 +51,8 @@ describe('shared store clone opt-in (#3352)', () => { (await listRegisteredRepos()).find((e) => e.path === checkout)?.storagePath; beforeEach(async () => { - tmpHome = await createTempDir('gitnexus-optin-home-'); - tmpRepo = await createTempDir('gitnexus-optin-repo-'); + tmpHome = await createTempDir('gitnexus-test-optin-home-'); + tmpRepo = await createTempDir('gitnexus-test-optin-repo-'); savedHome = process.env.GITNEXUS_HOME; process.env.GITNEXUS_HOME = tmpHome.dbPath; root = await fs.realpath(tmpRepo.dbPath); @@ -169,6 +169,12 @@ describe('shared store clone opt-in (#3352)', () => { }, 240_000); it('rejects --no-share in a linked worktree', async () => { + const before = await fs.readFile(path.join(storeLayout.checkoutSlot, 'gitnexus.json'), 'utf-8'); await expect(analyze(wt, { noShare: true })).rejects.toThrow(/GITNEXUS_SHARED_STORE=off/); + // Rejected before any work: no local index, slot metadata untouched. + expect(existsSync(path.join(wt, '.gitnexus', 'lbug'))).toBe(false); + expect(await fs.readFile(path.join(storeLayout.checkoutSlot, 'gitnexus.json'), 'utf-8')).toBe( + before, + ); }, 240_000); }); diff --git a/gitnexus/test/integration/shared-store-seed.test.ts b/gitnexus/test/integration/shared-store-seed.test.ts index f543668cf..2b50704b1 100644 --- a/gitnexus/test/integration/shared-store-seed.test.ts +++ b/gitnexus/test/integration/shared-store-seed.test.ts @@ -73,8 +73,8 @@ describe('shared store seeding (#3352)', () => { }; beforeEach(async () => { - tmpHome = await createTempDir('gitnexus-seed-home-'); - tmpRepo = await createTempDir('gitnexus-seed-repo-'); + tmpHome = await createTempDir('gitnexus-test-seed-home-'); + tmpRepo = await createTempDir('gitnexus-test-seed-repo-'); savedHome = process.env.GITNEXUS_HOME; process.env.GITNEXUS_HOME = tmpHome.dbPath; root = await fs.realpath(tmpRepo.dbPath); @@ -226,7 +226,7 @@ describe('ensurePrivateSharedGraph', () => { let savedHome: string | undefined; beforeEach(async () => { - tmpHome = await createTempDir('gitnexus-private-home-'); + tmpHome = await createTempDir('gitnexus-test-private-home-'); savedHome = process.env.GITNEXUS_HOME; process.env.GITNEXUS_HOME = tmpHome.dbPath; }); diff --git a/gitnexus/test/unit/storage/shared-store.test.ts b/gitnexus/test/unit/storage/shared-store.test.ts index 737db7f0e..9398eff58 100644 --- a/gitnexus/test/unit/storage/shared-store.test.ts +++ b/gitnexus/test/unit/storage/shared-store.test.ts @@ -13,6 +13,7 @@ import { sharedStoreLayout, type SharedStoreLayout, } from '../../../src/storage/shared-store.js'; +import { sanitizeSlotBasename } from '../../../src/storage/storage-slot.js'; import { STORAGE_PATH_ENV, STORAGE_ROOT_ENV, @@ -317,14 +318,18 @@ describe('resolveStoragePath store tier', () => { describe('slot naming edge cases (#3352 review)', () => { it.each(['CON.txt', 'com1.log', 'Lpt9.tar.gz'])( - 'prefixes a reserved Windows device name with an extension: %s', + 'prefixes a reserved device name with an extension on Windows only: %s', (base) => { - expect(storageSlotName(path.join(path.sep, 'tmp', base))).toMatch( - new RegExp(`^repository-${base.replace('.', '\\.')}-[0-9a-f]{12}$`), - ); + expect(sanitizeSlotBasename(base, 'win32')).toBe(`repository-${base}`); + expect(sanitizeSlotBasename(base, 'linux')).toBe(base); }, ); + it.each(['CON', 'nul', 'COM1'])('prefixes an exact device name on every platform: %s', (base) => { + expect(sanitizeSlotBasename(base, 'win32')).toBe(`repository-${base}`); + expect(sanitizeSlotBasename(base, 'linux')).toBe(`repository-${base}`); + }); + it('keeps an ordinary name that only starts like a device name', () => { expect(storageSlotName(path.join(path.sep, 'tmp', 'console'))).toMatch(/^console-/); });