fix: roll back configuration on a failed Fire so state stays transactional

Signed-off-by: Joshua Temple <joshua.temple@stablekernel.com>

Author: joshua-temple

state/CHANGELOG.md

@@ -21,6 +21,17 @@ ready to freeze on sign-off. The `analysis`, `evolution`, `conformance`, and
 
 ### Fixed
 
+- A failed `Fire` is now fully transactional on the instance's internal state: when
+  an action or assign errors or panics partway through a macrostep, the active
+  configuration, current leaf, context, and recorded history all roll back to their
+  pre-Fire values, `FireResult.NewState` reports the original state rather than the
+  abandoned target, and a snapshot taken afterward is identical to one never Fired.
+  This extends the existing effect-emission transactionality to the configuration —
+  previously a failed `Fire` left the configuration half-advanced. The successful
+  path is unchanged: the configuration still advances in place during the macrostep
+  so entry actions, the done cascade, and the run-to-completion loop observe it. A
+  partial parallel-region commit (an earlier region committing before a later region
+  fails) rolls back with the rest of the macrostep.
 - A `Raise` declared on a region-internal transition is now delivered instead of
   silently dropped (kernel parallel-region commit path).
 - A region-internal transition targeting a same-region history pseudostate now

state/doc.go

@@ -38,13 +38,19 @@
 // at dispatch, never silently dropped. Effects stay data the host applies — the
 // kernel never executes them.
 //
-// Effect emission is transactional with respect to the step's outcome: a Fire's
-// effects are emitted (returned in FireResult.Effects) only on a fully-successful
-// Fire. A Fire that fails partway through its cascade — an action or assign that
-// errors or panics — returns the error with NO effects, so a host replaying a
-// failed Fire cannot double-apply the effects that ran before the error. (The
-// configuration is not rolled back on failure; only effect emission is made
-// transactional, which is what a replay observes.)
+// A Fire is transactional with respect to the step's outcome: it commits to the
+// instance's internal state only on a fully-successful step. A Fire that fails
+// partway through its cascade — an action or assign that errors or panics —
+// returns the error and is a no-op on the instance's persisted state. The active
+// configuration, current leaf, context, and recorded history all roll back to their
+// pre-Fire values, and no effects are emitted (returned in FireResult.Effects), so
+// a host replaying a failed Fire cannot double-apply the effects that ran before the
+// error, and FireResult.NewState reports the original state rather than the abandoned
+// target. The configuration still advances in place during a successful macrostep
+// (entry actions, the done cascade, and the run-to-completion loop observe the
+// advancing configuration); a failure discards that advance. The one thing a failed
+// Fire cannot undo is a real-world side effect a host already applied from an effect
+// emitted by an earlier, successful step — those are outside the kernel's pure state.
 //
 // # The definition IR is the spec
 //

state/fire.go

@@ -105,11 +105,69 @@ const maxMicrosteps = 10_000
 // Trace microsteps. The internal queue is local to this call, so Fire stays
 // pure: no clock, no IO.
 func (i *Instance[S, E, C]) fireCore(ctx context.Context, event E) FireResult[S] {
+	// Snapshot the live internal state a macrostep mutates before any commit runs.
+	// A failed Fire (the triggering transition or any run-to-completion sub-step)
+	// must be a no-op on the instance's persisted state: configuration, current
+	// leaf, context, and recorded history all roll back together — matching the
+	// already-transactional context and effects so a snapshot taken after a failed
+	// Fire is identical to one never Fired. The successful path is untouched: the
+	// configuration still advances in place during the macrostep (entry actions,
+	// the done cascade, and the RTC loop read the advancing configuration), and the
+	// snapshot is simply discarded on success.
+	txn := i.beginTxn()
+
 	res := i.fireOnce(ctx, event)
+	if res.Err == nil {
+		res = i.runToCompletion(ctx, res)
+	}
 	if res.Err != nil {
-		return res
+		i.rollback(txn)
+		res.NewState = i.current
 	}
-	return i.runToCompletion(ctx, res)
+	return res
+}
+
+// fireTxn captures the live, macrostep-mutable fields of an Instance so a failed
+// Fire can restore them, making a failed macrostep a no-op on the instance's
+// persisted internal state. It holds the configuration, current leaf, context,
+// recorded history maps, and the internal-event queue — every field a commit
+// writes before it can fail.
+type fireTxn[S comparable, E comparable, C any] struct {
+	current        S
+	config         []S
+	entity         C
+	historyShallow map[S]S
+	historyDeep    map[S][]S
+	raised         []E
+}
+
+// beginTxn snapshots the instance's macrostep-mutable state before a Fire runs. The
+// configuration, history maps, and queue are copied (not aliased) so an in-place
+// mutation during the macrostep cannot corrupt the saved snapshot; the context and
+// current leaf are values. It allocates only the small per-macrostep snapshot and
+// never touches a clock or IO, so Fire stays pure.
+func (i *Instance[S, E, C]) beginTxn() fireTxn[S, E, C] {
+	return fireTxn[S, E, C]{
+		current:        i.current,
+		config:         append([]S(nil), i.config...),
+		entity:         i.entity,
+		historyShallow: copyMap(i.historyShallow),
+		historyDeep:    copyLeafMap(i.historyDeep),
+		raised:         append([]E(nil), i.raised...),
+	}
+}
+
+// rollback restores the instance to the state captured by beginTxn, discarding every
+// configuration, context, and history mutation a failed macrostep applied. Effects
+// are already suppressed on the failure path (the NTE contract), so after rollback
+// the instance is byte-for-byte identical to its pre-Fire state.
+func (i *Instance[S, E, C]) rollback(txn fireTxn[S, E, C]) {
+	i.current = txn.current
+	i.config = txn.config
+	i.entity = txn.entity
+	i.historyShallow = txn.historyShallow
+	i.historyDeep = txn.historyDeep
+	i.raised = txn.raised
 }
 
 // runToCompletion settles the macrostep after the triggering transition: it
