fix: preserve filesystem lock ownership during recovery

CHANGELOG.md

@@ -16,6 +16,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 
+- Prepare file and directory locks with unique owner records before publishing
+  them. Recover only the observed dead owner so delayed stale-lock recovery
+  cannot delete a successor's lock and allow overlapping updates. Preserve
+  ambiguous lock contents and junctions; report failed private-stage cleanup.
+
 - Publish generated OpenAPI clients through the same recoverable directory
   swap as other commands. Keep complete published output when backup cleanup
   fails, clean partial stages, and report retained stages after cleanup failure.

docs/0.5.0-api.md

@@ -48,6 +48,12 @@ The OpenAPI generator now uses that same publication routine and preserves its d
 
 Create reports success after publication completes. Child-process tests inject package-manager nonzero exit, missing-command and timeout results, then assert an error exit with no published destination or abandoned project stage. A successful install followed by failed publication also reports no success. These are subprocess result injections, not qualification of a real installer timeout or cancellation bound.
 
+## Filesystem lock failure qualification
+
+File transactions and directory publication now share one lock protocol: prepare a unique owner record in a sibling directory, publish the nonempty lock with a rename, then unlink only the observed dead owner or this operation's own owner and remove the directory only if empty. A delayed stale reaper cannot unlink a successor's different owner record. This replaces the two duplicated recursive-lock-removal implementations; the ten-second wait bound is unchanged. Existing dead `owner.json` records and aged empty/malformed legacy locks can be recovered. Ambiguous contents, symbolic links and owner-record junctions stop recovery and preserve unrelated paths.
+
+The executed matrix covers two simultaneous stale-owner observations for file and directory operations, exact stale-write rejection, unrelated lock contents, junctions, fresh malformed-record timeout, aged legacy recovery, partial owner writes, failed preparation cleanup and an exclusive preparation collision. It also verifies successor ownership during release, denied process probes and filesystem errors during acquisition. The original six minimal cases failed before the fix. Separate child-process cases terminate after preparing an owner but before publishing the lock: data is unchanged, no incomplete live lock is exposed, retry succeeds and the abandoned private preparation remains available for manual cleanup. Existing file/directory termination and recovery cases also run with the shared protocol. These results qualify the tested process checkpoints and cooperating current CLI commands, not power-loss durability or concurrent use of older lock protocols.
+
 ## Release boundaries
 
-The normal packed consumer checks both retained declaration names, nine rejected old imports and private paths under strict TypeScript 6 and 7, then runs the installed CLI help/version commands. No production dependency, package version or peer range changes in this cut. Full candidate graph, generated website API and maintainer review still gate release. CLI hardening issue #159 stays open until its remaining filesystem-failure qualification is complete.
+The normal packed consumer checks both retained declaration names, nine rejected old imports and private paths under strict TypeScript 6 and 7, then runs the installed CLI help/version commands. No production dependency, package version or peer range changes in this cut. Full candidate graph, generated website API and maintainer review still gate release. CLI hardening issue #159 stays open until its remaining qualification is complete.

src/directory-swap.ts

@@ -2,6 +2,7 @@ import { randomUUID } from "node:crypto";
 import { constants, type BigIntStats } from "node:fs";
 import fs from "node:fs/promises";
 import path from "node:path";
+import { acquireFilesystemLock } from "./filesystem-lock";
 
 async function directoryStat(target: string): Promise<BigIntStats | null> {
   let stat: BigIntStats;
@@ -134,70 +135,21 @@ async function recoverPublication(target: string): Promise<void> {
   }
 }
 
-const LOCK_RETRY_MS = 10;
-const LOCK_TIMEOUT_MS = 10_000;
-const ORPHANED_LOCK_AGE_MS = 30_000;
-
 function isNodeError(error: unknown, code: string): error is NodeJS.ErrnoException {
   return error instanceof Error && "code" in error && error.code === code;
 }
 
