orchestrator: Latch at the queue front, never lose the latch A failed effect with a full pending queue silently dropped the EffectFailed push: failed stayed set, nothing latched, and the run settled as if the actuation had succeeded. Fold both failure paths into one latch() helper: evict the newest queued event if the queue is full, and push the latch to the *front*, so it settles next and queued feedback drains into Locked instead of actuating hardware after a failure. Document that returned events must quiesce. Assisted-by: Claude:claude-fable-5 Signed-off-by: Christina Quast <christina.quast@9elements.com>
diff --git a/services/orchestrator/sm/src/lib.rs b/services/orchestrator/sm/src/lib.rs index 9faaad0..5a872f4 100644 --- a/services/orchestrator/sm/src/lib.rs +++ b/services/orchestrator/sm/src/lib.rs
@@ -893,9 +893,10 @@ /// /// `Ok(Some(event))` feeds back what the effect produced synchronously (e.g. /// a verification verdict); the driver queues it and settles it in the same -/// dispatch run. At most one event per effect. Never block in `execute`: -/// results that arrive later (boot progress, timer expiry) are delivered as -/// their own outside events via `dispatch`. +/// dispatch run. At most one event per effect. Synchronous results belong +/// here, not in a shell-side queue — one feedback path keeps ordering honest. +/// Never block in `execute`: results that arrive later (boot progress, timer +/// expiry) are delivered as their own outside events via `dispatch`. /// /// Failure stays on the error channel, never in a returned event: `Err` is /// checked between effects, so a failed actuation aborts the rest of the @@ -905,6 +906,10 @@ /// - **Honest, complete feedback.** The core's correctness rests entirely on /// the event stream the shell feeds back; dropping, reordering, or /// synthesizing events silently breaks the state machine's invariants. +/// - **Returned events quiesce.** Every returned event reports a result the +/// reducer consumes (its retry budgets bound re-verification cycles). An +/// executor that manufactures an event for every effect keeps one dispatch +/// run alive indefinitely. /// - **`AssertReset` holds, it does not pulse.** A reset must keep the component /// quiesced and non-executing until its matching `ReleaseReset`. The core's /// at-rest verification guarantee depends on this: it re-asserts reset on @@ -1004,22 +1009,37 @@ /// for each external effect in order. One call runs to quiescence. /// /// If `on_effect` reports an [`EffectError`], the driver injects an - /// [`Event::EffectFailed`] into the same run, so a failed actuation is - /// handled fail-closed (the machine latches to [`State::Locked`]) rather - /// than silently ignored. A pending-queue overflow is handled the same - /// way: losing a returned event would break the honest-feedback contract, - /// so the run latches instead. + /// [`Event::EffectFailed`] at the *front* of the queue, so a failed + /// actuation is handled fail-closed: the latch settles next, and feedback + /// still queued behind it drains into [`State::Locked`] (discarded) + /// instead of actuating hardware after a failure. A pending-queue + /// overflow is handled the same way: losing a returned event would break + /// the honest-feedback contract, so the run latches instead. pub fn dispatch_with( &mut self, event: Event, mut on_effect: impl FnMut(Effect) -> Result<Option<Event>, EffectError>, ) { + // Fail-closed latch: `EffectFailed` goes to the *front*, so it settles + // next and everything still queued drains into `Locked` (discarded) + // instead of actuating hardware after a failure. Prefer evicting the + // newest queued event over losing the latch itself. + fn latch(pending: &mut heapless::Deque<Event, PENDING_CAP>) { + if pending.is_full() { + pending.pop_back(); + } + // Dead Err arm: the eviction above guarantees room. + let _ = pending.push_front(Event::EffectFailed); + } + let mut pending: heapless::Deque<Event, PENDING_CAP> = heapless::Deque::new(); // Dead Err arm: `pending` is empty and `PENDING_CAP >= 3` (asserted at // build time), so the first push always fits. let _ = pending.push_back(event); // `EffectFailed` is injected at most once: it is idempotent and - // terminal (drives to `Locked`, which discards everything after). + // terminal (drives to `Locked`, which discards everything after). The + // only external effect executed after it settles is `Locked`'s own + // entry, whose failure must not inject again. let mut failed = false; while let Some(ev) = pending.pop_front() { @@ -1034,14 +1054,13 @@ Ok(follow_up) => follow_up, Err(_) => { // Fail-closed AND fail-fast: abandon the rest of - // this batch so no effect ordered after the failed - // one hits hardware. `step` has already advanced - // the state as if the whole batch applied, so - // actuating `k+1..` would carry out effects for a - // transition the latch to `Locked` overrides. + // this batch. `step` has already advanced the + // state as if the whole batch applied, and the + // latch overrides that transition, so nothing + // ordered after the failure may hit hardware. if !failed { failed = true; - let _ = pending.push_back(Event::EffectFailed); + latch(&mut pending); } break; } @@ -1051,12 +1070,10 @@ && pending.push_back(next).is_err() && !failed { - // Queue full: the event would be lost. Fail closed — drop - // the newest queued event to make room for the latch, - // which discards everything after it anyway. + // Queue full: `next` would be lost, breaking the + // honest-feedback contract. Fail closed instead. failed = true; - pending.pop_back(); - let _ = pending.push_back(Event::EffectFailed); + latch(&mut pending); break; } }
diff --git a/services/orchestrator/sm/src/tests.rs b/services/orchestrator/sm/src/tests.rs index 7db8226..b574c4c 100644 --- a/services/orchestrator/sm/src/tests.rs +++ b/services/orchestrator/sm/src/tests.rs
@@ -2044,3 +2044,78 @@ assert_eq!(orch.state(), State::Locked); } + +/// A failed effect with the pending queue already full still latches: the +/// latch evicts the newest queued event rather than being dropped itself. +/// Regression test — a back-of-queue push would be lost here, and the run +/// would settle to `Ready` as if nothing failed. +#[test] +fn failed_effect_with_full_queue_still_latches() { + struct ChattyThenFail { + rewalking: bool, + } + impl Platform for ChattyThenFail { + fn execute(&mut self, effect: Effect) -> Result<Option<Event>, EffectError> { + match effect { + // The re-walk after Restored asserts reset on all eight live + // components first — exactly PENDING_CAP returned events, so + // the queue is full (but never overflowed) when the read + // that follows fails. + Effect::AssertReset(_) if self.rewalking => Ok(Some(Event::AttestationChallenge)), + Effect::ReadFirmware(_) if self.rewalking => Err(EffectError), + Effect::VerifyFirmware(id) => Ok(Some(Event::VerificationPassed(id))), + _ => Ok(None), + } + } + } + + let ids: Vec<ComponentId> = (0..8).map(ComponentId::new).collect(); + let mut orch = Orchestrator::<CAPACITY, ECAP>::new( + passive_required(&ids).try_into().expect("valid chain"), + MAX_RETRY, + ); + let mut platform = ChattyThenFail { rewalking: false }; + + orch.dispatch(&mut platform, BOOT); + assert_eq!(orch.state(), State::Ready); + + orch.dispatch(&mut platform, Event::CorruptionDetected(C0)); + platform.rewalking = true; + orch.dispatch(&mut platform, Event::Restored(C0)); + + assert_eq!(orch.state(), State::Locked); +} + +/// An event returned by executing `LatchLockdown` itself is queued, settles +/// in `Locked`, and is discarded — the latch stays terminal and nothing is +/// actuated after the lockdown. +#[test] +fn event_returned_during_lockdown_is_discarded() { + struct FailRelease { + recorded: Vec<Effect>, + } + impl Platform for FailRelease { + fn execute(&mut self, effect: Effect) -> Result<Option<Event>, EffectError> { + self.recorded.push(effect); + match effect { + Effect::ReleaseReset(_) => Err(EffectError), + Effect::LatchLockdown => Ok(Some(Event::AttestationChallenge)), + _ => Ok(None), + } + } + } + + let mut orch = Orchestrator::<CAPACITY, ECAP>::new( + passive_required(&[C0]).try_into().expect("valid chain"), + MAX_RETRY, + ); + let mut platform = FailRelease { + recorded: Vec::new(), + }; + + orch.dispatch(&mut platform, BOOT); + orch.dispatch(&mut platform, Event::VerificationPassed(C0)); + + assert_eq!(orch.state(), State::Locked); + assert_eq!(platform.recorded.last(), Some(&Effect::LatchLockdown)); +}