diff --git a/gitnexus/scripts/cross-platform-tests.ts b/gitnexus/scripts/cross-platform-tests.ts index e301e5de8..76bb3891e 100644 --- a/gitnexus/scripts/cross-platform-tests.ts +++ b/gitnexus/scripts/cross-platform-tests.ts @@ -63,6 +63,9 @@ const PLATFORM_LOGIC = [ 'test/unit/lbug-config-pagesize.test.ts', 'test/unit/worker-pool-windows-quarantine.test.ts', 'test/unit/lbug-pool-fts-load.test.ts', + // Global registry writes use the platform-specific index-lock backend + // (Windows named pipe, Linux socket, or macOS file lock). This includes the + // overlapping-registration regression from #2716 on every OS matrix. 'test/unit/repo-manager.test.ts', 'test/unit/repo-manager-finalize-invariant.test.ts', 'test/unit/git-utils.test.ts', diff --git a/gitnexus/src/storage/repo-manager.ts b/gitnexus/src/storage/repo-manager.ts index 7ddc84bb1..805b28ce5 100644 --- a/gitnexus/src/storage/repo-manager.ts +++ b/gitnexus/src/storage/repo-manager.ts @@ -23,6 +23,7 @@ import { getInferredRepoName, resolveRepoIdentityRoot } from './git.js'; import { stripWindowsLongPathPrefix } from '../lib/utils.js'; import { retryRename } from './fs-atomic.js'; import { logger } from '../core/logger.js'; +import { acquireIndexLock, IndexLockTimeoutError, type IndexLockHandle } from './index-lock.js'; import { branchSlug, BRANCHES_DIR, @@ -1139,6 +1140,69 @@ export const getGlobalRegistryPath = (): string => { return path.join(getGlobalDir(), 'registry.json'); }; +/** + * Lock namespace for the global registry. + * + * Deliberately a dedicated sub-directory rather than {@link getGlobalDir} + * itself: an index slot's lock dir is always `/.gitnexus` (or + * `/.gitnexus/branches/`), so for a repository rooted at the + * user's home directory — dotfiles-at-`$HOME` is a real layout — the per-repo + * analyze lock and the global-dir lock would resolve to the SAME directory. + * `acquireIndexLock` is not reentrant, so `runFullAnalysis` (which holds the + * per-repo lock across its whole pipeline) would then self-deadlock the moment + * it reached `registerRepo`/`adoptFlatBranchLabel`. No repo's index slot can + * ever be named `registry-lock`, so this namespace cannot collide. + */ +const getRegistryLockDir = (): string => path.join(getGlobalDir(), 'registry-lock'); + +/** + * Wait ceiling for the registry lock. A registry transaction is a sub-second + * JSON read/merge/write, so it must NOT inherit the index lock's 10-minute + * default (sized for multi-minute analyze runs): `gitnexus augment` runs on + * every editor/agent tool call with a documented sub-500ms cold-start budget + * and reaches this lock via `listRegisteredRepos({ validate: true })`. + */ +const REGISTRY_LOCK_TIMEOUT_MS = 5_000; + +/** + * Serialize global registry read/merge/write transactions across processes. + * + * The registry is shared by every indexed repository, so per-index locks do + * not protect this file. Reuse the cross-platform index lock primitive with a + * registry-private lock namespace; the handle is kernel-owned on supported + * platforms and crash-reclaimable by the existing fallback. + * + * On timeout the transaction proceeds UNLOCKED rather than throwing: the lock + * closes a lost-update race that existed unguarded before #2716, so degrading + * to the old best-effort behaviour is strictly better than failing an + * `analyze`/`list`/`augment` outright on a wedged lock (a stale pid-reuse + * ghost on platforms without start-time verification can look live forever). + */ +const withRegistryLock = async (operation: () => Promise): Promise => { + let lock: IndexLockHandle | null = null; + try { + lock = await acquireIndexLock(getRegistryLockDir(), { + timeoutMs: REGISTRY_LOCK_TIMEOUT_MS, + // Registry contention was previously invisible: `acquireIndexLock`'s own + // `log` texts name an "analyze" holder, which misattributes a registry + // wait, so surface a registry-specific line instead (#2716 review). + onWaitStart: () => + logger.info('Waiting for another GitNexus process to finish a registry update…'), + }); + } catch (err) { + if (!(err instanceof IndexLockTimeoutError)) throw err; + logger.warn( + { timeoutMs: REGISTRY_LOCK_TIMEOUT_MS }, + 'Timed out waiting for the global registry lock; proceeding without it. A concurrent registry write may be lost.', + ); + } + try { + return await operation(); + } finally { + lock?.release(); + } +}; + /** * Read the global registry. Returns empty array if not found. */ @@ -1285,7 +1349,7 @@ const hasCustomAlias = (entry: RegistryEntry, inferredName: string | null): bool * caller can re-use it to keep AGENTS.md / skill files aligned with the * MCP-visible repo name (#979). */ -export const registerRepo = async ( +const registerRepoUnlocked = async ( repoPath: string, meta: RepoMeta, opts?: RegisterRepoOptions, @@ -1452,11 +1516,17 @@ export const registerRepo = async ( return name; }; +export const registerRepo = async ( + repoPath: string, + meta: RepoMeta, + opts?: RegisterRepoOptions, +): Promise => withRegistryLock(() => registerRepoUnlocked(repoPath, meta, opts)); + /** * Remove a repo from the global registry. * Called after `gitnexus clean`. */ -export const unregisterRepo = async (repoPath: string): Promise => { +const unregisterRepoUnlocked = async (repoPath: string): Promise => { // Canonicalise BOTH sides so an unregister call issued with the // symlink form (`/var/folders/.../repo`) still matches an entry // written with the realpath form (`/private/var/folders/.../repo`), @@ -1468,6 +1538,9 @@ export const unregisterRepo = async (repoPath: string): Promise => { await writeRegistry(filtered); }; +export const unregisterRepo = async (repoPath: string): Promise => + withRegistryLock(() => unregisterRepoUnlocked(repoPath)); + /** * Remove a single non-primary branch's summary from a repo's registry entry * (#2106 R7). Called by `gitnexus clean --branch`. Returns `true` when a @@ -1476,7 +1549,7 @@ export const unregisterRepo = async (repoPath: string): Promise => { * primary entry is left intact; an empty `branches[]` is dropped to keep the * registry shape legacy-clean. */ -export const removeBranchIndex = async (repoPath: string, branch: string): Promise => { +const removeBranchIndexUnlocked = async (repoPath: string, branch: string): Promise => { const resolved = canonicalizePath(repoPath); const entries = await readRegistry(); const idx = entries.findIndex((e) => registryPathEquals(canonicalizePath(e.path), resolved)); @@ -1493,6 +1566,9 @@ export const removeBranchIndex = async (repoPath: string, branch: string): Promi return true; }; +export const removeBranchIndex = async (repoPath: string, branch: string): Promise => + withRegistryLock(() => removeBranchIndexUnlocked(repoPath, branch)); + /** * Record that the flat workspace slot now serves `branch` (#2354). * @@ -1509,6 +1585,12 @@ export const removeBranchIndex = async (repoPath: string, branch: string): Promi * a no-op — including the sub-index deletion, which only runs for registered * repos (never self-heals an unregistered repo, per #2264/#1169; the registry * check precedes the rm per #2364 review F2) — and no subprocess is spawned. + * + * Only the closing re-read/mutate/write runs under the registry lock. The + * recursive `rm` stays outside it — mirroring `clean.ts`, which deletes the + * branch directory before calling the (locked) `removeBranchIndex` — so a slow + * delete (large sub-index, AV scan, network mount) never blocks every other + * registry operation on the machine. */ export const adoptFlatBranchLabel = async (repoPath: string, branch: string): Promise => { const canonicalInput = canonicalizePath(repoPath); @@ -1559,22 +1641,24 @@ export const adoptFlatBranchLabel = async (repoPath: string, branch: string): Pr } } - // Re-read AFTER the potentially slow recursive rm: the registry is a - // multi-writer whole-file overwrite, and writing a pre-rm snapshot would - // silently clobber concurrent registerRepo/removeBranchIndex writers — - // the #2106 R9 re-read-before-write discipline registerRepo follows. - const entries = await readRegistry(); - const idx = isRegistered(entries); - if (idx < 0) return; // unregistered concurrently → still a no-op - const entry = entries[idx]; - const remaining = dirGone ? entry.branches?.filter((b) => b.branch !== branch) : entry.branches; - const droppedSummary = (entry.branches?.length ?? 0) !== (remaining?.length ?? 0); - if (entry.branch === branch && !droppedSummary) return; // already coherent - entry.branch = branch; - if (remaining && remaining.length > 0) entry.branches = remaining; - else delete entry.branches; - entries[idx] = entry; - await writeRegistry(entries); + // Re-read AFTER the potentially slow recursive rm, and under the lock: the + // registry is a multi-writer whole-file overwrite, and writing a pre-rm + // snapshot would silently clobber concurrent registerRepo/removeBranchIndex + // writers — the #2106 R9 re-read-before-write discipline registerRepo follows. + await withRegistryLock(async () => { + const entries = await readRegistry(); + const idx = isRegistered(entries); + if (idx < 0) return; // unregistered concurrently → still a no-op + const entry = entries[idx]; + const remaining = dirGone ? entry.branches?.filter((b) => b.branch !== branch) : entry.branches; + const droppedSummary = (entry.branches?.length ?? 0) !== (remaining?.length ?? 0); + if (entry.branch === branch && !droppedSummary) return; // already coherent + entry.branch = branch; + if (remaining && remaining.length > 0) entry.branches = remaining; + else delete entry.branches; + entries[idx] = entry; + await writeRegistry(entries); + }); }; /** @@ -1908,9 +1992,21 @@ export const listRegisteredRepos = async (opts?: { } } - // If we pruned any entries, save the cleaned registry + // If we pruned any entries, save the cleaned registry — under the lock, and + // only then. The validation walk above is read-only (an fs.access per entry, + // slow on a network mount or a large registry) and the common case prunes + // nothing, so holding the global lock across it would serialize every + // `gitnexus augment` behind unrelated registry work for no benefit. Re-read + // inside the lock and drop the provably-absent paths from that fresh + // snapshot, so a concurrent registration in the validation window survives. if (valid.length !== entries.length) { - await writeRegistry(valid); + const pruned = new Set( + entries.filter((entry) => !valid.includes(entry)).map((entry) => entry.path), + ); + await withRegistryLock(async () => { + const fresh = await readRegistry(); + await writeRegistry(fresh.filter((entry) => !pruned.has(entry.path))); + }); } return valid; diff --git a/gitnexus/test/unit/repo-manager.test.ts b/gitnexus/test/unit/repo-manager.test.ts index ed95fb35c..a0a48d52e 100644 --- a/gitnexus/test/unit/repo-manager.test.ts +++ b/gitnexus/test/unit/repo-manager.test.ts @@ -23,6 +23,7 @@ import { readRegistry, loadCLIConfig, registerRepo, + unregisterRepo, removeBranchIndex, adoptFlatBranchLabel, listRegisteredRepos, @@ -38,6 +39,7 @@ import { type RegistryEntry, type RepoMeta, } from '../../src/storage/repo-manager.js'; +import { acquireIndexLock } from '../../src/storage/index-lock.js'; import { parseRepoNameFromUrl, getInferredRepoName } from '../../src/storage/git.js'; import { execSync } from 'child_process'; import { createTempDir } from '../helpers/test-db.js'; @@ -901,6 +903,74 @@ describe('registerRepo name override + collision guard (#829)', () => { await parentB.cleanup(); } }); + it('preserves all entries when distinct registrations overlap', async () => { + const repos = await Promise.all( + Array.from({ length: 6 }, (_, index) => createTempDir(`gitnexus-concurrent-repo-${index}-`)), + ); + try { + await Promise.all( + repos.map((repo, index) => + registerRepo(repo.dbPath, meta, { name: `concurrent-${index}` }), + ), + ); + + const entries = await listRegisteredRepos(); + expect(entries).toHaveLength(repos.length); + expect(new Set(entries.map((entry) => entry.name))).toEqual( + new Set(repos.map((_, index) => `concurrent-${index}`)), + ); + } finally { + await Promise.all(repos.map((repo) => repo.cleanup())); + } + }); + + it('keeps an overlapping unregisterRepo and registerRepo from clobbering each other', async () => { + await registerRepo(tmpRepoA.dbPath, meta, { name: 'stays' }); + await registerRepo(tmpRepoB.dbPath, meta, { name: 'goes' }); + const added = await createTempDir('gitnexus-concurrent-added-'); + + try { + await Promise.all([ + unregisterRepo(tmpRepoB.dbPath), + registerRepo(added.dbPath, meta, { name: 'added' }), + ]); + + const entries = await listRegisteredRepos(); + expect(new Set(entries.map((entry) => entry.name))).toEqual(new Set(['stays', 'added'])); + } finally { + await added.cleanup(); + } + }); + + it('registers while an index lock is held on the global directory (#2716)', async () => { + // A repo rooted at the user's home directory makes the per-repo analyze + // lock target `~/.gitnexus` — the very directory the registry lock would + // take if it shared that namespace. `runFullAnalysis` holds the per-repo + // lock across its call to `registerRepo` and `acquireIndexLock` is not + // reentrant, so a shared namespace self-deadlocks until the wait ceiling + // and then degrades. The registry lock lives in its own sub-directory, so + // the registration must contend with nothing: no wait announcement, no + // degraded-write warning. Asserted on the log rather than elapsed time — + // the outcome is what matters, and it stays deterministic on a slow runner. + const capture = _captureLogger(); + const held = await acquireIndexLock(tmpHome.dbPath); + try { + await registerRepo(tmpRepoA.dbPath, meta, { name: 'home-rooted' }); + } finally { + held.release(); + capture.restore(); + } + + const logged = capture.records().map((record) => record.msg); + expect(logged).not.toContain( + 'Waiting for another GitNexus process to finish a registry update…', + ); + expect(logged).not.toContain( + 'Timed out waiting for the global registry lock; proceeding without it. A concurrent registry write may be lost.', + ); + const entries = await listRegisteredRepos(); + expect(entries.map((entry) => entry.name)).toEqual(['home-rooted']); + }); }); // ─── registerRepo branch nesting (#2106) ───────────────────────────── @@ -1015,6 +1085,24 @@ describe('registerRepo branch nesting (#2106)', () => { expect(entry.branches?.map((b) => b.branch)).toEqual(['feature/y']); }); + it('overlapping removeBranchIndex calls drop both summaries (#2716)', async () => { + await registerRepo(tmpRepo.dbPath, metaFor('main', 'aaa1111')); + await registerRepo(tmpRepo.dbPath, metaFor('feature/x', 'bbb2222'), { branch: 'feature/x' }); + await registerRepo(tmpRepo.dbPath, metaFor('feature/y', 'ccc3333'), { branch: 'feature/y' }); + + // Unserialized, both writers read the same two-branch snapshot and the + // last rename wins — one summary survives as a lost update. + const removed = await Promise.all([ + removeBranchIndex(tmpRepo.dbPath, 'feature/x'), + removeBranchIndex(tmpRepo.dbPath, 'feature/y'), + ]); + + expect(removed).toEqual([true, true]); + const [entry] = await listRegisteredRepos(); + expect(entry.branch).toBe('main'); // primary intact + expect(entry.branches).toBeUndefined(); + }); + // ─── adoptFlatBranchLabel (#2354) ─────────────────────────────────── it('adoptFlatBranchLabel relabels the entry and removes a shadowed sub-index', async () => {