Skip to content

Keep 1DS SDK version and auth mutex alive for late exit workers - #33993

Merged
bmehta001 merged 2 commits into
microsoft:mainfrom
bmehta001:fix/telemetry-sdk-static-lifetimes
Nov 9, 2026
Merged

bmehta001 merged 2 commits into
microsoft:mainfrom
bmehta001:fix/telemetry-sdk-static-lifetimes

Conversation

@bmehta001

@bmehta001 bmehta001 commented Nov 8, 2026 •

Copy link
Copy Markdown
Contributor

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:

  • Return an allocation-free string literal from the internal PAL version accessor. The initial placement-new std::string solution avoided destruction but leaked its heap buffer whenever the embedding shared library unloaded.
  • Keep the auth-token mutex in aligned static placement-new storage, with guarded initialization and no registered destructor.

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

Check Result
Windows x64, Visual Studio 2026, Release Four lifetime tests and two-cycle SDK DLL unload/reload probe pass: 5/5
WSL Ubuntu 24.04, GCC, Debug with AddressSanitizer and detect_leaks=1 Same five checks pass: 5/5
Previous placement-new version implementation, Linux ASan unload/reload Reproduces CI's 56-byte leak in two allocations
Original SDK, before the initial PR All four lifetime negative controls fail with expected destruction diagnostics on Windows and Linux
SDK consumers Entire static SDK compiles on Windows and Linux, including decorator and HTTP encoder call sites
Patch application GNU patch --binary --ignore-whitespace -p1, dry run and application, plus normalized source-byte equality
Repository checks lintrunner --output oneline and git diff --check pass

Validation 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.SimError 405), not in a telemetry assertion. A fresh push queues validation of the corrected head; old-SHA jobs are not manually rerun.

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
Copilot AI balanced review requested due to automatic review settings November 8, 2026 22:19

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 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
@bmehta001

Copy link
Copy Markdown
Contributor Author

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.

@bmehta001

Copy link
Copy Markdown
Contributor Author

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.

@bmehta001 bmehta001 self-assigned this Nov 9, 2026
@bmehta001
bmehta001 enabled auto-merge (squash) November 9, 2026 07:23

@tianleiwu Tianlei Wu (tianleiwu) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 owner or auth mutex destroyed before the late exit owner diagnostic.
  • git diff --check passed.

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.

@bmehta001
bmehta001 merged commit b76411b into microsoft:main Nov 9, 2026
104 of 105 checks passed
Baiju Meswani (baijumeswani) pushed a commit to microsoft/onnxruntime-genai that referenced this pull request Nov 12, 2026
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants