One shared "claim that can go stale" for the install lock and mutation runs

TODO.md

@@ -1,5 +1,23 @@
 # TODO — remaining follow-ups
 
+## Let --kill stop a run through its supervisor, not the child's pid (from PR #2042)
+
+`deno task mutation --kill` signals the child pid stored in the run record
+(`signalRun` in `scripts/mutation/isolation.ts`). PR #2042 shrank the window
+where that pid can be somebody else's — the record drops the pid the moment
+the child's status resolves (`markChildEnded`) — but a kill that reads the
+record in the few milliseconds between the child exiting and that record
+write can still signal a pid the child no longer owns. CodeRabbit suggested
+removing the race outright by making the stop supervisor-mediated: store the
+supervisor's pid in the run record too, have `--kill` signal the supervisor,
+and let the supervisor stop its own child (it holds the child handle, so no
+reused pid can be confused with it). Out of scope for #2042 — it changes the
+record shape and the kill flow rather than the locking this PR unified.
+Starting point: `signalRun` and `markRunning` in
+`scripts/mutation/isolation.ts` / `scripts/mutation/isolation-state.ts`.
+
+---
+
 ## Numbered SQL parameters — adopt the pattern beyond the limiters (from PR #2040)
 
 PR #2040 rewrote the two rate-limiter upserts (`src/shared/db/login-attempts.ts`,
@@ -1565,67 +1583,6 @@ priced bundle reaches checkout today.
 
 ---
 
-## Give the stripe-mock install lock the same shape as every other file lock
-
-*Origin: noticed while unifying the locks behind `scripts/lock-file.ts`.*
-
-Every lock that is a file — the precommit gate, the browser-asset build, the
-stripe-mock start, each mutation run — now goes through `withFileLock`, which
-holds one advisory lock and checks that the lock it holds is still the file at
-its path. (The database migration lock is not one of these: it is a row in a
-table, and is named below for the pattern it shares.)
-
-`scripts/stripe-mock/install.ts` is the exception: it has a second,
-hand-rolled protocol underneath (`createNew` to claim the lock, a timestamp
-written inside it, and `removeStaleInstallLock` to break a lock whose owner
-died), guarded by a `withFileLock` on a separate guard file.
-
-It answers a question the shared lock cannot: "whoever claimed this walked away,
-so take it from them". Two other places answer that same question their own way:
-the mutation runner uses a record with a status, a pid, and a startup grace plus
-`processExists`, and the database migration lock
-(`src/shared/db/migrations/lock.ts`) writes an owner into a settings row and
-lets anyone break a claim older than `MIGRATION_LOCK_TTL_MS`. Three mechanisms
-for one job.
-
-Worth folding into one: either teach `lock-file.ts` about an owner that can go
-away, or let the install reuse the run-record shape. It stayed out of PR #1957
-because it is a whole protocol, not a shared helper, and the install path has
-its own timing tests.
-
-Starting point: `tryAcquireInstallLock`/`removeStaleInstallLock` in
-`scripts/stripe-mock/install.ts`; `acquireMigrationLock` in
-`src/shared/db/migrations/lock.ts` is the best-worked-out version of the pattern
-and the one to copy; `activeByRecord` in `scripts/mutation/isolation-cleanup.ts`
-is the third; and `test/scripts/stripe-mock/install/stale-locks.test.ts` and
-`waiting.test.ts` say what the timing tests expect.
-
-## Tell a clear-up's hold on a run apart from the run's own
-
-*Origin: reviewer suggestion (Codex) on PR #1957.*
-
-`processBelongsToRun` in `scripts/mutation/isolation-cleanup.ts` decides a run
-belongs to the process in its record when that process is alive *and* somebody
-is holding the run's lock. A clear-up deleting that run's folder holds the same
-lock, so during a deletion the two are indistinguishable.
-
-That matters in one case: the record's process id has since been given to some
-unrelated program. `--kill` then reads "alive and locked" as proof and signals a
-process that has nothing to do with us. Deleting a whole checkout copy takes
-long enough for the window to be real.
-
-It needs the lock evidence tied to the run's own child rather than to whoever
-holds the lock — the holder could write who it is, or a clear-up could hold
-something a run never holds. Either is a change to what a lock means here, which
-is why it did not ride along with PR #1957.
-
-Starting point: `processBelongsToRun` and `removeRun` in
-`scripts/mutation/isolation-cleanup.ts`, `signalRun` in
-`scripts/mutation/isolation.ts` for what acts on the answer, and
-`scripts/held-lock-process.ts` for the process that holds a lock on our behalf.
-
----
-
 ## Watch for ports being taken between tests
 
 *Origin: the chunk that took `scripts/stripe-mock/install.ts` to a full
