Skip to content

Fix Windows network detection leak and add periodic leak reports - #1536

Open
bmehta001 wants to merge 18 commits into
microsoft:mainfrom
bmehta001:bhamehta/periodic-memory-leak-analysis
Open

Fix Windows network detection leak and add periodic leak reports#1536
bmehta001 wants to merge 18 commits into
microsoft:mainfrom
bmehta001:bhamehta/periodic-memory-leak-analysis

Conversation

@bmehta001

@bmehta001 bmehta001 commented Sep 18, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

  • replace desktop PublicNetworkListManager use with Windows.Networking.Connectivity, preserving metered-network cost updates without loading the leaking netprofm.dll
  • run Dr. Memory analysis on Windows and Linux after relevant changes reach main, with manual dispatch available on demand
  • analyze unit tests, functional tests, and SampleCppMini with pinned, SHA-256-verified tooling
  • publish leak counts to the job summary and retain raw reports for 90 days
  • keep the current leak baseline non-gating while failing explicitly on instrumentation/target errors
  • fail Windows leak analysis if netprofm.dll is loaded again

Validation

  • Visual Studio 2026 x64 Debug build of UnitTests and SampleCppMini
  • focused Windows network/configuration tests: 31 passed
  • GCC/WSL syntax check of SampleCppMini
  • actionlint 1.7.12 and PowerShell parser validation
  • hosted Dr. Memory runs on Windows and Linux, including the netprofm.dll regression gate

Known instrumentation exclusion

  • BasicFuncTests.killSwitchWorks remains 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.StoreThousandEventsTakesLessThanASecond remains 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

@bmehta001
Add periodic memory leak reporting
f64a748 
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
@bmehta001
Validate leak workflow changes on pull requests
c64c35a 
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
@bmehta001
bmehta001 requested a review from a team as a code owner September 18, 2026 01:07
@bmehta001
Repair hosted leak analysis execution
c12d725 
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
@bmehta001
Stabilize hosted leak scenarios
fe74bd7 
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
@bmehta001
Make the sample portable under leak analysis
ba167fd 
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
@bmehta001
Stop loading the leaking Windows Network List Manager
c1888e5 
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
@bmehta001 bmehta001 changed the title Add periodic Windows and Linux memory leak reports Fix Windows network detection leak and add periodic leak reports Sep 18, 2026
bmehta001 and others added 4 commits September 18, 2026 03:16
@bmehta001
Release memory allocated by unit tests
f693154 
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
@bmehta001
Verify WinRT network detection lifecycle
77d6581 
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
@bmehta001
Remove obsolete Network List Manager state
e948a2c 
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
@bmehta001
Run leak analysis only after relevant changes
6c979a2 
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
@bmehta001 bmehta001 self-assigned this Sep 18, 2026
@bmehta001
bmehta001 requested a balanced review from Copilot September 18, 2026 14:32
Copilot AI reviewed Sep 18, 2026
View reviewed changes

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.

🟡 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.

Comment thread .github/workflows/memory-leak-analysis.yml Outdated
Comment thread lib/pal/desktop/NetworkDetector.cpp
Comment thread tests/unittests/NetworkDetectorTests.cpp
@bmehta001
Address Copilot leak-review findings
a266054 
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
@bmehta001
bmehta001 requested a balanced review from Copilot September 18, 2026 18:12
Copilot AI reviewed Sep 18, 2026
View reviewed changes

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.

🟡 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

Comment thread lib/pal/desktop/NetworkDetector.hpp Outdated
@bmehta001
Synchronize cached network detector state
3192701 
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
@bmehta001
bmehta001 requested a balanced review from Copilot September 18, 2026 18:28
Copilot AI reviewed Sep 18, 2026
View reviewed changes

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.

🔵 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_WININET is 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 returns Unknown still 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

@bmehta001
Cover WinRT network cost mapping
88c7338 
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
@bmehta001
bmehta001 requested a balanced review from Copilot September 18, 2026 18:42
Copilot AI reviewed Sep 18, 2026
View reviewed changes

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.

🟡 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.dll was never loaded. Dr. Memory's results.txt only mentions modules that appear in reported errors/stacks, while the module-loading diagnostics are written to global.<pid>.log (and require suitable verbosity); the *.txt filter excludes those logs. Thus a run that loads netprofm.dll without 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

