Skip to content

Commit da0feb7

Browse files
boskodev790JSONbored
authored andcommitted
#8320 Cover worktree-allocator's slot pool in the right-to-be-forgotten purge
1 parent fe816af commit da0feb7

5 files changed

Lines changed: 197 additions & 8 deletions

File tree

packages/loopover-miner/lib/purge-cli.ts

Lines changed: 20 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,8 @@ import { openReplaySnapshotStore, resolveReplaySnapshotDbPath } from "./replay-s
3737
import type { ReplaySnapshotStore } from "./replay-snapshot.js";
3838
import { initDenyHookSynthesisStore, resolveDenyHookSynthesisDbPath } from "./deny-hook-synthesis.js";
3939
import type { DenyHookSynthesisStore } from "./deny-hook-synthesis.js";
40+
import { countWorktreeSlotsToPurge, openWorktreeAllocator, resolveWorktreeAllocatorDbPath } from "./worktree-allocator.js";
41+
import type { WorktreeAllocator } from "./worktree-allocator.js";
4042
import { resolveAttemptLogDbPath } from "./attempt-log.js";
4143
import {
4244
CLAIM_LEDGER_PURGE_SPEC,
@@ -79,7 +81,8 @@ type PurgeOpenerKey =
7981
| "initPolicyVerdictCacheStore"
8082
| "initRankedCandidatesStore"
8183
| "openReplaySnapshotStore"
82-
| "initDenyHookSynthesisStore";
84+
| "initDenyHookSynthesisStore"
85+
| "openWorktreeAllocator";
8386

8487
export type PurgeCliOptions = {
8588
openClaimLedger?: () => ClaimLedger;
@@ -94,6 +97,7 @@ export type PurgeCliOptions = {
9497
initRankedCandidatesStore?: () => RankedCandidatesStore;
9598
openReplaySnapshotStore?: () => ReplaySnapshotStore;
9699
initDenyHookSynthesisStore?: () => DenyHookSynthesisStore;
100+
openWorktreeAllocator?: () => WorktreeAllocator;
97101
resolveDbPaths?: Record<string, () => string>;
98102
};
99103

@@ -104,6 +108,10 @@ type PurgeTarget = {
104108
resolveDbPath: () => string;
105109
spec?: LedgerPurgeSpec;
106110
specs?: LedgerPurgeSpec[];
111+
// A store whose per-repo purge lives on its store object (not a generic DELETE spec) supplies its own read-only
112+
// dry-run counter instead of `spec`/`specs`, so --dry-run counts EXACTLY the rows the real purge would touch
113+
// (e.g. worktree-allocator, whose fixed slot pool is cleared with a status-guarded UPDATE, never a DELETE).
114+
countDryRun?: (db: DatabaseSync, repoFullName: string) => number;
107115
};
108116

109117
const REAL_PURGE_TARGETS: PurgeTarget[] = [
@@ -124,6 +132,11 @@ const REAL_PURGE_TARGETS: PurgeTarget[] = [
124132
{ name: "ranked-candidates", optionKey: "initRankedCandidatesStore", opener: initRankedCandidatesStore, resolveDbPath: resolveRankedCandidatesDbPath, spec: RANKED_CANDIDATES_PURGE_SPEC },
125133
{ name: "replay-snapshot", optionKey: "openReplaySnapshotStore", opener: openReplaySnapshotStore, resolveDbPath: resolveReplaySnapshotDbPath, spec: REPLAY_SNAPSHOT_PURGE_SPEC },
126134
{ name: "deny-hook-synthesis", optionKey: "initDenyHookSynthesisStore", opener: initDenyHookSynthesisStore, resolveDbPath: resolveDenyHookSynthesisDbPath, spec: DENY_HOOK_SYNTHESIS_PURGE_SPEC },
135+
// worktree-allocator's `worktree_slots` is a FIXED pool of pre-allocated slot rows, not an append-only ledger, so
136+
// the generic DELETE-based spec is the wrong shape (deleting a slot would shrink the pool below maxConcurrency).
137+
// Its purge lives on the store object (a `status = 'free'` UPDATE blanking the repo columns, never touching a
138+
// live `active` slot), and its dry-run count uses the matching read-only counter — no `spec`/`specs` (#8320).
139+
{ name: "worktree-allocator", optionKey: "openWorktreeAllocator", opener: openWorktreeAllocator, resolveDbPath: resolveWorktreeAllocatorDbPath, countDryRun: countWorktreeSlotsToPurge },
127140
];
128141

129142
export type ParsedPurgeArgs = { json: boolean; dryRun: boolean; repoFullName: string } | { error: string };
@@ -216,13 +229,14 @@ export function runPurgeDryRun(
216229
const resolveDbPaths = options.resolveDbPaths ?? {};
217230
const stores: PurgeDryRunStoreResult[] = REAL_PURGE_TARGETS.map((target) => {
218231
const dbPath = (resolveDbPaths[target.name] ?? target.resolveDbPath)();
219-
// A target scopes one table (`spec`) or -- for governor-state -- several in one file (`specs`); sum the
220-
// per-table counts against the single read-only handle so the preview matches what a real purge removes.
221-
// Every REAL_PURGE_TARGETS entry declares exactly one of the two, so `target.spec` is always set here.
222-
const specs = target.specs ?? [target.spec!];
232+
// A target scopes one table (`spec`), several in one file (`specs`, e.g. governor-state), or supplies its own
233+
// read-only counter (`countDryRun`, e.g. worktree-allocator's status-guarded slot count). Every entry declares
234+
// exactly one of the three, so the `target.spec!` fallback below is only reached when neither other is set.
223235
try {
224236
const wouldPurge = countExistingRows(dbPath, (db) =>
225-
specs.reduce((sum, spec) => sum + countStoreByRepo(db, spec, parsed.repoFullName), 0),
237+
target.countDryRun
238+
? target.countDryRun(db, parsed.repoFullName)
239+
: (target.specs ?? [target.spec!]).reduce((sum, spec) => sum + countStoreByRepo(db, spec, parsed.repoFullName), 0),
226240
);
227241
return { store: target.name, wouldPurge };
228242
} catch (error) {

packages/loopover-miner/lib/worktree-allocator.ts

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@ export type WorktreeAllocator = {
3838
acquire(attemptId: string, repoFullName: string): WorktreeAllocation;
3939
release(attemptId: string): WorktreeAllocation | null;
4040
listSlots(): WorktreeAllocation[];
41+
purgeByRepo(repoFullName: string): number;
4142
close(): void;
4243
};
4344

@@ -304,6 +305,16 @@ export function openWorktreeAllocator(options: {
304305
const listSlots = db.prepare(
305306
"SELECT slot_index, worktree_path, attempt_id, repo_full_name, status, owner_pid, owner_host, allocated_at FROM worktree_slots ORDER BY slot_index",
306307
);
308+
// Right-to-be-forgotten backstop (#8320): blank a FREE slot's stale repo columns (mirroring release()'s own
309+
// clearing UPDATE), never DELETE — deleting a row would shrink the fixed pool below maxConcurrency and break
310+
// ensureSlots/selectFreeSlot's every-slot-exists invariant. The `status = 'free'` guard is what protects a live
311+
// in-flight attempt: an `active` slot's repo_full_name reflects a real on-disk worktree checkout, so clearing it
312+
// would desync the allocator from that checkout — such a row is never matched here.
313+
const purgeFreeByRepo = db.prepare(`
314+
UPDATE worktree_slots
315+
SET repo_full_name = NULL, attempt_id = NULL, owner_pid = NULL, owner_host = NULL, allocated_at = NULL
316+
WHERE status = 'free' AND repo_full_name = ?
317+
`);
307318

308319
const allocator: WorktreeAllocator = {
309320
dbPath: resolvedPath,
@@ -358,6 +369,14 @@ export function openWorktreeAllocator(options: {
358369
listSlots() {
359370
return (listSlots.all() as WorktreeSlotRow[]).map(rowToAllocation);
360371
},
372+
purgeByRepo(repoFullName) {
373+
// Normalize the same way acquire() persisted it (owner/repo), so the WHERE clause matches — mirroring
374+
// governor-state.ts's own purgeByRepo. Expected to affect 0 rows in the overwhelming majority of real
375+
// calls: only a `free` slot left carrying a stale repo_full_name (a crash between acquire and the normal
376+
// clear) can match, since release()/reclaimOrphanedAllocations() already blank these fields on every free.
377+
const normalizedRepo = normalizeRepoFullName(repoFullName);
378+
return Number(purgeFreeByRepo.run(normalizedRepo).changes);
379+
},
361380
close() {
362381
db.close();
363382
},
@@ -366,6 +385,20 @@ export function openWorktreeAllocator(options: {
366385
return allocator;
367386
}
368387

388+
/**
389+
* Read-only count of the rows {@link WorktreeAllocator.purgeByRepo} would clear for one repo — `free` slots still
390+
* carrying a stale `repo_full_name`, never an `active` slot (whose repo reflects a live in-flight checkout). Runs
391+
* against a bare read-only handle (purge-cli.js's `--dry-run` path opens the file itself), so it takes a
392+
* `DatabaseSync` rather than the allocator object, and its WHERE clause matches `purgeByRepo`'s EXACTLY so the
393+
* dry-run preview equals what a real purge removes.
394+
*/
395+
export function countWorktreeSlotsToPurge(db: DatabaseSync, repoFullName: string): number {
396+
const row = db
397+
.prepare("SELECT COUNT(*) AS count FROM worktree_slots WHERE status = 'free' AND repo_full_name = ?")
398+
.get(repoFullName) as CountRow | undefined;
399+
return Number(row?.count);
400+
}
401+
369402
function getDefaultWorktreeAllocator(): WorktreeAllocator {
370403
defaultWorktreeAllocator ??= openWorktreeAllocator();
371404
return defaultWorktreeAllocator;

test/unit/miner-attempt-cli.test.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1372,6 +1372,7 @@ describe("runAttempt (#5132)", () => {
13721372
},
13731373
release: vi.fn(),
13741374
listSlots: () => [],
1375+
purgeByRepo: vi.fn(() => 0),
13751376
close: vi.fn(),
13761377
}),
13771378
openClaimLedger: () => claimLedger,

0 commit comments

Comments
 (0)