Fix lock race condition, scan depth off-by-one, and minor cleanups

Close TOCTOU window in withGlobalLock by retrying mkdir immediately
after removing a stale lock. Fix off-by-one in scanAndRegister where
maxDepth was exceeded by one level. Export normalizePath to eliminate
duplicate logic in list.ts, use a Set for faster dedup in scan, and
simplify the styles map to a plain object literal.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
Chris Wanstrath 2026-03-20 22:12:40 -07:00
parent 965f233245
commit da7adc674d
2 changed files with 28 additions and 28 deletions

View File

@ -1,4 +1,4 @@
import { basename, resolve } from "path" import { basename } from "path"
import { homedir } from "os" import { homedir } from "os"
import { stat } from "fs/promises" import { stat } from "fs/promises"
import * as git from "../git.ts" import * as git from "../git.ts"
@ -8,13 +8,13 @@ import { die, reset, dim, green, yellow, cyan, magenta, red } from "../fmt.ts"
// ── Shared rendering ───────────────────────────────────────────────── // ── Shared rendering ─────────────────────────────────────────────────
const styleDefs: [string, string, string][] = [ const styles: Record<string, { icon: string; color: string }> = {
["idle", dim, "◯"], ["active", cyan, "◎"], ["dirty", yellow, "◐"], idle: { icon: `${dim}${reset}`, color: dim },
["saved", green, "●"], ["review", magenta, "⦿"], active: { icon: `${cyan}${reset}`, color: cyan },
] dirty: { icon: `${yellow}${reset}`, color: yellow },
const styles = Object.fromEntries( saved: { icon: `${green}${reset}`, color: green },
styleDefs.map(([k, c, ch]) => [k, { icon: `${c}${ch}${reset}`, color: c }]) review: { icon: `${magenta}⦿${reset}`, color: magenta },
) }
function renderSessions( function renderSessions(
sessions: state.GlobalSession[], sessions: state.GlobalSession[],
@ -69,7 +69,6 @@ async function clearStaleReviews(
) { ) {
const stale = sessions.filter((s, i) => s.in_review && results[i] !== "review") const stale = sessions.filter((s, i) => s.in_review && results[i] !== "review")
if (stale.length === 0) return if (stale.length === 0) return
for (const s of stale) s.in_review = false
const byRepo = Map.groupBy(stale, s => s.repoRoot) const byRepo = Map.groupBy(stale, s => s.repoRoot)
for (const [repoRoot, staleSessions] of byRepo) { for (const [repoRoot, staleSessions] of byRepo) {
const fresh = await state.load(repoRoot) const fresh = await state.load(repoRoot)
@ -169,7 +168,7 @@ async function actionAdd(dir: string) {
} }
async function actionRemove(dir: string) { async function actionRemove(dir: string) {
const resolved = resolve(dir.replace(/^~(?=\/|$)/, homedir())) const resolved = state.normalizePath(dir)
let removed = false let removed = false
try { try {
removed = await state.unregisterProject(dir) removed = await state.unregisterProject(dir)

View File

@ -66,32 +66,34 @@ const GLOBAL_STATE_PATH = join(GLOBAL_DIR, "registry.json")
async function withGlobalLock<T>(fn: () => Promise<T>): Promise<T> { async function withGlobalLock<T>(fn: () => Promise<T>): Promise<T> {
const lockPath = join(GLOBAL_DIR, "registry.lock") const lockPath = join(GLOBAL_DIR, "registry.lock")
await mkdir(GLOBAL_DIR, { recursive: true }) await mkdir(GLOBAL_DIR, { recursive: true })
let acquired = false
for (let i = 0; i < 20; i++) { for (let i = 0; i < 20; i++) {
try { try {
await mkdir(lockPath) await mkdir(lockPath)
acquired = true
break
} catch { } catch {
// If the lock is older than 5 minutes, assume it's stale (crashed process) // If the lock is older than 5 minutes, assume it's stale (crashed process)
try { try {
const info = await stat(lockPath) const info = await stat(lockPath)
if (Date.now() - info.mtimeMs > 300_000) { if (Date.now() - info.mtimeMs > 300_000) {
await rmdir(lockPath).catch(() => {}) await rmdir(lockPath).catch(() => {})
// Retry mkdir immediately to close the TOCTOU window
try { await mkdir(lockPath) } catch { continue }
} else {
if (i === 19) throw new Error("Could not acquire registry lock")
await Bun.sleep(50) await Bun.sleep(50)
continue continue
} }
} catch {} } catch {
if (i === 19) throw new Error("Could not acquire registry lock") // Lock dir vanished between our mkdir and stat — retry immediately
await Bun.sleep(50) continue
}
}
try {
return await fn()
} finally {
await rmdir(lockPath).catch(() => {})
} }
} }
if (!acquired) throw new Error("Could not acquire registry lock") throw new Error("Could not acquire registry lock")
try {
return await fn()
} finally {
await rmdir(lockPath).catch(() => {})
}
} }
async function loadGlobal(): Promise<GlobalState> { async function loadGlobal(): Promise<GlobalState> {
@ -99,9 +101,7 @@ async function loadGlobal(): Promise<GlobalState> {
if (await file.exists()) { if (await file.exists()) {
try { try {
const data = await file.json() const data = await file.json()
if (data && Array.isArray(data.projects)) { if (data && Array.isArray(data.projects)) return data
return { projects: data.projects as string[] }
}
} catch {} } catch {}
} }
return { projects: [] } return { projects: [] }
@ -113,7 +113,7 @@ async function saveGlobal(gs: GlobalState): Promise<void> {
await rename(tmpPath, GLOBAL_STATE_PATH) await rename(tmpPath, GLOBAL_STATE_PATH)
} }
function normalizePath(dir: string): string { export function normalizePath(dir: string): string {
return resolve(dir.replace(/^~(?=\/|$)/, homedir())) return resolve(dir.replace(/^~(?=\/|$)/, homedir()))
} }
@ -148,7 +148,7 @@ export async function scanAndRegister(dir: string, maxDepth = 5): Promise<string
const found: string[] = [] const found: string[] = []
async function walk(d: string, depth: number) { async function walk(d: string, depth: number) {
if (depth > maxDepth) return if (depth >= maxDepth) return
let entries let entries
try { try {
entries = await readdir(d, { withFileTypes: true }) entries = await readdir(d, { withFileTypes: true })
@ -173,9 +173,10 @@ export async function scanAndRegister(dir: string, maxDepth = 5): Promise<string
if (found.length > 0) { if (found.length > 0) {
await withGlobalLock(async () => { await withGlobalLock(async () => {
const gs = await loadGlobal() const gs = await loadGlobal()
const existing = new Set(gs.projects)
let changed = false let changed = false
for (const p of found) { for (const p of found) {
if (!gs.projects.includes(p)) { if (!existing.has(p)) {
gs.projects.push(p) gs.projects.push(p)
changed = true changed = true
} }