From 071663b9be19b2ccc5d0aa9368b15728ac26145f Mon Sep 17 00:00:00 2001 From: Julien Goux Date: Wed, 12 Aug 2026 15:21:21 +0200 Subject: [PATCH 1/5] chore(stack): resolve deferred managed-layer hygiene (CLI-2174) - extract the shared temp-write + hardlink claim protocol into managed/atomic-claim.ts and reuse it from workspace identity and StateManager, preserving each caller's race semantics - fall back to an exclusive create when the filesystem refuses hardlinks (EPERM/ENOTSUP on exFAT, FAT32, network mounts) - refuse reentrant registry transactions before BEGIN so an inner rollback can never discard the outer transaction's writes - strip comments before scanning for error-class definitions in the telemetry actionability coverage guard Co-Authored-By: Claude Fable 5 --- .../error-actionability-coverage.unit.test.ts | 125 +++++++++++++++++- packages/stack/docs/architecture.md | 12 +- packages/stack/src/StateManager.ts | 28 ++-- .../src/managed-atomic-claim.unit.test.ts | 107 +++++++++++++++ .../src/managed-service.integration.test.ts | 91 +++++++++++++ packages/stack/src/managed/atomic-claim.ts | 78 +++++++++++ packages/stack/src/managed/identity.ts | 42 +++--- packages/stack/src/managed/sqlite.ts | 41 ++++-- 8 files changed, 468 insertions(+), 56 deletions(-) create mode 100644 packages/stack/src/managed-atomic-claim.unit.test.ts create mode 100644 packages/stack/src/managed/atomic-claim.ts diff --git a/apps/cli/src/shared/telemetry/error-actionability-coverage.unit.test.ts b/apps/cli/src/shared/telemetry/error-actionability-coverage.unit.test.ts index 2b556c8f29..ff1171f9bb 100644 --- a/apps/cli/src/shared/telemetry/error-actionability-coverage.unit.test.ts +++ b/apps/cli/src/shared/telemetry/error-actionability-coverage.unit.test.ts @@ -40,6 +40,69 @@ import { const ERROR_DEFINITION_PATTERN = /TaggedError\(\s*"([A-Za-z0-9_]+)"|class\s+[A-Za-z0-9_]+\s+extends\s+[A-Za-z0-9_.]*Error\(\s*"([A-Za-z0-9_]+)"|class\s+([A-Za-z0-9_]+)\s+extends\s+Error\b/gs; +// Removes `//` line comments and `/* */` block comments from a source string +// so the regex-based scan below never mistakes a comment merely mentioning +// `class X extends Error` (or a TaggedError example) for a real definition. +// String and template literals are treated as opaque runs — a `//` or `/*` +// inside `"https://..."` (or a backtick template) must survive untouched. +// Stripped comment bytes are replaced with spaces (newlines are preserved) +// so line numbers and any line-based logic elsewhere stay unaffected. This +// is a compact scanner, not a full tokenizer: it does not special-case regex +// literals or `${...}` interpolation inside template literals. +function stripComments(source: string): string { + let out = ""; + let i = 0; + const n = source.length; + while (i < n) { + const c = source[i]; + const next = source[i + 1]; + + if (c === '"' || c === "'" || c === "`") { + const quote = c; + out += c; + i += 1; + while (i < n) { + const ch = source[i]; + out += ch; + i += 1; + if (ch === "\\" && i < n) { + out += source[i]; + i += 1; + continue; + } + if (ch === quote) break; + } + continue; + } + + if (c === "/" && next === "/") { + out += " "; + i += 2; + while (i < n && source[i] !== "\n") { + out += " "; + i += 1; + } + continue; + } + + if (c === "/" && next === "*") { + out += " "; + i += 2; + while (i < n && !(source[i] === "*" && source[i + 1] === "/")) { + out += source[i] === "\n" ? "\n" : " "; + i += 1; + } + out += " "; + i += 2; + continue; + } + + out += c; + i += 1; + } + return out; +} + function scanErrorTags(root: string): Map> { const tagsByFile = new Map>(); const walk = (dir: string) => { @@ -50,9 +113,9 @@ function scanErrorTags(root: string): Map> { continue; } if (!path.endsWith(".ts") || path.endsWith(".test.ts")) continue; - const tags = [...readFileSync(path, "utf8").matchAll(ERROR_DEFINITION_PATTERN)].map( - (match) => match[1] ?? match[2] ?? match[3] ?? "", - ); + const tags = [ + ...stripComments(readFileSync(path, "utf8")).matchAll(ERROR_DEFINITION_PATTERN), + ].map((match) => match[1] ?? match[2] ?? match[3] ?? ""); if (tags.length > 0) tagsByFile.set(path, tags); } }; @@ -60,6 +123,62 @@ function scanErrorTags(root: string): Map> { return tagsByFile; } +// Extracts the error tags a snippet of source would contribute to the scan, +// mirroring the comment-stripping + matching pipeline `scanErrorTags` runs +// against real files, without touching the filesystem. +function extractErrorTags(source: string): Array { + return [...stripComments(source).matchAll(ERROR_DEFINITION_PATTERN)].map( + (match) => match[1] ?? match[2] ?? match[3] ?? "", + ); +} + +describe("stripComments", () => { + it("removes a line comment mentioning a fake error class", () => { + const source = "// class Fake extends Error\nconst x = 1;"; + expect(extractErrorTags(source)).toEqual([]); + }); + + it("removes a block comment mentioning a fake TaggedError example", () => { + const source = '/* e.g. Data.TaggedError("FakeTag") */\nconst x = 1;'; + expect(extractErrorTags(source)).toEqual([]); + }); + + it("removes a block comment spanning multiple lines", () => { + const source = [ + "/*", + " * class AlsoFake extends Error", + ' * Data.TaggedError("AlsoFakeTag")', + " */", + "const x = 1;", + ].join("\n"); + expect(extractErrorTags(source)).toEqual([]); + }); + + it("keeps a string literal containing `//` intact and still finds a real definition after it", () => { + const source = [ + 'const url = "https://example.com/foo";', + "export class RealError extends Error {}", + ].join("\n"); + expect(extractErrorTags(source)).toEqual(["RealError"]); + }); + + it("keeps a template literal containing `//` intact and still finds a real definition after it", () => { + const source = [ + "const url = `https://example.com/${path}`;", + 'export class TemplateError extends Data.TaggedError("TemplateError") {}', + ].join("\n"); + expect(extractErrorTags(source)).toEqual(["TemplateError"]); + }); + + it("still finds a real definition that follows a comment about a fake one", () => { + const source = [ + "// This looks like a class Fake extends Error but is not", + 'export class RealTaggedError extends Data.TaggedError("RealTaggedError") {}', + ].join("\n"); + expect(extractErrorTags(source)).toEqual(["RealTaggedError"]); + }); +}); + const kindValues = new Set(Object.values(CliErrorKind)); const categoryValues = new Set(Object.values(CliErrorCategory)); const suggestionValues = new Set(Object.values(CliSuggestionType)); diff --git a/packages/stack/docs/architecture.md b/packages/stack/docs/architecture.md index c44640474d..6f8fc19f62 100644 --- a/packages/stack/docs/architecture.md +++ b/packages/stack/docs/architecture.md @@ -314,7 +314,11 @@ That marker protocol is the one place in the managed surface that uses raw `node of the `FileSystem` service the policy layer reclaims stack state through: writing a temporary file, hardlinking it into place, re-reading the winning marker on `EEXIST`, and removing the temporary path is a single indivisible claim, and the hardlink with that `EEXIST` contract is not part of the platform -service's surface. +service's surface. The claim itself is `claimFileAtomically` in `managed/atomic-claim.ts`, shared with +`StateManager`'s single-stack state claim so both settle a race the same way; a filesystem without +hardlinks (`EPERM` or `ENOTSUP`) falls back to an exclusive create, which still decides the race but +publishes without the hardlink's all-or-nothing guarantee. The marker protocol owns what a lost race +means: the identity claim adopts the winning marker, while a claimed stack state is a failure. No mutable runtime state or credential value is stored in that marker. Read-only discovery does not create it. The registry stores only an opaque credential reference, never resolved plaintext @@ -456,7 +460,11 @@ effects: the fiber scheduler preempts at its operation budget, and a fiber parke `COMMIT` would let another fiber `BEGIN IMMEDIATE` on the same connection — SQLite refuses the nested transaction, and either fiber's `COMMIT` could publish the other's writes. Keeping the whole transaction in one JavaScript turn is therefore what makes a partially applied decision -unobservable and keeps interruption from ever landing inside a transaction. +unobservable and keeps interruption from ever landing inside a transaction. Synchrony cannot rule out +the other way two transactions could meet, a decision that re-enters the repository, so the handles +currently inside a transaction are tracked and a re-entering `BEGIN` is refused before it runs: +SQLite has no nested transactions, and unwinding the inner attempt would roll back the outer +decision's writes. The database handle's lifetime is a scope. `sqliteManagedStackRepositoryLayer` acquires the handle with `Effect.acquireRelease`, so opening the file and registering its close are one step nothing can diff --git a/packages/stack/src/StateManager.ts b/packages/stack/src/StateManager.ts index 5b8b40e314..af1c3dbda7 100644 --- a/packages/stack/src/StateManager.ts +++ b/packages/stack/src/StateManager.ts @@ -1,9 +1,8 @@ import { Data, Effect, Layer, Schema, Context } from "effect"; import { FileSystem, Path } from "effect"; import { execFileSync } from "node:child_process"; -import { randomUUID } from "node:crypto"; import { existsSync, rmSync } from "node:fs"; -import { link, unlink, writeFile } from "node:fs/promises"; +import { claimFileAtomically } from "./managed/atomic-claim.ts"; import { AllocatedPortsSchema, type AllocatedPorts } from "./PortAllocator.ts"; import { PartialVersionManifestSchema, @@ -345,27 +344,24 @@ function makeClaim(deps: StateManagerDeps) { const dir = deps.stackDir(state.name); yield* deps.fs.makeDirectory(dir, { recursive: true }); const statePath = deps.stateFile(state.name); - const temporaryPath = `${statePath}.claim-${process.pid}-${randomUUID()}`; - yield* Effect.tryPromise({ - try: async () => { - await writeFile(temporaryPath, encodePrettyJson(encodeStackState(state)), { flag: "wx" }); - try { - await link(temporaryPath, statePath); - } finally { - await unlink(temporaryPath).catch(() => undefined); - } - }, + const outcome = yield* Effect.tryPromise({ + try: () => claimFileAtomically(statePath, encodePrettyJson(encodeStackState(state))), catch: (cause) => new StateClaimError({ name: state.name, path: statePath, - reason: - cause instanceof Error && "code" in cause && cause.code === "EEXIST" - ? "already-claimed" - : "io-error", + reason: "io-error", cause, }), }); + if (outcome === "already-exists") { + return yield* new StateClaimError({ + name: state.name, + path: statePath, + reason: "already-claimed", + cause: undefined, + }); + } }).pipe( Effect.catchTag("PlatformError", (cause) => Effect.fail( diff --git a/packages/stack/src/managed-atomic-claim.unit.test.ts b/packages/stack/src/managed-atomic-claim.unit.test.ts new file mode 100644 index 0000000000..d6c1fca512 --- /dev/null +++ b/packages/stack/src/managed-atomic-claim.unit.test.ts @@ -0,0 +1,107 @@ +import { mkdtempSync, readdirSync, readFileSync, rmSync, statSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, describe, expect, it } from "vitest"; +import { claimFileAtomically } from "./managed/atomic-claim.ts"; + +const temporaryRoots: Array = []; + +afterEach(() => { + for (const root of temporaryRoots.splice(0)) { + rmSync(root, { force: true, recursive: true }); + } +}); + +const makeRoot = (): string => { + const root = mkdtempSync(join(tmpdir(), "atomic-claim-test-")); + temporaryRoots.push(root); + return root; +}; + +const codedError = (code: string): Error => Object.assign(new Error(code), { code }); + +const refusingLink = (code: string) => (): Promise => Promise.reject(codedError(code)); + +const strayTemporaryFiles = (root: string): ReadonlyArray => + readdirSync(root).filter((entry) => entry.includes(".tmp.")); + +describe("atomic file claim", () => { + it("publishes the content when nothing holds the path yet", async () => { + const root = makeRoot(); + const target = join(root, "claim.json"); + + await expect(claimFileAtomically(target, "mine\n", { mode: 0o600 })).resolves.toBe("claimed"); + + expect(readFileSync(target, "utf8")).toBe("mine\n"); + expect(statSync(target).mode & 0o777).toBe(0o600); + expect(strayTemporaryFiles(root)).toEqual([]); + }); + + it("reports a claim someone else already published and leaves it untouched", async () => { + const root = makeRoot(); + const target = join(root, "claim.json"); + writeFileSync(target, "theirs\n"); + + await expect(claimFileAtomically(target, "mine\n")).resolves.toBe("already-exists"); + + expect(readFileSync(target, "utf8")).toBe("theirs\n"); + expect(strayTemporaryFiles(root)).toEqual([]); + }); + + it.each(["EPERM", "ENOTSUP"])( + "claims through an exclusive create where hardlinks refuse with %s", + async (code) => { + const root = makeRoot(); + const target = join(root, "claim.json"); + + await expect( + claimFileAtomically(target, "mine\n", { mode: 0o600, linkFile: refusingLink(code) }), + ).resolves.toBe("claimed"); + + expect(readFileSync(target, "utf8")).toBe("mine\n"); + expect(statSync(target).mode & 0o777).toBe(0o600); + expect(strayTemporaryFiles(root)).toEqual([]); + }, + ); + + it("still settles the race for a loser on a filesystem without hardlinks", async () => { + const root = makeRoot(); + const target = join(root, "claim.json"); + writeFileSync(target, "theirs\n"); + + await expect( + claimFileAtomically(target, "mine\n", { linkFile: refusingLink("EPERM") }), + ).resolves.toBe("already-exists"); + + expect(readFileSync(target, "utf8")).toBe("theirs\n"); + expect(strayTemporaryFiles(root)).toEqual([]); + }); + + it("propagates a publication failure that is neither a lost race nor a missing hardlink", async () => { + const root = makeRoot(); + const target = join(root, "claim.json"); + + await expect( + claimFileAtomically(target, "mine\n", { linkFile: refusingLink("EACCES") }), + ).rejects.toThrow("EACCES"); + + expect(readdirSync(root)).toEqual([]); + }); + + it("names the temporary file from an injected identifier so a run stays reproducible", async () => { + const root = makeRoot(); + const target = join(root, "claim.json"); + const observed: Array = []; + + await claimFileAtomically(target, "mine\n", { + temporaryId: "fixed-id", + linkFile: (existingPath) => { + observed.push(existingPath); + return Promise.reject(codedError("EPERM")); + }, + }); + + expect(observed).toEqual([`${target}.tmp.fixed-id`]); + expect(strayTemporaryFiles(root)).toEqual([]); + }); +}); diff --git a/packages/stack/src/managed-service.integration.test.ts b/packages/stack/src/managed-service.integration.test.ts index 4f4cb8db13..9c9da5cdbb 100644 --- a/packages/stack/src/managed-service.integration.test.ts +++ b/packages/stack/src/managed-service.integration.test.ts @@ -47,6 +47,7 @@ import { } from "./managed/model.ts"; import { createInMemoryManagedStackRepository } from "./managed/repository-memory.ts"; import { ManagedStackRepository, type ManagedStackRepositoryShape } from "./managed/repository.ts"; +import { sqliteManagedStackRepositoryLayer, type ManagedSqliteDatabase } from "./managed/sqlite.ts"; import type { MakeManagedStackServiceOptions, ManagedStackServiceHandle } from "./managed-bun.ts"; import { bunSqliteManagedStackRepositoryLayer, @@ -80,6 +81,53 @@ const openRegistry = async ( }; }; +/** + * An in-memory registry handle that runs `reenterOnce`'s callback the first time + * a decision reads a row, so a test can re-enter the repository from inside a + * transaction the way a mistaken caller would. + */ +const reentrantRegistry = (): { + readonly handle: ManagedSqliteDatabase; + readonly reenterOnce: (reentry: () => void) => void; +} => { + const database = new Database(":memory:"); + let pending: (() => void) | undefined; + const trigger = (): void => { + const reentry = pending; + pending = undefined; + reentry?.(); + }; + return { + reenterOnce: (reentry) => { + pending = reentry; + }, + handle: { + exec(sql) { + database.exec(sql); + }, + prepare(sql) { + const statement = database.query(sql); + return { + run(parameters = []) { + statement.run(...parameters); + }, + get(parameters = []) { + trigger(); + return statement.get(...parameters) ?? undefined; + }, + all(parameters = []) { + trigger(); + return statement.all(...parameters); + }, + }; + }, + close() { + database.close(); + }, + }, + }; +}; + const temporaryRoots: Array = []; afterEach(() => { @@ -2615,6 +2663,49 @@ describe("managed repository and lifecycle", () => { await registry.close(); }); + it("refuses a registry decision that re-enters the repository, keeping its own writes", async () => { + // SQLite has no nested transactions, so a decision that calls back into the + // repository can only lose: the inner `BEGIN` is refused, and unwinding the + // inner attempt would roll back the writes the outer decision has already + // made. The guard therefore refuses before any statement runs. + const root = makeRoot(); + const workspace = makeWorkspace(root); + const identity = (await Effect.runPromise(ensureOrdinaryWorkspaceIdentity(workspace))).identity; + const sqlite = reentrantRegistry(); + const runtime = ManagedRuntime.make(sqliteManagedStackRepositoryLayer(() => sqlite.handle)); + const repository = Context.get(await runtime.context(), ManagedStackRepository); + + let nested: Exit.Exit> | undefined; + sqlite.reenterOnce(() => { + nested = Effect.runSyncExit(repository.listStacks()); + }); + + const stackId = crypto.randomUUID(); + const prepared = runRepo( + repository.prepareOrdinaryStack({ + identity, + canonicalPath: realpathSync(workspace), + locationId: crypto.randomUUID(), + stackId, + stackName: "default", + paths: managedStackPaths(join(root, "managed"), stackId), + operationToken: crypto.randomUUID(), + now: "2026-08-11T00:00:00.000Z", + configuration: {}, + }), + ); + + if (nested === undefined || !Exit.isFailure(nested)) { + throw new Error("Expected the nested decision to be refused"); + } + expect(Cause.pretty(nested.cause)).toContain("A registry transaction is already open"); + expect(prepared.outcome).toBe("create"); + // The refusal never touched the transaction in flight, so the outer + // decision committed and the handle is free for the next one. + expect(runRepo(repository.listStacks()).map((stack) => stack.id)).toEqual([stackId]); + await runtime.dispose(); + }); + it("writes the current schema version into a fresh registry", async () => { const root = makeRoot(); const databasePath = managedRegistryPath(join(root, "fresh")); diff --git a/packages/stack/src/managed/atomic-claim.ts b/packages/stack/src/managed/atomic-claim.ts new file mode 100644 index 0000000000..d30ac86a6f --- /dev/null +++ b/packages/stack/src/managed/atomic-claim.ts @@ -0,0 +1,78 @@ +import { randomUUID } from "node:crypto"; +import { link, unlink, writeFile } from "node:fs/promises"; +import { errorCode } from "./error-code.ts"; + +export type FileClaimOutcome = "claimed" | "already-exists"; + +export interface FileClaimOptions { + /** Mode for the published file; defaults to the process umask. */ + readonly mode?: number; + /** + * Distinguishes one claimant's temporary file from another's; defaults to a + * random UUID. Callers that already draw identifiers from an injected factory + * pass one from there, so a deterministic run stays deterministic. + */ + readonly temporaryId?: string; + /** + * The hardlink step, overridable so a test can drive the hardlink-less + * fallback on a filesystem that does support hardlinks. + */ + readonly linkFile?: (existingPath: string, newPath: string) => Promise; +} + +const createExclusively = async ( + targetPath: string, + content: string, + mode: number | undefined, +): Promise => { + try { + await writeFile(targetPath, content, { flag: "wx", mode }); + return "claimed"; + } catch (error: unknown) { + if (errorCode(error) === "EEXIST") { + return "already-exists"; + } + throw error; + } +}; + +/** + * Publishes `content` at `targetPath` unless a claimant got there first. + * + * The content is written to a sibling temporary file and hardlinked into place, + * because `link` publishes the whole file in one step and refuses an existing + * target: writing `targetPath` directly could crash halfway and publish a + * partial claim, and testing for the file before writing it would lose the very + * race the claim exists to settle. Filesystems without hardlinks — exFAT, + * FAT32, some network mounts — refuse `link` with `EPERM` or `ENOTSUP`; those + * fall back to an exclusive create, which still settles the race but gives up + * the all-or-nothing publish. Any other failure is a real one and propagates. + * + * A `SIGKILL` between the temporary write and its removal strands a + * `.tmp.` sibling. Nothing ever reads those, so a stranded one is junk + * rather than a claim anybody can observe. + */ +export const claimFileAtomically = async ( + targetPath: string, + content: string, + options: FileClaimOptions = {}, +): Promise => { + const linkFile = options.linkFile ?? link; + const temporaryPath = `${targetPath}.tmp.${options.temporaryId ?? randomUUID()}`; + await writeFile(temporaryPath, content, { flag: "wx", mode: options.mode }); + try { + await linkFile(temporaryPath, targetPath); + return "claimed"; + } catch (error: unknown) { + const code = errorCode(error); + if (code === "EEXIST") { + return "already-exists"; + } + if (code !== "EPERM" && code !== "ENOTSUP") { + throw error; + } + return await createExclusively(targetPath, content, options.mode); + } finally { + await unlink(temporaryPath).catch(() => undefined); + } +}; diff --git a/packages/stack/src/managed/identity.ts b/packages/stack/src/managed/identity.ts index 8e9313472d..22ca100cd7 100644 --- a/packages/stack/src/managed/identity.ts +++ b/packages/stack/src/managed/identity.ts @@ -1,7 +1,8 @@ import { randomUUID } from "node:crypto"; -import { link, mkdir, readFile, realpath, stat, unlink, writeFile } from "node:fs/promises"; +import { mkdir, readFile, realpath, stat } from "node:fs/promises"; import { dirname } from "node:path"; import { Effect } from "effect"; +import { claimFileAtomically } from "./atomic-claim.ts"; import { InvalidManagedIdentityError, ORDINARY_WORKSPACE_IDENTITY_VERSION, @@ -117,11 +118,10 @@ export interface EnsureOrdinaryWorkspaceIdentityResult { /** * Claiming a workspace stays one `await` chain rather than an `Effect.gen` - * pipeline: the temporary file, the hardlink that makes the claim atomic, its - * `EEXIST` re-read of the winning marker, and the `finally` that removes the - * temporary path are a single indivisible protocol. Interleaving it with other - * work — or interrupting it between the link and the cleanup — could leave a - * workspace holding a stray temporary marker. + * pipeline: reading the marker, publishing the claim, and re-reading the marker + * a losing claimant must adopt are a single indivisible protocol, and an + * interruption between those steps would leave the caller with an identity no + * workspace agreed to. */ const ensureIdentity = async ( workspacePath: string, @@ -141,25 +141,21 @@ const ensureIdentity = async ( }; await mkdir(dirname(markerPath), { recursive: true }); - const temporaryPath = `${markerPath}.tmp.${createManagedUuid(idFactory, "identity temporary id")}`; - await writeFile(temporaryPath, `${JSON.stringify(identity, null, 2)}\n`, { mode: 0o600 }); - try { - await link(temporaryPath, markerPath); + const outcome = await claimFileAtomically(markerPath, `${JSON.stringify(identity, null, 2)}\n`, { + mode: 0o600, + temporaryId: createManagedUuid(idFactory, "identity temporary id"), + }); + if (outcome === "claimed") { return { identity, created: true, markerPath }; - } catch (error: unknown) { - if (errorCode(error) !== "EEXIST") { - throw error; - } - const winner = await readIdentity(workspacePath); - if (winner === undefined) { - throw new InvalidManagedIdentityError({ - message: "Identity publication raced without a winning marker", - }); - } - return { identity: winner, created: false, markerPath }; - } finally { - await unlink(temporaryPath).catch(() => undefined); } + + const winner = await readIdentity(workspacePath); + if (winner === undefined) { + throw new InvalidManagedIdentityError({ + message: "Identity publication raced without a winning marker", + }); + } + return { identity: winner, created: false, markerPath }; }; export const ensureOrdinaryWorkspaceIdentity = ( diff --git a/packages/stack/src/managed/sqlite.ts b/packages/stack/src/managed/sqlite.ts index 38a7856503..02bff82374 100644 --- a/packages/stack/src/managed/sqlite.ts +++ b/packages/stack/src/managed/sqlite.ts @@ -368,32 +368,49 @@ const commitPreservingCause = (database: ManagedSqliteDatabase): void => { } }; +/** The handles currently between `BEGIN` and `COMMIT` — see {@link runTransaction}. */ +const openTransactions = new WeakSet(); + /** * `BEGIN`, the decision's statements, and `COMMIT` as one synchronous block. * * Atomicity here rests on the drivers being synchronous and the handle being * single-threaded: nothing else can run between the statements, so a partially - * applied decision is never observable and two transactions can never nest on - * the same connection. That only holds while the whole block is one JavaScript - * turn — splitting the boundary across effects would reintroduce a suspension - * point where the fiber scheduler could preempt at its operation budget and let - * another fiber `BEGIN` on this very handle. + * applied decision is never observable. That only holds while the whole block is + * one JavaScript turn — splitting the boundary across effects would reintroduce + * a suspension point where the fiber scheduler could preempt at its operation + * budget and let another fiber `BEGIN` on this very handle. + * + * What synchrony cannot rule out is a decision that re-enters the repository: + * SQLite has no nested transactions, so the inner `BEGIN` would refuse with a + * driver message about the outer one, and unwinding the inner attempt would + * `ROLLBACK` the outer transaction's writes. Reentrancy is a bug in the calling + * code rather than a condition to recover from, so it is refused here — before + * any statement runs, and without touching the transaction already in flight. */ const runTransaction = ( database: ManagedSqliteDatabase, begin: "BEGIN" | "BEGIN IMMEDIATE", run: () => A, ): A => { + if (openTransactions.has(database)) { + throw new Error("A registry transaction is already open on this database handle"); + } database.exec(begin); - let decided: A; + openTransactions.add(database); try { - decided = run(); - } catch (error: unknown) { - rollbackPreservingCause(database); - throw error; + let decided: A; + try { + decided = run(); + } catch (error: unknown) { + rollbackPreservingCause(database); + throw error; + } + commitPreservingCause(database); + return decided; + } finally { + openTransactions.delete(database); } - commitPreservingCause(database); - return decided; }; /** From 02ad6b4d443cf9d057c263279b7087aafde98cba Mon Sep 17 00:00:00 2001 From: Julien Goux Date: Wed, 12 Aug 2026 15:32:20 +0200 Subject: [PATCH 2/5] chore(stack): resolve managed-layer verification follow-ups (CLI-2174 items 7-12) - track facade closure with an explicit flag instead of sniffing "disposed" out of rejection messages - preserve an operation's original failure when the catch-path claim release itself reports interruption - guard the publication poll against a bounded schedule returning None - stop interrupt-only causes from fabricating report entries at the liveness, inspection, and reclamation absorption points - document the claim-before-mask contract gap for future async repositories - run schema migration through the shared runTransaction helper Co-Authored-By: Claude Fable 5 --- packages/stack/docs/architecture.md | 20 ++- .../src/managed-effect.integration.test.ts | 72 ++++++++++ .../src/managed-service.integration.test.ts | 24 ++++ packages/stack/src/managed/create-service.ts | 30 +++-- packages/stack/src/managed/service.ts | 110 +++++++++++---- packages/stack/src/managed/sqlite.ts | 126 +++++++++--------- 6 files changed, 279 insertions(+), 103 deletions(-) diff --git a/packages/stack/docs/architecture.md b/packages/stack/docs/architecture.md index 6f8fc19f62..67e7c2d672 100644 --- a/packages/stack/docs/architecture.md +++ b/packages/stack/docs/architecture.md @@ -480,7 +480,10 @@ the caller's bound on the entire wait and is applied as a timeout around the rep the poll instead of being checked between polls. Both shipped adapters answer synchronously, so a look at the pending row always completes; with an embedder-supplied asynchronous repository that timeout can preempt a look that is still in flight. That is safe — a look has no side effects — but it means the -option bounds the wait, not the number of looks that finish. +option bounds the wait, not the number of looks that finish. The answer the repeat stops on is checked +rather than asserted through a type refinement: a recurrence bound added to that schedule later would +hand back the final still-pending answer, and the check turns that into a defect instead of an +unpublished stack presented as a published one. Interruption is part of the contract, not an afterthought. Provisioning owns a pending row, an operation claim, and the directories it created, so its create path runs under @@ -490,7 +493,16 @@ releases its claim the same way. An interrupted call stays interrupted rather th failure of the work — a caller's own timeout is not a `ManagedStackInitializationError` — and recovery re-raises interruption instead of recording a retained claim or a reconciliation failure that never happened, so the operation the next pass should still recover does not look like one recovery already -gave up on. +gave up on. That rule covers the steps whose exits recovery absorbs one at a time — the liveness probe, +the runtime inspection, the state reclamation — not just the pass as a whole. + +The single deliberate exception is the claim release on a failed operation's way out: it discards +whatever it raises, its own interruption included, because the caller's outcome is the failure the +operation actually suffered and a release reporting interruption would replace it. The mask itself +begins after the pending row and its claim exist, which is sound only because both shipped adapters +decide synchronously and offer no suspension point during that write. An asynchronous embedder +repository interrupted mid-prepare would leave a pending row and a claim nothing compensates, so the +mask has to be extended over row creation before asynchronous repositories become real. An Effect consumer provides the composed layer, which is the primary API: @@ -545,7 +557,9 @@ alongside those interruptions rather than after them, a statement already on its race the close and fail against a closed handle: a caller that closes while work is outstanding must read those rejections as "did not complete", not as evidence about the registry. A call made after `close()` rejects with an `Error` saying the handle is closed, rather than with the runtime's own bare -internal string. The handle is also an `AsyncDisposable`, so +internal string. That diagnosis comes from the handle's own closed state, never from what a rejection +says, so a caller's callback that refuses with a string mentioning disposal still reaches the caller +as itself. The handle is also an `AsyncDisposable`, so `await using service = await createManagedStackService()` closes it on every path out of the block. The facade hands back the very repository the service uses, so an embedder can read the registry without opening a second handle on it. diff --git a/packages/stack/src/managed-effect.integration.test.ts b/packages/stack/src/managed-effect.integration.test.ts index 434228f5aa..cdd78fa1a3 100644 --- a/packages/stack/src/managed-effect.integration.test.ts +++ b/packages/stack/src/managed-effect.integration.test.ts @@ -310,6 +310,78 @@ describe("managed stack Effect surface", () => { }).pipe(Effect.provide(layer)); }); + it.live("keeps a delete's own failure when releasing its claim reports interruption", () => { + // Releasing the claim on the way out is best effort in the strongest sense. + // An embedder repository whose `finishOperation` is cancelled must not turn + // the failure the caller actually suffered into an interruption the caller + // never asked for — the release has no outcome of its own to report. + const root = makeRoot(); + const stateRoot = join(root, "managed"); + const workspace = makeWorkspace(root); + const repository = createInMemoryManagedStackRepository(); + let releaseIsCancelled = false; + const cancelling: ManagedStackRepositoryShape = { + ...repository, + finishOperation: (stackId, operationToken, outcome, at, error) => + releaseIsCancelled + ? Effect.interrupt + : repository.finishOperation(stackId, operationToken, outcome, at, error), + }; + const layer = managedLayer(stateRoot, { repository: cancelling }); + class StopRefused { + readonly _tag = "StopRefused"; + } + return Effect.gen(function* () { + const managed = yield* ManagedStackService; + const { stack } = yield* managed.provisionOrdinaryStack({ workspacePath: workspace }); + yield* managed.updateStack(stack.id, { lifecycle: "running" }); + releaseIsCancelled = true; + + const exit = yield* managed + .deleteStack(stack.id, { stop: () => Effect.fail(new StopRefused()) }) + .pipe(Effect.exit); + + expect(Exit.isFailure(exit) && Cause.hasInterruptsOnly(exit.cause)).toBe(false); + expect(Exit.isFailure(exit) ? Cause.squash(exit.cause) : undefined).toBeInstanceOf( + StopRefused, + ); + expect((yield* managed.inspectStack(stack.id))?.status).toBe("active"); + }).pipe(Effect.provide(layer)); + }); + + it.live("propagates an interrupted runtime inspection instead of retaining the operation", () => { + // The absorbed steps inside a recovery pass follow the same rule as the pass + // itself: an interrupted inspection has no answer about the runtime, so + // retaining the operation on its behalf would report a decision recovery + // never made. + const { workspace, layer } = setupInMemory(); + return Effect.gen(function* () { + const managed = yield* ManagedStackService; + const repository = yield* ManagedStackRepository; + const { stack } = yield* managed.provisionOrdinaryStack({ workspacePath: workspace }); + const claimed = yield* repository.claimOperation({ + token: crypto.randomUUID(), + stackId: stack.id, + kind: "start", + now: "2026-08-11T00:00:00.000Z", + }); + if (!claimed.acquired) { + return yield* Effect.die(new Error("Expected to stage an abandoned operation")); + } + + const exit = yield* managed + .reconcileAbandonedOperations({ inspectRuntime: () => Effect.interrupt }) + .pipe(Effect.exit); + + expect(Exit.isFailure(exit) && Cause.hasInterruptsOnly(exit.cause)).toBe(true); + // The claim survives for the next pass, exactly as it would had the pass + // never looked at it. + expect( + (yield* repository.listActiveOperations()).map((operation) => operation.token), + ).toEqual([claimed.operation.token]); + }).pipe(Effect.provide(layer)); + }); + it.live("propagates an interrupted recovery pass instead of recording it as a failure", () => { // Recovery reports rather than fails, but an interrupted step has no outcome // to report: recording one would mark a stack failed and release a claim on diff --git a/packages/stack/src/managed-service.integration.test.ts b/packages/stack/src/managed-service.integration.test.ts index 9c9da5cdbb..0625f624dd 100644 --- a/packages/stack/src/managed-service.integration.test.ts +++ b/packages/stack/src/managed-service.integration.test.ts @@ -835,6 +835,30 @@ describe("managed service options", () => { await expect(service.listStacks()).rejects.toThrow(/closed/i); }); + it("reports a callback's own rejection as itself even when it mentions disposal", async () => { + // Whether the handle is closed is the handle's own state, never something + // read back out of what a rejection happens to say: a caller's callback that + // refuses with a string mentioning disposal must reach that caller unchanged. + const root = makeRoot(); + const service = await makePersistentService(root); + const { stack } = await service.provisionOrdinaryStack({ workspacePath: makeWorkspace(root) }); + await service.updateStack(stack.id, { lifecycle: "running" }); + + let rejection: unknown; + try { + await service.deleteStack(stack.id, { + stop: () => Promise.reject("the container was disposed"), + }); + } catch (error: unknown) { + rejection = error; + } + + expect(String(rejection)).toContain("the container was disposed"); + expect(String(rejection)).not.toContain("handle is closed"); + expect(await service.inspectStack(stack.id)).toMatchObject({ status: "active" }); + await service.close(); + }); + it("closes a service acquired with await using when its block ends", async () => { const root = makeRoot(); let acquired: ManagedStackServiceHandle | undefined; diff --git a/packages/stack/src/managed/create-service.ts b/packages/stack/src/managed/create-service.ts index fbb110bcf4..719fb8b567 100644 --- a/packages/stack/src/managed/create-service.ts +++ b/packages/stack/src/managed/create-service.ts @@ -122,16 +122,28 @@ const managedStackServiceHandle = async ( const repository = Context.get(context, ManagedStackRepository); /** - * Every method's run, so a call that arrives after `close` is reported as one. - * - * A disposed `ManagedRuntime` answers by dying with a bare string, which would - * reach the caller as a rejection with no name, message, or stack. Anything - * else is the failure itself and passes through untouched. + * Whether this handle has been closed, tracked here rather than read back out + * of the rejection a closed run produces: a disposed `ManagedRuntime` answers + * by dying with a bare string, and deciding from that string's contents would + * misreport a caller's own callback rejecting with a string that happens to + * mention disposal. + */ + let closed = false; + const dispose = (): Promise => { + closed = true; + return runtime.dispose(); + }; + + /** + * Every method's run, so a call that arrives after `close` is reported as one: + * the runtime's bare string reaches the caller as a rejection with no name, + * message, or stack. While the handle is open, every failure is the failure + * itself and passes through untouched. */ const run = (effect: Effect.Effect): Promise => runtime.runPromise(effect).catch((error: unknown) => { - throw typeof error === "string" && error.includes("disposed") - ? new Error(`The managed stack service handle is closed (${error})`) + throw closed + ? new Error(`The managed stack service handle is closed (${String(error)})`) : error; }); @@ -188,8 +200,8 @@ const managedStackServiceHandle = async ( fromCallback(() => shouldPrune(location), isBooleanAnswer), ), ), - close: () => runtime.dispose(), - [Symbol.asyncDispose]: () => runtime.dispose(), + close: dispose, + [Symbol.asyncDispose]: dispose, }; }; diff --git a/packages/stack/src/managed/service.ts b/packages/stack/src/managed/service.ts index 528101464a..8eac3e742a 100644 --- a/packages/stack/src/managed/service.ts +++ b/packages/stack/src/managed/service.ts @@ -279,6 +279,16 @@ const recordUnlessInterrupted = Cause.hasInterruptsOnly(cause) ? Effect.interrupt : record(cause), ); +/** + * The error an absorbed step refused with, for a report entry to carry — or an + * interruption, re-raised before any entry is built. It is the rule + * {@link recordUnlessInterrupted} applies to a whole step, applied where the + * step's exit is inspected instead: an interrupted step has no outcome, so it + * must not become a report entry either way. + */ +const absorbedError = (cause: Cause.Cause): Effect.Effect => + Cause.hasInterruptsOnly(cause) ? Effect.interrupt : Effect.succeed(Cause.squash(cause)); + /** What one look at a stack awaiting publication can refuse to wait for. */ type PublicationPollFailure = ManagedAbandonedOperationError | ManagedStackNotFoundError; @@ -396,6 +406,13 @@ export class ManagedStackService extends Context.Service< recordUnlessInterrupted((cause) => Effect.succeed(dataRetained(Cause.squash(cause)))), ); + /** + * Marks an operation failed as part of a recovery report, answering + * whether the claim was actually released — a claim that could not be + * released is reported, not hidden. Interruption is re-raised, because + * this is a recording site: there is no report to put an interrupted + * step in. + */ const finishOperationBestEffort = ( stackId: string, operationToken: string, @@ -407,6 +424,36 @@ export class ManagedStackService extends Context.Service< recordUnlessInterrupted(() => Effect.succeed(false)), ); + /** + * Releases this call's claim on the way out of a failed operation, then + * re-raises the cause that got here. + * + * The release absorbs everything it can raise, its own interruption + * included: the caller's outcome is the failure the operation suffered, + * and an embedder repository that reports interruption from + * `finishOperation` would otherwise replace that failure with an + * interruption the caller never asked for. That is the opposite of the + * recording sites above, where an interrupted step has no outcome and + * interruption is the only honest answer. + */ + const releasingClaimOnFailure = + (stackId: string, operationToken: string) => + (self: Effect.Effect): Effect.Effect => + Effect.catchCause(self, (cause) => + repository + .finishOperation( + stackId, + operationToken, + "failed", + now(), + String(Cause.squash(cause)), + ) + .pipe( + Effect.catchCause(() => Effect.void), + Effect.flatMap(() => Effect.failCause(cause)), + ), + ); + /** * A concurrent forced recovery can resolve this same operation before * this call closes it out, but only after the delete's own data removal @@ -516,12 +563,9 @@ export class ManagedStackService extends Context.Service< | ManagedStackPublicationTimeoutError > => pollPublication(pending).pipe( - // A refinement, so the answer the repeat stops on is narrowed to the - // published stack. The type argument is explicit because the - // narrowing is lost when the generic guard is inferred here. Effect.repeat({ schedule: publicationPollSchedule, - while: Option.isNone, + while: (answer: Option.Option) => Option.isNone(answer), }), // The timeout is the caller's bound on the whole wait, so it // interrupts the poll rather than being checked between polls. @@ -530,7 +574,21 @@ export class ManagedStackService extends Context.Service< orElse: () => Effect.fail(new ManagedStackPublicationTimeoutError({ stackId: pending.id })), }), - Effect.map((published) => published.value), + // Only an unbounded schedule guarantees the repeat stops on a + // published stack, and this one is unbounded. A recurrence bound + // added later would hand back the final `None` instead, so the + // answer is checked rather than asserted through a refinement: a + // schedule that gave up is a bug in this module, not an outcome a + // caller could act on. + Effect.flatMap((published) => + Option.isNone(published) + ? Effect.die( + new Error( + `The publication poll for ${pending.id} stopped before the stack was published`, + ), + ) + : Effect.succeed(published.value), + ), ); const updateStackRecord = ( @@ -550,11 +608,7 @@ export class ManagedStackService extends Context.Service< Effect.tap(() => repository.finishOperation(stackId, operation.token, "completed", now()), ), - Effect.catchCause((cause) => - finishOperationBestEffort(stackId, operation.token, Cause.squash(cause)).pipe( - Effect.flatMap(() => Effect.failCause(cause)), - ), - ), + releasingClaimOnFailure(stackId, operation.token), ); }); @@ -654,6 +708,17 @@ export class ManagedStackService extends Context.Service< // interrupted: a caller that times out or closes the service must // not leave a pending stack and a leaked directory behind. Only the // provisioning steps are interruptible; the rollback is not. + // + // The mask starts after `prepareOrdinaryStack`, so it covers the row + // this call owns but not the act of creating it. That is sound only + // because every repository this package ships decides synchronously: + // both adapters run the pending row and its claim as one SQLite + // transaction or one in-memory mutation, with no suspension point an + // interruption could land on. An asynchronous embedder repository + // breaks that assumption — interrupted mid-prepare it would leave a + // pending row and a claim nothing compensates — so the mask must be + // extended to cover row creation before async repositories become + // real. `deleteStack`'s claim has the same shape. return yield* Effect.uninterruptibleMask((restore) => restore( Effect.gen(function* () { @@ -780,13 +845,7 @@ export class ManagedStackService extends Context.Service< yield* finishDeleteOperationTolerantly(stackId, operation.token); return deletionResult("delete", tombstoned, dataReclamation); }), - ).pipe( - Effect.catchCause((cause) => - finishOperationBestEffort(stackId, operation.token, Cause.squash(cause)).pipe( - Effect.flatMap(() => Effect.failCause(cause)), - ), - ), - ), + ).pipe(releasingClaimOnFailure(stackId, operation.token)), ); }); @@ -826,11 +885,8 @@ export class ManagedStackService extends Context.Service< if (forcedOperation === undefined && isUsableManagedOwnerPid(operation.ownerPid)) { const alive = yield* Effect.exit(probeProcessAlive(operation.ownerPid)); if (Exit.isFailure(alive)) { - retained.push({ - operation, - reason: "owner-liveness-unknown", - error: Cause.squash(alive.cause), - }); + const error = yield* absorbedError(alive.cause); + retained.push({ operation, reason: "owner-liveness-unknown", error }); return; } if (alive.value) { @@ -858,11 +914,8 @@ export class ManagedStackService extends Context.Service< reconcileOptions.inspectRuntime(stack, operation), ); if (Exit.isFailure(inspected)) { - retained.push({ - operation, - reason: "runtime-inspection-failed", - error: Cause.squash(inspected.cause), - }); + const error = yield* absorbedError(inspected.cause); + retained.push({ operation, reason: "runtime-inspection-failed", error }); return; } if (inspected.value === "unknown") { @@ -889,11 +942,12 @@ export class ManagedStackService extends Context.Service< // failure is the whole report. const removal = yield* Effect.exit(removeStackState(stack)); if (Exit.isFailure(removal)) { + const error = yield* absorbedError(removal.cause); failures.push({ operation, phase: "state-reclamation", operationReleased: true, - error: Cause.squash(removal.cause), + error, }); return; } diff --git a/packages/stack/src/managed/sqlite.ts b/packages/stack/src/managed/sqlite.ts index 02bff82374..cdb25fe7f5 100644 --- a/packages/stack/src/managed/sqlite.ts +++ b/packages/stack/src/managed/sqlite.ts @@ -231,9 +231,69 @@ const rollbackPreservingCause = (database: ManagedSqliteDatabase): void => { } }; -const migrateSchema = (database: ManagedSqliteDatabase): void => { - database.exec("BEGIN IMMEDIATE"); +const commitPreservingCause = (database: ManagedSqliteDatabase): void => { + try { + database.exec("COMMIT"); + } catch (error: unknown) { + rollbackPreservingCause(database); + throw error; + } +}; + +/** The handles currently between `BEGIN` and `COMMIT` — see {@link runTransaction}. */ +const openTransactions = new WeakSet(); + +/** + * `BEGIN`, the decision's statements, and `COMMIT` as one synchronous block. + * + * Atomicity here rests on the drivers being synchronous and the handle being + * single-threaded: nothing else can run between the statements, so a partially + * applied decision is never observable. That only holds while the whole block is + * one JavaScript turn — splitting the boundary across effects would reintroduce + * a suspension point where the fiber scheduler could preempt at its operation + * budget and let another fiber `BEGIN` on this very handle. + * + * What synchrony cannot rule out is a decision that re-enters the repository: + * SQLite has no nested transactions, so the inner `BEGIN` would refuse with a + * driver message about the outer one, and unwinding the inner attempt would + * `ROLLBACK` the outer transaction's writes. Reentrancy is a bug in the calling + * code rather than a condition to recover from, so it is refused here — before + * any statement runs, and without touching the transaction already in flight. + */ +const runTransaction = ( + database: ManagedSqliteDatabase, + begin: "BEGIN" | "BEGIN IMMEDIATE", + run: () => A, +): A => { + if (openTransactions.has(database)) { + throw new Error("A registry transaction is already open on this database handle"); + } + database.exec(begin); + openTransactions.add(database); try { + let decided: A; + try { + decided = run(); + } catch (error: unknown) { + rollbackPreservingCause(database); + throw error; + } + commitPreservingCause(database); + return decided; + } finally { + openTransactions.delete(database); + } +}; + +/** + * The schema migration is a registry decision like any other, so it runs through + * {@link runTransaction}: it is the first thing an opened handle does, before the + * repository it initializes exists, so no transaction can be open on the handle + * yet. An already-current registry returns without writing and the transaction + * commits nothing. + */ +const migrateSchema = (database: ManagedSqliteDatabase): void => + runTransaction(database, "BEGIN IMMEDIATE", () => { const versionRow = database.prepare("PRAGMA user_version").get(); const version = getNumber(versionRow, "user_version"); if (version !== 0 && version !== MANAGED_REGISTRY_SCHEMA_VERSION) { @@ -243,7 +303,6 @@ const migrateSchema = (database: ManagedSqliteDatabase): void => { }); } if (version === MANAGED_REGISTRY_SCHEMA_VERSION) { - database.exec("COMMIT"); return; } database.exec(` @@ -324,12 +383,7 @@ const migrateSchema = (database: ManagedSqliteDatabase): void => { PRAGMA user_version = ${MANAGED_REGISTRY_SCHEMA_VERSION}; `); - database.exec("COMMIT"); - } catch (error: unknown) { - rollbackPreservingCause(database); - throw error; - } -}; + }); /** * Prepares a freshly opened handle for use as the registry. @@ -359,60 +413,6 @@ const initializeRegistry = ( }); }); -const commitPreservingCause = (database: ManagedSqliteDatabase): void => { - try { - database.exec("COMMIT"); - } catch (error: unknown) { - rollbackPreservingCause(database); - throw error; - } -}; - -/** The handles currently between `BEGIN` and `COMMIT` — see {@link runTransaction}. */ -const openTransactions = new WeakSet(); - -/** - * `BEGIN`, the decision's statements, and `COMMIT` as one synchronous block. - * - * Atomicity here rests on the drivers being synchronous and the handle being - * single-threaded: nothing else can run between the statements, so a partially - * applied decision is never observable. That only holds while the whole block is - * one JavaScript turn — splitting the boundary across effects would reintroduce - * a suspension point where the fiber scheduler could preempt at its operation - * budget and let another fiber `BEGIN` on this very handle. - * - * What synchrony cannot rule out is a decision that re-enters the repository: - * SQLite has no nested transactions, so the inner `BEGIN` would refuse with a - * driver message about the outer one, and unwinding the inner attempt would - * `ROLLBACK` the outer transaction's writes. Reentrancy is a bug in the calling - * code rather than a condition to recover from, so it is refused here — before - * any statement runs, and without touching the transaction already in flight. - */ -const runTransaction = ( - database: ManagedSqliteDatabase, - begin: "BEGIN" | "BEGIN IMMEDIATE", - run: () => A, -): A => { - if (openTransactions.has(database)) { - throw new Error("A registry transaction is already open on this database handle"); - } - database.exec(begin); - openTransactions.add(database); - try { - let decided: A; - try { - decided = run(); - } catch (error: unknown) { - rollbackPreservingCause(database); - throw error; - } - commitPreservingCause(database); - return decided; - } finally { - openTransactions.delete(database); - } -}; - /** * Runs one registry decision inside a transaction. `catchFailure` names the * domain failures the decision raises; anything else is a defect, and either way From f4d015350808951fa4de1b0c2f6363e4d371e97e Mon Sep 17 00:00:00 2001 From: Julien Goux Date: Wed, 12 Aug 2026 16:08:15 +0200 Subject: [PATCH 3/5] test(cli): mask literal contents in the error-actionability scanner Error-looking text inside string and template literals no longer registers as a definition: literal text is blanked before the pattern runs, template interpolations are re-entered as code, and only the double-quoted argument of an Error("Tag") call keeps its content, because the pattern captures tags from that one shape. Co-Authored-By: Claude Fable 5 --- .../error-actionability-coverage.unit.test.ts | 181 ++++++++++++++++-- 1 file changed, 160 insertions(+), 21 deletions(-) diff --git a/apps/cli/src/shared/telemetry/error-actionability-coverage.unit.test.ts b/apps/cli/src/shared/telemetry/error-actionability-coverage.unit.test.ts index ff1171f9bb..bca128fca3 100644 --- a/apps/cli/src/shared/telemetry/error-actionability-coverage.unit.test.ts +++ b/apps/cli/src/shared/telemetry/error-actionability-coverage.unit.test.ts @@ -43,35 +43,85 @@ const ERROR_DEFINITION_PATTERN = // Removes `//` line comments and `/* */` block comments from a source string // so the regex-based scan below never mistakes a comment merely mentioning // `class X extends Error` (or a TaggedError example) for a real definition. -// String and template literals are treated as opaque runs — a `//` or `/*` -// inside `"https://..."` (or a backtick template) must survive untouched. -// Stripped comment bytes are replaced with spaces (newlines are preserved) -// so line numbers and any line-based logic elsewhere stay unaffected. This -// is a compact scanner, not a full tokenizer: it does not special-case regex -// literals or `${...}` interpolation inside template literals. +// String and template literals are masked too — literal TEXT (single-quoted +// string bodies and the literal runs of a template literal) can never +// contain a real definition, so it is blanked to spaces just like a +// comment: `'class Fake extends Error'` must extract nothing, and neither +// must `` `class Fake extends Error` ``. Double-quoted strings get the same +// treatment UNLESS they are the direct argument of an `...Error(` call +// (`TaggedError("Tag")`, `CliError("Tag")`, ...) — that is the one shape +// ERROR_DEFINITION_PATTERN's own capturing groups read their tag out of, so +// masking it would blind the scan to every real definition. Template-literal +// INTERPOLATIONS (`${...}`) are the other exception — they are live code, so +// they are scanned recursively as code again (comments stripped, nested +// strings/templates masked, further nested interpolations handled the same +// way), with brace depth tracked so the interpolation's own `{`/`}` don't get +// confused with the `}` that closes it. Stripped/masked bytes are replaced +// with spaces (newlines are preserved) so line numbers and any line-based +// logic elsewhere stay unaffected. This is a compact scanner, not a full +// tokenizer: it does not special-case regex literals. +// +// A double-quoted string counts as an `...Error(` call argument when the +// (bounded) text immediately preceding its opening quote ends with +// `Error(` — optionally followed by whitespace, mirroring the `\(\s*"` the +// pattern itself requires before the tag. +const ERROR_CALL_ARGUMENT_LOOKBEHIND = /Error\(\s*$/; +const ERROR_CALL_ARGUMENT_LOOKBEHIND_WINDOW = 120; function stripComments(source: string): string { + return scanCode(source, 0, false).out; +} + +// Scans a run of *code* starting at `start`. When `stopAtInterpolationClose` +// is true, this call represents the inside of a `${...}` and returns as soon +// as it sees the matching top-level `}` (braces opened inside this same +// region, e.g. an object literal or block, are tracked and do not trigger an +// early return). `end` is the index of that terminating `}` (or the source +// length if the interpolation was left unterminated). +function scanCode( + source: string, + start: number, + stopAtInterpolationClose: boolean, +): { out: string; end: number } { let out = ""; - let i = 0; + let i = start; const n = source.length; + let braceDepth = 0; while (i < n) { const c = source[i]; const next = source[i + 1]; - if (c === '"' || c === "'" || c === "`") { - const quote = c; + if (stopAtInterpolationClose && c === "}" && braceDepth === 0) { + return { out, end: i }; + } + if (c === "{") { + braceDepth += 1; out += c; i += 1; - while (i < n) { - const ch = source[i]; - out += ch; - i += 1; - if (ch === "\\" && i < n) { - out += source[i]; - i += 1; - continue; - } - if (ch === quote) break; - } + continue; + } + if (c === "}") { + braceDepth -= 1; + out += c; + i += 1; + continue; + } + + if (c === '"' || c === "'") { + const precedingWindow = source.slice( + Math.max(0, i - ERROR_CALL_ARGUMENT_LOOKBEHIND_WINDOW), + i, + ); + const preserveContent = c === '"' && ERROR_CALL_ARGUMENT_LOOKBEHIND.test(precedingWindow); + const string = scanQuotedString(source, i, c, preserveContent); + out += string.out; + i = string.end; + continue; + } + + if (c === "`") { + const template = scanTemplateLiteral(source, i); + out += template.out; + i = template.end; continue; } @@ -100,7 +150,76 @@ function stripComments(source: string): string { out += c; i += 1; } - return out; + return { out, end: i }; +} + +// Scans a single/double-quoted string, keeping the quote characters so +// overall structure survives. Unless `preserveContent` is set (the string is +// the direct argument of an `...Error(` call — see the note above +// `stripComments`), the body is masked to spaces so it can never be +// mistaken for a real definition; an escape sequence (`\x`) is blanked as a +// pair so line/column-preserving length is unaffected either way. +function scanQuotedString( + source: string, + start: number, + quote: string, + preserveContent: boolean, +): { out: string; end: number } { + let out = quote; + let i = start + 1; + const n = source.length; + while (i < n) { + const ch = source[i]; + if (ch === "\\" && i + 1 < n) { + out += preserveContent ? source.slice(i, i + 2) : " "; + i += 2; + continue; + } + if (ch === quote) { + out += ch; + i += 1; + break; + } + out += preserveContent ? ch : ch === "\n" ? "\n" : " "; + i += 1; + } + return { out, end: i }; +} + +// Masks the literal-text runs of a template literal while re-entering code +// mode for every `${...}` interpolation, recursively. +function scanTemplateLiteral(source: string, start: number): { out: string; end: number } { + let out = "`"; + let i = start + 1; + const n = source.length; + while (i < n) { + const ch = source[i]; + if (ch === "\\" && i + 1 < n) { + out += " "; + i += 2; + continue; + } + if (ch === "`") { + out += ch; + i += 1; + break; + } + if (ch === "$" && source[i + 1] === "{") { + out += "${"; + const interpolation = scanCode(source, i + 2, true); + out += interpolation.out; + if (interpolation.end < n && source[interpolation.end] === "}") { + out += "}"; + i = interpolation.end + 1; + } else { + i = interpolation.end; + } + continue; + } + out += ch === "\n" ? "\n" : " "; + i += 1; + } + return { out, end: i }; } function scanErrorTags(root: string): Map> { @@ -177,6 +296,26 @@ describe("stripComments", () => { ].join("\n"); expect(extractErrorTags(source)).toEqual(["RealTaggedError"]); }); + + it("masks a double-quoted string literal so a fake class mentioned inside it is not extracted", () => { + const source = 'const x = "class Fake extends Error";'; + expect(extractErrorTags(source)).toEqual([]); + }); + + it("masks a single-quoted string literal so a fake class mentioned inside it is not extracted", () => { + const source = "const x = 'class Fake extends Error';"; + expect(extractErrorTags(source)).toEqual([]); + }); + + it("still strips comments inside a template literal interpolation", () => { + const source = "const x = `${/* class Fake extends Error */ value}`;"; + expect(extractErrorTags(source)).toEqual([]); + }); + + it("still finds a real definition written inside a template literal interpolation", () => { + const source = "const x = `${(class Boom extends Error {}).name}`;"; + expect(extractErrorTags(source)).toEqual(["Boom"]); + }); }); const kindValues = new Set(Object.values(CliErrorKind)); From b9da074901208738a8d6f194d035545339883819 Mon Sep 17 00:00:00 2001 From: Julien Goux Date: Wed, 12 Aug 2026 16:28:39 +0200 Subject: [PATCH 4/5] test: drop implementation-detail unit tests for type- and AST-level guards - rebuild the error-actionability coverage guard on the TypeScript compiler API, deleting the hand-rolled comment/string tokenizer and its self-test suite; discovery is unchanged (119 files, 485 tags) - enforce the managed error-code/tag contract at compile time in model.ts (derived unions + exhaustive array) and delete the reflection-based managed-model unit test - delete the legacy managed-stack unit test, which pinned StateManager internals through a fake filesystem Co-Authored-By: Claude Fable 5 --- apps/cli/package.json | 1 + .../error-actionability-coverage.unit.test.ts | 383 ++++++------------ packages/stack/src/managed-model.unit.test.ts | 123 ------ packages/stack/src/managed-stack.unit.test.ts | 217 ---------- packages/stack/src/managed/model.ts | 47 ++- pnpm-lock.yaml | 37 +- 6 files changed, 188 insertions(+), 620 deletions(-) delete mode 100644 packages/stack/src/managed-model.unit.test.ts delete mode 100644 packages/stack/src/managed-stack.unit.test.ts diff --git a/apps/cli/package.json b/apps/cli/package.json index a3c4aa09e6..0efd97bd4a 100644 --- a/apps/cli/package.json +++ b/apps/cli/package.json @@ -82,6 +82,7 @@ "semantic-release": "^25.0.8", "smol-toml": "^1.7.1", "tldts": "catalog:", + "typescript": "npm:@typescript/typescript6@^6.0.2", "vitest": "catalog:", "yaml": "^2.9.0" }, diff --git a/apps/cli/src/shared/telemetry/error-actionability-coverage.unit.test.ts b/apps/cli/src/shared/telemetry/error-actionability-coverage.unit.test.ts index bca128fca3..f24f937133 100644 --- a/apps/cli/src/shared/telemetry/error-actionability-coverage.unit.test.ts +++ b/apps/cli/src/shared/telemetry/error-actionability-coverage.unit.test.ts @@ -1,5 +1,6 @@ import { readFileSync, readdirSync, statSync } from "node:fs"; import { join, resolve } from "node:path"; +import ts from "typescript"; import { describe, expect, it } from "vitest"; import { CliError } from "effect/unstable/cli"; @@ -31,195 +32,87 @@ import { * forgot to classify". */ -// Matches every way an error class is defined in this workspace: direct -// `Data.TaggedError("Tag")`, any local `*Error(...)` factory whose heritage -// call carries the tag literal (`CliError("Tag")`, `LoginError("Tag")`, ...), -// and plain `extends Error` classes (identified by class name). Error -// factories must therefore be named `Error` to stay guarded — -// which also keeps `Data.TaggedClass` event types out of the scan. -const ERROR_DEFINITION_PATTERN = - /TaggedError\(\s*"([A-Za-z0-9_]+)"|class\s+[A-Za-z0-9_]+\s+extends\s+[A-Za-z0-9_.]*Error\(\s*"([A-Za-z0-9_]+)"|class\s+([A-Za-z0-9_]+)\s+extends\s+Error\b/gs; - -// Removes `//` line comments and `/* */` block comments from a source string -// so the regex-based scan below never mistakes a comment merely mentioning -// `class X extends Error` (or a TaggedError example) for a real definition. -// String and template literals are masked too — literal TEXT (single-quoted -// string bodies and the literal runs of a template literal) can never -// contain a real definition, so it is blanked to spaces just like a -// comment: `'class Fake extends Error'` must extract nothing, and neither -// must `` `class Fake extends Error` ``. Double-quoted strings get the same -// treatment UNLESS they are the direct argument of an `...Error(` call -// (`TaggedError("Tag")`, `CliError("Tag")`, ...) — that is the one shape -// ERROR_DEFINITION_PATTERN's own capturing groups read their tag out of, so -// masking it would blind the scan to every real definition. Template-literal -// INTERPOLATIONS (`${...}`) are the other exception — they are live code, so -// they are scanned recursively as code again (comments stripped, nested -// strings/templates masked, further nested interpolations handled the same -// way), with brace depth tracked so the interpolation's own `{`/`}` don't get -// confused with the `}` that closes it. Stripped/masked bytes are replaced -// with spaces (newlines are preserved) so line numbers and any line-based -// logic elsewhere stay unaffected. This is a compact scanner, not a full -// tokenizer: it does not special-case regex literals. -// -// A double-quoted string counts as an `...Error(` call argument when the -// (bounded) text immediately preceding its opening quote ends with -// `Error(` — optionally followed by whitespace, mirroring the `\(\s*"` the -// pattern itself requires before the tag. -const ERROR_CALL_ARGUMENT_LOOKBEHIND = /Error\(\s*$/; -const ERROR_CALL_ARGUMENT_LOOKBEHIND_WINDOW = 120; -function stripComments(source: string): string { - return scanCode(source, 0, false).out; +// The scan below recognizes every way an error class is defined in this +// workspace: direct `Data.TaggedError("Tag")`, any local `*Error(...)` factory +// whose heritage call carries the tag literal (`CliError("Tag")`, +// `LoginError("Tag")`, ...), and plain `extends Error` classes (identified by +// class name). Error factories must therefore be named `Error` to +// stay guarded — which also keeps `Data.TaggedClass` event types out of the +// scan. It runs on a real TypeScript AST rather than on text, so a definition +// merely *mentioned* in a comment, a string, or a template literal is +// structurally invisible and needs no special casing. + +// The simple name of a call's callee: `TaggedError` for both `TaggedError(...)` +// and `Data.TaggedError(...)`. +function calleeName(expression: ts.Expression): string { + if (ts.isIdentifier(expression)) return expression.text; + if (ts.isPropertyAccessExpression(expression)) return expression.name.text; + return ""; } -// Scans a run of *code* starting at `start`. When `stopAtInterpolationClose` -// is true, this call represents the inside of a `${...}` and returns as soon -// as it sees the matching top-level `}` (braces opened inside this same -// region, e.g. an object literal or block, are tracked and do not trigger an -// early return). `end` is the index of that terminating `}` (or the source -// length if the interpolation was left unterminated). -function scanCode( - source: string, - start: number, - stopAtInterpolationClose: boolean, -): { out: string; end: number } { - let out = ""; - let i = start; - const n = source.length; - let braceDepth = 0; - while (i < n) { - const c = source[i]; - const next = source[i + 1]; - - if (stopAtInterpolationClose && c === "}" && braceDepth === 0) { - return { out, end: i }; - } - if (c === "{") { - braceDepth += 1; - out += c; - i += 1; - continue; - } - if (c === "}") { - braceDepth -= 1; - out += c; - i += 1; - continue; - } +// The value of a plain string literal, seeing through an `as const` assertion +// (`readonly code = "X" as const`). A computed or interpolated string cannot be +// resolved statically, and none exists in this workspace. +function stringLiteralText(expression: ts.Expression | undefined): string | undefined { + const inner = + expression !== undefined && ts.isAsExpression(expression) ? expression.expression : expression; + return inner !== undefined && ts.isStringLiteral(inner) ? inner.text : undefined; +} - if (c === '"' || c === "'") { - const precedingWindow = source.slice( - Math.max(0, i - ERROR_CALL_ARGUMENT_LOOKBEHIND_WINDOW), - i, - ); - const preserveContent = c === '"' && ERROR_CALL_ARGUMENT_LOOKBEHIND.test(precedingWindow); - const string = scanQuotedString(source, i, c, preserveContent); - out += string.out; - i = string.end; - continue; - } +function extendsExpression(node: ts.ClassLikeDeclaration): ts.Expression | undefined { + const clause = node.heritageClauses?.find((c) => c.token === ts.SyntaxKind.ExtendsKeyword); + return clause?.types[0]?.expression; +} - if (c === "`") { - const template = scanTemplateLiteral(source, i); - out += template.out; - i = template.end; - continue; - } +function parse(fileName: string, source: string): ts.SourceFile { + return ts.createSourceFile(fileName, source, ts.ScriptTarget.Latest, false, ts.ScriptKind.TS); +} - if (c === "/" && next === "/") { - out += " "; - i += 2; - while (i < n && source[i] !== "\n") { - out += " "; - i += 1; +// Extracts the error identifiers a source file defines: the tag literal of +// every `class X extends Error("Tag")` heritage call and of every +// free-standing `TaggedError("Tag")` factory call, plus the class name of +// every plain `class X extends Error` (untagged classes are fingerprinted by +// name). A tagged class contributes its tag once — the heritage call is +// claimed by the class rule so the factory rule does not count it again. +function extractErrorTags(source: string, fileName = "scan.ts"): Array { + const tags: Array = []; + const claimed = new Set(); + + const visit = (node: ts.Node): void => { + if (ts.isClassLike(node)) { + const heritage = extendsExpression(node); + if (heritage !== undefined && ts.isCallExpression(heritage)) { + const tag = calleeName(heritage.expression).endsWith("Error") + ? stringLiteralText(heritage.arguments[0]) + : undefined; + if (tag !== undefined) { + tags.push(tag); + claimed.add(heritage); + } + } else if ( + heritage !== undefined && + ts.isIdentifier(heritage) && + heritage.text === "Error" && + node.name !== undefined + ) { + tags.push(node.name.text); } - continue; } - if (c === "/" && next === "*") { - out += " "; - i += 2; - while (i < n && !(source[i] === "*" && source[i + 1] === "/")) { - out += source[i] === "\n" ? "\n" : " "; - i += 1; - } - out += " "; - i += 2; - continue; + if ( + ts.isCallExpression(node) && + !claimed.has(node) && + calleeName(node.expression).endsWith("TaggedError") + ) { + const tag = stringLiteralText(node.arguments[0]); + if (tag !== undefined) tags.push(tag); } - out += c; - i += 1; - } - return { out, end: i }; -} - -// Scans a single/double-quoted string, keeping the quote characters so -// overall structure survives. Unless `preserveContent` is set (the string is -// the direct argument of an `...Error(` call — see the note above -// `stripComments`), the body is masked to spaces so it can never be -// mistaken for a real definition; an escape sequence (`\x`) is blanked as a -// pair so line/column-preserving length is unaffected either way. -function scanQuotedString( - source: string, - start: number, - quote: string, - preserveContent: boolean, -): { out: string; end: number } { - let out = quote; - let i = start + 1; - const n = source.length; - while (i < n) { - const ch = source[i]; - if (ch === "\\" && i + 1 < n) { - out += preserveContent ? source.slice(i, i + 2) : " "; - i += 2; - continue; - } - if (ch === quote) { - out += ch; - i += 1; - break; - } - out += preserveContent ? ch : ch === "\n" ? "\n" : " "; - i += 1; - } - return { out, end: i }; -} + ts.forEachChild(node, visit); + }; -// Masks the literal-text runs of a template literal while re-entering code -// mode for every `${...}` interpolation, recursively. -function scanTemplateLiteral(source: string, start: number): { out: string; end: number } { - let out = "`"; - let i = start + 1; - const n = source.length; - while (i < n) { - const ch = source[i]; - if (ch === "\\" && i + 1 < n) { - out += " "; - i += 2; - continue; - } - if (ch === "`") { - out += ch; - i += 1; - break; - } - if (ch === "$" && source[i + 1] === "{") { - out += "${"; - const interpolation = scanCode(source, i + 2, true); - out += interpolation.out; - if (interpolation.end < n && source[interpolation.end] === "}") { - out += "}"; - i = interpolation.end + 1; - } else { - i = interpolation.end; - } - continue; - } - out += ch === "\n" ? "\n" : " "; - i += 1; - } - return { out, end: i }; + ts.forEachChild(parse(fileName, source), visit); + return tags; } function scanErrorTags(root: string): Map> { @@ -232,9 +125,7 @@ function scanErrorTags(root: string): Map> { continue; } if (!path.endsWith(".ts") || path.endsWith(".test.ts")) continue; - const tags = [ - ...stripComments(readFileSync(path, "utf8")).matchAll(ERROR_DEFINITION_PATTERN), - ].map((match) => match[1] ?? match[2] ?? match[3] ?? ""); + const tags = extractErrorTags(readFileSync(path, "utf8"), path); if (tags.length > 0) tagsByFile.set(path, tags); } }; @@ -242,80 +133,39 @@ function scanErrorTags(root: string): Map> { return tagsByFile; } -// Extracts the error tags a snippet of source would contribute to the scan, -// mirroring the comment-stripping + matching pipeline `scanErrorTags` runs -// against real files, without touching the filesystem. -function extractErrorTags(source: string): Array { - return [...stripComments(source).matchAll(ERROR_DEFINITION_PATTERN)].map( - (match) => match[1] ?? match[2] ?? match[3] ?? "", - ); -} - -describe("stripComments", () => { - it("removes a line comment mentioning a fake error class", () => { - const source = "// class Fake extends Error\nconst x = 1;"; - expect(extractErrorTags(source)).toEqual([]); - }); - - it("removes a block comment mentioning a fake TaggedError example", () => { - const source = '/* e.g. Data.TaggedError("FakeTag") */\nconst x = 1;'; - expect(extractErrorTags(source)).toEqual([]); - }); - - it("removes a block comment spanning multiple lines", () => { - const source = [ - "/*", - " * class AlsoFake extends Error", - ' * Data.TaggedError("AlsoFakeTag")', - " */", - "const x = 1;", - ].join("\n"); - expect(extractErrorTags(source)).toEqual([]); - }); - - it("keeps a string literal containing `//` intact and still finds a real definition after it", () => { +describe("extractErrorTags", () => { + it("finds tagged, factory-tagged and plain error class definitions", () => { const source = [ - 'const url = "https://example.com/foo";', - "export class RealError extends Error {}", + 'export class TaggedThingError extends Data.TaggedError("TaggedThingError") {}', + 'export class FactoryThingError extends CliError("FactoryTag") {}', + "export class PlainThingError extends Error {}", + 'const Base = Data.TaggedError("FreeStandingTag");', ].join("\n"); - expect(extractErrorTags(source)).toEqual(["RealError"]); + expect(extractErrorTags(source)).toEqual([ + "TaggedThingError", + "FactoryTag", + "PlainThingError", + "FreeStandingTag", + ]); }); - it("keeps a template literal containing `//` intact and still finds a real definition after it", () => { + it("ignores definitions that only appear in comments", () => { const source = [ - "const url = `https://example.com/${path}`;", - 'export class TemplateError extends Data.TaggedError("TemplateError") {}', + "// class Fake extends Error", + '/* e.g. Data.TaggedError("FakeTag") */', + "const x = 1;", ].join("\n"); - expect(extractErrorTags(source)).toEqual(["TemplateError"]); + expect(extractErrorTags(source)).toEqual([]); }); - it("still finds a real definition that follows a comment about a fake one", () => { + it("ignores definitions that only appear inside string and template literals", () => { const source = [ - "// This looks like a class Fake extends Error but is not", - 'export class RealTaggedError extends Data.TaggedError("RealTaggedError") {}', + 'const a = "class Fake extends Error";', + 'const b = `Data.TaggedError("FakeTag")`;', + "const c = 'class AlsoFake extends Error';", ].join("\n"); - expect(extractErrorTags(source)).toEqual(["RealTaggedError"]); - }); - - it("masks a double-quoted string literal so a fake class mentioned inside it is not extracted", () => { - const source = 'const x = "class Fake extends Error";'; - expect(extractErrorTags(source)).toEqual([]); - }); - - it("masks a single-quoted string literal so a fake class mentioned inside it is not extracted", () => { - const source = "const x = 'class Fake extends Error';"; expect(extractErrorTags(source)).toEqual([]); }); - - it("still strips comments inside a template literal interpolation", () => { - const source = "const x = `${/* class Fake extends Error */ value}`;"; - expect(extractErrorTags(source)).toEqual([]); - }); - - it("still finds a real definition written inside a template literal interpolation", () => { - const source = "const x = `${(class Boom extends Error {}).name}`;"; - expect(extractErrorTags(source)).toEqual(["Boom"]); - }); }); const kindValues = new Set(Object.values(CliErrorKind)); @@ -461,22 +311,51 @@ describe("workspace package error tags have external adapters", () => { // what keeps the two halves of the contract joined — the (class, tag, code) // triples in the model must agree with the exported map, the code list, and the // code-keyed classification table. -const MANAGED_TAGGED_CLASS_PATTERN = - /class\s+([A-Za-z0-9_]+)\s+extends\s+Data\.TaggedError\(\s*"([A-Za-z0-9_]+)",?\s*\)[\s\S]*?readonly\s+code\s*=\s*"([A-Z0-9_]+)"/g; +interface ManagedErrorClass { + readonly className: string; + readonly tag: string; + readonly code: string; +} + +// Collects the (class, tag, code) triples of every `class X extends +// Data.TaggedError("Tag")` that also declares a string-literal `code` member. +function scanManagedErrorClasses(path: string): Array { + const classes: Array = []; + const visit = (node: ts.Node): void => { + if (ts.isClassDeclaration(node) && node.name !== undefined) { + const heritage = extendsExpression(node); + const tag = + heritage !== undefined && + ts.isCallExpression(heritage) && + calleeName(heritage.expression) === "TaggedError" + ? stringLiteralText(heritage.arguments[0]) + : undefined; + const code = stringLiteralText( + node.members + .filter(ts.isPropertyDeclaration) + .find((member) => ts.isIdentifier(member.name) && member.name.text === "code") + ?.initializer, + ); + if (tag !== undefined && code !== undefined) { + classes.push({ className: node.name.text, tag, code }); + } + } + ts.forEachChild(node, visit); + }; + ts.forEachChild(parse(path, readFileSync(path, "utf8")), visit); + return classes; +} describe("managed registry error codes are classified", () => { it("packages/stack/src/managed/model.ts", () => { const modelPath = resolve(repoRoot, "packages/stack/src/managed/model.ts"); - const matches = [...readFileSync(modelPath, "utf8").matchAll(MANAGED_TAGGED_CLASS_PATTERN)]; - // One match per declared code: a class written in a shape this regex cannot + const scanned = scanManagedErrorClasses(modelPath); + // One class per declared code: a class written in a shape this scan cannot // see would otherwise pass vacuously instead of failing loudly. - expect(matches.length).toBe(MANAGED_ERROR_CODES.length); + expect(scanned.length).toBe(MANAGED_ERROR_CODES.length); const declaredCodes = new Set(MANAGED_ERROR_CODES); const scannedCodes = new Set(); - for (const match of matches) { - const className = match[1] ?? ""; - const tag = match[2] ?? ""; - const code = match[3] ?? ""; + for (const { className, tag, code } of scanned) { scannedCodes.add(code); expect(tag, `${className} is tagged "${tag}" rather than its own export name`).toBe( className, diff --git a/packages/stack/src/managed-model.unit.test.ts b/packages/stack/src/managed-model.unit.test.ts deleted file mode 100644 index c104ca59e0..0000000000 --- a/packages/stack/src/managed-model.unit.test.ts +++ /dev/null @@ -1,123 +0,0 @@ -import { describe, expect, it } from "vitest"; -import * as model from "./managed/model.ts"; -import { - MANAGED_ERROR_CODES, - MANAGED_ERROR_TAG_BY_CODE, - isManagedStackError, -} from "./managed/model.ts"; - -interface ManagedErrorCase { - readonly exportName: string; - readonly error: Error; - readonly code: unknown; - readonly tag: unknown; -} - -/** - * A single fields bag covering every field name declared across the managed - * failures. Each class ignores the members it does not declare, so one probe - * constructs them all: `code` is a field initializer, and only the message - * getters read the fields. Values are supplied rather than left absent so a - * getter that reaches into a structured field (the in-progress operation - * record) stays evaluable; a class introducing a new field name constructs with - * that field missing and fails loudly here rather than silently. - */ -const CONSTRUCTOR_PROBE = { - message: "probe", - found: 0, - supported: 0, - identityId: "probe", - existingClaim: "probe", - requestedClaim: "probe", - stackName: "probe", - ownerPid: 0, - port: 0, - key: "probe", - stackId: "probe", - operation: { kind: "start" }, - ownerStackId: "probe", - path: "probe", - cleanupErrors: [], -}; - -/** - * Every error class exported from `./managed/model.ts`, discovered by - * reflection rather than by hand: the module declares nothing but managed - * failures, so an exported `Error` class that is missing from the code list or - * the tag map must fail here instead of silently classifying as `unknown` - * downstream. Discovery deliberately does not use {@link isManagedStackError} — - * that guard reads the tag map, which is one of the things under test. - */ -const managedErrorCases: ReadonlyArray = Object.entries(model).flatMap( - ([exportName, value]) => { - if (typeof value !== "function") return []; - const prototype: unknown = value.prototype; - if (typeof prototype !== "object" || prototype === null) return []; - if (!(prototype instanceof Error)) return []; - const error: unknown = Reflect.construct(value, [CONSTRUCTOR_PROBE]); - if (!(error instanceof Error)) return []; - return [ - { - exportName, - error, - code: Reflect.get(error, "code"), - tag: Reflect.get(error, "_tag"), - }, - ]; - }, -); - -/** - * Managed failures are `Data.TaggedError` classes: `_tag` is the Effect-native - * discriminant, and `code` is the wire-level contract. Identifier minification - * renames the constructors but touches neither, and the CLI's telemetry - * classifier dispatches on the tag while keying its table by the code - * (`apps/cli/src/shared/telemetry/error-actionability.ts`), so both literals - * and the mapping between them are published contracts rather than - * implementation details. - */ -describe("managed error contract", () => { - it("keeps MANAGED_ERROR_CODES exhaustive against the exported classes", () => { - expect(managedErrorCases.length).toBe(MANAGED_ERROR_CODES.length); - expect(managedErrorCases.map(({ code }) => code).sort()).toEqual( - [...MANAGED_ERROR_CODES].sort(), - ); - }); - - it.each(managedErrorCases)( - "exposes a stable code, tag and class name on $exportName", - (testCase) => { - expect(isManagedStackError(testCase.error)).toBe(true); - expect(typeof testCase.code).toBe("string"); - expect(MANAGED_ERROR_CODES).toContain(testCase.code); - expect(testCase.tag).toBe(testCase.exportName); - // `Data.TaggedError` installs the literal tag as a `name` data property - // on the generated base prototype, so `error.name` keeps reporting the - // class name — including in minified release builds, where - // `constructor.name` is renamed. - expect(testCase.error.name).toBe(testCase.exportName); - }, - ); - - it.each([...MANAGED_ERROR_CODES])("declares %s on exactly one class", (code) => { - expect(managedErrorCases.filter((testCase) => testCase.code === code)).toHaveLength(1); - }); - - it("maps every code to the tag of the class declaring it", () => { - expect(Object.keys(MANAGED_ERROR_TAG_BY_CODE).sort()).toEqual([...MANAGED_ERROR_CODES].sort()); - for (const { code, tag } of managedErrorCases) { - expect(Reflect.get(MANAGED_ERROR_TAG_BY_CODE, String(code))).toBe(tag); - } - }); - - it("recognizes managed failures without a shared base class", () => { - expect(isManagedStackError(new model.ManagedStackNotFoundError({ stackId: "stack" }))).toBe( - true, - ); - expect(isManagedStackError(new Error("plain"))).toBe(false); - // A bare structural lookalike is not a managed failure: the guard requires - // a real Error so it cannot promote arbitrary payloads. - expect(isManagedStackError({ _tag: "ManagedStackNotFoundError" })).toBe(false); - expect(isManagedStackError(undefined)).toBe(false); - }); -}); diff --git a/packages/stack/src/managed-stack.unit.test.ts b/packages/stack/src/managed-stack.unit.test.ts deleted file mode 100644 index 8f730fc090..0000000000 --- a/packages/stack/src/managed-stack.unit.test.ts +++ /dev/null @@ -1,217 +0,0 @@ -import { describe, expect, it } from "@effect/vitest"; -import { Effect, Layer } from "effect"; -import { FileSystem, Path } from "effect"; -import type { AllocatedPorts } from "./PortAllocator.ts"; -import { resolveManagedStack } from "./managed-stack.ts"; -import { StateManager, projectStateManagerPaths, type StackState } from "./StateManager.ts"; - -const DEFAULT_PORTS: AllocatedPorts = { - apiPort: 54321, - dbPort: 54322, - authPort: 54330, - postgrestPort: 54331, - postgrestAdminPort: 54332, - edgeRuntimePort: 54338, - edgeRuntimeInspectorPort: 54339, - realtimePort: 54333, - storagePort: 54334, - imgproxyPort: 54335, - mailpitPort: 54324, - mailpitSmtpPort: 54325, - mailpitPop3Port: 54326, - pgmetaPort: 54336, - studioPort: 54323, - analyticsPort: 54327, - poolerPort: 54329, - poolerApiPort: 54337, -}; - -function makeState(overrides: Partial = {}): StackState { - return { - pid: 12345, - name: "my-project", - projectDir: "/Users/test/Code/myapp", - apiPort: 54321, - dbPort: 54322, - ports: DEFAULT_PORTS, - socketPath: "/tmp/supabase/s-123456789abc/daemon.sock", - startedAt: "2026-03-04T10:00:00Z", - url: "http://127.0.0.1:54321", - dbUrl: "postgresql://postgres:postgres@127.0.0.1:54322/postgres", - publishableKey: "pk_test", - secretKey: "sk_test", - anonJwt: "anon_jwt", - serviceRoleJwt: "service_role_jwt", - serviceEndpoints: {}, - services: { - postgres: "17.6.1.081", - auth: "2.188.0-rc.15", - }, - ...overrides, - }; -} - -function mockFileSystem() { - const files = new Map(); - const dirs = new Set(); - - const layer = Layer.succeed(FileSystem.FileSystem, { - [FileSystem.FileSystem.key]: FileSystem.FileSystem.key, - exists: (path: string) => Effect.succeed(files.has(path) || dirs.has(path)), - makeDirectory: (dirPath: string) => - Effect.sync(() => { - let current = dirPath; - while (current && current !== "/") { - dirs.add(current); - const parent = require("node:path").dirname(current); - if (parent === current) break; - current = parent; - } - }), - readDirectory: (dirPath: string) => - Effect.sync(() => { - const entries: string[] = []; - const prefix = dirPath.endsWith("/") ? dirPath : `${dirPath}/`; - const allKeys = Array.from(files.keys()).concat(Array.from(dirs)); - for (const key of allKeys) { - if (key.startsWith(prefix)) { - const rest = key.slice(prefix.length); - const segment = rest.split("/")[0]; - if (segment && !entries.includes(segment)) { - entries.push(segment); - } - } - } - return entries; - }), - writeFileString: (path: string, content: string) => - Effect.sync(() => { - files.set(path, content); - }), - readFileString: (path: string) => - Effect.sync(() => { - const content = files.get(path); - if (content == null) throw new Error(`File not found: ${path}`); - return content; - }), - remove: (rmPath: string) => - Effect.sync(() => { - for (const key of Array.from(files.keys())) { - if (key === rmPath || key.startsWith(`${rmPath}/`)) files.delete(key); - } - for (const key of Array.from(dirs)) { - if (key === rmPath || key.startsWith(`${rmPath}/`)) dirs.delete(key); - } - }), - rename: (oldPath: string, newPath: string) => - Effect.sync(() => { - const content = files.get(oldPath); - if (content == null) throw new Error(`File not found: ${oldPath}`); - files.delete(oldPath); - files.set(newPath, content); - }), - } as unknown as FileSystem.FileSystem); - - return { layer, files }; -} - -function mockPath() { - const nodePath = require("node:path"); - return Layer.succeed(Path.Path, { - [Path.Path.key]: Path.Path.key, - ...nodePath, - } as unknown as Path.Path); -} - -function setup() { - const fsm = mockFileSystem(); - const layer = Layer.merge(fsm.layer, mockPath()); - return { layer, files: fsm.files }; -} - -const makeStateManager = StateManager.pipe( - Effect.provide( - StateManager.make(projectStateManagerPaths("/test-home", "/Users/test/Code/myapp")), - ), -); - -describe("resolveManagedStack", () => { - it.effect("resolves a live stack by explicit name", () => { - const { layer } = setup(); - return Effect.gen(function* () { - const mgr = yield* makeStateManager; - yield* mgr.write(makeState({ pid: process.pid })); - - const result = yield* resolveManagedStack({ - cacheRoot: "/test-home", - name: "my-project", - }); - - expect(result.alive).toBe(true); - expect(result.state.name).toBe("my-project"); - }).pipe(Effect.provide(layer)); - }); - - it.effect("resolves a live stack by cwd walk-up", () => { - const { layer } = setup(); - return Effect.gen(function* () { - const mgr = yield* makeStateManager; - yield* mgr.write(makeState({ pid: process.pid, projectDir: "/Users/test/Code/myapp" })); - - const result = yield* resolveManagedStack({ - cacheRoot: "/test-home", - cwd: "/Users/test/Code/myapp/src/components", - }); - - expect(result.alive).toBe(true); - expect(result.state.projectDir).toBe("/Users/test/Code/myapp"); - }).pipe(Effect.provide(layer)); - }); - - it.effect("resolves the requested named stack within the same project", () => { - const { layer } = setup(); - return Effect.gen(function* () { - const mgr = yield* makeStateManager; - yield* mgr.write(makeState({ name: "default", pid: 999999 })); - yield* mgr.write(makeState({ name: "preview", pid: process.pid })); - - const result = yield* resolveManagedStack({ - cacheRoot: "/test-home", - projectDir: "/Users/test/Code/myapp", - name: "preview", - }); - - expect(result.alive).toBe(true); - expect(result.state.name).toBe("preview"); - }).pipe(Effect.provide(layer)); - }); - - it.effect("removes stale state for dead stacks", () => { - const { layer } = setup(); - return Effect.gen(function* () { - const mgr = yield* makeStateManager; - yield* mgr.write(makeState({ pid: 999999 })); - - const result = yield* resolveManagedStack({ - cacheRoot: "/test-home", - name: "my-project", - }); - - expect(result.alive).toBe(false); - const readExit = yield* mgr.read("my-project").pipe(Effect.exit); - expect(readExit._tag).toBe("Failure"); - }).pipe(Effect.provide(layer)); - }); - - it.effect("fails when no stack matches", () => { - const { layer } = setup(); - return Effect.gen(function* () { - const exit = yield* resolveManagedStack({ - cacheRoot: "/test-home", - cwd: "/Users/test/Code/myapp", - }).pipe(Effect.exit); - - expect(exit._tag).toBe("Failure"); - }).pipe(Effect.provide(layer)); - }); -}); diff --git a/packages/stack/src/managed/model.ts b/packages/stack/src/managed/model.ts index 9504661236..0abfc34da2 100644 --- a/packages/stack/src/managed/model.ts +++ b/packages/stack/src/managed/model.ts @@ -344,20 +344,45 @@ export type ManagedStackError = | UnsupportedManagedRegistryVersionError; /** - * Every `code` literal declared by a managed failure. + * Every `code` literal declared by a managed failure, and every `_tag` + * declared alongside it. + * + * Both are derived from {@link ManagedStackError} itself — indexing a + * property on a union type distributes over its members — so adding, removing, + * or renaming a failure class's `code`/`_tag` changes these unions without any + * hand-maintained list to fall out of sync. What compile-time indexing cannot + * catch is two different classes declaring the *same* `code` literal: the + * union would just collapse to one member, so that particular mistake still + * needs a runtime guard (or review) rather than the type checker. + */ +export type ManagedErrorCode = ManagedStackError["code"]; +export type ManagedErrorTag = ManagedStackError["_tag"]; + +/** + * Requires `array` to contain every member of the string-literal union `T`, + * order and duplicates aside. If `T` has a member missing from the supplied + * array, `[T] extends [U[number]]` resolves to `never`, which makes the + * parameter type `never` and turns any array literal into a type error at the + * call site — so `MANAGED_ERROR_CODES` below cannot silently drop a code. + */ +function exhaustiveArrayOf() { + return >(array: U & ([T] extends [U[number]] ? unknown : never)): U => + array; +} + +/** + * Every `code` literal declared by a managed failure, checked exhaustive + * against {@link ManagedErrorCode} at compile time by {@link exhaustiveArrayOf}. * * `code` is the wire-level contract: identifier minification renames the * constructors, so a release build's telemetry and any cross-runtime consumer - * need a value the bundler cannot touch. `managed-model.unit.test.ts` keeps - * this list exhaustive against the exported classes, and the CLI's telemetry - * classifier types its dispatch table as `Record` so a - * new code cannot be added here without classifying it there. + * need a value the bundler cannot touch. * * This module must stay free of runtime-specific imports: it is published as * `@supabase/stack/managed-model` precisely so consumers can import the codes * under Bun and Node alike, without pulling in a SQLite driver. */ -export const MANAGED_ERROR_CODES = [ +export const MANAGED_ERROR_CODES = exhaustiveArrayOf()([ "DUPLICATE_MANAGED_IDENTITY", "INVALID_MANAGED_IDENTITY", "MANAGED_DUPLICATE_PORT_KEY", @@ -376,9 +401,7 @@ export const MANAGED_ERROR_CODES = [ "MANAGED_STACK_PUBLICATION_TIMEOUT", "UNSAFE_MANAGED_STACK_PATH", "UNSUPPORTED_MANAGED_REGISTRY_VERSION", -] as const; - -export type ManagedErrorCode = (typeof MANAGED_ERROR_CODES)[number]; +] as const); /** * The single source of truth linking each managed `code` to the `_tag` of the @@ -388,7 +411,9 @@ export type ManagedErrorCode = (typeof MANAGED_ERROR_CODES)[number]; * dispatch) and `code` is the stable wire-level contract. Consumers that key a * table by one and dispatch on the other — the CLI's telemetry classifier is * the motivating case — derive it from this map instead of restating all - * eighteen pairs by hand. + * eighteen pairs by hand. Typing this `satisfies Record` requires every code to be present with a valid tag, so a + * new error class that is not registered here is a compile error. */ export const MANAGED_ERROR_TAG_BY_CODE = { DUPLICATE_MANAGED_IDENTITY: "DuplicateManagedIdentityError", @@ -409,7 +434,7 @@ export const MANAGED_ERROR_TAG_BY_CODE = { MANAGED_STACK_PUBLICATION_TIMEOUT: "ManagedStackPublicationTimeoutError", UNSAFE_MANAGED_STACK_PATH: "UnsafeManagedStackPathError", UNSUPPORTED_MANAGED_REGISTRY_VERSION: "UnsupportedManagedRegistryVersionError", -} as const satisfies Record; +} as const satisfies Record; const MANAGED_ERROR_TAGS: ReadonlySet = new Set(Object.values(MANAGED_ERROR_TAG_BY_CODE)); diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 4624e456dd..b61fdda892 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -224,13 +224,16 @@ importers: version: 7.0.1 semantic-release: specifier: ^25.0.8 - version: 25.0.8(typescript@7.0.2) + version: 25.0.8(@typescript/typescript6@6.0.2) smol-toml: specifier: ^1.7.1 version: 1.7.1 tldts: specifier: 'catalog:' version: 7.4.9 + typescript: + specifier: npm:@typescript/typescript6@^6.0.2 + version: '@typescript/typescript6@6.0.2' vitest: specifier: 'catalog:' version: 4.1.10(@types/node@26.1.2)(@vitest/coverage-istanbul@4.1.10)(vite@8.1.4(@types/node@26.1.2)(esbuild@0.28.1)(jiti@2.7.0)(yaml@2.9.0)) @@ -8776,7 +8779,7 @@ snapshots: '@sec-ant/readable-stream@0.4.1': {} - '@semantic-release/commit-analyzer@13.0.1(semantic-release@25.0.8(typescript@7.0.2))': + '@semantic-release/commit-analyzer@13.0.1(semantic-release@25.0.8(@typescript/typescript6@6.0.2))': dependencies: conventional-changelog-angular: 8.3.1 conventional-changelog-writer: 8.4.0 @@ -8786,13 +8789,13 @@ snapshots: import-from-esm: 2.0.0 lodash-es: 4.18.1 micromatch: 4.0.8 - semantic-release: 25.0.8(typescript@7.0.2) + semantic-release: 25.0.8(@typescript/typescript6@6.0.2) transitivePeerDependencies: - supports-color '@semantic-release/error@4.0.0': {} - '@semantic-release/github@12.0.9(semantic-release@25.0.8(typescript@7.0.2))': + '@semantic-release/github@12.0.9(semantic-release@25.0.8(@typescript/typescript6@6.0.2))': dependencies: '@octokit/core': 7.0.6 '@octokit/plugin-paginate-rest': 14.0.0(@octokit/core@7.0.6) @@ -8808,7 +8811,7 @@ snapshots: lodash-es: 4.18.1 mime: 4.1.0 p-filter: 4.1.0 - semantic-release: 25.0.8(typescript@7.0.2) + semantic-release: 25.0.8(@typescript/typescript6@6.0.2) tinyglobby: 0.2.17 undici: 7.29.0 url-join: 5.0.0 @@ -8816,7 +8819,7 @@ snapshots: - kerberos - supports-color - '@semantic-release/npm@13.1.5(semantic-release@25.0.8(typescript@7.0.2))': + '@semantic-release/npm@13.1.5(semantic-release@25.0.8(@typescript/typescript6@6.0.2))': dependencies: '@actions/core': 3.0.1 '@semantic-release/error': 4.0.0 @@ -8831,11 +8834,11 @@ snapshots: rc: 1.2.8 read-pkg: 10.1.0 registry-auth-token: 5.1.1 - semantic-release: 25.0.8(typescript@7.0.2) + semantic-release: 25.0.8(@typescript/typescript6@6.0.2) semver: 7.8.5 tempy: 3.2.0 - '@semantic-release/release-notes-generator@14.1.1(semantic-release@25.0.8(typescript@7.0.2))': + '@semantic-release/release-notes-generator@14.1.1(semantic-release@25.0.8(@typescript/typescript6@6.0.2))': dependencies: conventional-changelog-angular: 8.3.1 conventional-changelog-writer: 8.4.0 @@ -8845,7 +8848,7 @@ snapshots: import-from-esm: 2.0.0 lodash-es: 4.18.1 read-package-up: 11.0.0 - semantic-release: 25.0.8(typescript@7.0.2) + semantic-release: 25.0.8(@typescript/typescript6@6.0.2) transitivePeerDependencies: - supports-color @@ -9934,14 +9937,14 @@ snapshots: object-assign: 4.1.1 vary: 1.1.2 - cosmiconfig@9.0.2(typescript@7.0.2): + cosmiconfig@9.0.2(@typescript/typescript6@6.0.2): dependencies: env-paths: 2.2.1 import-fresh: 3.3.1 js-yaml: 4.3.0 parse-json: 5.2.0 optionalDependencies: - typescript: 7.0.2 + typescript: '@typescript/typescript6@6.0.2' cross-spawn@7.0.6: dependencies: @@ -12903,15 +12906,15 @@ snapshots: dependencies: compute-scroll-into-view: 3.1.1 - semantic-release@25.0.8(typescript@7.0.2): + semantic-release@25.0.8(@typescript/typescript6@6.0.2): dependencies: - '@semantic-release/commit-analyzer': 13.0.1(semantic-release@25.0.8(typescript@7.0.2)) + '@semantic-release/commit-analyzer': 13.0.1(semantic-release@25.0.8(@typescript/typescript6@6.0.2)) '@semantic-release/error': 4.0.0 - '@semantic-release/github': 12.0.9(semantic-release@25.0.8(typescript@7.0.2)) - '@semantic-release/npm': 13.1.5(semantic-release@25.0.8(typescript@7.0.2)) - '@semantic-release/release-notes-generator': 14.1.1(semantic-release@25.0.8(typescript@7.0.2)) + '@semantic-release/github': 12.0.9(semantic-release@25.0.8(@typescript/typescript6@6.0.2)) + '@semantic-release/npm': 13.1.5(semantic-release@25.0.8(@typescript/typescript6@6.0.2)) + '@semantic-release/release-notes-generator': 14.1.1(semantic-release@25.0.8(@typescript/typescript6@6.0.2)) aggregate-error: 5.0.0 - cosmiconfig: 9.0.2(typescript@7.0.2) + cosmiconfig: 9.0.2(@typescript/typescript6@6.0.2) debug: 4.4.3(supports-color@7.2.0) env-ci: 11.2.0 execa: 9.6.1 From 5722f5abb6d4ccf032b23c4b9559420e62106fa5 Mon Sep 17 00:00:00 2001 From: Julien Goux Date: Wed, 12 Aug 2026 16:38:53 +0200 Subject: [PATCH 5/5] fix(stack): overwrite a stranded temporary file when a claim retries The temporary write was exclusive, so a temp file stranded by a killed run made a retry with the same injected id fail with EEXIST instead of claiming. The temp path is unique by construction and the hardlink is what settles the race, so the exclusive flag bought nothing. Co-Authored-By: Claude Fable 5 --- .../stack/src/managed-atomic-claim.unit.test.ts | 13 +++++++++++++ packages/stack/src/managed/atomic-claim.ts | 6 ++++-- 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/packages/stack/src/managed-atomic-claim.unit.test.ts b/packages/stack/src/managed-atomic-claim.unit.test.ts index d6c1fca512..63434089ca 100644 --- a/packages/stack/src/managed-atomic-claim.unit.test.ts +++ b/packages/stack/src/managed-atomic-claim.unit.test.ts @@ -88,6 +88,19 @@ describe("atomic file claim", () => { expect(readdirSync(root)).toEqual([]); }); + it("claims over a temporary file stranded by a killed run that reused the same id", async () => { + const root = makeRoot(); + const target = join(root, "claim.json"); + writeFileSync(`${target}.tmp.fixed-id`, "stranded\n"); + + await expect(claimFileAtomically(target, "mine\n", { temporaryId: "fixed-id" })).resolves.toBe( + "claimed", + ); + + expect(readFileSync(target, "utf8")).toBe("mine\n"); + expect(strayTemporaryFiles(root)).toEqual([]); + }); + it("names the temporary file from an injected identifier so a run stays reproducible", async () => { const root = makeRoot(); const target = join(root, "claim.json"); diff --git a/packages/stack/src/managed/atomic-claim.ts b/packages/stack/src/managed/atomic-claim.ts index d30ac86a6f..34fb293d1a 100644 --- a/packages/stack/src/managed/atomic-claim.ts +++ b/packages/stack/src/managed/atomic-claim.ts @@ -50,7 +50,9 @@ const createExclusively = async ( * * A `SIGKILL` between the temporary write and its removal strands a * `.tmp.` sibling. Nothing ever reads those, so a stranded one is junk - * rather than a claim anybody can observe. + * rather than a claim anybody can observe, and a retry that reuses the same + * temporary id overwrites it — which is why the temporary write is not + * exclusive. */ export const claimFileAtomically = async ( targetPath: string, @@ -59,7 +61,7 @@ export const claimFileAtomically = async ( ): Promise => { const linkFile = options.linkFile ?? link; const temporaryPath = `${targetPath}.tmp.${options.temporaryId ?? randomUUID()}`; - await writeFile(temporaryPath, content, { flag: "wx", mode: options.mode }); + await writeFile(temporaryPath, content, { mode: options.mode }); try { await linkFile(temporaryPath, targetPath); return "claimed";