In mirrored mode, handle route update as add instead of replace

[FetoiuCatalin]

Summary of the Pull Request

GNS is handling a route update operation using the NLM_F_REPLACE flag (equivalent to ip route replace), which collapses multiple routes that have the same destination, metric, tos, even if they have different next hops or interfaces. E.g. replacing one of the 2 routes below will collapse them into 1.
default via 10.130.174.1 dev eth1 proto kernel metric 25
default via 10.120.186.1 dev eth0 proto kernel metric 25

This was found when adding and removing and adapter on a test windows host VM - if the new adapter happens to have the same default route with same metric as existing one, after removing the new one connectivity will be lost in WSL as the route of the original one will be deleted if an update happens for the route of the new one.

Detailed Description of the Pull Request / Additional comments

The mirrored mode route synchronization logic never attempts an in-place update of a route. Whenever a route change occurs on the host (ProcessRouteChange), we mark for removal the routes that are known to have been synced in Linux but are no longer part of the latest set of host routes, we do not do a diff of the route properties to determine if an in-place update is needed.

Handling an update as an Add (ignoring already exists error, which already happens) is the intended behavior for route updates.

Note: NAT and virtio modes already use just add and remove operations for Routes

Validation Steps Performed

Scenario that found the bug: Add-VMNetworkAdapter/Remove-VMNetworkAdapter on a windows host VM and verifying connectivity
Connecting/disconnecting OpenVPN multiple times and verifying connectivity, which leads to routes being added/deleted multiple times
Existing automated NetworkTests + new tests that validate host route updates are correctly mirrored

[Copilot]

Pull request overview

This draft PR changes Linux route “Update” handling in netlinkutil to avoid inadvertently replacing another interface’s route when multiple routes share the same destination/metric key, and adds a Windows networking test to validate the new behavior.

Changes:

• Update route modification logic to omit NLM_F_REPLACE for Update operations (treat Update as “ensure exists” / add-only).
• Add a regression test that creates two same-metric default routes on different interfaces and verifies both persist after an Update.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
src/linux/netlinkutil/RoutingTable.cpp	Changes Update route flags to avoid NLM_F_REPLACE overwriting another interface’s route.
test/windows/NetworkTests.cpp	Adds a WSL2 test validating that updating eth0’s default route does not remove another interface’s same-metric default route.

---

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

[Copilot]

Pull request overview

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

Suppressed comments (1)

src/linux/netlinkutil/RoutingTable.cpp:103

