Skip to content

Fix tracked routes being collapsed by incomplete comparison - #41393

Open
Feng Wang (chemwolf6922) wants to merge 2 commits into
masterfrom
user/chemwolf6922/fix-dropped-routes-due-to-route-comparison
Open

Fix tracked routes being collapsed by incomplete comparison#41393
Feng Wang (chemwolf6922) wants to merge 2 commits into
masterfrom
user/chemwolf6922/fix-dropped-routes-due-to-route-comparison

Conversation

@chemwolf6922

Copy link
Copy Markdown
Contributor

Summary of the Pull Request

Mirrored route tracking used an incomplete comparator that considered only route class, destination address, and metric. Distinct routes with different prefix lengths or next hops could therefore be treated as equivalent and silently omitted.

The fix preserves the existing route-class ordering while using the complete EndpointRoute comparison for route identity. Regression tests cover prefix length, next hop, metric, exact duplicates, and dependency ordering.

PR Checklist

  • Closes: Link to issue #xxx
  • Communication: I've discussed this with core contributors already. If work hasn't been agreed, this work might be rejected
  • Tests: Added/updated if needed and all pass
  • Localization: All end user facing strings can be localized
  • Dev docs: Added/updated if needed
  • Documentation updated: If checked, please file a pull request on our docs repo and link it here: #xxx

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

Add test:
NetworkTests::NetworkTests::TrackedRouteOrderingPreservesRouteIdentity

Copilot AI lite review requested due to automatic review settings August 20, 2026 04:46
@chemwolf6922
Feng Wang (chemwolf6922) requested a review from a team as a code owner August 20, 2026 04:46

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.

Pull request overview

Fixes mirrored route tracking dropping distinct routes by correcting TrackedRoute’s std::set ordering: route-class precedence is preserved, but route identity now uses the full EndpointRoute comparison (including prefix length and next hop).

Changes:

  • Update TrackedRoute::operator< to compare by route class first, then by full EndpointRoute ordering.
  • Add a regression test covering route identity (prefix length, next hop, metric, duplicates) and route-class ordering.
  • Update test build include paths to access the tracked-route definitions.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
test/windows/NetworkTests.cpp Adds TrackedRouteOrderingPreservesRouteIdentity regression test for route identity + class ordering.
test/windows/CMakeLists.txt Adds an include path so tests can include WslCoreTcpIpStateTracking.h.
src/windows/service/exe/WslCoreTcpIpStateTracking.h Fixes TrackedRoute ordering to avoid collapsing distinct routes.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread test/windows/CMakeLists.txt
Copilot AI review requested due to automatic review settings August 20, 2026 05:24

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.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (1)

test/windows/CMakeLists.txt:39

  • Using a per-source INCLUDE_DIRECTORIES property is unusual in this repo and can be fragile with CMake features like UNITY_BUILD (the unity translation unit may not inherit source-specific include dirs consistently). Prefer adding the directory to the target include paths instead, and remove the source-level property so NetworkTests.cpp builds the same way in all configurations.
target_include_directories(wsltests PRIVATE
    ${CMAKE_SOURCE_DIR}/src/windows/WslcSDK
    ${CMAKE_BINARY_DIR}/src/windows/WslcSDK/winrt/${TARGET_PLATFORM}/${CMAKE_BUILD_TYPE})
set_property(SOURCE NetworkTests.cpp APPEND PROPERTY INCLUDE_DIRECTORIES
    ${CMAKE_SOURCE_DIR}/src/windows/service/exe)

@chemwolf6922

Copy link
Copy Markdown
Contributor Author

Hi Blue (@OneBlue) , Keith Horton (@keith-horton) . This may need review from you as well. Please help take a look. Thanks.

@FetoiuCatalin

Copy link
Copy Markdown
Contributor

thanks for finding this, just curious - was this found via code inspection or based on some github report?

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