@@ -643,10 +701,13 @@ func forbids[S comparable, E comparable, C any](s *State[S, E, C], event E) bool
 	return false
 }
 
-// commit advances the configuration (before running actions, per the locked
-// decision) and runs the exit cascade, the transition's bound actions, and the
-// entry cascade — building effects and recording the trace. matchedAt is the
-// ancestor whose transition fired (equal to the source leaf for a flat machine).
+// commit advances the configuration in place (before running actions, so entry
+// actions and the done cascade observe the advancing configuration) and runs the
+// exit cascade, the transition's bound actions, and the entry cascade — building
+// effects and recording the trace. matchedAt is the ancestor whose transition fired
+// (equal to the source leaf for a flat machine). A failure returns early with the
+// configuration left advanced; fireCore's transaction rolls it back so the failed
+// Fire is a no-op on the instance's persisted state.
 func (i *Instance[S, E, C]) commit(
 	ctx context.Context,
 	t *Transition[S, E, C],
@@ -678,11 +739,12 @@ func (i *Instance[S, E, C]) commit(
 		effects, errName, err := i.runActions(t.Effects, cur, &tr)
 		if err != nil {
 			tr.Outcome = OutcomeEffectError
-			// Effects are emitted only on a fully-successful Fire (the NTE
+			// Effects are emitted only on a fully-successful Fire (the
 			// transactionality contract): a failed Fire returns no partial effects so
 			// a host replaying it cannot double-apply the ones that ran before the
-			// error. Config is intentionally not rolled back (out of scope); only the
-			// effect emission is transactional.
+			// error. An internal transition leaves the configuration unchanged, so
+			// only the context fold is at stake here; fireCore's transaction rolls it
+			// back along with the rest of the macrostep on failure.
 			return FireResult[S]{
 				NewState: i.current, Effects: nil, Trace: tr,
 				Err: &ActionFailedError{

state/fire_transactional_test.go

@@ -0,0 +1,184 @@
+package state_test
+
+import (
+	"context"
+	"errors"
+	"reflect"
+	"testing"
+
+	"github.com/stablekernel/crucible/state"
+)
+
+// txnCtx is the value context for the failed-Fire transactionality regression. It
+// carries a balance an entry assign would fold so a rolled-back Fire is provably a
+// context no-op, and a JSON-marshalable shape so the snapshot round-trip works with
+// the default codec.
+type txnCtx struct {
+	Balance int `json:"balance"`
+}
+
+// TestFire_FailedEntryAction_RollsBackConfiguration pins the full-transactionality
+// contract: when a transition's ENTRY action fails, the failed Fire is a no-op on
+// the instance's persisted internal state. Config, current state, and context are
+// all left at their pre-Fire values, FireResult.NewState reports the ORIGINAL state
+// (not the abandoned target), and a snapshot taken afterward round-trips to an
+// instance identical to one that never Fired.
+//
+// The machine moves off -> target on "go"; target's OnEntry action errors. Before
+// the fix the kernel advanced i.current/i.config to "target" and reported
+// NewState=target while leaving the context rolled back — a split. The fix rolls
+// the configuration back together with the (already transactional) context and
+// effects.
+func TestFire_FailedEntryAction_RollsBackConfiguration(t *testing.T) {
+	boom := errors.New("entry boom")
+	bump := func(in state.AssignCtx[txnCtx]) txnCtx { c := in.Entity; c.Balance += 100; return c }
+	fail := func(state.ActionCtx[txnCtx]) (state.Effect, error) { return nil, boom }
+
+	build := func() *state.Machine[string, string, txnCtx] {
+		return state.Forge[string, string, txnCtx]("txn").
+			Action("explode", fail).
+			Reducer("bump", bump).
+			State("off").
+			Transition("off").On("go").GoTo("target").
+			State("target").OnEntry("explode").OnEntryAssign("bump").
+			Initial("off").
+			CurrentStateFn(func(txnCtx) string { return "off" }).
+			Quench()
+	}
+
+	m := build()
+	ctx := context.Background()
+
+	// A never-Fired control instance and its snapshot: the post-failure instance must
+	// be indistinguishable from this one.
+	control := m.Cast(txnCtx{Balance: 1}, state.WithInitialState("off"))
+	wantSnap := control.Snapshot()
+
+	inst := m.Cast(txnCtx{Balance: 1}, state.WithInitialState("off"))
+
+	res := inst.Fire(ctx, "go")
+	if res.Err == nil {
+		t.Fatalf("expected the failed entry action to error, got nil (state=%v)", res.NewState)
+	}
+	if !errors.Is(res.Err, boom) {
+		t.Fatalf("err = %v, want it to wrap %v", res.Err, boom)
+	}
+
+	// (b) FireResult reports the actual resulting (original) state.
+	if res.NewState != "off" {
+		t.Fatalf("NewState = %v on a failed Fire; want the original state off (no half-advanced config)", res.NewState)
+	}
+	if got := inst.Current(); got != "off" {
+		t.Fatalf("instance current = %v after a failed Fire; want off", got)
+	}
+	if cfg := inst.Configuration(); len(cfg) != 1 || cfg[0] != "off" {
+		t.Fatalf("configuration = %v after a failed Fire; want [off]", cfg)
+	}
+
+	// (c) Context unchanged: the entry assign that follows the failed action never
+	// commits, and there is no separate rollback to verify since context was already
+	// transactional — but assert it to lock the whole-instance no-op.
+	if got := inst.Entity().Balance; got != 1 {
+		t.Fatalf("context balance = %d after a failed Fire; want 1 (unchanged)", got)
+	}
+
+	// (d) Snapshot the post-failure instance: it must equal a snapshot of the
+	// never-Fired control, so nothing split was persisted.
+	gotSnap := inst.Snapshot()
+	if !reflect.DeepEqual(gotSnap, wantSnap) {
+		t.Fatalf("post-failure snapshot != never-Fired snapshot:\n got = %+v\nwant = %+v", gotSnap, wantSnap)
+	}
+
+	// The control instance can still Fire cleanly through a non-failing path, proving
+	// the rolled-back instance is genuinely re-fireable from its original state.
+	clean := build0(t)
+	if r := clean.Fire(ctx, "go"); r.Err != nil {
+		t.Fatalf("clean machine should advance: %v", r.Err)
+	} else if r.NewState != "target" {
+		t.Fatalf("clean machine NewState = %v, want target", r.NewState)
+	}
+}
+
+// build0 casts a no-fail variant of the txn machine for the re-fireability control.
+func build0(t *testing.T) *state.Instance[string, string, txnCtx] {
+	t.Helper()
+	m := state.Forge[string, string, txnCtx]("txn-ok").
+		State("off").
+		Transition("off").On("go").GoTo("target").
+		State("target").
+		Initial("off").
+		CurrentStateFn(func(txnCtx) string { return "off" }).
+		Quench()
+	return m.Cast(txnCtx{Balance: 1}, state.WithInitialState("off"))
+}
+
+// TestFire_FailedRegionEntry_RollsBackEarlierRegion covers the parallel-region
+// split: regions fire in declaration order within one macrostep, each committing its
+// own config/context fold to the live instance. When a LATER region's entry action
+// fails, an EARLIER region's already-committed transition must roll back too — a
+// partial-region commit is the parallel analog of the half-advanced config.
+//
+// r1 takes r1a -> r1b on "go" (committing a config swap and a context fold); r2 then
+// takes r2a -> r2b on "go" whose entry action errors. The whole Fire must be a no-op:
+// r1 stays at r1a and the r1 fold is discarded.
+func TestFire_FailedRegionEntry_RollsBackEarlierRegion(t *testing.T) {
+	boom := errors.New("region entry boom")
+	bump := func(in state.AssignCtx[txnCtx]) txnCtx { c := in.Entity; c.Balance += 10; return c }
+	fail := func(state.ActionCtx[txnCtx]) (state.Effect, error) { return nil, boom }
+
+	m := state.Forge[string, string, txnCtx]("txn-par").
+		Action("explode", fail).
+		Reducer("bump", bump).
+		SuperState("live").
+		Region("r1").
+		Initial("r1a").
+		SubState("r1a").On("go").GoTo("r1b").Assign("bump").
+		SubState("r1b").
+		EndRegion().
+		Region("r2").
+		Initial("r2a").
+		SubState("r2a").On("go").GoTo("r2b").
+		SubState("r2b").OnEntry("explode").
+		EndRegion().
+		EndSuperState().
+		Initial("live").
+		Quench()
+
+	ctx := context.Background()
+	control := m.Cast(txnCtx{Balance: 1}, state.WithInitialState("live"))
+	wantSnap := control.Snapshot()
+
+	inst := m.Cast(txnCtx{Balance: 1}, state.WithInitialState("live"))
+	res := inst.Fire(ctx, "go")
+	if res.Err == nil {
+		t.Fatalf("expected the failed region entry to error, got nil (state=%v)", res.NewState)
+	}
+	if !errors.Is(res.Err, boom) {
+		t.Fatalf("err = %v, want it to wrap %v", res.Err, boom)
+	}
+
+	// r1 must not have committed: configuration still holds r1a (and r2a), context
+	// fold discarded.
+	cfg := inst.Configuration()
+	if !containsLeaf(cfg, "r1a") || containsLeaf(cfg, "r1b") {
+		t.Fatalf("configuration = %v after a failed parallel Fire; r1 must stay at r1a", cfg)
+	}
+	if got := inst.Entity().Balance; got != 1 {
+		t.Fatalf("context balance = %d after a failed parallel Fire; want 1 (r1 fold rolled back)", got)
+	}
+
+	gotSnap := inst.Snapshot()
+	if !reflect.DeepEqual(gotSnap, wantSnap) {
+		t.Fatalf("post-failure parallel snapshot != never-Fired snapshot:\n got = %+v\nwant = %+v", gotSnap, wantSnap)
+	}
+}
+
+// containsLeaf reports whether leaf is present in cfg.
+func containsLeaf(cfg []string, leaf string) bool {
+	for _, s := range cfg {
+		if s == leaf {
+			return true
+		}
+	}
+	return false
+}

state/hardening_test.go

@@ -148,10 +148,13 @@ func TestSelfTransition(t *testing.T) {
 	}
 }
 
-// TestActionError_AdvancesStateRecordsEffectError asserts the locked decision:
-// state advances before actions run; a failing action records OutcomeEffectError
-// and the typed *ActionFailedError, with the state already advanced.
-func TestActionError_AdvancesStateRecordsEffectError(t *testing.T) {
+// TestActionError_RollsBackRecordsEffectError asserts that a failing transition
+// action records OutcomeEffectError and the typed *ActionFailedError, and that the
+// failed Fire is transactional: the configuration rolls back to the source state
+// rather than leaking the half-advanced target. The configuration still advances in
+// place during the macrostep (so entry actions and the done cascade observe it), but
+// a failure discards that advance, so NewState reports the original state.
+func TestActionError_RollsBackRecordsEffectError(t *testing.T) {
 	type s = int
 	type e = int
 	const a s = 0
@@ -178,8 +181,8 @@ func TestActionError_AdvancesStateRecordsEffectError(t *testing.T) {
 	if !errors.Is(res.Err, boom) {
 		t.Fatalf("err does not unwrap to boom: %v", res.Err)
 	}
-	if res.NewState != b {
-		t.Fatalf("state = %v, want b (advanced before action)", res.NewState)
+	if res.NewState != a {
+		t.Fatalf("state = %v, want a (failed Fire rolls back the advanced configuration)", res.NewState)
 	}
 	if res.Trace.Outcome != state.OutcomeEffectError {
 		t.Fatalf("outcome = %v, want OutcomeEffectError", res.Trace.Outcome)