• Remove the leftover commented-out assignment (// flags = NLM_F_CREATE;). It’s redundant with the next line and makes it look like the code is mid-edit.

        // Note: The EEXIST error is already treated as non-fatal.
        // flags = NLM_F_CREATE;
        flags = NLM_F_CREATE;

[Copilot]

Pull request overview

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

Suppressed comments (1)

src/windows/service/exe/WslMirroredNetworking.cpp:1415

• In the refresh-all-routes retry path, the code sets SyncStatus = PendingUpdate, but this PR now treats PendingUpdate identically to PendingAdd (both send ModifyRequestType::Add). Setting PendingAdd here would better reflect the actual retry operation and avoid future confusion when debugging route-sync state transitions.

                    hr = SendRouteRequestToGns(endpoint, trackedRoute, hns::ModifyRequestType::Add);
                    if (FAILED(hr.value()))
                    {
                        trackedRoute.SyncStatus = PendingUpdate;
                        trackedRoute.SyncRetryCount = TrackedRoute::MaxSyncRetryCount;
                    }

[Copilot]

Pull request overview

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

Suppressed comments (1)

src/windows/service/exe/WslMirroredNetworking.cpp:1416

• In the refresh-all-routes retry path, the code now only ever sends ModifyRequestType::Add, but on failure it sets SyncStatus = PendingUpdate. Since PendingUpdate no longer results in an Update request (it’s handled identically to PendingAdd), keeping the status as PendingUpdate is misleading and makes future debugging harder. Consider using PendingAdd here (or renaming the enum more broadly) to match the actual behavior.

                    hr = SendRouteRequestToGns(endpoint, trackedRoute, hns::ModifyRequestType::Add);
                    if (FAILED(hr.value()))
                    {
                        trackedRoute.SyncStatus = PendingUpdate;
                        trackedRoute.SyncRetryCount = TrackedRoute::MaxSyncRetryCount;
                    }

[Copilot]

Pull request overview

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

Suppressed comments (1)

src/windows/service/exe/WslMirroredNetworking.cpp:1403

• Grammar: use "A" (not "An") before "ModifyRequestType::Add".

            // An ModifyRequestType::Add plumbs a new route or leaves an existing one in place (EEXIST is ignored),

[Ben Hillis (benhillis)]

FetoiuCatalin - is this ready to be merged?

[Copilot]

🟡 Changes recommended

Unresolved route-tracking and validation defects, along with test gaps, block approval.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (6)

src/linux/netlinkutil/RoutingTable.cpp:76

• Because Operation::Update is still a valid value in the shared enum, this assert is the only guard against callers passing Update. In release builds the assert is removed, and ModifyRouteImpl treats every non-Create action as Remove, so an accidental route update deletes the route. Use a runtime validation (or make the invalid branch throw) before selecting `RTM_DELROUTE.

    assert(action == Operation::Create || action == Operation::Remove);

src/windows/service/exe/WslMirroredNetworking.cpp:1403

• Use the article "A" before ModifyRequestType::Add; "An ModifyRequestType::Add" is grammatically incorrect.

            // An ModifyRequestType::Add plumbs a new route or leaves an existing one in place (EEXIST is ignored),

test/windows/NetworkTests.cpp:5237

• This test removes destinationA before adding the replacement, and it uses only the single best interface, so it never has two routes with the same destination and metric on different interfaces. It therefore cannot catch the NLM_F_REPLACE regression described in the PR, where syncing one route must not collapse the other; add a two-interface, same-key route scenario and verify the untouched route remains.

        RemoveHostRoute(destinationA);
        AddHostRoute(ifIndex, destinationA, gatewayB, metricB);

test/windows/NetworkTests.cpp:5203

• A negative wait can never fail: when shouldExist is false but the route remains present for the full timeout, the loop falls through and returns std::nullopt here, which makes callers' VERIFY_IS_FALSE(...has_value()) pass. Preserve the last observed match or otherwise report a timeout with the route still present so the absence assertions distinguish success from timeout.

        return std::nullopt;

test/windows/NetworkTests.cpp:5227

• The selected ifIndex is the interface used for 8.8.8.8/2001:4860::8888, but gatewayA and gatewayB are hard-coded documentation addresses unrelated to that interface's subnet. Windows requires route next hops to be on-link, so New-NetRoute can fail before this test reaches the mirrored assertions. Derive the gateways from the selected interface or provision an on-link test interface/address.

        AddHostRoute(ifIndex, destinationA, gatewayA, metricA);

test/windows/NetworkTests.cpp:5234

• The metric-update assertion only checks that the new metric appears. If synchronization leaves both the old and new route in Linux, this still passes; add a negative check for initialRoute->Metric, as the gateway and destination changes already do for their old values.

        VERIFY_IS_TRUE(WaitForMirroredRoute(family, destinationA, gatewayA, true, initialRoute->Metric + (metricB - metricA)).has_value());

• Files reviewed: 4/4 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Copilot]

🟡 Changes recommended

Fix the test gateways so routes are valid on-link, and correct the comment typo.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

src/windows/service/exe/WslMirroredNetworking.cpp:1403

• The newly added comment has a grammatical typo: use “A ModifyRequestType::Add” rather than “An ModifyRequestType::Add.”

            // An ModifyRequestType::Add plumbs a new route or leaves an existing one in place (EEXIST is ignored),

• Files reviewed: 4/4 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

The route-update test must verify that the stale old-metric route is removed.

Review details

Suppressed comments (3)

test/windows/NetworkTests.cpp:5253

• After the metric change this only proves that the new metric appears; it would still pass if the old (destinationA, gatewayA, initialRoute->Metric) route remained in Linux. That would leave a stale duplicate after an update. Add a negative wait for the old metric before proceeding.

        VERIFY_IS_TRUE(WaitForMirroredRoute(family, destinationA, gatewayA, true, initialRoute->Metric + (metricB - metricA)));

test/windows/NetworkTests.cpp:3223

• Use “skips” here; the singular regex pattern is the subject of this new comment.

        // The (?:.*(metric N))? skip tokens that precede the metric (e.g. "proto kernel",

test/windows/NetworkTests.cpp:3191

• Use “skips” here; the singular regex pattern is the subject of this new comment.

        // The (?:.*(metric N))? skip tokens that precede the metric (e.g. "proto kernel",

• Files reviewed: 4/4 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

PendingUpdate-to-Add behavior and metric-clamping expectations still need correction.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (3)

src/windows/service/exe/WslMirroredNetworking.cpp:1412

• The added host-route tests exercise normal host-set changes, which create a new PendingAdd after the old route is marked PendingRemoval; they never drive the PendingUpdate state used by an interface refresh or a route reappearing. The new PendingUpdate-to-Add behavior is therefore untested. Add focused coverage that reaches this state and verifies the route is preserved without an in-place replace.

            case PendingUpdate:
                hr = SendRouteRequestToGns(endpoint, trackedRoute, hns::ModifyRequestType::Add);

test/windows/NetworkTests.cpp:3277

• The newly added comment has a subject-verb agreement error: “skip” should be “skips” because the regex expression is singular.

        // The (?:.*(metric N))? skip tokens that precede the metric (e.g. "proto kernel",
        // "scope link");

test/windows/NetworkTests.cpp:5336

• The production mirroring path clamps route metrics to UINT16_MAX, but this expectation does not. When the interface metric makes the updated value reach that limit, the test expects a value above the metric Linux receives and fails even though synchronization is correct; clamp the expected value before checking it.

        VERIFY_IS_TRUE(WaitForMirroredRoute(family, destinationA, gatewayA, true, initialRoute->Metric + (metricB - metricA)));

• Files reviewed: 4/4 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Two moderate test issues remain unresolved.

Review details

Suppressed comments (2)

test/windows/NetworkTests.cpp:5319

• The expected metric can exceed the product's UINT16_MAX clamp: ProcessRouteChange caps the summed route/interface metric at UINT16_MAX (src/windows/service/exe/WslMirroredNetworking.cpp:343-349). On an interface with a high metric, a correct mirrored route will be 65535 while this wait looks for initial + 400, causing a false failure; clamp the expected value before comparing.

        VERIFY_IS_TRUE(WaitForMirroredRoute(family, destinationA, gatewayA, true, initialRoute->Metric + (metricB - metricA)));

test/windows/NetworkTests.cpp:539

• The 7 passed in these expected routes is not actually checked: NetworkTests::Route::operator== compares only via, device, and prefix (NetworkTests.cpp:239-242), so this regression can pass when the routes have different metrics and does not exercise the same-(destination, metric) key involved in the NLM_F_REPLACE bug. Make these assertions compare Route::Metric explicitly (or update the shared equality helper).

        const bool v4RouteOnEth0 = RouteExists({L"192.168.0.12", L"eth0", L"192.168.77.0/24", 7});
        const bool v4RouteOnDummy0 = RouteExists({L"192.168.0.12", L"dummy0", L"192.168.77.0/24", 7});

• Files reviewed: 4/4 changed files
• Comments generated: 0 new
• Review effort level: Lite