@@ -1689,29 +1646,6 @@ done — proven by a test that forces the late rejection, not by timing luck.
 
 ---
 
-## The gap between a mutation child ending and its supervisor taking the lock
-
-*Raised by Codex on [PR #1976](https://github.com/chobbledotcom/tickets/pull/1976),
-about `scripts/mutation/isolation.ts` and `scripts/mutation/isolation-cleanup.ts`.*
-
-A run's copy is protected by its lock, held by the child while it works and by
-the supervisor afterwards. Between the child ending and the supervisor taking
-the lock, nobody holds it. A mutation command starting in that moment sees a
-record that says "running" with a process that has gone, and — once the run is
-older than the startup grace — may delete the run's folder.
-
-Today that costs a run its copy-back: the read fails, the run is reported as
-failed, and the work has to be run again. It is loud, not silent, and it needs
-a second mutation command to start inside a window of a few milliseconds.
-
-The fix is to stop judging a run's liveness by the child alone. If the record
-also carried the supervisor's process id, a run would count as live for as long
-as the supervisor is up, closing the gap. That means changing what
-`runProcessIsUp` and `activeByRecord` in `isolation-cleanup.ts` consider alive,
-and thinking again about the startup grace, which exists because a process id
-can be given to somebody else after the original has gone. Start at
-`RUN_STARTUP_GRACE_MS` in `isolation-state.ts` and the comment above it.
-
 ## Four feature modules had no test at their mirrored path — now they do
 
 *Origin: `deno task precommit:mutation` on the notes-migration branch, which

scripts/audit-equivalent-mutants.ts

@@ -61,10 +61,14 @@ const runAudit = async (options: AuditOptions): Promise<number> => {
     write: options.write,
   })
     .catch((error) => {
-      if (controller.signal.aborted) Deno.exit(130);
+      // An interrupt must return through the snapshot child's cleanup, not
+      // exit straight past it — an exit here would leave the run's claim
+      // reading as live until it aged out on its own.
+      if (controller.signal.aborted) return null;
       throw error;
     })
     .finally(() => offTerminationSignals(onSignal));
+  if (result === null) return 130;
   console.log(`Checked ${result.checked} equivalent mutants.`);
   console.log(`Killed by lint: ${result.killedByLint.length}`);
   console.log(`Killed by type-check: ${result.killedByTypeCheck.length}`);

scripts/lock-file.ts

@@ -8,9 +8,17 @@
  */
 
 import { dirname } from "@std/path";
-import { holdLockOrNull } from "#scripts/held-lock-process.ts";
 import { nullIfNotFound, statOrNull } from "#scripts/not-found.ts";
 
+/** A task run while a lock or claim is held, giving back its result. */
+export type LockBody<Result> = () => Promise<Result>;
+
+/** Holds the lock living at a path around a task. */
+export type PathLockHolder = <Result>(
+  path: string,
+  body: LockBody<Result>,
+) => Promise<Result>;
+
 /** Open (creating if needed) a file to hold an advisory lock. */
 const openLockFile = (path: string): Promise<Deno.FsFile> =>
   Deno.open(path, { create: true, read: true, write: true });
