mirror of
https://github.com/abhigyanpatwari/GitNexus.git
synced 2026-09-30 01:51:20 +00:00
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) <noreply@anthropic.com>
This commit is contained in:
parent
0d97f74545
commit
d746675c28
15 changed files with 128 additions and 46 deletions
|
|
@ -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}`);
|
||||
|
|
|
|||
|
|
@ -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 仓库。',
|
||||
|
|
|
|||
|
|
@ -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',
|
||||
|
|
|
|||
|
|
@ -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}`);
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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<boolean> =>
|
||||
(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;
|
||||
};
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
};
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
};
|
||||
|
||||
/**
|
||||
|
|
|
|||
|
|
@ -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');
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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-/);
|
||||
});
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue