fix: serialize global registry transactions across processes (#2716)

* fix: serialize global registry mutations * fix: serialize global registry mutations * fix: keep registry reads lock-free * test(registry): document cross-platform lock coverage * fix(registry): isolate the registry lock's namespace, timeout and diagnostics Review follow-ups on the global registry lock (#2716): - The lock took `getGlobalDir()` itself, which is byte-identical to a repo's index slot when that repo is rooted at the user's home directory (a real dotfiles layout). `runFullAnalysis` holds the per-repo lock across its whole pipeline and `acquireIndexLock` is not reentrant, so `registerRepo` / `adoptFlatBranchLabel` self-deadlocked until the wait ceiling and then failed the analyze. The registry now locks a private `<globalDir>/registry-lock` namespace no index slot can ever resolve to. - The lock inherited the index lock's 10-minute default timeout, sized for multi-minute analyze runs. `gitnexus augment` — documented to cold-start in under 500ms and shelled out from editor tool-use hooks — reaches it through `listRegisteredRepos({ validate: true })`. Registry transactions are sub-second, so they now get their own 5s ceiling. - Contention was silent: no `log`/`onWaitStart` was wired, and the primitive's own texts attribute a wait to "another gitnexus analyze", which misnames a registry holder. A registry-specific line is emitted on wait start instead. - On timeout the transaction now proceeds unlocked with a warning rather than throwing. The lost-update race it guards was unguarded before this branch, so degrading to the old best-effort behaviour beats failing `analyze`/`list`/ `index` outright — none of which wrap these calls in a handler — on a wedged lock. - `adoptFlatBranchLabel`'s recursive `fs.rm` no longer runs inside the lock; only the closing re-read/mutate/write does, mirroring `clean.ts`, which deletes the branch directory before calling the locked `removeBranchIndex`. A slow delete no longer blocks every registry operation on the machine. * test(registry): cover the remaining locked mutators and the colliding layout Three of the five functions the registry lock wraps had no overlap coverage, so a future narrowing of the lock would go unnoticed. Adds: - overlapping `removeBranchIndex` calls on two branches of one entry, - an overlapping `unregisterRepo` / `registerRepo` pair on distinct repos, - a registration issued while an index lock is held on the global directory, which reproduces the home-rooted self-contention the lock namespace fix addresses. Each fails without the corresponding fix: the two overlap tests lose an update when `withRegistryLock` is bypassed, and the collision test sees the wait announcement and the degraded-write warning once the lock namespace is reverted to `getGlobalDir()`. The collision test asserts on those log records rather than on elapsed time, so it stays deterministic on a slow runner. * perf(registry): keep the validation walk out of the registry lock `listRegisteredRepos({ validate: true })` held the global lock across its read-only validation walk — an `fs.access` per entry, slow on a network mount or a large registry — even though the common case prunes nothing and writes nothing. That is the same lock `gitnexus augment` takes on every editor tool call, so unrelated registry work serialized behind a walk that never touched the file. The walk now runs unlocked; the lock is taken only when an entry is provably gone, and the prune is applied to a snapshot re-read inside it, so a registration that lands during the walk is no longer clobbered by a stale write. Same shape as the `adoptFlatBranchLabel` split. --------- Co-authored-by: Gergő Magyar <gergomagyar@icloud.com> Co-authored-by: Gergo Magyar <gergomagyar0@gmail.com>
2026-08-28 05:25:25 +00:00 · 2026-08-01 10:47:14 +03:00 · 2026-08-01 10:47:14 +03:00 · 7ee0df9e55
commit 7ee0df9e55
parent 064832f50c
3 changed files with 208 additions and 21 deletions
--- 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',
--- 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 `<repo>/.gitnexus` (or
+ * `<repo>/.gitnexus/branches/<slug>`), 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 <T>(operation: () => Promise<T>): Promise<T> => {
+  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<string> => withRegistryLock(() => registerRepoUnlocked(repoPath, meta, opts));
+
 /**
 * Remove a repo from the global registry.
 * Called after `gitnexus clean`.
 */
-export const unregisterRepo = async (repoPath: string): Promise<void> => {
+const unregisterRepoUnlocked = async (repoPath: string): Promise<void> => {
  // 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<void> => {
  await writeRegistry(filtered);
 };

+export const unregisterRepo = async (repoPath: string): Promise<void> =>
+  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<void> => {
 * 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<boolean> => {
+const removeBranchIndexUnlocked = async (repoPath: string, branch: string): Promise<boolean> => {
  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<boolean> =>
+  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<void> => {
  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;
--- 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 () => {