Skip to content

orchestrator-sm: carry recovery attempt on RecoverComponent - #407

Draft
rusty1968 wants to merge 1 commit into
OpenPRoT:mainfrom
rusty1968:sm-recover-attempt
Draft

orchestrator-sm: carry recovery attempt on RecoverComponent#407
rusty1968 wants to merge 1 commit into
OpenPRoT:mainfrom
rusty1968:sm-recover-attempt

Conversation

@rusty1968

@rusty1968 rusty1968 commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

Effect::RecoverComponent now carries { id, attempt } instead of just id. attempt is the component's consecutive-recovery count (0 on the first try), taken straight from the core's retry counter.

Passing it in-band lets the Platform driver pick a recovery source per attempt (e.g. slot A → slot B → golden) without keeping its own counter, which could drift from the core's since the driver never sees when a recovery succeeds.

No behavior change in the core: the retry cap is still measured against the same counter; attempt only rides out on the effect. Tests updated.

Emit the per-component retry count as RecoverComponent { id, attempt } so
the driver picks a recovery source per attempt without its own counter.
@chrysh

chrysh commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

Looks good — one gap: every assertion in the suite pins attempt: 0, so the counter actually incrementing is never covered. If attempt were hardcoded to 0, all tests would still pass.

Two tests that close that, verified green on this branch — feel free to take them as-is (they slot in after corruption_while_recovering_retargets_to_new_component):

/// `RecoverComponent.attempt` mirrors the core's retry counter: the second
/// consecutive recovery episode for the same component rides out with
/// `attempt: 1`, so a driver can escalate its recovery source without
/// keeping a counter of its own.
#[test]
fn recover_component_attempt_increments_across_episodes() {
    let (effects, _) = drive(
        passive_required(&[C0]),
        &[
            BOOT,
            Event::VerificationFailed(C0), // episode 1 → attempt 0
            Event::Restored(C0),           // credited: retry becomes 1, re-walk
            Event::VerificationFailed(C0), // episode 2 → attempt 1
        ],
    );
    assert!(effects.contains(&Effect::RecoverComponent { id: C0, attempt: 1 }));
}

/// A recovery episode displaced by another component's corruption ends
/// without a `Restored`, so it is never credited to the retry counter: the
/// displaced component's next episode reports `attempt: 0` again. Pins the
/// consecutive-count semantics — a driver must not assume attempts are
/// monotonic across displaced episodes.
#[test]
fn displaced_recovery_episode_restarts_attempt_count() {
    let (effects, state) = drive(
        passive_required(&[C0, C1]),
        &[
            BOOT,
            Event::VerificationFailed(C0), // → Recovering(C0), never credited
            Event::CorruptionDetected(C1), // displaces → Recovering(C1)
            Event::Restored(C1),           // C1 credited, re-walk from the top
            Event::VerificationFailed(C0), // C0's next episode…
        ],
    );
    // …did start (this assert is what makes the negative one meaningful)…
    assert_eq!(state, State::Recovering(C0));
    // …and is attempt 0 again, not 1.
    assert!(!effects.contains(&Effect::RecoverComponent { id: C0, attempt: 1 }));
}

The second one is worth a sentence in the attempt doc too: a displaced episode is never credited, so attempts are not monotonic across episodes.

@chrysh chrysh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests and a doc suggestion for #407 are posted — take them as-is if they fit. From my side, merge #407 and #397 whenever you're happy, either order; they don't overlap.

self.statuses[i].released = false;
self.statuses[i].retry
}
None => 0,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Maybe a comment that this arm should be unreachable? Recovering(failed) can only be entered with an id that passed the dispatch-level chain-membership check, so status_index is always Some there.

/// driver can try a different recovery source each time (say, slot A on
/// attempt 0, slot B on 1, golden on 2) without counting attempts itself —
/// a count of its own could drift from the core's, since the driver never
/// sees when a recovery succeeds.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add?:

/// Consecutive, not monotonic: a displaced episode is never credited,
/// so that component's next episode reports `attempt: 0` again. Don't
/// carry ladder progress across episodes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants