Repository navigation
fix: close HTTP responses when conversion fails - #2808
Rana Singh (ranadeepsingh) merged 4 commits into
Conversation
## Summary Close responses in a finally block when converting HTTP response data. Add a regression that propagates the conversion failure while checking that the response is closed exactly once. ## Prompting Intent Publish the independent response-resource cleanup defect separately from AIPrompt's new opt-in transport behavior. Keep the fix narrow and prove the baseline failure directly. ## Linked Sources - Bug: microsoft#2802 - Scope: microsoft#2759 ## Rationale The existing success-only close skipped cleanup when body conversion threw. A try/finally fixes that path without changing retry accounting, adding response limits or depending on the new native buffering implementation. The regression failed on baseline and passed with the existing response inspection tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5ba39157-c8dd-4adb-90e0-ff963a1e6733
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
Hey Rana Singh (@ranadeepsingh) 👋! We use semantic commit messages to streamline the release process. Examples of commit messages with semantic prefixes:
To test your commit locally, please follow our guild on building from source. |
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues were identified for the HTTP response-cleanup change.
No open issues 🎉
What changed in this PR
This PR fixes HTTP response cleanup in SynapseML’s core HTTP client so conversion failures do not bypass response closure.
Changes:
- Moves response cleanup into a
finallyblock without changing the conversion logic. - Adds a regression test verifying that a failed body read closes the response exactly once and propagates the original read error.
| File | Description |
|---|---|
| core/src/test/scala/com/microsoft/azure/synapse/ml/io/split1/VerifyResponseBodyInspection.scala | Adds close-counting coverage for response conversion failure. |
| core/src/main/scala/com/microsoft/azure/synapse/ml/io/http/HTTPClients.scala | Ensures response closure after successful or failed conversion. |
Review effort: Balanced
|
CI triage: all 114 Vowpal Wabbit tests passed, including independently verified named published results. The failed job is the subsequent coverage publisher's TLS hostname mismatch, tracked in #2812. This is not a product-code failure, but the required publication/CI gate remains blocked. No blind full-build rerun or product workaround was made. |
|
Current-head Azure validation is complete for 9f2c4d4: build 244930475 validated its merge with master d3f3af6, and all 65 jobs succeeded. The published VerifyResponseBodyInspection results are 4/4 passed, including the response-conversion cleanup regression. One targeted retry was used for the separately diagnosed Vowpal Wabbit coverage-publication TLS failure; no product tests or assertions were weakened. The publication infrastructure issue remains tracked in #2812. |
## Summary Keep the original response conversion failure when closing the response also fails. Attach a distinct cleanup failure as suppressed, avoid self-suppression, and continue propagating cleanup failures after successful conversion. Make the regression fixtures independent of the response's default locale. ## Prompting Intent Complete validation, reviews, CI, comment resolution, and merge ordering for the AIPrompt prerequisite pull requests. Preserve existing public behavior, prove fixes with regressions, and do not merge any pull request. ## Linked Sources - HTTP response ownership fix: microsoft#2808 - Dependent AIPrompt integration: microsoft#2811 - Preserved review findings and dispositions: reviews/pr-2808/ ## Rationale Unconditional cleanup must not hide the exception that caused conversion to fail. Keeping the same exception instance preserves existing caller behavior; suppressed exceptions retain cleanup diagnostics without duplicate logging. Focused tests cover conversion failure, distinct and identical cleanup failures, and cleanup failure following successful conversion. Java 8 compilation, test compilation, all six response-inspection tests, and both core style checks passed. Integrated Spark port checks also passed. All eighteen reviews in the six-stage pre-commit pass qualified on the same two-file patch. Original superseded feedback and resolutions are retained for archival; current-head CI and the final CI-qualified pass remain pending. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5ba39157-c8dd-4adb-90e0-ff963a1e6733
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
## Summary Retain the complete pre-commit and CI-qualified final review sets for the HTTP response cleanup fix. Index superseded reports in their immutable published archive instead of keeping every historical attempt in the final PR diff. ## Prompting Intent Complete missing validation, reviews, CI, and comment resolution for the SynapseML PR stack before describing it as merge-ready. Preserve original feedback and do not merge or enable auto-merge. ## Linked Sources - Pull request: microsoft#2808 - Archived evidence: https://github.com/microsoft/SynapseML/tree/5a756d586411d0fc5bb4f7825c55972c18ee1803/reviews/pr-2808 - Review and retention rules: .github/skills/synapseml-pr-loop/references/review-gauntlet.md and references/loop-control.md ## Rationale All six final rounds completed with clean results from all three required reviewers on the same product patch. This artifact-only publication preserves that fingerprint and avoids recursively reviewing review text. Final-commit CI and remote review remain required; no product code, tests, or dependencies change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5ba39157-c8dd-4adb-90e0-ff963a1e6733
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
🚀 New features to boost your workflow:
|
## Summary Remove one trailing-whitespace line from a preserved PR review artifact. No production code, tests, build inputs, or review conclusions change. ## Prompting Intent Make the necessary minor fixes to the reviewed SynapseML PRs, address review feedback, and verify full CI against each final head without merging. ## Linked Sources - PR: microsoft#2808 - Issue: microsoft#2802 - Repository artifact policy: AGENTS.md, Pull requests and CI ## Rationale Keep the numbered review artifacts and their provenance as required by the repository guide while making the complete PR diff whitespace-clean. The verified HTTP response ownership fix and its tests are untouched. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0e8fdfb7-dcdd-4074-aabb-8634695107f2
|
/azp run |
|
Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command. |
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
CI triage for head The changed HTTP path did execute: Requested a retry of the failed jobs on the same build and exact source/target, preserving successful job results. No product, test, dependency, or pipeline change was made to work around the agent failure. Full CI remains pending until that retry completes and its named results are checked. |
|
Engineering readiness verified for The minor follow-up removed one trailing-whitespace defect only. Required review artifacts and provenance were retained; production and tests are unchanged from the independent ownership/leak evaluation.
Only the required human approval remains. The PR has not been merged. |
AB#5837660 ## Summary Add a combined persistence regression after integrating the latest master fixes. Verify that an explicitly assigned Spark session is used without promoting default scalar parameters into explicit values. Preserve the historical review archive through rebased links and include the focused integration review. ## Prompting Intent The engineer approved resolving the newly introduced merge conflicts without losing either CU behavior or upstream fixes, validating and reviewing the result, publishing with an explicit force-with-lease, and rerunning CI. ## Linked Sources - Feature PR: microsoft#2815 - Python method extraction: microsoft#2810 - Assigned Spark sessions: microsoft#2809 - Response cleanup: microsoft#2808 - Attachment cancellation: microsoft#2807 - Global parameter synchronization: microsoft#2806 - Integration target: 6c2699d - Focused review: reviews/pr-2815/task-5837660-attempt-10-review-0-harness.md ## Rationale Keep both sides of the code-generation and persistence conflicts rather than choosing one wholesale. A combined public save/load test checks the interaction that independent upstream and CU tests do not explicitly cover. Rebase preserves the six original changes and their messages; all 53 archived report blobs and modes are unchanged. Local validation passed 310 Scala and 73 Python tests, with one existing ignored global-fuzzing test, plus style, compilation, codegen and pinned formatting. New-head CI remains a separate post-publication gate. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 653e4257-0554-408f-9e0e-b23750249edb
Failed response conversion can leave HTTP connections open. Cleanup must also preserve the original conversion error rather than replace it with a second failure from closing the response. This change always closes the response and preserves both failures when they occur together.
Related Issues/PRs
Fixes #2802.
Extracted from the scope refinement for #2759. This fix is independent of the native feature and optional recovery work.
What changes are proposed in this pull request?
Close the response in
finally, including when conversion fails. Preserve a conversion failure as the primary exception and attach a distinct cleanup failure as suppressed; still propagate cleanup failure when conversion succeeds. Add focused regressions with locale-independent response fixtures.Retry behavior, buffering, response-size configuration and native inference remain outside this PR.
How is this patch tested?
The source published in
5a756d586411d0fc5bb4f7825c55972c18ee1803was validated in this standalone PR worktree before publication, using the branch-selected Java 8 toolchain:sbt core/compile core/Test/compile— passed.sbt "core/testOnly com.microsoft.azure.synapse.ml.io.split1.VerifyResponseBodyInspection"— 6 passed; zero failed, canceled, ignored or pending.sbt core/scalastyle core/Test/scalastyle— passed, zero errors or warnings.The tests cover conversion failure closing the response, simultaneous conversion/cleanup failure preserving the original error, cleanup failure after successful conversion, and the existing response-inspection behavior. The original missing-close regression failed before the fix; the additional exception-preservation regression also failed before its correction.
The six-stage, three-model pre-commit review is complete, with original reports retained under
reviews/pr-2808/. Current-head Azure validation has been requested; its results, current-head remote review, the separate CI-qualified final review and human approval remain outstanding. This PR is not yet being claimed merge-ready.Does this PR change any dependencies?
Does this PR add a new feature? If so, have you added samples on website?
website/docs/documentationfolder.Make sure you choose the correct class
estimators/transformersand namespace.DocTablepoints to correct API link.npm startto make sure the website renders correctly.<!--pytest-codeblocks:cont-->before each python code blocks to enable auto-tests for python samples.WebsiteSamplesTestsjob pass in the pipeline.