Repository navigation
Conversation
Prevent SDK-version storage and the auth-token mutex from being destroyed before an exit owner finishes its late workers. Extend the existing factory patch with aligned process-lifetime storage while keeping the pinned SDK version and platform dependency selection unchanged. Add deterministic destructor observers and main/threaded first-use cases that fail before destroyed-object access on the original SDK and pass with the patch, without constructing an upload transport. Changed files: - cmake/patches/cpp_client_telemetry/factory-static-storage.patch - cmake/onnxruntime_telemetry_tests.cmake - cmake/onnxruntime_telemetry_sdk_lifetime_tests.cmake - onnxruntime/test/platform/telemetry_1ds/telemetry_sdk_lifetime_test.cc - onnxruntime/test/platform/telemetry_1ds/README.md Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5484aed9-9464-4bc8-9ca7-fd67954034ff
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues remain for the changes preserving telemetry objects for late shutdown workers.
No open issues 🎉
What changed in this PR
Keeps the 1DS SDK version string and authentication mutex alive for late shutdown workers, extending ORT’s existing telemetry dependency patch without changing the SDK version or public APIs.
Changes:
- Uses aligned static placement-new storage for both objects while preserving the existing factory fix.
- Adds four isolated lifetime tests covering main-thread and application-thread first use, with destruction observers and instrumentation checks.
- Integrates the tests under existing native-desktop guards and documents how to run them.
| File | Description |
|---|---|
onnxruntime/test/platform/telemetry_1ds/telemetry_sdk_lifetime_test.cc |
Verifies object lifetime and identity during late-worker access. |
onnxruntime/test/platform/telemetry_1ds/README.md |
Documents lifetime coverage, constraints, and test commands. |
cmake/patches/cpp_client_telemetry/factory-static-storage.patch |
Prevents destruction of the SDK version string and auth mutex. |
cmake/onnxruntime_telemetry_tests.cmake |
Includes the new lifetime fixture under existing platform guards. |
cmake/onnxruntime_telemetry_sdk_lifetime_tests.cmake |
Instruments SDK sources, builds the fixture, and registers four CTests. |
Review effort: Balanced
Return literal storage from the internal PAL accessor instead of retaining a heap-backed string. The ASan reload test detected 56 bytes leaked across two unload cycles. Keep the auth-mutex lifetime fix and adapt deterministic lifetime instrumentation without disabling leak detection. Files: cmake/patches/cpp_client_telemetry/factory-static-storage.patch; cmake/onnxruntime_telemetry_sdk_lifetime_tests.cmake; onnxruntime/test/platform/telemetry_1ds/telemetry_sdk_lifetime_test.cc; onnxruntime/test/platform/telemetry_1ds/README.md Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5484aed9-9464-4bc8-9ca7-fd67954034ff
|
The cancelled React Native iOS E2E job exceeded its 90-minute job limit, but the log shows a simulator/bootstrap failure before the application was installed or launched: simctl bootstatus stalled, shutdown then returned CoreSimulator error 405 (device already Shutdown), and Jest retained an open simulator child process until cancellation. This is not an observed ORT/telemetry hang. Retried only the cancelled job on unchanged head fbc1116; successful jobs were not rerun. Linux Debug ASan and the other current-head checks passed. |
|
The isolated rerun passed: React Native CI iOS E2E job 129016386561 completed successfully on unchanged head fbc1116 (run 42539890294, attempt 2). The prior 90-minute cancellation was simulator/bootstrap infrastructure, not an observed telemetry or ORT hang. The current-head Linux Debug ASan check also passed. |
Tianlei Wu (tianleiwu)
left a comment
There was a problem hiding this comment.
Reviewed commit fbc1116. No actionable findings.
The version accessor's declarations and SDK callers are consistent with the literal return value, which preserves the version text without a destructible object or retained heap buffer. The auth mutex retains guarded initialization in aligned static storage without registering a destructor.
Independent validation on Linux x64 with GCC 13.2:
- Downloaded the exact pinned v3.10.309.1 archive, verified its SHA-1 against
cmake/deps.txt, and applied the committed patch successfully. - Built the complete patched static SDK and included the committed lifetime-test CMake fixture in a standalone harness. All four lifetime CTests passed with AddressSanitizer and
ASAN_OPTIONS=detect_leaks=1. - Built the same fixture against the original SDK. All four negative controls failed with the intended
SDK version destroyed before the late exit ownerorauth mutex destroyed before the late exit ownerdiagnostic. git diff --checkpassed.
Validation scope: this was a standalone SDK/fixture build, not a full ORT integration build; I did not independently rerun Windows/macOS or the real-transport unload/reload tests. The local ASan executables were linked with -no-pie because intermittent sanitizer startup crashes also reproduced in an empty executable on this host.
Rationale
ORT's pinned cpp_client_telemetry v3.10.309.1 gives the SDK version and auth-token mutex destructible function-local storage. Late workers can access these objects after they retire when telemetry is first enabled after its exit owner is registered.
Extend the existing factory static-storage patch while preserving its factory fix:
std::stringsolution avoided destruction but leaked its heap buffer whenever the embedding shared library unloaded.The PAL class and inline helper declarations are updated together; all SDK consumers are rebuilt. Installed public SDK APIs, dependency versions, Apple dependency selection, and Objective-C delegate names are unchanged.
Related upstream work is microsoft/cpp_client_telemetry#1560, originating from microsoft/foundry-local#1305. Remove the interim hunks only after verifying that an adopted SDK release handles both shutdown ordering and library-unload allocations.
Regression coverage
Four isolated native-desktop CTests cover version and mutex access with main-thread or application-thread first use. An exit owner registered before first use joins a late worker that checks version-data address/value/build suffix or locks the same mutex. Test-only destruction observers fail before destroyed-object access. Instrumentation supports the original destructible version and the patched literal accessor, and validates construction/access hook counts.
The existing real-transport reload test remains unchanged, including LeakSanitizer detection. No telemetry suppression is relaxed, no test is skipped, and no production transport override is introduced.
Validation
detect_leaks=1patch --binary --ignore-whitespace -p1, dry run and application, plus normalized source-byte equalitylintrunner --output onelineandgit diff --checkpassValidation uses an ignored standalone harness building the matching SDK and including the committed lifetime-test CMake fixture. The unload probe loads and genuinely unloads an SDK-backed shared library twice; its source is validation-only, not a production change. Cached dependencies are reused.
Full ORT CI remains the final integration check. The other initial failure, React Native iOS E2E, occurred during simulator uninstall/shutdown before app execution (
CoreSimulator.SimError405), not in a telemetry assertion. A fresh push queues validation of the corrected head; old-SHA jobs are not manually rerun.