fix(engine): report an unconfirmed host when an apply step fails - #429
Merged
Merged
Conversation
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.
…tten 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.
…tions 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Kensa reported a failed remediation step as a successful restoration. The engine does not reverse a step whose own Apply failed, so that step produced no rollback result, an empty rollback set counted as clean, and the transaction ended
rolled_backwithHostUnchanged: truewhile the host could still carry whatever the handler changed before it failed.A failed invoked Apply, by returned error or by a
Success: falseresult, now reportsrollback_failedwithHostUnchanged: false. Earlier successful capturable steps are still reversed. An unresolved failed Apply takes precedence over the stranded-step verdict, and successful non-capturable steps keep theirStrandedflag under either status. The flag is never set on a step that failed, and never on a successful transaction. Transactions whose Apply fully succeeded are unaffected: a clean reversal still reportsrolled_back, and remaining stranded steps still reportpartially_applied.Stranded marking is finalized before the evidence envelope is built, so the reporting change is inside the signature on both the returned and the persisted record.
Contract surfaces corrected alongside the code:
engine-transactionAC-02, which stated the old behavior as a critical criterion, AC-05, AC-17 and C-11;engine-audit-emissionAC-02; theStatusRolledBack,StatusPartiallyApplied,StatusRollbackFailedandStrandedgodoc, including removal of a recapture claim the code never implemented; and the operator guide's status table. AC-26 states the new contract.Validation:
RunandExecutePlan.OpenWatch confirmed no consumer change is required (
OW-KN-048): affected remediations move from "Reverted, host unchanged" to "Partially applied" with an inspection prompt.This change corrects reporting only. It does not reverse failed steps, change rollback execution, or authenticate
rollback_resultsin the v1 envelope. Validation is offline only.Draft pending required review, CI, and maintainer acceptance.