diff --git a/gitnexus/src/cli/clean.ts b/gitnexus/src/cli/clean.ts index 46dd55cff..6c5b24de5 100644 --- a/gitnexus/src/cli/clean.ts +++ b/gitnexus/src/cli/clean.ts @@ -40,7 +40,7 @@ import { reclaimAfterSlotRemoval, reclaimSharedStore, removeLegacyLocalIndex, - withCheckoutSlotLock, + removeCheckoutStorage, type ReclaimResult, } from '../storage/shared-store-lifecycle.js'; @@ -385,14 +385,7 @@ export const cleanCommand = async (options?: { for (const entry of entries) { try { const storagePath = await requireDeletableStoragePath(entry); - await withCheckoutSlotLock( - storagePath, - async () => { - await fs.rm(storagePath, { recursive: true, force: true }); - await unregisterRepo(entry.path); - }, - entry.path, - ); + await removeCheckoutStorage(storagePath, () => unregisterRepo(entry.path), entry.path); console.log(t('clean.deletedRepo', { name: entry.name, storagePath })); reportReclaim(await reclaimAfterSlotRemoval(storagePath)); } catch (err) { @@ -438,14 +431,7 @@ export const cleanCommand = async (options?: { } try { - await withCheckoutSlotLock( - storagePath, - async () => { - await fs.rm(storagePath, { recursive: true, force: true }); - await unregisterRepo(repo.repoPath); - }, - repo.repoPath, - ); + await removeCheckoutStorage(storagePath, () => unregisterRepo(repo.repoPath), repo.repoPath); console.log(t('common.deleted', { target: storagePath })); reportReclaim(await reclaimAfterSlotRemoval(storagePath)); } catch (err) { diff --git a/gitnexus/src/cli/remove.ts b/gitnexus/src/cli/remove.ts index 060e2ef93..352806f42 100644 --- a/gitnexus/src/cli/remove.ts +++ b/gitnexus/src/cli/remove.ts @@ -31,9 +31,8 @@ import { reclaimAfterSlotRemoval, - withCheckoutSlotLock, + removeCheckoutStorage, } from '../storage/shared-store-lifecycle.js'; -import fs from 'fs/promises'; import { logger } from '../core/logger.js'; import { cliError } from './cli-message.js'; import { t } from './i18n/index.js'; @@ -102,14 +101,7 @@ export const removeCommand = async (target: string, options?: { force?: boolean // orphaned — `listRegisteredRepos({ validate: true })` prunes those on // next read, so the failure is self-healing. try { - await withCheckoutSlotLock( - storagePath, - async () => { - await fs.rm(storagePath, { recursive: true, force: true }); - await unregisterRepo(entry.path); - }, - entry.path, - ); + await removeCheckoutStorage(storagePath, () => unregisterRepo(entry.path), entry.path); await reclaimAfterSlotRemoval(storagePath); console.log(t('remove.removed', { name: entry.name })); console.log(` ${t('common.path')}: ${entry.path}`); diff --git a/gitnexus/src/core/shared-store-analyze.ts b/gitnexus/src/core/shared-store-analyze.ts index 1e433abff..ddaa663bc 100644 --- a/gitnexus/src/core/shared-store-analyze.ts +++ b/gitnexus/src/core/shared-store-analyze.ts @@ -485,8 +485,9 @@ export const leaveSharedStore = async ( try { requireExclusiveIndexLock(lock, `Cannot acquire the index lock at ${previousSlot}.`); await registerLeftStore(repoPath, newStoragePath); - await fs.rm(previousSlot, { recursive: true, force: true }); await removeSharedStorePointer(repoPath); + // Last: the file lock backend keeps its lock file inside this directory. + await fs.rm(previousSlot, { recursive: true, force: true }); } finally { lock.release(); } diff --git a/gitnexus/src/server/api.ts b/gitnexus/src/server/api.ts index 07eeb3057..6963624a5 100644 --- a/gitnexus/src/server/api.ts +++ b/gitnexus/src/server/api.ts @@ -17,7 +17,7 @@ import { ensurePrivateSharedGraph } from '../core/shared-store-analyze.js'; import { resolveGraphPath } from '../storage/shared-store.js'; import { reclaimAfterSlotRemoval, - withCheckoutSlotLock, + removeCheckoutStorage, } from '../storage/shared-store-lifecycle.js'; import express from 'express'; import cors from 'cors'; @@ -1328,9 +1328,7 @@ export const createServer = async (port: number, host: string = '127.0.0.1') => } catch {} // 1. Delete the .gitnexus index/storage directory - await withCheckoutSlotLock(storagePath, () => - fs.rm(storagePath, { recursive: true, force: true }), - ).catch(() => {}); + await removeCheckoutStorage(storagePath).catch(() => {}); await reclaimAfterSlotRemoval(storagePath); // 2. Delete the cloned repo dir if it lives under ~/.gitnexus/repos/. diff --git a/gitnexus/src/storage/shared-store-lifecycle.ts b/gitnexus/src/storage/shared-store-lifecycle.ts index 29b045dc4..e30173efe 100644 --- a/gitnexus/src/storage/shared-store-lifecycle.ts +++ b/gitnexus/src/storage/shared-store-lifecycle.ts @@ -124,13 +124,10 @@ export const reclaimSharedStoreLocked = async ( if (opts.gc) { const orphans = await orphanMembers(slots); for (const slot of [...orphans]) { - if (opts.dryRun) { - 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. + // not orphaned. Judge (and, unless previewing, delete) only a slot + // whose lock is free, so the preview matches what --force would do. let lock: IndexLockHandle; try { lock = await acquireIndexLock(slot, { timeoutMs: 1 }); @@ -143,7 +140,7 @@ export const reclaimSharedStoreLocked = async ( orphans.delete(slot); continue; } - await fs.rm(slot, { recursive: true, force: true }); + if (!opts.dryRun) await fs.rm(slot, { recursive: true, force: true }); result.droppedMembers.push(slot); } finally { lock.release(); @@ -319,25 +316,31 @@ export const removeLegacyLocalIndex = async ( }; /** - * Run `fn` (delete a slot, unregister its checkout) under the slot's index - * lock when `storagePath` is a shared-store checkout slot, then remove - * `checkoutPath`'s store pointer before releasing it. An analyze holds that - * lock until it has registered the checkout and written its pointer, so it - * cannot re-register a slot this removes, write into it, or have its new - * pointer deleted afterwards. Other storage runs `fn` directly, as before. + * Delete a checkout's index storage and run `unregister`. For a shared-store + * checkout slot this happens under the slot's index lock, which an analyze + * holds until it has registered the checkout and written its pointer, so it + * cannot re-register a removed slot, write into it, or have its new pointer + * deleted. There `unregister` and removing `checkoutPath`'s pointer run + * first and the slot directory goes last: the file lock backend keeps its + * lock file inside that directory, so nothing may depend on the lock after + * it is deleted. Other storage is deleted, then unregistered, as before. */ -export const withCheckoutSlotLock = async ( +export const removeCheckoutStorage = async ( storagePath: string, - fn: () => Promise, + unregister: () => Promise = async () => {}, checkoutPath?: string, -): Promise => { - if (!storeRootOfCheckoutSlot(storagePath)) return fn(); +): Promise => { + if (!storeRootOfCheckoutSlot(storagePath)) { + await fs.rm(storagePath, { recursive: true, force: true }); + await unregister(); + return; + } const lock = await acquireIndexLock(storagePath); try { requireExclusiveIndexLock(lock, `Cannot acquire the index lock at ${storagePath}.`); - const result = await fn(); + await unregister(); if (checkoutPath) await removeSharedStorePointer(checkoutPath); - return result; + await fs.rm(storagePath, { recursive: true, force: true }); } finally { lock.release(); } diff --git a/gitnexus/test/integration/shared-store-clean.test.ts b/gitnexus/test/integration/shared-store-clean.test.ts index aa57509cc..f7d34fa9b 100644 --- a/gitnexus/test/integration/shared-store-clean.test.ts +++ b/gitnexus/test/integration/shared-store-clean.test.ts @@ -1,5 +1,5 @@ import { execFileSync } from 'child_process'; -import { existsSync } from 'fs'; +import { existsSync, readFileSync } from 'fs'; import fs from 'fs/promises'; import path from 'path'; import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it, vi } from 'vitest'; @@ -138,6 +138,32 @@ describe('shared store clean (#3352)', () => { expect(existsSync(layoutOf(wtA).root)).toBe(false); }, 240_000); + it('deletes the slot directory last, after unregistering and removing the pointer', async () => { + await analyze(wtA); + const slot = layoutOf(wtA).checkoutSlot; + const pointer = path.join(wtA, '.gitnexus', 'store.json'); + const registry = path.join(tmpHome.dbPath, 'registry.json'); + expect(readFileSync(registry, 'utf-8')).toContain(JSON.stringify(wtA).slice(1, -1)); + const seen: { pointer: boolean; registered: boolean }[] = []; + const realRm = fs.rm; + const rm = vi.spyOn(fs, 'rm').mockImplementation(async (target, options) => { + if (String(target) === slot) { + seen.push({ + pointer: existsSync(pointer), + registered: readFileSync(registry, 'utf-8').includes(JSON.stringify(wtA).slice(1, -1)), + }); + } + return realRm(target, options); + }); + try { + await cleanIn(wtA, { force: true }); + } finally { + rm.mockRestore(); + } + expect(seen).toEqual([{ pointer: false, registered: false }]); + expect(existsSync(slot)).toBe(false); + }, 240_000); + it('previews without --force and deletes nothing', async () => { await analyze(wtA); const layout = layoutOf(wtA); @@ -307,6 +333,21 @@ describe('reclaimSharedStore', () => { expect(after.droppedMembers).toEqual([slot]); }); + it('clean --gc preview does not count a slot whose index lock is held', 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 preview = await reclaimSharedStore(layout().root, { gc: true, dryRun: true }); + expect(preview.droppedMembers).toEqual([]); + } finally { + lock.release(); + } + const after = await reclaimSharedStore(layout().root, { gc: true, dryRun: true }); + expect(after.droppedMembers).toEqual([slot]); + expect(existsSync(slot)).toBe(true); + }); + it('counts references correctly when GITNEXUS_HOME is relative', async () => { const absoluteHome = process.env.GITNEXUS_HOME as string; process.env.GITNEXUS_HOME = path.relative(process.cwd(), absoluteHome);