Repository navigation
Recover uploads abandoned by preparation exceptions - #1559
Merged
bmehta001 merged 5 commits intoNov 6, 2026
Merged
Conversation
Port the SDK recovery from bmehta001/onnxruntime#11 so failed upload preparation cannot leave teardown waiting for a callback that was never registered. Release reserved records before retiring an upload, contain cleanup exceptions, and preserve HTTP ownership after registration. Protect pending callback allocation with RAII and evaluate request logging before handing ownership to the registry. Adapt all 14 component cases and six native fault modes to the SDK harness, plus a request-log failure test. Files changed: - lib/http/HttpClientManager.cpp - lib/system/Contexts.hpp - lib/system/TelemetrySystem.cpp - lib/tpm/TransmissionPolicyManager.cpp - lib/tpm/TransmissionPolicyManager.hpp - tests/functests/CMakeLists.txt - tests/functests/upload-exception-tests.cpp - tests/unittests/HttpClientManagerTests.cpp - tests/unittests/TransmissionPolicyManagerTests.cpp Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6982f030-2ce2-47a3-b52b-9d8a1cdb05d9
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Exception cleanup can target disk instead of RAM after partial retrieval, leaving RAM records reserved, while a task-lifetime race can make preparation-mode tests fail nondeterministically.
2 open issues
What changed in this PR
This PR adds upload-preparation exception recovery to the SDK, releasing abandoned reservations while preserving HTTP callback ownership of registered uploads.
Changes:
- Share recovery logic between scheduled and immediate uploads.
- Protect pending callbacks with RAII and move request-log formatting before registration.
- Add component regressions and six native-transport fault modes with bounded test timeouts.
| File | Description |
|---|---|
| tests/unittests/TransmissionPolicyManagerTests.cpp | Adds scheduled and immediate upload exception cases. |
| tests/unittests/HttpClientManagerTests.cpp | Tests logging failures and callback ownership. |
| tests/functests/upload-exception-tests.cpp | Adds native-transport recovery and teardown tests. |
| tests/functests/CMakeLists.txt | Registers six fault modes with 30-second timeouts. |
| lib/tpm/TransmissionPolicyManager.hpp | Declares shared recovery helpers and a record-release route. |
| lib/tpm/TransmissionPolicyManager.cpp | Handles preparation exceptions and retires abandoned uploads. |
| lib/system/TelemetrySystem.cpp | Connects failed-upload recovery to storage record release. |
| lib/system/Contexts.hpp | Tracks HTTP callback registration ownership. |
| lib/http/HttpClientManager.cpp | Guards pending callbacks and reorders request logging. |
Review effort: Balanced
bmehta001
enabled auto-merge (squash)
November 6, 2026 19:38
Address Copilot round 1 on microsoft#1559. 4496757055: Mark the active memory/disk backend before retrieval can throw and copy that provenance to the upload context during unwinding, so partial RAM reservations return to RAM rather than being stranded. Verified at lib/offline/OfflineStorageHandler.cpp:654 and lib/offline/StorageObserver.cpp:66. Add four standard/nonstandard RAM/disk regressions in tests/unittests/OfflineStorageTests.cpp. 4496757163: Verify the zero-delay barrier through its future rather than a task pointer that is cleared on completion. Verified at lib/pal/TaskDispatcher.hpp:73; update tests/functests/upload-exception-tests.cpp. Files: lib/offline/OfflineStorageHandler.cpp, lib/offline/StorageObserver.cpp, tests/unittests/OfflineStorageTests.cpp, tests/functests/upload-exception-tests.cpp. Validation: Windows 195 and Linux 199 targeted unit tests; six native upload cases and no-exceptions smoke on both platforms; 20 repetitions of all four Windows preparation barriers. Changed-code clang-format and git diff --check passed; no selected tests were skipped or excluded. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ffecbc0a-951e-4cae-9e94-49ddce94c050
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
An oversized RAM event can be incorrectly marked as disk-backed when the disk read consumes no records, leaving it queued for repeated uploads after successful completion.
1 open issue
2 resolved since last review
Review effort: Balanced
Address Copilot round 2, comment 4496977358: preserve pre-call disk exception provenance, but restore the previous source when a successful disk read consumes no records. This prevents an oversized first RAM event from being mislabeled as disk-backed and uploaded repeatedly after successful completion. Verified at lib/offline/MemoryStorage.cpp:163-168 and lib/packager/Packager.cpp:31-41. Files changed: lib/offline/OfflineStorageHandler.cpp and tests/unittests/OfflineStorageTests.cpp. The new regression failed before the fix, leaving the acknowledged RAM event queued, and passes afterward. Windows and Linux: 25 storage tests, six native upload CTests, and no-exceptions smoke passed without skipped or excluded selected tests. Changed-code clang-format and git diff --check passed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ffecbc0a-951e-4cae-9e94-49ddce94c050
Baiju Meswani (baijumeswani)
approved these changes
Nov 6, 2026
7 tasks done
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Port all SDK-relevant changes from bmehta001/onnxruntime#11 (source commit
611834d7eafd18ea0d4ca9ab28dfa3b539b3c61b) directly into the SDK, with regression tests adapted to this repository's harness.The native tests use a private collector URL on IPv4 loopback and the actual platform HTTP transport. They verify retry of the original reserved event, subsequent uploads, immediate teardown without retry after standard/nonstandard preparation failures, throwing HTTP handoff, and completion-then-throw. Each mode has a 30-second CTest timeout.
ORT-specific dependency-patch plumbing, telemetry wrapper/build integration, enable/disable behavior, and ORT documentation are intentionally not copied. The SDK now contains the fix itself rather than an external patch.
Validation
Fresh Debug builds with
MATSDK_WARNINGS_AS_ERRORS=ON, vendored SQLite/zlib, and the repository's pinned GoogleTest submodule:UnitTestsandUploadExceptionTestsupload-*native CTestsno-exceptions-smokebuild and CTestComponent selector:
TransmissionPolicyManagerTests.*:*UploadExceptionTests.*:HttpClientManager*.*. No selected tests were skipped or excluded; the two extra Linux cases exercise response-dispatcher fallback paths compiled only outside Windows.Formatting checks passed for the new native test and changed C++ implementation/test lines. Existing aligned header declarations were preserved.
git diff --cached --checkpassed before committing. No public package registries were used.This is targeted SDK/lifecycle validation, not a full SDK test-suite run or a reproduction of the reporter's COLMAP workload. No new sanitizer run was performed for this port. As in the source PR, microsoft/onnxruntime#33923's configuration-map SIGSEGV remains unconfirmed and is not claimed fixed.
Related: microsoft/onnxruntime#33929, microsoft/onnxruntime#33923 (non-closing references).