-async function removeOrphanedLock(lock: string): Promise<boolean> {
-  try {
-    const owner = JSON.parse(await fs.readFile(path.join(lock, "owner.json"), "utf8")) as {
-      pid?: unknown;
-    };
-    if (Number.isInteger(owner.pid) && (owner.pid as number) > 0) {
-      try {
-        process.kill(owner.pid as number, 0);
-        return false;
-      } catch (error) {
-        if (!isNodeError(error, "ESRCH") && !isNodeError(error, "EINVAL")) return false;
-      }
-    } else {
-      return false;
-    }
-  } catch {
-    const stat = await fs.stat(lock).catch(() => null);
-    if (!stat || Date.now() - stat.mtimeMs < ORPHANED_LOCK_AGE_MS) return false;
-  }
-  await fs.rm(lock, { recursive: true, force: true });
-  return true;
-}
-
 export async function withDirectoryTargetLock<T>(
   target: string,
   operation: () => Promise<T>,
 ): Promise<T> {
   const lock = `${path.resolve(target)}.askr-lock`;
-  const deadline = Date.now() + LOCK_TIMEOUT_MS;
-  while (true) {
-    try {
-      await fs.mkdir(lock);
-      await fs.writeFile(
-        path.join(lock, "owner.json"),
-        `${JSON.stringify({ pid: process.pid })}\n`,
-        {
-          flag: "wx",
-        },
-      );
-      break;
-    } catch (error) {
-      if (!isNodeError(error, "EEXIST")) {
-        await fs.rm(lock, { recursive: true, force: true }).catch(() => undefined);
-        throw error;
-      }
-      if (await removeOrphanedLock(lock)) continue;
-      if (Date.now() >= deadline)
-        throw new Error(`Timed out waiting for directory lock: ${target}`);
-      await new Promise((resolve) => setTimeout(resolve, LOCK_RETRY_MS));
-    }
-  }
+  const release = await acquireFilesystemLock(lock, `directory lock for ${JSON.stringify(target)}`);
   try {
     await recoverPublication(path.resolve(target));
     return await operation();
   } finally {
-    await fs.rm(lock, { recursive: true, force: true });
+    await release();
   }
 }
 

src/file-changes.ts

@@ -1,6 +1,7 @@
 import fs from "node:fs/promises";
 import path from "node:path";
 import { randomUUID } from "node:crypto";
