Repository navigation
Keep 1DS SDK version and auth mutex alive for late exit workers - #33993
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.
…cies (#2926) Recreates #2912 from a branch in **microsoft/onnxruntime-genai**, rather than a fork. The commit history and validated head `e9611a71789a08dfe7b80e8b3be980fcefd53049` are unchanged. Original review discussion and fork-CI diagnosis remain available in #2912. ## Why Upstream the remaining GenAI fixes from microsoft/foundry-local#1305 and adopt the released **v3.10.310.1** SDK. 1. `AuthTokensController::GetMutex()` can be initialized after the telemetry owner's exit registration, then destroyed before that owner joins a late upload worker. 2. The existing heap-backed, placement-new PAL version string avoids destruction during shutdown, but loses its allocated buffer when its DLL/DSO is unloaded: two real unloads reproduce **56 bytes leaked in two allocations**. 3. The pinned SDK can omit canonical Apple SYSTEM SQLite/zlib targets when it detects vcpkg. ## Implementation - Generate patched copies of SDK `PAL.cpp`, internal `pal/PAL.hpp`, and `AuthTokensController.cpp`, leaving downloaded sources/headers untouched. Every expected definition/declaration/include/source-list replacement fails closed on source drift. - Return the SDK version as the literal `EVTSDK_VERSION_PREFIX "-" OS_NAME "-C++-" ECS_SUPP "-" BUILD_VERSION_STR`. Both the internal class method and inline wrapper return `const char*`. **No version allocation and no version destructor**; public GenAI/SDK APIs and C ABI are unchanged. - Resolve the generated header's PAL-local includes with SDK-rooted paths, and prioritize the generated header consistently for SDK sources and white-box tests. - Keep the auth mutex in aligned static byte storage initialized with placement new, with no destructor registered at exit. - Supply missing Apple `SQLite3::SQLite3` / `ZLIB::ZLIB` imported INTERFACE targets linking `sqlite3` / `z` before FetchContent configuration; preserve caller-supplied targets and properties. - Preserve the Apple PRIVATE `MATStreamingSessionDelegate=ORTGenAIMATStreamingSessionDelegate`, existing third-party warning guards, and test-source exclusions. ## Regression coverage / files - `cmake/telemetry.cmake`: allocation-free internal PAL getter/header, auth-mutex lifetime replacement, Apple dependency protection. - `test/cmake/telemetry-shutdown-tests.cmake`: hook literal getter entry while retaining old string destruction/access negative-control hooks; auth instrumentation; register dependency and actual unload/reload tests through the existing `test/CMakeLists.txt` inclusion. - `test/cpp/telemetry/telemetry-shutdown-tests.cpp`: detect destroyed auth-mutex use and require at least two auth accesses during production upload-at-exit, alongside existing decoration/encoding/completion checks. - `test/cmake/telemetry-dependencies/CMakeLists.txt`: absent/preserved/mixed Apple-vcpkg target cases, exact links/properties, generated getter declarations/literal, and unchanged downloaded header. - `test/cpp/telemetry/telemetry-version-reload-fixture.cpp`: real SDK getter exposed from a test DLL/DSO; use `std::string_view(...).data()` so the heap-backed negative control is supported without taking a string-object address. - `test/cpp/telemetry/telemetry-version-reload-tests.cpp`: check version content/stable data and **two genuine load/unload cycles**. On Linux, test builds use `-fno-gnu-unique` for MAT and fixture `--exclude-libs,ALL` / `-Bsymbolic`; the test explicitly rejects a still-loaded module rather than letting NODELETE mask leaks. ## Validation for b0db17a Focused standalone telemetry CMake harness (ignored local `build/validation`, not an inference build) compiles the **real** GenAI telemetry owner, device info, environment helper, real 1DS SDK, existing shutdown executable/instrumented SDK sources, and the checked-in reload fixture/runner. Only unrelated logging/inference plumbing is stubbed. Existing shutdown HTTP interception keeps real decoration, queuing, persistence, encoding, and worker joins without sending collector data. All dependencies/tooling were cached; `FETCHCONTENT_FULLY_DISCONNECTED=ON`. No public npm/PyPI/NuGet installs. Downloaded SDK PAL header/source were verified unchanged against tag v3.10.309.1. ### Windows x64, VS2026 Visual Studio **18 2026**, MSVC **19.51.36260.0**, Release, CMake 4.4.4. GenAI telemetry subset and new fixture/runner built with `/W4 /WX /EHsc`. ```text cmake --build build/win --config Release --target telemetry_shutdown_tests telemetry_version_reload_tests telemetry_shutdown_auth_negative telemetry_shutdown_version_negative --parallel 8 ctest --test-dir build/win -C Release -R "^(TelemetryShutdown-|TelemetryAppleDependencies-|TelemetryVersionReload$)" --output-on-failure ``` **16/16 passed**, 22.59 seconds: 11 shutdown modes, 4 dependency cases, and real DLL unload/reload. Independent local shutdown negative controls, same production `upload-at-exit` environment: - Revert auth storage only: **exit 21**, `SDK accessed its auth mutex after destruction`. - Revert version storage to a destructible static only: **exit 21**, `SDK read its version after string destruction`. ### WSL Ubuntu 24.04, GCC 13.3 ASan/LSan Ninja **Debug**, CMake 3.28.3; native WSL build directory `$HOME/build/genai-telemetry-lifetimes-asan`; real cached SDK/curl 8.21.0/mbedTLS 3.6.7. Both C and C++ configured with `-fsanitize=address -fno-omit-frame-pointer`; GenAI telemetry subset and new fixture/runner also use `-Wall -Wextra -Werror`. ```text cmake --build "$HOME/build/genai-telemetry-lifetimes-asan" --target telemetry_shutdown_tests telemetry_version_reload_tests telemetry_version_reload_negative telemetry_shutdown_auth_negative telemetry_shutdown_version_negative --parallel 6 env ASAN_OPTIONS=detect_leaks=1:halt_on_error=1 LSAN_OPTIONS=exitcode=23 ctest --test-dir "$HOME/build/genai-telemetry-lifetimes-asan" -C Debug -R '^(TelemetryShutdown-|TelemetryAppleDependencies-|TelemetryVersionReload$)' --output-on-failure ``` **16/16 passed**, 34.46 seconds, **no ASan/LSan errors**, no suppressions or disabled leak detection. **Unload negative control:** a separate real-SDK DSO changes only version storage back to aligned immortal `std::string` storage (same internal pointer-return ABI, auth fix retained). The checked-in runner confirms both unloads, then LSan reports **two 28-byte direct leaks, 56 bytes total**, **exit 23**. The fixed fixture completes the same cycles without leaks. No manual delete or sanitizer suppression is used. Both independent shutdown negative controls also give the expected **exit 21** on WSL with ASan/leak detection enabled. ### Lint - Existing `lintrunner --take CLANGFORMAT test/cpp/telemetry/telemetry-version-reload-fixture.cpp test/cpp/telemetry/telemetry-version-reload-tests.cpp`: passed before push. - Cached WSL clang-format 20.1.8 `--dry-run --Werror` on those files: passed. - `git diff --check`: passed. ## Limits / eventual removal - Apple/vcpkg cases validate configuration on Windows/Linux, not native Apple compilation/linking or Objective-C runtime behavior. The new actual unload regression targets Windows/Linux. - No full inference/model/mobile build was run locally. The DLL/DSO fixture uses real MAT, not a stand-in implementation. - Caller-supplied `MSTelemetry::mat` packages return before generated source fixes and **must already carry the lifetime fixes, including allocation-free version storage for unload safety**. This PR cannot repair an imported binary; that limitation remains documented in the module. - The previous ADO integration failures were skipped registry/CUDA/model-download prerequisites for this external-fork run, not telemetry execution failures; see the detailed diagnosis comment. This patch does not change fork permissions or relax those checks. - Upstream microsoft/cpp_client_telemetry#1560 (microsoft/cpp_client_telemetry#1560) is the eventual release path for removing these interim fixes. This PR now adopts SDK v3.10.310.1; the lifetime patches remain necessary, and the unreleased delegate option is not adopted. The version-literal approach aligns with microsoft/onnxruntime#33993 commit fbc111612d0d5865f2cd4a87d524192f98aea03f. ## SDK 3.10.310.1 update (e9611a7) - Update `cmake/deps.txt` to the released source archive with verified SHA1 `716407730e04b8fd60938c0715595a23d933d10f`. - Set the matching SDK `BUILD_VERSION` in `cmake/telemetry.cmake` and update the real unload/reload test's expected version. - Retain the PAL version/auth-mutex patches: the relevant upstream source/header files are byte-identical between 3.10.309.1 and 3.10.310.1. The release adds zlib 1.4 and upload-preparation exception fixes, not these lifetime fixes. Rebuilt the real SDK/GenAI telemetry subset against **actual 3.10.310.1 sources** using the same focused harness documented above. Windows x64 VS2026 Release: **17/17 CTest checks passed** (21.29 s). WSL Ubuntu/GCC Release: **17/17 passed** (24.59 s). Includes SDK version unload/reload, shutdown/reinitialization, actual SDK-decorated app/process identity, dependency configuration, and helper tests. No runtime test skips or new exclusions/suppressions were added. Existing third-party warning policies remain unchanged. Configured `lintrunner --take CLANGFORMAT` and `git diff --check` passed before push. The exact GitHub source archive was downloaded through the repository's existing dependency URL; subsequent builds used disconnected FetchContent and cached transitive sources, without public package-registry installs. This addition was not validated with native Apple/mobile/full inference builds or sanitizers; earlier ASan/LSan results above remain historical results for the previous SDK pin. ## Host application/process attribution (b074d51) Also carries the requested ORT/Foundry-style client identity addition from #2912: - Preserve SDK `AppInfo.Name` and populate it with the bounded host executable basename. - Emit the same basename as `processName` on GenAI `ProcessInfo`; never emit directories or command-line arguments. - Collect names on Windows, Apple, Linux, and Android while retaining the other existing context suppressions. - Check basename privacy, UTF-8 limits, and the real SDK-decorated `ext.app.name` and custom `processName`, including after logger reinitialization. Existing HTTP interception prevents collector transmission. The 3.10.310.1 Windows/WSL results above include these regressions. Live Grafana ingestion/aggregate wiring and adoption of the updated GenAI build remain separate; this PR does not reconstruct missing historical app/process values. Original implementation details: #2912 (comment) --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5484aed9-9464-4bc8-9ca7-fd67954034ff Copilot-Session: 520bb5e6-0788-4d36-8866-fde8e1763fd9 Copilot-Session: 0e4f5fef-a9b4-4125-8dab-723d468d6bca Copilot-Session: 996a8a25-fe6d-45c2-8b19-c38c57c9b7e9 Copilot-Session: 79b1d540-d1c1-4b67-9df5-ae03849328f8
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.