diff --git a/TODO.md b/TODO.md index 500131a364..20c69b8207 100644 --- a/TODO.md +++ b/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 diff --git a/scripts/audit-equivalent-mutants.ts b/scripts/audit-equivalent-mutants.ts index df55b1509c..d960a8e7f6 100644 --- a/scripts/audit-equivalent-mutants.ts +++ b/scripts/audit-equivalent-mutants.ts @@ -61,10 +61,14 @@ const runAudit = async (options: AuditOptions): Promise => { 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}`); diff --git a/scripts/held-lock-process.ts b/scripts/held-lock-process.ts deleted file mode 100644 index 4366101d4e..0000000000 --- a/scripts/held-lock-process.ts +++ /dev/null @@ -1,120 +0,0 @@ -/** - * A lock held for us by a child process, so that giving up on the wait really - * does give up. - * - * Waiting for a lock in this process cannot be called off: the wait keeps the - * process alive until the lock arrives, however long that takes, and closing - * the file does not stop it. A command that gave up after a quarter of a second - * would still sit there until the other job finished. So a child does the - * waiting, and gives up by ending itself. - */ - -import { dirname } from "@std/path"; -import { statOrNull } from "#scripts/not-found.ts"; - -/** - * Waits for the lock, says which file it got, and holds it until its input - * closes — when we let go, or if we die first. It gives itself the same time - * we would, counted from when it starts waiting, and ends if that runs out. - */ -/** The child says the folder went with this, rather than dying of it. */ -const LOCK_FOLDER_GONE_EXIT_CODE = 3; - -const HOLD_LOCK_SCRIPT = ` -const [path, timeoutText] = Deno.args; -const say = (line) => Deno.stdout.write(new TextEncoder().encode(line + "\\n")); -const opened = await Deno.open(path, { create: true, read: true, write: true }) - .catch((why) => why); -// Its folder can go while it is starting. That is nobody's lock, not a failure -// to take one, and only the child can tell the two apart for certain. -if (opened instanceof Deno.errors.NotFound) { - Deno.exit(${LOCK_FOLDER_GONE_EXIT_CODE}); -} -if (!(opened instanceof Deno.FsFile)) throw opened; -const file = opened; -const gaveUp = setTimeout(() => Deno.exit(0), Number(timeoutText)); -await file.lock(true); -clearTimeout(gaveUp); -const { ino } = await file.stat(); -await say(\`held \${ino}\`); -await Deno.stdin.read(new Uint8Array(1)); -await file.unlock(); -file.close(); -`; - -interface HeldLock { - /** The file it locked, or `NaN` when the disk keeps no file numbers. */ - fileNumber: number; - letGo: () => Promise; -} - -/** The child's `held` line, or `null` if it ends before saying a whole one. */ -const heldLine = async ( - reader: ReadableStreamDefaultReader, -): Promise => { - let said = ""; - for (;;) { - const { value } = await reader.read(); - if (value === undefined) return null; - said += new TextDecoder().decode(value); - // Whole lines only — everything after the last newline is still arriving. - // "held" and its file number can come in two pieces, and half a line taken - // for the whole of one loses the number. - const whole = said.split("\n").slice(0, -1); - const held = whole.find((line) => line.startsWith("held")); - if (held !== undefined) return held; - } -}; - -/** Wait for the child to stop, leaving none of its pipes open behind us. */ -const closeDown = async ( - child: Deno.ChildProcess, -): Promise => { - await child.stdin.close(); - return await child.status; -}; - -/** - * Hold the lock at `path` through a child process. `null` means the lock did - * not come free within `timeoutMs`, or its folder is not there to lock in. A - * lock that could not be taken at all throws instead: that is not "busy". - */ -export const holdLockOrNull = async ( - path: string, - timeoutMs: number, -): Promise => { - // No folder to make the lock file in means nobody's lock to take. - if ((await statOrNull(dirname(path))) === null) return null; - const child = new Deno.Command(Deno.execPath(), { - args: ["eval", HOLD_LOCK_SCRIPT, "--", path, String(timeoutMs)], - stderr: "piped", - stdin: "piped", - stdout: "piped", - }).spawn(); - - const reader = child.stdout.getReader(); - const line = await heldLine(reader); - reader.releaseLock(); - await child.stdout.cancel(); - - if (line === null) { - // It ends quietly when its own time runs out, and says so when its folder - // went. Ending any other way means the lock could not be taken at all, - // which is not the same as busy. - const why = await new Response(child.stderr).text(); - const stopped = await closeDown(child); - const expected = - stopped.success || stopped.code === LOCK_FOLDER_GONE_EXIT_CODE; - if (!expected) { - throw new Error(`Could not take the lock at ${path}: ${why.trim()}`); - } - return null; - } - await child.stderr.cancel(); - return { - fileNumber: Number(line.split(" ")[1]), - letGo: async () => { - await closeDown(child); - }, - }; -}; diff --git a/scripts/lock-file.ts b/scripts/lock-file.ts index bdf635f775..266000127d 100644 --- a/scripts/lock-file.ts +++ b/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 = () => Promise; + +/** Holds the lock living at a path around a task. */ +export type PathLockHolder = ( + path: string, + body: LockBody, +) => Promise; + /** Open (creating if needed) a file to hold an advisory lock. */ const openLockFile = (path: string): Promise => 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 ( - path: string, - body: () => Promise, -): Promise => { +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 ( } } }; - -/** - * 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 ( - path: string, - timeoutMs: number, - body: () => Promise, -): Promise => { - 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(); - } -}; diff --git a/scripts/mutation.ts b/scripts/mutation.ts index 9845a6d45a..0cbbca6e1d 100644 --- a/scripts/mutation.ts +++ b/scripts/mutation.ts @@ -45,30 +45,32 @@ 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 => { +// 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 => { 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, @@ -76,12 +78,11 @@ const main = async (): Promise => { 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)); } diff --git a/scripts/mutation/equivalent-mutants/scripts.txt b/scripts/mutation/equivalent-mutants/scripts.txt index 813301afd0..ba0f7fd8a6 100644 --- a/scripts/mutation/equivalent-mutants/scripts.txt +++ b/scripts/mutation/equivalent-mutants/scripts.txt @@ -93,7 +93,9 @@ scripts/process.ts:60:17 0 → 1 # the promise body runs straight away and ov scripts/process.ts:62:12 = → += # the handle starts at 0, so adding to it and replacing it store the same timer id # Isolated mutation runs: the exit code before any path has set one. -scripts/mutation/isolation.ts:188:18 1 → 0 # every path overwrites this before it is returned: the lock always runs its callback, which either sets 130 on an interrupt or hands back a child whose status sets the code, and any throw is caught and sets it there +scripts/mutation/isolation.ts:216:20 1 → 0 # every path overwrites this before it is returned: the claim always runs its callback, which either sets 130 on an interrupt or hands back a child whose status sets the code, and any throw is caught and sets it there +scripts/mutation/isolation.ts:161:25 0 → 1 # keepFiles' interrupted arm only runs once wasInterrupted() has latched true, and finishChild then takes the `interrupted ? 130` branch, so the number this arm hands back is never read +scripts/mutation/isolation.ts:195:62 ?? → || # run.copyBack is a list or undefined, and a list — empty included — is truthy, so only undefined reaches the fallback and both operators yield the same [] src/shared/db/migrations.ts:252:49 ?? → || # opts.allowMissingSettings is boolean | undefined, and false ?? false and false || false both yield false # Cucumber runs: telling Cucumber to be strict, when we already reject the @@ -103,3 +105,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 diff --git a/scripts/mutation/isolation-cleanup.ts b/scripts/mutation/isolation-cleanup.ts index cd6b2cd4f9..b9a6a93372 100644 --- a/scripts/mutation/isolation-cleanup.ts +++ b/scripts/mutation/isolation-cleanup.ts @@ -2,15 +2,16 @@ * Which mutation runs are still going, and deleting the ones that are not. * * The supervisor in isolation.ts uses this both to clear up before a new run - * and to serve the explicit list/kill/clean commands. + * and to serve the explicit list/kill/clean commands. Whether a run is still + * going is answered by its claim (isolation-lock.ts): a fresh claim means its + * supervisor is alive and touching it, however long ago the record was + * written and whatever became of the child's process id. */ -import { join } from "@std/path"; import { chunk } from "#fp"; -import { statNumberOrNull, statOrNull } from "#scripts/not-found.ts"; -import { processExists, removeTree } from "#scripts/process.ts"; +import { removeTree } from "#scripts/process.ts"; import { errorMessage } from "#shared/error-message.ts"; -import { runLockIsHeld, withRunLockOrNull } from "./isolation-lock.ts"; +import { runClaimIsFresh, withRunClaimGuard } from "./isolation-lock.ts"; import { readRunRecord, recordInRunDirectory, @@ -18,48 +19,33 @@ import { } from "./isolation-records.ts"; import { isRunId, - MUTATION_RECORD_FILE, type MutationRunRecord, newRunRecord, recordPath, - runRoot, - runStartedRecently, - withinStartupGrace, } from "./isolation-state.ts"; -/** Is this record's run started, and its process still alive? */ -const runProcessIsUp = (record: MutationRunRecord): boolean => +/** Is this record's run started, its child signalable, and its folder still + * somebody's? Fresh claims are only ever the run's own — see isolation-lock. */ +export const runIsOwned = async (record: MutationRunRecord): Promise => record.status === "running" && record.pid !== undefined && - processExists(record.pid); + (await runClaimIsFresh(record)); -export const processBelongsToRun = async ( - record: MutationRunRecord, -): Promise => runProcessIsUp(record) && (await runLockIsHeld(record)); - -/** The record as it reads on disk right now, if it can be read at all. */ -const freshRecord = async ( - record: MutationRunRecord, -): Promise => { - const latest = await readRunRecord(join(record.root, MUTATION_RECORD_FILE)); - return latest === null ? null : { ...latest, root: record.root }; -}; +type RemoveRunResult = + | { record: MutationRunRecord; removed: true } + | { error: unknown; record: MutationRunRecord; removed: false }; export const liveRunIdSet = async ( records: MutationRunRecord[], ): Promise> => { const live = await Promise.all( records.map(async (record) => - (await processBelongsToRun(record)) ? record.id : null, + (await runIsOwned(record)) ? record.id : null, ), ); return new Set(live.filter((id): id is string => id !== null)); }; -type RemoveRunResult = - | { record: MutationRunRecord; removed: true } - | { error: unknown; record: MutationRunRecord; removed: false }; - /** Delete one folder of a run, treating an already-missing folder as done. */ const removeRunPath = async ( record: MutationRunRecord, @@ -77,37 +63,24 @@ const removeRunPath = async ( }; /** - * Judged from the record alone: the caller already holds the folder's lock, so - * a live run is one whose child is up, or one so young its record may not have - * caught up yet. + * Delete a run's whole folder, but only when its claim has gone stale. The + * run's own supervisor keeps the claim fresh from before the first record + * write until after the snapshot is thrown away, so a fresh claim means the + * folder is still someone's — `null` says it was left alone. The judgement + * and the delete happen under the claim takers' guard: a brand-new run makes + * its folder a moment before its claim lands in it, and holding the guard + * keeps this from reading that moment as an abandoned run. A stale claim + * cannot come back to life mid-delete: a run id is never claimed twice, and + * its one owner is gone. */ -const activeByRecord = (record: MutationRunRecord): boolean => - record.status === "copying" - ? runStartedRecently(record) - : runProcessIsUp(record) && runStartedRecently(record); - -/** - * Delete a run's whole folder, but only while holding its lock, and only if - * the record read under that lock still says the run is over. Holding the lock - * is what stops an owner claiming the folder in the moment between the two. - * `null` means the run is still someone's, so it was left alone. - */ -const removeRun = async ( +const removeRun = ( record: MutationRunRecord, -): Promise => { - const outcome = await withRunLockOrNull(record, async () => { - const latest = (await freshRecord(record)) ?? record; - return activeByRecord(latest) +): Promise => + withRunClaimGuard(record, async () => + (await runClaimIsFresh(record)) ? null - : await removeRunPath(record, record.root); - }); - if (outcome !== null) return outcome; - // Nothing left to leave alone means another clear-up got there first, which - // is the outcome we wanted, not a run we skipped. - return (await statOrNull(record.root)) === null - ? { record, removed: true } - : null; -}; + : await removeRunPath(record, record.root), + ); interface CleanedRuns { failed: Extract[]; @@ -116,10 +89,9 @@ interface CleanedRuns { } /** - * How many runs to clear away at once. Each one waits for its folder's lock in - * a process of its own, and a first run after a long time without one can find - * hundreds of folders waiting — enough processes at once to run a machine out - * of them. + * How many runs to clear away at once. Each folder is a whole checkout copy, + * and a first run after a long time without one can find hundreds waiting — + * deleting them all at once would swamp the disk. */ const RUNS_CLEARED_AT_ONCE = 8; @@ -169,29 +141,14 @@ export const removeWorkSnapshot = async ( if (!result.removed) reportRemoveFailure("the snapshot of", result); }; -/** When a folder last changed, or `null` when that cannot be told. */ -const folderChangedAt = statNumberOrNull((info) => info.mtime?.getTime()); - /** * A folder whose record cannot be read belongs to a run that was killed while - * writing it. Stand in for that record so the folder can be checked and - * cleared like any other. + * writing it. Stand in for that record so the folder can be judged by its + * claim and cleared like any other — a run writing its record right now holds + * a fresh claim, taken before the record's first write. */ -const recordForUnreadableRun = async ( - id: string, - root: string, -): Promise => { - const changedAt = await folderChangedAt(runRoot(id, root)); - // Young folders are left alone, in case a run is writing its record now — - // and so are folders whose age cannot be told at all. - const leaveAlone = changedAt === null || withinStartupGrace(changedAt); - return { - ...newRunRecord(id, [], root), - updatedAt: leaveAlone - ? new Date().toISOString() - : new Date(0).toISOString(), - }; -}; +const recordForUnreadableRun = (id: string, root: string): MutationRunRecord => + newRunRecord(id, [], root); const runsToSweep = async (root: string): Promise => { // Only folders this runner named, and picked out before anything inside them @@ -201,7 +158,7 @@ const runsToSweep = async (root: string): Promise => { ours.map(async (id) => { const record = await readRunRecord(recordPath(id, root)); return record === null - ? await recordForUnreadableRun(id, root) + ? recordForUnreadableRun(id, root) : recordInRunDirectory(record, id, root); }), ); diff --git a/scripts/mutation/isolation-lock.ts b/scripts/mutation/isolation-lock.ts index b743500516..69149dfb81 100644 --- a/scripts/mutation/isolation-lock.ts +++ b/scripts/mutation/isolation-lock.ts @@ -1,113 +1,94 @@ /** - * The lock each mutation run holds while it owns its folder. + * The claim each mutation run's supervisor holds on the run's folder, and the + * one checkout-wide lock runs share when bringing kept files back. * - * The locking itself lives in #scripts/lock-file.ts, which every lock in the - * repo goes through. This file only says where a run's lock is, and adds the - * one thing runs need that other locks do not: asking whether somebody else is - * holding one, without waiting to find out. + * The claim mechanics live in #scripts/stale-claim.ts, which the stripe-mock + * install shares. The supervisor takes its run's claim before the run's first + * record write and keeps it fresh until the snapshot is gone, so a clear-up + * can tell a run somebody still owns (fresh claim) from one whose supervisor + * has walked away (stale or missing claim) — without ever asking a process id + * that may since have been handed to somebody else. */ -import { withFileLock, withFileLockOrNull } from "#scripts/lock-file.ts"; -import { statOrNull } from "#scripts/not-found.ts"; -import { denoExitCode } from "./child-process.ts"; +import { type LockBody, withFileLock } from "#scripts/lock-file.ts"; +import { + ageClaimToStale, + claimIsFresh, + keepClaimFresh, + withClaim, + withClaimGuard, +} from "#scripts/stale-claim.ts"; import { copyBackLockPath, type MutationRunRecord, - runLockPath, + runClaimPath, } from "./isolation-state.ts"; -const LOCK_HELD_EXIT_CODE = 124; -const LOCK_FREE_EXIT_CODE = 0; +/** + * A claim untouched for this long belongs to a supervisor that has walked + * away — killed, or on a machine that stopped — and its folder may be + * cleared. Generous next to the touch below, so a briefly starved supervisor + * does not lose its run. + */ +export const RUN_CLAIM_STALE_MS = 30_000; -const LOCK_PROBE_SCRIPT = ` -const [path, timeoutText] = Deno.args; -const timeout = setTimeout( - () => Deno.exit(${LOCK_HELD_EXIT_CODE}), - Number(timeoutText), -); -const opened = await Deno.open(path, { read: true, write: true }).catch((why) => why); -// A lock file that has gone is nobody holding one — a clear-up can take the -// folder away between being asked and being looked at. -if (opened instanceof Deno.errors.NotFound) { - clearTimeout(timeout); - Deno.exit(${LOCK_FREE_EXIT_CODE}); -} -if (!(opened instanceof Deno.FsFile)) { - clearTimeout(timeout); - Deno.exit(2); -} -const file = opened; -try { - await file.lock(true); - await file.unlock(); - clearTimeout(timeout); - file.close(); - Deno.exit(${LOCK_FREE_EXIT_CODE}); -} catch { - clearTimeout(timeout); - file.close(); - Deno.exit(2); -} -`; +const RUN_CLAIM_SETTINGS = { + staleMs: RUN_CLAIM_STALE_MS, + touchMs: 1_000, +}; -const lockProbeExitCode = (path: string, timeoutMs: number): Promise => - denoExitCode(["eval", LOCK_PROBE_SCRIPT, "--", path, String(timeoutMs)], { - stderr: "null", - stdout: "null", - }); +/** Hold this run's claim while the whole run happens. A run id is brand new, + * so its claim is always free to take. */ +export const withRunClaim = ( + record: Pick, + body: LockBody, +): Promise => + withClaim( + runClaimPath(record), + { + ...RUN_CLAIM_SETTINGS, + name: `the claim on isolated mutation run ${record.id}`, + retryMs: 0, + timeoutMs: 0, + }, + body, + ); -/** - * Is somebody holding this run's lock? Asked in a child process, because a lock - * this process already holds would look free to itself. - */ -export const runLockIsHeld = async ( +/** Is this run's folder still somebody's? Only a run's own supervisor and its + * child write and touch the claim, so a fresh one is always the run's. */ +export const runClaimIsFresh = ( record: Pick, - timeoutMs = 50, -): Promise => { - const path = runLockPath(record); - // No lock file means no run holding one. - if ((await statOrNull(path)) === null) return false; - const stopped = await lockProbeExitCode(path, timeoutMs); - if (stopped === LOCK_HELD_EXIT_CODE) return true; - if (stopped === LOCK_FREE_EXIT_CODE) return false; - throw new Error( - `Could not tell whether the lock at ${path} is held: the check stopped with code ${stopped}`, - ); -}; +): Promise => claimIsFresh(runClaimPath(record), RUN_CLAIM_STALE_MS); /** - * Hold a run's lock while `run` works, but only if it is free within - * `timeoutMs`; otherwise give up and answer `null`. Clearing up must never - * queue behind a run that holds its folder for an hour. + * Hold the run-claim takers' guard while `body` runs, so judging the run and + * acting on the judgement cannot interleave with its supervisor mid-take. */ -export const withRunLockOrNull = ( +export const withRunClaimGuard = ( record: Pick, - run: () => Promise, - timeoutMs = 250, -): Promise => - withFileLockOrNull(runLockPath(record), timeoutMs, run); + body: LockBody, +): Promise => withClaimGuard(runClaimPath(record), body); -/** Holds the lock named by `Key` while the body runs. */ -export type LockHolder = ( - key: Key, - run: () => Promise, -) => Promise; - -/** A lock named by where it lives: say how to find it, get its holder. */ -const lockHolder = - (pathOf: (key: Key) => string): LockHolder => - (key, run) => - withFileLock(pathOf(key), run); +/** The child's half of the claim: keep it fresh while the child works, so a + * supervisor killed without cleaning up cannot cost a live child its copy. */ +export const keepRunClaimFresh = ( + record: Pick, +): ReturnType => + keepClaimFresh(runClaimPath(record), RUN_CLAIM_SETTINGS); -/** Hold a run's own lock, for as long as the run needs it. */ -export const withMutationRunLock: LockHolder = lockHolder( - (runRootPath: string) => runLockPath({ root: runRootPath }), -); +/** Age the run's claim so it reads as walked away — for a child whose + * supervisor died and so can never release it. */ +export const ageRunClaimToStale = ( + record: Pick, + ownedBy: string, +): Promise => ageClaimToStale(runClaimPath(record), ownedBy); /** * Hold the checkout's copy-back lock. Every run brings its kept files back * through this one lock, so two runs finishing together cannot both read the * checkout, agree it is unchanged, and then write over each other. */ -export const withCopyBackLock: LockHolder = - lockHolder(copyBackLockPath); +export const withCopyBackLock = ( + root: string, + run: LockBody, +): Promise => withFileLock(copyBackLockPath(root), run); diff --git a/scripts/mutation/isolation-state.ts b/scripts/mutation/isolation-state.ts index 405e4cd613..3c63fd6a5e 100644 --- a/scripts/mutation/isolation-state.ts +++ b/scripts/mutation/isolation-state.ts @@ -14,10 +14,11 @@ export const MUTATION_RUNS_DIR = ".mutation-runs"; export const MUTATION_WORK_DIR = "work"; export const MUTATION_RECORD_FILE = "run.json"; const MUTATION_RUN_ID_PREFIX = "mutation-"; -export const MUTATION_RUN_LOCK_FILE = "run.lock"; +export const MUTATION_CLAIM_FILE = "claim"; const MUTATION_COPY_BACK_LOCK_FILE = "copy-back.lock"; export const MUTATION_SNAPSHOT_CHILD_ENV = "TICKETS_MUTATION_SNAPSHOT_CHILD"; export const MUTATION_RUN_ID_ENV = "TICKETS_MUTATION_RUN_ID"; +export const MUTATION_SUPERVISOR_PID_ENV = "TICKETS_MUTATION_SUPERVISOR_PID"; export const MUTATION_RUN_ROOT_ENV = "TICKETS_MUTATION_RUN_ROOT"; export const MUTATION_WORK_ROOT_ENV = "TICKETS_MUTATION_WORK_ROOT"; @@ -204,8 +205,9 @@ export const workRoot = runChildPath(MUTATION_WORK_DIR); export const recordPath = runChildPath(MUTATION_RECORD_FILE); -export const runLockPath = (record: Pick): string => - join(record.root, MUTATION_RUN_LOCK_FILE); +/** Where a run's claim sits: the supervisor's proof the folder is still its. */ +export const runClaimPath = (record: Pick): string => + join(record.root, MUTATION_CLAIM_FILE); /** One lock for the whole checkout, shared by every run bringing files back. */ export const copyBackLockPath = (root = projectRoot): string => @@ -240,6 +242,17 @@ export const markRunning = ( updatedAt, }); +/** The record once its child has ended: the pid is dropped, so nothing can + * signal a process id that may since be somebody else's while the supervisor + * finishes the run's copy-back. */ +export const markChildEnded = ( + record: MutationRunRecord, + updatedAt = nowIso(), +): MutationRunRecord => { + const { pid: _endedChildPid, ...rest } = record; + return { ...rest, updatedAt }; +}; + export const markFinished = ( record: MutationRunRecord, exitCode: number, @@ -264,35 +277,6 @@ export const markInterrupted = ( export const isTerminalRunStatus = (status: MutationRunStatus): boolean => status === "passed" || status === "failed" || status === "interrupted"; -/** - * How long after a run is marked "running" we still treat it as active for - * cleanup, even if the child has not acquired the run lock yet. This covers - * the startup window between `spawn()` and the child taking the lock. After - * it expires, a running record with a live PID but no lock is stale (the PID - * may have been reused by an unrelated process) and can be cleaned. - */ -export const RUN_STARTUP_GRACE_MS = 30_000; - -/** - * Was `at` within the startup grace? Unknown times count as long ago, and so - * do times in the future: a clock put back must not make a folder look busy - * for ever. - */ -export const withinStartupGrace = ( - at: number, - now: Date = new Date(), - graceMs: number = RUN_STARTUP_GRACE_MS, -): boolean => { - const age = now.getTime() - at; - return at > 0 && age >= 0 && age < graceMs; -}; - -export const runStartedRecently = ( - record: MutationRunRecord, - now?: Date, - graceMs?: number, -): boolean => withinStartupGrace(Date.parse(record.updatedAt), now, graceMs); - const withTrailingSeparator = (path: string): string => path.endsWith(SEPARATOR) ? path : `${path}${SEPARATOR}`; diff --git a/scripts/mutation/isolation.ts b/scripts/mutation/isolation.ts index 77e1c096e6..0e99fa584a 100644 --- a/scripts/mutation/isolation.ts +++ b/scripts/mutation/isolation.ts @@ -20,13 +20,17 @@ import { errorMessage } from "#shared/error-message.ts"; import { envWith } from "./child-process.ts"; import { liveRunIdSet, - processBelongsToRun, removeFinishedRuns, removeInactiveRuns, removeWorkSnapshot, reportRemoveFailure, + runIsOwned, } from "./isolation-cleanup.ts"; -import { withCopyBackLock, withMutationRunLock } from "./isolation-lock.ts"; +import { + withCopyBackLock, + withRunClaim, + withRunClaimGuard, +} from "./isolation-lock.ts"; import { readRunRecords, writeRunRecord } from "./isolation-records.ts"; import { copyMutationSnapshot, @@ -36,14 +40,17 @@ import { MUTATION_RUN_ID_ENV, MUTATION_RUN_ROOT_ENV, MUTATION_SNAPSHOT_CHILD_ENV, + MUTATION_SUPERVISOR_PID_ENV, MUTATION_WORK_ROOT_ENV, type MutationRunRecord, + markChildEnded, markFinished, markInterrupted, markRunning, newRunRecord, parseIsolationCommand, rewriteMutationArgs, + runClaimPath, type SnapshotArgsFn, selectedRuns, } from "./isolation-state.ts"; @@ -71,7 +78,7 @@ const signalRun = async ( record: MutationRunRecord, force: boolean, ): Promise => { - if (!(await processBelongsToRun(record)) || record.pid === undefined) { + if (!(await runIsOwned(record)) || record.pid === undefined) { return false; } try { @@ -91,6 +98,9 @@ const childEnv = ( [MUTATION_SNAPSHOT_CHILD_ENV]: "1", [MUTATION_RUN_ID_ENV]: id, [MUTATION_RUN_ROOT_ENV]: runRootPath, + // The child keeps the run's claim fresh, and needs to know whether this + // supervisor is still alive to release it when the child ends. + [MUTATION_SUPERVISOR_PID_ENV]: String(Deno.pid), [MUTATION_WORK_ROOT_ENV]: snapshotRoot, }); @@ -103,9 +113,20 @@ const childArgs = ...rewriteMutationArgs(root, snapshotRoot, args), ]; -const forceStopChild = (child: Deno.ChildProcess | null): never => { +const forceStopChild = ( + child: Deno.ChildProcess | null, + record: MutationRunRecord, +): void => { if (child) stopProcessNow(child); - Deno.exit(130); + // Exiting here skips the claim's release, so take the claim down first — + // under the takers' guard, so a child touch mid-write cannot put it back + // after the removal. The child was just killed, so the guard frees in + // moments even if that touch held it. Without the claim the run reads as + // over at once instead of after a whole stale window. + void withRunClaimGuard(record, () => { + Deno.removeSync(runClaimPath(record)); + return Promise.resolve(); + }).finally(() => Deno.exit(130)); }; const killChildQuietly = (child: Deno.ChildProcess | null): void => { @@ -141,29 +162,30 @@ const keepFiles = ( : bringFilesBack(root, workRoot, copyBack), ); -/** Record the child's result, then bring back what the run means to keep. */ -const finishChild = ( +/** + * Record the child's result, then bring back what the run means to keep. The + * supervisor's claim on the run stays fresh throughout, so a clear-up + * elsewhere can neither take the copy before its kept files are read nor take + * the folder while the last record write lands in it. + */ +const finishChild = async ( record: MutationRunRecord, wasInterrupted: () => boolean, code: number, root: string, copyBack: CopyBackFile[], -): Promise<{ exitCode: number; record: MutationRunRecord }> => - // All under the lock, so a clear-up elsewhere can neither take the copy - // before its kept files are read nor take the folder while the last record - // write lands in it. - withMutationRunLock(record.root, async () => { - const failedToKeep = - wasInterrupted() || copyBack.length === 0 - ? 0 - : await keepFiles(wasInterrupted, root, record.workRoot, copyBack); - const interrupted = wasInterrupted(); - // A run that could not keep its files failed, whatever the child said. - const exitCode = interrupted ? 130 : failedToKeep || code; - const settled = settleRecord(record, interrupted, exitCode); - await writeRunRecord(settled); - return { exitCode, record: settled }; - }); +): Promise<{ exitCode: number; record: MutationRunRecord }> => { + const failedToKeep = + wasInterrupted() || copyBack.length === 0 + ? 0 + : await keepFiles(wasInterrupted, root, record.workRoot, copyBack); + const interrupted = wasInterrupted(); + // A run that could not keep its files failed, whatever the child said. + const exitCode = interrupted ? 130 : failedToKeep || code; + const settled = settleRecord(record, interrupted, exitCode); + await writeRunRecord(settled); + return { exitCode, record: settled }; +}; export const runInSnapshot = async ( run: SnapshotRun, @@ -174,71 +196,72 @@ export const runInSnapshot = async ( await removeInactiveRuns(root); const id = createRunId(); let record = newRunRecord(id, args, root); - await writeRunRecord(record); + // Claimed before the record's first write and held until the snapshot is + // gone, so there is no moment when a clear-up can see this run unowned. + return await withRunClaim(record, async () => { + await writeRunRecord(record); - let child: Deno.ChildProcess | null = null; - let interrupted = false; - const stopChild = (): void => { - if (interrupted) forceStopChild(child); - interrupted = true; - killChildQuietly(child); - }; - onTerminationSignals(stopChild); + let child: Deno.ChildProcess | null = null; + let interrupted = false; + const stopChild = (): void => { + if (interrupted) { + forceStopChild(child, record); + return; + } + interrupted = true; + killChildQuietly(child); + }; + onTerminationSignals(stopChild); - let exitCode = 1; - try { - console.log(`Creating isolated mutation run ${id}`); - console.log(`Snapshot: ${relative(root, record.workRoot)}`); - child = await withMutationRunLock(record.root, async () => { - // A `--clean` running in the gap above sees an unlocked copying run and - // removes it, so write the record again now the lock makes it safe. - await writeRunRecord(record); + let exitCode = 1; + try { + console.log(`Creating isolated mutation run ${id}`); + console.log(`Snapshot: ${relative(root, record.workRoot)}`); await copyMutationSnapshot(root, record.workRoot); if (interrupted) { record = markInterrupted(record); await writeRunRecord(record); exitCode = 130; - return null; - } - const spawned = new Deno.Command(Deno.execPath(), { - args: childArgs(run.entryScript)(root, record.workRoot, args), - cwd: record.workRoot, - env: childEnv(id, record.root, record.workRoot), - ...INHERIT_STDIO, - }).spawn(); - child = spawned; - record = markRunning(record, spawned.pid); - await writeRunRecord(record); - console.log(`Mutation child pid ${spawned.pid}`); - return spawned; - }); + } else { + const spawned = new Deno.Command(Deno.execPath(), { + args: childArgs(run.entryScript)(root, record.workRoot, args), + cwd: record.workRoot, + env: childEnv(id, record.root, record.workRoot), + ...INHERIT_STDIO, + }).spawn(); + child = spawned; + record = markRunning(record, spawned.pid); + await writeRunRecord(record); + console.log(`Mutation child pid ${spawned.pid}`); - if (child !== null) { - const status = await child.status; - const finished = await finishChild( - record, - () => interrupted, - status.code, - root, - copyBack, - ); - record = finished.record; - exitCode = finished.exitCode; - } - } catch (error) { - if (child !== null) await stopProcess(child, 250); - exitCode = interrupted ? 130 : 1; - record = settleRecord(record, interrupted, exitCode); - try { - await writeRunRecord(record); - } catch { - // A failed write should not mask the original error. + const status = await spawned.status; + record = markChildEnded(record); + await writeRunRecord(record); + const finished = await finishChild( + record, + () => interrupted, + status.code, + root, + copyBack, + ); + record = finished.record; + exitCode = finished.exitCode; + } + } catch (error) { + if (child !== null) await stopProcess(child, 250); + exitCode = interrupted ? 130 : 1; + record = settleRecord(record, interrupted, exitCode); + try { + await writeRunRecord(record); + } catch { + // A failed write should not mask the original error. + } + console.error(errorMessage(error)); } - console.error(errorMessage(error)); - } - offTerminationSignals(stopChild); - await removeWorkSnapshot(record); - return exitCode; + offTerminationSignals(stopChild); + await removeWorkSnapshot(record); + return exitCode; + }); }; export const runMutationInSnapshot: MutationCommandRunner = ( diff --git a/scripts/mutation/snapshot-child.ts b/scripts/mutation/snapshot-child.ts index f6d3b37cae..a5df2ea4d5 100644 --- a/scripts/mutation/snapshot-child.ts +++ b/scripts/mutation/snapshot-child.ts @@ -7,11 +7,12 @@ import { resolve } from "@std/path"; import { projectRoot } from "#scripts/project-root.ts"; -import { withMutationRunLock } from "./isolation-lock.ts"; +import { ageRunClaimToStale, keepRunClaimFresh } from "./isolation-lock.ts"; import { MUTATION_RUN_ID_ENV, MUTATION_RUN_ROOT_ENV, MUTATION_SNAPSHOT_CHILD_ENV, + MUTATION_SUPERVISOR_PID_ENV, MUTATION_WORK_ROOT_ENV, } from "./isolation-state.ts"; @@ -33,9 +34,17 @@ const runValue = (name: string): string => { return value; }; -const runRootFromEnv = (): string => { +type SnapshotRunHome = { runRoot: string; supervisorPid: number }; + +const runHomeFromEnv = (): SnapshotRunHome => { runValue(MUTATION_RUN_ID_ENV); const runRoot = runValue(MUTATION_RUN_ROOT_ENV); + const supervisorPid = Number(runValue(MUTATION_SUPERVISOR_PID_ENV)); + if (!Number.isInteger(supervisorPid) || supervisorPid <= 0) { + throw new Error( + `${MUTATION_SUPERVISOR_PID_ENV} must be a process id, not ${runValue(MUTATION_SUPERVISOR_PID_ENV)}.`, + ); + } const workRoot = resolve(runValue(MUTATION_WORK_ROOT_ENV)); // The copy it says it is in must be the one it is running from. Anything // else means these values belong to another run, and the work would land in @@ -45,10 +54,35 @@ const runRootFromEnv = (): string => { `A snapshot child says it works in ${workRoot}, but it is running in ${resolve(projectRoot)}.`, ); } - return runRoot; + return { runRoot, supervisorPid }; +}; + +const workUnderFreshClaim = async ( + { runRoot, supervisorPid }: SnapshotRunHome, + body: () => Promise, +): Promise => { + const record = { root: runRoot }; + const claim = await keepRunClaimFresh(record); + try { + return await body(); + } finally { + try { + await claim.stopTouching(); + } finally { + // With the supervisor gone, nobody is left to release the claim; age + // it so the run reads as over the moment this child ends, instead of + // staying "live" for a whole stale window. Gone means reparented: a + // pid probe could bless a pid already reused by a stranger, but no + // reused pid can become this child's parent. + if (Deno.ppid !== supervisorPid) { + await ageRunClaimToStale(record, claim.ownedBy); + } + } + } }; -/** Do the run's work, holding its lock so a clear-up cannot take the copy. */ +/** Do the run's work, keeping the supervisor's claim on the run fresh — so a + * clear-up cannot take the copy even if the supervisor is killed outright. */ export const runSnapshotChild = ( body: () => Promise, -): Promise => withMutationRunLock(runRootFromEnv(), body); +): Promise => workUnderFreshClaim(runHomeFromEnv(), body); diff --git a/scripts/process.ts b/scripts/process.ts index 1b5d9830cc..58d8a58ae8 100644 --- a/scripts/process.ts +++ b/scripts/process.ts @@ -1,5 +1,3 @@ -import nodeProcess from "node:process"; - /** Wire a child process's three stdio streams straight to the parent's — for * interactive or inherited runs where the child shares the same terminal. */ export const INHERIT_STDIO = { @@ -68,15 +66,6 @@ const beforeTimeout = async ( } }; -export const processExists = (pid: number): boolean => { - try { - nodeProcess.kill(pid, 0); - return true; - } catch { - return false; - } -}; - export const stopProcessNow = (process: Deno.ChildProcess): void => { try { process.kill("SIGKILL"); diff --git a/scripts/stale-claim.ts b/scripts/stale-claim.ts new file mode 100644 index 0000000000..b99474aca0 --- /dev/null +++ b/scripts/stale-claim.ts @@ -0,0 +1,340 @@ +/** + * A claim on a shared job, kept as a file whose owner can walk away. + * + * The plain file locks in #scripts/lock-file.ts end the moment their holder's + * process ends, and a waiter cannot tell a busy holder from a dead one. Some + * jobs need exactly that told apart: an install another runner should take + * over if its downloader died, a run folder that must not be cleared while + * its supervisor lives. So a claim is a file naming its owner and when that + * owner last checked in. The holder keeps touching the file while it works; a + * claim untouched for long enough counts as walked away from, and the next + * taker removes it and claims in its place. + * + * The database migration lock (src/shared/db/migrations/lock.ts) is this same + * shape written into a settings row — an owner token, a freshness cutoff, a + * steal of anything older, a release only of your own claim. This module is + * the file version, shared by the stripe-mock install and mutation runs. + */ + +import { + type LockBody, + type PathLockHolder, + withFileLock, +} from "#scripts/lock-file.ts"; +import { nullIfNotFound, rethrowUnlessNotFound } from "#scripts/not-found.ts"; +import { errorMessage } from "#shared/error-message.ts"; +import { delay } from "#shared/now.ts"; + +/** How a claim is kept fresh, and when it counts as walked away from. */ +export type StaleClaimSettings = { + /** A claim untouched for this long may be removed and taken over. */ + staleMs: number; + /** How often the holder rewrites its check-in time while it works. */ + touchMs: number; +}; + +/** How long to keep asking for a busy claim, and what to call it on giving up. */ +export type ClaimWait = { + /** Names the claim in the giving-up error, e.g. "stripe-mock install lock". */ + name: string; + retryMs: number; + timeoutMs: number; +}; + +/** A claim this caller holds: release it once the work is done. */ +type HeldClaim = { + owner: string; + release: () => Promise; +}; + +type ClaimRecord = { + owner?: string; + writtenAt: number; +}; + +const CLAIM_GUARD_SUFFIX = ".guard"; + +/** + * Takers serialize through a plain file lock beside the claim, so two of them + * cannot both judge a claim stale and both believe they created its successor. + */ +const claimGuardPath = (path: string): string => `${path}${CLAIM_GUARD_SUFFIX}`; + +/** + * Hold the takers' guard for the claim at `path` while `body` runs, so a + * decision made about the claim — is it fresh, may what it protects go — + * cannot interleave with somebody in the middle of taking it. + */ +export const withClaimGuard: PathLockHolder = (path, body) => + withFileLock(claimGuardPath(path), body); + +const formatClaimRecord = (owner: string): string => `${owner}\n${Date.now()}`; + +const parseClaimRecord = (text: string): ClaimRecord => { + // Splitting always yields at least one part, so first is always a string. + const [first, second] = text.split("\n") as [string, string?]; + const writtenAt = Number(second ?? first); + return second === undefined ? { writtenAt } : { owner: first, writtenAt }; +}; + +const readClaimRecord = async (path: string): Promise => { + const record = parseClaimRecord(await Deno.readTextFile(path)); + if (record.writtenAt > 0) return record; + + const stat = await Deno.stat(path); + if (stat.mtime === null) { + throw new Error( + `The claim at ${path} names no readable time, and the filesystem keeps no last-write time to judge it by.`, + ); + } + return { ...record, writtenAt: stat.mtime.getTime() }; +}; + +const writeClaimTime = (path: string, owner: string): Promise => + Deno.writeTextFile(path, formatClaimRecord(owner)); + +const claimAgeMs = async (path: string): Promise => + Date.now() - (await readClaimRecord(path)).writtenAt; + +/** + * Is somebody's claim here, touched recently enough to still be theirs? A + * missing claim is nobody's, which is the expected way for one to be absent. + */ +export const claimIsFresh = async ( + path: string, + staleMs: number, +): Promise => { + const age = await nullIfNotFound(claimAgeMs(path)); + // A time from the future means a clock was put back; counting that as + // fresh would leave a dead owner's claim unstealable until it catches up. + return age !== null && age >= 0 && age < staleMs; +}; + +/** Remove the claim, treating one already cleared by another taker as gone. */ +const removeClaim = async (path: string): Promise => { + try { + await Deno.remove(path); + } catch (error) { + rethrowUnlessNotFound(error); + } +}; + +const claimLostError = (path: string): Error => + new Error( + `The claim at ${path} was lost while the work ran: another taker judged it walked away and took or removed it, so the same work may also have run elsewhere.`, + ); + +/** + * Run `act` under the takers' guard, but only while the claim is still the + * given owner's; answers whether it was. A paused holder may have been + * stolen from, and acting blindly then would trample the new owner's record. + */ +const whileStillOwned = ( + path: string, + owner: string, + act: () => Promise, +): Promise => + withClaimGuard(path, async () => { + const record = await nullIfNotFound(readClaimRecord(path)); + if (record === null || record.owner !== owner) return false; + await act(); + return true; + }); + +const touchIfStillOurs = (path: string, owner: string): Promise => + whileStillOwned(path, owner, () => writeClaimTime(path, owner)); + +/** Remove the claim at release. Anything but our own claim there means the + * work ran unprotected for a while, which cannot end quietly. */ +const releaseOwnClaim = async (path: string, owner: string): Promise => { + if (!(await whileStillOwned(path, owner, () => removeClaim(path)))) { + throw claimLostError(path); + } +}; + +/** + * Age the claim so the next look judges it walked away — for a helper whose + * owner is gone and can never release it. Somebody else's claim is left be. + */ +export const ageClaimToStale = async ( + path: string, + owner: string, +): Promise => { + const aged = await whileStillOwned(path, owner, () => + Deno.writeTextFile(path, `${owner}\n1`), + ); + // A claim that is no longer that owner's cannot end quietly: the work it + // protected may already have run again elsewhere. + if (!aged) throw claimLostError(path); +}; + +/** Stops touching a claim, and throws if the claim was seen lost meanwhile. */ +export type StopTouching = () => Promise; + +const startClaimTouching = ( + path: string, + owner: string, + { touchMs }: StaleClaimSettings, +): StopTouching => { + let stopped = false; + let claimLost = false; + let timeout = setTimeout(touchClaim, touchMs); + let latestTouch: Promise = Promise.resolve(); + + const scheduleNextTouch = () => { + if (stopped || claimLost) return; + timeout = setTimeout(touchClaim, touchMs); + }; + + const touchOutcome = (stillOurs: boolean) => { + if (!stillOurs) claimLost = true; + scheduleNextTouch(); + }; + + function touchClaim() { + // A timer can fire in the very moment of the stop; a touch that went + // ahead then could re-create the claim file after release removed it. + if (stopped) return; + // A write that failed is left alone: the file just ages, and the + // ownership checks here and at release notice anything that let happen. + latestTouch = touchIfStillOurs(path, owner).then( + touchOutcome, + scheduleNextTouch, + ); + } + + return async (): Promise => { + stopped = true; + clearTimeout(timeout); + // A touch already under way finishes without booking another, because + // both writing and scheduling check the flag above. + await latestTouch; + if (claimLost) throw claimLostError(path); + }; +}; + +const createClaim = async (path: string, owner: string): Promise => { + const file = await Deno.open(path, { createNew: true, write: true }); + file.close(); + await writeClaimTime(path, owner); +}; + +const heldClaim = ( + path: string, + owner: string, + settings: StaleClaimSettings, +): HeldClaim => { + const stopTouching = startClaimTouching(path, owner, settings); + return { + owner, + release: async () => { + try { + await stopTouching(); + } finally { + await releaseOwnClaim(path, owner); + } + }, + }; +}; + +/** + * Take the claim at `path` if it is free or walked away from, and start + * keeping it fresh. `null` means somebody else's claim is still fresh. + */ +const tryTakeClaim = ( + path: string, + settings: StaleClaimSettings, +): Promise => + withClaimGuard(path, async () => { + const owner = crypto.randomUUID(); + try { + await createClaim(path, owner); + } catch (error) { + if (!(error instanceof Deno.errors.AlreadyExists)) throw error; + if (await claimIsFresh(path, settings.staleMs)) return null; + await removeClaim(path); + await createClaim(path, owner); + } + return heldClaim(path, owner, settings); + }); + +/** + * Keep somebody else's claim at `path` fresh on their behalf, starting with a + * touch right now, until the returned stop is called. For a helper working + * under a claim its parent took — the mutation child inside its snapshot — + * so the claim outlives a parent that was killed without cleaning up. + * Stopping never removes the claim: that stays the owner's to release. + */ +export const keepClaimFresh = async ( + path: string, + settings: StaleClaimSettings, +): Promise<{ ownedBy: string; stopTouching: StopTouching }> => { + // The owner is read and first touched under one hold of the takers' guard: + // an unguarded read could catch the owner's own touch mid-write and see a + // half-written record with no owner in it. + const owner = await withClaimGuard(path, async () => { + const record = await readClaimRecord(path); + if (record.owner === undefined) { + throw new Error( + `The claim at ${path} names no owner to keep it fresh for.`, + ); + } + await writeClaimTime(path, record.owner); + return record.owner; + }); + return { + ownedBy: owner, + stopTouching: startClaimTouching(path, owner, settings), + }; +}; + +/** + * Take the claim at `path`, waiting out a fresh holder until `timeoutMs` runs + * out. Whoever holds it keeps touching it, so a wait that ends without the + * claim means a live holder, not an abandoned one. + */ +const takeClaim = async ( + path: string, + settings: StaleClaimSettings & ClaimWait, +): Promise => { + const startedAt = Date.now(); + while (true) { + const claim = await tryTakeClaim(path, settings); + if (claim) return claim; + + if (Date.now() - startedAt >= settings.timeoutMs) { + throw new Error(`Timed out waiting for ${settings.name}`); + } + await delay(settings.retryMs); + } +}; + +/** + * Take the claim, do the work, and let the claim go — even on failure. A + * release that finds the claim lost fails the work; when the work had + * already failed on its own, that failure stays the story, with the loss + * noted alongside it. + */ +export const withClaim = async ( + path: string, + settings: StaleClaimSettings & ClaimWait, + body: LockBody, +): Promise => { + const claim = await takeClaim(path, settings); + let result: Result; + try { + result = await body(); + } catch (failure) { + try { + await claim.release(); + } catch (releaseFailure) { + throw new Error( + `${errorMessage(failure)}; and letting the claim go then found more wrong: ${errorMessage(releaseFailure)}`, + { cause: failure }, + ); + } + throw failure; + } + await claim.release(); + return result; +}; diff --git a/scripts/stripe-mock.ts b/scripts/stripe-mock.ts index 6774d90966..ecba94efda 100644 --- a/scripts/stripe-mock.ts +++ b/scripts/stripe-mock.ts @@ -13,10 +13,9 @@ import { stripeMock } from "#src/shared/stripe/mock.ts"; import { withFileLock } from "./lock-file.ts"; import { stopProcess, stopProcessNow } from "./process.ts"; import { + binDirGuard, defaultStripeMockPaths, downloadStripeMock, - ensureBinDir, - type LockBody, type StripeMockCommands, type StripeMockInstallOptions, type StripeMockPaths, @@ -224,13 +223,9 @@ const captureStreamText = ( const stripeMockStartLockPath = (paths: StripeMockPaths): string => join(paths.binDir, START_LOCK_NAME); -const withStripeMockStartLock = async ( - paths: StripeMockPaths, - body: LockBody, -): Promise => { - await ensureBinDir(paths); - return withFileLock(stripeMockStartLockPath(paths), body); -}; +const withStripeMockStartLock = binDirGuard( + (paths) => (body) => withFileLock(stripeMockStartLockPath(paths), body), +); const resolveStripeMockPort = ( options: StartStripeMockOptions, diff --git a/scripts/stripe-mock/install.ts b/scripts/stripe-mock/install.ts index b7432a2830..ef43c79313 100644 --- a/scripts/stripe-mock/install.ts +++ b/scripts/stripe-mock/install.ts @@ -1,10 +1,9 @@ /* jscpd:ignore-start */ import { join } from "node:path"; -import { withFileLock } from "#scripts/lock-file.ts"; -import { rethrowUnlessNotFound } from "#scripts/not-found.ts"; +import type { LockBody } from "#scripts/lock-file.ts"; import { removeTree } from "#scripts/process.ts"; import { projectRoot } from "#scripts/project-root.ts"; -import { delay } from "#shared/now.ts"; +import { type StaleClaimSettings, withClaim } from "#scripts/stale-claim.ts"; /* jscpd:ignore-end */ @@ -14,7 +13,6 @@ const INSTALL_LOCK_TIMEOUT_MS = 60_000; const INSTALL_LOCK_STALE_MS = 30_000; const INSTALL_LOCK_TOUCH_MS = 1_000; const INSTALL_TEMP_PREFIX = "stripe-mock-"; -const INSTALL_LOCK_GUARD_SUFFIX = ".guard"; const BIN_DIR = join(projectRoot, ".bin"); const STRIPE_MOCK_PATH = join(BIN_DIR, "stripe-mock"); @@ -24,13 +22,25 @@ export type StripeMockPaths = { binaryPath: string; }; -/** A task run while a lock is held, giving back its result. */ -export type LockBody = () => Promise; - /** Ensure the bin directory (and any parents) exists. */ -export const ensureBinDir = (paths: StripeMockPaths): Promise => +const ensureBinDir = (paths: StripeMockPaths): Promise => Deno.mkdir(paths.binDir, { recursive: true }); +/** Holds one of the bin folder's guards around a task, making the folder. */ +export type BinDirGuard = ( + paths: StripeMockPaths, + body: LockBody, +) => Promise; + +export const binDirGuard = + ( + holdAt: (paths: StripeMockPaths) => (body: LockBody) => Promise, + ): BinDirGuard => + async (paths: StripeMockPaths, body: LockBody): Promise => { + await ensureBinDir(paths); + return holdAt(paths)(body); + }; + export type StripeMockCommands = { chmod: string; curl: string; @@ -46,23 +56,6 @@ export type StripeMockInstallOptions = { paths?: StripeMockPaths; }; -type InstallLockSettings = { - retryMs: number; - staleMs: number; - timeoutMs: number; - touchMs: number; -}; - -type InstallLock = { - file: Deno.FsFile; - owner: string; -}; - -type InstallLockRecord = { - owner?: string; - writtenAt: number; -}; - export const defaultStripeMockPaths: StripeMockPaths = { binaryPath: STRIPE_MOCK_PATH, binDir: BIN_DIR, @@ -115,9 +108,6 @@ const stripeMockBinaryExists = async ( export const installLockPath = (paths: StripeMockPaths): string => join(paths.binDir, "stripe-mock.install.lock"); -const installLockGuardPath = (lockPath: string): string => - `${lockPath}${INSTALL_LOCK_GUARD_SUFFIX}`; - const installLockSettings = ({ // Only a missing setting takes the standard value, so an explicit one — // including a deliberate zero — is always kept. @@ -125,174 +115,26 @@ const installLockSettings = ({ installLockStaleMs: staleMs = INSTALL_LOCK_STALE_MS, installLockTimeoutMs: timeoutMs = INSTALL_LOCK_TIMEOUT_MS, installLockTouchMs: touchMs = INSTALL_LOCK_TOUCH_MS, -}: StripeMockInstallOptions): InstallLockSettings => ({ +}: StripeMockInstallOptions): StaleClaimSettings & { + retryMs: number; + timeoutMs: number; +} => ({ retryMs, staleMs, timeoutMs, touchMs, }); -const formatInstallLockRecord = (owner: string): string => - `${owner}\n${Date.now()}`; - -const parseInstallLockRecord = (text: string): InstallLockRecord => { - // Splitting always yields at least one part, so first is always a string. - const [first, second] = text.split("\n") as [string, string?]; - const writtenAt = Number(second ?? first); - return second === undefined ? { writtenAt } : { owner: first, writtenAt }; -}; - -const readInstallLockRecord = async ( - lockPath: string, -): Promise => { - const record = parseInstallLockRecord(await Deno.readTextFile(lockPath)); - if (record.writtenAt > 0) return record; - - const stat = await Deno.stat(lockPath); - return { ...record, writtenAt: stat.mtime!.getTime() }; -}; - -const writeInstallLockTime = (lockPath: string, owner: string): Promise => - Deno.writeTextFile(lockPath, formatInstallLockRecord(owner)); - -const startInstallLockRefresh = ( - lockPath: string, - owner: string, - touchMs: number, -) => { - let stopped = false; - let timeout = setTimeout(refreshLock, touchMs); - let latestRefresh: Promise = Promise.resolve(); - - const scheduleNextRefresh = () => { - if (stopped) return; - timeout = setTimeout(refreshLock, touchMs); - }; - - function refreshLock() { - latestRefresh = writeInstallLockTime(lockPath, owner).then( - scheduleNextRefresh, - scheduleNextRefresh, - ); - } - - return async (): Promise => { - stopped = true; - clearTimeout(timeout); - // A timer that fired just before the clear has already queued refreshLock, - // and it runs before this zero delay does. Waiting one turn means the wait - // below catches the write it starts, instead of leaking it past the stop — - // a leaked write could re-create the lock file after cleanup removed it. - await delay(0); - // A refresh already under way finishes without booking another, because - // scheduling checks the flag above. - await latestRefresh; - }; -}; - -const installLockAgeMs = async (lockPath: string): Promise => - Date.now() - (await readInstallLockRecord(lockPath)).writtenAt; - -const removeStaleInstallLock = async ( - lockPath: string, - staleMs: number, -): Promise => { - try { - if ((await installLockAgeMs(lockPath)) < staleMs) return false; - await Deno.remove(lockPath); - return true; - } catch (error) { - // NotFound: another runner already cleared the stale lock — it's gone - // either way, which is exactly what this returns true for. - rethrowUnlessNotFound(error); - return true; - } -}; - -const removeLockIfOwned = async ( - lockPath: string, - owner: string, -): Promise => { - try { - if ((await readInstallLockRecord(lockPath)).owner === owner) { - await Deno.remove(lockPath); - } - } catch (error) { - rethrowUnlessNotFound(error); - } -}; - -const createInstallLock = async (lockPath: string): Promise => { - const owner = crypto.randomUUID(); - const file = await Deno.open(lockPath, { createNew: true, write: true }); - try { - await writeInstallLockTime(lockPath, owner); - return { file, owner }; - } catch (error) { - file.close(); - throw error; - } -}; - -const tryAcquireInstallLock = ( - lockPath: string, - settings: InstallLockSettings, -): Promise => - withFileLock(installLockGuardPath(lockPath), async () => { - try { - return await createInstallLock(lockPath); - } catch (error) { - if (!(error instanceof Deno.errors.AlreadyExists)) throw error; - if (await removeStaleInstallLock(lockPath, settings.staleMs)) { - return await createInstallLock(lockPath); - } - return null; - } - }); - -const acquireInstallLock = async ( - lockPath: string, - settings: InstallLockSettings, -): Promise => { - const startedAt = Date.now(); - while (true) { - const lock = await tryAcquireInstallLock(lockPath, settings); - if (lock) return lock; - - if (Date.now() - startedAt >= settings.timeoutMs) { - throw new Error("Timed out waiting for stripe-mock install lock"); - } - await delay(settings.retryMs); - } -}; - -const withInstallLock = async ( - paths: StripeMockPaths, - options: StripeMockInstallOptions, - body: LockBody, -): Promise => { - const lockPath = installLockPath(paths); - const settings = installLockSettings(options); - await ensureBinDir(paths); - const lock = await acquireInstallLock(lockPath, settings); - const stopRefreshingLock = startInstallLockRefresh( - lockPath, - lock.owner, - settings.touchMs, +const installLockHolder = (options: StripeMockInstallOptions) => + binDirGuard( + (paths) => (body) => + withClaim( + installLockPath(paths), + { ...installLockSettings(options), name: "stripe-mock install lock" }, + body, + ), ); - try { - return await body(); - } finally { - try { - await stopRefreshingLock(); - } finally { - lock.file.close(); - await removeLockIfOwned(lockPath, lock.owner); - } - } -}; - const commandsWithDefaults = ( commands: Partial | undefined, ): StripeMockCommands => ({ ...defaultCommands, ...commands }); @@ -363,7 +205,7 @@ export const downloadStripeMock = async ( const { paths = defaultStripeMockPaths } = options; if (await stripeMockBinaryExists(paths)) return; - await withInstallLock(paths, options, async () => { + await installLockHolder(options)(paths, async () => { if (await stripeMockBinaryExists(paths)) return; await installStripeMock(paths, commandsWithDefaults(options.commands)); }); diff --git a/test/scripts/held-lock-process.test.ts b/test/scripts/held-lock-process.test.ts deleted file mode 100644 index 6caf4c1f04..0000000000 --- a/test/scripts/held-lock-process.test.ts +++ /dev/null @@ -1,111 +0,0 @@ -import { join } from "node:path"; -import { expect } from "@std/expect"; -import { describe, it as test } from "@std/testing/bdd"; -import { holdLockOrNull } from "#scripts/held-lock-process.ts"; -import { withFileLock } from "#scripts/lock-file.ts"; -import { withTempDir } from "#test-utils/files.ts"; - -/** Long enough that a lock the operating system would hand over has arrived. */ -const LONG_ENOUGH_TO_BE_LET_IN_MS = 200; - -/** A lock that was taken, as `holdLockOrNull` hands it back. */ -type HeldLock = NonNullable>>; - -const pause = (milliseconds: number): Promise => - new Promise((resolve) => setTimeout(resolve, milliseconds)); - -/** - * Take a lock that must be free, and let it go however the work inside ends — - * a holder left behind would keep the next test out of the same file. - */ -const holdingLock = async ( - path: string, - work: (held: HeldLock) => Promise, -): Promise => { - const held = await holdLockOrNull(path, LONG_ENOUGH_TO_BE_LET_IN_MS); - expect(held).not.toBeNull(); - if (held === null) return; - try { - await work(held); - } finally { - await held.letGo(); - } -}; - -describe("a lock held for us by another process", () => { - test("takes a free lock and says which file it got", async () => { - await withTempDir(async (root) => { - const path = join(root, "one.lock"); - - await holdingLock(path, async (held) => { - // The number names the file it locked, so we can tell it is still ours. - expect(held.fileNumber).toBe((await Deno.stat(path)).ino); - }); - }); - }); - - test("keeps everyone else out while it holds the lock", async () => { - await withTempDir(async (root) => { - const path = join(root, "one.lock"); - let ranInside = false; - let second: Promise = Promise.resolve(); - - await holdingLock(path, async (_held) => { - second = withFileLock(path, () => { - ranInside = true; - return Promise.resolve(); - }); - // Long enough that a lock excluding nobody would have been handed over. - await pause(LONG_ENOUGH_TO_BE_LET_IN_MS); - - expect(ranInside).toBe(false); - }); - - await second; - expect(ranInside).toBe(true); - }); - }); - - test("gives up on a lock somebody else is holding", async () => { - await withTempDir(async (root) => { - const path = join(root, "one.lock"); - await holdingLock(path, async () => { - expect(await holdLockOrNull(path, 30)).toBeNull(); - }); - }); - }); - - test("says so loudly when the lock cannot be taken at all", async () => { - await withTempDir(async (root) => { - const path = join(root, "one.lock"); - // A folder where the lock file should be: the lock can never be taken, - // which is not the same as somebody else holding it. - await Deno.mkdir(path); - - await expect( - holdLockOrNull(path, LONG_ENOUGH_TO_BE_LET_IN_MS), - ).rejects.toThrow("Could not take the lock"); - }); - }); - - test("gives up when there is no folder to make the lock in", async () => { - await withTempDir(async (root) => { - const answer = await holdLockOrNull( - join(root, "never-made", "one.lock"), - LONG_ENOUGH_TO_BE_LET_IN_MS, - ); - - expect(answer).toBeNull(); - }); - }); - - test("takes the lock again once the last holder lets go", async () => { - await withTempDir(async (root) => { - const path = join(root, "one.lock"); - await holdingLock(path, () => Promise.resolve()); - - // A holder that did not really let go would make this wait for ever. - await holdingLock(path, () => Promise.resolve()); - }); - }); -}); diff --git a/test/scripts/lock-file.test.ts b/test/scripts/lock-file.test.ts index 0e8a320263..20ed690079 100644 --- a/test/scripts/lock-file.test.ts +++ b/test/scripts/lock-file.test.ts @@ -3,7 +3,7 @@ import { expect } from "@std/expect"; import { afterEach, beforeEach, describe, it as test } from "@std/testing/bdd"; import { stub } from "@std/testing/mock"; import { removeIfPresent } from "#scripts/cleanup.ts"; -import { withFileLock, withFileLockOrNull } from "#scripts/lock-file.ts"; +import { withFileLock } from "#scripts/lock-file.ts"; import { withTempDir } from "#test-utils/files.ts"; const LOCK_PATH = join( @@ -299,95 +299,3 @@ describe("a lock that stops being the file at its path", () => { }); }); }); - -describe("withFileLockOrNull", () => { - test("takes a free lock and gives it back", async () => { - await withTempDir(async (root) => { - const path = join(root, "one.lock"); - - expect( - await withFileLockOrNull(path, 250, () => Promise.resolve("done")), - ).toBe("done"); - // Free again, or this second take would wait for ever. - expect( - await withFileLockOrNull(path, 250, () => Promise.resolve("again")), - ).toBe("again"); - }); - }); - - test("gives up rather than queue behind whoever holds it", async () => { - await withTempDir(async (root) => { - const path = join(root, "one.lock"); - const release = Promise.withResolvers(); - const holdingIt = Promise.withResolvers(); - let ranInside = false; - - const holding = withFileLock(path, () => { - holdingIt.resolve(); - return release.promise; - }); - await holdingIt.promise; - const gaveUp = await withFileLockOrNull(path, 30, () => { - ranInside = true; - return Promise.resolve("should not happen"); - }); - - expect(gaveUp).toBeNull(); - expect(ranInside).toBe(false); - - release.resolve(); - await holding; - // The lock it gave up on is handed back when it finally arrives, so the - // path is free for the next holder. - await new Promise((resolve) => setTimeout(resolve, 30)); - expect( - await withFileLockOrNull(path, 250, () => Promise.resolve("free")), - ).toBe("free"); - }); - }); - - test("leaves a folder that has already gone to whoever took it", async () => { - await withTempDir(async (root) => { - let ranInside = false; - - const answer = await withFileLockOrNull( - join(root, "never-made", "one.lock"), - 250, - () => { - ranInside = true; - return Promise.resolve("should not happen"); - }, - ); - - expect(answer).toBeNull(); - expect(ranInside).toBe(false); - }); - }); - - test("gives up when the folder was deleted while it queued", async () => { - await withTempDir(async (root) => { - const folder = join(root, "doomed"); - const path = join(folder, "one.lock"); - const holdingIt = Promise.withResolvers(); - const swept = Promise.withResolvers(); - let ranInside = false; - - const holding = withFileLock(path, async () => { - holdingIt.resolve(); - await swept.promise; - await Deno.remove(folder, { recursive: true }); - }); - await holdingIt.promise; - const waiting = withFileLockOrNull(path, 5_000, () => { - ranInside = true; - return Promise.resolve("should not happen"); - }); - await new Promise((resolve) => setTimeout(resolve, 30)); - swept.resolve(); - await holding; - - expect(await waiting).toBeNull(); - expect(ranInside).toBe(false); - }); - }); -}); diff --git a/test/scripts/mutation/isolation-cleanup/removing.test.ts b/test/scripts/mutation/isolation-cleanup/removing.test.ts index b49a10a408..5a482dfbcc 100644 --- a/test/scripts/mutation/isolation-cleanup/removing.test.ts +++ b/test/scripts/mutation/isolation-cleanup/removing.test.ts @@ -1,7 +1,7 @@ import { expect } from "@std/expect"; import { describe, it as test } from "@std/testing/bdd"; import { removeFinishedRuns } from "#scripts/mutation/isolation-cleanup.ts"; -import { withMutationRunLock } from "#scripts/mutation/isolation-lock.ts"; +import { withRunClaimGuard } from "#scripts/mutation/isolation-lock.ts"; import { writeRunRecord } from "#scripts/mutation/isolation-records.ts"; import { markFinished, @@ -10,45 +10,93 @@ import { } from "#scripts/mutation/isolation-state.ts"; import { LONG_AGO, + removeRunClaim, runIdNamed, withTempDir, + writeRunClaim, } from "#test/scripts/mutation/isolation-helpers.ts"; describe("deleting the runs that are finished with", () => { - test("keeps a run that came to life while the lock was probed", async () => { + test("keeps a run that came to life after its record was read", async () => { await withTempDir(async (root) => { - // What a sweep read before waiting on the lock: an old copying run. + // What a sweep read earlier: an old copying run. On disk now: the same + // run, running under a fresh claim. const asRead = newRunRecord( runIdNamed("woke-up"), [], root, LONG_AGO.toISOString(), ); - // What is on disk now: the same run, started moments ago. await writeRunRecord(markRunning(asRead, Deno.pid)); + await writeRunClaim(asRead); expect((await removeFinishedRuns([asRead])).skipped).toEqual([asRead]); }); }); - test("keeps a finished run whose folder is still held", async () => { + test("waits at the takers' door, honouring a claim made meanwhile", async () => { await withTempDir(async (root) => { - // A supervisor writing its last record holds the lock after its child - // has gone, so the record can already read as finished. - const settling = markFinished( + // A brand-new run makes its folder a moment before its claim lands in + // it. A sweep looking in that moment must wait for the taker, not read + // the bare folder as abandoned and delete it. + const young = newRunRecord(runIdNamed("mid-take"), [], root); + await Deno.mkdir(young.root, { recursive: true }); + + const inside = Promise.withResolvers(); + const release = Promise.withResolvers(); + const taking = withRunClaimGuard(young, async () => { + inside.resolve(); + await release.promise; + await writeRunClaim(young); + }); + await inside.promise; + + const sweeping = removeFinishedRuns([young]); + release.resolve(); + await taking; + + expect((await sweeping).skipped).toEqual([young]); + }); + }); + + test("keeps a run whose child has gone while its supervisor holds on", async () => { + await withTempDir(async (root) => { + // The child's process id is dead and the record was written long ago — + // the moment between a child ending and its supervisor settling the + // record. The supervisor's claim is what says the run is still someone's. + const settling = markRunning( newRunRecord(runIdNamed("settling"), [], root, LONG_AGO.toISOString()), - 0, + 99_999_999, + LONG_AGO.toISOString(), ); await writeRunRecord(settling); + await writeRunClaim(settling); - await withMutationRunLock(settling.root, async () => { - expect((await removeFinishedRuns([settling])).skipped).toEqual([ - settling, - ]); - }); - expect((await removeFinishedRuns([settling])).removed).toEqual([ + expect((await removeFinishedRuns([settling])).skipped).toEqual([ settling, ]); }); }); + + test("keeps a finished run until its supervisor lets the claim go", async () => { + await withTempDir(async (root) => { + // A supervisor clearing its snapshot away holds the claim after its + // child has gone, so the record can already read as finished. + const finished = markFinished( + newRunRecord(runIdNamed("settled"), [], root, LONG_AGO.toISOString()), + 0, + ); + await writeRunRecord(finished); + await writeRunClaim(finished); + + expect((await removeFinishedRuns([finished])).skipped).toEqual([ + finished, + ]); + + await removeRunClaim(finished); + expect((await removeFinishedRuns([finished])).removed).toEqual([ + finished, + ]); + }); + }); }); diff --git a/test/scripts/mutation/isolation-cleanup/startup.test.ts b/test/scripts/mutation/isolation-cleanup/startup.test.ts index 6a9bd38d41..fd69d47a65 100644 --- a/test/scripts/mutation/isolation-cleanup/startup.test.ts +++ b/test/scripts/mutation/isolation-cleanup/startup.test.ts @@ -5,6 +5,7 @@ import { stub } from "@std/testing/mock"; import { writeRunRecord } from "#scripts/mutation/isolation-records.ts"; import { createRunId, + MUTATION_CLAIM_FILE, MUTATION_RECORD_FILE, markFinished, newRunRecord, @@ -16,6 +17,7 @@ import { LONG_AGO, withTempDir, writeFakeMutationScript, + writeRunClaim, } from "#test/scripts/mutation/isolation-helpers.ts"; import { pathExists } from "#test-utils/files.ts"; @@ -31,93 +33,67 @@ const OLD_RUN_ID = createRunId( ); /** A run folder holding a snapshot and a record too broken to read. */ -const writeUnreadableRun = async ( - root: string, - changedAt: Date, -): Promise => { +const writeUnreadableRun = async (root: string): Promise => { const folder = runRoot(BROKEN_RUN_ID, root); await Deno.mkdir(join(folder, "work"), { recursive: true }); await Deno.writeTextFile(join(folder, MUTATION_RECORD_FILE), "{ half"); - await Deno.utime(folder, changedAt, changedAt); return folder; }; -/** Answer stats about the broken run folder with `info`, and pass the rest on. */ -const stubRunsFolderStat = ( - info: Partial | Error, -): Disposable => { - const stat = Deno.stat; - return stub(Deno, "stat", ((path: string | URL) => { - if (!`${path}`.includes(BROKEN_RUN_ID)) return stat(path); - return info instanceof Error - ? Promise.reject(info) - : Promise.resolve(info as Deno.FileInfo); - }) as typeof Deno.stat); -}; - -const runAfterUnreadableRun = async ( - root: string, - changedAt: Date, -): Promise => { +const runAfterUnreadableRun = async (root: string): Promise => { await writeFakeMutationScript(root, "Deno.exit(0);\n"); - const folder = await writeUnreadableRun(root, changedAt); + const folder = await writeUnreadableRun(root); await captureSimpleSnapshotMutation(root); return folder; }; -/** - * Run with a just-made unreadable run folder that answers stats with `info`, - * and report whether the folder was still there afterwards. - */ -const runWithStatAnswer = ( - info: Partial | Error, -): Promise => - withTempDir(async (root) => { - await writeFakeMutationScript(root, "Deno.exit(0);\n"); - const folder = await writeUnreadableRun(root, new Date()); - - await (async () => { - using _stat = stubRunsFolderStat(info); - await captureSimpleSnapshotMutation(root); - })(); - - return await pathExists(folder); - }); - describe("clearing up before a mutation run", () => { test("clears out a run folder whose record cannot be read", async () => { await withTempDir(async (root) => { - const folder = await runAfterUnreadableRun(root, LONG_AGO); + const folder = await runAfterUnreadableRun(root); expect(await pathExists(folder)).toBe(false); }); }); - test("leaves a run folder alone while its record is being written", async () => { + test("leaves an unreadable run folder alone while its claim is fresh", async () => { await withTempDir(async (root) => { - const folder = await runAfterUnreadableRun(root, new Date()); + await writeFakeMutationScript(root, "Deno.exit(0);\n"); + const folder = await writeUnreadableRun(root); + // Another run may be writing this record right now: its supervisor's + // claim, taken before the record's first write, is what says so. + await writeRunClaim({ root: folder }); + + await captureSimpleSnapshotMutation(root); - // Another run may be writing this record right now. expect(await pathExists(folder)).toBe(true); }); }); - test("leaves alone a run folder whose age cannot be told", async () => { - // No way to know it is over, so it stays put rather than being deleted. - expect(await runWithStatAnswer({ isDirectory: true, mtime: null })).toBe( - true, - ); - expect(await runWithStatAnswer(new Deno.errors.NotFound("gone"))).toBe( - true, - ); - }); - - test("gives up when a run folder cannot be asked about at all", async () => { - // Anything other than a missing folder means the disk is in a state we - // must not guess about, so the run stops instead of deleting blind. - await expect( - runWithStatAnswer(new Deno.errors.PermissionDenied("no access")), - ).rejects.toThrow("no access"); + test("gives up when a run's claim cannot be asked about at all", async () => { + await withTempDir(async (root) => { + await writeFakeMutationScript(root, "Deno.exit(0);\n"); + const folder = await writeUnreadableRun(root); + await writeRunClaim({ root: folder }); + + const readTextFile = Deno.readTextFile; + using _read = stub(Deno, "readTextFile", (( + path: string | URL, + options?: Deno.ReadFileOptions, + ) => { + if (`${path}` === join(folder, MUTATION_CLAIM_FILE)) { + // Anything other than a missing claim means the disk is in a state + // we must not guess about, so the run stops instead of deleting blind. + return Promise.reject(new Deno.errors.PermissionDenied("no access")); + } + return readTextFile(path, options); + }) as typeof Deno.readTextFile); + + await expect(captureSimpleSnapshotMutation(root)).rejects.toThrow( + "no access", + ); + expect(await pathExists(folder)).toBe(true); + }); }); test("leaves alone a folder this runner did not name", async () => { diff --git a/test/scripts/mutation/isolation-helpers.ts b/test/scripts/mutation/isolation-helpers.ts index f4c271762a..09982cf52f 100644 --- a/test/scripts/mutation/isolation-helpers.ts +++ b/test/scripts/mutation/isolation-helpers.ts @@ -2,10 +2,12 @@ import { join } from "node:path"; import { stub } from "@std/testing/mock"; import { runIsolatedMutationCommand } from "#scripts/mutation/isolation.ts"; import { + type MutationRunRecord, markFinished, markRunning, newRunRecord, recordPath, + runClaimPath, runRoot, } from "#scripts/mutation/isolation-state.ts"; import { withTempDir as withSharedTempDir } from "#test-utils/files.ts"; @@ -52,9 +54,29 @@ export const finishedRun = ( ): ReturnType => markFinished(newRunRecord(runIdNamed(label), [], root), 0); -/** A time far enough back that the startup grace no longer covers it. */ +/** A time far enough back that no claim written then is still fresh. */ export const LONG_AGO = new Date("2026-01-01T00:00:00.000Z"); +/** + * A claim as another run's supervisor would leave it on disk. Written at + * `writtenAt`, so a moment ago means a live supervisor and `LONG_AGO` means + * one that walked away. + */ +export const writeRunClaim = async ( + record: Pick, + writtenAt = Date.now(), +): Promise => { + await Deno.mkdir(record.root, { recursive: true }); + await Deno.writeTextFile( + runClaimPath(record), + `another-supervisor\n${writtenAt}`, + ); +}; + +export const removeRunClaim = ( + record: Pick, +): Promise => Deno.remove(runClaimPath(record)); + const lineFrom = (values: unknown[]): string => values.map(String).join(" "); export const captureConsole = async ( diff --git a/test/scripts/mutation/isolation-lock.test.ts b/test/scripts/mutation/isolation-lock.test.ts index 471142b165..56861b96b8 100644 --- a/test/scripts/mutation/isolation-lock.test.ts +++ b/test/scripts/mutation/isolation-lock.test.ts @@ -1,19 +1,20 @@ import { join } from "node:path"; import { expect } from "@std/expect"; import { describe, it as test } from "@std/testing/bdd"; -import { stub } from "@std/testing/mock"; -import type { LockHolder } from "#scripts/mutation/isolation-lock.ts"; import { - runLockIsHeld, + runClaimIsFresh, withCopyBackLock, - withMutationRunLock, - withRunLockOrNull, + withRunClaim, } from "#scripts/mutation/isolation-lock.ts"; import { copyBackLockPath, - MUTATION_RUN_LOCK_FILE, + runClaimPath, } from "#scripts/mutation/isolation-state.ts"; -import { withTempDir } from "#test/scripts/mutation/isolation-helpers.ts"; +import { + LONG_AGO, + withTempDir, + writeRunClaim, +} from "#test/scripts/mutation/isolation-helpers.ts"; import { pathExists } from "#test-utils/files.ts"; /** @@ -26,199 +27,58 @@ const LONG_ENOUGH_TO_BE_LET_IN_MS = 30; const pause = (milliseconds: number): Promise => new Promise((resolve) => setTimeout(resolve, milliseconds)); -/** - * Answer some stats differently, and pass the rest to the real one. `null` from - * `answer` means "not this path, look at it properly". - */ -const statSaying = ( - answer: ( - path: string, - real: typeof Deno.stat, - ) => Promise | null, -): Disposable => { - const real = Deno.stat; - return stub( - Deno, - "stat", - ((path: string | URL) => - answer(`${path}`, real) ?? real(path)) as typeof Deno.stat, - ); -}; - -/** - * Send two bodies through the same lock, and report the order they got in: - * once while the first is still inside, and once after both are done. - */ -const orderThroughLock = async ( - hold: LockHolder, - key: string, -): Promise<{ afterwards: string[]; whileHeld: string[] }> => { - const order: string[] = []; - const firstInside = Promise.withResolvers(); - const releaseFirst = Promise.withResolvers(); - - const first = hold(key, async () => { - order.push("first in"); - firstInside.resolve(); - await releaseFirst.promise; - order.push("first out"); - }); - await firstInside.promise; - - const second = hold(key, () => { - order.push("second in"); - return Promise.resolve(); - }); - await pause(LONG_ENOUGH_TO_BE_LET_IN_MS); - const whileHeld = [...order]; - - releaseFirst.resolve(); - await Promise.all([first, second]); - return { afterwards: order, whileHeld }; -}; - -describe("the lock that keeps two runs out of one folder", () => { - test("puts a run's lock inside the folder it was given", async () => { - await withTempDir(async (root) => { - const runFolder = join(root, ".mutation-runs", "mutation-new"); - - expect( - await withMutationRunLock(runFolder, () => Promise.resolve(7)), - ).toBe(7); - - expect(await pathExists(join(runFolder, MUTATION_RUN_LOCK_FILE))).toBe( - true, - ); - }); - }); - - test("keeps a second run out until the first one is done", async () => { - await withTempDir(async (root) => { - const runFolder = join(root, ".mutation-runs", "mutation-shared"); - - const order = await orderThroughLock(withMutationRunLock, runFolder); - - expect(order.whileHeld).toEqual(["first in"]); - expect(order.afterwards).toEqual(["first in", "first out", "second in"]); - }); - }); - - test("gives up rather than queue behind a run that holds its folder", async () => { - await withTempDir(async (root) => { - const record = { root: join(root, ".mutation-runs", "mutation-taken") }; - const release = Promise.withResolvers(); - const holdingIt = Promise.withResolvers(); - let ranInside = false; - - const holding = withMutationRunLock(record.root, () => { - holdingIt.resolve(); - return release.promise; - }); - await holdingIt.promise; - const gaveUp = await withRunLockOrNull( - record, - () => { - ranInside = true; - return Promise.resolve("should not happen"); - }, - LONG_ENOUGH_TO_BE_LET_IN_MS, - ); - - expect(gaveUp).toBeNull(); - expect(ranInside).toBe(false); - - release.resolve(); - await holding; - }); - }); - - test("reports a run nobody is holding as not held", async () => { - await withTempDir(async (root) => { - const record = { root: join(root, ".mutation-runs", "mutation-idle") }; - await Deno.mkdir(record.root, { recursive: true }); - - // No timeout given, so this also checks the standard one is long enough - // to tell "free" from "held" rather than calling everything held. - expect(await runLockIsHeld(record)).toBe(false); - }); - }); - - test("reports a run that has let its lock go as not held", async () => { - await withTempDir(async (root) => { - const record = { root: join(root, ".mutation-runs", "mutation-done") }; - // The lock file outlives the run that made it, so a file on its own is - // not somebody holding it. - await withMutationRunLock(record.root, () => Promise.resolve()); - - expect(await runLockIsHeld(record)).toBe(false); - }); - }); +const runAt = (root: string, name: string): { id: string; root: string } => ({ + id: name, + root: join(root, ".mutation-runs", name), +}); - test("reports a run somebody is holding as held", async () => { +describe("the claim a supervisor holds on its run", () => { + test("claims the run's own folder for the work and frees it after", async () => { await withTempDir(async (root) => { - const record = { root: join(root, ".mutation-runs", "mutation-busy") }; - const asked = Promise.withResolvers(); - const release = Promise.withResolvers(); + const record = runAt(root, "mutation-new"); - const holding = withMutationRunLock(record.root, async () => { - asked.resolve(await runLockIsHeld(record)); - await release.promise; + const answer = await withRunClaim(record, async () => { + expect(await pathExists(runClaimPath(record))).toBe(true); + expect(await runClaimIsFresh(record)).toBe(true); + return 7; }); - expect(await asked.promise).toBe(true); - release.resolve(); - await holding; - }); - }); - - test("reports a run whose folder is not there as not held", async () => { - await withTempDir(async (root) => { - // Nothing to make a lock file in, so nobody can be holding one. - expect(await runLockIsHeld({ root: join(root, "never-made") }, 100)).toBe( - false, - ); + expect(answer).toBe(7); + expect(await pathExists(runClaimPath(record))).toBe(false); + expect(await runClaimIsFresh(record)).toBe(false); }); }); - test("reports a lock that goes while being looked at as not held", async () => { + test("refuses a run somebody else still claims, naming the run", async () => { await withTempDir(async (root) => { - const record = { root: join(root, ".mutation-runs", "mutation-swept") }; - await Deno.mkdir(record.root, { recursive: true }); - // Says the lock file is there, but it is not — which is what a clear-up - // taking the folder away mid-question looks like. - using _stat = statSaying((path, real) => - path.endsWith(MUTATION_RUN_LOCK_FILE) ? real(record.root) : null, + const record = runAt(root, "mutation-taken"); + await writeRunClaim(record); + let ranAnyway = false; + + await expect( + withRunClaim(record, () => { + ranAnyway = true; + return Promise.resolve(); + }), + ).rejects.toThrow( + "Timed out waiting for the claim on isolated mutation run mutation-taken", ); - - expect(await runLockIsHeld(record)).toBe(false); + expect(ranAnyway).toBe(false); }); }); - test("says so loudly when the lock cannot be looked at", async () => { + test("counts a walked-away claim as nobody's", async () => { await withTempDir(async (root) => { - const record = { root: join(root, "unreadable") }; - await Deno.mkdir(record.root, { recursive: true }); - using _stat = statSaying((path) => - path.includes("unreadable") - ? Promise.reject(new Deno.errors.PermissionDenied("no entry")) - : null, - ); + const record = runAt(root, "mutation-left"); + await writeRunClaim(record, LONG_AGO.getTime()); - // Calling this "nobody is holding it" would let a clear-up delete a run - // that is very much still going. - await expect(runLockIsHeld(record)).rejects.toThrow("no entry"); + expect(await runClaimIsFresh(record)).toBe(false); }); }); - test("says so loudly when the check itself cannot be made", async () => { + test("counts a run with no claim at all as nobody's", async () => { await withTempDir(async (root) => { - const record = { root: join(root, ".mutation-runs", "mutation-odd") }; - // A folder where the lock file should be: the check can never answer. - await Deno.mkdir(join(record.root, MUTATION_RUN_LOCK_FILE), { - recursive: true, - }); - - await expect(runLockIsHeld(record)).rejects.toThrow("Could not tell"); + expect(await runClaimIsFresh(runAt(root, "mutation-none"))).toBe(false); }); }); }); @@ -226,10 +86,28 @@ describe("the lock that keeps two runs out of one folder", () => { describe("the lock that keeps two runs out of one copy-back", () => { test("keeps a second run out until the first has brought its files back", async () => { await withTempDir(async (root) => { - const order = await orderThroughLock(withCopyBackLock, root); + const order: string[] = []; + const firstInside = Promise.withResolvers(); + const releaseFirst = Promise.withResolvers(); + + const first = withCopyBackLock(root, async () => { + order.push("first in"); + firstInside.resolve(); + await releaseFirst.promise; + order.push("first out"); + }); + await firstInside.promise; + + const second = withCopyBackLock(root, () => { + order.push("second in"); + return Promise.resolve(); + }); + await pause(LONG_ENOUGH_TO_BE_LET_IN_MS); + expect(order).toEqual(["first in"]); - expect(order.whileHeld).toEqual(["first in"]); - expect(order.afterwards).toEqual(["first in", "first out", "second in"]); + releaseFirst.resolve(); + await Promise.all([first, second]); + expect(order).toEqual(["first in", "first out", "second in"]); expect(await pathExists(copyBackLockPath(root))).toBe(true); }); }); diff --git a/test/scripts/mutation/isolation-state/records.test.ts b/test/scripts/mutation/isolation-state/records.test.ts index 8cffd02f3b..aad58874bf 100644 --- a/test/scripts/mutation/isolation-state/records.test.ts +++ b/test/scripts/mutation/isolation-state/records.test.ts @@ -3,11 +3,11 @@ import { describe, it as test } from "@std/testing/bdd"; import { createRunId, isRunId, + markChildEnded, markFinished, markInterrupted, markRunning, newRunRecord, - runStartedRecently, statusForExitCode, workRoot, } from "#scripts/mutation/isolation-state.ts"; @@ -37,61 +37,26 @@ describe("mutation isolation run records", () => { }); }); - test("maps exit codes to run status", () => { - expect(statusForExitCode(0)).toBe("passed"); - expect(statusForExitCode(130)).toBe("interrupted"); - expect(statusForExitCode(2)).toBe("failed"); - }); - - test("treats a run as recently started only within the grace period", () => { - const now = new Date("2026-07-10T12:00:00.000Z"); - const fresh = markRunning( - newRunRecord("fresh", [], "/repo", "2026-07-10T11:59:45.000Z"), - 1, - "2026-07-10T11:59:45.000Z", - ); - const stale = markRunning( - newRunRecord("stale", [], "/repo", "2026-07-10T11:00:00.000Z"), - 2, - "2026-07-10T11:00:00.000Z", + test("drops the pid once the child has ended", () => { + const running = markRunning( + newRunRecord("ended", [], "/repo"), + 42, + "2026-07-09T12:35:00.000Z", ); - expect(runStartedRecently(fresh, now)).toBe(true); - expect(runStartedRecently(stale, now)).toBe(false); - }); - test("treats a record whose time makes no sense as not started recently", () => { - const broken = markRunning( - newRunRecord("broken", [], "/repo", "not a time"), - 3, - "not a time", - ); + const ended = markChildEnded(running, "2026-07-09T12:36:00.000Z"); - expect( - runStartedRecently(broken, new Date("2026-07-10T12:00:00.000Z")), - ).toBe(false); + // The id may be somebody else's at any moment now, so nothing — + // --kill above all — may keep treating it as this run's child. + expect(ended.pid).toBeUndefined(); + expect(ended.status).toBe("running"); + expect(ended.updatedAt).toBe("2026-07-09T12:36:00.000Z"); }); - test("treats a run stamped in the future as not recent", () => { - // A clock put back must not leave a dead run looking busy for ever. - const future = markRunning( - newRunRecord("future", [], "/repo", "2027-01-01T00:00:00.000Z"), - 5, - "2027-01-01T00:00:00.000Z", - ); - - expect( - runStartedRecently(future, new Date("2026-07-10T12:00:00.000Z")), - ).toBe(false); - }); - - test("counts a run stamped a moment after the epoch", () => { - const justAfterEpoch = markRunning( - newRunRecord("epoch", [], "/repo", "1970-01-01T00:00:00.001Z"), - 4, - "1970-01-01T00:00:00.001Z", - ); - - expect(runStartedRecently(justAfterEpoch, new Date(1), 1000)).toBe(true); + test("maps exit codes to run status", () => { + expect(statusForExitCode(0)).toBe("passed"); + expect(statusForExitCode(130)).toBe("interrupted"); + expect(statusForExitCode(2)).toBe("failed"); }); test("gives each run a plain id with an eight-letter tail", () => { diff --git a/test/scripts/mutation/isolation/commands.test.ts b/test/scripts/mutation/isolation/commands.test.ts index a317085147..6a3ad03420 100644 --- a/test/scripts/mutation/isolation/commands.test.ts +++ b/test/scripts/mutation/isolation/commands.test.ts @@ -2,14 +2,12 @@ import { join } from "node:path"; import { expect } from "@std/expect"; import { describe, it as test } from "@std/testing/bdd"; import { stub } from "@std/testing/mock"; -import { withMutationRunLock } from "#scripts/mutation/isolation-lock.ts"; import { writeRunRecord } from "#scripts/mutation/isolation-records.ts"; import { markFinished, markRunning, newRunRecord, runRoot, - runStartedRecently, } from "#scripts/mutation/isolation-state.ts"; import { captureMutationCommand, @@ -21,6 +19,7 @@ import { runQuietMutationCommand, withTempDir, writeMovedRunRecord, + writeRunClaim, } from "#test/scripts/mutation/isolation-helpers.ts"; import { pathExists } from "#test-utils/files.ts"; @@ -75,24 +74,18 @@ describe("mutation isolation commands", () => { }); }); - test("skips active runs during cleanup", async () => { + test("skips claimed runs during cleanup", async () => { await withTempDir(async (root) => { + // Claimed by a live supervisor, so kept whatever the record says. const copying = newRunRecord(runIdNamed("copying"), [], root); - // Old enough that the startup grace no longer covers it. - const staleCopying = newRunRecord( - runIdNamed("stale-copying"), - [], - root, - LONG_AGO.toISOString(), - ); const running = markRunning( newRunRecord(runIdNamed("running"), [], root), Deno.pid, ); - const starting = markRunning( - newRunRecord(runIdNamed("starting"), [], root), - Deno.pid, - ); + // Records with nobody's claim behind them, so all cleanable — even ones + // written moments ago, and even when the process id in them is alive. + const staleCopying = newRunRecord(runIdNamed("stale-copying"), [], root); + const unclaimed = runningRun("unclaimed", root, Deno.pid); const stale = runningRun("stale", root, 99_999_999); const noPid = runningWithoutPid("nopid", root); const passed = finishedRun("passed", root); @@ -100,46 +93,35 @@ describe("mutation isolation commands", () => { copying, staleCopying, running, - starting, + unclaimed, stale, noPid, passed, ]) { await writeRunRecord(record); } + await writeRunClaim(copying); + await writeRunClaim(running); expect( - await runQuietMutationCommand( - ["--clean", runIdNamed("starting")], - root, - ), + await runQuietMutationCommand(["--clean", runIdNamed("running")], root), ).toBe(1); - await withMutationRunLock(copying.root, async () => { - await withMutationRunLock(running.root, async () => { - expect( - await runQuietMutationCommand( - ["--clean", runIdNamed("running")], - root, - ), - ).toBe(1); - expect(await runQuietMutationCommand(["--clean", "all"], root)).toBe( - 0, - ); - }); - }); + expect(await runQuietMutationCommand(["--clean", "all"], root)).toBe(0); expect(await pathExists(copying.root)).toBe(true); - expect(await pathExists(staleCopying.root)).toBe(false); expect(await pathExists(running.root)).toBe(true); - expect(await pathExists(starting.root)).toBe(true); + expect(await pathExists(staleCopying.root)).toBe(false); + expect(await pathExists(unclaimed.root)).toBe(false); expect(await pathExists(stale.root)).toBe(false); expect(await pathExists(noPid.root)).toBe(false); expect(await pathExists(passed.root)).toBe(false); }); }); - test("cleans stale running records whose pid was reused after the grace period", async () => { + test("cleans a running record whose claim went stale, whatever its pid says", async () => { await withTempDir(async (root) => { + // The supervisor died and its process id has since been handed to a + // live process; the walked-away claim is what tells the truth. const staleReused = markRunning( newRunRecord( runIdNamed("stale-reused"), @@ -151,8 +133,7 @@ describe("mutation isolation commands", () => { LONG_AGO.toISOString(), ); await writeRunRecord(staleReused); - - expect(runStartedRecently(staleReused)).toBe(false); + await writeRunClaim(staleReused, LONG_AGO.getTime()); expect( await runQuietMutationCommand( diff --git a/test/scripts/mutation/isolation/copy-back.test.ts b/test/scripts/mutation/isolation/copy-back.test.ts index 167e2a1f7a..390fd5420a 100644 --- a/test/scripts/mutation/isolation/copy-back.test.ts +++ b/test/scripts/mutation/isolation/copy-back.test.ts @@ -2,10 +2,7 @@ import { join } from "node:path"; import { expect } from "@std/expect"; import { describe, it as test } from "@std/testing/bdd"; import { runInSnapshot } from "#scripts/mutation/isolation.ts"; -import { - withCopyBackLock, - withMutationRunLock, -} from "#scripts/mutation/isolation-lock.ts"; +import { withCopyBackLock } from "#scripts/mutation/isolation-lock.ts"; import { captureConsole, withTempDir, @@ -101,74 +98,53 @@ ${REWRITE_KEPT}`; }); }); -/** Each lock a finishing run waits for, and how a test can hold it first. */ -const FINISHING_LOCKS: { - hold: ( - root: string, - runRoot: string, - body: () => Promise, - ) => Promise; - name: string; -}[] = [ - { - hold: (_root, runRoot, body) => withMutationRunLock(runRoot, body), - name: "the run's own lock", - }, - { - hold: (root, _runRoot, body) => withCopyBackLock(root, body), - name: "the copy-back lock", - }, -]; - describe("interrupting a run as it finishes", () => { - for (const lock of FINISHING_LOCKS) { - test(`keeps nothing when the signal arrives while waiting for ${lock.name}`, async () => { - await withTempDir(async (root) => { - await writeFakeScript(root, ENTRY, "Deno.exit(0);\n"); - await Deno.writeTextFile(join(root, KEPT), "first\nsecond\n"); - const child = controlledChild(42_431, () => {}); - using _command = stubCommand(child.child); - - await withCapturedStopChild(async (getStopChild) => { - const run = captureConsole(() => - runInSnapshot( - { args: [], copyBack: [KEPT], entryScript: `scripts/${ENTRY}` }, - root, - ), - ); - const started = await waitForRunningRecord(root); - - // Held here, so the run waits for it and the signal lands in that - // wait. The copy is given something worth keeping, so a run that - // carried on regardless would be caught writing it out. - const released = Promise.withResolvers(); - const taken = Promise.withResolvers(); - const holding = lock.hold(root, started.root, async () => { - await Deno.writeTextFile( - join(started.workRoot, KEPT), - "kept from the copy\n", - ); - taken.resolve(); - await released.promise; - }); - // Only once it is really held does the run have to wait for it. - await taken.promise; - child.finish({ code: 0, signal: null, success: true }); - // The signal has to land while the run is inside that wait, not - // before it gets there. - await pause(LONG_ENOUGH_TO_BE_WAITING_MS); - getStopChild()?.(); - released.resolve(); - await holding; - - expect((await run).result).toBe(130); - const finished = await readOnlyRunRecord(root); - expect(finished.status).toBe("interrupted"); - expect(await Deno.readTextFile(join(root, KEPT))).toBe( - "first\nsecond\n", + test("keeps nothing when the signal arrives while waiting for the copy-back lock", async () => { + await withTempDir(async (root) => { + await writeFakeScript(root, ENTRY, "Deno.exit(0);\n"); + await Deno.writeTextFile(join(root, KEPT), "first\nsecond\n"); + const child = controlledChild(42_431, () => {}); + using _command = stubCommand(child.child); + + await withCapturedStopChild(async (getStopChild) => { + const run = captureConsole(() => + runInSnapshot( + { args: [], copyBack: [KEPT], entryScript: `scripts/${ENTRY}` }, + root, + ), + ); + const started = await waitForRunningRecord(root); + + // Held here, so the run waits for it and the signal lands in that + // wait. The copy is given something worth keeping, so a run that + // carried on regardless would be caught writing it out. + const released = Promise.withResolvers(); + const taken = Promise.withResolvers(); + const holding = withCopyBackLock(root, async () => { + await Deno.writeTextFile( + join(started.workRoot, KEPT), + "kept from the copy\n", ); + taken.resolve(); + await released.promise; }); + // Only once it is really held does the run have to wait for it. + await taken.promise; + child.finish({ code: 0, signal: null, success: true }); + // The signal has to land while the run is inside that wait, not + // before it gets there. + await pause(LONG_ENOUGH_TO_BE_WAITING_MS); + getStopChild()?.(); + released.resolve(); + await holding; + + expect((await run).result).toBe(130); + const finished = await readOnlyRunRecord(root); + expect(finished.status).toBe("interrupted"); + expect(await Deno.readTextFile(join(root, KEPT))).toBe( + "first\nsecond\n", + ); }); }); - } + }); }); diff --git a/test/scripts/mutation/isolation/helpers.ts b/test/scripts/mutation/isolation/helpers.ts index 86e60e75f6..fac1538c49 100644 --- a/test/scripts/mutation/isolation/helpers.ts +++ b/test/scripts/mutation/isolation/helpers.ts @@ -1,5 +1,3 @@ -import { existsSync } from "node:fs"; -import { join } from "node:path"; import { expect } from "@std/expect"; import { stub } from "@std/testing/mock"; import { runMutationInSnapshot } from "#scripts/mutation/isolation.ts"; @@ -8,8 +6,8 @@ import { writeRunRecord, } from "#scripts/mutation/isolation-records.ts"; import { - MUTATION_RECORD_FILE, MUTATION_RUNS_DIR, + MUTATION_WORK_DIR, type MutationRunRecord, } from "#scripts/mutation/isolation-state.ts"; import { captureConsole } from "#test/scripts/mutation/isolation-helpers.ts"; @@ -71,11 +69,19 @@ export const failRunningStatusWrite = (): Disposable => (_writes, data) => typeof data === "string" && data.includes('"running"'), ); -/** Refuse every removal, standing in for a busy or read-only run folder. */ -export const failWorkRemoval = (): Disposable => - stub(Deno, "remove", (() => { - throw new Error("work is busy"); +/** Refuse to remove the run's snapshot, standing in for a busy work folder. */ +export const failWorkRemoval = (): Disposable => { + const remove = Deno.remove; + return stub(Deno, "remove", (( + path: string | URL, + options?: Deno.RemoveOptions, + ) => { + if (`${path}`.endsWith(`/${MUTATION_WORK_DIR}`)) { + throw new Error("work is busy"); + } + return remove(path, options); }) as typeof Deno.remove); +}; export const capturePlainSnapshotFailure = async ( root: string, @@ -148,6 +154,12 @@ export const withCapturedStopChild = async ( export type KillCall = { pid: number; signal: Deno.Signal | undefined }; +/** Note every signal sent instead of sending it, so a test can read them. */ +export const recordKillCalls = (calls: KillCall[]): Disposable => + stub(Deno, "kill", ((pid: number, signal?: Deno.Signal) => { + calls.push({ pid, signal }); + }) as typeof Deno.kill); + /** `Deno.Command` is a class, so stubbing it needs a looser view of `Deno`. */ type DenoCommandShim = { Command: (...args: unknown[]) => unknown }; const denoCommand = Deno as unknown as DenoCommandShim; @@ -168,48 +180,6 @@ export const stubCommand = ( return { spawn: () => child }; }); -/** - * Run a snapshot mutation while a `--clean` sweeps its record away in the gap - * before the lock, and report whether the record was there at each point. - */ -export const recordAroundClean = async ( - root: string, -): Promise<{ beforeLock: boolean; atChildStart: boolean }> => { - const realMkdir = Deno.mkdir; - let runDir = ""; - // The run folder is made twice: once to write the record, then again by the - // lock. The second time is the moment the run starts queueing. - const madeRunFolder = new Set(); - const result = { atChildStart: false, beforeLock: false }; - const recordPath = () => join(runDir, MUTATION_RECORD_FILE); - - using _mkdir = stub(Deno, "mkdir", (async ( - path: string | URL, - options?: Deno.MkdirOptions, - ) => { - const isRunFolder = - !runDir && `${path}`.includes(`${MUTATION_RUNS_DIR}/mutation-`); - const isLockMakingIt = isRunFolder && madeRunFolder.has(`${path}`); - if (isRunFolder) madeRunFolder.add(`${path}`); - if (!isLockMakingIt) { - await realMkdir(path, options); - return; - } - runDir = `${path}`; - result.beforeLock = existsSync(recordPath()); - // Stand in for a `--clean`, which takes the whole folder of a run whose - // lock it does not see held. The mkdir below is the lock putting it back. - await Deno.remove(runDir, { recursive: true }); - await realMkdir(path, options); - }) as typeof Deno.mkdir); - using _command = stubCommand(finishedChild(42_427), [], () => { - result.atChildStart = existsSync(recordPath()); - }); - - await captureSimpleSnapshotMutation(root); - return result; -}; - /** A child that has already finished with the given exit code. */ export const finishedChild = (pid: number, code = 0): Deno.ChildProcess => ({ diff --git a/test/scripts/mutation/isolation/interrupts.test.ts b/test/scripts/mutation/isolation/interrupts.test.ts new file mode 100644 index 0000000000..ce52ad960d --- /dev/null +++ b/test/scripts/mutation/isolation/interrupts.test.ts @@ -0,0 +1,169 @@ +import { join } from "node:path"; +import { expect } from "@std/expect"; +import { describe, it as test } from "@std/testing/bdd"; +import { stub } from "@std/testing/mock"; +import { runMutationInSnapshot } from "#scripts/mutation/isolation.ts"; +import { runClaimPath } from "#scripts/mutation/isolation-state.ts"; +import { + captureConsole, + withTempDir, + writeFakeMutationScript, +} from "#test/scripts/mutation/isolation-helpers.ts"; +import { eventually } from "#test/scripts/stale-claim/helpers.ts"; +import { pathExists } from "#test-utils/files.ts"; +import { + captureSimpleSnapshotMutation, + controlledChild, + failRunningStatusWrite, + readOnlyRunRecord, + sendFirstSignalImmediately, + stubCommand, + waitForRunningRecord, + withCapturedStopChild, +} from "./helpers.ts"; + +describe("interrupting a snapshot run", () => { + test("marks the run interrupted when a signal arrives before the child starts", async () => { + await withTempDir(async (root) => { + await writeFakeMutationScript(root, "Deno.exit(0);\n"); + + using _addSignal = sendFirstSignalImmediately(); + + const run = await captureSimpleSnapshotMutation(root); + const record = await readOnlyRunRecord(root); + + expect(run.result).toBe(130); + expect(record.status).toBe("interrupted"); + expect(record.exitCode).toBe(130); + }); + }); + + test("records an interrupted run when snapshot copying fails after a signal", async () => { + await withTempDir(async (root) => { + await Deno.mkdir(join(root, "src")); + await Deno.symlink("missing.ts", join(root, "src", "missing.ts")); + + using _addSignal = sendFirstSignalImmediately(); + + const run = await captureConsole(() => + runMutationInSnapshot(["src/missing.ts", "test/missing.test.ts"], root), + ); + const record = await readOnlyRunRecord(root); + + expect(run.result).toBe(130); + expect(run.errors).toHaveLength(1); + expect(record.status).toBe("interrupted"); + expect(record.exitCode).toBe(130); + }); + }); + + test("gives a stopping child a moment to end before forcing it", async () => { + await withTempDir(async (root) => { + await writeFakeMutationScript(root, "Deno.exit(0);\n"); + + const killCalls: (Deno.Signal | undefined)[] = []; + // The child takes a moment to end, well inside the grace period. + const process = controlledChild(42_425, (signal, finish) => { + killCalls.push(signal); + setTimeout( + () => finish({ code: 143, signal: "SIGTERM", success: false }), + 50, + ); + }); + using _command = stubCommand(process.child); + using _writeTextFile = failRunningStatusWrite(); + + await captureSimpleSnapshotMutation(root); + + // Only the polite signal: it ended on its own, so it was never forced. + expect(killCalls).toEqual([undefined]); + }); + }); + + test("records interrupted when a running child exits after a signal", async () => { + await withTempDir(async (root) => { + await writeFakeMutationScript(root, "await new Promise(() => {});\n"); + + await withCapturedStopChild(async (getStopChild) => { + const run = captureSimpleSnapshotMutation(root); + await waitForRunningRecord(root); + + getStopChild()?.(); + await run; + + const finished = await readOnlyRunRecord(root); + expect(finished.status).toBe("interrupted"); + expect(finished.exitCode).toBe(130); + }); + }); + }); + + test("records interrupted when the signalled child already stopped", async () => { + await withTempDir(async (root) => { + await writeFakeMutationScript(root, "Deno.exit(0);\n"); + + let killCalls = 0; + const process = controlledChild(42_425, () => { + killCalls += 1; + throw new Error("already stopped"); + }); + using _command = stubCommand(process.child); + + await withCapturedStopChild(async (getStopChild) => { + const run = captureSimpleSnapshotMutation(root); + await waitForRunningRecord(root); + + getStopChild()?.(); + process.finish({ code: 0, signal: null, success: true }); + + expect((await run).result).toBe(130); + expect(killCalls).toBe(1); + const finished = await readOnlyRunRecord(root); + expect(finished.status).toBe("interrupted"); + expect(finished.exitCode).toBe(130); + }); + }); + }); + + test("escalates repeated interrupts", async () => { + await withTempDir(async (root) => { + await writeFakeMutationScript(root, "await new Promise(() => {});\n"); + + const exitCodes: number[] = []; + using _exit = stub(Deno, "exit", ((code?: number) => { + exitCodes.push(code ?? 0); + }) as unknown as typeof Deno.exit); + await withCapturedStopChild(async (getStopChild) => { + // The failure is expected and caught from the very start: it can + // land while the test is still watching for the claim to go. + const run = (async () => { + try { + await captureSimpleSnapshotMutation(root); + return null; + } catch (error) { + return error; + } + })(); + const record = await waitForRunningRecord(root); + + const stopChild = getStopChild(); + expect(stopChild).toBeDefined(); + if (!stopChild) return; + stopChild(); + stopChild(); + // The forced stop takes the claim down — under the takers' guard, + // so a child touch mid-write cannot put it back — and then exits: + // the run reads as over at once, not after a whole stale window. + await eventually( + async () => !(await pathExists(runClaimPath({ root: record.root }))), + ); + expect(exitCodes).toEqual([130]); + + // Only because the test stubbed the exit away does the run carry on + // to its release, which finds the claim gone — in production the + // process is over the moment the claim comes down. + expect(String(await run)).toContain("was lost while the work ran"); + }); + }); + }); +}); diff --git a/test/scripts/mutation/isolation/list-and-kill.test.ts b/test/scripts/mutation/isolation/list-and-kill.test.ts index 5396a8b77f..5dda84a3b6 100644 --- a/test/scripts/mutation/isolation/list-and-kill.test.ts +++ b/test/scripts/mutation/isolation/list-and-kill.test.ts @@ -1,7 +1,6 @@ import { expect } from "@std/expect"; import { describe, it as test } from "@std/testing/bdd"; import { stub } from "@std/testing/mock"; -import { withMutationRunLock } from "#scripts/mutation/isolation-lock.ts"; import { writeRunRecord } from "#scripts/mutation/isolation-records.ts"; import { ISOLATION_USAGE, @@ -17,10 +16,12 @@ import { runningWithoutPid, runQuietMutationCommand, withTempDir, + writeRunClaim, } from "#test/scripts/mutation/isolation-helpers.ts"; import { finishedChild, type KillCall, + recordKillCalls, stubCommand, writeRecords, } from "./helpers.ts"; @@ -47,11 +48,13 @@ describe("listing, signalling and cleaning isolated runs", () => { }); }); - test("lists running runs only when their pid and lock are live", async () => { + test("lists running runs only while their claim is fresh", async () => { await withTempDir(async (root) => { - const unlocked = markRunning( + // A live pid is not enough: with nobody's claim on the folder, the + // supervisor is gone and the run reads as stale. + const unclaimed = markRunning( newRunRecord( - runIdNamed("unlocked"), + runIdNamed("unclaimed"), [], root, "2026-07-09T12:04:00.000Z", @@ -77,26 +80,25 @@ describe("listing, signalling and cleaning isolated runs", () => { root, "2026-07-09T12:01:00.000Z", ); - await writeRecords([unlocked, live, stale, copying]); - - await withMutationRunLock(live.root, async () => { - const list = await captureMutationCommand(["list"], root); - - expect(list).toEqual({ - errors: [], - logs: [ - `${runIdNamed("unlocked")} stale pid=${Deno.pid} exit=- work=.mutation-runs/${runIdNamed("unlocked")}/work`, - `${runIdNamed("live")} running pid=${Deno.pid} exit=- work=.mutation-runs/${runIdNamed("live")}/work args=src/live.ts`, - `${runIdNamed("stale")} stale pid=99999999 exit=- work=.mutation-runs/${runIdNamed("stale")}/work`, - `${runIdNamed("copying")} copying pid=- exit=- work=.mutation-runs/${runIdNamed("copying")}/work`, - ], - result: 0, - }); + await writeRecords([unclaimed, live, stale, copying]); + await writeRunClaim(live); + + const list = await captureMutationCommand(["list"], root); + + expect(list).toEqual({ + errors: [], + logs: [ + `${runIdNamed("unclaimed")} stale pid=${Deno.pid} exit=- work=.mutation-runs/${runIdNamed("unclaimed")}/work`, + `${runIdNamed("live")} running pid=${Deno.pid} exit=- work=.mutation-runs/${runIdNamed("live")}/work args=src/live.ts`, + `${runIdNamed("stale")} stale pid=99999999 exit=- work=.mutation-runs/${runIdNamed("stale")}/work`, + `${runIdNamed("copying")} copying pid=- exit=- work=.mutation-runs/${runIdNamed("copying")}/work`, + ], + result: 0, }); }); }); - test("lists stale pid records without shelling out to kill", async () => { + test("lists stale records without shelling anything out", async () => { await withTempDir(async (root) => { const stale = runningRun("stale", root, 99_999_999); await writeRunRecord(stale); @@ -106,7 +108,7 @@ describe("listing, signalling and cleaning isolated runs", () => { const list = await captureMutationCommand(["--list"], root); - // A stale pid is judged from the record alone, with nothing shelled out. + // Staleness is read straight from the claim file, nothing shelled out. expect(starts).toEqual([]); expect(list).toEqual({ errors: [], @@ -118,7 +120,7 @@ describe("listing, signalling and cleaning isolated runs", () => { }); }); - test("signals live runs and reports missing or stale targets", async () => { + test("signals claimed runs and reports missing or stale targets", async () => { await withTempDir(async (root) => { const live = runningRun("live", root, Deno.pid); const noPid = runningWithoutPid("nopid", root); @@ -130,23 +132,22 @@ describe("listing, signalling and cleaning isolated runs", () => { 0, ); await writeRecords([live, noPid, passed, done]); + await writeRunClaim(live); + await writeRunClaim(noPid); + await writeRunClaim(done); const calls: KillCall[] = []; - using _kill = stub(Deno, "kill", ((pid, signal) => { - calls.push({ pid, signal }); - }) as typeof Deno.kill); + using _kill = recordKillCalls(calls); - await withMutationRunLock(live.root, async () => { - expect( - await runQuietMutationCommand(["--kill", runIdNamed("live")], root), - ).toBe(0); - expect( - await runQuietMutationCommand( - ["kill", runIdNamed("live"), "--force"], - root, - ), - ).toBe(0); - }); + expect( + await runQuietMutationCommand(["--kill", runIdNamed("live")], root), + ).toBe(0); + expect( + await runQuietMutationCommand( + ["kill", runIdNamed("live"), "--force"], + root, + ), + ).toBe(0); expect(calls).toEqual([ { pid: Deno.pid, signal: "SIGTERM" }, @@ -169,20 +170,37 @@ describe("listing, signalling and cleaning isolated runs", () => { }); }); - test("reports live runs that cannot be signalled", async () => { + test("never signals a pid its run's supervisor has walked away from", async () => { + await withTempDir(async (root) => { + // The record's pid is alive, but it may have been handed to an + // unrelated program after the supervisor died: with no fresh claim to + // vouch for the run, nothing may be signalled. + const orphaned = runningRun("orphaned", root, Deno.pid); + await writeRunRecord(orphaned); + + const calls: KillCall[] = []; + using _kill = recordKillCalls(calls); + + expect( + await runQuietMutationCommand(["--kill", runIdNamed("orphaned")], root), + ).toBe(1); + expect(calls).toEqual([]); + }); + }); + + test("reports claimed runs that cannot be signalled", async () => { await withTempDir(async (root) => { const live = runningRun("live", root, Deno.pid); await writeRunRecord(live); + await writeRunClaim(live); using _kill = stub(Deno, "kill", (() => { throw new Error("cannot signal"); }) as typeof Deno.kill); - await withMutationRunLock(live.root, async () => { - expect( - await runQuietMutationCommand(["--kill", runIdNamed("live")], root), - ).toBe(1); - }); + expect( + await runQuietMutationCommand(["--kill", runIdNamed("live")], root), + ).toBe(1); }); }); diff --git a/test/scripts/mutation/isolation/snapshot-runs.test.ts b/test/scripts/mutation/isolation/snapshot-runs.test.ts index b07d8ed588..8a53122c08 100644 --- a/test/scripts/mutation/isolation/snapshot-runs.test.ts +++ b/test/scripts/mutation/isolation/snapshot-runs.test.ts @@ -2,9 +2,10 @@ import { join } from "node:path"; import { expect } from "@std/expect"; import { describe, it as test } from "@std/testing/bdd"; import { stub } from "@std/testing/mock"; -import { runMutationInSnapshot } from "#scripts/mutation/isolation.ts"; +import { runIsolatedMutationCommand } from "#scripts/mutation/isolation.ts"; import { writeRunRecord } from "#scripts/mutation/isolation-records.ts"; import { + copyBackLockPath, createRunId, MUTATION_RECORD_FILE, MUTATION_SNAPSHOT_CHILD_ENV, @@ -12,7 +13,6 @@ import { newRunRecord, } from "#scripts/mutation/isolation-state.ts"; import { - captureConsole, captureMutationCommand, withTempDir, writeFakeMutationScript, @@ -28,13 +28,10 @@ import { failWorkRemoval, finishedChild, readOnlyRunRecord, - recordAroundClean, SNAPSHOT_FAILED, - sendFirstSignalImmediately, stubCommand, waitForRecord, waitForRunningRecord, - withCapturedStopChild, } from "./helpers.ts"; describe("running mutation inside a snapshot", () => { @@ -54,11 +51,17 @@ describe("running mutation inside a snapshot", () => { true, ); // The pid is printed so a stray run can be found and stopped by hand. - expect(run.logs).toContain(`Mutation child pid ${record.pid}`); + expect( + run.logs.some((line) => /^Mutation child pid \d+$/.test(line)), + ).toBe(true); expect(record.status).toBe("failed"); expect(record.exitCode).toBe(7); expect(record.args).toEqual(["src/a.ts", join(root, "test/a.test.ts")]); - expect(typeof record.pid).toBe("number"); + // The child is gone by the time the record settles, and its id with it. + expect(record.pid).toBeUndefined(); + // Nothing to bring back, so the shared copy-back lock is never taken — + // a run with no kept files must not queue behind one that has them. + expect(await pathExists(copyBackLockPath(root))).toBe(false); }); }); @@ -136,18 +139,22 @@ describe("running mutation inside a snapshot", () => { }); }); - test("writes the run record before it queues for the lock", async () => { + test("keeps its folder claimed against a clean while the run is going", async () => { await withTempDir(async (root) => { - // A record on disk this early is what makes a queued run findable. - expect((await recordAroundClean(root)).beforeLock).toBe(true); - }); - }); + const child = controlledChild(42_427, () => {}); + using _command = stubCommand(child.child); - test("writes the run record again if a clean removes it while queueing", async () => { - await withTempDir(async (root) => { - // The clean above dropped the record; the run must put it back, or the - // snapshot it is about to make would belong to no run anyone can find. - expect((await recordAroundClean(root)).atChildStart).toBe(true); + const run = captureSimpleSnapshotMutation(root); + const started = await waitForRunningRecord(root); + + // A `--clean` sweeping mid-run finds the folder claimed and leaves it. + expect(await runIsolatedMutationCommand(["--clean", "all"], root)).toBe( + 1, + ); + expect(await pathExists(started.root)).toBe(true); + + child.finish({ code: 0, signal: null, success: true }); + expect((await run).result).toBe(0); }); }); @@ -159,40 +166,6 @@ describe("running mutation inside a snapshot", () => { }); }); - test("marks the run interrupted when a signal arrives before the child starts", async () => { - await withTempDir(async (root) => { - await writeFakeMutationScript(root, "Deno.exit(0);\n"); - - using _addSignal = sendFirstSignalImmediately(); - - const run = await captureSimpleSnapshotMutation(root); - const record = await readOnlyRunRecord(root); - - expect(run.result).toBe(130); - expect(record.status).toBe("interrupted"); - expect(record.exitCode).toBe(130); - }); - }); - - test("records an interrupted run when snapshot copying fails after a signal", async () => { - await withTempDir(async (root) => { - await Deno.mkdir(join(root, "src")); - await Deno.symlink("missing.ts", join(root, "src", "missing.ts")); - - using _addSignal = sendFirstSignalImmediately(); - - const run = await captureConsole(() => - runMutationInSnapshot(["src/missing.ts", "test/missing.test.ts"], root), - ); - const record = await readOnlyRunRecord(root); - - expect(run.result).toBe(130); - expect(run.errors).toHaveLength(1); - expect(record.status).toBe("interrupted"); - expect(record.exitCode).toBe(130); - }); - }); - test("records a failed run when snapshot copying throws a plain value", async () => { await withTempDir(async (root) => { const run = await capturePlainSnapshotFailure(root); @@ -246,97 +219,33 @@ describe("running mutation inside a snapshot", () => { }); }); - test("gives a stopping child a moment to end before forcing it", async () => { + test("publishes the pid-less record the moment the child ends", async () => { await withTempDir(async (root) => { await writeFakeMutationScript(root, "Deno.exit(0);\n"); - - const killCalls: (Deno.Signal | undefined)[] = []; - // The child takes a moment to end, well inside the grace period. - const process = controlledChild(42_425, (signal, finish) => { - killCalls.push(signal); - setTimeout( - () => finish({ code: 143, signal: "SIGTERM", success: false }), - 50, - ); - }); - using _command = stubCommand(process.child); - using _writeTextFile = failRunningStatusWrite(); + const written: { pid?: number; status: string }[] = []; + const writeTextFile = Deno.writeTextFile; + using _write = stub(Deno, "writeTextFile", (( + target: string | URL, + data: string | ReadableStream, + options?: Deno.WriteFileOptions, + ) => { + // Records land in a pending file first, so match by name, not tail. + if ( + `${target}`.includes(MUTATION_RECORD_FILE) && + typeof data === "string" + ) { + written.push(JSON.parse(data)); + } + return writeTextFile(target, data, options); + }) as typeof Deno.writeTextFile); await captureSimpleSnapshotMutation(root); - // Only the polite signal: it ended on its own, so it was never forced. - expect(killCalls).toEqual([undefined]); - }); - }); - - test("records interrupted when a running child exits after a signal", async () => { - await withTempDir(async (root) => { - await writeFakeMutationScript(root, "await new Promise(() => {});\n"); - - await withCapturedStopChild(async (getStopChild) => { - const run = captureSimpleSnapshotMutation(root); - await waitForRunningRecord(root); - - getStopChild()?.(); - await run; - - const finished = await readOnlyRunRecord(root); - expect(finished.status).toBe("interrupted"); - expect(finished.exitCode).toBe(130); - }); - }); - }); - - test("records interrupted when the signalled child already stopped", async () => { - await withTempDir(async (root) => { - await writeFakeMutationScript(root, "Deno.exit(0);\n"); - - let killCalls = 0; - const process = controlledChild(42_425, () => { - killCalls += 1; - throw new Error("already stopped"); - }); - using _command = stubCommand(process.child); - - await withCapturedStopChild(async (getStopChild) => { - const run = captureSimpleSnapshotMutation(root); - await waitForRunningRecord(root); - - getStopChild()?.(); - process.finish({ code: 0, signal: null, success: true }); - - expect((await run).result).toBe(130); - expect(killCalls).toBe(1); - const finished = await readOnlyRunRecord(root); - expect(finished.status).toBe("interrupted"); - expect(finished.exitCode).toBe(130); - }); - }); - }); - - test("escalates repeated interrupts", async () => { - await withTempDir(async (root) => { - await writeFakeMutationScript(root, "await new Promise(() => {});\n"); - - const originalKill = Deno.kill; - using _exit = stub(Deno, "exit", ((code) => { - throw new Error(`exit ${code}`); - }) as typeof Deno.exit); - await withCapturedStopChild(async (getStopChild) => { - const run = captureSimpleSnapshotMutation(root); - const record = await waitForRunningRecord(root); - - expect(getStopChild()).toBeDefined(); - getStopChild()?.(); - expect(() => getStopChild()?.()).toThrow("exit 130"); - - try { - originalKill(record.pid!, "SIGKILL"); - } catch { - // The supervisor may already have stopped it. - } - expect((await run).result).not.toBe(0); - }); + // The pid leaves the written record the moment the child ends — before + // the kept files come back — so --kill can never trust it late. + const running = written.filter((record) => record.status === "running"); + expect(running.at(0)?.pid).toBeGreaterThan(0); + expect(running.at(-1)?.pid).toBeUndefined(); }); }); diff --git a/test/scripts/mutation/snapshot-child.test.ts b/test/scripts/mutation/snapshot-child.test.ts index 92cf5567c8..6d76db3e7b 100644 --- a/test/scripts/mutation/snapshot-child.test.ts +++ b/test/scripts/mutation/snapshot-child.test.ts @@ -1,24 +1,28 @@ import { join } from "node:path"; import { expect } from "@std/expect"; import { afterEach, describe, it as test } from "@std/testing/bdd"; +import { runClaimIsFresh } from "#scripts/mutation/isolation-lock.ts"; import { MUTATION_RUN_ID_ENV, - MUTATION_RUN_LOCK_FILE, MUTATION_RUN_ROOT_ENV, MUTATION_SNAPSHOT_CHILD_ENV, + MUTATION_SUPERVISOR_PID_ENV, MUTATION_WORK_ROOT_ENV, + runClaimPath, } from "#scripts/mutation/isolation-state.ts"; import { isSnapshotChild, runSnapshotChild, } from "#scripts/mutation/snapshot-child.ts"; import { projectRoot } from "#scripts/project-root.ts"; +import { LONG_AGO } from "#test/scripts/mutation/isolation-helpers.ts"; import { pathExists, withTempDir } from "#test-utils/files.ts"; const CHILD_VARS = [ MUTATION_RUN_ID_ENV, MUTATION_RUN_ROOT_ENV, MUTATION_SNAPSHOT_CHILD_ENV, + MUTATION_SUPERVISOR_PID_ENV, MUTATION_WORK_ROOT_ENV, ]; @@ -26,13 +30,30 @@ const clearChildVars = (): void => { for (const name of CHILD_VARS) Deno.env.delete(name); }; -/** The run values a child started properly would see: it runs in its copy. */ -const setRunVars = (runRoot: string): void => { +/** A pid that is not this process's parent, as a child killed out from + * under its supervisor would see — whoever now holds that pid. */ +const DEAD_SUPERVISOR_PID = 99_999_999; + +/** The run values a child started properly would see: it runs in its copy, + * and the supervisor that spawned it is its parent process. */ +const setRunVars = ( + runRoot: string, + supervisorPid: number = Deno.ppid, +): void => { Deno.env.set(MUTATION_RUN_ID_ENV, "mutation-test"); Deno.env.set(MUTATION_RUN_ROOT_ENV, runRoot); + Deno.env.set(MUTATION_SUPERVISOR_PID_ENV, String(supervisorPid)); Deno.env.set(MUTATION_WORK_ROOT_ENV, projectRoot); }; +/** The supervisor's claim, last touched long ago — as it reads moments + * after a supervisor was killed outright. */ +const writeAgedSupervisorClaim = (runRoot: string): Promise => + Deno.writeTextFile( + runClaimPath({ root: runRoot }), + `the-supervisor\n${LONG_AGO.getTime()}`, + ); + describe("the worker inside a snapshot", () => { afterEach(clearChildVars); @@ -44,14 +65,58 @@ describe("the worker inside a snapshot", () => { expect(isSnapshotChild()).toBe(true); }); - test("holds the run's lock while it works", async () => { + test("keeps the supervisor's claim fresh while it works", async () => { await withTempDir(async (runRoot) => { setRunVars(runRoot); - const lock = join(runRoot, MUTATION_RUN_LOCK_FILE); + await writeAgedSupervisorClaim(runRoot); + + const freshDuringWork = await runSnapshotChild(() => + runClaimIsFresh({ root: runRoot }), + ); + + expect(freshDuringWork).toBe(true); + // Still the supervisor's, and still fresh: the supervisor lives, so + // releasing stays its job. + expect( + (await Deno.readTextFile(runClaimPath({ root: runRoot }))).startsWith( + "the-supervisor", + ), + ).toBe(true); + expect(await runClaimIsFresh({ root: runRoot })).toBe(true); + }); + }); + + test("ages the claim on exit when its supervisor is gone", async () => { + await withTempDir(async (runRoot) => { + setRunVars(runRoot, DEAD_SUPERVISOR_PID); + await writeAgedSupervisorClaim(runRoot); - const held = await runSnapshotChild(() => pathExists(lock)); + await runSnapshotChild(() => Promise.resolve()); - expect(held).toBe(true); + // Nobody is left to release the claim, so the child ages it: the run + // reads as over right away, not after a whole stale window. + expect(await pathExists(runClaimPath({ root: runRoot }))).toBe(true); + expect(await runClaimIsFresh({ root: runRoot })).toBe(false); + expect( + (await Deno.readTextFile(runClaimPath({ root: runRoot }))).startsWith( + "the-supervisor", + ), + ).toBe(true); + }); + }); + + test("refuses to work in a snapshot no claim protects", async () => { + await withTempDir(async (runRoot) => { + setRunVars(runRoot); + let ranAnyway = false; + + await expect( + runSnapshotChild(() => { + ranAnyway = true; + return Promise.resolve(0); + }), + ).rejects.toThrow(); + expect(ranAnyway).toBe(false); }); }); @@ -76,6 +141,31 @@ describe("the worker inside a snapshot", () => { }); }); + test("accepts the smallest real process id as a supervisor", async () => { + await withTempDir(async (runRoot) => { + // Pid 1 is a real process id, so the child must work — whatever this + // test process's own parent happens to be. + setRunVars(runRoot, 1); + await writeAgedSupervisorClaim(runRoot); + + expect(await runSnapshotChild(() => Promise.resolve("worked"))).toBe( + "worked", + ); + }); + }); + + test("refuses to work with a supervisor pid that is not a process id", async () => { + await withTempDir((runRoot) => { + setRunVars(runRoot); + Deno.env.set(MUTATION_SUPERVISOR_PID_ENV, "not-a-pid"); + + expect(() => runSnapshotChild(() => Promise.resolve(0))).toThrow( + "must be a process id", + ); + return Promise.resolve(); + }); + }); + test("refuses to work when the copy it names is not the one it is in", async () => { await withTempDir((runRoot) => { setRunVars(runRoot); diff --git a/test/scripts/not-found.test.ts b/test/scripts/not-found.test.ts new file mode 100644 index 0000000000..af41d596ad --- /dev/null +++ b/test/scripts/not-found.test.ts @@ -0,0 +1,45 @@ +import { join } from "node:path"; +import { expect } from "@std/expect"; +import { describe, it as test } from "@std/testing/bdd"; +import { stub } from "@std/testing/mock"; +import { statNumberOrNull } from "#scripts/not-found.ts"; +import { withTempDir } from "#test-utils/files.ts"; + +const fileSizeOrNull = statNumberOrNull((info) => info.size); + +describe("reading one number from a path's details", () => { + test("answers the number for a file that is there", async () => { + await withTempDir(async (dir) => { + const path = join(dir, "sized.txt"); + await Deno.writeTextFile(path, "12345"); + + expect(await fileSizeOrNull(path)).toBe(5); + }); + }); + + test("answers null for a path with nothing at it", async () => { + await withTempDir(async (dir) => { + expect(await fileSizeOrNull(join(dir, "never-made"))).toBeNull(); + }); + }); + + test("answers null when the filesystem does not keep the number", async () => { + await withTempDir(async (dir) => { + const path = join(dir, "sized.txt"); + await Deno.writeTextFile(path, "12345"); + const unkept = statNumberOrNull(() => undefined); + + expect(await unkept(path)).toBeNull(); + }); + }); + + test("surfaces a disk that cannot be asked at all", async () => { + // Anything other than "nothing there" must not read as a missing number. + using _stat = stub(Deno, "stat", (() => + Promise.reject( + new Deno.errors.PermissionDenied("no access"), + )) as typeof Deno.stat); + + await expect(fileSizeOrNull("/anywhere")).rejects.toThrow("no access"); + }); +}); diff --git a/test/scripts/process.test.ts b/test/scripts/process.test.ts index f3ef9df28c..1fd3d47aa2 100644 --- a/test/scripts/process.test.ts +++ b/test/scripts/process.test.ts @@ -2,7 +2,6 @@ import { expect } from "@std/expect"; import { describe, it as test } from "@std/testing/bdd"; import { INHERIT_STDIO, - processExists, removeTree, runDeno, stopProcess, @@ -75,11 +74,6 @@ describe("script process helpers", () => { expect(result.success).toBe(false); }); - test("checks process liveness without spawning a shell command", () => { - expect(processExists(Deno.pid)).toBe(true); - expect(processExists(99_999_999)).toBe(false); - }); - test("stops a child process gracefully and closes resources", async () => { let closed = false; const process = fakeChild((_signal, finish) => finish()); diff --git a/test/scripts/stale-claim/freshness.test.ts b/test/scripts/stale-claim/freshness.test.ts new file mode 100644 index 0000000000..baf08a6510 --- /dev/null +++ b/test/scripts/stale-claim/freshness.test.ts @@ -0,0 +1,114 @@ +import { expect } from "@std/expect"; +import { describe, it as test } from "@std/testing/bdd"; +import { stub } from "@std/testing/mock"; +import { FakeTime } from "@std/testing/time"; +import { claimIsFresh, withClaimGuard } from "#scripts/stale-claim.ts"; +import { + FRESH_FOR_MS, + LONG_AGO_MS, + SETTINGS, + withClaimDir, + withTestClaim, + writeClaim, +} from "#test/scripts/stale-claim/helpers.ts"; + +describe("asking whether a claim is fresh", () => { + test("a missing claim is nobody's", async () => { + await withClaimDir(async (path) => { + expect(await claimIsFresh(path, FRESH_FOR_MS)).toBe(false); + }); + }); + + test("a held claim is fresh and a walked-away one is not", async () => { + await withClaimDir(async (path) => { + await writeClaim(path, `someone\n${Date.now()}`); + expect(await claimIsFresh(path, FRESH_FOR_MS)).toBe(true); + + await writeClaim(path, `someone\n${LONG_AGO_MS}`); + expect(await claimIsFresh(path, FRESH_FOR_MS)).toBe(false); + }); + }); + + test("a claim stamped in the future is nobody's to trust", async () => { + await withClaimDir(async (path) => { + // A frozen clock, so "this very instant" below is exact, not racy. + using _clock = new FakeTime(); + // A clock put back must not leave a dead owner's claim unstealable + // until the wall clock catches its stamp up. + await writeClaim(path, `someone\n${Date.now() + 5_000}`); + expect(await claimIsFresh(path, FRESH_FOR_MS)).toBe(false); + + // Stamped this very instant, the claim is exactly fresh. + await writeClaim(path, `someone\n${Date.now()}`); + expect(await claimIsFresh(path, FRESH_FOR_MS)).toBe(true); + }); + }); + + test("says so loudly when there is no time at all to judge by", async () => { + await withClaimDir(async (path) => { + await writeClaim(path, "someone\nnot-a-time"); + const stat = Deno.stat; + using _stat = stub(Deno, "stat", (async (target: string | URL) => ({ + ...(await stat(target)), + mtime: null, + })) as typeof Deno.stat); + + // Neither the record nor the filesystem offers a time: guessing + // either way could steal a live claim or keep a dead one for ever. + await expect(claimIsFresh(path, FRESH_FOR_MS)).rejects.toThrow( + "keeps no last-write time", + ); + }); + }); + + test("a claim from before owners were named is judged by its time", async () => { + await withClaimDir(async (path) => { + await writeClaim(path, String(Date.now())); + + expect(await claimIsFresh(path, FRESH_FOR_MS)).toBe(true); + }); + }); + + test("a disk that cannot be read does not read as nobody's claim", async () => { + await withClaimDir(async (path) => { + using _read = stub(Deno, "readTextFile", (() => + Promise.reject( + new Deno.errors.PermissionDenied("no access"), + )) as typeof Deno.readTextFile); + + await expect(claimIsFresh(path, FRESH_FOR_MS)).rejects.toThrow( + "no access", + ); + }); + }); +}); + +describe("holding the takers' guard", () => { + test("keeps a taker at the door until the guard is let go", async () => { + await withClaimDir(async (path) => { + const order: string[] = []; + const inside = Promise.withResolvers(); + const release = Promise.withResolvers(); + + const holding = withClaimGuard(path, async () => { + inside.resolve(); + await release.promise; + order.push("guard let go"); + }); + await inside.promise; + + const taking = withTestClaim( + path, + () => { + order.push("claim taken"); + return Promise.resolve(); + }, + { ...SETTINGS, timeoutMs: 5_000 }, + ); + release.resolve(); + await Promise.all([holding, taking]); + + expect(order).toEqual(["guard let go", "claim taken"]); + }); + }); +}); diff --git a/test/scripts/stale-claim/helpers.ts b/test/scripts/stale-claim/helpers.ts new file mode 100644 index 0000000000..58fa0b28dd --- /dev/null +++ b/test/scripts/stale-claim/helpers.ts @@ -0,0 +1,232 @@ +/** + * Shared fixtures for the stale-claim suites: quick one-try settings, claim + * files written as other holders leave them, and clocks the tests drive. + */ + +import { join } from "node:path"; +// Real time's setTimeout: node's own binding, which a fake clock stubbed onto +// globalThis never touches. +import { setTimeout as realSetTimeout } from "node:timers"; +import { expect } from "@std/expect"; +import { stub } from "@std/testing/mock"; +import { FakeTime } from "@std/testing/time"; +import { withClaim } from "#scripts/stale-claim.ts"; +import { withTempDir } from "#test-utils/files.ts"; + +/** Generous enough that nothing ages out mid-test on a slow machine. */ +const FRESH_FOR_MS = 60_000; + +/** + * One try, touching so rarely that a short test never sees a touch it did not + * ask for: a claim that is not free at once is reported straight away. + */ +const SETTINGS = { + name: "the test claim", + retryMs: 1, + staleMs: FRESH_FOR_MS, + timeoutMs: 0, + touchMs: FRESH_FOR_MS, +}; + +const LONG_AGO_MS = Date.now() - 60 * 60 * 1000; + +const withClaimDir = ( + run: (path: string) => Promise, +): Promise => + withTempDir((dir) => run(join(dir, "job.claim")), { + prefix: "stale-claim-", + }); + +const writeClaim = (path: string, text: string): Promise => + Deno.writeTextFile(path, text); + +const ageFile = async (path: string, at: number): Promise => { + const date = new Date(at); + await Deno.utime(path, date, date); +}; + +/** Hold the claim around `run`, with the quick one-try settings above. */ +const withTestClaim = ( + path: string, + run: () => Promise, + settings = SETTINGS, +): Promise => withClaim(path, settings, run); + +/** Work with nothing to observe, for tests about the claim alone. */ +const doNothing = (): Promise => Promise.resolve(); + +/** Tries to take the held claim, and checks the holder's record survives. */ +const expectClaimRefused = async ( + path: string, + settings = SETTINGS, +): Promise => { + const holdersRecord = await Deno.readTextFile(path); + await expect(withTestClaim(path, doNothing, settings)).rejects.toThrow( + "Timed out waiting for the test claim", + ); + // Had the work run, its release would have removed or replaced the record. + expect(await Deno.readTextFile(path)).toBe(holdersRecord); +}; + +/** Hold the test claim until told to let go, resolving once it is really + * held — so a clock or waiter driven meanwhile is genuinely mid-hold. */ +const holdClaimUntilReleased = async ( + path: string, + settings = SETTINGS, +): Promise<{ holding: Promise; release: () => void }> => { + const taken = Promise.withResolvers(); + const released = Promise.withResolvers(); + const holding = withTestClaim( + path, + () => { + taken.resolve(); + return released.promise; + }, + settings, + ); + await taken.promise; + return { holding, release: () => released.resolve() }; +}; + +/** Count every look at the claim at `path`, letting the reads go through. */ +const countClaimReads = ( + path: string, + counter: { reads: number }, +): Disposable => { + const readTextFile = Deno.readTextFile; + return stub(Deno, "readTextFile", (( + target: string | URL, + options?: Deno.ReadFileOptions, + ) => { + if (`${target}` === path) counter.reads += 1; + return readTextFile(target, options); + }) as typeof Deno.readTextFile); +}; + +/** A real-time pause, for waits a fake clock must not intercept — letting a + * file write that has already started reach the disk. */ +const settle = (): Promise => + new Promise((resolve) => realSetTimeout(resolve, 5)); + +/** Step the fake clock, letting each touch settle before the next fires. */ +const tickBy = async (time: FakeTime, ...steps: number[]): Promise => { + for (const step of steps) { + await time.tickAsync(step); + // A touch is a guarded read-then-write of real files; give it real + // event-loop time to land before the clock moves on. + await settle(); + } +}; + +type FakeHold = { + held: Awaited>; + time: FakeTime; +}; + +/** Hold the claim with a 30ms stale window, on a clock the test drives. + * Dispose the clock with `using` at the call site. */ +const holdOnFakeClock = async ( + path: string, + touchMs = 10, +): Promise => { + const time = new FakeTime(); + const held = await holdClaimUntilReleased(path, { + ...SETTINGS, + staleMs: 30, + touchMs, + }); + return { held, time }; +}; + +/** Break every touch of `path` while `stillFailing` says so. The very first + * write — the one that creates the claim — always goes through. */ +const failClaimTouches = ( + path: string, + stillFailing: () => boolean, +): Disposable => { + const writeTextFile = Deno.writeTextFile; + let writes = 0; + return stub(Deno, "writeTextFile", (( + target: string | URL, + data: string | ReadableStream, + options?: Deno.WriteFileOptions, + ) => { + if (`${target}` === path) { + writes += 1; + if (writes > 1 && stillFailing()) { + return Promise.reject(new Error("the disk went read-only")); + } + } + return writeTextFile(target, data, options); + }) as typeof Deno.writeTextFile); +}; + +/** Hold the claim while its touches fail, on a clock the scenario drives; + * calling `recover` lets touches land again from then on. */ +const holdWhileTouchesFail = async ( + path: string, + scenario: (context: { + held: Awaited>; + recover: () => void; + time: FakeTime; + }) => Promise, + touchMs?: number, +): Promise => { + let recovered = false; + using _write = failClaimTouches(path, () => !recovered); + const { held, time } = await holdOnFakeClock(path, touchMs); + using _clock = time; + await scenario({ + held, + recover: () => { + recovered = true; + }, + time, + }); +}; + +/** Hold the claim on a fake clock, then let a taker's record land over it. + * Dispose the clock with `using` at the call site. */ +const holdThenLoseClaim = async ( + path: string, + touchMs?: number, +): Promise => { + const { held, time } = await holdOnFakeClock(path, touchMs); + await writeClaim(path, `new-owner\n${Date.now()}`); + return { held, takenRecord: await Deno.readTextFile(path), time }; +}; + +/** Wait, in real time, for the look to say yes — for a write already on its + * way to the disk. Gives up loudly rather than waiting for ever. */ +const eventually = async ( + look: () => Promise, + attempts = 400, +): Promise => { + for (let attempt = 0; attempt < attempts; attempt += 1) { + if (await look()) return; + await settle(); + } + throw new Error("The looked-for state never arrived."); +}; + +export { + ageFile, + countClaimReads, + doNothing, + eventually, + expectClaimRefused, + type FakeHold, + FRESH_FOR_MS, + failClaimTouches, + holdClaimUntilReleased, + holdOnFakeClock, + holdThenLoseClaim, + holdWhileTouchesFail, + LONG_AGO_MS, + SETTINGS, + settle, + tickBy, + withClaimDir, + withTestClaim, + writeClaim, +}; diff --git a/test/scripts/stale-claim/losing.test.ts b/test/scripts/stale-claim/losing.test.ts new file mode 100644 index 0000000000..f021672be9 --- /dev/null +++ b/test/scripts/stale-claim/losing.test.ts @@ -0,0 +1,242 @@ +import { expect } from "@std/expect"; +import { describe, it as test } from "@std/testing/bdd"; +import { FakeTime } from "@std/testing/time"; +import { + ageClaimToStale, + claimIsFresh, + keepClaimFresh, + withClaimGuard, +} from "#scripts/stale-claim.ts"; +import { + countClaimReads, + eventually, + FRESH_FOR_MS, + holdOnFakeClock, + holdThenLoseClaim, + holdWhileTouchesFail, + LONG_AGO_MS, + settle, + tickBy, + withClaimDir, + writeClaim, +} from "#test/scripts/stale-claim/helpers.ts"; + +describe("a holder whose touches stop landing or land elsewhere", () => { + test("carries on when every touch fails but the claim stays its own", async () => { + await withClaimDir((path) => + holdWhileTouchesFail(path, async ({ held, time }) => { + // The file ages unwritten for the whole window — but nobody took the + // claim, so no duplicate work can have started and the hold may end + // quietly. + await tickBy(time, 10, 10, 10, 10); + + held.release(); + await held.holding; + await expect(Deno.stat(path)).rejects.toThrow(); + }), + ); + }); + + test("carries on past a failed touch, touching again so the blip heals", async () => { + await withClaimDir((path) => + holdWhileTouchesFail(path, async ({ held, recover, time }) => { + await tickBy(time, 10); + recover(); + + // Step the clock until a healed touch lands: on a slow machine the + // failed touch's own settling can push the next touch a step later, + // so no fixed tick script can say exactly when it fires. Fresh + // against a window this small can only mean a touch after the blip + // — the claim's first write is many steps old by now. + await eventually(async () => { + await tickBy(time, 10); + return claimIsFresh(path, 12); + }); + held.release(); + await held.holding; + }), + ); + }); +}); + +describe("a holder whose claim is taken while it works", () => { + test("stops touching and reports a claim another taker now holds", async () => { + await withClaimDir(async (path) => { + // A taker judged the aged claim abandoned and took it. + const { held, takenRecord, time } = await holdThenLoseClaim(path); + using _clock = time; + + // The next touch finds somebody else's name and must not write. + await tickBy(time, 10, 10); + + expect(await Deno.readTextFile(path)).toBe(takenRecord); + held.release(); + await expect(held.holding).rejects.toThrow("was lost while the work ran"); + // The new owner's record is left exactly as it was. + expect(await Deno.readTextFile(path)).toBe(takenRecord); + }); + }); + + test("reports a claim that vanished mid-run without re-creating it", async () => { + await withClaimDir(async (path) => { + const { held, time } = await holdOnFakeClock(path); + using _clock = time; + // A taker took the aged claim, finished, and released it. + await Deno.remove(path); + + await tickBy(time, 10, 10); + + held.release(); + await expect(held.holding).rejects.toThrow("was lost while the work ran"); + await expect(Deno.stat(path)).rejects.toThrow(); + }); + }); + + test("stops looking at a claim it has learned is lost", async () => { + await withClaimDir(async (path) => { + const { held, time } = await holdThenLoseClaim(path); + using _clock = time; + const counter = { reads: 0 }; + using _read = countClaimReads(path, counter); + + // The touch that discovers the loss is the last look it takes. + await tickBy(time, 10); + await eventually(() => Promise.resolve(counter.reads > 0)); + const readsAtDiscovery = counter.reads; + await tickBy(time, 10, 10, 10); + + expect(counter.reads).toBe(readsAtDiscovery); + held.release(); + await expect(held.holding).rejects.toThrow("was lost while the work ran"); + }); + }); + + test("reports at release a taking no touch was awake to see", async () => { + await withClaimDir(async (path) => { + // Touches far rarer than the hold is long: the only ownership check + // left is the release's own. + const { held, takenRecord, time } = await holdThenLoseClaim(path, 60_000); + using _clock = time; + + await tickBy(time, 40); + + held.release(); + await expect(held.holding).rejects.toThrow("was lost while the work ran"); + expect(await Deno.readTextFile(path)).toBe(takenRecord); + }); + }); +}); + +describe("keeping somebody else's claim fresh for them", () => { + test("touches it from the very first moment, keeping their name on it", async () => { + await withClaimDir(async (path) => { + await writeClaim(path, `their-supervisor\n${LONG_AGO_MS}`); + + const claim = await keepClaimFresh(path, { + staleMs: FRESH_FOR_MS, + touchMs: FRESH_FOR_MS, + }); + expect(claim.ownedBy).toBe("their-supervisor"); + expect(await claimIsFresh(path, FRESH_FOR_MS)).toBe(true); + await claim.stopTouching(); + + // Stopping never removes the claim: it stays theirs to release. + expect( + (await Deno.readTextFile(path)).startsWith("their-supervisor"), + ).toBe(true); + }); + }); + + test("reports a claim taken while it was being kept fresh", async () => { + await withClaimDir(async (path) => { + await writeClaim(path, `their-supervisor\n${Date.now()}`); + using time = new FakeTime(); + const claim = await keepClaimFresh(path, { staleMs: 30, touchMs: 10 }); + await writeClaim(path, `new-owner\n${Date.now()}`); + + await tickBy(time, 10); + + await expect(claim.stopTouching()).rejects.toThrow( + "was lost while the work ran", + ); + // Not the keeper's to touch or remove: the new owner's record stays. + expect((await Deno.readTextFile(path)).startsWith("new-owner")).toBe( + true, + ); + }); + }); + + test("waits at the takers' door before reading who to keep fresh for", async () => { + await withClaimDir(async (path) => { + await writeClaim(path, `their-supervisor\n${Date.now()}`); + // The owner's own touch is mid-write: it holds the guard. Reading the + // claim now could catch a truncated record with no owner in it. + const inside = Promise.withResolvers(); + const letGo = Promise.withResolvers(); + const guarding = withClaimGuard(path, async () => { + inside.resolve(); + await letGo.promise; + }); + await inside.promise; + const counter = { reads: 0 }; + using _read = countClaimReads(path, counter); + + const keeping = keepClaimFresh(path, { + staleMs: FRESH_FOR_MS, + touchMs: FRESH_FOR_MS, + }); + await settle(); + expect(counter.reads).toBe(0); + + letGo.resolve(); + await guarding; + const claim = await keeping; + expect(claim.ownedBy).toBe("their-supervisor"); + await claim.stopTouching(); + }); + }); + + test("refuses a claim that names nobody to keep it fresh for", async () => { + await withClaimDir(async (path) => { + // A record from before owners were named carries a time alone. + await writeClaim(path, String(Date.now())); + + await expect( + keepClaimFresh(path, { staleMs: FRESH_FOR_MS, touchMs: FRESH_FOR_MS }), + ).rejects.toThrow("names no owner"); + }); + }); + + test("refuses when there is no claim there at all", async () => { + await withClaimDir(async (path) => { + // Missing means missing — not the "names no owner" of a real record. + await expect( + keepClaimFresh(path, { staleMs: FRESH_FOR_MS, touchMs: FRESH_FOR_MS }), + ).rejects.toThrow(Deno.errors.NotFound); + }); + }); +}); + +describe("aging a claim for an owner who cannot release it", () => { + test("refuses to age a claim that is no longer that owner's", async () => { + await withClaimDir(async (path) => { + await writeClaim(path, `new-owner\n${Date.now()}`); + + await expect(ageClaimToStale(path, "their-supervisor")).rejects.toThrow( + "was lost while the work ran", + ); + // The taker's record is not the ager's to touch. + expect((await Deno.readTextFile(path)).startsWith("new-owner")).toBe( + true, + ); + }); + }); + + test("refuses to age a claim that has already gone", async () => { + await withClaimDir(async (path) => { + await expect(ageClaimToStale(path, "their-supervisor")).rejects.toThrow( + "was lost while the work ran", + ); + }); + }); +}); diff --git a/test/scripts/stale-claim/releasing.test.ts b/test/scripts/stale-claim/releasing.test.ts new file mode 100644 index 0000000000..fe5f5d855d --- /dev/null +++ b/test/scripts/stale-claim/releasing.test.ts @@ -0,0 +1,129 @@ +import { expect } from "@std/expect"; +import { describe, it as test } from "@std/testing/bdd"; +import { stub } from "@std/testing/mock"; +import { + doNothing, + expectClaimRefused, + holdClaimUntilReleased, + SETTINGS, + withClaimDir, + withTestClaim, + writeClaim, +} from "#test/scripts/stale-claim/helpers.ts"; + +describe("releasing a claim", () => { + test("removes the claim so the next taker finds it free", async () => { + await withClaimDir(async (path) => { + await withTestClaim(path, doNothing); + + await expect(Deno.stat(path)).rejects.toThrow(); + await withTestClaim(path, doNothing); + }); + }); + + test("reports a claim that has since become somebody else's, leaving it", async () => { + await withClaimDir(async (path) => { + await expect( + withTestClaim(path, () => writeClaim(path, `new-owner\n${Date.now()}`)), + ).rejects.toThrow("was lost while the work ran"); + + expect((await Deno.readTextFile(path)).startsWith("new-owner")).toBe( + true, + ); + }); + }); + + test("frees the claim even when the work itself fails", async () => { + await withClaimDir(async (path) => { + await expect( + withTestClaim(path, () => Promise.reject(new Error("the work broke"))), + ).rejects.toThrow("the work broke"); + + await expect(Deno.stat(path)).rejects.toThrow(); + }); + }); + + test("notes a lost claim alongside the work's own failure", async () => { + await withClaimDir(async (path) => { + await expect( + withTestClaim(path, async () => { + await writeClaim(path, `new-owner\n${Date.now()}`); + throw new Error("the work broke"); + }), + ).rejects.toThrow(/the work broke.*was lost while the work ran/s); + + // The new owner's record is not the failed work's to remove. + expect((await Deno.readTextFile(path)).startsWith("new-owner")).toBe( + true, + ); + }); + }); + + test("reports a claim that is already gone as lost", async () => { + await withClaimDir(async (path) => { + await expect( + withTestClaim(path, () => Deno.remove(path)), + ).rejects.toThrow("was lost while the work ran"); + + await expect(Deno.stat(path)).rejects.toThrow(); + }); + }); +}); + +describe("waiting for a claim", () => { + test("does the work once the claim's holder lets go, then frees it", async () => { + await withClaimDir(async (path) => { + const held = await holdClaimUntilReleased(path); + setTimeout(held.release, 20); + + const answer = await withTestClaim( + path, + () => Promise.resolve("worked"), + { ...SETTINGS, timeoutMs: 5_000 }, + ); + + await held.holding; + expect(answer).toBe("worked"); + await expect(Deno.stat(path)).rejects.toThrow(); + }); + }); + + test("pauses between tries instead of asking as fast as it can", async () => { + await withClaimDir(async (path) => { + const held = await holdClaimUntilReleased(path); + let tries = 0; + const open = Deno.open; + using _open = stub(Deno, "open", (( + target: string | URL, + options?: Deno.OpenOptions, + ) => { + if (`${target}` === path && options?.createNew) tries += 1; + return open(target, options); + }) as typeof Deno.open); + + await expectClaimRefused(path, { + ...SETTINGS, + retryMs: 5, + timeoutMs: 60, + }); + + // Sixty milliseconds of waiting is a handful of tries when they pause + // five apart. With no pause between them it would be many thousands. + expect(tries).toBeLessThan(50); + + held.release(); + await held.holding; + }); + }); + + test("gives up by name when the holder never lets go", async () => { + await withClaimDir(async (path) => { + const held = await holdClaimUntilReleased(path); + + await expectClaimRefused(path, { ...SETTINGS, timeoutMs: 20 }); + + held.release(); + await held.holding; + }); + }); +}); diff --git a/test/scripts/stale-claim/taking.test.ts b/test/scripts/stale-claim/taking.test.ts new file mode 100644 index 0000000000..8ba79ac9f9 --- /dev/null +++ b/test/scripts/stale-claim/taking.test.ts @@ -0,0 +1,119 @@ +import { expect } from "@std/expect"; +import { describe, it as test } from "@std/testing/bdd"; +import { stub } from "@std/testing/mock"; +import { + ageFile, + doNothing, + expectClaimRefused, + LONG_AGO_MS, + withClaimDir, + withTestClaim, + writeClaim, +} from "#test/scripts/stale-claim/helpers.ts"; + +describe("taking a claim", () => { + test("takes a free claim, writing who holds it and since when", async () => { + await withClaimDir(async (path) => { + const before = Date.now(); + await withTestClaim(path, async () => { + const [owner, time] = (await Deno.readTextFile(path)).split("\n"); + // A random name and the time of the last touch, nothing else. + expect(owner).toMatch(/^[0-9a-f-]{36}$/); + expect(Number(time)).toBeGreaterThanOrEqual(before); + expect(Number(time)).toBeLessThanOrEqual(Date.now()); + }); + }); + }); + + test("refuses while somebody else's claim is fresh", async () => { + await withClaimDir(async (path) => { + await writeClaim(path, `someone-else\n${Date.now()}`); + + await expectClaimRefused(path); + }); + }); + + test("removes and takes over a claim whose owner walked away", async () => { + await withClaimDir(async (path) => { + await writeClaim(path, `someone-else\n${LONG_AGO_MS}`); + + await withTestClaim(path, async () => { + expect(await Deno.readTextFile(path)).not.toContain("someone-else"); + }); + }); + }); + + test("judges a claim by its file's age when it names no readable time", async () => { + await withClaimDir(async (path) => { + await writeClaim(path, "someone-else\nnot-a-time"); + await ageFile(path, LONG_AGO_MS); + + await withTestClaim(path, doNothing); + }); + }); + + test("keeps a fresh file's claim when its time has nothing after it", async () => { + await withClaimDir(async (path) => { + // A time with a newline and nothing else is not a record we ever write, + // so the file's own age decides — and this file is new. + await writeClaim(path, `${LONG_AGO_MS}\n`); + + await expectClaimRefused(path); + }); + }); + + test("takes over a claim naming a time from the very start of the clock", async () => { + await withClaimDir(async (path) => { + // The file was touched a moment ago, so only reading the time it + // claims — 1970, however unlikely — can tell that it is abandoned. + await writeClaim(path, "someone-else\n1"); + + await withTestClaim(path, doNothing); + }); + }); + + test("leaves a freshly made claim alone while its time is unreadable", async () => { + await withClaimDir(async (path) => { + // A taker between creating the file and writing the record: the file is + // moments old, so it is somebody's claim, not an abandoned one. + await writeClaim(path, ""); + + await expectClaimRefused(path); + }); + }); + + test("surfaces a claim file that cannot be made at all", async () => { + await withClaimDir(async (path) => { + // Only the claim file itself is unmakeable; the takers' guard beside + // it opens fine, so the failure under test is the claim's own. + const open = Deno.open; + using _open = stub(Deno, "open", (( + target: string | URL, + options?: Deno.OpenOptions, + ) => + `${target}` === path + ? Promise.reject(new Deno.errors.PermissionDenied("no access")) + : open(target, options)) as typeof Deno.open); + + await expect(withTestClaim(path, doNothing)).rejects.toThrow("no access"); + }); + }); + + test("takes the claim when a stale one vanishes during the steal", async () => { + await withClaimDir(async (path) => { + await writeClaim(path, `someone-else\n${LONG_AGO_MS}`); + + const remove = Deno.remove; + using _remove = stub(Deno, "remove", (async ( + target: string | URL, + options?: Deno.RemoveOptions, + ) => { + // Another taker clears the stale claim first; ours must carry on. + await remove(target, options).catch(() => {}); + throw new Deno.errors.NotFound("already gone"); + }) as typeof Deno.remove); + + await withTestClaim(path, doNothing); + }); + }); +}); diff --git a/test/scripts/stale-claim/touching.test.ts b/test/scripts/stale-claim/touching.test.ts new file mode 100644 index 0000000000..0e005e2670 --- /dev/null +++ b/test/scripts/stale-claim/touching.test.ts @@ -0,0 +1,133 @@ +import { expect } from "@std/expect"; +import { describe, it as test } from "@std/testing/bdd"; +import { stub } from "@std/testing/mock"; +import { claimIsFresh } from "#scripts/stale-claim.ts"; +import { + countClaimReads, + doNothing, + eventually, + holdOnFakeClock, + settle, + tickBy, + withClaimDir, + withTestClaim, + writeClaim, +} from "#test/scripts/stale-claim/helpers.ts"; + +describe("keeping a claim fresh while it is held", () => { + test("touches the claim on time, so it never reads as walked away", async () => { + await withClaimDir(async (path) => { + const { held, time } = await holdOnFakeClock(path, 5); + using _clock = time; + // Two rounds, so the touch after a touch is proven too — not just the + // first one armed when the claim was taken. + for (let round = 0; round < 2; round += 1) { + // Age the record — keeping its owner — as if the holder had + // stalled for a long while without anyone taking the claim. + const [owner] = (await Deno.readTextFile(path)).split("\n"); + await writeClaim(path, `${owner}\n${Date.now() - 40}`); + expect(await claimIsFresh(path, 25)).toBe(false); + + await time.tickAsync(5); + // The touch's write is real disk work; wait for it to land. + await eventually(() => claimIsFresh(path, 25)); + } + held.release(); + await held.holding; + }); + }); + + test("a touch armed but not yet landed dies with the release", async () => { + await withClaimDir(async (path) => { + const { held, time } = await holdOnFakeClock(path, 2); + using _clock = time; + // A few touches land, so the release meets a rearmed timer rather + // than the very first one. + await tickBy(time, 2, 2, 2, 1); + + held.release(); + await held.holding; + + await expect(Deno.stat(path)).rejects.toThrow(); + // Time for a stray touch to land, if one had survived the release. + await tickBy(time, 25); + await settle(); + await expect(Deno.stat(path)).rejects.toThrow(); + }); + }); + + test("a touch already under way when the release starts is waited out", async () => { + await withClaimDir(async (path) => { + // The second write to the claim — the first touch — is held in flight + // until the test lets it finish. + const writeStarted = Promise.withResolvers(); + const finishWrite = Promise.withResolvers(); + const writeTextFile = Deno.writeTextFile; + let writes = 0; + using _write = stub(Deno, "writeTextFile", (async ( + target: string | URL, + data: string | ReadableStream, + options?: Deno.WriteFileOptions, + ) => { + if (`${target}` === path) { + writes += 1; + if (writes === 2) { + writeStarted.resolve(); + await finishWrite.promise; + } + } + return writeTextFile(target, data, options); + }) as typeof Deno.writeTextFile); + const { held, time } = await holdOnFakeClock(path, 5); + using _clock = time; + await time.tickAsync(5); + await writeStarted.promise; + + held.release(); + let released = false; + const holding = (async () => { + await held.holding; + released = true; + })(); + await settle(); + // Still waiting on the touch in flight: letting go now could leave + // that write landing after the claim was removed. + expect(released).toBe(false); + + finishWrite.resolve(); + await holding; + await expect(Deno.stat(path)).rejects.toThrow(); + }); + }); + + test("the patient wait for a look gives up loudly, never silently", async () => { + // Two attempts, so the giving-up is proven without a real two-second wait. + await expect(eventually(() => Promise.resolve(false), 2)).rejects.toThrow( + "The looked-for state never arrived.", + ); + }); + + test("a timer that fires after the release writes nothing", async () => { + await withClaimDir(async (path) => { + // Capture the touch timers instead of running them, so the test can + // fire one "late" — after the release — the way a real timer can go + // off in the very moment of the stop. + const lateTouches: Array<() => void> = []; + using _timer = stub(globalThis, "setTimeout", ((handler: () => void) => { + lateTouches.push(handler); + return 0; + }) as unknown as typeof setTimeout); + + await withTestClaim(path, doNothing); + const counter = { reads: 0 }; + using _read = countClaimReads(path, counter); + for (const touch of lateTouches) touch(); + + // Real time for anything wrongly started to land, if there was any. + await settle(); + // The late touch bailed before so much as looking at the claim. + expect(counter.reads).toBe(0); + await expect(Deno.stat(path)).rejects.toThrow(); + }); + }); +});