Enable the retry for cert for handling InvalidCertificate error - #614
Enable the retry for cert for handling InvalidCertificate error#614SivaParvathip21 wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the DDI MBOR integration-test helpers to make certificate retrieval more resilient to transient InvalidCertificate failures that can occur when a device soft-reset (iDFU) overlaps with DDI operations.
Changes:
- Added retry helpers for
GetCertificateto reattempt onDdiStatus::InvalidCertificatewith a fixed backoff. - Switched leaf-cert retrieval and cert-chain enumeration paths to optionally use the new retry logic.
- Applied the retry behavior to POTAs endorsement helper certificate retrieval.
Comments suppressed due to low confidence (3)
ddi/mbor/types/tests/integration/common.rs:238
- This retry path uses
println!, which produces unstructured stdout and is inconsistent with the surroundingtracing::*usage. Prefertracing::debug!/warn!for consistent, filterable logs.
println!("Retrying the get_cert operation for cert_id {}", cert_id);
ddi/mbor/types/tests/integration/common.rs:351
- The retry behavior in
helper_verify_leaf_certis also gated behindIDFU=1, which meansInvalidCertificateflakes won’t be handled unless the environment variable is set. Since the retry helper only delays on the specific transient error, using it unconditionally is typically safer for integration tests.
let idfu_enabled = std::env::var("IDFU").map(|v| v == "1").unwrap_or(false);
let result = helper_get_cert_chain_info(dev);
assert!(result.is_ok(), "result {:?}", result);
let resp = result.unwrap();
ddi/mbor/types/tests/integration/common.rs:387
helper_get_pota_endorsementhas the sameIDFU=1gating for retries. IfInvalidCertificatecan occur during concurrent soft reset, tests calling this helper will still flake unless that env var is set. Consider always using the retry helper and only sleeping when the transient error actually happens.
let idfu_enabled = std::env::var("IDFU").map(|v| v == "1").unwrap_or(false);
let get_cert_chain_info = helper_get_cert_chain_info(dev).unwrap();
let leaf_cert_id = get_cert_chain_info.data.num_certs - 1;
let cert_resp = if idfu_enabled {
tracing::debug!("Device is in IDfu mode");
| let cert_info = helper_get_cert_chain_info(dev); | ||
| assert!(cert_info.is_ok(), "cert_info {:?}", cert_info); | ||
| let resp = cert_info.unwrap(); | ||
| let num_certs = resp.data.num_certs; |
| if start.elapsed() > retry_window { | ||
| break; | ||
| } | ||
| println!("Retrying the get_cert operation"); |
| let idfu_enabled = std::env::var("IDFU").map(|v| v == "1").unwrap_or(false); | ||
|
|
||
| let resp = if idfu_enabled { | ||
| tracing::debug!("Device is in IDfu mode"); | ||
| helper_get_cert_with_retry(dev, 5).unwrap() | ||
| } else { | ||
| tracing::debug!("Device is not in IDfu mode"); | ||
| let result = helper_get_cert_chain_info(dev); | ||
| assert!(result.is_ok(), "result {:?}", result); | ||
| let chain_resp = result.unwrap(); | ||
| let num_certs = chain_resp.data.num_certs; | ||
| let result = helper_get_certificate(dev, num_certs - 1); | ||
| assert!(result.is_ok(), "result {:?}", result); | ||
| result.unwrap() | ||
| }; |
| // let result: Result<DdiGetCertificateCmdResp, DdiError> = helper_get_certificate(dev, i); | ||
| // assert!(result.is_ok(), "result {:?}", result); | ||
|
|
||
| // let resp = result.unwrap(); |
|
@SivaParvathip21 please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (5)
ddi/mbor/types/tests/integration/common.rs:200
- The docstring says this helper returns the last success/error, but it currently panics on
GetCertChainInfofailures due toassert!/unwrap(). Since the function already returnsResult, propagate the error instead (and avoid re-fetching the cert chain info on every retry when only the leaf cert id is needed).
let cert_info = helper_get_cert_chain_info(dev);
assert!(cert_info.is_ok(), "cert_info {:?}", cert_info);
let resp = cert_info.unwrap();
let num_certs = resp.data.num_certs;
ddi/mbor/types/tests/integration/common.rs:208
- Using
println!in retry loops can spam CI output and bypass the test logging configuration. Prefertracing(already used elsewhere in this file) and the existingstd::threadimport.
println!("Retrying the get_cert operation");
std::thread::sleep(std::time::Duration::from_millis(50));
ddi/mbor/types/tests/integration/common.rs:239
- Using
println!here can generate noisy test output on transient retries; prefertracingso it can be filtered/collected consistently.
println!("Retrying the get_cert operation for cert_id {}", cert_id);
std::thread::sleep(std::time::Duration::from_millis(50));
ddi/mbor/types/tests/integration/common.rs:368
- Remove the commented-out old implementation to keep the helper easier to read and maintain.
// let result: Result<DdiGetCertificateCmdResp, DdiError> = helper_get_certificate(dev, i);
// assert!(result.is_ok(), "result {:?}", result);
// let resp = result.unwrap();
ddi/mbor/types/tests/integration/common.rs:252
- The tracing message uses inconsistent capitalization ("IDfu") compared to the docstring’s "iDFU". Standardizing improves log searchability; please update the other occurrences in this file too.
tracing::debug!("Device is in IDfu mode");
Enable the get_cert_retry for handling the InvalidCertificate issue while device goes for a softreset concurrently with DDI Operation.