Repository navigation
Conversation
Retain the SDK version and auth-token lock through exit-owner teardown. Honor Apple system SQLite/zlib even with vcpkg, and let independent embedded copies select a private Objective-C HTTP delegate name without changing the default. Files: CMakeLists.txt; cmake/MatsdkOptions.cmake; docs/embedding-with-cmake.md; lib/CMakeLists.txt; lib/api/AuthTokensController.cpp; lib/pal/PAL.cpp; tests/apple/embedding-options-tests.cmake; tests/apple/embedding-options/CMakeLists.txt; tests/embedding/CMakeLists.txt; tests/embedding/lifetime-tests.cmake; tests/embedding/lifetime-tests.cpp Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5484aed9-9464-4bc8-9ca7-fd67954034ff
bmehta001
changed the title
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The iOS guard ignores custom delegate names when Apple HTTP is disabled, risking process-global class collisions, and the new embedding test requires CMake 3.19 despite supporting 3.15–3.18.
2 open issues
What changed in this PR
Prepares SDK release 3.10.312.1 and improves shutdown safety and coexistence for independently embedded SDK copies.
Changes:
- Keeps SDK-version and auth-token mutex storage alive for late shutdown workers.
- Uses Apple system SQLite/zlib under vcpkg and adds a configurable private HTTP delegate name.
- Adds lifetime regressions and an 18-case Apple configuration matrix.
| File | Description |
|---|---|
| tests/embedding/lifetime-tests.cpp | Checks state identity and availability during shutdown. |
| tests/embedding/lifetime-tests.cmake | Instruments production sources and registers lifetime tests. |
| tests/embedding/CMakeLists.txt | Integrates lifetime and Apple configuration tests. |
| tests/apple/embedding-options/CMakeLists.txt | Validates dependency targets and delegate definitions. |
| tests/apple/embedding-options-tests.cmake | Runs the 18-case configuration matrix. |
| Package.swift | Updates the binary artifact reference to 3.10.310.1. |
| lib/pal/PAL.cpp | Preserves SDK-version storage through process exit. |
| lib/include/public/Version.hpp | Advances SDK version constants to 3.10.312.1. |
| lib/CMakeLists.txt | Applies the private Apple delegate-name definition. |
| lib/api/AuthTokensController.cpp | Preserves the auth-token mutex through process exit. |
| docs/embedding-with-cmake.md | Documents Apple dependencies and delegate naming. |
| CMakeLists.txt | Resolves Apple system libraries with vcpkg active. |
| cmake/MatsdkOptions.cmake | Adds and validates the delegate-class option. |
Review effort: Balanced
| message(STATUS "Building static SDK library") | ||
| add_library(mat STATIC ${SRCS}) | ||
| endif() | ||
| if(APPLE AND MATSDK_BUILD_APPLE_HTTP) |
| enable_testing() | ||
| add_test(NAME embedding_test COMMAND embedding_test) | ||
| include("${CMAKE_CURRENT_LIST_DIR}/lifetime-tests.cmake") | ||
| add_test(NAME apple_embedding_options |
The placement-new string avoided exit-time destruction but abandoned its heap buffer on unload: two real reload cycles leak 56 bytes under LSan. Return the compile-time literal through the internal const-char accessor instead, preserving late-worker safety without allocation or destruction. Keep the auth mutex and existing Apple embedding fixes unchanged. Files: - lib/pal/PAL.cpp, lib/pal/PAL.hpp: allocation-free internal version accessor. - tests/embedding/lifetime-tests.cpp: string-view value/address expectations. - tests/embedding/lifetime-tests.cmake: entry instrumentation and real DSO test. - tests/embedding/version-reload-fixture.cpp: embed the production SDK archive. - tests/embedding/version-reload-test.cpp: verify two actual unload cycles. - tests/embedding/CMakeLists.txt: opt-in Linux ASan/LSan coverage. - .github/workflows/test-embedding.yml: enforce sanitizer coverage in CI. Validation: Windows VS2026 and Linux GCC13 ASan/LSan embedding suites each pass all six tests. The old placement-new buffer leaks exactly 56 bytes; original destructible statics fail all three lifetime tests on both OSes. Formatting, existing misspell workflow and git diff --check pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5484aed9-9464-4bc8-9ca7-fd67954034ff
bmehta001
added a commit
to microsoft/onnxruntime
that referenced
this pull request
Nov 9, 2026
## 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. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5484aed9-9464-4bc8-9ca7-fd67954034ff
Repeat 25% trims while the database remains oversized, stopping if resizing fails or makes no progress. Large flushes can otherwise leave the database above its limit and fail the iOS functional test. Files changed: lib/offline/OfflineStorage_SQLite.cpp; tests/unittests/OfflineStorageTests_SQLite.cpp Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2b4dcf34-18a6-4f77-9013-1db2fe20cc40
Restore upstream's cached BZip2 capability contract and pkg-config defaults instead of carrying SDK-unrelated dependency naming and configuration policy. Keep demonstrated gzip/ZIP runtime fixes, archive regressions, SDK shims, and the focused minizip include-directory, version, and large-file checks. Remove tests tied to the reverted packaging policies; preserve the original package consumers and shared/static archive tests. Files changed: - zlib/contrib/minizip/CMakeLists.txt - zlib/contrib/minizip/minizip.pc.txt - zlib/contrib/minizip/minizipConfig.cmake.in - zlib/contrib/minizip/test/CMakeLists.txt - zlib/contrib/minizip/test/find_package_test.cmake.in - zlib/contrib/minizip/test/pkg-config-test.cmake.in (removed) Validation: Windows minizip 11/11 and SDK embedding 6/6; Linux minizip 11/11 with BZip2 OFF and 11/11 ON, ASan/LSan embedding 6/6; existing misspell workflow and git diff --check. No remaining tests skipped. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 976e8fb6-5cab-4bb8-955e-9eeb6f8d44d9
This branch has not been deployed
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
const char*accessor: no allocation, destructor, or leaked buffer on library unload. Keep the auth mutex in aligned static placement-new storage so late upload workers remain safe; normal SDK worker/resource teardown is unchanged.MATSDK_APPLE_HTTP_DELEGATE_CLASSfor independently embedded Apple SDK copies. The default class name is unchanged; the definition is private to Apple's HTTP implementation and does not rename public wrapper classes.Vendored minizip cleanup
minizip_HAS_BZIP2contract and upstream pkg-config metadata. Remove custom imported-library filename/configuration reconstruction, dependency-path injection, and redundant check-state handling.fopen64with the same large-file definition used to compile it, use minizip's own library version, and export the actualinclude/minizipheader directory rather than two competing include roots.Cleanup validation
git diff --checkpass. No tests in these remaining suites were skipped or filtered; the deleted tests belong to the deliberately reverted packaging behavior.SDK-version leak follow-up (
e630e2c9d11176499856393ffac9179520c80da6)std::stringretained a 28-byte heap buffer per unloaded SDK copy. Two actual unload/reload cycles reproduce a 56-byte LSan leak. A string literal removes the allocation while retaining late-worker safety.PAL.hppis internal, not an installed public API; SDK consumers compile on both Windows and Linux.std::string_viewcontents and.data()identity. Preserve destructor-observing instrumentation for original-SDK negative controls.sdk_version_reload: load a shared fixture containing the real static SDK archive, validate the SDK version, verify actual unload, and repeat. Linux disables GNU-unique pinning in the test archive and uses local symbol binding for the fixture.Follow-up validation
cmake --build out/lifetime-windows --config Release --target embedding_test --parallel 6;ctest --test-dir out/lifetime-windows -C Release --output-on-failure: 6/6 pass./home/bhamehta/build/1ds-upstream-lifetimes, configured with-DMATSDK_EMBEDDING_ADDRESS_SANITIZER=ON: build targetembedding_test; unfiltered CTest: 6/6 pass, including real unload/reload under ASan/LSan.ctest -R '^sdk_version_reload$'fails with 56 bytes leaked in 2 allocations. Restore the literal implementation: passes.ctest -R '^sdk_lifetime_'fails 3/3 with expected destruction diagnostics on Windows and Linux. Restore production fixes: all pass.clang-format --dry-run --Werroron all changed/new C++ tests; changed-linegit clang-format --diff HEADon PAL sources; complete existing cached misspell workflow;git diff --check: all pass.Earlier validation (before the leak follow-up)
git diff --checkpass before push.Scope and limitations