From 97ec905b5dadac9482e79a90bedcf2c2971b4fc8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gerg=C5=91=20Magyar?= Date: Fri, 30 Oct 2026 20:47:29 +0000 Subject: [PATCH] fix(storage): preserve fresh registry entries (#3777) Recheck prune candidates under the registry lock so stale inspections cannot remove a completed re-registration. Preserve transient errors and unreadable registry state. Refs #3515 Co-authored-by: Gergo Magyar --- gitnexus/src/storage/repo-manager.ts | 31 ++- ...o-manager-registry-validation-race.test.ts | 243 ++++++++++++++++-- 2 files changed, 246 insertions(+), 28 deletions(-) diff --git a/gitnexus/src/storage/repo-manager.ts b/gitnexus/src/storage/repo-manager.ts index c6a362bff..c3c731418 100644 --- a/gitnexus/src/storage/repo-manager.ts +++ b/gitnexus/src/storage/repo-manager.ts @@ -63,6 +63,7 @@ import { LIST_STORAGE_REQUIREMENTS, requireDeletableStoragePath, StorageDeletionError, + type StorageInspection, validateConfiguredStoragePath, } from './storage-resolver.js'; @@ -1745,6 +1746,12 @@ export const listRegisteredRepos = async (opts?: { const prunedSlots = new Set(); const registrySlotKey = (entry: RegistryEntry): string => `${canonicalizePath(entry.path)}\0${canonicalizePath(entry.storagePath)}`; + const isSafelyPrunable = (inspection: StorageInspection): boolean => + !isTransientStorageInspection(inspection) && + (inspection.state === 'missing' || + inspection.state === 'empty' || + (inspection.state === 'unowned' && + inspection.reason === 'Storage directory contains data but no valid ownership metadata.')); const inspections = await mapPool(entries, inspectRegisteredStorage, 8); for (const [entry, inspection] of entries.map( (entry, i) => [entry, inspections[i]] as [RegistryEntry, (typeof inspections)[number]], @@ -1754,12 +1761,7 @@ export const listRegisteredRepos = async (opts?: { (!LIST_STORAGE_REQUIREMENTS.requireCodeIndexDB || inspection.hasCodeIndexDB); if (meetsRequirements) { valid.push(entry); - } else if ( - inspection.state === 'missing' || - inspection.state === 'empty' || - (inspection.state === 'unowned' && - inspection.reason === 'Storage directory contains data but no valid ownership metadata.') - ) { + } else if (isSafelyPrunable(inspection)) { // A missing/empty directory or a registry slot with no ownership // metadata is not a usable index. Removing only the registry row is // safe; the storage directory itself is never deleted here. @@ -1786,15 +1788,22 @@ export const listRegisteredRepos = async (opts?: { // only then. The validation walk above is read-only and can touch several // files per entry, 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 only the same provably-absent storage - // slots from that fresh snapshot, so a concurrent re-registration of the - // same repository path into another slot survives. + // Reinspect only deletion candidates under the lock: even the same slot + // with identical metadata may have been rebuilt and re-registered since + // the initial inspection (#3515). Other slots keep the lock-free read path. if (prunedSlots.size > 0) { try { await withRegistryLock(async () => { - const fresh = await readRegistry(); + const fresh = await readRegistryStrictIfPresent(); + if (!fresh) return; + const candidates = fresh.filter((entry) => prunedSlots.has(registrySlotKey(entry))); + const rechecked = await mapPool(candidates, inspectRegisteredStorage, 8); + const confirmedSlots = new Set( + candidates.filter((_, i) => isSafelyPrunable(rechecked[i])).map(registrySlotKey), + ); + if (confirmedSlots.size === 0) return; await writeRegistry( - fresh.filter((entry) => !prunedSlots.has(registrySlotKey(entry))), + fresh.filter((entry) => !confirmedSlots.has(registrySlotKey(entry))), 1, ); }); diff --git a/gitnexus/test/unit/repo-manager-registry-validation-race.test.ts b/gitnexus/test/unit/repo-manager-registry-validation-race.test.ts index eec94d89c..4c97eb8a6 100644 --- a/gitnexus/test/unit/repo-manager-registry-validation-race.test.ts +++ b/gitnexus/test/unit/repo-manager-registry-validation-race.test.ts @@ -15,8 +15,10 @@ import { listRegisteredRepos, readRegistry, registerRepo, + type RegistryEntry, type RepoMeta, } from '../../src/storage/repo-manager.js'; +import type { StorageInspection } from '../../src/storage/storage-resolver.js'; import { createTempDir } from '../helpers/test-db.js'; describe('listRegisteredRepos validation persistence', () => { @@ -72,25 +74,232 @@ describe('listRegisteredRepos validation persistence', () => { const releaseInspectionPromise = new Promise((resolve) => { releaseInspection = resolve; }); - storageResolverCtx.inspectRegisteredStorage.mockImplementationOnce(async (entry: any) => { - inspectionStarted(); - await releaseInspectionPromise; - return { - repoPath: entry.path, - storagePath: entry.storagePath, - state: 'missing', - hasCodeIndexDB: false, - }; - }); + storageResolverCtx.inspectRegisteredStorage.mockImplementationOnce( + async (entry: RegistryEntry) => { + inspectionStarted(); + await releaseInspectionPromise; + return { + repoPath: entry.path, + storagePath: entry.storagePath, + state: 'missing', + hasCodeIndexDB: false, + }; + }, + ); const validatingRead = listRegisteredRepos({ validate: true }); - await inspectionStartedPromise; - await registerRepo(repoPath, meta, { name: 'repo', storagePath: replacementStoragePath }); - releaseInspection(); + try { + await inspectionStartedPromise; + await registerRepo(repoPath, meta, { name: 'repo', storagePath: replacementStoragePath }); + releaseInspection(); - await expect(validatingRead).resolves.toEqual([]); - await expect(readRegistry()).resolves.toMatchObject([ - { path: repoPath, storagePath: replacementStoragePath }, - ]); + await expect(validatingRead).resolves.toEqual([]); + await expect(readRegistry()).resolves.toMatchObject([ + { path: repoPath, storagePath: replacementStoragePath }, + ]); + } finally { + releaseInspection(); + await validatingRead.catch(() => undefined); + } + }); + + it.each(['updated', 'unchanged'] as const)( + 'keeps a concurrent same-slot re-registration with %s metadata', + async (metadata) => { + const repoPath = tmpRepo.dbPath; + const storagePath = path.join(repoPath, '.gitnexus'); + const meta: RepoMeta = { + repoPath, + storagePath, + lastCommit: 'initial-commit', + indexedAt: '2026-09-05T00:00:00.000Z', + stats: { files: 1, nodes: 1 }, + }; + await registerRepo(repoPath, meta, { name: 'repo', storagePath }); + + let inspectionStarted!: () => void; + let releaseInspection!: () => void; + const started = new Promise((resolve) => { + inspectionStarted = resolve; + }); + const released = new Promise((resolve) => { + releaseInspection = resolve; + }); + storageResolverCtx.inspectRegisteredStorage + .mockResolvedValue({ + repoPath, + storagePath, + state: 'owned', + hasCodeIndexDB: true, + } satisfies StorageInspection) + .mockImplementationOnce(async (): Promise => { + inspectionStarted(); + await released; + return { repoPath, storagePath, state: 'missing', hasCodeIndexDB: false }; + }); + + const validatingRead = listRegisteredRepos({ validate: true }); + try { + await started; + const replacement = + metadata === 'updated' + ? { ...meta, lastCommit: 'rebuilt-commit', indexedAt: '2026-09-06T00:00:00.000Z' } + : meta; + await registerRepo(repoPath, replacement, { name: 'repo', storagePath }); + releaseInspection(); + + await expect(validatingRead).resolves.toEqual([]); + await expect(readRegistry()).resolves.toMatchObject([ + { + path: repoPath, + storagePath, + lastCommit: replacement.lastCommit, + indexedAt: replacement.indexedAt, + }, + ]); + } finally { + releaseInspection(); + await validatingRead.catch(() => undefined); + } + }, + ); + + it('still persists pruning when the registered storage remains missing', async () => { + const repoPath = tmpRepo.dbPath; + const storagePath = path.join(repoPath, '.gitnexus'); + await registerRepo(repoPath, { + repoPath, + lastCommit: 'initial-commit', + indexedAt: '2026-09-05T00:00:00.000Z', + stats: { files: 1, nodes: 1 }, + }); + storageResolverCtx.inspectRegisteredStorage.mockResolvedValue({ + repoPath, + storagePath, + state: 'missing', + hasCodeIndexDB: false, + } satisfies StorageInspection); + + await expect(listRegisteredRepos({ validate: true })).resolves.toEqual([]); + await expect(readRegistry()).resolves.toEqual([]); + }); + + it.each([ + { label: 'owned', state: 'owned', hasCodeIndexDB: true }, + { + label: 'temporarily unreadable', + state: 'invalid_storage', + hasCodeIndexDB: false, + reason: 'EIO: Storage directory could not be inspected.', + }, + { + label: 'empty with a transient database error', + state: 'empty', + hasCodeIndexDB: false, + reason: 'EIO: LadybugDB directory could not be inspected.', + }, + ] satisfies Array< + Pick & { label: string } + >)( + 'does not rewrite the registry when the candidate becomes $label', + async ({ label: _label, ...inspection }) => { + const repoPath = tmpRepo.dbPath; + const storagePath = path.join(repoPath, '.gitnexus'); + const registryPath = path.join(tmpHome.dbPath, 'registry.json'); + const original = JSON.stringify([{ name: 'repo', path: repoPath, storagePath }]); + // Compact formatting makes an otherwise identical atomic rewrite visible. + await fs.writeFile(registryPath, original); + storageResolverCtx.inspectRegisteredStorage + .mockResolvedValue({ repoPath, storagePath, ...inspection } satisfies StorageInspection) + .mockResolvedValueOnce({ + repoPath, + storagePath, + state: 'missing', + hasCodeIndexDB: false, + } satisfies StorageInspection); + + await expect(listRegisteredRepos({ validate: true })).resolves.toEqual([]); + expect(await fs.readFile(registryPath, 'utf8')).toBe(original); + }, + ); + + it('keeps an initially empty slot when its database inspection failed transiently', async () => { + const repoPath = tmpRepo.dbPath; + const storagePath = path.join(repoPath, '.gitnexus'); + const registryPath = path.join(tmpHome.dbPath, 'registry.json'); + const entry = { name: 'repo', path: repoPath, storagePath }; + const original = JSON.stringify([entry]); + await fs.writeFile(registryPath, original); + storageResolverCtx.inspectRegisteredStorage.mockResolvedValue({ + repoPath, + storagePath, + state: 'empty', + hasCodeIndexDB: false, + reason: 'EIO: LadybugDB directory could not be inspected.', + } satisfies StorageInspection); + + await expect(listRegisteredRepos({ validate: true })).resolves.toEqual([entry]); + expect(await fs.readFile(registryPath, 'utf8')).toBe(original); + }); + + it.each(['corrupt', 'deleted'] as const)( + 'does not overwrite a registry that becomes %s during validation', + async (change) => { + const repoPath = tmpRepo.dbPath; + const storagePath = path.join(repoPath, '.gitnexus'); + const registryPath = path.join(tmpHome.dbPath, 'registry.json'); + const corrupt = '{"truncated":'; + await fs.writeFile( + registryPath, + JSON.stringify([{ name: 'repo', path: repoPath, storagePath }]), + ); + storageResolverCtx.inspectRegisteredStorage.mockImplementation( + async (): Promise => { + if (change === 'corrupt') await fs.writeFile(registryPath, corrupt); + else await fs.rm(registryPath, { force: true }); + return { repoPath, storagePath, state: 'missing', hasCodeIndexDB: false }; + }, + ); + + await expect(listRegisteredRepos({ validate: true })).resolves.toEqual([]); + if (change === 'corrupt') expect(await fs.readFile(registryPath, 'utf8')).toBe(corrupt); + else await expect(fs.access(registryPath)).rejects.toMatchObject({ code: 'ENOENT' }); + }, + ); + + it('preserves an unrelated registration completed during validation', async () => { + const repoPath = tmpRepo.dbPath; + const storagePath = path.join(repoPath, '.gitnexus'); + const addedRepoPath = path.join(tmpHome.dbPath, 'new-repo'); + const registryPath = path.join(tmpHome.dbPath, 'registry.json'); + await fs.writeFile( + registryPath, + JSON.stringify([{ name: 'gone', path: repoPath, storagePath }]), + ); + storageResolverCtx.inspectRegisteredStorage + .mockResolvedValue({ + repoPath, + storagePath, + state: 'missing', + hasCodeIndexDB: false, + } satisfies StorageInspection) + .mockImplementationOnce(async (): Promise => { + await registerRepo( + addedRepoPath, + { + repoPath: addedRepoPath, + lastCommit: 'new-commit', + indexedAt: '2026-09-06T00:00:00.000Z', + stats: { files: 1, nodes: 1 }, + }, + { name: 'new-repo' }, + ); + return { repoPath, storagePath, state: 'missing', hasCodeIndexDB: false }; + }); + + await expect(listRegisteredRepos({ validate: true })).resolves.toEqual([]); + const persisted = await readRegistry(); + expect(persisted).toHaveLength(1); + expect(persisted[0]).toMatchObject({ name: 'new-repo', path: addedRepoPath }); }); });