[WorkItem #19558] Resolved ptests failure in trident package#17427
[WorkItem #19558] Resolved ptests failure in trident package#17427Ratiranjan5 wants to merge 1 commit into
ptests failure in trident package#17427Conversation
|
Buddy build has passed. |
suresh-thelkar
left a comment
There was a problem hiding this comment.
I have the following questions, can you please go through them and answer?
-
Upstream PR for the actual fix. "Upstream reference: microsoft/trident#458" points to the PR that introduced the code, not one that contains this fix. Please open a corresponding PR against microsoft/trident (main) that lands the same TimedOut change in crates/trident/src/io_utils/http/mod.rs::retriable_request_sender, and link that PR here. Right now this patch is a downstream-only divergence from upstream main.
-
Justify collapsing the error classification. The current patch replaces both branches (response.error_for_status().map_err(http_to_io_err) and Err(http_to_io_err(e))) with an unconditional IoErrorKind::TimedOut. That means a final-attempt 404, 401/403, 5xx, or a transport error (is_connect, is_request) — which http_to_io_err maps to NotFound / PermissionDenied / ConnectionAborted / ConnectionRefused / InvalidData — will now all be reported as TimedOut. Either:
narrow the fix so the status/transport-derived IoErrorKind is preserved on the final attempt and only synthesize TimedOut when the underlying error has no better classification, or
explain why losing that classification is acceptable for all callers.
- Why patch the source instead of fixing the ptest? The PR summary says the failure is "nondeterministic error kind on HTTP retry timeout exhaustion." Was fixing the test (accept either TimedOut or the mapped HTTP kind) considered? If yes, why was changing production behavior preferred?
Merge Checklist
All boxes should be checked before merging the PR (just tick any boxes which don't apply to this PR)
*-staticsubpackages, etc.) have had theirReleasetag incremented../cgmanifest.json,./toolkit/scripts/toolchain/cgmanifest.json,.github/workflows/cgmanifest.json)./LICENSES-AND-NOTICES/SPECS/data/licenses.json,./LICENSES-AND-NOTICES/SPECS/LICENSES-MAP.md,./LICENSES-AND-NOTICES/SPECS/LICENSE-EXCEPTIONS.PHOTON)*.signatures.jsonfilessudo make go-tidy-allandsudo make go-test-coveragepassSummary
What does the PR accomplish, why was it needed?
TimedOutconsistently when the HTTP retry deadline is exhausted.AI Analysis relevance (In %) / Reasoning
AI Analysis relevance (In %): N/A (no work Item was created for it)
Upstream reference: microsoft/trident#458
Change Log
Does this affect the toolchain?
NO
Associated issues
Links to CVEs
Test Methodology