From 53e7390570f6f38f02daff89bd377a273ace8840 Mon Sep 17 00:00:00 2001 From: Carlos Cima Date: Wed, 19 Aug 2026 06:29:08 -0400 Subject: [PATCH 1/4] fix: address architecture review findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Non-breaking remediation of the v4.1.0 architecture review. Each fix landed test-first; the suite grew from 369 to 388 tests. Bugs: - A mutex releaseLock() that reports failure by returning false — the signal LockAdapterInterface defines, and what the documented PostgreSQL advisory-lock adapter produces — was silently swallowed: the operation resolved, onReleaseError never fired, isLockAcquired() stayed true forever, and every later operation piggybacked on the stuck lock without ever releasing it. Both failure modes (throw and false) now flow through one path and raise the new LockCanNotBeReleasedError. - whenIdle() lacked the re-entrancy guard triggerEvent and checkTransitions have, so awaiting it inside an observer deadlocked the machine permanently and silently. It now throws ReentrancyError. API gaps: - Factory could not configure the v4 engine options, so factory-created machines silently ran on defaults with no back-pressure and no diagnostic sinks — precisely the fleet case those options exist for. Adds a FactoryStatemachineOptions template, excluding the three fields the factory derives per subject. - Statemachine.releaseLock() now reports failures through onReleaseError; the Promise return type stays deferred to v5 because it is breaking. - Observer accessors return snapshots instead of the live arrays. - AmbiguousTransitionError carries the competing transitions, which is what identifying the culprits actually requires. - LockAdapterMutex shares one in-flight acquire, so overlapping calls cannot double-acquire a non-idempotent adapter. Also documents the passive nature of Timeout (it schedules nothing) and the cross-machine sharing of event observers, and re-dates the overdue "removed in v4" deprecation notices to v5. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01JLxceiNFfXfpGoxdfQEZqw --- docs/conditions.md | 8 ++ docs/core.md | 37 +++--- docs/errors.md | 71 ++++++++++- docs/factory.md | 30 ++++- docs/mutex.md | 22 ++-- docs/observers.md | 2 + src/Event.ts | 4 +- src/Statemachine.ts | 81 ++++++++---- src/error/AmbiguousTransitionError.ts | 39 +++++- src/error/LockCanNotBeReleasedError.ts | 21 ++++ src/error/index.ts | 2 + src/factory/Factory.ts | 25 ++++ src/factory/index.ts | 1 + src/index.ts | 3 + src/interfaces/DispatcherInterface.ts | 4 +- src/mutex/LockAdapterMutex.ts | 22 +++- src/observer/CallbackObserver.ts | 8 +- src/selector/OneOrNoneActiveTransition.ts | 10 +- tests/factory-options.test.ts | 143 ++++++++++++++++++++++ tests/mutex.test.ts | 53 ++++++++ tests/observer-snapshot.test.ts | 76 ++++++++++++ tests/reentrancy.test.ts | 37 ++++++ tests/resolve-after-release.test.ts | 85 +++++++++++++ tests/typed-throws.test.ts | 40 ++++++ 24 files changed, 760 insertions(+), 64 deletions(-) create mode 100644 src/error/LockCanNotBeReleasedError.ts create mode 100644 tests/factory-options.test.ts create mode 100644 tests/observer-snapshot.test.ts diff --git a/docs/conditions.md b/docs/conditions.md index c37d60f..7eaaf28 100644 --- a/docs/conditions.md +++ b/docs/conditions.md @@ -272,6 +272,14 @@ class Subscription { The condition calculates `lastStateChangedDate + timeoutMs` and returns `true` if that time is in the past (i.e., the timeout has elapsed). +> **`Timeout` is passive — it schedules nothing.** Like every condition, it is only evaluated when something drives the machine. A transition guarded by `Timeout` fires when the elapsed time has passed **and** someone calls `checkTransitions()` (or triggers an event). Nothing happens on its own at the deadline, so drive expiry from your own scheduler: +> +> ```typescript +> setInterval(() => { +> void sm.checkTransitions().catch(handleError); +> }, 60_000); +> ``` + --- ## AndComposite diff --git a/docs/core.md b/docs/core.md index 330ebbb..44abb09 100644 --- a/docs/core.md +++ b/docs/core.md @@ -370,21 +370,22 @@ new Statemachine( ### Methods -| Method | Return Type | Description | -| ------------------------------ | ------------------------ | --------------------------------------------------- | -| `getCurrentState()` | `StateInterface` | Returns the current state | -| `getLastState()` | `StateInterface \| null` | Returns the state before the most recent transition | -| `getSubject()` | `TSubject` | Returns the managed subject | -| `getProcess()` | `ProcessInterface` | Returns the process | -| `triggerEvent(name, context?)` | `Promise` | Triggers a named event on the current state | -| `checkTransitions(context?)` | `Promise` | Evaluates automatic transitions | -| `acquireLock()` | `Promise` | Manually acquires the lock | -| `releaseLock()` | `Promise` | Manually releases the lock | -| `isLockAcquired()` | `boolean` | Checks if the lock is currently acquired | -| `isAutoreleaseLock()` | `boolean` | Checks if auto-release is enabled | -| `setAutoreleaseLock(value)` | `void` | Enables/disables auto-release | -| `attachBefore(observer)` | `void` | Attaches a `BeforeTransitionObserver` | -| `attachAfter(observer)` | `void` | Attaches an `AfterTransitionObserver` | +| Method | Return Type | Description | +| ------------------------------ | ------------------------ | ----------------------------------------------------------- | +| `getCurrentState()` | `StateInterface` | Returns the current state | +| `getLastState()` | `StateInterface \| null` | Returns the state before the most recent transition | +| `getSubject()` | `TSubject` | Returns the managed subject | +| `getProcess()` | `ProcessInterface` | Returns the process | +| `triggerEvent(name, context?)` | `Promise` | Triggers a named event on the current state | +| `checkTransitions(context?)` | `Promise` | Evaluates automatic transitions | +| `whenIdle()` | `Promise` | Resolves once the queue is drained and the runner is idle | +| `acquireLock()` | `Promise` | Manually acquires the lock | +| `releaseLock()` | `Promise` | Manually releases the lock; failures go to `onReleaseError` | +| `isLockAcquired()` | `boolean` | Checks if the lock is currently acquired | +| `isAutoreleaseLock()` | `boolean` | Checks if auto-release is enabled | +| `setAutoreleaseLock(value)` | `void` | Enables/disables auto-release | +| `attachBefore(observer)` | `void` | Attaches a `BeforeTransitionObserver` | +| `attachAfter(observer)` | `void` | Attaches an `AfterTransitionObserver` | ### Event Processing Flow @@ -441,6 +442,12 @@ const sm = new Statemachine(subject, process); sm.attachAfter(new AuditObserver()); ``` +`getBeforeObservers()` and `getAfterObservers()` return a **snapshot**: detaching an observer afterwards does not change a list you already hold, and mutating that list does not change the machine's registrations. + +`triggerEvent()`, `checkTransitions()` and `whenIdle()` must not be called on the same machine from inside an observer, condition or selector — see [ReentrancyError](errors.md#reentrancyerror). Use the `EnqueueContext` given to after-observers to chain events instead. + +The `frame.context` map is a per-transition copy shared by every observer of that frame; it is `ReadonlyMap` at compile time only. Treat it as read-only — a caller that casts it and mutates will affect the observers that run after it. + ### Concurrency Concurrent calls to `triggerEvent` and `checkTransitions` on the same instance are automatically serialized into a FIFO queue. There is no longer any "already running" error for same-instance concurrency. diff --git a/docs/errors.md b/docs/errors.md index f0f0e0b..a51cc36 100644 --- a/docs/errors.md +++ b/docs/errors.md @@ -7,6 +7,7 @@ Custom error classes thrown by the state machine. - [FinitaError](#finitaerror) - [WrongEventForStateError](#wrongeventforstateerror) - [LockCanNotBeAcquiredError](#lockcannotbeacquirederror) +- [LockCanNotBeReleasedError](#lockcannotbereleasederror) - [DuplicateStateError](#duplicatestateerror) - [ProcessFinalizedError](#processfinalizederror) - [GraphValidationError](#graphvalidationerror) @@ -109,6 +110,39 @@ With the default `NullMutex`, this error is never thrown because `acquireLock()` --- +## LockCanNotBeReleasedError + +**Import:** `import { LockCanNotBeReleasedError } from '@camcima/finita'` + +Thrown when the mutex reports a failed release by returning `false` — the failure signal `MutexInterface` and `LockAdapterInterface` define (a PostgreSQL advisory unlock that returns `false`, a Redis `DEL` that removed nothing). + +### Properties + +| Property | Type | Description | +| --------- | -------- | --------------------------------------------------------------------------------------- | +| `code` | `string` | `'lockCanNotBeReleased'` | +| `message` | `string` | `'Lock can not be released! releaseLock() returned false; the lock may still be held.'` | + +### When It's Thrown + +After a top-level operation completes, the engine releases the lock it acquired. If that release fails — whether the mutex **throws** or **returns `false`** — the failure is reported to `onReleaseError`, and: + +- if the operation itself succeeded, the caller's promise rejects with the release error, because the lock may still be held; +- if the operation itself failed, the caller's promise rejects with the **operation** error (the release error is not masked over it) and `onReleaseError` is the only place the release failure appears. + +A failed release must never be mistaken for a successful one: the engine skips acquisition when the mutex reports it is already held, so a silently stuck lock would let every later operation piggyback on it and never release it. + +`Statemachine.releaseLock()` (manual lock management) reports failures through `onReleaseError` but does **not** throw, preserving its `Promise` contract. Inspect `isLockAcquired()` to confirm the lock was freed. + +```typescript +const sm = new Statemachine(order, process, { + mutex, + onReleaseError: (error) => logger.error("lock release failed", { error }), +}); +``` + +--- + ## DuplicateStateError **Import:** `import { DuplicateStateError } from '@camcima/finita'` @@ -401,11 +435,36 @@ Thrown by `OneOrNoneActiveTransition.selectTransition(transitions)` when more th ### Properties -| Property | Type | Description | -| ------------- | ----------------------- | -------------------------------- | -| `code` | `"ambiguousTransition"` | Discriminator | -| `activeCount` | `number` | How many transitions were active | -| `name` | `string` | `'AmbiguousTransitionError'` | +| Property | Type | Description | +| ------------- | ----------------------------------------- | ------------------------------------- | +| `code` | `"ambiguousTransition"` | Discriminator | +| `activeCount` | `number` | How many transitions were active | +| `candidates` | `readonly AmbiguousTransitionCandidate[]` | The competing transitions (see below) | +| `name` | `string` | `'AmbiguousTransitionError'` | + +### `AmbiguousTransitionCandidate` shape + +Each candidate describes one of the simultaneously-active transitions, which is what you need to resolve the ambiguity — a count alone does not identify the culprits. The candidates are also rendered into the error message. + +```typescript +interface AmbiguousTransitionCandidate { + targetStateName: string; + eventName: string | null; // null for automatic transitions + conditionName: string | null; + weight: number; +} +``` + +```typescript +try { + await sm.triggerEvent("submit"); +} catch (error) { + if (error instanceof AmbiguousTransitionError) { + // e.g. ['approved', 'rejected'] — both guards passed + console.log(error.candidates.map((c) => c.targetStateName)); + } +} +``` --- @@ -434,7 +493,7 @@ Concretely, the error is thrown on the automatic hop _after_ `maxAutomaticHops` **Import:** `import { ReentrancyError } from '@camcima/finita'` -Rejects the promise returned by `triggerEvent()` / `checkTransitions()` when either is called from inside an observer, condition, or transition selector of the **same** `Statemachine` — before the callback's first `await`. Awaiting such a call would deadlock permanently: the machine runs one operation at a time, and the runner is blocked waiting for your callback to finish. +Rejects the promise returned by `triggerEvent()` / `checkTransitions()` / `whenIdle()` when any of them is called from inside an observer, condition, or transition selector of the **same** `Statemachine` — before the callback's first `await`. Awaiting such a call would deadlock permanently: the machine runs one operation at a time, and the runner is blocked waiting for your callback to finish. (`whenIdle()` is guarded for the same reason: the machine cannot reach idle while the runner is blocked on that very callback.) **Detection scope:** only the _synchronous portion_ of a callback is guarded. A re-entrant call made **after** a prior `await` inside the callback cannot be detected (that would require Node-only `AsyncLocalStorage`) and will still deadlock silently. Keep re-entrant calls out of callbacks entirely. diff --git a/docs/factory.md b/docs/factory.md index 841231a..ee61551 100644 --- a/docs/factory.md +++ b/docs/factory.md @@ -49,7 +49,8 @@ flowchart TD ```typescript new Factory( processDetector: ProcessDetectorInterface, - stateNameDetector?: StateNameDetectorInterface | null + stateNameDetector?: StateNameDetectorInterface | null, + options?: FactoryStatemachineOptions ) ``` @@ -57,6 +58,33 @@ new Factory( | ------------------- | ---------------------------------------------- | ---------- | ------------------------------------------------------------------------- | | `processDetector` | `ProcessDetectorInterface` | (required) | Determines which process to use for the subject | | `stateNameDetector` | `StateNameDetectorInterface \| null` | `null` | Detects the current state from the subject (for restoring state machines) | +| `options` | `FactoryStatemachineOptions` | `{}` | Engine options applied to every machine the factory creates | + +### Engine options + +`FactoryStatemachineOptions` is `StatemachineOptions` without the three fields +the factory derives per subject — `initialStateName` (from the state-name +detector), `mutex` (from the mutex factory) and `transitionSelector` (from +`setTransitionSelector`). Everything else is forwarded unchanged: +`autoreleaseLock`, `maxAutomaticHops`, `maxQueueLength`, +`onChainedOperationError` and `onReleaseError`. + +This matters most for the fleet use case the factory exists to serve: without +it, a service creating one machine per order would silently run every machine +on the defaults, with no back-pressure and no diagnostic sinks. + +```typescript +const factory = new Factory( + new SingleProcessDetector(orderProcess), + new StatefulStateNameDetector(), + { + maxQueueLength: 100, + onChainedOperationError: (error, info) => + logger.error("chained op failed", { error, event: info.eventName }), + onReleaseError: (error) => logger.error("lock release failed", { error }), + }, +); +``` ### Methods diff --git a/docs/mutex.md b/docs/mutex.md index ed60c9b..f07549e 100644 --- a/docs/mutex.md +++ b/docs/mutex.md @@ -87,12 +87,12 @@ new LockAdapterMutex(lockAdapter: LockAdapterInterface, resourceName: string) ### Methods -| Method | Returns | Behavior | -| --------------- | ------------------ | -------------------------------------------------------------------------------------------------------------------- | -| `acquireLock()` | `Promise` | If not already acquired, delegates to `lockAdapter.acquireLock(resourceName)`. Returns result. | -| `releaseLock()` | `Promise` | If acquired, delegates to `lockAdapter.releaseLock(resourceName)`. Returns result. If not acquired, returns `false`. | -| `isAcquired()` | `boolean` | Returns local acquired state | -| `isLocked()` | `Promise` | Delegates to `lockAdapter.isLocked(resourceName)` | +| Method | Returns | Behavior | +| --------------- | ------------------ | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | +| `acquireLock()` | `Promise` | If not already acquired, delegates to `lockAdapter.acquireLock(resourceName)`. Returns result. Overlapping calls share one underlying acquire, so a non-idempotent adapter is never acquired twice; a failed acquire can still be retried. | +| `releaseLock()` | `Promise` | If acquired, delegates to `lockAdapter.releaseLock(resourceName)`. Returns result. If not acquired, returns `false`. | +| `isAcquired()` | `boolean` | Returns local acquired state | +| `isLocked()` | `Promise` | Delegates to `lockAdapter.isLocked(resourceName)` | ### Example @@ -337,7 +337,15 @@ sequenceDiagram ## Release Error Behavior -If the automatic release throws while the operation also failed, the caller's rejection carries the operation error; pass `onReleaseError` in `StatemachineOptions` to observe the release failure (e.g. to alert that the lock may still be held). +A release fails in one of two ways, and the engine treats them identically: the mutex **throws**, or it **returns `false`** — the failure signal this interface defines (a PostgreSQL advisory unlock returning `false`, a Redis `DEL` that removed nothing). + +When the automatic release fails: + +- `onReleaseError` (in `StatemachineOptions`) is called with the error — a `LockCanNotBeReleasedError` for the `false` case; +- if the operation itself succeeded, the caller's promise **rejects**, because the lock may still be held. Silently resolving would let every later operation piggyback on a stuck lock and never release it (the engine skips acquisition whenever the mutex reports it is already held); +- if the operation itself failed, the caller's rejection carries the **operation** error, and `onReleaseError` is the only place the release failure appears. + +`Statemachine.releaseLock()` (manual lock management) reports failures through `onReleaseError` but does not throw, preserving its `Promise` contract; check `isLockAcquired()` to confirm the lock was actually freed. ## Manual Lock Management diff --git a/docs/observers.md b/docs/observers.md index 6c209b9..5fa441a 100644 --- a/docs/observers.md +++ b/docs/observers.md @@ -486,6 +486,8 @@ class SendEmailCommand implements Observer { process.getState("shipped").getEvent("ship").attach(new SendEmailCommand()); ``` +> **Event observers are shared by every machine built from the process.** `Event` objects live on the `Process` graph, and neither `Statemachine` nor `Factory` clones it. Attaching a command "for one order" attaches it to every machine that uses that process, so attach event commands once at setup and read the per-transition subject from `args[0]` (as above) rather than closing over one subject. For behavior that belongs to a single machine, use `attachAfter`/`attachBefore` on that machine instead. + ### State Machine Observer (After) React to any state change using the frame parameter: diff --git a/src/Event.ts b/src/Event.ts index d727574..d7a268d 100644 --- a/src/Event.ts +++ b/src/Event.ts @@ -44,8 +44,10 @@ export class Event implements EventInterface { } } + /** Snapshot — detaching later does not change an already-returned list, + * and mutating it does not change the event's registrations. */ getObservers(): Iterable { - return this.observers; + return [...this.observers]; } getMetadata(): Record { diff --git a/src/Statemachine.ts b/src/Statemachine.ts index 4ff7386..3e6b69c 100644 --- a/src/Statemachine.ts +++ b/src/Statemachine.ts @@ -1,7 +1,6 @@ import type { StatemachineInterface } from "./interfaces/StatemachineInterface.js"; import type { StateInterface } from "./interfaces/StateInterface.js"; import type { ProcessInterface } from "./interfaces/ProcessInterface.js"; -import type { TransitionInterface } from "./interfaces/TransitionInterface.js"; import type { EventInterface } from "./interfaces/EventInterface.js"; import type { MutexInterface } from "./interfaces/MutexInterface.js"; import type { TransitionSelectorInterface } from "./interfaces/TransitionSelectorInterface.js"; @@ -19,6 +18,7 @@ import type { QueuedOperation } from "./internal/OperationQueue.js"; import { ActiveTransitionFilter } from "./filter/ActiveTransitionFilter.js"; import { WrongEventForStateError } from "./error/WrongEventForStateError.js"; import { LockCanNotBeAcquiredError } from "./error/LockCanNotBeAcquiredError.js"; +import { LockCanNotBeReleasedError } from "./error/LockCanNotBeReleasedError.js"; import { AutomaticTransitionCycleError } from "./error/AutomaticTransitionCycleError.js"; import { ReentrancyError } from "./error/ReentrancyError.js"; import { QueueLimitExceededError } from "./error/QueueLimitExceededError.js"; @@ -118,8 +118,10 @@ export class Statemachine< if (idx >= 0) this.beforeObservers.splice(idx, 1); } + /** Snapshot — detaching later does not change an already-returned list, + * and mutating it does not change the machine's registrations. */ getBeforeObservers(): Iterable> { - return this.beforeObservers; + return [...this.beforeObservers]; } attachAfter(observer: AfterTransitionObserver): void { @@ -132,8 +134,9 @@ export class Statemachine< if (idx >= 0) this.afterObservers.splice(idx, 1); } + /** Snapshot — see getBeforeObservers. */ getAfterObservers(): Iterable> { - return this.afterObservers; + return [...this.afterObservers]; } // --- public locking --- @@ -142,8 +145,15 @@ export class Statemachine< return this.mutex.acquireLock(); } + /** + * Releases the mutex. A failed release — whether the mutex throws or + * returns false — is reported to the onReleaseError hook; it is not thrown, + * so manual lock management keeps its existing control flow. Inspect + * isLockAcquired() (or the hook) to learn whether the lock was actually + * freed. + */ async releaseLock(): Promise { - await this.mutex.releaseLock(); + await this.releaseMutex(); } isLockAcquired(): boolean { @@ -180,8 +190,14 @@ export class Statemachine< * EnqueueContext.enqueue(), has completed. Resolves immediately if the * machine is already idle. Note this is a quiescence point, not a * receipt: work scheduled later (e.g. from a timer) starts a new drain. + * + * Like triggerEvent/checkTransitions, this may not be called from inside an + * observer or condition of the same machine: the machine cannot reach idle + * while the runner is blocked on that very callback, so awaiting it there + * always deadlocks. */ whenIdle(): Promise { + this.assertNotReentrant("whenIdle()"); if (!this.running && this.queue.isEmpty()) { return Promise.resolve(); } @@ -290,23 +306,12 @@ export class Statemachine< failure = { err }; } finally { if (acquiredHere && this.autoreleaseLock) { - try { - await this.mutex.releaseLock(); - } catch (err) { - // Surface every release failure through the diagnostic hook — when - // the operation also failed, the rejection carries the operation - // error and this hook is the only place the release error appears. - try { - this.onReleaseError?.(err); - } catch { - /* a throwing hook must not mask engine errors */ - } - // A release failure must not mask an operation error, but when the - // operation succeeded the caller must learn the lock may still be - // held — otherwise every later operation silently piggybacks on - // (and never releases) the stuck lock. - if (!failure) failure = { err }; - } + const releaseFailure = await this.releaseMutex(); + // A release failure must not mask an operation error, but when the + // operation succeeded the caller must learn the lock may still be + // held — otherwise every later operation silently piggybacks on + // (and never releases) the stuck lock. + if (releaseFailure && !failure) failure = releaseFailure; } } if (failure) { @@ -316,6 +321,38 @@ export class Statemachine< } } + /** + * Releases the mutex, normalizing its two failure modes into one result: a + * thrown error, and a false return — the failure signal MutexInterface / + * LockAdapterInterface define (a PostgreSQL advisory unlock that returns + * false, a Redis DEL that removed nothing). A false return means the lock + * may still be held, so it must never be mistaken for a successful release. + * + * Every failure is surfaced through the diagnostic hook — when the + * operation also failed, the rejection carries the operation error and this + * hook is the only place the release error appears. + * + * @returns null on success, or the failure wrapped for the caller to raise. + */ + private async releaseMutex(): Promise<{ err: unknown } | null> { + let failure: { err: unknown } | null = null; + try { + if (!(await this.mutex.releaseLock())) { + failure = { err: new LockCanNotBeReleasedError() }; + } + } catch (err) { + failure = { err }; + } + if (failure) { + try { + this.onReleaseError?.(failure.err); + } catch { + /* a throwing hook must not mask engine errors */ + } + } + return failure; + } + private resolveEvent(name: string): EventInterface { if (!this.currentState.hasEvent(name)) { throw new WrongEventForStateError(this.currentState.getName(), name); @@ -369,7 +406,7 @@ export class Statemachine< ); const selected = this.guardSync(() => this.transitionSelector.selectTransition(active), - ) as TransitionInterface | null; + ); if (!selected) { return; diff --git a/src/error/AmbiguousTransitionError.ts b/src/error/AmbiguousTransitionError.ts index 3d3b0fe..f4927d9 100644 --- a/src/error/AmbiguousTransitionError.ts +++ b/src/error/AmbiguousTransitionError.ts @@ -1,12 +1,47 @@ import { FinitaError } from "./FinitaError.js"; +/** One of the simultaneously-active transitions that caused the ambiguity. */ +export interface AmbiguousTransitionCandidate { + targetStateName: string; + eventName: string | null; + conditionName: string | null; + weight: number; +} + export class AmbiguousTransitionError extends FinitaError { readonly code = "ambiguousTransition"; readonly activeCount: number; + /** The competing transitions — what you need to resolve the ambiguity. */ + readonly candidates: readonly Readonly[]; - constructor(activeCount: number) { - super(`More than one transition is active! (active count: ${activeCount})`); + constructor( + activeCount: number, + candidates: Iterable = [], + ) { + const list = Array.from(candidates, (c) => Object.freeze({ ...c })); + const detail = + list.length > 0 + ? ` Candidates: ${list.map(describeCandidate).join("; ")}.` + : ""; + super( + `More than one transition is active! (active count: ${activeCount})${detail}`, + ); this.name = "AmbiguousTransitionError"; this.activeCount = activeCount; + this.candidates = Object.freeze(list); + } +} + +function describeCandidate(candidate: AmbiguousTransitionCandidate): string { + const parts = [`-> "${candidate.targetStateName}"`]; + parts.push( + candidate.eventName === null + ? "on " + : `on event "${candidate.eventName}"`, + ); + if (candidate.conditionName !== null) { + parts.push(`if ${candidate.conditionName}`); } + parts.push(`weight ${candidate.weight}`); + return parts.join(" "); } diff --git a/src/error/LockCanNotBeReleasedError.ts b/src/error/LockCanNotBeReleasedError.ts new file mode 100644 index 0000000..c4ba77a --- /dev/null +++ b/src/error/LockCanNotBeReleasedError.ts @@ -0,0 +1,21 @@ +import { FinitaError } from "./FinitaError.js"; + +/** + * The mutex reported a failed release by returning false, as + * LockAdapterInterface specifies (e.g. a PostgreSQL advisory unlock that + * returns false, or a Redis DEL that removed nothing). + * + * The lock must be assumed to still be held: the engine surfaces this so a + * failed release can never be mistaken for a successful one, which would let + * every later operation piggyback on — and never release — a stuck lock. + */ +export class LockCanNotBeReleasedError extends FinitaError { + readonly code = "lockCanNotBeReleased"; + + constructor( + message = "Lock can not be released! releaseLock() returned false; the lock may still be held.", + ) { + super(message); + this.name = "LockCanNotBeReleasedError"; + } +} diff --git a/src/error/index.ts b/src/error/index.ts index d638257..2798391 100644 --- a/src/error/index.ts +++ b/src/error/index.ts @@ -1,6 +1,7 @@ export { FinitaError } from "./FinitaError.js"; export { WrongEventForStateError } from "./WrongEventForStateError.js"; export { LockCanNotBeAcquiredError } from "./LockCanNotBeAcquiredError.js"; +export { LockCanNotBeReleasedError } from "./LockCanNotBeReleasedError.js"; export { DuplicateStateError } from "./DuplicateStateError.js"; export { ProcessFinalizedError } from "./ProcessFinalizedError.js"; export { GraphValidationError } from "./GraphValidationError.js"; @@ -15,3 +16,4 @@ export { ReentrancyError } from "./ReentrancyError.js"; export { QueueLimitExceededError } from "./QueueLimitExceededError.js"; export type { GraphValidationCode } from "./GraphValidationError.js"; export type { DuplicateTransitionConflict } from "./DuplicateTransitionError.js"; +export type { AmbiguousTransitionCandidate } from "./AmbiguousTransitionError.js"; diff --git a/src/factory/Factory.ts b/src/factory/Factory.ts index 920af1a..57c169a 100644 --- a/src/factory/Factory.ts +++ b/src/factory/Factory.ts @@ -6,8 +6,22 @@ import type { MutexFactoryInterface } from "../interfaces/MutexFactoryInterface. import type { StatemachineInterface } from "../interfaces/StatemachineInterface.js"; import type { BeforeTransitionObserver } from "../interfaces/BeforeTransitionObserverInterface.js"; import type { AfterTransitionObserver } from "../interfaces/AfterTransitionObserverInterface.js"; +import type { StatemachineOptions } from "../interfaces/StatemachineOptions.js"; import { Statemachine } from "../Statemachine.js"; +/** + * Engine options applied to every machine the factory creates. + * + * `initialStateName`, `mutex` and `transitionSelector` are excluded: the + * factory derives them per subject from the state-name detector, the mutex + * factory and setTransitionSelector, so a template value could only + * contradict them. + */ +export type FactoryStatemachineOptions = Omit< + StatemachineOptions, + "initialStateName" | "mutex" | "transitionSelector" +>; + export class Factory implements FactoryInterface { private readonly processDetector: ProcessDetectorInterface; private readonly stateNameDetector: StateNameDetectorInterface | null; @@ -18,13 +32,23 @@ export class Factory implements FactoryInterface { private transitionSelector: TransitionSelectorInterface | null = null; private mutexFactory: MutexFactoryInterface | null = null; + private readonly options: FactoryStatemachineOptions; + /** + * @param options Engine options applied to every machine this factory + * creates — back-pressure (maxQueueLength), the automatic-hop bound, lock + * autorelease, and the onChainedOperationError / onReleaseError diagnostic + * sinks. Without them, factory-created machines would silently run on + * defaults, which is precisely where those sinks matter most. + */ constructor( processDetector: ProcessDetectorInterface, stateNameDetector?: StateNameDetectorInterface | null, + options: FactoryStatemachineOptions = {}, ) { this.processDetector = processDetector; this.stateNameDetector = stateNameDetector ?? null; + this.options = { ...options }; } setMutexFactory(factory: MutexFactoryInterface | null): void { @@ -63,6 +87,7 @@ export class Factory implements FactoryInterface { : undefined; const sm = new Statemachine(subject, process, { + ...this.options, initialStateName: stateName ?? undefined, transitionSelector: this.transitionSelector ?? undefined, mutex: mutex ?? undefined, diff --git a/src/factory/index.ts b/src/factory/index.ts index 1f7e9f1..2f5537b 100644 --- a/src/factory/index.ts +++ b/src/factory/index.ts @@ -1,4 +1,5 @@ export { Factory } from "./Factory.js"; +export type { FactoryStatemachineOptions } from "./Factory.js"; export { SingleProcessDetector } from "./SingleProcessDetector.js"; export { AbstractNamedProcessDetector } from "./AbstractNamedProcessDetector.js"; export { StatefulStateNameDetector } from "./StatefulStateNameDetector.js"; diff --git a/src/index.ts b/src/index.ts index b21eda4..f2359c1 100644 --- a/src/index.ts +++ b/src/index.ts @@ -95,6 +95,7 @@ export { AbstractNamedProcessDetector, StatefulStateNameDetector, } from "./factory/index.js"; +export type { FactoryStatemachineOptions } from "./factory/index.js"; // Graph export { GraphBuilder } from "./graph/index.js"; @@ -112,6 +113,7 @@ export { FinitaError, WrongEventForStateError, LockCanNotBeAcquiredError, + LockCanNotBeReleasedError, DuplicateStateError, ProcessFinalizedError, GraphValidationError, @@ -128,4 +130,5 @@ export { export type { GraphValidationCode, DuplicateTransitionConflict, + AmbiguousTransitionCandidate, } from "./error/index.js"; diff --git a/src/interfaces/DispatcherInterface.ts b/src/interfaces/DispatcherInterface.ts index f8310b2..c986614 100644 --- a/src/interfaces/DispatcherInterface.ts +++ b/src/interfaces/DispatcherInterface.ts @@ -1,12 +1,12 @@ import type { EventInterface } from "./EventInterface.js"; import type { MaybePromise } from "../MaybePromise.js"; -/** @deprecated No longer used internally; will be removed in v4. */ +/** @deprecated No longer used internally; will be removed in v5. */ export interface CallbackInterface { invoke(): MaybePromise; } -/** @deprecated No longer used internally; will be removed in v4. */ +/** @deprecated No longer used internally; will be removed in v5. */ export interface DispatcherInterface extends CallbackInterface { dispatch(event: EventInterface, args?: unknown[]): void; invoke(): Promise; diff --git a/src/mutex/LockAdapterMutex.ts b/src/mutex/LockAdapterMutex.ts index 20ac3af..9fa9f72 100644 --- a/src/mutex/LockAdapterMutex.ts +++ b/src/mutex/LockAdapterMutex.ts @@ -5,17 +5,33 @@ export class LockAdapterMutex implements MutexInterface { private readonly lockAdapter: LockAdapterInterface; private readonly resourceName: string; private acquired = false; + private pendingAcquire: Promise | null = null; constructor(lockAdapter: LockAdapterInterface, resourceName: string) { this.lockAdapter = lockAdapter; this.resourceName = resourceName; } + /** + * Overlapping calls share one underlying acquire: the `acquired` flag is + * only set after the adapter resolves, so without this both callers would + * pass the check and acquire twice on a non-idempotent adapter (database + * advisory locks, redis SET NX). The pending promise is cleared once it + * settles, so a failed acquire can still be retried. + */ async acquireLock(): Promise { - if (!this.acquired) { - this.acquired = await this.lockAdapter.acquireLock(this.resourceName); + if (this.acquired) { + return true; } - return this.acquired; + this.pendingAcquire ??= (async () => { + try { + this.acquired = await this.lockAdapter.acquireLock(this.resourceName); + return this.acquired; + } finally { + this.pendingAcquire = null; + } + })(); + return this.pendingAcquire; } async releaseLock(): Promise { diff --git a/src/observer/CallbackObserver.ts b/src/observer/CallbackObserver.ts index ce246a4..a607089 100644 --- a/src/observer/CallbackObserver.ts +++ b/src/observer/CallbackObserver.ts @@ -2,11 +2,11 @@ import type { Observer, ObservableSubject } from "../interfaces/Observer.js"; import type { MaybePromise } from "../MaybePromise.js"; /** - * Legacy Observer for Event observers (commands attached to specific events). + * Observer for Event observers (commands attached to specific events). * - * In v3 this is no longer used as a Statemachine observer. To run a - * callback after every transition, implement AfterTransitionObserver - * directly or compose a small wrapper. + * This is not a Statemachine observer. To run a callback after every + * transition, implement AfterTransitionObserver directly or compose a small + * wrapper. */ export class CallbackObserver implements Observer { private readonly callback: (...args: unknown[]) => MaybePromise; diff --git a/src/selector/OneOrNoneActiveTransition.ts b/src/selector/OneOrNoneActiveTransition.ts index 15159e7..8b4f113 100644 --- a/src/selector/OneOrNoneActiveTransition.ts +++ b/src/selector/OneOrNoneActiveTransition.ts @@ -15,7 +15,15 @@ export class OneOrNoneActiveTransition< case 1: return arr[0]; default: - throw new AmbiguousTransitionError(arr.length); + throw new AmbiguousTransitionError( + arr.length, + arr.map((transition) => ({ + targetStateName: transition.getTargetState().getName(), + eventName: transition.getEventName(), + conditionName: transition.getConditionName(), + weight: transition.getWeight(), + })), + ); } } } diff --git a/tests/factory-options.test.ts b/tests/factory-options.test.ts new file mode 100644 index 0000000..4152315 --- /dev/null +++ b/tests/factory-options.test.ts @@ -0,0 +1,143 @@ +import { describe, it, expect } from "vitest"; +import { + ProcessBuilder, + Factory, + SingleProcessDetector, + Tautology, + QueueLimitExceededError, + LockCanNotBeReleasedError, +} from "../src/index.js"; +import type { MutexInterface, ProcessInterface } from "../src/index.js"; + +const simpleProcess = (): ProcessInterface => + new ProcessBuilder("p") + .addState("a", { initial: true }) + .addState("b") + .addTransition("a", "b", { event: "go" }) + .build(); + +describe("Factory forwards StatemachineOptions to the machines it creates", () => { + it("applies autoreleaseLock", async () => { + const factory = new Factory( + new SingleProcessDetector(simpleProcess()), + null, + { + autoreleaseLock: false, + }, + ); + const sm = await factory.createStatemachine({}); + expect(sm.isAutoreleaseLock()).toBe(false); + }); + + it("applies maxQueueLength as back-pressure", async () => { + const factory = new Factory( + new SingleProcessDetector(simpleProcess()), + null, + { + maxQueueLength: 1, + }, + ); + const sm = await factory.createStatemachine({}); + // First op is already running, second waits in the queue, third exceeds + // the limit and is rejected rather than queued. + const first = sm.triggerEvent("go"); + const second = sm.triggerEvent("go"); + await expect(sm.triggerEvent("go")).rejects.toBeInstanceOf( + QueueLimitExceededError, + ); + await Promise.allSettled([first, second]); + }); + + it("applies maxAutomaticHops", async () => { + const looping = new ProcessBuilder("loop") + .addState("a", { initial: true }) + .addState("b") + .addTransition("a", "b", { condition: new Tautology() }) + .addTransition("b", "a", { condition: new Tautology() }) + .build(); + const factory = new Factory(new SingleProcessDetector(looping), null, { + maxAutomaticHops: 3, + }); + const sm = await factory.createStatemachine({}); + // Assert the configured limit, not just the error type — the default of + // 100 would raise the same error and hide a dropped option. + await expect(sm.checkTransitions()).rejects.toMatchObject({ + name: "AutomaticTransitionCycleError", + hopLimit: 3, + }); + }); + + it("applies onReleaseError", async () => { + class FalseReleaseMutex implements MutexInterface { + private acquired = false; + async acquireLock(): Promise { + this.acquired = true; + return true; + } + async releaseLock(): Promise { + return false; + } + isAcquired(): boolean { + return this.acquired; + } + async isLocked(): Promise { + return this.acquired; + } + } + const releaseErrors: unknown[] = []; + const factory = new Factory( + new SingleProcessDetector(simpleProcess()), + null, + { + onReleaseError: (err) => releaseErrors.push(err), + }, + ); + factory.setMutexFactory({ createMutex: () => new FalseReleaseMutex() }); + const sm = await factory.createStatemachine({}); + + await expect(sm.triggerEvent("go")).rejects.toBeInstanceOf( + LockCanNotBeReleasedError, + ); + expect(releaseErrors).toHaveLength(1); + }); + + it("applies onChainedOperationError", async () => { + const process = new ProcessBuilder("p") + .addState("a", { initial: true }) + .addState("b") + .addTransition("a", "b", { event: "go" }) + .build(); + const chainedErrors: unknown[] = []; + const factory = new Factory(new SingleProcessDetector(process), null, { + onChainedOperationError: (err) => chainedErrors.push(err), + }); + // The chained event does not exist on the target state, so the chained + // operation fails — its error is only observable through the sink. + factory.attachAfterObserver({ + notify: (_frame, ctx) => { + ctx.enqueue("missing"); + }, + }); + const sm = await factory.createStatemachine({}); + await sm.triggerEvent("go"); + await sm.whenIdle(); + expect(chainedErrors).toHaveLength(1); + }); + + it("excludes factory-owned settings from the template at compile time", async () => { + // initialStateName, mutex and transitionSelector are derived by the + // factory itself (from the state-name detector, the mutex factory and + // setTransitionSelector), so the template must not be able to contradict + // them. tsconfig.test.json type-checks this file, so these are enforced. + const factory = new Factory( + new SingleProcessDetector(simpleProcess()), + null, + { + // @ts-expect-error initialStateName comes from the state-name detector + initialStateName: "b", + }, + ); + const sm = await factory.createStatemachine({}); + expect(sm.getCurrentState().getName()).toBe("a"); + }); +}); diff --git a/tests/mutex.test.ts b/tests/mutex.test.ts index 2668794..f404e3a 100644 --- a/tests/mutex.test.ts +++ b/tests/mutex.test.ts @@ -143,3 +143,56 @@ describe("Statemachine mutex regression", () => { expect(mutex.isAcquired()).toBe(false); }); }); + +describe("LockAdapterMutex concurrent acquire", () => { + it("acquires the underlying lock once when two acquires overlap", async () => { + let adapterAcquires = 0; + const adapter: LockAdapterInterface = { + async acquireLock(): Promise { + adapterAcquires += 1; + await new Promise((r) => setTimeout(r, 10)); + return true; + }, + async releaseLock(): Promise { + return true; + }, + async isLocked(): Promise { + return true; + }, + }; + const mutex = new LockAdapterMutex(adapter, "resource"); + + // Both calls start before either resolves — the `!this.acquired` check + // alone lets both through and double-acquires a non-idempotent adapter. + const [first, second] = await Promise.all([ + mutex.acquireLock(), + mutex.acquireLock(), + ]); + + expect(first).toBe(true); + expect(second).toBe(true); + expect(adapterAcquires).toBe(1); + expect(mutex.isAcquired()).toBe(true); + }); + + it("retries after a failed acquire", async () => { + let adapterAcquires = 0; + const adapter: LockAdapterInterface = { + async acquireLock(): Promise { + adapterAcquires += 1; + return adapterAcquires > 1; // first attempt fails, later ones succeed + }, + async releaseLock(): Promise { + return true; + }, + async isLocked(): Promise { + return false; + }, + }; + const mutex = new LockAdapterMutex(adapter, "resource"); + + expect(await mutex.acquireLock()).toBe(false); + expect(await mutex.acquireLock()).toBe(true); + expect(adapterAcquires).toBe(2); + }); +}); diff --git a/tests/observer-snapshot.test.ts b/tests/observer-snapshot.test.ts new file mode 100644 index 0000000..87eb03d --- /dev/null +++ b/tests/observer-snapshot.test.ts @@ -0,0 +1,76 @@ +import { describe, it, expect } from "vitest"; +import { + ProcessBuilder, + Statemachine, + CallbackObserver, +} from "../src/index.js"; +import type { AfterTransitionObserver } from "../src/index.js"; + +const build = () => + new ProcessBuilder("p") + .addState("a", { initial: true }) + .addState("b") + .addTransition("a", "b", { event: "go" }) + .build(); + +describe("observer accessors hand out snapshots, not live collections", () => { + it("an already-returned after-observer list is unaffected by a later detach", () => { + const sm = new Statemachine({}, build()); + const first: AfterTransitionObserver = { notify: () => {} }; + const second: AfterTransitionObserver = { notify: () => {} }; + sm.attachAfter(first); + sm.attachAfter(second); + + const observers = sm.getAfterObservers(); + sm.detachAfter(second); + + expect([...observers]).toHaveLength(2); + expect([...sm.getAfterObservers()]).toHaveLength(1); + }); + + it("an already-returned before-observer list is unaffected by a later detach", () => { + const sm = new Statemachine({}, build()); + const first = { notify: () => {} }; + const second = { notify: () => {} }; + sm.attachBefore(first); + sm.attachBefore(second); + + const observers = sm.getBeforeObservers(); + sm.detachBefore(second); + + expect([...observers]).toHaveLength(2); + expect([...sm.getBeforeObservers()]).toHaveLength(1); + }); + + it("an already-returned event observer list is unaffected by a later detach", () => { + const process = build(); + const event = process.getState("a").getEvent("go"); + const first = new CallbackObserver(() => {}); + const second = new CallbackObserver(() => {}); + event.attach(first); + event.attach(second); + + const observers = event.getObservers(); + event.detach(second); + + expect([...observers]).toHaveLength(2); + expect([...event.getObservers()]).toHaveLength(1); + }); + + it("mutating a returned list does not change the machine's observers", async () => { + const sm = new Statemachine({}, build()); + let calls = 0; + sm.attachAfter({ + notify: () => { + calls += 1; + }, + }); + + // A caller that casts the Iterable back to an array must not be able to + // clear the engine's own registrations. + (sm.getAfterObservers() as AfterTransitionObserver[]).length = 0; + + await sm.triggerEvent("go"); + expect(calls).toBe(1); + }); +}); diff --git a/tests/reentrancy.test.ts b/tests/reentrancy.test.ts index 3d40bcb..82584c0 100644 --- a/tests/reentrancy.test.ts +++ b/tests/reentrancy.test.ts @@ -132,6 +132,43 @@ describe("re-entrant triggerEvent from an observer", () => { expect(sm.getCurrentState().getName()).toBe("b"); }); + it("throws ReentrancyError when an observer awaits whenIdle()", async () => { + // whenIdle() from inside a callback can never resolve: the machine cannot + // reach idle while the runner is blocked on that very callback. + const process = new ProcessBuilder("p") + .addState("a", { initial: true }) + .addState("b") + .addTransition("a", "b", { event: "go" }) + .build(); + const sm = new Statemachine({}, process); + + const observer = { + async notify(_frame: TransitionFrame): Promise { + await sm.whenIdle(); // forbidden — would deadlock + }, + }; + sm.attachAfter(observer); + + await expect(sm.triggerEvent("go")).rejects.toBeInstanceOf(ReentrancyError); + + // The machine is NOT deadlocked: it still drains once the observer is gone. + sm.detachAfter(observer); + await sm.whenIdle(); + expect(sm.getCurrentState().getName()).toBe("b"); + }, 2000); + + it("still resolves whenIdle() for external callers", async () => { + const process = new ProcessBuilder("p") + .addState("a", { initial: true }) + .addState("b") + .addTransition("a", "b", { event: "go" }) + .build(); + const sm = new Statemachine({}, process); + void sm.triggerEvent("go"); + await sm.whenIdle(); // outside any callback — must resolve normally + expect(sm.getCurrentState().getName()).toBe("b"); + }, 2000); + it("does not flag a benign observer that awaits non-reentrant work", async () => { const process = new ProcessBuilder("p") .addState("a", { initial: true }) diff --git a/tests/resolve-after-release.test.ts b/tests/resolve-after-release.test.ts index f4e99ed..f28a76d 100644 --- a/tests/resolve-after-release.test.ts +++ b/tests/resolve-after-release.test.ts @@ -3,6 +3,7 @@ import { ProcessBuilder, Statemachine, WrongEventForStateError, + LockCanNotBeReleasedError, } from "../src/index.js"; import type { MutexInterface } from "../src/index.js"; @@ -88,3 +89,87 @@ describe("releaseLock failures", () => { ); }); }); + +/** + * Mutex whose release fails by RETURNING FALSE rather than throwing — the + * failure signal defined by LockAdapterInterface, and what the documented + * PostgreSQL advisory-lock adapter produces on a failed unlock. + */ +class FalseReleaseMutex implements MutexInterface { + acquireCount = 0; + releaseCount = 0; + private acquired = false; + async acquireLock(): Promise { + this.acquireCount += 1; + if (this.acquired) return false; + this.acquired = true; + return true; + } + async releaseLock(): Promise { + this.releaseCount += 1; + return false; // release failed; the lock is still held + } + isAcquired(): boolean { + return this.acquired; + } + async isLocked(): Promise { + return this.acquired; + } +} + +describe("releaseLock reporting failure by returning false", () => { + const buildProcess = () => + new ProcessBuilder("p") + .addState("a", { initial: true }) + .addState("b") + .addTransition("a", "b", { event: "go" }) + .build(); + + it("rejects the operation instead of resolving as if the lock were freed", async () => { + const sm = new Statemachine({}, buildProcess(), { + mutex: new FalseReleaseMutex(), + }); + // Silently resolving would let every later operation piggyback on — and + // never release — a lock the engine believes it dropped. + await expect(sm.triggerEvent("go")).rejects.toBeInstanceOf( + LockCanNotBeReleasedError, + ); + // The transition itself committed before the release ran. + expect(sm.getCurrentState().getName()).toBe("b"); + }); + + it("reports the failure through onReleaseError", async () => { + const releaseErrors: unknown[] = []; + const sm = new Statemachine({}, buildProcess(), { + mutex: new FalseReleaseMutex(), + onReleaseError: (err) => releaseErrors.push(err), + }); + await expect(sm.triggerEvent("go")).rejects.toBeInstanceOf( + LockCanNotBeReleasedError, + ); + expect(releaseErrors).toHaveLength(1); + expect(releaseErrors[0]).toBeInstanceOf(LockCanNotBeReleasedError); + }); + + it("does not mask an operation error with the release failure", async () => { + const sm = new Statemachine({}, buildProcess(), { + mutex: new FalseReleaseMutex(), + }); + await expect(sm.triggerEvent("nope")).rejects.toBeInstanceOf( + WrongEventForStateError, + ); + }); + + it("reports a failed manual release through onReleaseError", async () => { + const releaseErrors: unknown[] = []; + const sm = new Statemachine({}, buildProcess(), { + mutex: new FalseReleaseMutex(), + autoreleaseLock: false, + onReleaseError: (err) => releaseErrors.push(err), + }); + await sm.acquireLock(); + await sm.releaseLock(); + expect(releaseErrors).toHaveLength(1); + expect(releaseErrors[0]).toBeInstanceOf(LockCanNotBeReleasedError); + }); +}); diff --git a/tests/typed-throws.test.ts b/tests/typed-throws.test.ts index e6eb4b8..269e283 100644 --- a/tests/typed-throws.test.ts +++ b/tests/typed-throws.test.ts @@ -143,6 +143,46 @@ describe("typed throws", () => { // selector.test.ts asserts the message contains "More than one" expect(e.message).toContain("More than one"); }); + + it("carries the candidate transitions that caused the ambiguity", async () => { + const { OneOrNoneActiveTransition } = + await import("../src/selector/OneOrNoneActiveTransition.js"); + const { CallbackCondition } = + await import("../src/condition/CallbackCondition.js"); + const process = new ProcessBuilder("p") + .addState("a", { initial: true }) + .addState("b") + .addState("c") + .addTransition("a", "b", { event: "go" }) + .addTransition("a", "c", { + event: "go", + condition: new CallbackCondition("isVip", () => true), + weight: 7, + }) + .build(); + const transitions = Array.from(process.getState("a").getTransitions()); + let caught: unknown; + try { + new OneOrNoneActiveTransition().selectTransition(transitions); + } catch (err) { + caught = err; + } + const e = caught as AmbiguousTransitionError; + // Debugging an ambiguity needs the candidates, not just how many. + expect(e.candidates.map((c) => c.targetStateName).sort()).toEqual([ + "b", + "c", + ]); + expect(e.candidates).toContainEqual({ + targetStateName: "c", + eventName: "go", + conditionName: "isVip", + weight: 7, + }); + expect(e.message).toContain('"b"'); + expect(e.message).toContain('"c"'); + expect(e.message).toContain("isVip"); + }); }); describe("Statemachine automatic-cycle detection", () => { From c8b841592490a73acb839df3dd9cfa3f8f9af5f1 Mon Sep 17 00:00:00 2001 From: Carlos Cima Date: Wed, 19 Aug 2026 06:29:47 -0400 Subject: [PATCH 2/4] docs: add architecture review with remediation status MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Full-tree review of v4.1.0: two reproduced bugs, API-consistency gaps, and a deferred list for the next major. Records what this branch fixed and, for each deferred item, why — the breaking ones (Event mutability, the OnEnterObserver event-name convention, the machineName misnomer, the releaseLock return type) belong to a planned v5 scope, and the mermaid label quoting needs verification in a real renderer before changing output that tests and docs pin. Follows the precedent of CODE_REVIEW.md from the previous review cycle. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01JLxceiNFfXfpGoxdfQEZqw --- ARCHITECTURE_REVIEW.md | 185 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 185 insertions(+) create mode 100644 ARCHITECTURE_REVIEW.md diff --git a/ARCHITECTURE_REVIEW.md b/ARCHITECTURE_REVIEW.md new file mode 100644 index 0000000..521b1fa --- /dev/null +++ b/ARCHITECTURE_REVIEW.md @@ -0,0 +1,185 @@ +# Architecture Review — finita v4.1.0 + +**Date:** 2026-08-18 +**Scope:** Full `src/` tree (~3,000 lines, 90 files) at `main` (9932a58), plus docs, tests, packaging, and CI. +**Method:** Complete read of every source file by a single reviewer, cross-checked against the June 2026 review (`CODE_REVIEW.md`) and its remediation. Both confirmed bugs below were **reproduced empirically** with throwaway test files (since deleted). Baseline verified before review: `pnpm lint` clean, `pnpm test` 369/369 passing across 46 files. + +--- + +## Executive summary + +This is a mature, unusually well-hardened library for its size. The June v3.0.1 review's ten findings are **all fixed** in v4 (verified individually — see the table at the end), the graph model is genuinely immutable, the operation-queue engine is carefully reasoned about, and the test suite pins edge cases most libraries never think about. + +This pass found **two confirmed bugs** — both in the newest code (the v4.1.0 lock-diagnostics and idle-await features), both reproduced: + +1. A mutex `releaseLock()` that **returns `false`** (the documented failure signal of `LockAdapterInterface`) is silently ignored — producing exactly the silent stuck-lock scenario the v4.1.0 `onReleaseError` work was built to prevent. +2. `whenIdle()` is missing the re-entrancy guard that `triggerEvent`/`checkTransitions` received in v4 — awaiting it inside an observer deadlocks the machine permanently and silently. + +Beyond those, the findings are API-consistency gaps (the `Factory` can't configure v4's engine options; `Event` is the one mutable back door in the frozen graph) and smaller cleanups. Nothing here threatens the core transition semantics. + +--- + +## Confirmed bugs + +### 1. `releaseLock()` returning `false` is silently swallowed → silent stuck lock — **High** + +**Where:** `src/Statemachine.ts:292-310` (the release block in `runOperation`). + +**Contract mismatch:** `MutexInterface.releaseLock()` returns `MaybePromise`, and `false` means the release failed. `LockAdapterMutex.releaseLock()` (`src/mutex/LockAdapterMutex.ts:23-32`) returns the adapter's `false` and keeps `acquired = true`. The engine, however, handles only the _throwing_ release path: + +```ts +await this.mutex.releaseLock(); // boolean result discarded +``` + +**Reproduced:** with a mutex whose release returns `false`: + +- the operation **resolves successfully** — the caller learns nothing; +- `onReleaseError` never fires (it only sees exceptions); +- `isLockAcquired()` stays `true` forever; +- every subsequent operation sees `isAcquired() === true`, skips acquisition (`acquiredHere = false`), and therefore **never releases** — the exact "every later operation silently piggybacks on the stuck lock" scenario the inline comment at `src/Statemachine.ts:303-308` says must not happen. + +This is not a contrived mutex: the project's own PostgreSQL advisory-lock example in `docs/mutex.md` returns `rows[0].released === true` — i.e. `false` on a failed unlock — and `tests/resolve-after-release.test.ts` pins only the throwing path. + +**Recommendation:** treat `false` exactly like a thrown release error: + +```ts +let released = false; +try { + released = await this.mutex.releaseLock(); +} catch (err) { + /* existing handling */ +} +if (!released && !failure) { + /* synthesize error, call onReleaseError, set failure */ +} +``` + +Add the `false`-return case to `resolve-after-release.test.ts`, and extend the "Release Error Behavior" section of `docs/mutex.md` (it currently only covers throws). + +### 2. `whenIdle()` has no re-entrancy guard → silent permanent deadlock — **Medium-High** + +**Where:** `src/Statemachine.ts:184-191`. + +`triggerEvent` and `checkTransitions` call `assertNotReentrant(...)` so that a synchronous re-entrant call from an observer/condition throws `ReentrancyError` instead of deadlocking (the v4 fix for June finding #4). `whenIdle()` — added in the same release — did not get the guard, and calling it from inside a callback is _always_ a deadlock: the machine cannot become idle while the runner is awaiting that callback. + +**Reproduced:** an after-observer that does `await sm.whenIdle()` hangs forever. The transition commits, the caller's `triggerEvent` promise never settles, no error is ever surfaced, and the runner stays blocked, so every future operation enqueues and never runs — the machine is permanently wedged. + +**Recommendation:** add `this.assertNotReentrant("whenIdle()")` at the top of `whenIdle()` (before the fast-path return). This gives the same synchronous-portion coverage as the other guards, with the same documented post-`await` gap — acceptable parity. + +--- + +## API-consistency and design gaps + +### 3. `Statemachine.releaseLock(): Promise` discards the mutex result + +`src/Statemachine.ts:145-147` awaits `mutex.releaseLock()` and drops the boolean; `StatemachineInterface` declares `Promise`. Users doing manual lock management (`autoreleaseLock: false` — the documented batch pattern in `docs/mutex.md`) have **no way to detect a failed release** short of interrogating the mutex directly. Companion to bug #1; recommend returning `Promise` (breaking, so next major) or throwing on failure. + +### 4. `Factory` cannot configure the v4 engine options + +`Factory.createStatemachine` (`src/factory/Factory.ts:55-76`) passes only `initialStateName`, `transitionSelector`, and `mutex`. There is no way to give factory-created machines `maxQueueLength`, `maxAutomaticHops`, `autoreleaseLock`, `onChainedOperationError`, or `onReleaseError`. The fleet-of-machines use case is precisely where the diagnostics sinks and back-pressure matter most — a service creating a machine per order gets _none_ of the v4 hardening unless it abandons the Factory. Recommend a `StatemachineOptions` template on the Factory (constructor parameter or `setDefaultOptions()`), with the factory-owned fields (`initialStateName`, selector, mutex) layered on top. + +### 5. `Event` is the mutable back door in an otherwise frozen graph + +`State`, `Transition`, and `Process` are frozen at build time and construction-key protected — a genuinely strong invariant. `Event` breaks it: `attach()`, `detach()`, `setMetadataValue()`, `deleteMetadataValue()` are mutable forever, and the documented way to register commands is post-build mutation (`process.getState("draft").getEvent("publish").attach(...)`, per `docs/observers.md`). + +Consequences: + +- **Cross-machine shared state remains.** Events live on the shared `Process` graph, so event-attached observers and event metadata are global to every machine built from that process. The v4 fix for June finding #1 removed the _per-invocation args_ race, but the observer set and metadata are still shared mutable state — attaching a command "for" one machine attaches it to all of them. +- **`Event.invoke()` / `Event.notify()` are public** and bypass the engine entirely: no queue, no mutex, no re-entrancy guard. A user who calls `event.invoke(...)` directly gets observer side effects outside every serialization guarantee the engine provides. + +**Recommendation (next major):** register commands at build time (`builder.addCommand(state, event, cb)` or similar), freeze `Event` with the rest of the graph, and drop `invoke`/`notify` from the public surface (or move event observers into the `Statemachine`, making them per-machine). Meanwhile, document the sharing hazard where `attach()` is taught. + +### 6. `OnEnterObserver` still rides on a magic event-name convention + +Carried over from the June review's design note, still open: an entry hook on state `approved` requires declaring a **sentinel self-transition** `addTransition("approved", "approved", { event: "onEnter" })` (the documented pattern in `docs/observers.md:327`). These sentinel edges pollute `GraphBuilder` exports, can collide with a real event named `onEnter`, and lean on the (intentional but subtle) no-op semantics of self-transitions. The `ifStateName` mechanism fixed the wrong-state bug; the deeper fix — first-class per-state entry hooks registered at build time — remains worth doing in the next major. + +--- + +## Minor findings + +7. **Live internal collections escape.** `getBeforeObservers()`/`getAfterObservers()` (`src/Statemachine.ts:121-123, 135-137`) return the live arrays; `Event.getObservers()` returns the live `Set`. The engine itself iterates snapshots, but external callers can observe mid-mutation state or cast-and-mutate. Return copies (the arrays are tiny) or a read-only wrapper. +8. **Deprecation debt is past its promised date.** `DispatcherInterface`/`CallbackInterface` are marked _"will be removed in v4"_ (`src/interfaces/DispatcherInterface.ts`) yet are still exported at v4.1.0; `Event.getInvokeArgs()` is a deprecated stub; `CallbackObserver`'s doc comment still says "In v3…". Remove in the next major, and re-date the promises now. +9. **Redundant cast.** `src/Statemachine.ts:370-372`: the `as TransitionInterface | null` on the selector result is unnecessary — verified that `tsc --noEmit` passes without it. +10. **`AmbiguousTransitionError` carries only a count.** Sibling errors (`StateNotFoundError`, `ProcessNotFoundError`) carry rich context; this one omits the state name, event, and candidate target names — the three things you need to debug an ambiguity. Cheap DX win. +11. **`TransitionFrame.machineName` is a misnomer.** It is the _process_ name (`src/Statemachine.ts:400`) — every machine sharing the process reports the same value — and it is typed `string | null` but never null. Rename to `processName` in the next major (or document the alias). +12. **`LockAdapterMutex` acquire race.** Two overlapping `acquireLock()` calls both pass the `!this.acquired` check before either await resolves, double-acquiring on a non-idempotent adapter (`src/mutex/LockAdapterMutex.ts:15-20`). The engine serializes its own calls, but the method is public. Memoizing the in-flight acquire promise closes it. +13. **`readonlyContext` is compile-time-only immutability**, and the single per-frame copy is shared by all observers of that frame — one observer casting and mutating affects later observers (`src/Statemachine.ts:464-471`). Acceptable trade-off; worth a sentence in the observer docs. +14. **`OperationQueue.dequeue` uses `Array.shift()`** — O(n) per dequeue. Irrelevant at sane queue depths; noted only because `maxQueueLength` defaults to `Infinity`. A ring buffer or head index is a 10-line fix if it ever matters. +15. **Mermaid output quotes state descriptions** — `s_x : "label"` (`src/graph/GraphBuilder.ts:177-178`). In `stateDiagram-v2` the text after `:` renders verbatim, so the quotes likely appear in the rendered diagram. Cosmetic; verify in a renderer and drop the quotes if unintended. + +--- + +## Improvement opportunities + +- **Close the post-`await` re-entrancy gap with `AsyncLocalStorage`.** `guardSync` (documented at `src/Statemachine.ts:193-198`) only catches re-entrant calls made before a callback's first `await`. `engines` requires Node ≥ 20, where ALS is stable; an opt-in (injected guard strategy, so the core stays runtime-agnostic for browser bundles) would convert the remaining silent deadlocks into `ReentrancyError`s. +- **`Timeout` is a passive condition** — it never schedules anything; someone must poll `checkTransitions()`. This is easy to miss from the class name. Document it prominently and/or ship a small opt-in scheduler helper (interval-driven `checkTransitions` with `whenIdle` coordination). +- **Introspection:** there is no way to ask the machine its queue depth or whether it is draining. Once teams set `maxQueueLength`, they will want a `getQueueLength()` / `isRunning()` for metrics. +- **`Observer.update(subject, args)` naming:** `subject` is the observable _Event_, while the machine's domain subject arrives as `args[0]`. Two meanings of "subject" in one call signature; rename the parameter (`source`?) in the next major. +- **Packaging:** the dual ESM/CJS setup (tsup, `exports` map with per-condition `types`) looks correct. Add an `@arethetypeswrong/cli` check to CI to lock it in. +- **Docs:** overall excellent (per-module references, migration guides, behavior-change notes). Gaps found: release-failure semantics (see bug #1), and the `Event.attach` cross-machine sharing hazard (see #5). + +--- + +## Strengths worth preserving + +- **Frozen graph, two-phase construction, symbol construction key.** The `State`/`Transition`/`Process` graph is immutable, cycle-safe by construction, and only obtainable through the validating builder. This is the architectural backbone and it is done right. +- **Builder validation quality:** unified name rules, endpoint checks, initial-state cardinality, conflict-vs-idempotent-duplicate distinction with a shared identity key, opt-in orphan detection — with typed, machine-readable errors (`GraphValidationError.code`) throughout. +- **The operation queue engine:** single enqueue entry point, a documented completion boundary (caller resolves only after lock release), chained-operation semantics with an error sink, back-pressure, and idle waiters. The inline comments state _contracts and constraints_, not narration — rare and valuable. +- **Error hierarchy:** every `FinitaError` subclass carries a stable `code` plus structured fields; native `RangeError` is consistently reserved for programmer errors (bad option values). +- **Test discipline:** 46 files / 369 tests, roughly one file per pinned behavior, including regression reproductions of past review findings (`resolve-after-release`, `event-args-race`, `late-enqueue`, `weight-selector-order`…). +- **Zero runtime dependencies**, dual-format packaging, CI matrix on Node 20/22/24 with CodeQL, OSV-Scanner, and gitleaks. + +--- + +## Status of the June 2026 review (v3.0.1, `CODE_REVIEW.md`) + +All ten findings verified fixed in the current source: + +| # | Finding (abbreviated) | Status in v4.1.0 | +| --- | -------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------- | +| 1 | `Event.invokeArgs` shared-state race | Fixed — args passed through `Observer.update(subject, args)`; `getInvokeArgs()` is a deprecated stub | +| 2 | `StatefulStatusChanger` bound to one subject | Fixed — `frame.subject` on the frame; observer defaults to it | +| 3 | `onEnter` resolves against wrong state | Fixed — `ifStateName` guard on chained ops | +| 4 | Re-entrant `triggerEvent` deadlock | Fixed — `ReentrancyError` via `guardSync` (documented post-`await` gap remains; see bug #2 for the `whenIdle` omission) | +| 5 | Cycle detector rejects legitimate loops | Fixed — `maxAutomaticHops` bound replaces the visited set | +| 6 | `op.resolve()` before lock release | Fixed — resolve/reject moved after the `finally`; pinned by test | +| 7 | `EnqueueContext.enqueue` strands late ops | Fixed — all enqueues route through `enqueueOperation`, which kicks the runner | +| 8 | Duplicate check ignored weight | Fixed — weight in the conflict check | +| 9 | `WeightTransition` order-dependent epsilon tie | Fixed — true max first, then epsilon window | +| 10 | Empty-string `initialStateName` silently discarded | Fixed — `!== undefined` plus builder-level state-name validation | + +The "Cleanup" section items also landed (shared composite base, unified name validation, shared `nameOrString` util, `Timeout` typed error + single `Date.now()`, `Dispatcher` deleted, `enqueueOperation` helper, shared transition key). The one open design note is the `OnEnterObserver` convention (finding #6 above). + +--- + +## Suggested priority + +1. **Bug #1** — handle `releaseLock() === false` (small, testable, closes a silent distributed-lock integrity hole). +2. **Bug #2** — guard `whenIdle()` (one line plus a test). +3. **#3** — decide the `releaseLock` return-type story alongside #1; update `docs/mutex.md`. +4. **#4** — Factory options passthrough (non-breaking, high leverage for the fleet use case). +5. Batch the next-major items (#5, #6, #8, #11, `Observer.update` rename) into a planned v5 scope rather than fixing piecemeal. + +--- + +## Remediation status + +Everything non-breaking was fixed on `fix/architecture-review-findings`; each fix landed test-first, with the failing test observed before the change. + +| # | Finding | Status | +| -------- | ---------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------- | +| Bug 1 | `releaseLock()` returning false swallowed | **Fixed** — new `LockCanNotBeReleasedError`; both failure modes normalized in one path | +| Bug 2 | `whenIdle()` deadlock | **Fixed** — guarded with `assertNotReentrant` | +| 3 | `releaseLock()` discards the mutex result | **Partly fixed** — failures now reach `onReleaseError`; the `Promise` return type is deferred to v5 as breaking | +| 4 | Factory cannot configure engine options | **Fixed** — `FactoryStatemachineOptions` constructor template | +| 7 | Live internal collections escape | **Fixed** — observer accessors return snapshots | +| 8 | Deprecation debt | **Partly fixed** — notices re-dated to v5; removal is itself breaking | +| 9 | Redundant cast | **Fixed** | +| 10 | `AmbiguousTransitionError` lacked context | **Fixed** — carries `candidates`, rendered into the message | +| 12 | `LockAdapterMutex` acquire race | **Fixed** — overlapping acquires share one in-flight promise | +| 13 | `readonlyContext` immutability is nominal | **Documented** in `docs/core.md` | +| 15 | Mermaid label quoting | **Deferred** — needs verification in a real renderer; it is pinned by tests and docs, so it should not be changed on a hypothesis | +| 5, 6, 11 | `Event` mutability, `OnEnterObserver`, `machineName` | **Deferred to v5** — all breaking | +| 14 | `OperationQueue` uses `shift()` | **Deferred** — irrelevant at realistic queue depths | + +Also documented in this pass: the passive nature of `Timeout` (`docs/conditions.md`) and the cross-machine sharing of event observers (`docs/observers.md`), both of which are easy to misread from the API alone. From a946f0e46387370dcaf0e0c37579e6f38a28e044 Mon Sep 17 00:00:00 2001 From: Carlos Cima Date: Wed, 19 Aug 2026 06:46:25 -0400 Subject: [PATCH 3/4] docs: link the review's deferred items to their tracking issues Files the outstanding work as GitHub issues so the deferred rows point somewhere actionable: #53-#57 batched under the v5 milestone, #58-#63 non-breaking, indexed by #64. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01JLxceiNFfXfpGoxdfQEZqw --- ARCHITECTURE_REVIEW.md | 36 +++++++++++++++++++++--------------- 1 file changed, 21 insertions(+), 15 deletions(-) diff --git a/ARCHITECTURE_REVIEW.md b/ARCHITECTURE_REVIEW.md index 521b1fa..c3b491f 100644 --- a/ARCHITECTURE_REVIEW.md +++ b/ARCHITECTURE_REVIEW.md @@ -166,20 +166,26 @@ The "Cleanup" section items also landed (shared composite base, unified name val Everything non-breaking was fixed on `fix/architecture-review-findings`; each fix landed test-first, with the failing test observed before the change. -| # | Finding | Status | -| -------- | ---------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------- | -| Bug 1 | `releaseLock()` returning false swallowed | **Fixed** — new `LockCanNotBeReleasedError`; both failure modes normalized in one path | -| Bug 2 | `whenIdle()` deadlock | **Fixed** — guarded with `assertNotReentrant` | -| 3 | `releaseLock()` discards the mutex result | **Partly fixed** — failures now reach `onReleaseError`; the `Promise` return type is deferred to v5 as breaking | -| 4 | Factory cannot configure engine options | **Fixed** — `FactoryStatemachineOptions` constructor template | -| 7 | Live internal collections escape | **Fixed** — observer accessors return snapshots | -| 8 | Deprecation debt | **Partly fixed** — notices re-dated to v5; removal is itself breaking | -| 9 | Redundant cast | **Fixed** | -| 10 | `AmbiguousTransitionError` lacked context | **Fixed** — carries `candidates`, rendered into the message | -| 12 | `LockAdapterMutex` acquire race | **Fixed** — overlapping acquires share one in-flight promise | -| 13 | `readonlyContext` immutability is nominal | **Documented** in `docs/core.md` | -| 15 | Mermaid label quoting | **Deferred** — needs verification in a real renderer; it is pinned by tests and docs, so it should not be changed on a hypothesis | -| 5, 6, 11 | `Event` mutability, `OnEnterObserver`, `machineName` | **Deferred to v5** — all breaking | -| 14 | `OperationQueue` uses `shift()` | **Deferred** — irrelevant at realistic queue depths | +| # | Finding | Status | +| -------- | ---------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------- | +| Bug 1 | `releaseLock()` returning false swallowed | **Fixed** — new `LockCanNotBeReleasedError`; both failure modes normalized in one path | +| Bug 2 | `whenIdle()` deadlock | **Fixed** — guarded with `assertNotReentrant` | +| 3 | `releaseLock()` discards the mutex result | **Partly fixed** — failures now reach `onReleaseError`; the `Promise` return type is deferred to v5 as breaking | +| 4 | Factory cannot configure engine options | **Fixed** — `FactoryStatemachineOptions` constructor template | +| 7 | Live internal collections escape | **Fixed** — observer accessors return snapshots | +| 8 | Deprecation debt | **Partly fixed** — notices re-dated to v5; removal is itself breaking | +| 9 | Redundant cast | **Fixed** | +| 10 | `AmbiguousTransitionError` lacked context | **Fixed** — carries `candidates`, rendered into the message | +| 12 | `LockAdapterMutex` acquire race | **Fixed** — overlapping acquires share one in-flight promise | +| 13 | `readonlyContext` immutability is nominal | **Documented** in `docs/core.md` | +| 15 | Mermaid label quoting | **Deferred** (#58) — needs verification in a real renderer; it is pinned by tests and docs, so it should not be changed on a hypothesis | +| 5, 6, 11 | `Event` mutability, `OnEnterObserver`, `machineName` | **Deferred to v5** — all breaking (#53, #54, #56) | +| 14 | `OperationQueue` uses `shift()` | **Deferred** (#63) — irrelevant at realistic queue depths | Also documented in this pass: the passive nature of `Timeout` (`docs/conditions.md`) and the cross-machine sharing of event observers (`docs/observers.md`), both of which are easy to misread from the API alone. + +## Open follow-up + +Everything still outstanding is tracked in **#64**, which indexes the breaking work batched for the [v5 milestone](https://github.com/camcima/finita/milestone/1) (#53 `Event` immutability and build-time commands, #54 first-class entry hooks, #55 `releaseLock` return type, #56 `processName` rename, #57 deprecation removal) alongside the non-breaking items (#58 Mermaid verification, #59 `AsyncLocalStorage` re-entrancy detection, #60 queue introspection, #61 a `Timeout` scheduler helper, #62 an are-the-types-wrong CI check, #63 queue dequeue cost). + +Of those, #59 is the one worth doing first: it is the only remaining item whose failure mode is a silent permanent deadlock — the same class of defect this review found in `whenIdle()`. From c32c9f9f626822ce46e8de97b3b708e1a9ad5848 Mon Sep 17 00:00:00 2001 From: Carlos Cima Date: Wed, 19 Aug 2026 06:52:11 -0400 Subject: [PATCH 4/4] test: cover AmbiguousTransitionError without candidates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The candidates argument is optional so a custom TransitionSelectorInterface can still raise the error with only a count, as it could before #52 added candidates — but nothing exercised that path, leaving the no-detail message branch untested and dropping the repo below its 100% coverage bar. Also asserts the candidates array is frozen, which the constructor guarantees but no test checked. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01JLxceiNFfXfpGoxdfQEZqw --- tests/typed-throws.test.ts | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/tests/typed-throws.test.ts b/tests/typed-throws.test.ts index 269e283..53d413c 100644 --- a/tests/typed-throws.test.ts +++ b/tests/typed-throws.test.ts @@ -182,6 +182,19 @@ describe("typed throws", () => { expect(e.message).toContain('"b"'); expect(e.message).toContain('"c"'); expect(e.message).toContain("isVip"); + expect(Object.isFrozen(e.candidates)).toBe(true); + }); + + it("omits the candidate detail when constructed without candidates", () => { + // The candidates argument is optional so that a custom + // TransitionSelectorInterface can still raise this error with only a + // count, exactly as it could before candidates existed. + const e = new AmbiguousTransitionError(3); + expect(e.activeCount).toBe(3); + expect(e.candidates).toEqual([]); + expect(e.message).toBe( + "More than one transition is active! (active count: 3)", + ); }); });