diff --git a/CHANGELOG.md b/CHANGELOG.md index f7bf2a3..960e167 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,14 @@ # Changelog +## Unreleased + +- Replace the caller-owned, effectful `Machine.Fire` interface with pure + `Machine.Next`. `Next` runs Guards and selects a destination but cannot run + `Do`; transition effects now execute only through `Instance`, queued Runtime, + or Store-backed state owners. This is a breaking interface change that removes + the lost-transition hazard where an effect ran but its returned state was + discarded. + ## 1.3.0 This release is the API. The project is pre-adoption, so it stays in v1 diff --git a/CONTEXT.md b/CONTEXT.md index 1169677..88afc6e 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -5,7 +5,8 @@ This repository separates immutable transition definitions from the mutable exec ## Language **Machine**: -An immutable compiled table defining flat state transitions. +An immutable compiled table defining flat state transitions. Next runs Guards +and reports a destination but never runs Do; effects require a state owner. _Avoid_: Instance, runtime **Instance**: @@ -79,6 +80,8 @@ _Avoid_: Store, Recorder ## Relationships - One **Machine** is shared by zero or more **Instances** and **Runtimes**. +- A **Machine** may answer a pure Next query for caller-owned state, but only an + **Instance**, **Runtime**, or **Store**-backed execution runs a transition's Do. - One **Instance** owns exactly one current state. - One **Runtime** owns exactly one current state and serializes zero or more **Runs**. - One **Run** contains one root event and zero or more follow-up events. diff --git a/README.md b/README.md index f42c4e2..50372d0 100644 --- a/README.md +++ b/README.md @@ -13,14 +13,17 @@ import "github.com/open-ships/statemachine" A finite state machine is a partial function from `(state, event)` to `state`. This package keeps that function as its core and builds optional state owners around it. -**A `Machine` does not hold the current state.** It is an immutable compiled definition. State can -remain a value you own — a struct field, a database column — and a step is the application of the -definition to that value: +**A `Machine` does not hold the current state.** It is an immutable compiled definition. `Next` +answers a pure question about a value you own and never runs the selected row's effect: ```go -order.State, err = orders.Fire(ctx, order.State, Pay, cmd) +next, err := orders.Next(ctx, order.State, Submit, cmd) ``` +Use `Next` for planning or a row with no `Do`. To perform an effectful row, use +an `Instance`, queued Runtime, or Store-backed execution; assigning the `Next` +result would intentionally skip its effect. + One definition can serve a million aggregates and any number of goroutines. When the package should own execution state instead, construct one `Instance` per aggregate. The [`queued`](queued) package adds serialized run-to-completion execution, [`persist`](persist) runs the flat definition inside an @@ -49,7 +52,7 @@ var light = statemachine.MustCompile([]statemachine.Transition[State, Event, str func main() { s := Off - s, _ = light.Fire(context.Background(), s, Flip, struct{}{}) + s, _ = light.Next(context.Background(), s, Flip, struct{}{}) fmt.Println(s) // on } ``` @@ -83,7 +86,7 @@ failure — it drops that row and tries the next one, so a trailing unguarded ro semantics of a `switch`. Only when no row is left does the reason reach the caller: ```go -_, err := orders.Fire(ctx, Delivered, Refund, cmd) +_, err := orders.Next(ctx, Delivered, Refund, cmd) errors.Is(err, statemachine.ErrNotPermitted) // true: the machine refused -> 409 errors.Is(err, ErrWindowClosed) // true: and this is why -> 422 @@ -91,9 +94,9 @@ errors.Is(err, ErrWindowClosed) // true: and this is why -> That is the whole 409-versus-422 story, with no second error type and no `errors.As`. -**Effects fail with `return err`.** `Fire` reports the destination if and only if `Do` returned `nil`, -and the error comes back to you unwrapped, so `errors.Is` against your own sentinels works with no -ceremony: +**Effects run only behind a state owner.** `Machine.Next` never calls `Do`. `Instance.Fire`, a queued +runtime, or Store-backed execution runs the selected effect and publishes the destination only when +`Do` returns `nil`. The error comes back unwrapped: ```go {From: Pending, Event: Pay, To: Paid, Do: func(ctx context.Context, c *Cmd) error { @@ -101,7 +104,7 @@ ceremony: }}, ``` -**Affordances come from the same rule as firing**, so a rendered button and the handler that receives +**Affordances come from the same selection rule as execution**, so a rendered button and the handler that receives its click cannot disagree about where an event leads: ```go @@ -118,7 +121,7 @@ for event, to := range orders.Permitted(ctx, o.State, cmd) { | `Machine[S, E comparable, T any]` | a compiled table; immutable, safe for concurrent use | | `Compile(transitions)` | build a machine, reporting an unreachable row | | `MustCompile(transitions)` | the same, panicking — for tables that are program text | -| `Machine.Fire(ctx, from, event, data)` | apply an event; report the state to move to | +| `Machine.Next(ctx, from, event, data)` | select and report a destination without running `Do` | | `Machine.Permitted(ctx, from, data)` | iterate the events accepted now, each with its destination | | `ErrNotPermitted` | the sentinel every refusal wraps | @@ -144,8 +147,9 @@ observation-silent. A shared observer can be called concurrently by different executions and must synchronize its own census or sink. `T` is the value handed to every `Guard` and `Do` — your aggregate, plus whatever this command needs. -It is passed to `Fire` rather than stored, so one immutable `Machine` serves every request while -still seeing request-scoped values. A machine with nothing to carry uses `struct{}`. +It is passed to `Next` or the state-owning execution rather than stored, so one immutable `Machine` +serves every request while still seeing request-scoped values. A machine with nothing to carry uses +`struct{}`. Visualization and reachability checking remain ordinary loops over the flat table. Per-state entry and exit actions do not belong to a flat `Machine`; use the `statechart` package when those semantics @@ -155,7 +159,7 @@ are required. Runnable flat-machine examples are in [`example_test.go`](example_ | Need | Module | State and concurrency semantics | |---|---|---| -| Database aggregate or explicit assignment | `Machine` | caller-owned | +| Pure planning or explicit assignment with no effects | `Machine.Next` | caller-owned value; never runs `Do` | | One in-memory aggregate | `Instance` | owned state; overlapping fire fails fast | | Follow-up events and FIFO serialization | `queued.Runtime` | owned state; each root run drains to completion | | Database state and outbox work | `persist.Fire` | transactional when the Store supplies a transaction; never auto-retried | @@ -190,9 +194,10 @@ relationships. ## Hazards -**Discarding `Machine.Fire`'s returned state is never correct.** The effect has already run, and -neither the compiler nor `go vet` reports the lost transition. A state-owning `Instance.Fire` may -discard its returned state, but its error still must be handled. +**`Machine.Next` never runs `Do`.** Effectful execution is available only through a state owner, so +discarding a pure query cannot leave an external effect behind. A state-owning `Instance.Fire` may +discard its returned state because the Instance has already committed it, but its error still must +be handled. An `Instance` or queued runtime is the sole owner of its state. Do not also keep an authoritative copy in the value passed as `T`. Effects can still be partial: the state owner guarantees its state @@ -217,16 +222,16 @@ documented in full on ## Performance Apple M1 Pro, Go 1.26, measured with `testing.B.Loop`. Nothing on the flat `Machine`'s successful -path allocates: `Fire` is a map lookup, a guard call and a return, and `Permitted`'s lazy iterator +path allocates: `Next` is a map lookup, a guard call and a return, and `Permitted`'s lazy iterator stays on the stack. A refusal allocates only the error. The state-owning modules intentionally add synchronization and, where required, eager snapshots or queued work. ``` -BenchmarkFireAccepted-10 60591013 21.32 ns/op 0 B/op 0 allocs/op -BenchmarkFireDefaultArm-10 45832159 25.39 ns/op 0 B/op 0 allocs/op -BenchmarkFireRefused-10 11011408 113.90 ns/op 80 B/op 2 allocs/op -BenchmarkRefusalErrorsIs-10 117287949 10.30 ns/op 0 B/op 0 allocs/op -BenchmarkPermitted-10 26448175 47.00 ns/op 0 B/op 0 allocs/op +BenchmarkNextAccepted-10 44001490 27.18 ns/op 0 B/op 0 allocs/op +BenchmarkNextDefaultArm-10 61790773 19.42 ns/op 0 B/op 0 allocs/op +BenchmarkNextRefused-10 25778685 46.09 ns/op 80 B/op 2 allocs/op +BenchmarkRefusalErrorsIs-10 132888273 9.03 ns/op 0 B/op 0 allocs/op +BenchmarkPermitted-10 30415488 39.53 ns/op 0 B/op 0 allocs/op ``` Go 1.26 is what makes `Permitted` free: the `iter.Seq2` it returns closes over the machine, the state diff --git a/SAFETY.md b/SAFETY.md index 40fee98..51818d3 100644 --- a/SAFETY.md +++ b/SAFETY.md @@ -6,6 +6,12 @@ This repository is general-purpose software, not a safety-rated controller or a The flat Machine, Instance, queued Runtime, Store-backed execution, Statechart, and supervised Supervisor may coordinate application logic. Hazardous motion must remain bounded by an independent, hazard-analyzed safety layer responsible for functions such as emergency stop, safe torque off, protective stop, guarding, overspeed, collision protection, and human-presence separation. +`Machine.Next` is selection-only: it runs Guards and reports a destination but +never runs a transition's `Do`. Flat effects are available only through an +Instance, queued Runtime, or Store-backed execution that owns the associated +state commit. This prevents an ignored Machine result from leaving an effect +behind; it does not make arbitrary external I/O atomic or reversible. + The `supervised` module exists for safety-adjacent orchestration where callers need mandatory checks, explicit issue and verification, finite time budgets, first-cause Fault latching, and reconciliation before startup or recovery. It does not preempt arbitrary Go code or stop hardware. ## Logical and physical state diff --git a/doc.go b/doc.go index c2ef6d1..d16e567 100644 --- a/doc.go +++ b/doc.go @@ -1,20 +1,18 @@ -// Package statemachine compiles finite state-machine definitions and runs them -// with caller-owned or package-owned state. +// Package statemachine compiles finite state-machine definitions and executes +// them through explicit state owners. // // A [Machine] is the flat transition function: an immutable table of // [Transition] rows, compiled once and safe for use by any number of // goroutines. It depends only on the standard library. // -// A Machine does not hold the current state. State is a value the caller owns — -// a field on a struct, a column in a row — and a step is the application of the -// machine to that value: +// A Machine does not hold the current state. [Machine.Next] is a pure +// transition query for a caller-owned value: it runs Guards and reports the +// selected destination, but never runs the row's Do effect: // -// var err error -// order.State, err = orders.Fire(ctx, order.State, Pay, cmd) +// next, err := orders.Next(ctx, order.State, Submit, cmd) // -// This form is useful for database rows and explicit read-modify-write flows: -// restoring an aggregate means passing the loaded state to Fire, and one -// Machine can serve millions of aggregates without creating runtime objects. +// One Machine can serve millions of aggregates and pure planning queries. To +// run effects, choose the module that owns the resulting state. // // When the package should own one in-memory aggregate's current state, use an // [Instance]: @@ -29,9 +27,9 @@ // unrestricted state setter. // // State ownership does not change definition semantics. This package remains -// a flat machine with row effects. The queued subpackage adds serialized -// run-to-completion cascades, persist places Machine.Fire inside an -// adapter-owned unit of work, and statechart supplies hierarchy, initial +// a flat machine with row effects, but only a state owner may run those effects. +// The queued subpackage adds serialized run-to-completion cascades, persist +// executes through an adapter-owned unit of work, and statechart supplies hierarchy, initial // substates, lifecycle actions, and explicit transition kinds. The supervised // subpackage supplies a separate strict definition with mandatory checks, // explicit issue and verification, finite time limits, startup reconciliation, @@ -72,9 +70,9 @@ // // The third type parameter is the value handed to every Guard and Do of that // machine — the aggregate being transitioned, plus whatever this particular -// command needs. It is passed to [Machine.Fire] rather than stored, so one -// immutable Machine serves every request while still seeing request-scoped -// values. A machine with nothing to carry uses struct{}. +// command needs. It is passed to [Machine.Next] or a state-owning execution +// rather than stored, so one immutable Machine serves every request while still +// seeing request-scoped values. A machine with nothing to carry uses struct{}. // // Two rows may share a From and an Event. The first whose Guard applies wins, so // a trailing unguarded row is a default arm — the semantics of a switch, of @@ -83,26 +81,11 @@ // // # Observing transitions // -// A Machine has no observer or per-state hooks. Fire returns the new state and -// the caller already holds the old state and the event, so every transition is -// visible on the line that performs it. Write the wrapper once per machine and -// put the logging, the metrics and the write there: -// -// func (s *Service) fire(ctx context.Context, o *Order, e Event, c *Cmd) error { -// from := o.State -// next, err := orders.Fire(ctx, from, e, c) -// o.State = next -// s.log.InfoContext(ctx, "transition", -// "id", o.ID, "from", from, "event", e, "to", next, "err", err) -// return err -// } -// -// Assigning the returned state is always correct: Fire reports the state it was -// given whenever it reports an error. -// -// State-owning executions are different: reading State before Fire is a second, -// racy operation, and a queued Runtime may commit several follow-up transitions -// before Fire returns. [NewInstanceWithObservers], queued.NewWithObservers and +// A Machine has no observer or per-state hooks because Next selects without +// committing anything. State-owning executions can publish committed changes. +// Reading State before Fire is a second, racy operation, and a queued Runtime +// may commit several follow-up transitions before Fire returns. +// [NewInstanceWithObservers], queued.NewWithObservers and // statechart.Chart.NewWithObservers attach immutable observers at the ownership // seam. They emit an [Observation] for every committed node exit and entry. A // flat self-transition changes no position and emits nothing. @@ -132,9 +115,11 @@ // // # Hazards // -// - Discarding [Machine.Fire]'s returned state is never correct. The effect -// has already run. [Instance.Fire] owns and publishes its state, so callers -// may discard that state result but must still handle its error. +// - [Machine.Next] never runs Do. Effects are available only through a +// state-owning execution, so discarding a pure query cannot leave an effect +// behind. Do not use Next to commit an effectful row: that would skip Do. +// [Instance.Fire] owns and publishes its state, so callers may discard that +// state result but must still handle its error. // - An Instance is the sole authority for its state. Do not retain another // authoritative state field in T; it can diverge on errors and panics. // - Instance rejects overlap and same-instance recursive firing. Use the @@ -150,8 +135,7 @@ // - A Guard must be pure. It is called for rows that lose, and by Permitted. // - A Guard vetoes only if no other row for that From and Event applies. // - [ErrNotPermitted] is a sentinel and travels like one. A Guard or Do that -// fires another machine must not return its refusal, wrapped or otherwise. +// runs another execution must not return its refusal, wrapped or otherwise. // - Guards and effects see data, which the caller also owns. Recursively -// calling Machine.Fire for the same caller-owned state loses the nested -// transition; Instance reports ErrInFlight instead. +// calling [Instance.Fire] on the same execution reports ErrInFlight. package statemachine diff --git a/docs/adr/0004-keep-machine-selection-pure.md b/docs/adr/0004-keep-machine-selection-pure.md new file mode 100644 index 0000000..0a0c8be --- /dev/null +++ b/docs/adr/0004-keep-machine-selection-pure.md @@ -0,0 +1,20 @@ +# Keep Machine selection pure and effects behind state owners + +`Machine.Next` runs Guards and reports the first applicable transition's +destination, but never runs `Do`. The prior `Machine.Fire` interface is removed. +It combined an external effect with a destination returned to caller-owned +state, allowing valid Go code to discard the only state commit after the effect +had already occurred. + +Flat effects execute only through a module that owns the corresponding state: +Instance for fail-fast in-memory execution, Runtime for queued execution, or a +Store-backed execution for persistence. Runtime and Store-backed execution use +Instance internally so transition selection, `Do`, and in-process publication +remain local to one implementation. + +This is intentionally a breaking interface change. A pure caller-owned query +may still be ignored and therefore accomplish nothing, but it cannot leave an +effect behind. State ownership does not make arbitrary I/O atomic: effects can +remain partial on error, panic, cancellation, or process failure, so durable or +physical work still requires the Store and supervised protocols documented by +their modules. diff --git a/docs/assurance/requirements.md b/docs/assurance/requirements.md index 5fc27ce..9ea048c 100644 --- a/docs/assurance/requirements.md +++ b/docs/assurance/requirements.md @@ -22,6 +22,7 @@ This matrix separates behavior proved in this repository from product obligation | LIB-PER-001 | Store callback repetition or omission is detected; transition effects run at most once per library call. | `persist_test.TestStoreCallbackContractIsEnforced` | | LIB-OBS-001 | Committed Observations are timestamped; Observer panic and runtime.Goexit retain a stack, later observers run, and committed state remains visible. | `statemachine_test.TestInstanceObservesCommittedPositionChanges`, `statemachine_test.TestInstanceObserverFailuresAreIsolated`, `statechart_test.TestSuccessfulStatechartObserverFailuresAreReturnedAfterCommit` | | LIB-DEF-001 | Executions own compiled definition values and are unaffected by caller whole-value overwrite. | `queued_test.TestRuntimeOwnsCompiledMachineValue`, `statechart_test.TestInstanceOwnsCompiledChartValue`, `supervised.TestSupervisorOwnsCompiledMachineValue` | +| LIB-DEF-002 | Machine.Next never executes a transition Do; flat effects execute only through a state owner. | `statemachine_test.TestMachineNextNeverExecutesDo`, `statemachine_test.TestNextSelectsFirstApplicableRowWithoutRunningEffects` | | LIB-KEY-001 | Interface-bearing state/event types and dynamically uncomparable Store keys return errors rather than panic. | `statemachine_test.TestStrictComparableTypesAndDynamicValuesNeverPanic`, `statechart_test.TestCompileRejectsInterfaceBearingKeyTypesBeforeMapUse`, `supervised.TestCompileRejectsNestedInterfaceKeyBeforeMapUse`, `persist_test.TestMemoryStoreRejectsDynamicallyUncomparableKey` | | LIB-SUP-011 | Lifecycle Record identity (ExecutionID, Restarts, Seq) is assigned atomically with the state decision, is strictly increasing, and is never reused across restorations. | `supervised.FuzzSupervisorModel`, `supervised.FuzzConcurrentSupervisor`, `supervised.TestRestoreContinuesRecordIdentityAcrossIncarnations` | | LIB-SUP-012 | In-process Record history is bounded, evictions are counted, and Recorder failures are counted and visible without rewriting outcomes. | `supervised.TestRecordsRingBoundsHistoryAndCountsDrops`, `supervised.FuzzRecorderJournalFaultInjection` | @@ -31,7 +32,7 @@ This matrix separates behavior proved in this repository from product obligation | LIB-SUP-016 | Restore validates the Snapshot schema version and refuses structurally inconsistent Snapshots with documented sentinels, never a panic. | `supervised.TestRestoreValidatesSnapshotSchemaVersion`, `supervised.FuzzSnapshotRestore` | | LIB-SUP-017 | A delivered callback outcome is the primary cause of its operation's decision; secondary-cause Records exist only for outcomes abandoned at a deadline. | `supervised.TestSecondaryCauseIsRecordedOnlyForAbandonedCallbacks` | | LIB-OBS-002 | Observer delivery can be time-bounded and observation health can be separated from the transition error channel without un-committing state. | `statemachine_test.TestTimeoutObserverBoundsBlockedDelivery`, `statemachine_test.TestContainedObserverSeparatesObservationHealth`, `statemachine_test.TestContainedTimeoutCompositionKeepsFireClean` | -| LIB-FUZ-001 | Model-based fuzzing checks every package against independent reference implementations: supervisor lifecycle and power-loss restart, adversarial clock, concurrent schedules, recorder/journal fault injection, definition and snapshot fuzzing, flat-machine and statechart selection/path semantics, queued cascade semantics, and Store contract violations. | `FuzzSupervisorModel`, `FuzzSupervisorRestartModel`, `FuzzAdversarialClock`, `FuzzConcurrentSupervisor`, `FuzzSnapshotRestore`, `FuzzDefinitionCompile`, `FuzzRecorderJournalFaultInjection`, `FuzzMachineFire`, `FuzzZeroAndCompiledKeyChecks`, `FuzzStatechartCompile`, `FuzzStatechartFire`, `FuzzStoreContract`, `FuzzMemoryStoreSerialization`, `FuzzQueuedRuntimeModel`, `FuzzQueuedRuntimeLimits`, `FuzzQueuedCancellation` | +| LIB-FUZ-001 | Model-based fuzzing checks every package against independent reference implementations: supervisor lifecycle and power-loss restart, adversarial clock, concurrent schedules, recorder/journal fault injection, definition and snapshot fuzzing, flat-machine and statechart selection/path semantics, queued cascade semantics, and Store contract violations. | `FuzzSupervisorModel`, `FuzzSupervisorRestartModel`, `FuzzAdversarialClock`, `FuzzConcurrentSupervisor`, `FuzzSnapshotRestore`, `FuzzDefinitionCompile`, `FuzzRecorderJournalFaultInjection`, `FuzzInstanceFire`, `FuzzZeroAndCompiledKeyChecks`, `FuzzStatechartCompile`, `FuzzStatechartFire`, `FuzzStoreContract`, `FuzzMemoryStoreSerialization`, `FuzzQueuedRuntimeModel`, `FuzzQueuedRuntimeLimits`, `FuzzQueuedCancellation` | Repository verification runs `go test`, `go test -race`, `go vet`, errcheck, staticcheck, formatting, a coverage floor, vulnerability scanning, per-change fuzz smoke runs of every target, benchmarks, and a scheduled sustained fuzz campaign. Failing fuzz inputs are retained under `testdata/fuzz` as permanent regression seeds. After successful CI on the current `main` commit, the exact-version-tagged shared release workflow reruns source verification, creates an annotated tag automatically, and retains the source artifact, coverage, toolchain, SBOM, checksums, and separate build-provenance and SBOM attestations. diff --git a/example_test.go b/example_test.go index 4a7f249..3f491fc 100644 --- a/example_test.go +++ b/example_test.go @@ -29,7 +29,7 @@ func Example() { s := Off for range 3 { - s, _ = light.Fire(context.Background(), s, Flip, struct{}{}) + s, _ = light.Next(context.Background(), s, Flip, struct{}{}) fmt.Println(s) } @@ -67,9 +67,8 @@ const ( Refund Event = "refund" ) -// Order is the aggregate used by the caller-owned Machine examples. In that -// execution model State is an ordinary field: restoration means loading it and -// passing it to Fire. +// Order is the aggregate used by the pure Machine examples. State is an +// ordinary field passed to Next; effects require a state-owning execution. type Order struct { ID string State State @@ -79,7 +78,7 @@ type Order struct { } // Cmd is the third type parameter: the aggregate plus whatever this particular -// command needs. It is passed to Fire rather than stored, so one immutable +// command needs. It is passed to Next or a state-owning execution rather than stored, so one immutable // Machine serves every request while still seeing request-scoped values. type Cmd struct { Order *Order @@ -150,39 +149,35 @@ var orderTable = []Row{ var lifecycle = statemachine.MustCompile(orderTable) -// Fire reports where to go next; the caller owns the state and writes it down. -func ExampleMachine_Fire() { +// Next reports where to go next without running the selected row's effect. +func ExampleMachine_Next() { ctx := context.Background() o := &Order{ID: "A1", State: Draft, Lines: 2} var err error - o.State, err = lifecycle.Fire(ctx, o.State, Submit, &Cmd{Order: o}) + o.State, err = lifecycle.Next(ctx, o.State, Submit, &Cmd{Order: o}) fmt.Println(o.State, err) // A guard declined and no other row applied: the state is unchanged and the // reason travels with the refusal. empty := &Order{ID: "A2", State: Draft} - next, err := lifecycle.Fire(ctx, empty.State, Submit, &Cmd{Order: empty}) + next, err := lifecycle.Next(ctx, empty.State, Submit, &Cmd{Order: empty}) fmt.Println(next, err) fmt.Println("refused:", errors.Is(err, statemachine.ErrNotPermitted), "| why:", errors.Is(err, ErrNoLines)) - // An effect failed: Fire returns the effect's own error, unwrapped, and the - // state does not advance. - next, err = lifecycle.Fire(ctx, o.State, Pay, &Cmd{Order: o}) - fmt.Println(next, err) - fmt.Println("refused:", errors.Is(err, statemachine.ErrNotPermitted), - "| declined:", errors.Is(err, ErrDeclined)) + // Pay has a charge effect, but asking what comes next cannot run it. + next, err = lifecycle.Next(ctx, o.State, Pay, &Cmd{Order: o}) + fmt.Println(next, err, "charged:", o.Charged) // Output: // pending // draft statemachine: submit in state draft: transition not permitted: order has no lines // refused: true | why: true - // pending card declined - // refused: false | declined: true + // paid charged: false } -// Permitted answers "what may happen next", following the same guards Fire +// Permitted answers "what may happen next", following the same guards Next // follows, so a rendered affordance and the server that receives it cannot // disagree about where an event leads. func ExampleMachine_Permitted() { @@ -240,7 +235,8 @@ func Example_httpStatus() { {State: Draft}, // no lines {State: Delivered}, // wrong state for submit } { - _, err := lifecycle.Fire(ctx, o.State, Submit, &Cmd{Order: o}) + run := statemachine.NewInstance(lifecycle, o.State) + _, err := run.Fire(ctx, Submit, &Cmd{Order: o}) fmt.Println(classify(err)) } @@ -357,7 +353,9 @@ func Example_entryAction() { for _, from := range []State{Draft, Pending, Paid} { o := &Order{State: from} c := &Cmd{Order: o} - o.State, _ = machine.Fire(ctx, o.State, Cancel, c) + run := statemachine.NewInstance(machine, o.State) + _, _ = run.Fire(ctx, Cancel, c) + o.State = run.State() fmt.Println(from, "->", o.State, c.Log) } @@ -391,7 +389,7 @@ func Example_fanIn() { fmt.Println("compile:", err) return } - next, err := machine.Fire(context.Background(), Shipped, Cancel, &Cmd{}) + next, err := machine.Next(context.Background(), Shipped, Cancel, &Cmd{}) fmt.Println(Shipped, "->", next, err) // Output: diff --git a/fuzz_test.go b/fuzz_test.go index aef4c18..e2d41b5 100644 --- a/fuzz_test.go +++ b/fuzz_test.go @@ -112,11 +112,11 @@ func referenceFire(rows []fuzzRow, from fuzzState, event fuzzEvent) (fuzzState, return from, false, false, declined } -// FuzzMachineFire drives arbitrary tables and event sequences through the -// compiled Machine and asserts exact agreement with the reference +// FuzzInstanceFire drives arbitrary tables and event sequences through a +// state-owning Instance and asserts exact agreement with the reference // interpreter: selected row, resulting state, error identity, and refusal // reasons. -func FuzzMachineFire(f *testing.F) { +func FuzzInstanceFire(f *testing.F) { f.Add([]byte{0, 0, 1, 0}, []byte{0, 0}) f.Add([]byte{0, 0, 1, 3, 0, 0, 2, 0}, []byte{0, 0, 1, 1}) f.Add([]byte{0, 0, 1, 4}, []byte{0, 0}) @@ -144,7 +144,8 @@ func FuzzMachineFire(f *testing.F) { event := fuzzEvent(fires[index+1] % 4) wantState, wantOK, wantEffectErr, declined := referenceFire(rows, from, event) - got, err := machine.Fire(ctx, from, event, 0) + execution := statemachine.NewInstance(machine, from) + got, err := execution.Fire(ctx, event, 0) if got != wantState { t.Fatalf("Fire(%v, %v) state = %v, want %v (rows %+v)", from, event, got, wantState, rows) } @@ -210,14 +211,14 @@ func FuzzZeroAndCompiledKeyChecks(f *testing.F) { var zero statemachine.Machine[fuzzState, fuzzEvent, int] state := fuzzState(stateByte) event := fuzzEvent(eventByte) - got, err := zero.Fire(context.Background(), state, event, 0) + got, err := zero.Next(context.Background(), state, event, 0) if got != state || !errors.Is(err, statemachine.ErrNotPermitted) { - t.Fatalf("zero Machine Fire = %v, %v", got, err) + t.Fatalf("zero Machine Next = %v, %v", got, err) } compiled := statemachine.MustCompile([]statemachine.Transition[fuzzState, fuzzEvent, int]{}) - got, err = compiled.Fire(context.Background(), state, event, 0) + got, err = compiled.Next(context.Background(), state, event, 0) if got != state || !errors.Is(err, statemachine.ErrNotPermitted) { - t.Fatalf("empty compiled Machine Fire = %v, %v", got, err) + t.Fatalf("empty compiled Machine Next = %v, %v", got, err) } }) } diff --git a/instance.go b/instance.go index d35b1e3..c585418 100644 --- a/instance.go +++ b/instance.go @@ -114,7 +114,7 @@ func (i *Instance[S, E, T]) Fire(ctx context.Context, event E, data T) (S, error i.mu.Unlock() }() - next, err := machine.Fire(ctx, from, event, data) + next, err := machine.fire(ctx, from, event, data) var step uint64 var observers []Observer[S, E, T] diff --git a/persist/persist.go b/persist/persist.go index 7413fae..478c3ef 100644 --- a/persist/persist.go +++ b/persist/persist.go @@ -76,7 +76,8 @@ type FuncStore[K, S comparable, X any] struct { // StepResult describes the transition attempt made inside a Store update. // From and To are meaningful only when Attempted is true. TransitionError is -// the exact error returned by Machine.Fire, before any later Store error. +// the exact error returned by the state-owning transition execution, before +// any later Store error. // Confirmed is true only when Store.Update returned success; false does not // prove that an external commit did not happen. type StepResult[S, E comparable] struct { @@ -171,7 +172,8 @@ func apply[K, S, E comparable, T, X any]( result.From = from result.To = from result.Attempted = true - to, transitionErr := machine.Fire(ctx, from, event, value) + execution := statemachine.NewInstance(machine, from) + to, transitionErr := execution.Fire(ctx, event, value) result.To = to result.TransitionError = transitionErr return to, transitionErr diff --git a/queued/queued.go b/queued/queued.go index 5f298d4..fdf96c4 100644 --- a/queued/queued.go +++ b/queued/queued.go @@ -64,8 +64,7 @@ var defaultLimits = Limits{MaxRoots: DefaultMaxRoots, MaxRunEvents: DefaultMaxRu // whenever the root queue becomes empty. type Runtime[S, E comparable, T any] struct { mu sync.Mutex - machine statemachine.Machine[S, E, T] - state S + instance statemachine.Instance[S, E, T] roots []*root[E, T, S] running bool limits Limits @@ -118,11 +117,9 @@ func newRuntime[S, E comparable, T any]( observers []statemachine.Observer[S, E, T], ) *Runtime[S, E, T] { runtime := &Runtime[S, E, T]{ - state: initial, limits: limits, observers: copyObservers(observers), - } - if machine != nil { - runtime.machine = *machine + limits: limits, observers: copyObservers(observers), } + runtime.instance = *statemachine.NewInstance(machine, initial) return runtime } @@ -130,9 +127,7 @@ func newRuntime[S, E comparable, T any]( // continues to report the state that event started from; the destination is // published only after Do returns nil. func (r *Runtime[S, E, T]) State() S { - r.mu.Lock() - defer r.mu.Unlock() - return r.state + return r.instance.State() } // Status reports committed state, scheduler activity, admitted roots, and @@ -145,7 +140,7 @@ func (r *Runtime[S, E, T]) Status() Status[S] { limits = defaultLimits } return Status[S]{ - State: r.state, Running: r.running, + State: r.instance.State(), Running: r.running, OutstandingRoots: r.rootCount, Limits: limits, } } @@ -191,7 +186,7 @@ func (r *Runtime[S, E, T]) Fire(ctx context.Context, event E, data T) (S, error) r.limits = defaultLimits } if r.rootCount >= r.limits.MaxRoots { - state := r.state + state := r.instance.State() r.mu.Unlock() return state, ErrRootLimit } @@ -248,17 +243,12 @@ func (r *Runtime[S, E, T]) removeRootLocked(target *root[E, T, S]) bool { // can overlap an executing callback; callers must synchronize mutable data in // T and keep Guards pure. func (r *Runtime[S, E, T]) Permitted(ctx context.Context, data T) iter.Seq2[E, S] { - r.mu.Lock() - state := r.state - machine := r.machine - r.mu.Unlock() - type pair struct { event E to S } var snapshot []pair - for event, to := range machine.Permitted(ctx, state, data) { + for event, to := range r.instance.Permitted(ctx, data) { snapshot = append(snapshot, pair{event, to}) } @@ -489,7 +479,7 @@ func (r *Runtime[S, E, T]) run(req *root[E, T, S]) (result outcome[S]) { } from := r.State() - to, err := r.fireMachine(current.ctx, from, current.event, current.data) + to, err := r.instance.Fire(current.ctx, current.event, current.data) if err != nil { c.abort() result = outcome[S]{state: r.State(), err: err} @@ -499,7 +489,6 @@ func (r *Runtime[S, E, T]) run(req *root[E, T, S]) (result outcome[S]) { var step uint64 var observers []statemachine.Observer[S, E, T] r.mu.Lock() - r.state = to if from != to && len(r.observers) != 0 { step = r.seq + 1 r.seq += 2 @@ -532,7 +521,3 @@ func (r *Runtime[S, E, T]) run(req *root[E, T, S]) (result outcome[S]) { completed = true return result } - -func (r *Runtime[S, E, T]) fireMachine(ctx context.Context, from S, event E, data T) (S, error) { - return r.machine.Fire(ctx, from, event, data) -} diff --git a/scratch/codex-review.md b/scratch/codex-review.md new file mode 100644 index 0000000..cf61093 --- /dev/null +++ b/scratch/codex-review.md @@ -0,0 +1,306 @@ +# Maritime Safety, CE Readiness, and Go Architecture Review + +Review date: 2026-08-09 + +Scope: the repository at main/70a7308, reviewed as a maritime robotics execution library where human safety, restart behavior, incident evidence, and product liability matter. + +## Executive verdict + +This codebase is a strong safety-adjacent application-orchestration library. It is not sufficient as a safety-rated control element, the sole controller for hazardous motion, or evidence of CE conformity. + +The correct deployment judgment is: + +- Go for coordinating application logic inside a larger, independently protected vessel architecture. +- No-go for directly owning the only path to thrusters, winches, brakes, gangways, steering, launch/recovery machinery, or mobile robotic motion around people. + +The repository itself states this limitation accurately in [SAFETY.md](../SAFETY.md#safety-adjacent-use). Logical state is not physical truth, and Supervisor.Trip is not an emergency stop. + +Direct answers: + +| Question | Assessment | +|---|---| +| Does it do everything needed? | No. Independent protective functions, durable command closure, recovery adjudication, authority/fencing, evidence integrity, and the product safety case remain external. | +| Are its patterns abnormal? | Mostly sound. Record creation after operation completion, post-commit Observer errors, prepare-only journaling, and goroutine-based callback containment have important sharp edges. | +| Is it immutable enough? | Structurally yes for ordinary Go; no for forensic evidence. States, events, errors, callbacks, Records, Snapshots, and application data are shallow. | +| Is it observable enough? | Supervisor.Status is strong, but lifecycle order, persistence, retention, and ordinary Observers are not liability-grade. | +| Does it follow strong Go practice? | Generally yes, with one release-blocking module-version defect and several medium concurrency/interface issues. | + +## Release and deployment blockers + +### 1. This is not an independent safety function + +Supervisor.Trip and operation timeout cancel a Go context and latch a software Fault. They cannot stop a callback that ignores cancellation and cannot prove that physical work stopped. See [Trip](../supervised/execution.go#L668), [callback timeout selection](../supervised/execution.go#L920), and [fault latching](../supervised/execution.go#L1148). + +Concrete scenario: a thruster or winch command reaches its controller, the callback blocks, and the operation times out. The Supervisor becomes Faulted and reports CallbackRunning, but thrust or haul-in may continue. + +A deployed system still requires independent emergency stop, protective stop, safe torque off or brake application, overspeed protection, collision avoidance, human-presence separation, watchdogs, command expiry, and measured worst-case physical response. + +### 2. The declared v2 release is invalid under Go module versioning + +[VERSION](../VERSION) and [CHANGELOG.md](../CHANGELOG.md#200) declare 2.0.0, while [go.mod](../go.mod) still declares: + + module github.com/open-ships/statemachine + +A v2 Go module must use github.com/open-ships/statemachine/v2, including its internal imports and public examples. Otherwise a v2.0.0 tag is not a valid consumable v2 at the advertised import path. + +Before release, either migrate the module and imports to /v2 or keep the release on v1. Add a clean external-consumer test that resolves the exact proposed tag. + +Official Go rule: https://go.dev/ref/mod#major-version-suffixes + +## High-severity implementation findings + +### 3. Lifecycle Record ordering is not causally authoritative + +Record.Seq is documented as authoritative ordering, but public operations create their Record only after the internal operation has released the Supervisor. See [Issue](../supervised/execution.go#L381) and [recordResult](../supervised/record.go#L86). + +Record.Mode and Record.At are sampled when recordResult eventually obtains its locks, rather than at the operation's state-linearization point. A concurrent Issue, Trip, Recover, or verification expiry can therefore: + +- receive an earlier Record sequence despite occurring later; +- cause an earlier Record to describe a later Mode; or +- make Record.At represent recorder serialization delay rather than outcome time. + +Restoration creates another identity defect. Snapshot preserves ExecutionID but not the lifecycle Record high-water mark. A restored execution begins again at Record.Seq 1, duplicating the durable identity pair (ExecutionID, Seq). + +For incident reconstruction, Record identity, time, Mode, and sequence must be assigned atomically with the state decision. Delivery can occur later, but identity cannot. + +### 4. Persistence is prepare-only and cannot durably close an issued Change + +The Journal receives an in-doubt Snapshot before external Issue in [prepareIssue](../supervised/execution.go#L544). Successful commit updates only in-memory state and Revision in [commit](../supervised/execution.go#L1015). The Journal is not updated with a committed, rejected, faulted, or reconciled closure. + +Concrete scenario: + +1. Engage propulsion is issued. +2. Physical Verification succeeds. +3. Logical state commits to Underway. +4. The process loses power before an application-owned clean Snapshot is persisted. +5. The last durable Journal entry still says in doubt from Ready. + +Restoration conservatively faults, which is correct, but Recover can only run predicate-like Reconcilers. A successful Recover clears the Fault while keeping the old state at [recovery completion](../supervised/execution.go#L772). It cannot explicitly adopt a controller-proven Destination, retain Source, or select and record a minimum-risk state. + +The durable execution protocol needs explicit closure and a typed, auditable reconciliation decision for every power-loss point. + +### 5. Observation and recording can exhaust or freeze execution + +Observer isolation launches a goroutine and waits indefinitely for it at [internal/observer.Call](../internal/observer/observer.go#L17). There is no timeout or cancellation select. + +A blocked network logger can therefore: + +- leave an Instance or Statechart in flight after state already committed; +- freeze a queued Runtime's drain loop and every later root; and +- prevent the caller from learning the committed outcome. + +Supervisor Records also grow without limit at [record append](../supervised/record.go#L108). Recorder timeout bounds recordResult's wait, but it cannot terminate a Recorder that ignores cancellation. One permanently blocked Recorder goroutine can be leaked per operation. + +Vessel-lifetime execution needs bounded history, bounded outstanding deliveries, and an explicit drop, retry, degrade, or fault policy. + +### 6. Post-commit Observer failure shares the transition error channel + +Instance commits state and then joins an Observer failure into Fire's ordinary error at [Instance.Fire](../instance.go#L122). Statechart commits and can return only an Observer error at [Statechart.Fire](../statechart/statechart.go#L687). queued.Runtime behaves similarly after one or more committed events. + +This is documented through ErrObserverFailed, but it violates the common operational assumption that err != nil means retry is safe. A retry can occur after effects and state changes already committed. + +Transition outcome and observation-health outcome should be structurally distinct, or observation health should be reported through an independent seam. + +### 7. Safety-critical durability remains opt-in + +The ordinary Supervisor constructors accept a Machine containing external Issue callbacks without a Journal or Recorder. [prepareIssue](../supervised/execution.go#L560) silently proceeds if Journal is nil. RequireJournal is optional. + +For a safety-focused use profile, external Issue should default to requiring durable preparation and durable lifecycle recording. A weaker mode should be explicitly named for tests or non-hazardous application work. + +### 8. Snapshot rollback protection and definition provenance are external + +Restore validates DefinitionID, declared state, counters, and pending-transition structure at [Restore](../supervised/execution.go#L183), which is useful. It does not supply: + +- an anti-rollback generation; +- a signature or MAC; +- an independent durable Attempt high-water authority; +- a Snapshot schema version; +- controller-side rejection of stale fencing tokens; or +- a cryptographic digest tying DefinitionID to callbacks, source, and build artifact. + +A valid older Snapshot can therefore be restored unless an external durable authority detects rollback. The documentation correctly assigns fencing, replay prevention, and provenance to the integration, but they remain mandatory. + +### 9. Verification evidence is conventional rather than enforceable + +Verify, Invariants, Postconditions, and Reconcilers receive arbitrary shallow T. The Module cannot require or validate: + +- sensor and controller source identity; +- acquisition time and maximum evidence age; +- coherent sampling across subsystems; +- controller boot or lease identity; +- calibration and health state; +- plausibility and independence; +- operator authority; or +- an immutable evidence snapshot. + +This is acceptable for a generic Go library, but a CE-oriented integration profile needs typed evidence with those invariants before any Check runs. + +## Immutability assessment + +### Strong + +- Machine and Chart compilation defensively copies tables and action slices. +- State-owning executions copy compiled definition values, preventing caller whole-value overwrite. +- Internal compiled maps and slices are private. +- Fault snapshots copy public Fault structs rather than returning Supervisor storage. +- Construction acts as the restoration seam; unrestricted state setters are absent. + +### Insufficient for safety evidence + +- comparable permits pointer-bearing state and event types. +- Function values and mutable closure captures remain shared. +- T is caller-owned and may be concurrently mutated. +- Snapshot, Record, Observation, state, and event values are shallow. +- Fault.Cause retains an arbitrary possibly mutable error object. +- Records returns a new outer slice but does not deep-copy referenced values. + +The existing [known limitations](../docs/assurance/known-limitations.md) acknowledge most of this. A safety profile should restrict states and events to small defined scalar values, forbid mutable closure captures, serialize evidence at durable seams, and bind definitions to reviewed artifacts. + +## Visibility and observability assessment + +Supervisor.Status is strong. It exposes Mode, logical Snapshot, active Operation, Phase, deadlines, pending Change, Fault, callback liveness, and Recorder failure. + +Supervisor Records also distinguish starts, issues, verification, trips, recovery, expiry, and secondary causes. + +Remaining gaps: + +- causal and restart-unique Record sequencing is broken; +- Record cause is primarily text and loses structured callback/check identity; +- durable recording is optional and fail-open; +- Recorder failure is only a volatile string, with no lost-record range, sequence, count, or replay queue; +- ordinary Observations omit attempts, refusals, action execution, self-transitions, and internal transitions by design; +- ordinary Observers cannot return an I/O error; +- Observation sequence restarts on reconstructed executions and has no built-in ExecutionID; +- non-supervised observation timestamps use time.Now directly; and +- all in-memory histories are process-local. + +Ordinary Observations are useful application deltas, not a safety audit trail. + +## Go design assessment + +### Strong and idiomatic + +- Clear package separation by state ownership and failure semantics. +- Typed generic states, events, and application data. +- Context is consistently the first callback argument. +- Errors support errors.Is, errors.As, and errors.Join appropriately. +- Constructors return concrete values; Store, Clock, Recorder, and Journal are narrow seams. +- Function adapters such as FuncStore are idiomatic. +- User callbacks generally run without the execution mutex held. +- Zero-value behavior is intentional where it is safe; Supervisor construction is strict. +- Compile and MustCompile follow familiar Go conventions. +- iter.Seq inspection methods keep immutable definitions read-only. +- No runtime dependencies beyond the standard library. + +### Abnormal or risky + +- Observer functions cannot return errors, so composition uses panic containment as an error-transport mechanism. +- Result plus a duplicate ordinary error is unusual, though justified by errcheck visibility. +- Goroutine-per-callback containment is sophisticated but can leak when application code ignores cancellation. +- Clock.Now, Clock.AfterFunc, and Timer.Stop are called while the Supervisor mutex is held. The Clock contract does not explicitly prohibit synchronous callbacks or lock reentrancy, permitting a custom Clock deadlock. +- Store callback-contract enforcement cannot prevent a broken Store from invoking its first callback after Update has already returned. +- Operation, Record, and Observation counters can wrap even though their contracts imply non-reuse or authoritative ordering. + +The large supervised/execution.go file is not itself a defect. It preserves locality for difficult synchronization invariants. Splitting it mechanically would likely make the Module shallower and harder to audit. + +## Assurance and test evidence + +Local verification with Go 1.26.0: + +- go test ./... +- go test -race ./... +- go vet ./... +- gofmt +- errcheck +- staticcheck +- govulncheck + +All passed. Race-run statement coverage was 94.3%, and govulncheck reported no called vulnerability. + +The repository also has strong multi-platform CI, a coverage floor, vulnerability scanning, signed release intent, SBOM generation, checksums, and provenance attestation. + +Important limitations: + +- the sole fuzz model exercises synchronous Supervisor lifecycle commands; +- CI runs fuzz seeds, not a sustained fuzz campaign; +- there is no durable-record/restoration model; +- no adversarial Clock model; +- no concurrent Trip/Verify/expiry state model; +- no Observer liveness or resource-exhaustion property test; +- no Store contract fuzzing; +- no power-loss matrix at every durable seam; and +- statement coverage is not evidence of hazard coverage or independence. + +No library test establishes a required PLr, SIL, systematic capability, diagnostic coverage, or physical response time. + +## CE and maritime context + +At the review date, the EU Machinery Regulation is scheduled to apply from 20 January 2027. It explicitly recognizes software as a possible safety component, requires risk assessment, and requires control-system logic faults not to produce hazardous situations. Autonomous software-based safety systems may also have safety-decision recording obligations. + +Official text: https://eur-lex.europa.eu/eli/reg/2023/1230/en + +The non-mandatory IMO MASS Code has been effective since 1 July 2026. It emphasizes risk assessment, robust system design, cybersecurity, alert management, remote operations, and continued human responsibility. This library supplies only a small execution primitive within that system. + +IMO overview: https://www.imo.org/en/mediacentre/hottopics/pages/autonomous-shipping.aspx + +For products placed on the market after 9 December 2026, the revised Product Liability Directive treats software as a product and provides for evidence disclosure and legal presumptions in specified circumstances. Durable, causal records therefore matter to liability as well as operations. + +Official text: https://eur-lex.europa.eu/eli/dir/2024/2853/oj + +A real conformity route still needs a product-specific hazard analysis and selection of applicable standards, potentially including: + +- ISO 12100 for machinery risk assessment and risk reduction; +- ISO 13849-1:2023 or IEC 62061:2021 with current amendments for safety-related control systems; +- IEC 61508 where its lifecycle is applicable; +- applicable marine equipment, flag-state, class, SOLAS, and MASS requirements; and +- cybersecurity obligations and standards appropriate to the product and deployment. + +This review is an engineering assessment, not a conformity certificate or legal opinion. + +## Architectural deepening priorities + +### 1. Durable Supervisor lifecycle Module + +Files: supervised/execution.go, supervised/record.go, supervised/result.go. + +Problem: state decisions, Record identity, Journal state, and Recorder delivery do not share one causal point. + +Solution: concentrate lifecycle identity and durable closure behind one deep Module, while allowing delivery to occur after the decision. + +Benefits: stronger locality, restart-safe identity, causal audit order, and power-loss testing through one interface. + +### 2. Recovery adjudication Module + +Files: supervised/execution.go and supervised/definition.go. + +Problem: Recover can validate only the old logical state and cannot record a decision to retain Source, adopt Destination, or select a minimum-risk state. + +Solution: make reconciliation produce an explicit, durable adjudication before the Supervisor returns to Ready. + +Benefits: leverage across controller-restart cases, better locality for recovery rules, and direct tests for every in-doubt physical outcome. + +### 3. Bounded observation-delivery Module + +Files: observe.go, internal/observer, queued/observe.go, and statechart/observe.go. + +Problem: synchronous delivery is duplicated, can block forever, and conflates observation failure with transition failure. + +Solution: concentrate delivery deadlines, backpressure, lineage, and failure classification at one seam. + +Benefits: deterministic resource bounds, clearer caller behavior, and reusable liveness tests. + +### 4. Deterministic Supervisor model Module + +Files: principally supervised/execution.go. + +Problem: state decisions, Clock behavior, callback ownership, persistence, and delivery are interwoven. + +Solution: isolate a deterministic execution model behind the existing Supervisor interface while keeping callbacks and Adapters in the implementation. + +Benefits: model checking, replay, fault injection, and improved locality without expanding the public interface. + +## Bottom line + +The repository is substantially better than a typical Go state-machine library. Its domain language, failure semantics, documentation, tests, and explicit safety limitations are excellent. + +It should be used as application orchestration behind independent protection, not represented as CE-ready or safety-rated. Before v2 release, the module path must be corrected. Before any safety-adjacent production claim, causal durable lifecycle recording, closed-loop restart adjudication, bounded observation resources, and a hazard-derived system assurance case are the most important next steps. diff --git a/scratch/fable-review.md b/scratch/fable-review.md new file mode 100644 index 0000000..2453bf3 --- /dev/null +++ b/scratch/fable-review.md @@ -0,0 +1,200 @@ +# Codebase Evaluation — maritime roboticist, CE-grade lens + +Reviewer perspective: maritime-oriented robotics, CE-marking / class-society integration +(IEC 61508-3 SOUP justification, ISO 13849, DNV-RU-SHIP Pt.4 Ch.9-style software assessment), +human safety and liability as first priority. + +Scope reviewed: every source file in `statemachine`, `queued`, `statechart`, `persist`, +`supervised`, `internal/*`; `SAFETY.md`, `docs/assurance/*`, `docs/adr/*`, CI workflows. +Verification run locally: `go vet ./...` clean; `go test -race -count=1 ./...` all green +(177 test funcs, ~6.7k test lines vs ~5.5k source lines). + +--- + +## Overall verdict + +One of the most honest and disciplined "safety-adjacent" libraries reviewed. The critical +insight — a committed logical state is **not evidence an actuator moved** — is threaded +through the whole design (SAFETY.md, the Issue/Verify split, `Uncertain` latching, in-doubt +snapshots). The scope boundary ("not a safety controller; e-stop must not depend on our lock +or queue") is exactly the claim structure a notified body or class society wants from a COTS +component. The `docs/assurance/requirements.md` LIB-*→test traceability matrix and INT-* +integration obligations are rare and genuinely useful for a safety case. + +Found: **one release-blocking defect, one genuine resource-safety gap, one undocumented +deadlock hazard**, plus feature gaps to weigh before betting a vessel architecture on it. + +--- + +## Findings, ranked + +### 1. 🔴 Release-blocking: v2.0.0 without a `/v2` module path +`VERSION` says `2.0.0`, `CHANGELOG.md` documents 2.0.0, git tags stop at `v1.2.1` — but +`go.mod:1` still reads `module github.com/open-ships/statemachine`. Go's semantic import +versioning **rejects** a `v2.0.0` tag on a module whose `go.mod` path doesn't end in `/v2`. +The signed release process described in the assurance docs will produce a tag `go get` +cannot resolve. Either the module path must become +`github.com/open-ships/statemachine/v2` (updating every internal import — `statemachine.go:10`, +`supervised/result.go:8`, etc.), or the release must stay in v1 space. + +### 2. 🔴 Unbounded in-process `Records` history +`supervised/record.go:108` — `s.records = append(s.records, record)` with no cap, ring, or +trim. Every Start/Issue/Verify/Trip/Recover/expiry/secondary-cause appends forever. The +project's own assurance list demands "deterministic resource limits," and `queued` got finite +root/run limits in v2 — but a Supervisor on a months-long deployment (the maritime norm) is +a slow, unbounded memory leak, and `Records()` copies the whole slice each call. Needs a +bounded ring (Seq already provides authoritative ordering, so drop-oldest is sound) or at +minimum an entry in `known-limitations.md`. Currently neither bounded nor documented. + +### 3. 🟠 Clock/Timer contract is undocumented and lock-coupled +`s.clock.Now()`, `s.clock.AfterFunc(...)`, and `timer.Stop()` are invoked **while holding +`s.mu`** (`supervised/execution.go:879`, `:975`, `:1065`, `:1184`). Consequences: + +- A Clock that blocks (network time source, contended fake) blocks **`Trip`**, which needs + `s.mu` — quietly undermining the SAFETY.md claim that Trip is the supervisory inhibition + path. +- A test fake whose `AfterFunc` fires the callback synchronously deadlocks instantly + (`verificationExpired` re-acquires `s.mu`), as does a fake that advances time under its own + lock (lock-order inversion with `s.mu`). + +`clock.go` documents that verification timers are notification-only, but not the real +contract: *Now/AfterFunc/Stop must be non-blocking and must never invoke callbacks +synchronously.* For a seam explicitly designed for injection, that contract belongs in the +interface docs — an assessor will ask. + +Related, minor: `Trip` latches immediately under `s.mu` (good), but its *return* can stall +up to ~2× `RecorderTimeout` behind `recordMu` (`record.go:87-115`). Worth one sentence in +the Trip docs so callers don't put Trip on a latency-sensitive path expecting fast return. + +### 4. 🟡 Redundant per-Fire reflection in the hot path +`statemachine.go:223` runs `keycheck.Value(from)` and `keycheck.Value(event)` on **every** +`Fire`. Strictness is a property of the *type*, and `Compile` already proved it via +`StrictType` — for any compiled Machine these checks can never fail; they exist only to +protect the zero Machine that bypassed `Compile`. Each is a reflection call with a likely +heap escape, twice per transition, forever. Store a `strict bool` at compile time and only +value-check when false. Not a correctness issue, but free latency in a control loop. + +### 5. 🟡 Wall-clock time in evidence, GPS/NTP steps +Deadline math is monotonic-safe (`clock.Now().Add(...)` / `.Before(...)` preserve the +monotonic reading, and restored in-doubt snapshots never resume timers — correctly latched +Faulted instead). But `Observation.At`, `Record.At`, `Snapshot.RecordedAt` are wall time, +and vessels see real clock steps (GPS discipline, NTP after link loss). Fine and +conventional for incident correlation — but integration guidance should note that `At` +ordering across a clock step is not trustworthy; `Seq` is authoritative (which +`record.go:44` does state — good). + +### 6. Edge: `StepResult` race on a contract-violating Store +`persist/persist.go:156-187` — detection of double-invoke/no-invoke is excellent (CAS + +`firstDone` join handles a Store that calls `step` from another goroutine). But a truly +rogue Store that calls `step` again *after* `Update` returns writes +`result.From/To/TransitionError` while the caller reads the returned `StepResult` — a data +race outside the detection window. Best-effort is reasonable; a one-line doc note +("detection covers calls made before Update returns") would close it. + +--- + +## Does it do everything a maritime roboticist needs? + +**In scope and present, done well:** mandatory non-bypassable checks with correct +Guard-vs-interlock separation (SAFETY.md is *right* that guards route and must never be +interlocks — a distinction most FSM libraries botch); split Issue/Verify with fresh-evidence +seam; first-cause fault latching with secondary-cause recording; in-doubt journaling +*before* Issue; non-reusable Execution/Attempt identity for fencing correlation; +counter-exhaustion checks (`ErrCounterExhausted`); reconcile-before-start/recover; +`CallbackRunning` visibility for un-killable Go callbacks; deep Restore validation +(`supervised/execution.go:186-262`). + +**Absent — decide if you can live without:** + +- **No hierarchical supervised machine.** `supervised` is flat-only; `statechart` has + hierarchy but no mandatory checks, budgets, or fault latching. Maritime mode structures + (DP2 ⊃ auto-heading ⊃ …) must be flattened into a supervised definition or split across + Supervisors. Real modeling cost. +- **No parallel/orthogonal regions and no history states** in `statechart`. Propulsion × + steering × nav-lights as independent concurrent regions means N separate Instances with + app-level coordination. +- **No timed states / dwell limits.** Operation and verification budgets exist, but "must + leave `Maneuvering` within 30 s or fault" needs an app-side timer firing an event. Common + IEC 61508-style supervisory requirement; it will be rebuilt repeatedly. +- **No event priority or preemption.** `queued` is strictly FIFO; the only preemption is + `Trip`→Fault. Coherent with "e-stop lives outside," but degraded-mode transitions + competing with a deep queue have no fast lane. +- **No graceful abort of a pending Verify.** Operator abort = Trip → Fault → Recover. + Workable and conservative, but the recovery ceremony for a routine abort may irritate + operators; decide whether that's a feature (audit trail) or friction. +- **No snapshot schema versioning.** `Snapshot` carries `DefinitionID` but no format-version + field; the docs demand "explicit Snapshot migration" yet the type gives migrations nothing + to key on. Cheap to add now, painful after fielded journals exist. + +All consistent with stated scope — but for a CE-marked product the dwell-timer, priority, +and mode-hierarchy layers land in application code, inside your safety case. + +--- + +## Immutability and observability + +**Immutability: exemplary.** Compiled definitions are deep-copied at construction and +*owned by value* inside executions (`instance.go:72`, `queued/queued.go:125`, +`supervised/execution.go:140`) so a caller overwriting `*machine` can't mutate a live +execution — tested (LIB-DEF-001). Faults are snapshotted on every exposure so no caller can +mutate the retained first cause (`faultRecord.snapshot`, tested by LIB-SUP-005). `Records()` +returns a copy. `known-limitations.md` honestly flags the residual hole (shared function +values / mutable closure captures / shallow T) — the correct place, since Go can't enforce it. + +**Observability: strong, with the right semantics.** Observations fire only on *commit*, +never on attempt (correct — a census must never count uncommitted motion); +construction/restoration emit nothing and docs warn a fleet census must seed independently. +Seq/Step/Run/Remaining gives batch-complete detection. Observer panic/Goexit containment +preserves original stacks and reports post-commit as `ErrObserverFailed` without +un-committing state — the correct liability posture (the state change happened; the witness +failed). `Status` exposes `CallbackRunning` and `RecorderError`. Gaps: the `Records` bound +(finding #2), and `Record.CauseText` is free text — fine for forensics, but +machine-classifiable incident taxonomies need app-side error codes. + +--- + +## Abnormal design patterns (and whether they're justified) + +| Pattern | Verdict | +|---|---| +| Value-copy of compiled Machine into executions | Abnormal (Go norm: store the pointer) but deliberate, tested, cheap. Keep. | +| Goroutine-per-callback in `invoke`/`observer.Call` to contain `runtime.Goexit` | Very unusual; the only way to survive Goexit in Go. Cost: every guard/check/effect runs off the caller's goroutine (scheduling jitter, no goroutine-locals). Justified for this scope. | +| `Observers(...)` combinator communicating inner failures via `panic(errors.Join(...))` (`observe.go:111`) | Works only because delivery re-contains it; a user invoking the combined observer directly gets a surprise panic. Undocumented — add one sentence or return-based plumbing. | +| Returning `(Result, error)` where `err == result.Err` | Redundant by Go norms, but the errcheck rationale is stated and sound for this domain. Keep. | +| Type-erased execution context in `queued` (`execution.enqueue func(ctx, any, any) error`, `assign[T]`) | The `assign` failure path is unreachable in practice (owner check precedes it) — defensive dead code, mildly smelly but harmless. | +| Inconsistencies: `statechart.Instance.Fire` returns only `error` while flat `Instance.Fire` returns `(S, error)`; `sync.RWMutex` in statechart vs `Mutex` in Instance; `root[E, T, S]` type-param order | Cosmetic, but for a v2 API freeze reconcile the `Fire` signature asymmetry now. | + +--- + +## Go idiom scorecard + +**Excellent:** +- Useful zero values that fail safe (zero Machine *refuses* everything — the right default + polarity for this domain). +- Sentinel errors with `Unwrap() []error` multi-error trees, used correctly and — unusually — + with documented sentinel-travel hazards (`statemachine.go:27-30`). +- Modern `iter.Seq2` iterators with eager-vs-lazy semantics explicitly distinguished between + `Machine.Permitted` and `Instance.Permitted`. +- `MustCompile` mirroring `regexp`, with correct "literals are program text, generated tables + are input" guidance. +- No global state (v2 killed ambient `Enqueue`); `internal/` packages for shared invariants. +- `errors.Join` compile diagnostics reporting *all* defects at once. +- Zero dependencies verified in CI; CI actions pinned by SHA; race detector, coverage floor, + govulncheck, fuzz, and benchmarks in the pipeline. +- Doc comments are reference-grade — every hazard a user will hit is written down at the API + element where they'll hit it. + +**Deductions:** per-Fire reflection (#4), unbounded records slice (#2), undocumented Clock +contract (#3), and the module-path defect (#1) — the one outright violation of Go ecosystem +rules. + +--- + +## Bottom line + +As the coordination/orchestration layer above an independent hazard-rated safety chain — the +only role it claims — this is acceptable for a maritime system's software bill of materials, +and the assurance documentation would actively help the certification file. Fix the `/v2` +module path before tagging, bound or document the `Records` growth, and write down the Clock +contract; then the remaining questions are architectural fit (flat-only supervision, no +parallel regions, no dwell timers), not quality. diff --git a/statemachine.go b/statemachine.go index 5ddc21f..794932e 100644 --- a/statemachine.go +++ b/statemachine.go @@ -14,7 +14,7 @@ import ( // table has this From and Event, or every row that does has a Guard that // declined. // -// The error [Machine.Fire] reports wraps ErrNotPermitted together with every +// The error [Machine.Next] reports wraps ErrNotPermitted together with every // reason a Guard returned during the attempt — it implements // Unwrap() []error — so both of these can hold at once: // @@ -24,10 +24,10 @@ import ( // Test for your own reasons first: they are the more precise answer, and the // only one a caller can act on. // -// Like every sentinel in Go, this one travels: a Guard or Do that fires -// another Machine and returns that call's refusal will make errors.Is report a -// refusal for a transition this Machine permitted. Wrapping with %w does not -// help — it preserves the sentinel. Return a different error instead. +// Like every sentinel in Go, this one travels: returning a nested execution's +// refusal from a Guard or Do will make errors.Is report that refusal to its +// caller. Wrapping with %w does not help — it preserves the sentinel. Return a +// different error instead. var ( ErrNotPermitted = errors.New("transition not permitted") // ErrInvalidKey reports a state or event value that cannot safely be used @@ -36,7 +36,7 @@ var ( ) // A Transition is one row of a transition table: in state From, event Event -// moves the machine to state To. +// moves an execution to state To. // // Declare an alias to keep tables readable — note the =, which makes it an // alias rather than a defined type, so that [Compile] can still infer S, E and @@ -78,18 +78,19 @@ type Transition[S, E comparable, T any] struct { // Return a nil error, never a typed nil: a (*MyError)(nil) returned as an // error is non-nil, so the row silently declines and the next one wins. // - // Guard must not modify data, perform I/O or panic: [Machine.Fire] calls it + // Guard must not modify data, perform I/O or panic: [Machine.Next] calls it // on rows it does not select, and [Machine.Permitted] calls it while // iterating. Nothing enforces this. Guard func(ctx context.Context, data T) error - // Do performs the effect of the transition. It runs once, after this row is - // selected and before Fire reports the new state, so the state advances if - // and only if Do returned nil. A nil Do does nothing. + // Do performs the effect of the transition. It is never run by [Machine.Next] + // or [Machine.Permitted]. It runs only through a state-owning execution such + // as [Instance], queued.Runtime, or persist Store-backed execution. A nil Do + // does nothing. // - // Do is the only thing that can fail a row that was selected, and the error - // it returns is the error Fire returns, unwrapped. A failing Do does not - // fall through to the next matching row: the row was already chosen. + // Do runs once after its row is selected and before the execution publishes + // the destination. A failing Do leaves that execution's state unchanged and + // does not fall through to the next matching row: the row was already chosen. Do func(ctx context.Context, data T) error } @@ -103,13 +104,13 @@ type key[S, E comparable] struct { // // A Machine is immutable and safe for concurrent use by any number of // goroutines. This package takes no locks, so no deadlock originates here; the -// state values and the data passed to [Machine.Fire] belong to the caller and +// state values and the data passed to [Machine.Next] belong to the caller and // are the caller's to synchronize. The zero Machine has no rows and refuses // every event. // // S and E must be strictly comparable. Go's comparable constraint also admits // interface-bearing types; Compile rejects them before building any map. A -// zero Machine reports [ErrInvalidKey] from Fire for an uncomparable dynamic +// zero Machine reports [ErrInvalidKey] from Next for an uncomparable dynamic // value, while Permitted returns an empty sequence. Prefer distinct defined // string or integer types for S and E. type Machine[S, E comparable, T any] struct { @@ -119,7 +120,7 @@ type Machine[S, E comparable, T any] struct { // strict records that Compile proved S and E free of interface-bearing // types, so no dynamic value of either can be an uncomparable map key. // Strictness is a property of the type: once proved at compile time, the - // per-value reflection check in Fire and Permitted is unnecessary. The + // per-value reflection check in Next and Permitted is unnecessary. The // zero Machine has not been through Compile and keeps the per-value check. strict bool } @@ -192,69 +193,86 @@ func MustCompile[S, E comparable, T any](transitions []Transition[S, E, T]) *Mac return m } -// Fire applies event to state from and reports the state to move to. +// Next selects the transition for event in state from and reports its +// destination without running its effect. // -// Fire considers the rows whose From and Event match, in table order, and -// selects the first whose Guard is nil or returns nil. It runs that row's Do -// and, if Do returns nil, reports the row's To. A failing Do does not fall -// through to the next row. +// Next considers the rows whose From and Event match, in table order, and +// selects the first whose Guard is nil or returns nil. It reports that row's +// To. It never calls Do, so discarding its result cannot leave an effect behind. // -// In every other case Fire reports from, unchanged, so assigning the result is -// always correct: +// In every other case Next reports from, unchanged: // -// var err error -// order.State, err = orders.Fire(ctx, order.State, Pay, cmd) +// next, err := orders.Next(ctx, order.State, Pay, cmd) // -// Discarding it never is: the effect has already run, and neither the compiler -// nor go vet reports the lost transition. +// Next is for planning and for caller-owned transitions whose selected row has +// no Do. Use a state-owning execution to perform a transition with an effect; +// assigning Next's destination would deliberately skip that effect. // // The error wraps [ErrNotPermitted], and every reason a Guard returned, when no -// row was selected; otherwise it is exactly the error Do returned. +// row was selected. A selected row always returns a nil error because Next does +// not execute effects. // -// Fire reads the state from the from argument, never from data. A Guard or Do -// that writes a state field on data is writing a second copy that this package -// neither reads nor updates: the write is discarded on success and left behind -// on failure. +// Next reads the state from the from argument, never from data. A Guard that +// writes a state field on data is writing a second copy that this package +// neither reads nor updates. // -// Fire never reads ctx. It passes ctx to Guard and Do, which may honor -// cancellation themselves; a Fire under an already-cancelled context still -// transitions if its Guard and Do do not object. -// -// A Guard or Do may call Fire — on this Machine or another — with no -// restriction, because there is no lock and no in-flight state. What a nested -// call reaches is returned only to the nested caller: the outer Fire still -// reports its own row's To on success, and reports from on failure, discarding -// any state the nested call reached while its effects stand. Nest only across -// distinct aggregates, and never fire the machine that owns the state the -// current transition is advancing. -func (m *Machine[S, E, T]) Fire(ctx context.Context, from S, event E, data T) (S, error) { +// Next never reads ctx itself. It passes ctx to Guard, which may honor +// cancellation; a Next under an already-cancelled context still selects a row +// if its Guard does not object. +func (m *Machine[S, E, T]) Next(ctx context.Context, from S, event E, data T) (S, error) { + transition, err := m.selectTransition(ctx, from, event, data) + if err != nil { + return from, err + } + return transition.To, nil +} + +// fire is the effectful half of flat execution. Keeping it package-private is +// the architectural invariant: a caller-owned state value can ask a Machine +// what comes next, but only a state-owning Instance can perform Do. +func (m *Machine[S, E, T]) fire(ctx context.Context, from S, event E, data T) (S, error) { + transition, err := m.selectTransition(ctx, from, event, data) + if err != nil { + return from, err + } + if transition.Do != nil { + if err := transition.Do(ctx, data); err != nil { + return from, err + } + } + return transition.To, nil +} + +func (m *Machine[S, E, T]) selectTransition( + ctx context.Context, + from S, + event E, + data T, +) (*Transition[S, E, T], error) { if !m.strict && (!keycheck.Value(from) || !keycheck.Value(event)) { - return from, ErrInvalidKey + return nil, ErrInvalidKey } var reasons []error - for _, t := range m.rows[key[S, E]{from, event}] { + rows := m.rows[key[S, E]{from, event}] + for index := range rows { + t := &rows[index] if t.Guard != nil { if err := t.Guard(ctx, data); err != nil { reasons = append(reasons, err) continue } } - if t.Do != nil { - if err := t.Do(ctx, data); err != nil { - return from, err - } - } - return t.To, nil + return t, nil } - return from, &refusal[S, E]{from, event, append([]error{ErrNotPermitted}, reasons...)} + return nil, &refusal[S, E]{from, event, append([]error{ErrNotPermitted}, reasons...)} } -// Permitted iterates the events [Machine.Fire] would accept in state from for -// data, each paired with the state that firing it would reach. +// Permitted iterates the events [Machine.Next] would accept in state from for +// data, each paired with the state that selecting it would reach. // // An event is yielded at most once, in the order of the first row that mentions -// it for from, and the state yielded with it is exactly the state Fire would -// report, because Permitted selects rows by the rule Fire uses. An event whose +// it for from, and the state yielded with it is exactly the state Next would +// report, because Permitted selects rows by the rule Next uses. An event whose // every matching row declines is not yielded. No Do runs and nothing changes. // // The iterator is lazy: Guards run as it is ranged, not when Permitted is @@ -263,14 +281,12 @@ func (m *Machine[S, E, T]) Fire(ctx context.Context, from S, event E, data T) (S // the remaining Guards uncalled. If you adapt it with [iter.Pull2], call the // returned stop function. // -// Permitted and Fire agree on row selection when passed equal data. They can -// still disagree on outcome: a Do may fail, and data assembled for display -// often omits fields — a request payload, an open transaction — that a Guard -// consults. Pass the same construction to both, or keep guard-relevant fields -// where both can see them. +// Permitted and Next agree on row selection when passed equal data. Data +// assembled for display often omits fields that a Guard consults; pass the same +// construction to both, or keep guard-relevant fields where both can see them. // // Use Permitted to offer choices — the buttons on a page, the links in a -// response — not to decide whether to fire. Firing and handling +// response — not to decide whether to execute. Executing and handling // [ErrNotPermitted] cannot go stale between the question and the answer. func (m *Machine[S, E, T]) Permitted(ctx context.Context, from S, data T) iter.Seq2[E, S] { return func(yield func(E, S) bool) { @@ -291,7 +307,8 @@ func (m *Machine[S, E, T]) Permitted(ctx context.Context, from S, data T) iter.S } } -// refusal is the error Fire returns when no row was selected. It holds from and +// refusal is the error Next and state-owning executions return when no row was +// selected. It holds from and // event rather than a formatted string so that the common path — errors.Is, // without ever printing the error — does not format one, and it holds its // unwrap slice rather than rebuilding it on every errors.Is call. diff --git a/statemachine_test.go b/statemachine_test.go index c25e7af..0f47f89 100644 --- a/statemachine_test.go +++ b/statemachine_test.go @@ -98,9 +98,9 @@ var orders = statemachine.MustCompile([]row{ func ctx() context.Context { return context.Background() } -// --- Fire: the three outcomes ------------------------------------------------ +// --- Next: pure transition selection ----------------------------------------- -func TestFireSelectsFirstApplicableRow(t *testing.T) { +func TestNextSelectsFirstApplicableRowWithoutRunningEffects(t *testing.T) { for _, tc := range []struct { name string from state @@ -113,16 +113,16 @@ func TestFireSelectsFirstApplicableRow(t *testing.T) { {"bare row", draft, cancel, data{}, cancelled, true, nil}, {"guard applies", draft, submit, data{lines: 1}, pending, true, nil}, {"guard declines, no other row", draft, submit, data{}, draft, false, nil}, - {"effect runs", pending, pay, data{}, paid, true, []string{"charge"}}, - {"guarded row wins", paid, ship, data{inStock: true}, shipped, true, []string{"dispatch"}}, + {"effect is not run", pending, pay, data{}, paid, true, nil}, + {"guarded row wins without effect", paid, ship, data{inStock: true}, shipped, true, nil}, {"default arm wins", paid, ship, data{}, backorder, true, nil}, - {"self-transition", backorder, ship, data{inStock: true}, backorder, true, []string{"retry"}}, + {"self-transition without effect", backorder, ship, data{inStock: true}, backorder, true, nil}, {"wrong state", shipped, pay, data{}, shipped, false, nil}, {"unknown event", draft, event("teleport"), data{}, draft, false, nil}, } { t.Run(tc.name, func(t *testing.T) { d := tc.d - got, err := orders.Fire(ctx(), tc.from, tc.ev, &d) + got, err := orders.Next(ctx(), tc.from, tc.ev, &d) if got != tc.want { t.Errorf("state = %v, want %v", got, tc.want) } @@ -136,8 +136,26 @@ func TestFireSelectsFirstApplicableRow(t *testing.T) { } } -func TestFireReturnsFromUnchangedOnEveryFailure(t *testing.T) { - // The assignment idiom is only safe because this holds on all three paths. +func TestMachineNextNeverExecutesDo(t *testing.T) { + called := false + machine := statemachine.MustCompile([]row{{ + From: draft, Event: submit, To: pending, + Do: func(context.Context, *data) error { + called = true + panic("Machine.Next crossed the state-ownership seam") + }, + }}) + + next, err := machine.Next(ctx(), draft, submit, &data{}) + if err != nil || next != pending { + t.Fatalf("Next = %v, %v; want pending, nil", next, err) + } + if called { + t.Fatal("Machine.Next executed Do") + } +} + +func TestInstanceReturnsCommittedStateOnEveryFailure(t *testing.T) { m := statemachine.MustCompile([]row{ {From: draft, Event: submit, To: pending, Guard: hasLines}, {From: pending, Event: pay, To: paid, Do: fail(errDeclined)}, @@ -153,7 +171,7 @@ func TestFireReturnsFromUnchangedOnEveryFailure(t *testing.T) { } { t.Run(tc.name, func(t *testing.T) { d := data{} - got, err := m.Fire(ctx(), tc.from, tc.ev, &d) + got, err := statemachine.NewInstance(m, tc.from).Fire(ctx(), tc.ev, &d) if err == nil { t.Fatal("want an error") } @@ -169,7 +187,7 @@ func TestEffectErrorIsReturnedUnwrapped(t *testing.T) { {From: pending, Event: pay, To: paid, Do: fail(errDeclined)}, }) d := data{} - got, err := m.Fire(ctx(), pending, pay, &d) + got, err := statemachine.NewInstance(m, pending).Fire(ctx(), pay, &d) if !errors.Is(err, errDeclined) { t.Errorf("errors.Is(err, errDeclined) = false, err = %v", err) @@ -196,7 +214,7 @@ func TestFailingEffectDoesNotFallThroughToTheNextRow(t *testing.T) { {From: paid, Event: ship, To: backorder, Do: record("backorder")}, }) d := data{inStock: true} - got, err := m.Fire(ctx(), paid, ship, &d) + got, err := statemachine.NewInstance(m, paid).Fire(ctx(), ship, &d) if !errors.Is(err, errDeclined) { t.Errorf("err = %v, want the effect's error", err) } @@ -221,9 +239,9 @@ func TestGuardsRunInTableOrderAndStopAtTheFirstThatApplies(t *testing.T) { {From: draft, Event: submit, To: paid, Guard: guard("second", nil)}, {From: draft, Event: submit, To: shipped, Guard: guard("third", nil)}, }) - got, err := m.Fire(ctx(), draft, submit, &data{}) + got, err := m.Next(ctx(), draft, submit, &data{}) if err != nil { - t.Fatalf("Fire: %v", err) + t.Fatalf("Next: %v", err) } if got != paid { t.Errorf("state = %v, want paid (the second row)", got) @@ -240,7 +258,7 @@ func TestRefusalCarriesEveryGuardReason(t *testing.T) { {From: paid, Event: ship, To: shipped, Guard: inStock}, {From: paid, Event: ship, To: backorder, Guard: hasLines}, }) - _, err := m.Fire(ctx(), paid, ship, &data{}) + _, err := m.Next(ctx(), paid, ship, &data{}) if !errors.Is(err, statemachine.ErrNotPermitted) { t.Error("want ErrNotPermitted") @@ -257,7 +275,7 @@ func TestRefusalCarriesEveryGuardReason(t *testing.T) { } func TestRefusalWithoutReasons(t *testing.T) { - _, err := orders.Fire(ctx(), shipped, pay, &data{}) + _, err := orders.Next(ctx(), shipped, pay, &data{}) want := "statemachine: pay in state shipped: transition not permitted" if err.Error() != want { t.Errorf("message = %q, want %q", err.Error(), want) @@ -270,7 +288,7 @@ func TestRefusalWithoutReasons(t *testing.T) { func TestRefusalUnwrapIsStableAndDoesNotRebuild(t *testing.T) { // errors.Is walks Unwrap repeatedly; rebuilding the slice there made a // refusal allocate on every comparison. - _, err := orders.Fire(ctx(), draft, submit, &data{}) + _, err := orders.Next(ctx(), draft, submit, &data{}) u, ok := err.(interface{ Unwrap() []error }) if !ok { t.Fatal("a refusal must implement Unwrap() []error") @@ -295,9 +313,9 @@ func TestTypedNilGuardDeclines(t *testing.T) { }}, {From: draft, Event: submit, To: cancelled}, }) - got, err := m.Fire(ctx(), draft, submit, &data{}) + got, err := m.Next(ctx(), draft, submit, &data{}) if err != nil { - t.Fatalf("Fire: %v", err) + t.Fatalf("Next: %v", err) } if got != cancelled { t.Errorf("state = %v, want cancelled — a typed nil declines", got) @@ -359,11 +377,11 @@ func TestCompileCopiesTheTable(t *testing.T) { table[0].To = cancelled // mutate table = append(table, row{From: pending, Event: pay, To: paid}) // and extend - got, err := m.Fire(ctx(), draft, submit, &data{}) + got, err := m.Next(ctx(), draft, submit, &data{}) if err != nil || got != pending { - t.Errorf("Fire = %v, %v; want pending — the Machine must be immutable", got, err) + t.Errorf("Next = %v, %v; want pending — the Machine must be immutable", got, err) } - if _, err := m.Fire(ctx(), pending, pay, &data{}); !errors.Is(err, statemachine.ErrNotPermitted) { + if _, err := m.Next(ctx(), pending, pay, &data{}); !errors.Is(err, statemachine.ErrNotPermitted) { t.Error("a row appended after Compile must not be in the Machine") } _ = table @@ -391,9 +409,9 @@ func TestMustCompilePanicsWithCompileError(t *testing.T) { func TestZeroMachineRefusesEverything(t *testing.T) { var m statemachine.Machine[state, event, *data] - got, err := m.Fire(ctx(), draft, submit, &data{}) + got, err := m.Next(ctx(), draft, submit, &data{}) if got != draft || !errors.Is(err, statemachine.ErrNotPermitted) { - t.Errorf("Fire = %v, %v; want (draft, ErrNotPermitted)", got, err) + t.Errorf("Next = %v, %v; want (draft, ErrNotPermitted)", got, err) } if n := len(maps.Collect(m.Permitted(ctx(), draft, &data{}))); n != 0 { t.Errorf("Permitted yielded %d events, want 0", n) @@ -406,16 +424,16 @@ func TestCompileOfNilSliceBehavesLikeTheZeroMachine(t *testing.T) { if err != nil { t.Fatalf("Compile: %v", err) } - if _, err := m.Fire(ctx(), draft, submit, &data{}); !errors.Is(err, statemachine.ErrNotPermitted) { + if _, err := m.Next(ctx(), draft, submit, &data{}); !errors.Is(err, statemachine.ErrNotPermitted) { t.Errorf("err = %v, want ErrNotPermitted", err) } } // --- Permitted --------------------------------------------------------------- -func TestPermittedMatchesFire(t *testing.T) { +func TestPermittedMatchesNext(t *testing.T) { // The contract that makes Permitted worth its place: the state it yields is - // the state Fire would report, for every event, in every state. + // the state Next would report, for every event, in every state. for _, from := range []state{draft, pending, paid, shipped, backorder, cancelled} { for _, inStockNow := range []bool{true, false} { d := data{lines: 1, inStock: inStockNow} @@ -423,16 +441,16 @@ func TestPermittedMatchesFire(t *testing.T) { for _, ev := range []event{submit, pay, ship, cancel} { fresh := data{lines: 1, inStock: inStockNow} - got, err := orders.Fire(ctx(), from, ev, &fresh) + got, err := orders.Next(ctx(), from, ev, &fresh) want, isOffered := offered[ev] switch { case isOffered && err != nil: - t.Errorf("%v/%v: offered but Fire refused: %v", from, ev, err) + t.Errorf("%v/%v: offered but Next refused: %v", from, ev, err) case isOffered && got != want: - t.Errorf("%v/%v: offered %v but Fire reached %v", from, ev, want, got) + t.Errorf("%v/%v: offered %v but Next reached %v", from, ev, want, got) case !isOffered && err == nil: - t.Errorf("%v/%v: not offered but Fire succeeded to %v", from, ev, got) + t.Errorf("%v/%v: not offered but Next succeeded to %v", from, ev, got) } } } @@ -553,7 +571,7 @@ func TestMachineIsSafeForConcurrentUse(t *testing.T) { wg.Go(func() { for range 200 { d := data{lines: 1, inStock: true} - if _, err := orders.Fire(ctx(), paid, ship, &d); err != nil { + if _, err := orders.Next(ctx(), paid, ship, &d); err != nil { t.Error(err) return } @@ -565,21 +583,21 @@ func TestMachineIsSafeForConcurrentUse(t *testing.T) { wg.Wait() } -func TestFireIsReentrant(t *testing.T) { - // No lock and no in-flight state, so an effect may fire another machine. +func TestEffectMayFireADistinctInstance(t *testing.T) { inner := statemachine.MustCompile([]row{ {From: draft, Event: cancel, To: cancelled, Do: record("inner")}, }) + innerRun := statemachine.NewInstance(inner, draft) outer := statemachine.MustCompile([]row{ {From: pending, Event: pay, To: paid, Do: func(c context.Context, d *data) error { - _, err := inner.Fire(c, draft, cancel, d) + _, err := innerRun.Fire(c, cancel, d) return err }}, }) d := data{} - got, err := outer.Fire(ctx(), pending, pay, &d) + got, err := statemachine.NewInstance(outer, pending).Fire(ctx(), pay, &d) if err != nil { - t.Fatalf("Fire: %v", err) + t.Fatalf("Instance.Fire: %v", err) } if got != paid { t.Errorf("state = %v, want paid — the outer row's To, not the nested one's", got) @@ -591,13 +609,13 @@ func TestFireIsReentrant(t *testing.T) { // --- Documented invariants --------------------------------------------------- -func TestFireIgnoresContextCancellation(t *testing.T) { - // Fire never reads ctx; only Guard and Do may honor it. +func TestNextIgnoresContextCancellation(t *testing.T) { + // Next never reads ctx; Guards may honor it. dead, stop := context.WithCancel(ctx()) stop() - got, err := orders.Fire(dead, draft, cancel, &data{}) + got, err := orders.Next(dead, draft, cancel, &data{}) if err != nil { - t.Errorf("err = %v; Fire must not check ctx itself", err) + t.Errorf("err = %v; Next must not check ctx itself", err) } if got != cancelled { t.Errorf("state = %v, want cancelled", got) @@ -619,8 +637,8 @@ func TestContextReachesGuardsAndEffects(t *testing.T) { {From: draft, Event: submit, To: pending, Guard: probe("guard"), Do: probe("do")}, }) c := context.WithValue(ctx(), ckey{}, "carried") - if _, err := m.Fire(c, draft, submit, &data{}); err != nil { - t.Fatalf("Fire: %v", err) + if _, err := statemachine.NewInstance(m, draft).Fire(c, submit, &data{}); err != nil { + t.Fatalf("Instance.Fire: %v", err) } if want := []string{"guard", "do"}; !slices.Equal(seen, want) { t.Errorf("ctx reached %v, want %v", seen, want) @@ -629,9 +647,9 @@ func TestContextReachesGuardsAndEffects(t *testing.T) { func TestNilGuardAndNilEffectAreNoOps(t *testing.T) { m := statemachine.MustCompile([]row{{From: draft, Event: submit, To: pending}}) - got, err := m.Fire(ctx(), draft, submit, &data{}) + got, err := m.Next(ctx(), draft, submit, &data{}) if got != pending || err != nil { - t.Errorf("Fire = %v, %v; want (pending, nil)", got, err) + t.Errorf("Next = %v, %v; want (pending, nil)", got, err) } } @@ -640,9 +658,9 @@ func TestSelfTransitionRunsItsEffectOnce(t *testing.T) { {From: paid, Event: ship, To: paid, Do: record("retry")}, }) d := data{} - got, err := m.Fire(ctx(), paid, ship, &d) + got, err := statemachine.NewInstance(m, paid).Fire(ctx(), ship, &d) if got != paid || err != nil { - t.Fatalf("Fire = %v, %v", got, err) + t.Fatalf("Instance.Fire = %v, %v", got, err) } if !slices.Equal(d.trace, []string{"retry"}) { t.Errorf("trace = %v, want exactly one run", d.trace) @@ -652,9 +670,9 @@ func TestSelfTransitionRunsItsEffectOnce(t *testing.T) { func TestUnknownStateHasNoAffordances(t *testing.T) { // A value loaded from storage that no row mentions: refuses, never panics. corrupt := state("who-knows") - got, err := orders.Fire(ctx(), corrupt, ship, &data{}) + got, err := orders.Next(ctx(), corrupt, ship, &data{}) if got != corrupt || !errors.Is(err, statemachine.ErrNotPermitted) { - t.Errorf("Fire = %v, %v; want (%v, ErrNotPermitted)", got, err, corrupt) + t.Errorf("Next = %v, %v; want (%v, ErrNotPermitted)", got, err, corrupt) } if n := len(maps.Collect(orders.Permitted(ctx(), corrupt, &data{}))); n != 0 { t.Errorf("Permitted yielded %d events for an unknown state", n) @@ -662,7 +680,7 @@ func TestUnknownStateHasNoAffordances(t *testing.T) { } func TestRefusalMessageNamesTheEventAndState(t *testing.T) { - _, err := orders.Fire(ctx(), shipped, ship, &data{}) + _, err := orders.Next(ctx(), shipped, ship, &data{}) for _, want := range []string{"statemachine:", string(ship), string(shipped)} { if !strings.Contains(err.Error(), want) { t.Errorf("message %q does not mention %q", err, want) @@ -672,7 +690,7 @@ func TestRefusalMessageNamesTheEventAndState(t *testing.T) { // --- Benchmarks -------------------------------------------------------------- -// benchMachine has no recording effects, so the benchmarks below measure Fire +// benchMachine has no recording effects, so the benchmarks below measure Next // rather than a test fixture's growing trace slice. var benchMachine = statemachine.MustCompile([]row{ {From: paid, Event: ship, To: shipped, Guard: inStock}, @@ -681,36 +699,36 @@ var benchMachine = statemachine.MustCompile([]row{ }) // The accepted path: a map lookup, one guard, and a return. -func BenchmarkFireAccepted(b *testing.B) { +func BenchmarkNextAccepted(b *testing.B) { d := data{lines: 1, inStock: true} b.ReportAllocs() for b.Loop() { - _, _ = benchMachine.Fire(ctx(), paid, ship, &d) + _, _ = benchMachine.Next(ctx(), paid, ship, &d) } } // The default arm: the first guard declines, so a reason is collected and // discarded before the second row wins. -func BenchmarkFireDefaultArm(b *testing.B) { +func BenchmarkNextDefaultArm(b *testing.B) { d := data{} b.ReportAllocs() for b.Loop() { - _, _ = benchMachine.Fire(ctx(), paid, ship, &d) + _, _ = benchMachine.Next(ctx(), paid, ship, &d) } } // The refused path, which the docs steer callers toward instead of // check-then-act: it must not be expensive. -func BenchmarkFireRefused(b *testing.B) { +func BenchmarkNextRefused(b *testing.B) { d := data{} b.ReportAllocs() for b.Loop() { - _, _ = benchMachine.Fire(ctx(), shipped, pay, &d) + _, _ = benchMachine.Next(ctx(), shipped, pay, &d) } } func BenchmarkRefusalErrorsIs(b *testing.B) { - _, err := orders.Fire(ctx(), draft, submit, &data{}) + _, err := orders.Next(ctx(), draft, submit, &data{}) b.ReportAllocs() for b.Loop() { if !errors.Is(err, statemachine.ErrNotPermitted) { @@ -757,8 +775,8 @@ func TestStrictComparableTypesAndDynamicValuesNeverPanic(t *testing.T) { t.Fatalf("nested interface Compile = %v", err) } var zero statemachine.Machine[any, any, struct{}] - if _, err := zero.Fire(context.Background(), any([]int{1}), "start", struct{}{}); !errors.Is(err, statemachine.ErrInvalidKey) { - t.Fatalf("zero Fire = %v", err) + if _, err := zero.Next(context.Background(), any([]int{1}), "start", struct{}{}); !errors.Is(err, statemachine.ErrInvalidKey) { + t.Fatalf("zero Next = %v", err) } } diff --git a/todo.md b/todo.md new file mode 100644 index 0000000..7b14059 --- /dev/null +++ b/todo.md @@ -0,0 +1,37 @@ +# Next steps + +## Production transactional Store + +The `persist.Store` interface already defines an adapter-owned unit of work, +and `persist.FuncStore` plus the SQL example show how to pass `*sql.Tx` through +to a transition effect. The repository does not currently ship a production +database Store. `MemoryStore` provides optimistic in-memory state updates only; +it cannot roll back arbitrary effects. + +- [ ] Decide which production database adapters or SQL dialects the library + will support, or explicitly keep the seam application-owned. +- [ ] Implement a production Store that begins a transaction, loads state and + revision, invokes the transition exactly once, conditionally writes the new + state, and commits or rolls back. +- [ ] Ensure panics, transition errors, and version conflicts roll back the + database transaction without retrying the transition effect. +- [ ] Add an outbox writer that uses the same transaction as the state update + and enforces a stable command/idempotency key with a unique constraint. +- [ ] Treat cancellation and commit errors as potentially ambiguous outcomes; + require reload/reconciliation before an application retry. +- [ ] Require idempotent outbox relays and consumers because delivery may still + occur more than once. +- [ ] Document that direct network or hardware actions inside `Do` are not + transactional. Use an outbox for network delivery and the supervised + Issue–Verify protocol for physical actions. +- [ ] Add database integration and fault-injection tests covering atomic + state/outbox commit, rollback, conflicts, ambiguous commit outcomes, and + callback-at-most-once behavior. + +### Acceptance criteria + +- State and its outbox record become visible together or not at all. +- A failed or conflicting transaction leaves neither change committed. +- The Store never automatically repeats a transition effect. +- An unknown commit outcome is reported as unknown and reconciled by reload, + never treated as proof that nothing happened.