From a52e11361d1b9ff3b353e09fc794cc501ab2acae Mon Sep 17 00:00:00 2001 From: Remylus Losius Date: Sat, 19 Sep 2026 08:16:46 -0400 Subject: [PATCH 1/3] fix(engine): report an unconfirmed host when an apply step fails A failed remediation step was reported as a successful restoration. The engine does not reverse a step whose own Apply failed, so that step produces no RollbackResult, an empty rollback set counted as clean, and the verdict came out rolled_back with HostUnchanged true. The host could still carry whatever the handler changed before it failed. rollbackStatus now gives an unresolved failed Apply precedence: either failure shape, a returned error or a Success:false result, yields RollbackFailed with HostUnchanged false, whether or not the reversible steps rolled back cleanly. HostUnchanged needs no separate predicate, since RollbackFailed is already outside the set that reports it true. Stranded marking widens to match. It ran only on PartiallyApplied, so a successful non-capturable step lost its flag when a failed Apply changed the verdict. The gate is now the two failure statuses, not the transactional flag alone, because a gate on the flag alone would mark every successful non-capturable step on the commit path. The Success test in the gate is load-bearing: OpenWatch renders a stranded step as "it succeeded before a later step failed", so the flag must never land on the step that failed. The marking stays above envelope construction, inside the signature, as the signing work requires. Three existing acceptance criteria stated the old contract and are corrected, not merely re-tested: - engine-transaction AC-02 said Run returns RolledBack when any apply step fails. That WAS the defect, written down as critical. - AC-05 said a transactional:false failure is PartiallyApplied, with no qualifier for a failed apply. - AC-17 said a clean reversal yields RolledBack, which now also requires that no invoked Apply failed. - C-11 made the terminal status a verdict over RollbackResults alone; it is a verdict over the apply results too. - engine-audit-emission AC-02 named rolled_back as the terminal phase for a failed apply. AC-26 states the new contract. Four tests reached the rollback path through a failed Apply as a convenience; their fixtures now fail validation instead, so each keeps testing its own subject. That matters most for AC-16: driven by a failed apply it would have reported RollbackFailed for the wrong reason and could no longer tell an unclean rollback from an unresolved one. OpenWatch confirmed no consumer change is required (OW-KN-048). Their OutcomeOf already maps rollback_failed to StatusPartiallyApplied, so the affected remediations move from "Reverted, host unchanged" to "Partially applied" with an inspection prompt. Failure-mode analysis --------------------- What could this change do wrong in production? An Apply that failed without changing anything will also report restoration as unconfirmed, raising an inspection prompt and a notification. That is intentional: the engine cannot establish whether a mutation occurred, and reporting an unconfirmed host is the safe direction. The cost is alert fatigue, and a handler-reported "no effect" signal that would narrow it is deferred, not part of this fix. Incorrect precedence is the other risk: it could hide a rollback failure, misclassify a successful transaction, or mark a failed step stranded. Each has a test, and each test is proven to bite by a targeted mutation, across both Run and ExecutePlan. Is the captured state sufficient to fully restore the system on rollback? Unaffected. This corrects reporting only. It does not reverse failed steps, establish capture completeness, or improve rollback. Residual mutations may remain on the host; saying so is the point of the change. What edge case is this not safe for, and is it documented and gated? The engine still cannot distinguish a failed Apply that mutated from one that did not, so the status is conservative rather than precise. Recapture remains advisory and does not determine the verdict; the godoc said otherwise and is corrected. rollback_results is still unauthenticated in the v1 envelope, tracked separately. Verification is offline only: no live host ran this. --- CHANGELOG.md | 18 ++ api/transaction.go | 43 +-- internal/engine/apply_success_test.go | 276 +++++++++++++++++++ internal/engine/audit_emit_test.go | 9 +- internal/engine/commit.go | 11 +- internal/engine/commit_durability_test.go | 26 +- internal/engine/engine.go | 22 ++ internal/engine/engine_test.go | 37 ++- internal/engine/failed_apply_test.go | 306 ++++++++++++++++++++++ internal/engine/progress_emission_test.go | 14 +- internal/engine/recapture_test.go | 9 +- specs/engine/audit-emission.spec.yaml | 8 +- specs/engine/transaction.spec.yaml | 72 +++-- 13 files changed, 780 insertions(+), 71 deletions(-) create mode 100644 internal/engine/apply_success_test.go create mode 100644 internal/engine/failed_apply_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index f986d4e4..cec63698 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -80,6 +80,24 @@ any pair). ### Fixed +- **A failed remediation step no longer reports the host as restored.** When a + remediation handler failed partway, Kensa reported `rolled_back` with + `HostUnchanged: true`. The engine does not reverse a step whose own apply + failed, and it cannot know what that step changed before failing, so the + host could still carry a partial change while the record said it had been + restored. Those transactions now report `rollback_failed` with + `HostUnchanged: false`, which is the status that means the host is in an + unconfirmed state. Steps that succeeded and could not be reversed keep their + stranded marking, and the flag is never set on the step that failed. + Transactions whose steps all applied are unaffected: a clean reversal after + a failed validation still reports `rolled_back`. + + In OpenWatch this moves the affected remediations from "Reverted, host + unchanged" to "Partially applied" with an inspection prompt. That is the + point: those hosts may need a look. It also means a handler that failed + without changing anything now reports an unconfirmed restoration, because + the engine has no evidence either way. + - **Evidence signature verification is fixed** for partially applied transactions with stranded steps, and for returned evidence after a terminal persistence failure. The engine settles every signed field before diff --git a/api/transaction.go b/api/transaction.go index 8b30ac2a..1917199b 100644 --- a/api/transaction.go +++ b/api/transaction.go @@ -19,15 +19,18 @@ const ( // evidence envelope is signed and persisted. StatusCommitted TransactionStatus = "committed" - // StatusRolledBack indicates apply or validate failed and every - // applied capturable step was reversed using captured pre-state. - // The host is in the exact pre-change state. + // StatusRolledBack indicates apply or validate failed, no invoked + // Apply reported failure, and every applied capturable step was + // reversed using captured pre-state. The host is in the exact + // pre-change state. StatusRolledBack TransactionStatus = "rolled_back" // StatusPartiallyApplied indicates that for a transactional:false // rule, at least one non-capturable step ran before a failure. // Those steps are not reversed; per-step Stranded flags in - // [TransactionResult.Steps] identify which ones. + // [TransactionResult.Steps] identify which ones. A transaction that + // also has a failed invoked Apply reports [StatusRollbackFailed] + // instead; the Stranded flags are set either way. StatusPartiallyApplied TransactionStatus = "partially_applied" // StatusErrored indicates the engine could not complete a phase @@ -38,14 +41,23 @@ const ( // that never mutated the host from one that left a mutation behind. StatusErrored TransactionStatus = "errored" - // StatusRollbackFailed indicates apply or validate failed and the - // engine attempted to reverse the applied steps, but the restoration - // could NOT be machine-verified as complete (a rollback step failed - // or reported PartialRestore, or the recaptured post-state did not - // match the captured pre-state). The host is in an unconfirmed state; - // per-step detail is in [TransactionResult.RollbackResults]. Produced - // by the engine when a reversed step fails or reports a partial - // restore (the verdict-computed rollback status). + // StatusRollbackFailed indicates the host is in an unconfirmed state, + // from either of two causes. A rollback handler reported failure or a + // partial restore for a step it attempted to reverse; or an invoked + // Apply reported failure, whose effects are unresolved because the + // engine does not reverse a failed step and cannot know what it + // changed before failing. Per-step detail is in + // [TransactionResult.RollbackResults] and [TransactionResult.Steps]. + // + // A transaction can reach this status with a rollback set in which + // every row succeeded: the failed step produces no RollbackResult, + // while earlier successful steps may have reversed cleanly. The + // status describes the transaction, not the rollback rows. + // + // Recapture is advisory and separate: the engine re-reads post-state + // and records, per reversed step, whether it byte-matches the captured + // pre-state. That finding is carried in the RollbackResult Detail for + // audit and does not determine this status. StatusRollbackFailed TransactionStatus = "rollback_failed" // StatusRecovered indicates an interrupted transaction (the process @@ -277,9 +289,10 @@ type StepResult struct { Success bool // Detail is human-readable per-step output suitable for logs and UI. Detail string - // Stranded is true for non-capturable steps that succeeded before - // a later failure left the rule [StatusPartiallyApplied]. Such - // steps are not reversed by rollback. + // Stranded is true for non-capturable steps that succeeded before a + // later failure, in any terminal status of a transactional:false + // transaction. Such steps are not reversed by rollback. It is never + // set on a step that failed, and never on a successful transaction. Stranded bool // Staged is true when the mechanism could not converge live state and // instead wrote a reboot-deferred persist change (audit_rule_set on an diff --git a/internal/engine/apply_success_test.go b/internal/engine/apply_success_test.go new file mode 100644 index 00000000..b4e2eb87 --- /dev/null +++ b/internal/engine/apply_success_test.go @@ -0,0 +1,276 @@ +package engine_test + +import ( + "context" + "errors" + "path/filepath" + "testing" + + "github.com/Hanalyx/kensa/api" + "github.com/Hanalyx/kensa/internal/engine" + "github.com/Hanalyx/kensa/internal/evidence" + "github.com/Hanalyx/kensa/internal/handler" + "github.com/Hanalyx/kensa/internal/store" +) + +// errCaptureInduced is the shared induced failure for these fixtures. +var errCaptureInduced = errors.New("induced failure") + +// Group B fixtures: Apply fully succeeds and validation then fails. These +// contracts are unchanged, and they are the regression risk of the +// failed-Apply work, so every assertion here passes before and after. + +// TestApplySucceeded_CleanRollbackStillRolledBack is the case the change must +// not disturb: nothing failed to apply, the reversal was reported clean, so +// the host is provably back and HostUnchanged stays true. +// @spec engine-transaction +// @ac AC-26 +func TestApplySucceeded_CleanRollbackStillRolledBack(t *testing.T) { + t.Log("// @spec engine-transaction") + t.Log("// @ac AC-26") + h := capOK("clean_rollback") + res := runTxn(t, applyFailTxn(true, "clean_rollback"), + []engine.Option{engine.WithForceValidateFail()}, h) + + if res.Status != api.StatusRolledBack { + t.Errorf("got Status=%s, want RolledBack", res.Status) + } + if !res.HostUnchanged { + t.Error("a verified rollback lost HostUnchanged=true") + } +} + +// TestApplySucceeded_StrandedStillPartiallyApplied keeps the ordinary +// stranded-step verdict intact when no Apply failed. +// @spec engine-transaction +// @ac AC-26 +func TestApplySucceeded_StrandedStillPartiallyApplied(t *testing.T) { + t.Log("// @spec engine-transaction") + t.Log("// @ac AC-26") + h := noncapOK("stranded_only") + res := runTxn(t, applyFailTxn(false, "stranded_only"), + []engine.Option{engine.WithForceValidateFail()}, h) + + if res.Status != api.StatusPartiallyApplied { + t.Errorf("got Status=%s, want PartiallyApplied", res.Status) + } + if !res.Steps[0].Stranded { + t.Error("the successful non-capturable step is not marked Stranded") + } + if res.HostUnchanged { + t.Error("a partially applied outcome reported HostUnchanged=true") + } +} + +// TestApplySucceeded_MixedCapturability covers a stranded step beside a +// cleanly reversed one. +// @spec engine-transaction +// @ac AC-26 +func TestApplySucceeded_MixedCapturability(t *testing.T) { + t.Log("// @spec engine-transaction") + t.Log("// @ac AC-26") + nc := noncapOK("mixed_nc") + c := capOK("mixed_cap") + res := runTxn(t, applyFailTxn(false, "mixed_nc", "mixed_cap"), + []engine.Option{engine.WithForceValidateFail()}, nc, c) + + if res.Status != api.StatusPartiallyApplied { + t.Errorf("got Status=%s, want PartiallyApplied", res.Status) + } + if !res.Steps[0].Stranded { + t.Error("the non-capturable step is not marked Stranded") + } + if res.Steps[1].Stranded { + t.Error("a capturable step was marked Stranded") + } +} + +// TestApplySucceeded_RollbackFailureStillRollbackFailed keeps the original +// cause of RollbackFailed working. +// @spec engine-transaction +// @ac AC-26 +func TestApplySucceeded_RollbackFailureStillRollbackFailed(t *testing.T) { + t.Log("// @spec engine-transaction") + t.Log("// @ac AC-26") + h := capOK("rb_fails") + h.RollbackResult = &api.RollbackResult{Success: false, Detail: "induced rollback failure"} + res := runTxn(t, applyFailTxn(true, "rb_fails"), + []engine.Option{engine.WithForceValidateFail()}, h) + + if res.Status != api.StatusRollbackFailed { + t.Errorf("got Status=%s, want RollbackFailed", res.Status) + } + if res.HostUnchanged { + t.Error("an unconfirmed restoration reported HostUnchanged=true") + } +} + +// TestSuccessfulTransaction_NoStrandedFlag is the preservation test for the +// status-gated marking. A successful transactional:false transaction has no +// stranded steps: nothing failed, so nothing was left behind by a failure. +// A gate that tested only Transactional would mark every successful +// non-capturable step here. +// @spec engine-transaction +// @ac AC-26 +func TestSuccessfulTransaction_NoStrandedFlag(t *testing.T) { + t.Log("// @spec engine-transaction") + t.Log("// @ac AC-26") + signer, err := evidence.Generate() + if err != nil { + t.Fatalf("generate signer: %v", err) + } + st, err := store.OpenSQLite(context.Background(), filepath.Join(t.TempDir(), "results.db")) + if err != nil { + t.Fatalf("open store: %v", err) + } + t.Cleanup(func() { _ = st.Close() }) + + res := runTxn(t, applyFailTxn(false, "success_nc"), + []engine.Option{engine.WithStore(st), engine.WithSigner(signer)}, noncapOK("success_nc")) + + if res.Status != api.StatusCommitted { + t.Fatalf("got Status=%s, want Committed", res.Status) + } + if res.Steps[0].Stranded { + t.Error("a successful transaction marked its non-capturable step Stranded") + } + if res.Envelope.ApplySteps[0].Stranded { + t.Error("the returned envelope carries a Stranded flag for a successful transaction") + } + rec, err := st.Get(context.Background(), res.TransactionID, api.WithEnvelope()) + if err != nil { + t.Fatalf("store Get: %v", err) + } + if rec.Envelope.ApplySteps[0].Stranded { + t.Error("the persisted envelope carries a Stranded flag for a successful transaction") + } +} + +// TestPreApplyFailure_HostUnchangedStaysTrue confirms the pre-apply boundary is +// untouched: a capture failure never invoked Apply, so the host is provably +// clean and the errored path still says so. +// @spec engine-transaction +// @ac AC-14 +func TestPreApplyFailure_HostUnchangedStaysTrue(t *testing.T) { + t.Log("// @spec engine-transaction") + t.Log("// @ac AC-14") + h := capOK("capture_fails") + h.CaptureErr = errCaptureInduced + res := runTxn(t, applyFailTxn(true, "capture_fails"), nil, h) + + if res.Status != api.StatusErrored { + t.Fatalf("got Status=%s, want Errored", res.Status) + } + if !res.HostUnchanged { + t.Error("a pre-apply failure lost HostUnchanged=true") + } + if h.ApplyCalls != 0 { + t.Errorf("Apply ran %d times on a pre-apply failure", h.ApplyCalls) + } +} + +// TestFailedApply_EvidenceIsSignedAndStranded proves the reporting change +// rides inside the signature: the flags are settled before the envelope is +// built, and both the returned and the reloaded records verify. +// @spec evidence-envelope +// @ac AC-03 +func TestFailedApply_EvidenceIsSignedAndStranded(t *testing.T) { + t.Log("// @spec evidence-envelope") + t.Log("// @ac AC-03") + signer, err := evidence.Generate() + if err != nil { + t.Fatalf("generate signer: %v", err) + } + st, err := store.OpenSQLite(context.Background(), filepath.Join(t.TempDir(), "results.db")) + if err != nil { + t.Fatalf("open store: %v", err) + } + t.Cleanup(func() { _ = st.Close() }) + + strand := noncapOK("ev_strand") + bad := capOK("ev_fails") + bad.ApplyErr = errCaptureInduced + res := runTxn(t, applyFailTxn(false, "ev_strand", "ev_fails"), + []engine.Option{engine.WithStore(st), engine.WithSigner(signer)}, strand, bad) + + if res.Status != api.StatusRollbackFailed { + t.Fatalf("got Status=%s, want RollbackFailed", res.Status) + } + if r, err := signer.Verify(res.Envelope); err != nil || r == nil || !r.Valid { + t.Errorf("returned envelope does not verify: %v", err) + } + if res.Envelope.Decision != api.StatusRollbackFailed { + t.Errorf("envelope decision %s, want rollback_failed", res.Envelope.Decision) + } + if !res.Envelope.ApplySteps[0].Stranded { + t.Error("the signed evidence lost the Stranded flag") + } + if res.Envelope.ApplySteps[1].Stranded { + t.Error("the signed evidence marks the FAILED step Stranded") + } + rec, err := st.Get(context.Background(), res.TransactionID, api.WithEnvelope()) + if err != nil { + t.Fatalf("store Get: %v", err) + } + if r, err := signer.Verify(rec.Envelope); err != nil || r == nil || !r.Valid { + t.Errorf("reloaded envelope does not verify: %v", err) + } + if !rec.Envelope.ApplySteps[0].Stranded { + t.Error("the reloaded evidence lost the Stranded flag") + } +} + +// TestFailedApply_PlanPathClassifiesTheSame exercises the second entry point. +// rollbackStatus is shared by Run and ExecutePlan, and a fix applied to one +// path only would leave the other reporting the old verdict. +// @spec engine-transaction +// @ac AC-26 +func TestFailedApply_PlanPathClassifiesTheSame(t *testing.T) { + t.Log("// @spec engine-transaction") + t.Log("// @ac AC-26") + strand := noncapOK("plan_strand") + bad := capOK("plan_fails") + bad.ApplyErr = errCaptureInduced + + r := handler.NewRegistry() + r.Register(strand) + r.Register(bad) + e := engine.New(engine.WithRegistry(r)) + + rule := &api.Rule{ + ID: "plan-failed-apply", + Title: "plan path failed apply", + Transactional: false, + Implementations: []api.Implementation{ + { + Default: true, + Remediation: api.Remediation{ + Steps: []api.RemediationStep{ + {Mechanism: "plan_strand"}, + {Mechanism: "plan_fails"}, + }, + }, + }, + }, + } + plan, err := e.PlanTransaction(context.Background(), engine.NewFakeTransport(), rule) + if err != nil { + t.Fatalf("PlanTransaction: %v", err) + } + res, err := e.ExecutePlan(context.Background(), engine.NewFakeTransport(), plan) + if err != nil { + t.Fatalf("ExecutePlan: %v", err) + } + if res.Status != api.StatusRollbackFailed { + t.Errorf("plan path got Status=%s, want RollbackFailed", res.Status) + } + if res.HostUnchanged { + t.Error("plan path reported HostUnchanged=true after a failed Apply") + } + if !res.Steps[0].Stranded { + t.Error("plan path lost the Stranded flag") + } + if res.Steps[1].Stranded { + t.Error("plan path marked the FAILED step Stranded") + } +} diff --git a/internal/engine/audit_emit_test.go b/internal/engine/audit_emit_test.go index 4dd4ccb4..504c17f7 100644 --- a/internal/engine/audit_emit_test.go +++ b/internal/engine/audit_emit_test.go @@ -78,10 +78,13 @@ func TestEmit_RolledBackSequence(t *testing.T) { if err != nil { t.Fatalf("Run: %v", err) } - if res.Status != api.StatusRolledBack { - t.Fatalf("status = %s, want rolled_back", res.Status) + // A failed apply leaves the host unconfirmed, so the terminal phase is + // rollback_failed. The property this test exists for is unchanged: + // there is no validate phase, because apply did not succeed. + if res.Status != api.StatusRollbackFailed { + t.Fatalf("status = %s, want rollback_failed", res.Status) } - want := []string{"started", "capture", "apply", "rolled_back"} + want := []string{"started", "capture", "apply", "rollback_failed"} if !reflect.DeepEqual(em.phases, want) { t.Errorf("emitted phases = %v, want %v", em.phases, want) } diff --git a/internal/engine/commit.go b/internal/engine/commit.go index 1ad50a61..de995d29 100644 --- a/internal/engine/commit.go +++ b/internal/engine/commit.go @@ -60,7 +60,16 @@ func (e *Engine) finalize( // of the signed payload, so setting it after Sign rewrote signed bytes // through the array the envelope shared with the result and left both // the returned and the persisted evidence unverifiable. - if status == api.StatusPartiallyApplied { + // + // The status test is what keeps this honest. A successful transaction + // strands nothing, and a failed one strands its non-capturable work + // whether the verdict came out PartiallyApplied or RollbackFailed. The + // Success test is load-bearing too: consumers read the flag as "this step + // succeeded and was not reversed", so it must never land on a step that + // failed. + if !txn.Transactional && + (status == api.StatusPartiallyApplied || + status == api.StatusRollbackFailed) { for i := range steps { if steps[i].Success && !steps[i].Capturable { steps[i].Stranded = true diff --git a/internal/engine/commit_durability_test.go b/internal/engine/commit_durability_test.go index 2915e749..0a41f389 100644 --- a/internal/engine/commit_durability_test.go +++ b/internal/engine/commit_durability_test.go @@ -197,19 +197,22 @@ func TestEngine_AC14_HostUnchangedPredicate(t *testing.T) { func TestEngine_AC15_RollbackResultsOnResultAndEnvelope(t *testing.T) { t.Log("// @spec engine-transaction") t.Log("// @ac AC-15") - // Two steps: step 0 applies, step 1 fails → step 0 is rolled back, - // producing a RollbackResult that must reach both surfaces. + // Two steps apply, validation fails, both are rolled back, producing + // RollbackResults that must reach both surfaces. h0 := &engine.FakeHandler{ HandlerName: "s0", IsCapturable: true, RollbackResult: &api.RollbackResult{Success: true, Detail: "s0 restored"}, } h1 := &engine.FakeHandler{ - HandlerName: "s1", - IsCapturable: true, - ApplyErr: errors.New("induced apply failure"), + HandlerName: "s1", + IsCapturable: true, + RollbackResult: &api.RollbackResult{Success: true, Detail: "s1 restored"}, } - e := durabilityEngine(t, nil, nil, h0, h1) + r := handler.NewRegistry() + r.Register(h0) + r.Register(h1) + e := engine.New(engine.WithRegistry(r), engine.WithForceValidateFail()) txn := &api.Transaction{ ID: uuid.New(), @@ -248,9 +251,16 @@ func twoStepRollbackTxn(t *testing.T, step0Rollback *api.RollbackResult) (*engin h1 := &engine.FakeHandler{ HandlerName: "rb_s1", IsCapturable: true, - ApplyErr: errors.New("induced apply failure"), } - e := durabilityEngine(t, nil, nil, h0, h1) + // Both steps apply cleanly and validation then fails, so the rollback + // verdict is decided by the rollback results alone. Reaching this path + // through a FAILED apply would make every outcome here RollbackFailed + // for that reason instead, and AC-16 could no longer tell an unclean + // rollback from an unresolved apply. + r := handler.NewRegistry() + r.Register(h0) + r.Register(h1) + e := engine.New(engine.WithRegistry(r), engine.WithForceValidateFail()) txn := &api.Transaction{ ID: uuid.New(), RuleID: "test-rule", diff --git a/internal/engine/engine.go b/internal/engine/engine.go index 1f7d767f..51bd120b 100644 --- a/internal/engine/engine.go +++ b/internal/engine/engine.go @@ -468,6 +468,15 @@ func anyStaged(rs []api.StepResult) bool { // rollback that left stranded non-capturable steps (transactional:false) is // PartiallyApplied; an otherwise-clean rollback is RolledBack. func rollbackStatus(rb []api.RollbackResult, txn *api.Transaction, applyResults []api.StepResult) api.TransactionStatus { + // An invoked Apply that reported failure leaves its effects unresolved. + // The engine does not reverse a failed step (see rollback), and it cannot + // know what the handler changed before it failed, so the host is + // unconfirmed — which is what RollbackFailed means. This outranks the + // stranded-step verdict because it is the more severe claim; the stranded + // flags survive on the steps either way. + if hasFailedApply(applyResults) { + return api.StatusRollbackFailed + } if !rollbackClean(rb) { return api.StatusRollbackFailed } @@ -477,6 +486,19 @@ func rollbackStatus(rb []api.RollbackResult, txn *api.Transaction, applyResults return api.StatusRolledBack } +// hasFailedApply reports whether any step's handler was invoked and reported +// failure, by returning an error or a Success:false result. Both shapes land +// here identically (see applySteps). A step that never ran produces no +// StepResult at all, so presence in this slice means the handler was invoked. +func hasFailedApply(steps []api.StepResult) bool { + for _, s := range steps { + if !s.Success { + return true + } + } + return false +} + // rollbackClean reports whether every reversed step restored cleanly — // succeeded and reported no partial restore. An empty set is clean: there // was nothing to reverse (e.g. the first step failed to apply). diff --git a/internal/engine/engine_test.go b/internal/engine/engine_test.go index df19f2d4..e2f2adbb 100644 --- a/internal/engine/engine_test.go +++ b/internal/engine/engine_test.go @@ -63,9 +63,12 @@ func TestEngine_AC01_CommittedOnFullSuccess(t *testing.T) { // @spec engine-transaction // @ac AC-02 -func TestEngine_AC02_RolledBackOnApplyFailure(t *testing.T) { +func TestEngine_AC02_RollbackFailedOnApplyFailure(t *testing.T) { t.Log("// @spec engine-transaction") t.Log("// @ac AC-02") + // A failed step is not reversed, so its effects are unresolved and the + // host is unconfirmed. This asserted RolledBack with a rolled-back + // timestamp until the failed-apply reporting fix. h := &engine.FakeHandler{ HandlerName: "fake_apply_fails", IsCapturable: true, @@ -77,11 +80,14 @@ func TestEngine_AC02_RolledBackOnApplyFailure(t *testing.T) { if err != nil { t.Fatalf("Run returned err: %v", err) } - if res.Status != api.StatusRolledBack { - t.Errorf("got Status=%s, want RolledBack", res.Status) + if res.Status != api.StatusRollbackFailed { + t.Errorf("got Status=%s, want RollbackFailed", res.Status) } - if res.RolledBackAt == nil { - t.Error("expected RolledBackAt to be set on rolled-back transaction") + if res.RolledBackAt != nil { + t.Error("RolledBackAt is set on a transaction that did not roll back cleanly") + } + if res.HostUnchanged { + t.Error("HostUnchanged is true after an invoked Apply reported failure") } } @@ -133,8 +139,11 @@ func TestEngine_AC02_RollbackInReverseOrder(t *testing.T) { if err != nil { t.Fatalf("Run err: %v", err) } - if res.Status != api.StatusRolledBack { - t.Fatalf("got Status=%s, want RolledBack", res.Status) + // The third step's Apply failed, so the transaction is RollbackFailed. + // The reverse-order property this test exists for is unaffected: the + // steps that DID apply are still reversed, last applied first. + if res.Status != api.StatusRollbackFailed { + t.Fatalf("got Status=%s, want RollbackFailed", res.Status) } // Step 0 and 1 applied successfully; both should have rolled back. if h0.RollbackCalls != 1 { @@ -163,18 +172,20 @@ func TestEngine_AC05_PartiallyAppliedForNonCapturableSuccess(t *testing.T) { t.Log("// @spec engine-transaction") t.Log("// @ac AC-05") // Two steps. Step 0 is non-capturable and succeeds; step 1 is - // capturable and fails. Rule is transactional:false (it must be, - // because it has a non-capturable step). + // capturable and succeeds, and validation then fails. Rule is + // transactional:false (it must be, because it has a non-capturable + // step). The failure is in validation rather than apply so this stays a + // PartiallyApplied fixture: an apply failure would leave the host + // unconfirmed and report RollbackFailed instead. h0 := &engine.FakeHandler{HandlerName: "noncap_ok", IsCapturable: false} h1 := &engine.FakeHandler{ - HandlerName: "cap_fails", + HandlerName: "cap_ok", IsCapturable: true, - ApplyErr: errors.New("induced fail"), } r := handler.NewRegistry() r.Register(h0) r.Register(h1) - e := engine.New(engine.WithRegistry(r)) + e := engine.New(engine.WithRegistry(r), engine.WithForceValidateFail()) txn := &api.Transaction{ ID: uuid.New(), @@ -182,7 +193,7 @@ func TestEngine_AC05_PartiallyAppliedForNonCapturableSuccess(t *testing.T) { HostID: "test-host", Steps: []api.Step{ {Index: 0, Mechanism: "noncap_ok"}, - {Index: 1, Mechanism: "cap_fails"}, + {Index: 1, Mechanism: "cap_ok"}, }, Transactional: false, } diff --git a/internal/engine/failed_apply_test.go b/internal/engine/failed_apply_test.go new file mode 100644 index 00000000..77072255 --- /dev/null +++ b/internal/engine/failed_apply_test.go @@ -0,0 +1,306 @@ +package engine_test + +import ( + "context" + "errors" + "os" + "path/filepath" + "testing" + "time" + + "github.com/google/uuid" + + "github.com/Hanalyx/kensa/api" + "github.com/Hanalyx/kensa/internal/engine" + "github.com/Hanalyx/kensa/internal/handler" +) + +// Group A fixtures: an invoked Apply that reports failure. Kept separate from +// the Group B fixtures, where Apply fully succeeds and validation then fails, +// because the two produce different contracts and share no expectations. + +// applyFailTxn builds a transaction over the named mechanisms. +func applyFailTxn(transactional bool, mechanisms ...string) *api.Transaction { + steps := make([]api.Step, len(mechanisms)) + for i, m := range mechanisms { + steps[i] = api.Step{Index: i, Mechanism: m} + } + return &api.Transaction{ + ID: uuid.New(), + RuleID: "failed-apply-rule", + HostID: "test-host", + Severity: "medium", + Steps: steps, + StartedAt: time.Now().UTC(), + Deadline: time.Now().Add(time.Minute), + Transactional: transactional, + } +} + +// runTxn drives one transaction through Run. +func runTxn(t *testing.T, txn *api.Transaction, opts []engine.Option, handlers ...api.Handler) *api.TransactionResult { + t.Helper() + r := handler.NewRegistry() + for _, h := range handlers { + r.Register(h) + } + res, err := engine.New(append([]engine.Option{engine.WithRegistry(r)}, opts...)...). + Run(context.Background(), engine.NewFakeTransport(), txn, false) + if err != nil { + t.Fatalf("Run returned err: %v", err) + } + return res +} + +// capOK is a capturable handler that applies cleanly. +func capOK(name string) *engine.FakeHandler { + return &engine.FakeHandler{ + HandlerName: name, + IsCapturable: true, + CapturePreState: &api.PreState{ + Mechanism: name, + Capturable: true, + Data: map[string]any{"prior": "original"}, + }, + } +} + +// noncapOK is a non-capturable handler that applies cleanly. +func noncapOK(name string) *engine.FakeHandler { + return &engine.FakeHandler{HandlerName: name, IsCapturable: false} +} + +// TestFailedApply_ReturnedError is the first failure shape: the handler +// returns an error. The engine does not reverse a failed step, so its effects +// are unresolved and the host is unconfirmed. +// @spec engine-transaction +// @ac AC-26 +func TestFailedApply_ReturnedError(t *testing.T) { + t.Log("// @spec engine-transaction") + t.Log("// @ac AC-26") + h := capOK("err_shape") + h.ApplyErr = errors.New("induced apply failure") + res := runTxn(t, applyFailTxn(true, "err_shape"), nil, h) + + if res.Status != api.StatusRollbackFailed { + t.Errorf("got Status=%s, want RollbackFailed", res.Status) + } + if res.HostUnchanged { + t.Error("HostUnchanged is true after an invoked Apply reported failure") + } +} + +// TestFailedApply_SuccessFalse is the second failure shape: the handler +// returns a result with Success false. Asserted independently, not by +// comparison with the first shape, because before the fix both shapes produced +// the same incorrect outcome and an equivalence check would have passed. +// @spec engine-transaction +// @ac AC-26 +func TestFailedApply_SuccessFalse(t *testing.T) { + t.Log("// @spec engine-transaction") + t.Log("// @ac AC-26") + h := capOK("false_shape") + h.ApplyResult = &api.StepResult{Success: false, Detail: "handler reported failure"} + res := runTxn(t, applyFailTxn(true, "false_shape"), nil, h) + + if res.Status != api.StatusRollbackFailed { + t.Errorf("got Status=%s, want RollbackFailed", res.Status) + } + if res.HostUnchanged { + t.Error("HostUnchanged is true after an invoked Apply reported failure") + } +} + +// TestFailedApply_ShapesAgree is preservation coverage: the two failure shapes +// must not diverge. It passes before and after the fix, so it never stands in +// for the two tests above. +// @spec engine-transaction +// @ac AC-26 +func TestFailedApply_ShapesAgree(t *testing.T) { + t.Log("// @spec engine-transaction") + t.Log("// @ac AC-26") + hErr := capOK("agree_err") + hErr.ApplyErr = errors.New("induced apply failure") + hFalse := capOK("agree_false") + hFalse.ApplyResult = &api.StepResult{Success: false} + + a := runTxn(t, applyFailTxn(true, "agree_err"), nil, hErr) + b := runTxn(t, applyFailTxn(true, "agree_false"), nil, hFalse) + + if a.Status != b.Status { + t.Errorf("failure shapes disagree on status: %s vs %s", a.Status, b.Status) + } + if a.HostUnchanged != b.HostUnchanged { + t.Errorf("failure shapes disagree on HostUnchanged: %v vs %v", a.HostUnchanged, b.HostUnchanged) + } + // Rollback eligibility is unchanged by either shape: a failed step is + // never reversed, so neither run produces a rollback result. + if len(a.RollbackResults) != 0 || len(b.RollbackResults) != 0 { + t.Errorf("a failed step was reversed: %d and %d rollback results", + len(a.RollbackResults), len(b.RollbackResults)) + } +} + +// TestFailedApply_EarlierStepStillReversed proves the verdict is about the +// transaction, not about the rollback rows: an earlier capturable step is +// still reversed and can report success while the transaction reports an +// unconfirmed host. +// @spec engine-transaction +// @ac AC-26 +func TestFailedApply_EarlierStepStillReversed(t *testing.T) { + t.Log("// @spec engine-transaction") + t.Log("// @ac AC-26") + ok := capOK("earlier_ok") + bad := capOK("later_fails") + bad.ApplyErr = errors.New("induced apply failure") + res := runTxn(t, applyFailTxn(true, "earlier_ok", "later_fails"), nil, ok, bad) + + if res.Status != api.StatusRollbackFailed { + t.Errorf("got Status=%s, want RollbackFailed", res.Status) + } + if res.HostUnchanged { + t.Error("HostUnchanged is true after an invoked Apply reported failure") + } + if len(res.RollbackResults) != 1 { + t.Fatalf("got %d rollback results, want 1 (step 0 only)", len(res.RollbackResults)) + } + if res.RollbackResults[0].StepIndex != 0 { + t.Errorf("reversed step %d, want step 0", res.RollbackResults[0].StepIndex) + } + if !res.RollbackResults[0].Success { + t.Error("step 0 should have reversed cleanly") + } + if ok.RollbackCalls != 1 || bad.RollbackCalls != 0 { + t.Errorf("rollback eligibility wrong: ok=%d bad=%d", ok.RollbackCalls, bad.RollbackCalls) + } +} + +// TestFailedApply_TakesPrecedenceOverStranded covers the precedence rule: an +// unresolved failed Apply outranks the stranded-step verdict, and the stranded +// evidence survives the change of status. +// @spec engine-transaction +// @ac AC-26 +func TestFailedApply_TakesPrecedenceOverStranded(t *testing.T) { + t.Log("// @spec engine-transaction") + t.Log("// @ac AC-26") + strand := noncapOK("strand_ok") + bad := capOK("strand_fails") + bad.ApplyErr = errors.New("induced apply failure") + res := runTxn(t, applyFailTxn(false, "strand_ok", "strand_fails"), nil, strand, bad) + + if res.Status != api.StatusRollbackFailed { + t.Errorf("got Status=%s, want RollbackFailed (precedence over PartiallyApplied)", res.Status) + } + if !res.Steps[0].Stranded { + t.Error("stranded evidence lost when the verdict became RollbackFailed") + } + if res.Steps[1].Stranded { + t.Error("the FAILED step was marked Stranded") + } +} + +// TestFailedApply_FailedNonCapturableIsNotStranded is the condition OpenWatch +// depends on: their step note reads "it succeeded before a later step failed", +// so the flag must never appear on a step that failed. The failing step here is +// itself non-capturable, which is the shape a careless gate would mark. +// @spec engine-transaction +// @ac AC-26 +func TestFailedApply_FailedNonCapturableIsNotStranded(t *testing.T) { + t.Log("// @spec engine-transaction") + t.Log("// @ac AC-26") + ok := noncapOK("nc_ok") + bad := noncapOK("nc_fails") + bad.ApplyErr = errors.New("induced apply failure") + res := runTxn(t, applyFailTxn(false, "nc_ok", "nc_fails"), nil, ok, bad) + + if res.Status != api.StatusRollbackFailed { + t.Errorf("got Status=%s, want RollbackFailed", res.Status) + } + if !res.Steps[0].Stranded { + t.Error("the successful non-capturable step lost its Stranded flag") + } + if res.Steps[1].Stranded { + t.Error("the FAILED non-capturable step was marked Stranded") + } +} + +// TestFailedApply_AlsoFailedRollback covers both causes at once. The status is +// already RollbackFailed before this change because the rollback failed, so +// the status assertion is preservation; what this adds is that both causes +// stay visible. +// @spec engine-transaction +// @ac AC-26 +func TestFailedApply_AlsoFailedRollback(t *testing.T) { + t.Log("// @spec engine-transaction") + t.Log("// @ac AC-26") + ok := capOK("both_ok") + ok.RollbackResult = &api.RollbackResult{Success: false, Detail: "induced rollback failure"} + bad := capOK("both_fails") + bad.ApplyErr = errors.New("induced apply failure") + res := runTxn(t, applyFailTxn(true, "both_ok", "both_fails"), nil, ok, bad) + + if res.Status != api.StatusRollbackFailed { + t.Errorf("got Status=%s, want RollbackFailed", res.Status) + } + if res.Steps[1].Success { + t.Error("the failed apply step is not recorded as failed") + } + if len(res.RollbackResults) != 1 || res.RollbackResults[0].Success { + t.Error("the failed rollback of step 0 is not visible") + } +} + +// TestFailedApply_ResidualHostChange is the stateful fixture. The handler +// writes a real file and then fails, so the run has an observable residual +// change to sit beside the verdict. Before the fix this reported RolledBack +// with HostUnchanged true while the file was on disk. +// @spec engine-transaction +// @ac AC-26 +func TestFailedApply_ResidualHostChange(t *testing.T) { + t.Log("// @spec engine-transaction") + t.Log("// @ac AC-26") + dir := t.TempDir() + residue := filepath.Join(dir, "written-before-failure") + h := &mutatingFailHandler{name: "mutating_fail", path: residue} + res := runTxn(t, applyFailTxn(true, "mutating_fail"), nil, h) + + if _, err := os.Stat(residue); err != nil { + t.Fatalf("fixture did not leave a residual change: %v", err) + } + if res.Status != api.StatusRollbackFailed { + t.Errorf("got Status=%s, want RollbackFailed, with a residual change on disk", res.Status) + } + if res.HostUnchanged { + t.Error("reported HostUnchanged=true while a file written by Apply is still present") + } +} + +// mutatingFailHandler changes state and then reports failure, which is the +// shape the reporting contract exists for: the engine cannot know what a +// failed handler did before it failed. +type mutatingFailHandler struct { + name string + path string +} + +func (h *mutatingFailHandler) Name() string { return h.name } +func (h *mutatingFailHandler) Capturable() bool { return true } + +func (h *mutatingFailHandler) Apply(_ context.Context, _ api.Transport, _ api.Params, _ *api.PreState) (*api.StepResult, error) { + if err := os.WriteFile(h.path, []byte("partial change\n"), 0o600); err != nil { + return nil, err + } + return nil, errors.New("induced failure after the change was made") +} + +func (h *mutatingFailHandler) Capture(_ context.Context, _ api.Transport, _ api.Params) (*api.PreState, error) { + return &api.PreState{ + Mechanism: h.name, + Capturable: true, + Data: map[string]any{"path": h.path, "file_existed": false}, + }, nil +} + +func (h *mutatingFailHandler) Rollback(_ context.Context, _ api.Transport, _ *api.PreState) (*api.RollbackResult, error) { + return nil, errors.New("rollback must not be invoked for a failed step") +} diff --git a/internal/engine/progress_emission_test.go b/internal/engine/progress_emission_test.go index 84f28e13..562168af 100644 --- a/internal/engine/progress_emission_test.go +++ b/internal/engine/progress_emission_test.go @@ -2,7 +2,6 @@ package engine_test import ( "context" - "errors" "testing" "time" @@ -34,19 +33,22 @@ func TestEngine_StreamingPayloadsCarryRuleID(t *testing.T) { t.Run("progress-emission/AC-04", func(t *testing.T) { const wantRuleID = "rule-emit-ac04" - // A handler whose Apply fails forces the rollback path, so the - // same run exercises TransactionStarted, PhaseCompleted, AND - // RolledBack — all three RuleID-bearing payloads. + // A clean apply whose validation fails forces the rollback path, + // so the same run exercises TransactionStarted, PhaseCompleted, + // AND RolledBack — all three RuleID-bearing payloads. Driving it + // with a FAILED apply instead would end RollbackFailed, which + // publishes the same RolledBack event kind but for an unconfirmed + // host; this test wants the clean verdict. h := &engine.FakeHandler{ HandlerName: "fake_apply_fails", IsCapturable: true, - ApplyErr: errors.New("induced apply failure"), } r := handler.NewRegistry() r.Register(h) bus := engine.NewInMemoryEventBus() - e := engine.New(engine.WithRegistry(r), engine.WithEvents(bus)) + e := engine.New(engine.WithRegistry(r), engine.WithEvents(bus), + engine.WithForceValidateFail()) // Long-lived subscription drained after the run; cancel closes // the channel so drainEvents terminates. diff --git a/internal/engine/recapture_test.go b/internal/engine/recapture_test.go index 1765468c..bd252d9d 100644 --- a/internal/engine/recapture_test.go +++ b/internal/engine/recapture_test.go @@ -91,8 +91,13 @@ func TestEngine_AC23_RollbackRecapturesAndMatches(t *testing.T) { IsCapturable: true, RollbackResult: &api.RollbackResult{Success: true, Detail: "restored"}, } - h1 := &engine.FakeHandler{HandlerName: "rc_s1", IsCapturable: true, ApplyErr: errors.New("boom")} - e := durabilityEngine(t, nil, nil, h0, h1) + h1 := &engine.FakeHandler{HandlerName: "rc_s1", IsCapturable: true} + // Validation failure drives the rollback, so this exercises recapture on + // a clean RolledBack outcome rather than on an unresolved apply. + rr := handler.NewRegistry() + rr.Register(h0) + rr.Register(h1) + e := engine.New(engine.WithRegistry(rr), engine.WithForceValidateFail()) txn := &api.Transaction{ ID: uuid.New(), diff --git a/specs/engine/audit-emission.spec.yaml b/specs/engine/audit-emission.spec.yaml index 6cbe8fdc..08f12fde 100644 --- a/specs/engine/audit-emission.spec.yaml +++ b/specs/engine/audit-emission.spec.yaml @@ -86,9 +86,11 @@ spec: priority: critical - id: AC-02 description: | - A rolled-back transaction (apply fails) emits started, - capture, apply, rolled_back — with NO validate phase (apply - did not succeed). + A transaction whose apply fails emits started, capture, apply, + rollback_failed — with NO validate phase (apply did not + succeed). The terminal phase carries the transaction's status, + and a failed apply leaves the host unconfirmed. This criterion + named rolled_back until the failed-apply reporting correction. references_constraints: [C-01] priority: critical - id: AC-03 diff --git a/specs/engine/transaction.spec.yaml b/specs/engine/transaction.spec.yaml index dbf33a75..0c433b0b 100644 --- a/specs/engine/transaction.spec.yaml +++ b/specs/engine/transaction.spec.yaml @@ -183,16 +183,19 @@ spec: - id: C-11 description: | The terminal status of a transaction that rolled back MUST be a - VERDICT over the per-step RollbackResults, not a constant. The - status is RolledBack only when every reversed step restored - cleanly (Success == true AND PartialRestore == false). If any - reversed step failed or reported a partial restore, the host is - in an unconfirmed state and the status MUST be RollbackFailed - (with HostUnchanged == false and the per-step divergence carried - in RollbackResults) — the engine MUST NOT report RolledBack / - "the host is unchanged" for a restoration it did not confirm. A - clean rollback that left stranded non-capturable steps - (transactional:false) is PartiallyApplied. + VERDICT over the per-step RollbackResults AND the apply results, + not a constant. The status is RolledBack only when no invoked + Apply reported failure and every reversed step restored cleanly + (Success == true AND PartialRestore == false). If any reversed + step failed or reported a partial restore, or any invoked Apply + reported failure, the host is in an unconfirmed state and the + status MUST be RollbackFailed (with HostUnchanged == false and + the per-step divergence carried in RollbackResults and Steps) — + the engine MUST NOT report RolledBack / "the host is unchanged" + for a restoration it did not confirm, and it does not attempt to + reverse a failed step, so it cannot confirm one. A clean + rollback with no failed apply that left stranded non-capturable + steps (transactional:false) is PartiallyApplied. type: security enforcement: error - id: C-14 @@ -221,17 +224,26 @@ spec: priority: critical - id: AC-02 description: | - Run returns TransactionResult{Status: RolledBack} when any - apply step fails; every preceding applied capturable step is - reversed in reverse order using its captured pre-state. + When any apply step fails, every PRECEDING applied capturable + step is reversed in reverse order using its captured pre-state. + The failed step itself is not reversed, so its effects are + unresolved and Run returns TransactionResult{Status: + RollbackFailed} with HostUnchanged false, even when every + reversed step restored cleanly. This criterion stated + RolledBack until the failed-apply reporting correction; the + engine reported a restoration it had not performed. references_constraints: [C-01, C-02] priority: critical - id: AC-05 description: | For rules declared transactional:false, non-capturable steps - that have already executed are NOT reversed on failure. The - outcome is Status=PartiallyApplied with per-step stranded - flags in the result. + that have already executed are NOT reversed on failure. Where + no invoked Apply reported failure, the outcome is + Status=PartiallyApplied with per-step stranded flags in the + result. Where one did, the outcome is RollbackFailed and the + stranded flags are still set: an unresolved apply outranks the + stranded-step verdict without erasing its evidence. The flags + are never set on a step that failed. references_constraints: [C-01] priority: high @@ -336,10 +348,13 @@ spec: priority: critical - id: AC-17 description: | - When every reversed step restores cleanly (Success == true and - PartialRestore == false), the terminal Status is RolledBack and - HostUnchanged is true — the clean-rollback verdict is unchanged - from prior behavior. + When no invoked Apply reported failure and every reversed step + restores cleanly (Success == true and PartialRestore == false), + the terminal Status is RolledBack and HostUnchanged is true. + The clean-rollback verdict is otherwise unchanged from prior + behavior; the added qualifier is the failed-apply correction, + since a clean reversal of the steps that DID apply says nothing + about the one that did not. references_constraints: [C-11] priority: critical - id: AC-18 @@ -408,6 +423,23 @@ spec: evidence envelope records Decision == staged. references_constraints: [C-14] priority: critical + - id: AC-26 + description: | + An invoked Apply that reports failure, by returned error or by + a Success:false result, yields StatusRollbackFailed with + HostUnchanged=false, whether or not the reversible steps rolled + back cleanly. Successful non-capturable steps in the same + transaction keep their Stranded flag. + + When Apply fully succeeds and validation fails, any reported + rollback failure or partial restoration yields + StatusRollbackFailed. Otherwise, remaining successful + non-capturable steps yield StatusPartiallyApplied; with none + remaining, clean reported rollback yields StatusRolledBack and + HostUnchanged=true. A successful transaction sets no Stranded + flag. + references_constraints: [C-09, C-11] + priority: critical depends_on: - spec_id: handler-interface From 391df6a3dd956ad19b3f8113f0dbea8af09ea121 Mon Sep 17 00:00:00 2001 From: Remylus Losius Date: Sun, 20 Sep 2026 16:01:12 -0400 Subject: [PATCH 2/3] docs: state the full status precedence everywhere the contract is written Three artifacts still carried a partial or outdated contract after the failed-apply reporting change. None involved a code defect. AC-05 said no failed Apply means PartiallyApplied, which overlooked a failed or partial reversal. AC-17 promised RolledBack after clean reversals, which overlooked stranded non-capturable steps. Both now state their case in terms of AC-26's complete precedence and point to it. The Stranded godoc on the frozen api promised marking "in any terminal status" of a transactional:false transaction. A transaction whose apply and validation succeeded and which then errored on signing or persistence carries no marking, because its verdict at marking time was Committed. The comment now states the two verdicts that set the flag, that the flag is set before the evidence is built, and that it survives a later demotion to Errored. Runtime behavior is unchanged. The operator guide's status table still said a failed apply ends rolled_back, and described rollback_failed as requiring an attempted reversal. The rows now explain the unresolved failed apply, that either cause of rollback_failed wins over partially_applied, that stranded flags survive it, and that recapture is advisory. Adds a test combining successful non-capturable work with a capturable step whose reversal fails. At the parent commit its status and HostUnchanged assertions pass, since an unclean reversal already produced RollbackFailed, but the stranded-flag assertion fails: the narrower gate dropped the flag under that status. It is preservation coverage for the precedence and a defect regression for the retained evidence. --- api/transaction.go | 10 ++++++--- docs/guide/05-rollback-and-history.md | 6 ++--- internal/engine/apply_success_test.go | 32 +++++++++++++++++++++++++++ specs/engine/transaction.spec.yaml | 30 +++++++++++++++---------- 4 files changed, 60 insertions(+), 18 deletions(-) diff --git a/api/transaction.go b/api/transaction.go index 1917199b..7dfdf28e 100644 --- a/api/transaction.go +++ b/api/transaction.go @@ -290,9 +290,13 @@ type StepResult struct { // Detail is human-readable per-step output suitable for logs and UI. Detail string // Stranded is true for non-capturable steps that succeeded before a - // later failure, in any terminal status of a transactional:false - // transaction. Such steps are not reversed by rollback. It is never - // set on a step that failed, and never on a successful transaction. + // later failure in a transactional:false transaction. It is set when + // the transaction's verdict is [StatusPartiallyApplied] or + // [StatusRollbackFailed], before the evidence is built, and it stays + // set if a later evidence or persistence failure demotes the result to + // [StatusErrored]. A transaction whose verdict was Committed carries no + // stranded marking even if it is later demoted. Such steps are not + // reversed by rollback. The flag is never set on a step that failed. Stranded bool // Staged is true when the mechanism could not converge live state and // instead wrote a reboot-deferred persist change (audit_rule_set on an diff --git a/docs/guide/05-rollback-and-history.md b/docs/guide/05-rollback-and-history.md index 96da431f..bcc1f934 100644 --- a/docs/guide/05-rollback-and-history.md +++ b/docs/guide/05-rollback-and-history.md @@ -26,10 +26,10 @@ terminal status the transaction lands in is what the log records: | Status | How it gets there | |---|---| | `committed` | Every apply step succeeded and every validator passed. The host is in the target state; the signed evidence envelope (the Ed25519-signed record of what the transaction did) is persisted. This is the status `rollback` reverses. | -| `rolled_back` | Apply or validate failed, and the engine reversed every applied capturable step using the captured pre-state, in the **same run**. The host is back in its exact pre-change state. | -| `partially_applied` | A `transactional: false` rule ran at least one non-capturable step before failing. Those steps are not reversed; per-step `Stranded` flags say which. | +| `rolled_back` | Validation failed after every apply step succeeded, and the engine reversed every applied capturable step using the captured pre-state, in the **same run**, with each reversal reporting success. The host is back in its exact pre-change state. | +| `partially_applied` | A `transactional: false` rule ran at least one non-capturable step, every apply step succeeded, validation failed, and every reversal reported success. The non-capturable steps are not reversed; per-step `Stranded` flags say which. | | `errored` | A phase could not complete within the deadline, or a terminal step (signing or persistence) failed. `HostUnchanged` distinguishes an abort that never mutated the host. | -| `rollback_failed` | Apply/validate failed and the engine tried to reverse, but the restoration could not be machine-verified (a rollback step failed or reported a partial restore). The host is in an unconfirmed state. | +| `rollback_failed` | The host is in an unconfirmed state, for either of two reasons. **An apply step failed.** The engine does not reverse a step whose own apply failed, and it cannot know what that step changed before failing, so its effects are unresolved; earlier steps that did apply are still reversed. **Or a reversal failed** or reported a partial restore. Either reason wins over `partially_applied`, and any successful non-capturable step keeps its `Stranded` flag. Recapture is recorded per reversed step for the audit record and does not decide this status. | | `recovered` | An interrupted transaction (the process died after pre-state was persisted but before any terminal record) was reversed out-of-band by [`kensa recover`](#recover-crash-recovery). | The key distinction: **`rolled_back`** is the engine undoing a failure diff --git a/internal/engine/apply_success_test.go b/internal/engine/apply_success_test.go index b4e2eb87..75368008 100644 --- a/internal/engine/apply_success_test.go +++ b/internal/engine/apply_success_test.go @@ -274,3 +274,35 @@ func TestFailedApply_PlanPathClassifiesTheSame(t *testing.T) { t.Error("plan path marked the FAILED step Stranded") } } + +// TestApplySucceeded_StrandedWithFailedRollback combines successful +// non-capturable work with a capturable step whose reversal fails. The +// unclean reversal outranks the stranded-step verdict, and the stranded +// evidence survives the change of status. The status here was already +// RollbackFailed before the failed-apply work; what that work added is the +// retained flag, which the narrower gate used to drop. +// +// @spec engine-transaction +// @ac AC-26 +func TestApplySucceeded_StrandedWithFailedRollback(t *testing.T) { + t.Log("// @spec engine-transaction") + t.Log("// @ac AC-26") + nc := noncapOK("swfr_nc") + c := capOK("swfr_cap") + c.RollbackResult = &api.RollbackResult{Success: false, Detail: "induced rollback failure"} + res := runTxn(t, applyFailTxn(false, "swfr_nc", "swfr_cap"), + []engine.Option{engine.WithForceValidateFail()}, nc, c) + + if res.Status != api.StatusRollbackFailed { + t.Errorf("got Status=%s, want RollbackFailed (unclean reversal outranks stranded)", res.Status) + } + if res.HostUnchanged { + t.Error("an unconfirmed restoration reported HostUnchanged=true") + } + if !res.Steps[0].Stranded { + t.Error("the successful non-capturable step lost its Stranded flag under RollbackFailed") + } + if res.Steps[1].Stranded { + t.Error("a capturable step was marked Stranded") + } +} diff --git a/specs/engine/transaction.spec.yaml b/specs/engine/transaction.spec.yaml index 0c433b0b..d54d69df 100644 --- a/specs/engine/transaction.spec.yaml +++ b/specs/engine/transaction.spec.yaml @@ -238,12 +238,14 @@ spec: description: | For rules declared transactional:false, non-capturable steps that have already executed are NOT reversed on failure. Where - no invoked Apply reported failure, the outcome is - Status=PartiallyApplied with per-step stranded flags in the - result. Where one did, the outcome is RollbackFailed and the - stranded flags are still set: an unresolved apply outranks the - stranded-step verdict without erasing its evidence. The flags - are never set on a step that failed. + no invoked Apply reported failure and every reversed step + restored cleanly, the outcome is Status=PartiallyApplied with + per-step stranded flags in the result. Where an invoked Apply + failed, or a reversed step failed or reported a partial + restore, the outcome is RollbackFailed and the stranded flags + are still set: an unconfirmed host outranks the stranded-step + verdict without erasing its evidence (see AC-26 for the full + precedence). The flags are never set on a step that failed. references_constraints: [C-01] priority: high @@ -348,13 +350,17 @@ spec: priority: critical - id: AC-17 description: | - When no invoked Apply reported failure and every reversed step + When no invoked Apply reported failure, every reversed step restores cleanly (Success == true and PartialRestore == false), - the terminal Status is RolledBack and HostUnchanged is true. - The clean-rollback verdict is otherwise unchanged from prior - behavior; the added qualifier is the failed-apply correction, - since a clean reversal of the steps that DID apply says nothing - about the one that did not. + and no successful non-capturable step remains stranded, the + terminal Status is RolledBack and HostUnchanged is true. With + stranded steps remaining and everything else clean, the Status + is PartiallyApplied (AC-05); with any failed apply or unclean + reversal, it is RollbackFailed (AC-26). The clean-rollback + verdict is otherwise unchanged from prior behavior; the + failed-apply qualifier is the correction, since a clean + reversal of the steps that DID apply says nothing about the + one that did not. references_constraints: [C-11] priority: critical - id: AC-18 From 727f3b9fec68f23bd910becd2ec8ed623f222177 Mon Sep 17 00:00:00 2001 From: Remylus Losius Date: Sun, 20 Sep 2026 17:16:05 -0400 Subject: [PATCH 3/3] docs(guide): describe the rollback verdicts by the classifier's conditions The status table still enumerated triggers, and got the conditions wrong in four places: rolled_back did not require the absence of stranded steps, a clean reversal was described as success alone rather than success without a partial restore, validation failure was named as the only route into reversal when a failed deadman cancellation also reaches it, and the failed-apply row said every earlier successful step is reversed when only the capturable ones are. The three rows now state what must hold once the engine begins reversing, whatever brought it there, matching engine-transaction AC-26. Documentation only; no runtime change. --- docs/guide/05-rollback-and-history.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/guide/05-rollback-and-history.md b/docs/guide/05-rollback-and-history.md index bcc1f934..5d3dc33b 100644 --- a/docs/guide/05-rollback-and-history.md +++ b/docs/guide/05-rollback-and-history.md @@ -26,10 +26,10 @@ terminal status the transaction lands in is what the log records: | Status | How it gets there | |---|---| | `committed` | Every apply step succeeded and every validator passed. The host is in the target state; the signed evidence envelope (the Ed25519-signed record of what the transaction did) is persisted. This is the status `rollback` reverses. | -| `rolled_back` | Validation failed after every apply step succeeded, and the engine reversed every applied capturable step using the captured pre-state, in the **same run**, with each reversal reporting success. The host is back in its exact pre-change state. | -| `partially_applied` | A `transactional: false` rule ran at least one non-capturable step, every apply step succeeded, validation failed, and every reversal reported success. The non-capturable steps are not reversed; per-step `Stranded` flags say which. | +| `rolled_back` | The engine began reversing in the **same run**, and three things held: no apply step failed, every reversed step reported `Success: true` with no partial restore, and no successful non-capturable step was left behind. The host is back in its exact pre-change state. | +| `partially_applied` | The same conditions on apply and reversal held, but a `transactional: false` rule ran at least one non-capturable step that succeeded and cannot be reversed. Those steps keep their per-step `Stranded` flags, which say which ones. | | `errored` | A phase could not complete within the deadline, or a terminal step (signing or persistence) failed. `HostUnchanged` distinguishes an abort that never mutated the host. | -| `rollback_failed` | The host is in an unconfirmed state, for either of two reasons. **An apply step failed.** The engine does not reverse a step whose own apply failed, and it cannot know what that step changed before failing, so its effects are unresolved; earlier steps that did apply are still reversed. **Or a reversal failed** or reported a partial restore. Either reason wins over `partially_applied`, and any successful non-capturable step keeps its `Stranded` flag. Recapture is recorded per reversed step for the audit record and does not decide this status. | +| `rollback_failed` | The host is in an unconfirmed state, for either of two reasons. **An apply step failed.** The engine does not reverse a step whose own apply failed, and it cannot know what that step changed before failing, so its effects are unresolved; earlier successful capturable steps are still reversed. **Or a reversed step reported failure or a partial restore.** Either reason wins over `partially_applied`, and any successful non-capturable step keeps its `Stranded` flag. Recapture is recorded per reversed step for the audit record and does not decide this status. | | `recovered` | An interrupted transaction (the process died after pre-state was persisted but before any terminal record) was reversed out-of-band by [`kensa recover`](#recover-crash-recovery). | The key distinction: **`rolled_back`** is the engine undoing a failure