Distinguish poll-again from stop-polling in the initiator contract

[chrysh]

Concrete proposal for the poll-contract discussion from the #12 review — code to argue about instead of an issue thread. Stacked on #14 (retarget/rebase as things merge); only the last commit is new here.

The problem: the polling caller can't implement a correct loop. A request in flight with T2 not yet elapsed — the most common polling state — surfaces as Err(FdInitiatorModeError), the same error as calling in a non-initiator state. Waiting, done, and misuse are indistinguishable. caliptra-mcu-sw grew ResponderAction::Continue/Complete plus a cancellation flag for the responder side of the same problem; their T1 path sets neither, so outcomes keep leaking into side-channels.

The proposal: put the outcome in the return type. generate_initiator_request returns InitiatorAction:

• Request(n) — transmit msg_buf[..n] (1 MCTP type byte + encoded PLDM request)
• Waiting — request in flight or operation still running; poll again
• Cancelled — the UA stayed silent for T1 and the update was cancelled (Modified generate_initiator_request  #12); reported once, next poll returns Idle. Report it and keep serving responder commands so the UA can still GetStatus the reason.
• Idle — the FD left initiator mode; stop polling. Carries no reason: GetStatusReasonCode is the channel for "why"

Err is reserved for real faults (codec, transport, FdOps). The T1 cancel started out as Err(T1Timeout) here, but that pushed callers the wrong way — ? in a polling loop would exit the loop and take the responder path down with it, leaving the UA's follow-up GetStatus unanswered — so it moved into the action type (review discussion below).

Side effect worth its own look: turning the waiting case into a non-error inside fd_progress makes the T1 check reachable while waiting — previously the error short-circuited past it, so a silent UA could only be timed out on the T2 resend path. There's a test pinning that (test_fd_progress_t1_fires_while_waiting).

Resolved during review:

• FdReqState::Failed now arms T1 rather than mapping to Waiting forever: a silent UA that owes a CancelUpdate after a failed transfer/verify/apply gets timed out like any other silence.
• Complete was renamed to Idle — it also covers "update was cancelled and FD is idle again", so "complete" overpromised.

Relates to #13.

[leongross]

I can't find usage of FdReqState::Failed in this PRs changes, could you elaborate a bit more on this?

[CourtneyDrant]

Referencing your question about the return of Complete, perhaps not. We may want the UA to do a GetStatus and retrieve a GetStatusReasonCode to examine the Timeout.

[chrysh]

I can't find usage of FdReqState::Failed in this PRs changes, could you elaborate a bit more on this?

I would need to dig into PLDM in detail, but

The PR never touches FdReqState::Failed directly — but the PR does change its observable behavior, indirectly. That's the elaboration he's asking for. The chain:

1. When a TransferComplete/VerifyComplete/ApplyComplete response reports failure, the FD sets FdReqState::Failed and waits for the UA to cancel (fd_context.rs:816,844,869).
2. should_send_fd_request returns false for Failed (fd_context.rs:1053), so fd_progress produces no request and returns 0.
3. Before this PR, that 0/no-request path surfaced as Err(FdInitiatorModeError). After this PR, it surfaces as InitiatorAction::Waiting. So Failed → "poll again", without any line mentioning Failed changing.
   That's why the PR body raises it as an open question: is "poll again" the honest answer for a failed update, or does it deserve a terminal signal?

We agreed with the AI on this (see latest commit)

The T1 cancel check in fd_progress now matches Sent | Failed instead of only Sent, and the verify/apply pre-check T1 refresh skips Failed (otherwise it would reset the clock every poll and the timeout could never elapse in Verify/Apply).

[chrysh]

Referencing your question about the return of Complete, perhaps not.

Renamed to idle

We may want the UA to do a GetStatus and retrieve a GetStatusReasonCode to examine the Timeout.

Is that not done yet?

[CourtneyDrant]

Referencing your question about the return of Complete, perhaps not.

Renamed to idle

We may want the UA to do a GetStatus and retrieve a GetStatusReasonCode to examine the Timeout.

Is that not done yet?

The UA, not OpenProt, will do a GetStatus Request. The FD, OpenProt, will give the response which includes GetStatusReasonCode.

GetStatusResponse is implemented in OpenProt FD.

[chrysh]

Referencing your question about the return of Complete, perhaps not.

Renamed to idle

We may want the UA to do a GetStatus and retrieve a GetStatusReasonCode to examine the Timeout.

Is that not done yet?

The UA, not OpenProt, will do a GetStatus Request. The FD, OpenProt, will give the response which includes GetStatusReasonCode.

GetStatusResponse is implemented in OpenProt FD.

Thanks for the clarification. Can you explain in more detail what change you suggested with your original comment?

[CourtneyDrant]

Referencing your question about the return of Complete, perhaps not.

Renamed to idle

We may want the UA to do a GetStatus and retrieve a GetStatusReasonCode to examine the Timeout.

Is that not done yet?

The UA, not OpenProt, will do a GetStatus Request. The FD, OpenProt, will give the response which includes GetStatusReasonCode.
GetStatusResponse is implemented in OpenProt FD.

Thanks for the clarification. Can you explain in more detail what change you suggested with your original comment?

Could a user of run_terminus, like the Orchestrator, see an unexpected Idle and report that so that a UA can do a GetStatus to see why?

[chrysh]

Could a user of run_terminus, like the Orchestrator, see an unexpected Idle and report that so that a UA can do a GetStatus to see why?

@CourtneyDrant A bit AI-ish, but hopefully helpful:

Yes — and chasing this exposed a bug in the proposal, fixed in 7a339fd. The T1 cancel surfaced as Err(T1Timeout), so the obvious run_terminus port (? on every error) would have exited the loop — taking the responder path down and leaving exactly that GetStatus unanswered. It's now InitiatorAction::Cancelled: reported once, next poll returns Idle, Err is faults only.

With that, Idle can't happen unannounced: the FD only reaches it through a UA command (the UA knows why) or through T1, which announces itself as Cancelled. The reason code (e.g. DownloadTimeout) is stored before Cancelled is reported, so GetStatus works immediately after.

Phase 1 of run_terminus_inner on the openprot side then becomes (tested against this branch, all 5 host tests pass):

let action = self
    .cmd_interface
    .generate_initiator_request(&mut fw_buf)
    .map_err(PldmServiceError::MsgHandler)?;
let initiator_active = !matches!(action, InitiatorAction::Idle);
match action {
    InitiatorAction::Request(pldm_len) => {
        // unchanged: send via requester_transport, feed the response
        // back through process_initiator_response
    }
    // Nothing due; fall through to the responder poll.
    InitiatorAction::Waiting => {}
    // T1 fired, update cancelled, FD is Idle again. Keep serving so
    // the UA can GetStatus the reason. The platform already saw the
    // cancel synchronously via FdOps::cancel_update_component.
    InitiatorAction::Cancelled => {}
    // Not in initiator mode; the responder poll below blocks with
    // the caller's idle timeout.
    InitiatorAction::Idle => {}
}

Two things fall out:

1. The should_start_initiator_mode() pre-check disappears — Idle is that answer, and initiator_active (short vs. long responder-poll timeout) is derived from the action.
2. Dropping the Cancelled or Idle arm is a compile error, so the kill-the-loop mistake can't come back in a future caller.

If the Orchestrator wants more than the FdOps callback — a log line or counter in the Cancelled arm — that's additive and local to that arm.

@CourtneyDrant If you think #14 and/or #15 are useful, can you merge them? If not, also feel free to close them!

[CourtneyDrant]

Could a user of run_terminus, like the Orchestrator, see an unexpected Idle and report that so that a UA can do a GetStatus to see why?

@CourtneyDrant A bit AI-ish, but hopefully helpful:

Yes — and chasing this exposed a bug in the proposal, fixed in 7a339fd. The T1 cancel surfaced as Err(T1Timeout), so the obvious run_terminus port (? on every error) would have exited the loop — taking the responder path down and leaving exactly that GetStatus unanswered. It's now InitiatorAction::Cancelled: reported once, next poll returns Idle, Err is faults only.

With that, Idle can't happen unannounced: the FD only reaches it through a UA command (the UA knows why) or through T1, which announces itself as Cancelled. The reason code (e.g. DownloadTimeout) is stored before Cancelled is reported, so GetStatus works immediately after.

Phase 1 of run_terminus_inner on the openprot side then becomes (tested against this branch, all 5 host tests pass):

let action = self
    .cmd_interface
    .generate_initiator_request(&mut fw_buf)
    .map_err(PldmServiceError::MsgHandler)?;
let initiator_active = !matches!(action, InitiatorAction::Idle);
match action {
    InitiatorAction::Request(pldm_len) => {
        // unchanged: send via requester_transport, feed the response
        // back through process_initiator_response
    }
    // Nothing due; fall through to the responder poll.
    InitiatorAction::Waiting => {}
    // T1 fired, update cancelled, FD is Idle again. Keep serving so
    // the UA can GetStatus the reason. The platform already saw the
    // cancel synchronously via FdOps::cancel_update_component.
    InitiatorAction::Cancelled => {}
    // Not in initiator mode; the responder poll below blocks with
    // the caller's idle timeout.
    InitiatorAction::Idle => {}
}

Two things fall out:

1. The should_start_initiator_mode() pre-check disappears — Idle is that answer, and initiator_active (short vs. long responder-poll timeout) is derived from the action.
2. Dropping the Cancelled or Idle arm is a compile error, so the kill-the-loop mistake can't come back in a future caller.

If the Orchestrator wants more than the FdOps callback — a log line or counter in the Cancelled arm — that's additive and local to that arm.

@CourtneyDrant If you think #14 and/or #15 are useful, can you merge them? If not, also feel free to close them!

We shouldn't get rid of "should_start_initiator_mode. We are only in initiator mode in three states, Download, Verify, and Apply.