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

- clean --gc never collects a slot whose index lock is held: an analyze
  holds it until it registers the checkout, so a seeded but not yet
  registered slot is busy, not orphaned.
- Reclaim compares absolute paths, so a relative GITNEXUS_HOME does not
  make live commit graphs look unreferenced.
- graphPath is followed only into a published <commit>-<featureKey>
  dir, never .publish-* staging (TS and all three hook copies).
- clean --gc fails on an unreadable stores root instead of reporting
  nothing to collect.
- --no-share help states it is for opted-in clones.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Gergo Magyar 2026-09-24 18:11:25 +00:00
parent d746675c28
commit e2a9467171
10 changed files with 85 additions and 12 deletions

View file

@ -321,9 +321,11 @@ function resolveGraphPath(storagePath, metadata) {
const recorded = metadata && metadata.graphPath; const recorded = metadata && metadata.graphPath;
if (typeof recorded !== 'string' || !path.isAbsolute(recorded)) return own; if (typeof recorded !== 'string' || !path.isAbsolute(recorded)) return own;
const graph = path.resolve(recorded); const graph = path.resolve(recorded);
// Only a published `<commit>-<featureKey>` dir, never `.publish-*` staging.
const valid = const valid =
path.basename(graph) === LBUG_DIRECTORY && path.basename(graph) === LBUG_DIRECTORY &&
isDirectChild(path.join(root, 'commits'), path.dirname(graph)); 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; return valid ? graph : own;
} }

View file

@ -321,9 +321,11 @@ function resolveGraphPath(storagePath, metadata) {
const recorded = metadata && metadata.graphPath; const recorded = metadata && metadata.graphPath;
if (typeof recorded !== 'string' || !path.isAbsolute(recorded)) return own; if (typeof recorded !== 'string' || !path.isAbsolute(recorded)) return own;
const graph = path.resolve(recorded); const graph = path.resolve(recorded);
// Only a published `<commit>-<featureKey>` dir, never `.publish-*` staging.
const valid = const valid =
path.basename(graph) === LBUG_DIRECTORY && path.basename(graph) === LBUG_DIRECTORY &&
isDirectChild(path.join(root, 'commits'), path.dirname(graph)); 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; return valid ? graph : own;
} }

View file

@ -321,9 +321,11 @@ function resolveGraphPath(storagePath, metadata) {
const recorded = metadata && metadata.graphPath; const recorded = metadata && metadata.graphPath;
if (typeof recorded !== 'string' || !path.isAbsolute(recorded)) return own; if (typeof recorded !== 'string' || !path.isAbsolute(recorded)) return own;
const graph = path.resolve(recorded); const graph = path.resolve(recorded);
// Only a published `<commit>-<featureKey>` dir, never `.publish-*` staging.
const valid = const valid =
path.basename(graph) === LBUG_DIRECTORY && path.basename(graph) === LBUG_DIRECTORY &&
isDirectChild(path.join(root, 'commits'), path.dirname(graph)); 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; return valid ? graph : own;
} }

View file