Comment thread lib/pal/desktop/NetworkDetector.cpp Outdated
@bmehta001
Synchronize WinRT callback teardown
355b250 
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
@bmehta001
bmehta001 requested a balanced review from Copilot September 18, 2026 18:58
Copilot AI reviewed Sep 18, 2026
View reviewed changes

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.

🟡 Changes recommended

Network callback shutdown has unresolved deadlock and exceptional-cleanup hazards.

Get a fresh assessment by requesting another Copilot review.

Review details
  • Files reviewed: 13/13 changed files
  • Comments generated: 2
  • Review effort level: Balanced

Comment thread lib/pal/desktop/NetworkDetector.cpp Outdated
Comment thread lib/pal/desktop/NetworkDetector.cpp
@bmehta001
Decouple WinRT callbacks from detector lifetime
68f5276 
Capture COM and atomic cache state by shared ownership so an in-flight notification never dereferences a destroyed detector and teardown does not wait on a callback that can synchronously initiate shutdown.

Run Reset from an SEH finally path so subscriptions are disabled and released even when Windows Runtime raises a structured exception.

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
@bmehta001
bmehta001 requested a balanced review from Copilot September 18, 2026 19:16
Copilot AI reviewed Sep 18, 2026
View reviewed changes

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.

🔵 Needs a closer look

Windows event-lifecycle concurrency and new cross-platform instrumentation warrant final human verification.

Review details
  • Files reviewed: 13/13 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

@bmehta001
Exclude instrumented SQLite timing assertion
ffc696e 
Dr. Memory changes execution speed and caused the one-second storage benchmark to fail at 1.384 seconds even though its functional operations succeeded. Keep the benchmark in normal CI while excluding only that wall-clock assertion from leak analysis.

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
@bmehta001
bmehta001 requested a balanced review from Copilot September 18, 2026 20:14
Copilot AI reviewed Sep 18, 2026
View reviewed changes

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.

🔵 Needs a closer look

The listener uses WinRT objects after initializing only classic COM rather than the required Windows Runtime apartment.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

lib/pal/desktop/NetworkDetector.cpp:234

  • The listener now activates and uses Windows Runtime objects, but this initializes only classic COM. Microsoft requires threads that activate/interact with WinRT objects to be initialized with RoInitialize (or Windows::Foundation::Initialize); relying on CoInitialize can make GetActivationFactory/later WinRT calls fail on supported systems. Initialize with RoInitialize(RO_INIT_MULTITHREADED) and balance successful calls with RoUninitialize instead of CoUninitialize.
  • Files reviewed: 13/13 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

@bmehta001
Initialize the WinRT listener apartment
6da670a 
Use RoInitialize with the multithreaded apartment and balance successful initialization with RoUninitialize so Windows Runtime activation is valid on every supported Windows target.

Verified at:
- lib/pal/desktop/NetworkDetector.cpp

Files changed:
- lib/pal/desktop/NetworkDetector.cpp

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: df9f344e-e064-404e-ae6f-c0ef455d747c
@bmehta001
bmehta001 requested a balanced review from Copilot September 18, 2026 20:26
Copilot AI reviewed Sep 18, 2026
View reviewed changes

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.

🟡 Changes recommended

The new detector tests fail to compile for supported custom Windows configurations that disable HAVE_MAT_NETDETECT.

Get a fresh assessment by requesting another Copilot review.

Review details
  • Files reviewed: 13/13 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread tests/unittests/NetworkDetectorTests.cpp Outdated
@bmehta001
Guard network detector tests by feature
ce3e63c 
Compile the Windows detector tests only when HAVE_MAT_NETDETECT is enabled so compact custom SKUs that omit the detector still build the test project.

Verified at:
- tests/unittests/NetworkDetectorTests.cpp
- Solutions/build.compact.props

Files changed:
- tests/unittests/NetworkDetectorTests.cpp

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: df9f344e-e064-404e-ae6f-c0ef455d747c
@bmehta001
bmehta001 requested a balanced review from Copilot September 18, 2026 20:39
Copilot AI reviewed Sep 18, 2026
View reviewed changes

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.

🔵 Needs a closer look

The platform-specific callback lifecycle and new instrumentation pipeline warrant final human validation.

Review details
  • Files reviewed: 13/13 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort, defaults to Balanced
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@bmehta001 bmehta001

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Periodic memory leak analysis reports

2 participants

@bmehta001