From d3861b6af6eb53cc225cc8181250136b51967f21 Mon Sep 17 00:00:00 2001 From: luciferlive112116 <291889058+luciferlive112116@users.noreply.github.com> Date: Fri, 17 Jul 2026 03:30:39 +0800 Subject: [PATCH] fix(miner): open the worktree allocator through local-store's openLocalStoreDb (#6600) The allocator's own header comment claimed to mirror run-state.js/claim-ledger.js/ portfolio-queue.js, but those have opened via openLocalStoreDb since #4272 while this still hand-rolled the mkdirSync/DatabaseSync/chmodSync/PRAGMA sequence they used to have. Going through the helper is what calls registerCleanupResource (#4826), so the allocator was never registered for crash-safe cleanup -- despite being the store where a SIGINT/SIGTERM mid-write strands a worktree slot leased to a dead process. Env-var resolution is unchanged: the deleted inline resolver was line-for-line what resolveLocalStoreDbPath already does. resolveWorktreeBaseDir is untouched -- it resolves a git-worktree checkout directory, not a store DB path. Closes #6600 --- .../loopover-miner/lib/worktree-allocator.js | 42 +++++++------------ test/unit/miner-worktree-allocator.test.ts | 37 ++++++++++++++++ 2 files changed, 53 insertions(+), 26 deletions(-) diff --git a/packages/loopover-miner/lib/worktree-allocator.js b/packages/loopover-miner/lib/worktree-allocator.js index ade02d63cd..dd660f60d1 100644 --- a/packages/loopover-miner/lib/worktree-allocator.js +++ b/packages/loopover-miner/lib/worktree-allocator.js @@ -1,11 +1,19 @@ -import { chmodSync, mkdirSync } from "node:fs"; +// mkdirSync is still needed for the git-worktree CHECKOUT dirs below (resolveWorktreeBaseDir's tree) — that is +// a filesystem directory, not a store DB path, and is deliberately out of this migration's scope. Only the DB +// handle's own mkdir/chmod moved into openLocalStoreDb. +import { mkdirSync } from "node:fs"; import { homedir } from "node:os"; -import { dirname, join } from "node:path"; -import { DatabaseSync } from "node:sqlite"; +import { join } from "node:path"; +import { normalizeLocalStoreDbPath, openLocalStoreDb, resolveLocalStoreDbPath } from "./local-store.js"; // Git-worktree-per-attempt allocator (#4297): durable local bookkeeping for which worktree paths are -// allocated to which fleet attempts. Mirrors the package's existing local-store pattern (run-state.js, -// claim-ledger.js, portfolio-queue.js) — plain JS + node:sqlite, never phones home. +// allocated to which fleet attempts. Opens its handle through local-store.js's openLocalStoreDb (#4272), the +// same call run-state.js / claim-ledger.js / portfolio-queue.js use — plain JS + node:sqlite, never phones +// home. Going through openLocalStoreDb is what registers the handle for crash-safe cleanup +// (process-lifecycle.js, #4826), which matters most for exactly this store: a SIGINT/SIGTERM mid-write is what +// leaves a worktree slot leased to a process that no longer exists (#6600). It previously hand-rolled the +// identical mkdirSync/chmodSync/PRAGMA sequence and so was never registered, despite this comment already +// claiming to mirror those three files. const defaultDbFileName = "worktree-allocator.sqlite3"; const defaultWorktreeDirName = "worktrees"; @@ -13,20 +21,7 @@ const defaultMaxConcurrency = 2; let defaultWorktreeAllocator = null; export function resolveWorktreeAllocatorDbPath(env = process.env) { - const explicitPath = typeof env.LOOPOVER_MINER_WORKTREE_ALLOCATOR_DB === "string" - ? env.LOOPOVER_MINER_WORKTREE_ALLOCATOR_DB.trim() - : ""; - if (explicitPath) return explicitPath; - - const explicitConfigDir = typeof env.LOOPOVER_MINER_CONFIG_DIR === "string" - ? env.LOOPOVER_MINER_CONFIG_DIR.trim() - : ""; - if (explicitConfigDir) return join(explicitConfigDir, defaultDbFileName); - - const configHome = typeof env.XDG_CONFIG_HOME === "string" && env.XDG_CONFIG_HOME.trim() - ? env.XDG_CONFIG_HOME.trim() - : join(homedir(), ".config"); - return join(configHome, "loopover-miner", defaultDbFileName); + return resolveLocalStoreDbPath(defaultDbFileName, "LOOPOVER_MINER_WORKTREE_ALLOCATOR_DB", env); } export function resolveWorktreeBaseDir(env = process.env) { @@ -47,9 +42,7 @@ export function resolveWorktreeBaseDir(env = process.env) { } function normalizeDbPath(dbPath) { - const path = (dbPath ?? resolveWorktreeAllocatorDbPath()).trim(); - if (!path) throw new Error("invalid_worktree_allocator_db_path"); - return path; + return normalizeLocalStoreDbPath(dbPath, resolveWorktreeAllocatorDbPath(), "invalid_worktree_allocator_db_path"); } function normalizeWorktreeBaseDir(worktreeBaseDir) { @@ -154,10 +147,7 @@ export function openWorktreeAllocator(options = {}) { const maxConcurrency = normalizeMaxConcurrency(options.maxConcurrency); const processPid = Number.isInteger(options.processPid) ? options.processPid : process.pid; - mkdirSync(dirname(resolvedPath), { recursive: true, mode: 0o700 }); - const db = new DatabaseSync(resolvedPath); - chmodSync(resolvedPath, 0o600); - db.exec("PRAGMA busy_timeout = 5000"); + const db = openLocalStoreDb(resolvedPath); ensureSlotTable(db); ensureSlots(db, worktreeBaseDir, maxConcurrency); reclaimOrphanedAllocations(db); diff --git a/test/unit/miner-worktree-allocator.test.ts b/test/unit/miner-worktree-allocator.test.ts index 2d62e06f2c..10acf27b50 100644 --- a/test/unit/miner-worktree-allocator.test.ts +++ b/test/unit/miner-worktree-allocator.test.ts @@ -9,10 +9,24 @@ import { resolveWorktreeAllocatorDbPath, resolveWorktreeBaseDir, } from "../../packages/loopover-miner/lib/worktree-allocator.js"; +import { + cleanupResourceCount, + closeAllCleanupResources, + resetProcessLifecycleForTesting, +} from "../../packages/loopover-miner/lib/process-lifecycle.js"; const roots: string[] = []; const allocators: Array<{ close(): void }> = []; +/** Opt an allocator out of the shared afterEach close, for a test that closes the handle itself. `close()` is + * not idempotent (node:sqlite throws "database is not open"), so a test asserting the close path must own the + * handle's lifetime outright rather than be closed a second time on the way out. */ +function ownClose(allocator: T): T { + const index = allocators.indexOf(allocator); + if (index >= 0) allocators.splice(index, 1); + return allocator; +} + function tempAllocator(options: { maxConcurrency?: number; processPid?: number } = {}) { const root = mkdtempSync(join(tmpdir(), "loopover-miner-worktree-allocator-")); roots.push(root); @@ -98,4 +112,27 @@ describe("loopover-miner worktree allocator scaffolding (#4298)", () => { const second = allocator.acquire("attempt-a", "acme/widgets"); expect(second.worktreePath).toBe(first.worktreePath); }); + + it("registers the store for crash-safe cleanup and unregisters it on close (#6600)", () => { + // The whole point of routing through openLocalStoreDb: a SIGINT/SIGTERM mid-write is what leaves a worktree + // slot leased to a dead process, so this store must be closed by the signal handlers like its 3 siblings. + // Hand-rolling `new DatabaseSync(...)` registered nothing, so this count stayed at 0. + resetProcessLifecycleForTesting(); + expect(cleanupResourceCount()).toBe(0); + const allocator = ownClose(tempAllocator({ maxConcurrency: 1 })); + expect(cleanupResourceCount()).toBe(1); + allocator.close(); + // The normal close() unregisters, so a long-running loop doesn't accumulate stale handles or double-close. + expect(cleanupResourceCount()).toBe(0); + }); + + it("is closed by closeAllCleanupResources when the process dies mid-write (#6600)", () => { + resetProcessLifecycleForTesting(); + const allocator = ownClose(tempAllocator({ maxConcurrency: 1 })); + allocator.acquire("attempt-a", "acme/widgets"); + expect(cleanupResourceCount()).toBe(1); + + closeAllCleanupResources(); // what installCliSignalHandlers invokes on SIGINT/SIGTERM + expect(cleanupResourceCount()).toBe(0); + }); });