fix: clean up stale numbered lock files and harden signal/exit handling (#1315) (#1323)

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);
   }
 }
 

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 <N>.lock" or "auto (<N>).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 <N>.lock" or ".gsd (<N>).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;

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 () => {

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 });
+});