@@ -41,10 +49,7 @@ const sameFileAt = async (
  * mid-wait, leaving a lock with nothing pointing at it, so it makes the folder
  * and takes the lock again until it has the one at the path.
  */
-export const withFileLock = async <Result>(
-  path: string,
-  body: () => Promise<Result>,
-): Promise<Result> => {
+export const withFileLock: PathLockHolder = async (path, body) => {
   for (;;) {
     await Deno.mkdir(dirname(path), { recursive: true });
     const file = await openLockFileOrNull(path);
@@ -60,27 +65,3 @@ export const withFileLock = async <Result>(
     }
   }
 };
-
-/**
- * Hold the lock at `path` while `body` runs, but only if it comes free within
- * `timeoutMs`; otherwise answer `null`. Unlike `withFileLock` this never makes
- * the folder or waits it out — a caller clearing up after other people must
- * not queue behind them for an hour, nor bring back a folder they removed.
- *
- * A child process does the waiting, so giving up really lets this one finish.
- */
-export const withFileLockOrNull = async <Result>(
-  path: string,
-  timeoutMs: number,
-  body: () => Promise<Result>,
-): Promise<Result | null> => {
-  const held = await holdLockOrNull(path, timeoutMs);
-  if (held === null) return null;
-  try {
-    // The wait may have been won because somebody deleted the folder: a lock on
-    // a file nothing points at keeps nobody out, so it is not worth holding.
-    return (await sameFileAt(path, held.fileNumber)) ? await body() : null;
-  } finally {
-    await held.letGo();
-  }
-};

scripts/mutation.ts

@@ -45,43 +45,44 @@ Examples:
   deno task mutation src/shared/dates.ts test/shared/dates.test.ts
   deno task mutation 'src/shared/forms/definition.ts' 'test/shared/forms/definition/*.test.ts' --exhaustive`;
 
-const main = async (): Promise<void> => {
+// Ends by returning, never by Deno.exit: as a snapshot child this runs
+// inside runSnapshotChild, whose clean-up must go ahead after the work.
+const main = async (): Promise<number> => {
   const args = parseArgs(Deno.args);
   if (args.error !== null) {
     console.error(args.error);
-    Deno.exit(1);
+    return 1;
   }
   if (args.help || args.sources.length === 0 || args.tests.length === 0) {
     console.log(USAGE);
-    Deno.exit(args.help ? 0 : 1);
+    return args.help ? 0 : 1;
   }
 
   const sourceFiles = await expand(args.sources);
   const testFiles = await expand(args.tests);
   if (sourceFiles.length === 0) {
     console.error("No source files matched.");
-    Deno.exit(1);
+    return 1;
   }
   if (testFiles.length === 0) {
     console.error("No test files matched.");
-    Deno.exit(1);
+    return 1;
   }
 
   const { runMutationTesting } = await import("./mutation/runner.ts");
-  const code = await runMutationTesting({
+  return await runMutationTesting({
     ...(args.batchJobs === undefined ? {} : { batchJobs: args.batchJobs }),
     exhaustive: args.exhaustive,
     sourceFiles,
     testFiles,
     timeout: args.timeout,
     useHarness: args.useHarness,
   });
-  Deno.exit(code);
 };
 
 if (import.meta.main) {
   if (isSnapshotChild()) {
-    await runSnapshotChild(main);
+    Deno.exit(await runSnapshotChild(main));
   } else {
     Deno.exit(await runIsolatedMutationCommand(Deno.args));
   }

scripts/mutation/equivalent-mutants/scripts.txt

@@ -103,3 +103,10 @@ src/shared/crypto/encryption.ts:120:5  false → true   # importKey is module-pr
 src/features/admin/attendee-page.ts:122:43  ?? → ||   # URLSearchParams.get returns string | null, and the fallback is the empty string: null and "" both yield "" under either operator, so no query can tell them apart
 src/features/admin/attendees-list.ts:56:52  ?? → ||   # parsePositiveInt validates against PositiveIntSchema (minValue 1), so it returns a number of 1 or more, or null — it can never return 0, the only falsy number that would tell ?? and || apart
 src/features/admin/listing-page-data.ts:282:61  ?? → ||   # Map.get returns ListingWithCount[] | undefined, and every array — empty included — is truthy, so only undefined reaches the fallback and both operators yield the same []
+
+# stale-claim touch rescheduling: corrupting the remembered timer id only
+# stops the release's clearTimeout from cancelling the armed timer, so it
+# fires once more — and the touch it starts bails on the stopped flag before
+# writing anything. No file state or output differs; only when the last no-op
+# timer drains.
+scripts/stale-claim.ts:186:12  = → +=    # the uncancelled timer's one late touch bails on the stopped flag before any read or write