diff --git a/ARCHITECTURE_REVIEW.md b/ARCHITECTURE_REVIEW.md new file mode 100644 index 0000000..c3b491f --- /dev/null +++ b/ARCHITECTURE_REVIEW.md @@ -0,0 +1,191 @@ +# 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** (#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()`. 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..53d413c 100644 --- a/tests/typed-throws.test.ts +++ b/tests/typed-throws.test.ts @@ -143,6 +143,59 @@ 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"); + 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)", + ); + }); }); describe("Statemachine automatic-cycle detection", () => {