Repository navigation
Fix Windows network detection leak, add periodic leak reports, and standardize dependencies - #1536
Conversation
Track Windows and Linux leak counts without making the known baseline block unrelated changes. Pin and verify Dr. Memory, retain raw reports, and publish per-scenario summaries for unit tests, functional tests, and SampleCppMini. Files changed: - .github/workflows/memory-leak-analysis.yml - .github/scripts/run-drmemory.ps1 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: df9f344e-e064-404e-ae6f-c0ef455d747c
Run the expensive analysis only when its workflow or helper changes, so this PR and future maintenance updates exercise both hosted platforms before merge. Files changed: - .github/workflows/memory-leak-analysis.yml Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: df9f344e-e064-404e-ae6f-c0ef455d747c
Build Linux targets without entering the package deployment path, and ignore Dr. Memory's incomplete Windows bootstrap report while retaining it in the raw artifact. Files changed: - .github/scripts/run-drmemory.ps1 - .github/workflows/memory-leak-analysis.yml Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: df9f344e-e064-404e-ae6f-c0ef455d747c
Install the Linux curl development dependency, avoid the unrelated installed-package target regression when compiling the sample, and exclude the one functional assertion whose exact asynchronous drop count changes under instrumentation. Files changed: - .github/workflows/memory-leak-analysis.yml Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: df9f344e-e064-404e-ae6f-c0ef455d747c
Recognize Dr. Memory's clean-report marker and disambiguate SampleCppMini's signed 64-bit EventProperty construction so the same sample compiles under GCC and MSVC. Files changed: - .github/scripts/run-drmemory.ps1 - examples/cpp/SampleCppMini/main.cpp Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: df9f344e-e064-404e-ae6f-c0ef455d747c
Use Windows.Networking.Connectivity for both cost queries and change notifications so network detection preserves behavior without instantiating PublicNetworkListManager or loading netprofm.dll. Fail periodic leak analysis if netprofm returns. Files changed: - lib/pal/desktop/NetworkDetector.cpp - lib/pal/desktop/NetworkDetector.hpp - .github/workflows/memory-leak-analysis.yml Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: df9f344e-e064-404e-ae6f-c0ef455d747c
Use scoped objects for temporary event and buffer allocations, and destroy the log-session provider during fixture teardown so leak reports represent SDK behavior rather than test fixture ownership. Files changed: tests/unittests/AnnexKTests.cpp tests/unittests/LogSessionDataDBTests.cpp tests/unittests/TransmissionPolicyManagerTests.cpp Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: df9f344e-e064-404e-ae6f-c0ef455d747c
Exercise the real Windows detector so CI proves that WinRT status registration starts, network cost remains valid, shutdown completes, and netprofm.dll is not loaded. Files changed: tests/unittests/NetworkDetectorTests.cpp tests/unittests/CMakeLists.txt tests/unittests/UnitTests.vcxproj Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: df9f344e-e064-404e-ae6f-c0ef455d747c
Delete the unused NLM interfaces, connection points, callbacks, maps, compatibility branch, and manual reference counting now that network detection is entirely WinRT-based. This reduces object and binary overhead while keeping ownership with unique_ptr. Files changed: docs/building-custom-SKU.md lib/pal/desktop/NetworkDetector.cpp lib/pal/desktop/NetworkDetector.hpp lib/pal/desktop/WindowsDesktopNetworkInformationImpl.cpp Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: df9f344e-e064-404e-ae6f-c0ef455d747c
Avoid spending hosted runner time when main has not changed while preserving manual analysis on demand. Files changed: - .github/workflows/memory-leak-analysis.yml Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: df9f344e-e064-404e-ae6f-c0ef455d747c
There was a problem hiding this comment.
🟡 Changes recommended
Network-cost behavior regresses for approaching-limit connections, and the leak-analysis coverage has gaps.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Replaces leaking Windows Network List Manager usage with WinRT connectivity APIs and adds automated Dr. Memory reporting.
Changes:
- Migrates Windows network-cost detection and adds regression coverage.
- Adds Windows/Linux leak-analysis workflows with verified tooling and retained reports.
- Fixes test leaks and portability issues.
File summaries
| File | Description |
|---|---|
tests/unittests/UnitTests.vcxproj |
Adds network detector tests. |
tests/unittests/TransmissionPolicyManagerTests.cpp |
Uses stack-owned event contexts. |
tests/unittests/NetworkDetectorTests.cpp |
Tests WinRT detector lifecycle. |
tests/unittests/LogSessionDataDBTests.cpp |
Releases the session provider. |
tests/unittests/CMakeLists.txt |
Includes Windows detector tests. |
tests/unittests/AnnexKTests.cpp |
Adds RAII for allocated buffers. |
lib/pal/desktop/WindowsDesktopNetworkInformationImpl.cpp |
Removes obsolete COM reference counting. |
lib/pal/desktop/NetworkDetector.hpp |
Defines the simplified WinRT detector. |
lib/pal/desktop/NetworkDetector.cpp |
Implements WinRT cost monitoring. |
examples/cpp/SampleCppMini/main.cpp |
Makes integer width explicit. |
docs/building-custom-SKU.md |
Documents WinRT network detection. |
.github/workflows/memory-leak-analysis.yml |
Adds cross-platform leak-analysis jobs. |
.github/scripts/run-drmemory.ps1 |
Runs Dr. Memory and summarizes results. |
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Restore approaching-data-limit handling in the WinRT cost mapping, verified in lib/pal/desktop/NetworkDetector.cpp. Check netprofm.dll while the detector is active, verified in tests/unittests/NetworkDetectorTests.cpp. Include APITest.C_API_Test in Linux leak analysis after confirming the test passes on Linux, verified in .github/workflows/memory-leak-analysis.yml. Files changed: - .github/workflows/memory-leak-analysis.yml - lib/pal/desktop/NetworkDetector.cpp - tests/unittests/NetworkDetectorTests.cpp Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: df9f344e-e064-404e-ae6f-c0ef455d747c
There was a problem hiding this comment.
🟡 Changes recommended
The asynchronously updated network-cost cache has an unsynchronized read/write data race.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 1
- Review effort level: Balanced
Store network cost and running state atomically so WinRT callbacks cannot race caller reads. Return the cached cost by value instead of exposing a concurrently updated reference. Verified at: - lib/pal/desktop/NetworkDetector.cpp - lib/pal/desktop/NetworkDetector.hpp Files changed: - lib/pal/desktop/NetworkDetector.cpp - lib/pal/desktop/NetworkDetector.hpp Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: df9f344e-e064-404e-ae6f-c0ef455d747c
There was a problem hiding this comment.
🔵 Needs a closer look
The workflow contains an ignored build property, and the new network-cost test does not validate cost mapping or updates.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
.github/workflows/memory-leak-analysis.yml:67
MATSDK_USE_WININETis not consumed by any project, props, targets, or source in this repository, so this MSBuild property is silently ignored; the Win32 factory still selects WinInet (lib/http/HttpClientFactory.hpp:27-29). Remove the no-op argument, or wire the intended transport selection into the build before relying on it for this analysis.
tests/unittests/NetworkDetectorTests.cpp:25- This assertion accepts every possible
NetworkCost, so an implementation that always returnsUnknownstill passes and the test does not verify the stated preservation of metered-cost behavior. Add a mocked/injectable WinRT source (or extract the mapping helper) and assert unrestricted, fixed/variable, and roaming/limit mappings, including a status-change refresh.
- Files reviewed: 13/13 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Remove the ignored MATSDK_USE_WININET build property so leak analysis does not imply a transport selection it never made. Extract and test WinRT cost mapping for unrestricted, metered, roaming, over-limit, and approaching-limit states, and verify synchronous refresh updates the cache. Verified at: - .github/workflows/memory-leak-analysis.yml - lib/pal/desktop/NetworkDetector.cpp - tests/unittests/NetworkDetectorTests.cpp Files changed: - .github/workflows/memory-leak-analysis.yml - lib/pal/desktop/NetworkDetector.cpp - lib/pal/desktop/NetworkDetector.hpp - tests/unittests/NetworkDetectorTests.cpp Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: df9f344e-e064-404e-ae6f-c0ef455d747c
There was a problem hiding this comment.
🟡 Changes recommended
Callback teardown has a potential use-after-free, and the DLL regression gate does not reliably detect module loading.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
.github/workflows/memory-leak-analysis.yml:115
- This does not establish that
netprofm.dllwas never loaded. Dr. Memory'sresults.txtonly mentions modules that appear in reported errors/stacks, while the module-loading diagnostics are written toglobal.<pid>.log(and require suitable verbosity); the*.txtfilter excludes those logs. Thus a run that loadsnetprofm.dllwithout a report involving it passes this advertised regression gate. Enable module-load logging and inspect the global logs, or instrument each target to query its loaded modules directly.
- Files reviewed: 13/13 changed files
- Comments generated: 1
- Review effort level: Balanced
Keep per-subscription callback state alive independently, reject callbacks after shutdown starts, and wait for active callbacks before releasing detector resources. Inspect Dr. Memory global module logs for netprofm.dll and require logs for every Windows scenario so the regression gate cannot pass vacuously. Verified at: - lib/pal/desktop/NetworkDetector.cpp - .github/workflows/memory-leak-analysis.yml Files changed: - .github/workflows/memory-leak-analysis.yml - lib/pal/desktop/NetworkDetector.cpp - lib/pal/desktop/NetworkDetector.hpp Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: df9f344e-e064-404e-ae6f-c0ef455d747c
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The detector invokes a Windows 8+ API without preserving the repository’s Windows 7 compatibility guard.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
Open (1)
Resolve the Windows workflow conflict while preserving CMake setup and the WinHTTP/WinInet matrix. Set the desktop API floor to Windows 8.1 and remove the pre-8.1 WinHTTP proxy fallback so CI enforces the supported contract. Files changed: merged upstream main; .github/workflows/test-win-latest.yml; README.md; lib/CMakeLists.txt; lib/http/HttpClient_WinHttp.cpp; Solutions Windows project files; tests/headers/check_public_headers.cmd. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Version and validate the Android CMake/NDK marker so stale setup state cannot hide missing tools. Provision CMake for the Linux no-exceptions job that failed under the runner's 3.31 release. Raise desktop builds and header gates to the Windows 10 API floor. Remove the Windows 7 runtime probe, hand-defined network-cost GUID, and obsolete warning suppressions in favor of the SDK IID. Files changed: Android and Linux setup paths, Windows workflows/docs/project definitions, WinHTTP and network detection sources, and the public-header gate. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: fa477318-3257-45cd-8711-d5214b5cb119
Expose the public mbedTLS threading macros to curl so both dependencies compile public context types with identical layouts and avoid an entropy-context overflow. Files changed: cmake/MatsdkFetchCurl.cmake. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: fa477318-3257-45cd-8711-d5214b5cb119
Added commands to install Android SDK platforms and sources. Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Add uuid to the Windows target dependencies so SDK-declared COM GUID symbols resolve for CMake consumers instead of relying on toolchain defaults. Files changed: - lib/CMakeLists.txt: propagate the Windows UUID import library through mat. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: c72c9f67-f709-4c28-a8fa-809e0aefc14d
…endencies' into bhamehta/standardize-telemetry-dependencies
Disable curl's build-host CA auto-detection and remove generated CA path macros so redistributable Linux binaries rely on the target host's runtime CA selection. Files changed: - cmake/MatsdkFetchCurl.cmake: sanitize fetched curl CA defaults and enforce that no build-time path remains. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: c72c9f67-f709-4c28-a8fa-809e0aefc14d
Integrate PR microsoft#1537 into PR microsoft#1536 so the dependency and platform updates ship with the leak-analysis work. Resolve the network detector overlap in favor of the leak-safe WinRT lifecycle, narrow the SEH warning suppressions, and consistently enforce the Windows 10 API floor without legacy Windows fallbacks. Files changed: Windows workflows/projects/docs, CMake dependency setup, Android build setup, WinHTTP transport, and WinRT network detection. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d2b55402-0dec-4ad7-bf0f-d30d92c96171
A live upload to external collector endpoints can fail or stall on CI without indicating a certificate-policy regression. Verify that both Windows transports apply the log configuration directly; retain the separate cold-session HTTPS test for real certificate enforcement. Files changed: tests/functests/APITest.cpp Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d2b55402-0dec-4ad7-bf0f-d30d92c96171
SurvivesManyRequests leaked all 100 SDK-created request objects, overflowing Dr. Memory's indirect-byte summary and failing Linux CI parsing. Keep the requests alive through their terminal callbacks and release them at test exit instead of relaxing the leak gate. Files changed: tests/unittests/HttpClientTests.cpp Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d2b55402-0dec-4ad7-bf0f-d30d92c96171
The Windows 10 API-floor label renamed previously required Win32 and x64 Release checks, leaving their old contexts without runs even though the equivalent builds passed. Keep the Windows 10 build settings and restore stable CI job names so the two release checks run again. Files changed: .github/workflows/test-win-latest.yml Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d2b55402-0dec-4ad7-bf0f-d30d92c96171
build-android.cmd, tools/setup-buildtools-android.cmd: store normalized LF text in Git while preserving CRLF Windows checkouts. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Baiju Meswani (baijumeswani)
left a comment
There was a problem hiding this comment.
The earlier correctness concerns are addressed and the current CI checks pass. I have one non-blocking baseline follow-up.
| @@ -0,0 +1,7 @@ | |||
| Platform,Scenario,UniqueLeaks,TotalLeaks,LeakBytes,UniquePossibleLeaks,TotalPossibleLeaks,PossibleLeakBytes,UniqueReachable,TotalReachable,ReachableBytes | |||
| Windows,unit-tests,10,113,4256,14,15,7206,467,644,227717 | |||
There was a problem hiding this comment.
Non-blocking: the latest successful run is already above this baseline for 8 Linux unit-test metrics and 4 Windows unit-test metrics. For example, possible-leak bytes increased from 3,452 to 208,600 on Linux and from 7,206 to 225,649 on Windows. Please check whether these increases are expected after the recent test and dependency changes. Then run the jobs again to confirm the numbers are stable and update this baseline with a short explanation if needed. This will keep future warnings useful.
### Description <!-- Describe your changes. --> - Delegate Linux curl/mbedTLS construction to the 1DS SDK instead of maintaining a duplicate ORT builder; retain the static-package curl export fix. - Use system SQLite and libz on Apple (with process-safe SQLite lifecycle), minimal private SQLite and vendored zlib on other non-Windows source builds, and avoid unnecessary vcpkg SQLite/zlib packages on Apple. - Preserve Apple static-package system-library and CMake dependency metadata. - Merge current `main` and keep cpp_client_telemetry v3.10.267.1. The SDK now includes the upstream curl, threading, CA-path, and logging fixes, so the old compatibility patch is removed. ### Motivation and Context <!-- - Why is this change required? What problem does it solve? - If it fixes an open issue, please link to the issue here. --> Keep one owner for the Linux 1DS transport and avoid shipping an extra process-global SQLite copy on Apple. The dependency work from microsoft/cpp_client_telemetry#1536 is included in the pinned SDK release; this PR carries only the ORT integration that remains necessary. ### Validation - `lintrunner cmake/deps.txt cmake/external/onnxruntime_external_deps.cmake cmake/CMakeLists.txt cmake/onnxruntime.cmake cmake/onnxruntime_common.cmake cmake/vcpkg-ports/cpp-client-telemetry/vcpkg.json onnxruntime/core/platform/posix/telemetry.cc` — passed on Windows. - `clang-format --dry-run --Werror onnxruntime/core/platform/posix/telemetry.cc` — passed under WSL. - `git diff origin/main --check` — passed on Windows. - No full build or test suite run after the SDK update; platform CI is pending. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2d3afa07-bc40-4851-bc49-ccf0a37e6da0 Copilot-Session: 4e6ddf54-85d1-4f61-b358-aa6b56895ddb

Summary
PublicNetworkListManageruse withWindows.Networking.Connectivity, preserving metered-network cost updates without loading the leakingnetprofm.dllmain, with manual dispatch available on demandnetprofm.dllis loaded againValidation
netprofm.dllregression gateKnown instrumentation exclusion
BasicFuncTests.killSwitchWorksremains covered by normal CI but is excluded under Dr. Memory because instrumentation changes its exact asynchronous drop count (400 observed versus 100 expected).OfflineStorageTests_SQLite.StoreThousandEventsTakesLessThanASecondremains covered by normal CI but is excluded under Dr. Memory because instrumentation invalidates its one-second wall-clock performance threshold (1.384 seconds observed).Closes #634
Related external evidence: microsoft/onnxruntime-genai#2590