+import { acquireFilesystemLock } from "./filesystem-lock";
 
 export interface FileChange {
   readonly filePath: string;
@@ -19,80 +20,25 @@ export interface FileChangeWriterOptions {
 }
 
 interface FileLock {
-  readonly lockPath: string;
+  readonly release: () => Promise<void>;
 }
 
-const LOCK_RETRY_MS = 10;
-const LOCK_TIMEOUT_MS = 10_000;
-const ORPHANED_LOCK_AGE_MS = 30_000;
-
 function isNodeError(error: unknown, code: string): error is NodeJS.ErrnoException {
   return error instanceof Error && "code" in error && error.code === code;
 }
 
-function delay(milliseconds: number): Promise<void> {
-  return new Promise((resolve) => setTimeout(resolve, milliseconds));
-}
-
-async function ownerIsAlive(lockPath: string): Promise<boolean | undefined> {
-  try {
-    const owner = JSON.parse(await fs.readFile(path.join(lockPath, "owner.json"), "utf8")) as {
-      pid?: unknown;
-    };
-    if (!Number.isInteger(owner.pid) || (owner.pid as number) <= 0) return undefined;
-    try {
-      process.kill(owner.pid as number, 0);
-      return true;
-    } catch (error) {
-      if (isNodeError(error, "ESRCH")) return false;
-      return true;
-    }
-  } catch {
-    return undefined;
-  }
-}
-
-async function removeOrphanedLock(lockPath: string): Promise<boolean> {
-  const ownerAlive = await ownerIsAlive(lockPath);
-  if (ownerAlive === true) return false;
-  if (ownerAlive === undefined) {
-    const stat = await fs.stat(lockPath).catch(() => null);
-    if (!stat || Date.now() - stat.mtimeMs < ORPHANED_LOCK_AGE_MS) return false;
-  }
-  await fs.rm(lockPath, { recursive: true, force: true });
-  return true;
-}
-
 async function acquireFileLock(filePath: string): Promise<FileLock> {
   const lockPath = path.join(path.dirname(filePath), `.${path.basename(filePath)}.askr-lock`);
-  const deadline = Date.now() + LOCK_TIMEOUT_MS;
-  while (true) {
-    try {
-      await fs.mkdir(lockPath);
-      await fs.writeFile(
-        path.join(lockPath, "owner.json"),
-        `${JSON.stringify({ pid: process.pid })}\n`,
-        { flag: "wx" },
-      );
-      return { lockPath };
-    } catch (error) {
-      if (!isNodeError(error, "EEXIST")) {
-        await fs.rm(lockPath, { recursive: true, force: true }).catch(() => undefined);
-        throw error;
-      }
-      if (await removeOrphanedLock(lockPath)) continue;
-      if (Date.now() >= deadline) {
-        throw new Error(`Timed out waiting for file transaction lock: ${filePath}`);
-      }
-      await delay(LOCK_RETRY_MS);
-    }
-  }
+  return {
+    release: await acquireFilesystemLock(
+      lockPath,
+      `file transaction lock for ${JSON.stringify(filePath)}`,
+    ),
+  };
 }
 
 async function releaseFileLocks(locks: readonly FileLock[]): Promise<void> {
-  await Promise.all(
-    [...locks].reverse().map((lock) => fs.rm(lock.lockPath, { recursive: true, force: true })),
-  );
+  await Promise.all([...locks].reverse().map((lock) => lock.release()));
 }
 
 async function readCurrentContent(filePath: string): Promise<string | null> {

src/filesystem-lock.ts

@@ -0,0 +1,147 @@
+import fs from "node:fs/promises";
+import path from "node:path";
+import { randomUUID } from "node:crypto";
+
+const LOCK_RETRY_MS = 10;
+const LOCK_TIMEOUT_MS = 10_000;
+const ORPHANED_LOCK_AGE_MS = 30_000;
+const OWNER_FILE = /^owner-[\da-f]{8}-[\da-f]{4}-[\da-f]{4}-[\da-f]{4}-[\da-f]{12}\.json$/i;
+const RENAME_CONTENTION_CODES = new Set(["EEXIST", "ENOTEMPTY", "EPERM", "EBUSY", "EACCES"]);
+
+function isNodeError(error: unknown, code: string): error is NodeJS.ErrnoException {
+  return error instanceof Error && "code" in error && error.code === code;
+}
+
+async function statIfPresent(target: string) {
+  try {
+    return await fs.lstat(target);
+  } catch (error) {
+    if (isNodeError(error, "ENOENT")) return null;
+    throw error;
+  }
+}
+
+function ambiguousLock(lock: string): Error {
+  return new Error(
+    `Cannot recover filesystem lock ${JSON.stringify(lock)}: its contents do not identify one regular owner record. Inspect the lock and preserve unrelated files before retrying.`,
+  );
+}
+
+async function removeEmptyLock(lock: string): Promise<boolean> {
+  try {
+    await fs.rmdir(lock);
+    return true;
+  } catch (error) {
+    if (isNodeError(error, "ENOENT")) return true;
+    if (isNodeError(error, "ENOTEMPTY") || isNodeError(error, "EEXIST")) return false;
+    throw error;
+  }
+}
+
+async function removeOrphanedLock(lock: string): Promise<boolean> {
+  const stat = await statIfPresent(lock);
+  if (!stat) return false;
+  if (!stat.isDirectory() || stat.isSymbolicLink()) throw ambiguousLock(lock);
+  let entries: string[];
+  try {
+    entries = await fs.readdir(lock);
+  } catch (error) {
+    if (isNodeError(error, "ENOENT")) return false;
+    throw error;
+  }
+  if (entries.length === 0) {
+    if (Date.now() - stat.mtimeMs < ORPHANED_LOCK_AGE_MS) return false;
+    return removeEmptyLock(lock);
+  }
+  if (entries.length !== 1 || (entries[0] !== "owner.json" && !OWNER_FILE.test(entries[0]))) {
+    throw ambiguousLock(lock);
+  }
+  const ownerPath = path.join(lock, entries[0]);
+  const ownerStat = await statIfPresent(ownerPath);
+  if (!ownerStat) return false;
+  if (!ownerStat.isFile() || ownerStat.isSymbolicLink()) throw ambiguousLock(lock);
+  let pid: unknown;
+  try {
+    const owner = JSON.parse(await fs.readFile(ownerPath, "utf8")) as { pid?: unknown } | null;
+    pid = owner?.pid;
+  } catch (error) {
+    if (isNodeError(error, "ENOENT")) return false;
+    if (!(error instanceof SyntaxError)) throw error;
+  }
+  if (Number.isInteger(pid) && (pid as number) > 0 && (pid as number) <= 2_147_483_647) {
+    try {
+      process.kill(pid as number, 0);
+      return false;
+    } catch (error) {
+      if (!isNodeError(error, "ESRCH") && !isNodeError(error, "EINVAL")) return false;
+    }
+  } else if (Date.now() - stat.mtimeMs < ORPHANED_LOCK_AGE_MS) {
+    return false;
+  }
+  // Only one reaper can unlink this observed token. A new acquisition publishes
+  // a different token atomically, so a delayed reaper cannot remove its owner.
+  try {
+    await fs.unlink(ownerPath);
+  } catch (error) {
+    if (isNodeError(error, "ENOENT")) return false;
+    throw error;
+  }
+  return removeEmptyLock(lock);
+}
+
+/** Acquires a cooperative lock and returns a release operation for its unique owner. */
+export async function acquireFilesystemLock(
+  lock: string,
+  description: string,
+): Promise<() => Promise<void>> {
+  const ownerName = `owner-${randomUUID()}.json`;
+  const stage = `${lock}.stage-${randomUUID()}`;
+  // Build a nonempty lock privately before publishing it. Never expose a newly
+  // acquired empty directory that another reaper could mistake for an orphan.
+  await fs.mkdir(stage);
+  try {
+    await fs.writeFile(path.join(stage, ownerName), `${JSON.stringify({ pid: process.pid })}\n`, {
+      flag: "wx",
+    });
+    const deadline = Date.now() + LOCK_TIMEOUT_MS;
+    let lastError: unknown;
+    while (true) {
+      if (Date.now() >= deadline) {
+        throw new Error(
+          `Timed out waiting for ${description} at ${JSON.stringify(lock)}. Inspect its owner before retrying.`,
+          { cause: lastError },
+        );
+      }
+      const lockStat = await statIfPresent(lock);
+      if (lockStat && (!lockStat.isDirectory() || lockStat.isSymbolicLink())) {
+        throw ambiguousLock(lock);
+      }
+      try {
+        await fs.rename(stage, lock);
+        return async () => {
+          await fs.unlink(path.join(lock, ownerName));
+          // Another contender can already have replaced the empty old lock with
+          // its nonempty lock. Never recursively remove a successor's contents.
+          await removeEmptyLock(lock);
+        };
+      } catch (error) {
+        const code = error instanceof Error && "code" in error ? String(error.code) : "";
+        if (!RENAME_CONTENTION_CODES.has(code)) throw error;
+        lastError = error;
+        if (await removeOrphanedLock(lock)) continue;
+        await new Promise((resolve) => setTimeout(resolve, LOCK_RETRY_MS));
+      }
+    }
+  } catch (error) {
+    try {
+      await fs.rm(stage, { recursive: true, force: true });
+    } catch (cleanupFailure) {
+      throw new AggregateError(
+        [error, cleanupFailure],
+        `Filesystem lock preparation failed; retained private lock stage ${JSON.stringify(stage)}. Remove this stage after resolving the filesystem error and retry.`,
+        { cause: error },
+      );
+    }
+    throw error;
+  }
+}

tests/cli-smoke.test.ts

@@ -2714,7 +2714,6 @@ test("should ensure skill sync never swaps the live tree while another sync is c
   });
   const originalCp = fs.cp.bind(fs);
   const originalRename = fs.rename.bind(fs);
-  const originalMkdir = fs.mkdir.bind(fs);
   const cp = vi.spyOn(fs, "cp").mockImplementation((async (...args: Parameters<typeof fs.cp>) => {
     if (path.resolve(String(args[0])) !== skillsRoot) return originalCp(...args);
     copiesInFlight += 1;
@@ -2738,19 +2737,14 @@ test("should ensure skill sync never swaps the live tree while another sync is c
         code: "EPERM",
       });
     }
-    return originalRename(...args);
-  }) as typeof fs.rename);
-  const mkdir = vi.spyOn(fs, "mkdir").mockImplementation((async (
-    ...args: Parameters<typeof fs.mkdir>
-  ) => {
     try {
-      return await originalMkdir(...args);
+      return await originalRename(...args);
     } catch (error) {
       // A second sync waiting on the lock lets the first one finish its copy.
-      if (path.resolve(String(args[0])) === lockPath) releaseGate();
+      if (path.resolve(String(args[1])) === lockPath) releaseGate();
       throw error;
     }
-  }) as typeof fs.mkdir);
+  }) as typeof fs.rename);
 
   try {
     const results = await Promise.all([
@@ -2766,7 +2760,6 @@ test("should ensure skill sync never swaps the live tree while another sync is c
   } finally {
     cp.mockRestore();
     rename.mockRestore();
-    mkdir.mockRestore();
     await fs.rm(tempRoot, { recursive: true, force: true });
   }
 });