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..7dfdf28e 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,14 @@ 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 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..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` | 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` | 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` | 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 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 diff --git a/internal/engine/apply_success_test.go b/internal/engine/apply_success_test.go new file mode 100644 index 00000000..75368008 --- /dev/null +++ b/internal/engine/apply_success_test.go @@ -0,0 +1,308 @@ +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") + } +} + +// 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/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..d54d69df 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,28 @@ 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 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 @@ -336,10 +350,17 @@ 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, every reversed step + restores cleanly (Success == true and PartialRestore == false), + 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 @@ -408,6 +429,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