@ -168,7 +168,12 @@ const reportReclaim = (result: ReclaimResult | null): void => {
/** `clean --gc`: collect every shared store under GITNEXUS_HOME (#3352). */ /** `clean --gc`: collect every shared store under GITNEXUS_HOME (#3352). */
const collectSharedStores = async (force: boolean): Promise<void> => { const collectSharedStores = async (force: boolean): Promise<void> => {
const storesDir = path.join(getGlobalDir(), STORES_DIR); const storesDir = path.join(getGlobalDir(), STORES_DIR);
const names = await fs.readdir(storesDir).catch(() => [] as string[]); // Only a missing stores root means "nothing to collect"; an unreadable one
// must fail loudly rather than report success.
const names = await fs.readdir(storesDir).catch((err: NodeJS.ErrnoException) => {
if (err.code === 'ENOENT') return [] as string[];
throw err;
});
if (names.length === 0) { if (names.length === 0) {
console.log(t('clean.gc.none')); console.log(t('clean.gc.none'));
return; return;

View file

@ -267,7 +267,8 @@ export const zhCN = {
'即使已有其他路径使用相同 --name 别名,也注册该仓库。会使两个路径的 `-r <name>` 产生歧义;请用 -r <path> 消除歧义。', '即使已有其他路径使用相同 --name 别名,也注册该仓库。会使两个路径的 `-r <name>` 产生歧义;请用 -r <path> 消除歧义。',
'help.option.analyze.shareWith': 'help.option.analyze.shareWith':
'加入同一仓库已注册工作树的共享索引存储(名称或路径);远程 URL 必须一致。之后的运行会记住此选择。', '加入同一仓库已注册工作树的共享索引存储(名称或路径);远程 URL 必须一致。之后的运行会记住此选择。',
'help.option.analyze.noShare': '离开共享索引存储,重新索引到 <repo>/.gitnexus', 'help.option.analyze.noShare':
'仅限已加入的克隆:离开共享索引存储,重新索引到 <repo>/.gitnexus(链接工作树始终共享;请改用 GITNEXUS_SHARED_STORE=off)',
'help.option.verbose': '启用详细输出', 'help.option.verbose': '启用详细输出',
'help.option.analyze.maxFileSize': 'help.option.analyze.maxFileSize':
'跳过大于该值的文件(KB)。默认:512。硬上限:32768(tree-sitter 限制)。', '跳过大于该值的文件(KB)。默认:512。硬上限:32768(tree-sitter 限制)。',

View file

@ -150,7 +150,11 @@ program
'Join the shared index store of a registered worktree of the same repository ' + 'Join the shared index store of a registered worktree of the same repository ' +
'(name or path); the remote URL must match. Remembered for later runs.', '(name or path); the remote URL must match. Remembered for later runs.',
) )
.option('--no-share', 'Leave the shared index store and index into <repo>/.gitnexus again') .option(
'--no-share',
'Opted-in clones only: leave the shared index store and index into <repo>/.gitnexus again ' +
'(linked worktrees always share; set GITNEXUS_SHARED_STORE=off instead)',
)
.option('-v, --verbose', 'Enable verbose ingestion warnings (default: false)') .option('-v, --verbose', 'Enable verbose ingestion warnings (default: false)')
.option( .option(
'--max-file-size <kb>', '--max-file-size <kb>',

View file

@ -12,7 +12,7 @@
import { existsSync } from 'fs'; import { existsSync } from 'fs';
import fs from 'fs/promises'; import fs from 'fs/promises';
import path from 'path'; import path from 'path';
import { acquireIndexLock, requireExclusiveIndexLock } from './index-lock.js'; import { acquireIndexLock, requireExclusiveIndexLock, type IndexLockHandle } from './index-lock.js';
import { import {
canonicalizePath, canonicalizePath,
findRegistryEntryByRepoPath, findRegistryEntryByRepoPath,
@ -94,9 +94,12 @@ const orphanMembers = async (slots: string[]): Promise<Set<string>> => {
* (KTD7), and slot pointers written under the same lock are always counted. * (KTD7), and slot pointers written under the same lock are always counted.
*/ */
export const reclaimSharedStoreLocked = async ( export const reclaimSharedStoreLocked = async (
storeRoot: string, storeRootInput: string,
opts: { gc?: boolean; dryRun?: boolean } = {}, opts: { gc?: boolean; dryRun?: boolean } = {},
): Promise<ReclaimResult> => { ): Promise<ReclaimResult> => {
// Absolute, so commit dirs compare equal to the resolved graphPath parents
// even when GITNEXUS_HOME is relative.
const storeRoot = path.resolve(storeRootInput);
const result: ReclaimResult = { removed: [], kept: [], droppedMembers: [], storeRemoved: false }; const result: ReclaimResult = { removed: [], kept: [], droppedMembers: [], storeRemoved: false };
const checkoutsDir = path.join(storeRoot, 'checkouts'); const checkoutsDir = path.join(storeRoot, 'checkouts');
const commitsDir = path.join(storeRoot, 'commits'); const commitsDir = path.join(storeRoot, 'commits');
@ -104,9 +107,31 @@ export const reclaimSharedStoreLocked = async (
let slots = (await listDir(checkoutsDir)).map((name) => path.join(checkoutsDir, name)); let slots = (await listDir(checkoutsDir)).map((name) => path.join(checkoutsDir, name));
if (opts.gc) { if (opts.gc) {
const orphans = await orphanMembers(slots); const orphans = await orphanMembers(slots);
for (const slot of orphans) { for (const slot of [...orphans]) {
if (!opts.dryRun) await fs.rm(slot, { recursive: true, force: true }); if (opts.dryRun) {
result.droppedMembers.push(slot); 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.
let lock: IndexLockHandle;
try {
lock = await acquireIndexLock(slot, { timeoutMs: 1 });
} catch {
orphans.delete(slot);
continue;
}
try {
if (lock.lockFree || !(await orphanMembers([slot])).has(slot)) {
orphans.delete(slot);
continue;
}
await fs.rm(slot, { recursive: true, force: true });
result.droppedMembers.push(slot);
} finally {
lock.release();
}
} }
slots = slots.filter((slot) => !orphans.has(slot)); slots = slots.filter((slot) => !orphans.has(slot));
} }

View file

@ -50,6 +50,7 @@ const slotName = (p: string): string => {
const DISABLED_VALUES = new Set(['off', '0', 'false', 'no']); const DISABLED_VALUES = new Set(['off', '0', 'false', 'no']);
const COMMIT_RE = /^[0-9a-f]{7,64}$/; const COMMIT_RE = /^[0-9a-f]{7,64}$/;
const FEATURE_KEY_RE = /^[0-9a-f]{8,64}$/; const FEATURE_KEY_RE = /^[0-9a-f]{8,64}$/;
const COMMIT_GRAPH_DIR_RE = /^[0-9a-f]{7,64}-[0-9a-f]{8,64}$/;
export interface SharedStoreLayout { export interface SharedStoreLayout {
/** Store key: readable basename plus a hash of the canonical git common dir. */ /** Store key: readable basename plus a hash of the canonical git common dir. */
@ -267,8 +268,11 @@ export const resolveGraphPath = (storagePath: string): string => {
if (typeof recorded !== 'string' || !path.isAbsolute(recorded)) return own; if (typeof recorded !== 'string' || !path.isAbsolute(recorded)) return own;
const graph = path.resolve(recorded); const graph = path.resolve(recorded);
const commitDir = path.dirname(graph); const commitDir = path.dirname(graph);
// Only a published `<commit>-<featureKey>` dir, never `.publish-*` staging.
const valid = const valid =
path.basename(graph) === LBUG_DIRECTORY && isDirectChild(path.join(root, 'commits'), commitDir); path.basename(graph) === LBUG_DIRECTORY &&
isDirectChild(path.join(root, 'commits'), commitDir) &&
COMMIT_GRAPH_DIR_RE.test(path.basename(commitDir));
return valid ? graph : own; return valid ? graph : own;
}; };

View file

@ -240,6 +240,30 @@ describe('reclaimSharedStore', () => {
expect(existsSync(outside)).toBe(true); expect(existsSync(outside)).toBe(true);
}); });
it('clean --gc keeps a slot whose index lock is held (analyze in progress)', 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 result = await reclaimSharedStore(layout().root, { gc: true });
expect(result.droppedMembers).toEqual([]);
expect(existsSync(slot)).toBe(true);
} finally {
lock.release();
}
const after = await reclaimSharedStore(layout().root, { gc: true });
expect(after.droppedMembers).toEqual([slot]);
});
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);
});
it('reports a graph it cannot delete instead of failing', async () => { it('reports a graph it cannot delete instead of failing', async () => {
const orphan = await commitGraph('ccccccc-3333333333333333'); const orphan = await commitGraph('ccccccc-3333333333333333');
await member('wt-000000000000', {}); await member('wt-000000000000', {});

View file

@ -249,6 +249,10 @@ describe('resolveGraphPath', () => {
['outside GITNEXUS_HOME', () => '/etc/lbug'], ['outside GITNEXUS_HOME', () => '/etc/lbug'],
['a relative path', () => 'commits/abc1234-deadbeef/lbug'], ['a relative path', () => 'commits/abc1234-deadbeef/lbug'],
['a traversal', (l: SharedStoreLayout) => path.join(l.commitsDir, '..', '..', 'x', 'lbug')], ['a traversal', (l: SharedStoreLayout) => path.join(l.commitsDir, '..', '..', 'x', 'lbug')],
[
'in-progress publish staging',
(l: SharedStoreLayout) => path.join(l.commitsDir, '.publish-0f3c', 'lbug'),
],
[ [
'a non-lbug file', 'a non-lbug file',
(l: SharedStoreLayout) => path.join(l.commitsDir, 'abc1234-deadbeef', 'gitnexus.json'), (l: SharedStoreLayout) => path.join(l.commitsDir, 'abc1234-deadbeef', 'gitnexus.json'),