diff --git a/src/resources/extensions/gsd/auto-supervisor.ts b/src/resources/extensions/gsd/auto-supervisor.ts index 4e794e0de..f5c1a6336 100644 --- a/src/resources/extensions/gsd/auto-supervisor.ts +++ b/src/resources/extensions/gsd/auto-supervisor.ts @@ -1,5 +1,5 @@ /** - * Auto-mode Supervisor — SIGTERM handling and working-tree activity detection. + * Auto-mode Supervisor — signal handling and working-tree activity detection. * * Pure functions — no module-level globals or AutoContext dependency. */ @@ -8,10 +8,10 @@ import { clearLock } from "./crash-recovery.js"; import { releaseSessionLock } from "./session-lock.js"; import { nativeHasChanges } from "./native-git-bridge.js"; -// ─── SIGTERM Handling ───────────────────────────────────────────────────────── +// ─── Signal Handling ────────────────────────────────────────────────────────── /** - * Register a SIGTERM handler that clears the lock file and exits cleanly. + * Register SIGTERM and SIGINT handlers that clear lock files and exit cleanly. * Captures the active base path at registration time so the handler * always references the correct path even if the module variable changes. * Removes any previously registered handler before installing the new one. @@ -22,20 +22,25 @@ export function registerSigtermHandler( currentBasePath: string, previousHandler: (() => void) | null, ): () => void { - if (previousHandler) process.off("SIGTERM", previousHandler); + if (previousHandler) { + process.off("SIGTERM", previousHandler); + process.off("SIGINT", previousHandler); + } const handler = () => { releaseSessionLock(currentBasePath); clearLock(currentBasePath); process.exit(0); }; process.on("SIGTERM", handler); + process.on("SIGINT", handler); return handler; } -/** Deregister the SIGTERM handler (called on stop/pause). */ +/** Deregister signal handlers (called on stop/pause). */ export function deregisterSigtermHandler(handler: (() => void) | null): void { if (handler) { process.off("SIGTERM", handler); + process.off("SIGINT", handler); } } diff --git a/src/resources/extensions/gsd/session-lock.ts b/src/resources/extensions/gsd/session-lock.ts index da7086d4e..1541d9172 100644 --- a/src/resources/extensions/gsd/session-lock.ts +++ b/src/resources/extensions/gsd/session-lock.ts @@ -17,7 +17,7 @@ */ import { createRequire } from "node:module"; -import { existsSync, readFileSync, mkdirSync, unlinkSync, rmSync, statSync } from "node:fs"; +import { existsSync, readFileSync, readdirSync, mkdirSync, unlinkSync, rmSync, statSync } from "node:fs"; import { join, dirname } from "node:path"; import { gsdRoot } from "./paths.js"; import { atomicWriteSync } from "./atomic-write.js"; @@ -54,12 +54,81 @@ let _lockPid: number = 0; /** Set to true when proper-lockfile fires onCompromised (mtime drift, sleep, etc.). */ let _lockCompromised: boolean = false; +/** Whether we've already registered a process.on('exit') handler. */ +let _exitHandlerRegistered: boolean = false; + const LOCK_FILE = "auto.lock"; function lockPath(basePath: string): string { return join(gsdRoot(basePath), LOCK_FILE); } +// ─── Stray Lock Cleanup ───────────────────────────────────────────────────── + +/** + * Remove numbered lock file variants (e.g. "auto 2.lock", "auto 3.lock") + * that accumulate from macOS file conflict resolution (iCloud/Dropbox/OneDrive) + * or other filesystem-level copy-on-conflict behavior (#1315). + * + * Also removes stray proper-lockfile directories beyond the canonical `.gsd.lock/`. + */ +export function cleanupStrayLockFiles(basePath: string): void { + const gsdDir = gsdRoot(basePath); + + // Clean numbered auto lock files inside .gsd/ + try { + if (existsSync(gsdDir)) { + for (const entry of readdirSync(gsdDir)) { + // Match "auto .lock" or "auto ().lock" variants but NOT the canonical "auto.lock" + if (entry !== LOCK_FILE && /^auto\s.+\.lock$/i.test(entry)) { + try { unlinkSync(join(gsdDir, entry)); } catch { /* best-effort */ } + } + } + } + } catch { /* non-fatal: directory read failure */ } + + // Clean stray proper-lockfile directories (e.g. ".gsd 2.lock/") + // The canonical one is ".gsd.lock/" — anything else is stray. + try { + const parentDir = dirname(gsdDir); + const gsdDirName = gsdDir.split("/").pop() || ".gsd"; + if (existsSync(parentDir)) { + for (const entry of readdirSync(parentDir)) { + // Match ".gsd .lock" or ".gsd ().lock" directories but NOT ".gsd.lock" + if (entry !== `${gsdDirName}.lock` && entry.startsWith(gsdDirName) && entry.endsWith(".lock")) { + const fullPath = join(parentDir, entry); + try { + const stat = statSync(fullPath); + if (stat.isDirectory()) { + rmSync(fullPath, { recursive: true, force: true }); + } + } catch { /* best-effort */ } + } + } + } + } catch { /* non-fatal */ } +} + +/** + * Register a single process exit handler that cleans up lock state. + * Uses module-level references so it always operates on current state. + * Only registers once — subsequent calls are no-ops. + */ +function ensureExitHandler(gsdDir: string): void { + if (_exitHandlerRegistered) return; + _exitHandlerRegistered = true; + + process.once("exit", () => { + try { + if (_releaseFunction) { _releaseFunction(); _releaseFunction = null; } + } catch { /* best-effort */ } + try { + const lockDir = join(gsdDir + ".lock"); + if (existsSync(lockDir)) rmSync(lockDir, { recursive: true, force: true }); + } catch { /* best-effort */ } + }); +} + // ─── Public API ───────────────────────────────────────────────────────────── /** @@ -77,6 +146,9 @@ export function acquireSessionLock(basePath: string): SessionLockResult { // Ensure the directory exists mkdirSync(dirname(lp), { recursive: true }); + // Clean up numbered lock file variants from cloud sync conflicts (#1315) + cleanupStrayLockFiles(basePath); + // Write our lock data first (the content is informational; the OS lock is the real guard) const lockData: SessionLockData = { pid: process.pid, @@ -124,15 +196,7 @@ export function acquireSessionLock(basePath: string): SessionLockResult { // Safety net: clean up lock dir on process exit if _releaseFunction // wasn't called (e.g., normal exit after clean completion) (#1245). - const lockDirForCleanup = join(gsdDir + ".lock"); - process.once("exit", () => { - try { - if (_releaseFunction) { _releaseFunction(); _releaseFunction = null; } - } catch { /* best-effort */ } - try { - if (existsSync(lockDirForCleanup)) rmSync(lockDirForCleanup, { recursive: true, force: true }); - } catch { /* best-effort */ } - }); + ensureExitHandler(gsdDir); // Write the informational lock data atomicWriteSync(lp, JSON.stringify(lockData, null, 2)); @@ -158,18 +222,15 @@ export function acquireSessionLock(basePath: string): SessionLockResult { update: 10_000, onCompromised: () => { _lockCompromised = true; + _releaseFunction = null; }, }); _releaseFunction = release; _lockedPath = basePath; _lockPid = process.pid; - // Safety net for retry path too - const retryLockDir = join(gsdDir + ".lock"); - process.once("exit", () => { - try { if (_releaseFunction) { _releaseFunction(); _releaseFunction = null; } } catch {} - try { if (existsSync(retryLockDir)) rmSync(retryLockDir, { recursive: true, force: true }); } catch {} - }); + // Safety net — uses centralized handler to avoid double-registration + ensureExitHandler(gsdDir); atomicWriteSync(lp, JSON.stringify(lockData, null, 2)); return { acquired: true }; @@ -310,6 +371,9 @@ export function releaseSessionLock(basePath: string): void { // Non-fatal } + // Clean up numbered lock file variants from cloud sync conflicts (#1315) + cleanupStrayLockFiles(basePath); + _lockedPath = null; _lockPid = 0; _lockCompromised = false; diff --git a/src/resources/extensions/gsd/tests/loop-regression.test.ts b/src/resources/extensions/gsd/tests/loop-regression.test.ts index 92d90db49..3fbf223d4 100644 --- a/src/resources/extensions/gsd/tests/loop-regression.test.ts +++ b/src/resources/extensions/gsd/tests/loop-regression.test.ts @@ -609,10 +609,48 @@ test("session lock: onCompromised handler exists in both primary and retry paths const compromisedMatches = [...lockSource.matchAll(/onCompromised/g)]; // Should have at least 2 onCompromised handlers (primary + retry) // plus the flag declaration and the check in validateSessionLock - assert.ok(compromisedMatches.length >= 3, + assert.ok(compromisedMatches.length >= 3, `expected ≥3 onCompromised references (primary + retry + flag), got ${compromisedMatches.length}`); }); +test("session lock: both onCompromised handlers null _releaseFunction (#1315)", async () => { + const lockSource = readFileSync( + "src/resources/extensions/gsd/session-lock.ts", "utf-8" + ); + // Extract onCompromised handler blocks — both should set _releaseFunction = null + const handlers = lockSource.match(/onCompromised:\s*\(\)\s*=>\s*\{[^}]+\}/g) || []; + assert.ok(handlers.length >= 2, `expected ≥2 onCompromised handlers, got ${handlers.length}`); + for (const h of handlers) { + assert.ok(h.includes("_releaseFunction = null"), + `onCompromised handler should null _releaseFunction: ${h}`); + } +}); + +test("session lock: exit handler uses ensureExitHandler to prevent double-registration (#1315)", async () => { + const lockSource = readFileSync( + "src/resources/extensions/gsd/session-lock.ts", "utf-8" + ); + // Should use ensureExitHandler instead of direct process.once("exit") in acquire paths + const directExitHandlers = (lockSource.match(/process\.once\("exit"/g) || []).length; + const ensureExitCalls = (lockSource.match(/ensureExitHandler\(/g) || []).length; + // Only 1 direct process.once("exit") allowed — inside ensureExitHandler itself + assert.ok(directExitHandlers <= 1, + `expected ≤1 direct process.once("exit") (inside ensureExitHandler), got ${directExitHandlers}`); + assert.ok(ensureExitCalls >= 2, + `expected ≥2 ensureExitHandler calls (primary + retry path), got ${ensureExitCalls}`); +}); + +test("signal handler: SIGINT handler registered alongside SIGTERM (#1315)", async () => { + const supervisorSource = readFileSync( + "src/resources/extensions/gsd/auto-supervisor.ts", "utf-8" + ); + // registerSigtermHandler should register on both SIGTERM and SIGINT + assert.ok(supervisorSource.includes('process.on("SIGINT"') || supervisorSource.includes("process.on('SIGINT'"), + "registerSigtermHandler should register SIGINT handler"); + assert.ok(supervisorSource.includes('process.off("SIGINT"') || supervisorSource.includes("process.off('SIGINT'"), + "deregisterSigtermHandler should deregister SIGINT handler"); +}); + // ─── Scope 5: Crash Recovery — Message Guidance per Unit Type ──────────── test("crash recovery: formatCrashInfo includes guidance for bootstrap crash", async () => { diff --git a/src/resources/extensions/gsd/tests/session-lock.test.ts b/src/resources/extensions/gsd/tests/session-lock.test.ts index 06fe7cc3b..882726caf 100644 --- a/src/resources/extensions/gsd/tests/session-lock.test.ts +++ b/src/resources/extensions/gsd/tests/session-lock.test.ts @@ -12,6 +12,7 @@ import { readSessionLockData, isSessionLockHeld, isSessionLockProcessAlive, + cleanupStrayLockFiles, } from "../session-lock.ts"; // ─── acquireSessionLock ────────────────────────────────────────────────── @@ -313,3 +314,121 @@ test("acquireSessionLock creates .gsd/ if it does not exist", () => { releaseSessionLock(dir); rmSync(dir, { recursive: true, force: true }); }); + +// ─── cleanupStrayLockFiles (#1315) ────────────────────────────────────── + +test("cleanupStrayLockFiles removes numbered lock variants but preserves auto.lock", () => { + const dir = mkdtempSync(join(tmpdir(), "gsd-session-lock-")); + const gsdDir = join(dir, ".gsd"); + mkdirSync(gsdDir, { recursive: true }); + + // Create canonical lock file + numbered variants + writeFileSync(join(gsdDir, "auto.lock"), '{"pid":1}'); + writeFileSync(join(gsdDir, "auto 2.lock"), '{"pid":2}'); + writeFileSync(join(gsdDir, "auto 3.lock"), '{"pid":3}'); + writeFileSync(join(gsdDir, "auto 4.lock"), '{"pid":4}'); + + cleanupStrayLockFiles(dir); + + assert.ok(existsSync(join(gsdDir, "auto.lock")), "canonical auto.lock should be preserved"); + assert.ok(!existsSync(join(gsdDir, "auto 2.lock")), "auto 2.lock should be removed"); + assert.ok(!existsSync(join(gsdDir, "auto 3.lock")), "auto 3.lock should be removed"); + assert.ok(!existsSync(join(gsdDir, "auto 4.lock")), "auto 4.lock should be removed"); + + rmSync(dir, { recursive: true, force: true }); +}); + +test("cleanupStrayLockFiles handles parenthesized variants", () => { + const dir = mkdtempSync(join(tmpdir(), "gsd-session-lock-")); + const gsdDir = join(dir, ".gsd"); + mkdirSync(gsdDir, { recursive: true }); + + // macOS sometimes uses parenthesized format: "auto (2).lock" + writeFileSync(join(gsdDir, "auto.lock"), '{"pid":1}'); + writeFileSync(join(gsdDir, "auto (2).lock"), '{"pid":2}'); + + cleanupStrayLockFiles(dir); + + assert.ok(existsSync(join(gsdDir, "auto.lock")), "canonical auto.lock should be preserved"); + assert.ok(!existsSync(join(gsdDir, "auto (2).lock")), "auto (2).lock should be removed"); + + rmSync(dir, { recursive: true, force: true }); +}); + +test("cleanupStrayLockFiles does not remove unrelated files", () => { + const dir = mkdtempSync(join(tmpdir(), "gsd-session-lock-")); + const gsdDir = join(dir, ".gsd"); + mkdirSync(gsdDir, { recursive: true }); + + // Create unrelated files that should NOT be removed + writeFileSync(join(gsdDir, "auto.lock"), '{"pid":1}'); + writeFileSync(join(gsdDir, "config.json"), '{}'); + writeFileSync(join(gsdDir, "other.lock"), '{}'); + + cleanupStrayLockFiles(dir); + + assert.ok(existsSync(join(gsdDir, "auto.lock")), "auto.lock should be preserved"); + assert.ok(existsSync(join(gsdDir, "config.json")), "config.json should be preserved"); + assert.ok(existsSync(join(gsdDir, "other.lock")), "other.lock should be preserved"); + + rmSync(dir, { recursive: true, force: true }); +}); + +test("cleanupStrayLockFiles is safe on empty directory", () => { + const dir = mkdtempSync(join(tmpdir(), "gsd-session-lock-")); + const gsdDir = join(dir, ".gsd"); + mkdirSync(gsdDir, { recursive: true }); + + // Should not throw + cleanupStrayLockFiles(dir); + + rmSync(dir, { recursive: true, force: true }); +}); + +test("cleanupStrayLockFiles is safe when .gsd/ does not exist", () => { + const dir = mkdtempSync(join(tmpdir(), "gsd-session-lock-")); + + // Should not throw even without .gsd/ + cleanupStrayLockFiles(dir); + + rmSync(dir, { recursive: true, force: true }); +}); + +test("acquireSessionLock cleans stray lock files before acquiring", () => { + const dir = mkdtempSync(join(tmpdir(), "gsd-session-lock-")); + const gsdDir = join(dir, ".gsd"); + mkdirSync(gsdDir, { recursive: true }); + + // Plant stray lock files before acquire + writeFileSync(join(gsdDir, "auto 2.lock"), '{"pid":9999999}'); + writeFileSync(join(gsdDir, "auto 3.lock"), '{"pid":9999998}'); + + const result = acquireSessionLock(dir); + assert.equal(result.acquired, true, "should acquire lock"); + + // Stray files should be cleaned up + assert.ok(!existsSync(join(gsdDir, "auto 2.lock")), "auto 2.lock should be removed during acquire"); + assert.ok(!existsSync(join(gsdDir, "auto 3.lock")), "auto 3.lock should be removed during acquire"); + + releaseSessionLock(dir); + rmSync(dir, { recursive: true, force: true }); +}); + +test("releaseSessionLock cleans stray lock files after releasing", () => { + const dir = mkdtempSync(join(tmpdir(), "gsd-session-lock-")); + const gsdDir = join(dir, ".gsd"); + mkdirSync(gsdDir, { recursive: true }); + + const result = acquireSessionLock(dir); + assert.equal(result.acquired, true); + + // Plant stray lock files (simulating cloud sync creating them during session) + writeFileSync(join(gsdDir, "auto 2.lock"), '{"pid":9999999}'); + + releaseSessionLock(dir); + + assert.ok(!existsSync(join(gsdDir, "auto 2.lock")), "auto 2.lock should be removed during release"); + assert.ok(!existsSync(join(gsdDir, "auto.lock")), "auto.lock should also be removed"); + + rmSync(dir, { recursive: true, force: true }); +});