[fix] Fix data collection channels to use negotiated protocol version instead of V1 by nohwnd · Pull Request #16096 · microsoft/vstest

Jakub Jareš (nohwnd) · 2026-06-05T14:00:21Z

🤖 This is an automated fix generated by the Issue Triage agent.

Root Cause

The data collection IPC channels (vstest.console ↔ datacollector and testhost ↔ datacollector) always used V1 serialization because every SendMessage(type, payload) call omitted the version parameter, which defaults to 1 in SocketCommunicationManager. This meant TestCase objects in DataCollectionTestStart/DataCollectionTestEnd messages were serialized with the slow V1 serializer instead of the modern V2+ serializer.

By contrast, the primary test execution channel (vstest.console ↔ testhost) already performs a proper version-negotiation handshake via CheckVersionWithTestHostAsync.

What the Fix Does

Main channel (`DataCollectionRequestSender` / `DataCollectionRequestHandler`)

Version negotiation is piggybacked on the existing BeforeTestRunStart / BeforeTestRunStartResult exchange (no new round-trip needed):

Sender sends BeforeTestRunStart at ProtocolVersioning.HighestSupportedVersion (V7) to advertise its capability.
Handler reads message.Version, computes _protocolVersion = Math.Min(request.Version, HighestSupportedVersion), stores it, and echoes it in the BeforeTestRunStartResult response.
Sender reads message.Version from BeforeTestRunStartResult, adopts it as _protocolVersion for all subsequent messages (TestHostLaunched, AfterTestRunEnd).
Handler uses _protocolVersion for all outgoing messages (BeforeTestRunStartResult, AfterTestRunEndResult, DataCollectionMessage).

Test-case event channel (`DataCollectionTestCaseEventSender` / `DataCollectionTestCaseEventHandler`)

Sender always sends at HighestSupportedVersion.
Handler echoes the incoming message version in DataCollectionTestEndResult.

Backward compatibility

Sender version	Handler version	Negotiated
New (V7)	Old (omits version → V1 in response)	V1 ✓
Old (omits version → V1 in request)	New	V1 ✓
New (V7)	New (V7)	V7 ✓

No new public API surface is added.

Tests

Updated unit tests in DataCollectionRequestSenderTests, DataCollectionRequestHandlerTests, DataCollectionTestCaseEventSenderTests, and DataCollectionTestCaseEventHandlerTests to verify the 3-argument SendMessage(type, payload, version) overload is called.

All 625 Microsoft.TestPlatform.CommunicationUtilities.UnitTests tests pass.

🔍 Triaged by Issue Repro Triage & Auto-Fix 🔍

…ad of V1 The datacollector IPC channels (vstest.console↔datacollector and testhost↔datacollector) were always using V1 serialization because all SendMessage calls omitted the version parameter, which defaults to 1 in SocketCommunicationManager. This meant TestCase objects in DataCollectionTestStart/DataCollectionTestEnd messages used the slow V1 serializer instead of the modern serializer. Main channel (DataCollectionRequestSender / DataCollectionRequestHandler): - Sender sends BeforeTestRunStart at HighestSupportedVersion (V7) to advertise its capability. - Handler reads the request version, stores min(request, own highest) as _protocolVersion, and echoes that version in BeforeTestRunStartResult. - Sender reads the response version and adopts it as _protocolVersion for all subsequent messages (TestHostLaunched, AfterTestRunEnd). - Handler uses _protocolVersion for all outgoing messages (BeforeTestRunStartResult, AfterTestRunEndResult, DataCollectionMessage). Test-case event channel (DataCollectionTestCaseEventSender / DataCollectionTestCaseEventHandler): - Sender always sends at HighestSupportedVersion. - Handler echoes the incoming message version in DataCollectionTestEndResult. Backward compatibility is preserved: an old handler responds at V1 (no version param), which the sender reads and stores; an old sender sends at V1, which the handler stores. Fixes #15623 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

This PR updates the data collection IPC channels (vstest.console ↔ datacollector and testhost ↔ datacollector) to stop defaulting to protocol V1 serialization by ensuring the protocol version is explicitly included on SendMessage(type, payload, version) calls and (partially) negotiated.

Changes:

Add protocol version negotiation state to DataCollectionRequestSender/DataCollectionRequestHandler and use it for subsequent messages on that channel.
Update DataCollectionTestCaseEventSender/DataCollectionTestCaseEventHandler to send/echo protocol versions for test-case start/end events.
Update CommunicationUtilities unit tests to validate the 3-argument SendMessage overload is used.

Reviewed changes

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

Show a summary per file

File	Description
src/Microsoft.TestPlatform.CommunicationUtilities/DataCollectionRequestSender.cs	Tracks negotiated protocol version and uses it for datacollector-bound messages.
src/Microsoft.TestPlatform.CommunicationUtilities/DataCollectionRequestHandler.cs	Negotiates protocol version on `BeforeTestRunStart` and uses it for responses.
src/Microsoft.TestPlatform.CommunicationUtilities/DataCollectionTestCaseEventSender.cs	Sends test-case event messages using `HighestSupportedVersion`.
src/Microsoft.TestPlatform.CommunicationUtilities/DataCollectionTestCaseEventHandler.cs	Sends test-case end result using the incoming message version.
test/Microsoft.TestPlatform.CommunicationUtilities.UnitTests/DataCollectionRequestSenderTests.cs	Verifies `BeforeTestRunStart` is sent with an explicit protocol version.
test/Microsoft.TestPlatform.CommunicationUtilities.UnitTests/DataCollectionRequestHandlerTests.cs	Updates verifications to expect the versioned `SendMessage` overload.
test/Microsoft.TestPlatform.CommunicationUtilities.UnitTests/DataCollectionTestCaseEventSenderTests.cs	Verifies test-case start/end messages are sent with an explicit protocol version.
test/Microsoft.TestPlatform.CommunicationUtilities.UnitTests/DataCollectionTestCaseEventHandlerTests.cs	Updates verification to expect the versioned `SendMessage` overload.

+                if (message.Version > 0)
+                {
+                    _protocolVersion = message.Version;
+                }


                    }

-                    _communicationManager.SendMessage(MessageType.DataCollectionTestEndResult, attachmentSets);
+                    _communicationManager.SendMessage(MessageType.DataCollectionTestEndResult, attachmentSets, message.Version);


Jakub Jareš (nohwnd)

Review: Fix data collection channels to use negotiated protocol version

The main channel fix (DataCollectionRequestSender ↔ DataCollectionRequestHandler) is mechanically correct end-to-end:

Sender advertises HighestSupportedVersion in BeforeTestRunStart.
Handler computes Math.Min(request.Version, HighestSupportedVersion), stores the result, echoes it in BeforeTestRunStartResult.
Sender adopts the echoed version for all subsequent messages (TestHostLaunched, AfterTestRunEnd).
Old-sender / old-handler backward compat is preserved via the >0 guard and the default 1 fallback.

Three issues flagged with inline comments:

Sub-channel missing negotiation (medium) — DataCollectionTestCaseEventSender always sends at HighestSupportedVersion (V7) with no _protocolVersion field and no Math.Min guard. The PR's backward-compat table covers the main channel only. If the datacollector was built before V7 was added to the GetPayloadOptions switch, it would throw NotSupportedException for every incoming test-case event. This is mitigated by the fact that V7 is already in both the STJ and Jsonite version switches today, but the risk is undocumented and the guarantee is fragile.
Tests don't assert the negotiated version value (medium) — All handler tests set BeforeTestRunStart.Version = 7 and then verify with It.IsAny<int>(). Because Math.Min(7, 7) == 7, the invariant is never exercised with a request version lower than HighestSupportedVersion. A test with Version = 4 asserting the response sends exactly 4 would catch silent regressions in the negotiation logic.
DataCollectionTestStartAck carries no version (low) — DataCollectionTestEndResult echoes message.Version but the start ack is sent with the 1-arg overload. Currently harmless; noted for consistency and forward-compat.

🧠 Reviewed by Expert Code Review · Dimensions: IPC Transport & Protocol Stability, Backward Compatibility & Rollback Safety

🧠 Reviewed by Expert Code Reviewer 🧠

Jakub Jareš (nohwnd) · 2026-06-05T14:16:47Z

    public void SendTestCaseStart(TestCaseStartEventArgs e)
    {
-        _communicationManager.SendMessage(MessageType.DataCollectionTestStart, e);
+        _communicationManager.SendMessage(MessageType.DataCollectionTestStart, e, ProtocolVersioning.HighestSupportedVersion);


[IPC Transport & Protocol Stability] Unlike the main data collection channel (DataCollectionRequestSender/DataCollectionRequestHandler) — which negotiates via Math.Min(request.Version, HighestSupportedVersion) and stores the result in _protocolVersion — the test case event sub-channel always sends at HighestSupportedVersion with no negotiation and no _protocolVersion field.

The backward-compat table in the PR description covers the main channel only. For this sub-channel, if the DataCollectionTestCaseEventHandler in the datacollector process is from an older vstest build whose GetPayloadOptions switch does not yet include V7 (the current HighestSupportedVersion), it will throw NotSupportedException on every incoming test-case event.

This is mitigated today because both the STJ and Jsonite implementations already list 7 in their version switches, so any datacollector built against a recent vstest can handle V7 payloads. However:

V7 carries no documented change in ProtocolVersioning.cs (no summary comment), so the boundary where "old" becomes "unsafe" is unclear.

The handler side properly guards DataCollectionTestEndResult by echoing message.Version, but the sender never uses that echoed value to adapt future sends — so the echo only helps the sender deserialize the response, not to detect version mismatches before they happen.

Consider threading the negotiated _protocolVersion from the main-channel handshake into DataCollectionTestCaseEventSender (e.g. via BeforeTestRunStartResult), or adding a minimal echo/ack round-trip on this channel similar to CheckVersionWithTestHostAsync, so the sub-channel enjoys the same Math.Min safety as the main channel.

Fixed. The sub-channel now has proper Math.Min negotiation on both sides:

DataCollectionTestCaseEventSender gains a _protocolVersion field (initialized to HighestSupportedVersion). After each SendTestCaseStart, the negotiated version is adopted from the DataCollectionTestStartAck version echo for all subsequent sends (SendTestCaseEnd, SendTestSessionEnd).

DataCollectionTestCaseEventHandler now sends DataCollectionTestStartAck with Math.Min(message.Version, ProtocolVersioning.HighestSupportedVersion) so the handler's capability boundary is communicated back to the sender.

New tests verify: (1) DataCollectionTestStartAck echoes the min of sender version and highest supported, (2) SendTestCaseEnd and SendTestSessionEnd use the negotiated version after a test-case start negotiation.

🔧 Iterated by PR Iteration Agent 🔧

Jakub Jareš (nohwnd) · 2026-06-05T14:16:47Z

        // Verify SessionStarted events
        _mockDataCollectionManager.Verify(x => x.SessionStarted(It.IsAny<SessionStartEventArgs>()), Times.Once);
-        _mockCommunicationManager.Verify(x => x.SendMessage(MessageType.BeforeTestRunStartResult, It.IsAny<BeforeTestRunStartResult>()), Times.Once);
+        _mockCommunicationManager.Verify(x => x.SendMessage(MessageType.BeforeTestRunStartResult, It.IsAny<BeforeTestRunStartResult>(), It.IsAny<int>()), Times.Once);


[Backward Compatibility & Rollback Safety / Test Coverage] It.IsAny<int>() verifies that the 3-arg SendMessage overload is called, but it does not validate the negotiated version value.

The critical invariant is _protocolVersion = Math.Min(request.Version, HighestSupportedVersion). A test that sends BeforeTestRunStart at a version lower than HighestSupportedVersion (e.g. Version = 4) and then asserts the response uses exactly 4 — not 7 — would catch regressions where the handler accidentally uses the wrong version (e.g. always responds at HighestSupportedVersion, or always responds at 1). The current tests all set Version = 7, so Math.Min(7, 7) == 7 and the It.IsAny<int>() matcher would accept any wrong value silently.

Added. A new test ProcessRequestsShouldNegotiateProtocolVersionToMinOfRequestAndHighest sends BeforeTestRunStart at version 4 (less than HighestSupportedVersion = 7) and asserts that both BeforeTestRunStartResult and AfterTestRunEndResult are sent at exactly version 4 — verifying the Math.Min(4, 7) = 4 invariant and catching regressions where the handler might respond at the wrong version.

🔧 Iterated by PR Iteration Agent 🔧

Jakub Jareš (nohwnd) · 2026-06-05T14:16:47Z

                    }

-                    _communicationManager.SendMessage(MessageType.DataCollectionTestEndResult, attachmentSets);
+                    _communicationManager.SendMessage(MessageType.DataCollectionTestEndResult, attachmentSets, message.Version);


[IPC Transport & Protocol Stability] DataCollectionTestEndResult now correctly echoes message.Version, but DataCollectionTestStartAck (nine lines earlier) is still sent with the 1-arg overload and carries no version. The sender currently ignores the ack version so this is harmless, but the asymmetry is a maintenance hazard: if the sender ever starts tracking version from the ack (e.g. to initialise _protocolVersion on the sub-channel), it would silently read 0 instead of the handler's supported version. For consistency, consider echoing message.Version here too.

Fixed. DataCollectionTestStartAck now uses the 3-arg overload and echoes Math.Min(message.Version, ProtocolVersioning.HighestSupportedVersion), matching the pattern of DataCollectionTestEndResult. A new test ProcessRequestsShouldEchoNegotiatedVersionInTestCaseStartAck verifies the echo.

🔧 Iterated by PR Iteration Agent 🔧

… coverage - DataCollectionTestCaseEventHandler: echo Math.Min(message.Version, HighestSupportedVersion) in DataCollectionTestStartAck so the sender can detect the handler's maximum supported version - DataCollectionTestCaseEventSender: add _protocolVersion field (initialized to HighestSupportedVersion); adopt the negotiated version from the DataCollectionTestStartAck for all subsequent sends (SendTestCaseEnd, SendTestSessionEnd), giving the sub-channel the same Math.Min safety as the main DataCollection channel - DataCollectionRequestHandlerTests: add test that sends BeforeTestRunStart at version 4 and asserts both BeforeTestRunStartResult and AfterTestRunEndResult are sent at exactly version 4, verifying the Math.Min negotiation invariant - DataCollectionTestCaseEventHandlerTests: add test verifying DataCollectionTestStartAck echoes Math.Min(incomingVersion, HighestSupportedVersion) - DataCollectionTestCaseEventSenderTests: add tests verifying SendTestCaseEnd and SendTestSessionEnd use the version negotiated via the DataCollectionTestStartAck Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Jakub Jareš (nohwnd) · 2026-06-05T14:39:01Z

Commit pushed: 4b99347

🔧 Iterated by PR Iteration Agent 🔧

Jakub Jareš (nohwnd)

Review: Iteration update (commit `4b99347`)

All three issues from the prior review have been addressed in the second commit:

✅ Sub-channel negotiation — DataCollectionTestCaseEventSender now tracks _protocolVersion, initialised to HighestSupportedVersion and updated from the DataCollectionTestStartAck echo. DataCollectionTestCaseEventHandler responds with Math.Min(message.Version, HighestSupportedVersion).
✅ Test coverage for negotiation invariant — New test sends BeforeTestRunStart at version 4 and asserts both BeforeTestRunStartResult and AfterTestRunEndResult are sent at exactly 4, covering the Math.Min(4, 7) = 4 invariant.
✅ DataCollectionTestStartAck version — Now sends with Math.Min(message.Version, HighestSupportedVersion), matching DataCollectionTestEndResult.

Two follow-up observations on the iterated code (see inline comments):

DataCollectionTestEndResult missing Math.Min guard (low) — DataCollectionTestStartAck uses Math.Min(message.Version, HighestSupportedVersion), but DataCollectionTestEndResult echoes raw message.Version. Not a correctness issue in the current negotiated flow (the sender's _protocolVersion is already bounded by the ack), but the asymmetry is a maintenance hazard.
_protocolVersion adoption without Math.Min (low) — Both DataCollectionRequestSender (line 147) and DataCollectionTestCaseEventSender (line 101) adopt the handler-echoed version without clamping. If a buggy/out-of-sync handler ever reported a version above HighestSupportedVersion, the sender would crash on the next serialization. The handler side is correctly guarded; adding Math.Min on the adoption side would close the loop.

Description drift: The PR description's sub-channel section ("Sender always sends at HighestSupportedVersion. Handler echoes the incoming message version in DataCollectionTestEndResult.") no longer fully describes the post-iteration behavior. The sub-channel now has a proper _protocolVersion field and ack-based Math.Min negotiation — worth updating the description to reflect this.

🧠 Reviewed by Expert Code Reviewer 🧠 · Dimensions: IPC Transport & Protocol Stability, Backward Compatibility & Rollback Safety

🧠 Reviewed by Expert Code Reviewer 🧠

Jakub Jareš (nohwnd) · 2026-06-05T14:51:52Z

                    }

-                    _communicationManager.SendMessage(MessageType.DataCollectionTestEndResult, attachmentSets);
+                    _communicationManager.SendMessage(MessageType.DataCollectionTestEndResult, attachmentSets, message.Version);


[IPC Transport & Protocol Stability] DataCollectionTestStartAck (line 100) uses Math.Min(message.Version, ProtocolVersioning.HighestSupportedVersion), but DataCollectionTestEndResult here echoes message.Version unguarded.

In the current negotiated flow this is harmless — the sender's _protocolVersion was already bounded by the ack's Math.Min, so the version arriving in DataCollectionTestEnd cannot exceed HighestSupportedVersion. However, the asymmetry is a maintenance hazard: if message.Version ever arrived out of range (buggy/rogue sender), GetPayloadOptions would throw NotSupportedException on this response path while the ack path would not.

For consistency with the ack and with the main-channel handler (DataCollectionRequestHandler.HandleBeforeTestRunStart), consider:

_communicationManager.SendMessage(MessageType.DataCollectionTestEndResult, attachmentSets, Math.Min(message.Version, ProtocolVersioning.HighestSupportedVersion));

Fixed. DataCollectionTestEndResult now echoes Math.Min(message.Version, ProtocolVersioning.HighestSupportedVersion) for consistency with DataCollectionTestStartAck and the main-channel handler. A new test ProcessRequestsShouldNegotiateVersionInTestCaseEndResult verifies the guard with a sender at version 4.

🔧 Iterated by PR Iteration Agent 🔧

🔧 Iterated by PR Iteration Agent 🔧

Jakub Jareš (nohwnd) · 2026-06-05T14:51:52Z

+                // protocol version for all subsequent messages on this channel.
+                if (message.Version > 0)
+                {
+                    _protocolVersion = message.Version;


[IPC Transport & Protocol Stability] _protocolVersion is adopted directly from message.Version with no Math.Min guard. The same pattern appears in DataCollectionTestCaseEventSender.SendTestCaseStart (line 101).

In practice the handler always responds with Math.Min(request.Version, HighestSupportedVersion), so the echoed value will never exceed HighestSupportedVersion. But if a rogue or out-of-sync handler ever reported a higher version, the sender would try to serialize subsequent messages at an unsupported version and hit NotSupportedException in GetPayloadOptions.

Defense-in-depth suggestion for both sites:

if (message.Version > 0) { _protocolVersion = Math.Min(message.Version, ProtocolVersioning.HighestSupportedVersion); }

Fixed. Both DataCollectionRequestSender (line 147) and DataCollectionTestCaseEventSender (line 101) now use _protocolVersion = Math.Min(message.Version, ProtocolVersioning.HighestSupportedVersion) when adopting the echoed version, guarding against a rogue/out-of-sync handler reporting a version above HighestSupportedVersion.

🔧 Iterated by PR Iteration Agent 🔧

🔧 Iterated by PR Iteration Agent 🔧

…estEndResult echo - DataCollectionTestCaseEventHandler: echo Math.Min(message.Version, HighestSupportedVersion) in DataCollectionTestEndResult (defense-in-depth, consistent with TestStartAck) - DataCollectionRequestSender: guard _protocolVersion = Math.Min(message.Version, HighestSupportedVersion) when adopting BeforeTestRunStartResult version - DataCollectionTestCaseEventSender: guard _protocolVersion = Math.Min(message.Version, HighestSupportedVersion) when adopting TestStartAck version - Add test ProcessRequestsShouldNegotiateVersionInTestCaseEndResult to verify Math.Min for TestEndResult Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Jakub Jareš (nohwnd) · 2026-06-05T15:06:44Z

Commit pushed: a0f6309

🔧 Iterated by PR Iteration Agent 🔧

Copilot

Pull request overview

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

        var message = _communicationManager.ReceiveMessage();
        if (message != null && message.MessageType != MessageType.DataCollectionTestStartAck)
        {
            EqtTrace.Error("DataCollectionTestCaseEventSender.SendTestCaseStart : MessageType.DataCollectionTestStartAck not received.");
        }
+
+        // Adopt the version echoed by the handler as the negotiated protocol version for all
+        // subsequent sends on this sub-channel.
+        if (message?.Version > 0)
+        {
+            _protocolVersion = Math.Min(message.Version, ProtocolVersioning.HighestSupportedVersion);
+        }
    }


                    }

-                    _communicationManager.SendMessage(MessageType.DataCollectionTestStartAck);
+                    _communicationManager.SendMessage(MessageType.DataCollectionTestStartAck, string.Empty, Math.Min(message.Version, ProtocolVersioning.HighestSupportedVersion));


                    }

-                    _communicationManager.SendMessage(MessageType.DataCollectionTestEndResult, attachmentSets);
+                    _communicationManager.SendMessage(MessageType.DataCollectionTestEndResult, attachmentSets, Math.Min(message.Version, ProtocolVersioning.HighestSupportedVersion));


+        var message = new Message
+        {
+            MessageType = MessageType.DataCollectionTestStart,
+            Version = 4,
+            RawMessage = JsonDataSerializer.Instance.SerializePayload(MessageType.DataCollectionTestStart, new TestCaseEndEventArgs(), 4),
+        };


+        var testCase = new TestCase("hello", new Uri("world://how"), "1.dll");
+        var message = new Message
+        {
+            MessageType = MessageType.DataCollectionTestEnd,
+            Version = 4,
+            RawMessage = JsonDataSerializer.Instance.SerializePayload(MessageType.DataCollectionTestEnd, new TestResultEventArgs(new VisualStudio.TestPlatform.ObjectModel.TestResult(testCase)), 4),
+        };


+        // After negotiating version 4 via the ack, SendTestSessionEnd must use version 4.
+        _mockCommunicationManager.Verify(x => x.SendMessage(MessageType.SessionEnd, It.IsAny<SessionEndEventArgs>(), 4), Times.Once);
+    }
 }


Jakub Jareš (nohwnd)

Review: Iteration update (commit `a0f6309`)

Both issues from the third review have been resolved in this commit:

✅ DataCollectionTestEndResult Math.Min guard — DataCollectionTestCaseEventHandler now echoes Math.Min(message.Version, ProtocolVersioning.HighestSupportedVersion) in both DataCollectionTestStartAck and DataCollectionTestEndResult, fully symmetric.
✅ _protocolVersion adoption guard — Both DataCollectionRequestSender (BeforeTestRunStartResult path) and DataCollectionTestCaseEventSender (DataCollectionTestStartAck path) now adopt the echoed version with Math.Min(message.Version, ProtocolVersioning.HighestSupportedVersion), closing the defense-in-depth gap.

New tests cover both fixes:

ProcessRequestsShouldNegotiateVersionInTestCaseEndResult — verifies DataCollectionTestEndResult echoes Math.Min(4, 7) = 4
The sender-side Math.Min is implicitly exercised by SendTestCaseEndShouldUseVersionNegotiatedFromTestCaseStartAck and SendTestSessionEndShouldUseVersionNegotiatedFromTestCaseStartAck

The code is now in a clean, consistent state: every version adoption and version echo in all four classes uses Math.Min with the same > 0 guard. No correctness, compatibility, or protocol-safety issues remain.

One remaining housekeeping item — PR description: The sub-channel section of the description still reflects the state after the first iteration, not the final design:

"Sender always sends at HighestSupportedVersion. Handler echoes the incoming message version in DataCollectionTestEndResult."

The final implementation goes further: the sender now carries _protocolVersion and adapts from the DataCollectionTestStartAck echo; the handler guards both responses with Math.Min; and the sender adoption itself is Math.Min-guarded. The backward-compat table only covers the main channel but the sub-channel has an equivalent negotiation path now. Worth a quick description update before merge so it accurately reflects what landed.

🧠 Reviewed by Expert Code Reviewer 🧠 · Dimensions: IPC Transport & Protocol Stability, Backward Compatibility & Rollback Safety

🧠 Reviewed by Expert Code Reviewer 🧠

Jakub Jareš (nohwnd) · 2026-06-07T09:09:09Z

The expert reviewer flagged that the PR description's sub-channel section is out of date. Here is a suggested replacement for the "Data Collection Sub-Channel Protocol" portion:

Before:

Sender always sends at HighestSupportedVersion.

Handler echoes the incoming message version in DataCollectionTestEndResult.

Suggested updated text:

Data Collection Sub-Channel Protocol Negotiation

The sub-channel now performs the same full bidirectional Math.Min negotiation as the main channel:

Party Class Behavior

Sender DataCollectionTestCaseEventSender Sends DataCollectionTestStart at HighestSupportedVersion. On receiving DataCollectionTestStartAck, adopts Math.Min(ack.Version, HighestSupportedVersion) into _protocolVersion. All subsequent sends (DataCollectionTestEnd) use _protocolVersion.

Handler DataCollectionTestCaseEventHandler Both DataCollectionTestStartAck and DataCollectionTestEndResult are sent at Math.Min(message.Version, HighestSupportedVersion).

Backward Compatibility

Scenario Behavior

New vstest.console + old testhost Old testhost ignores unknown protocol fields; sub-channel negotiation uses Math.Min and safely caps at old handler's HighestSupportedVersion

Old vstest.console + new testhost New testhost echoes Math.Min(incoming, own max) → negotiation converges on old console's max

Both new Full negotiation completes at mutual max

🔧 Iterated by PR Iteration Agent 🔧

Copilot AI review requested due to automatic review settings June 5, 2026 14:00

Copilot started reviewing on behalf of Jakub Jareš (nohwnd) June 5, 2026 14:00 View session

Copilot AI reviewed Jun 5, 2026

View reviewed changes

Jakub Jareš (nohwnd) commented Jun 5, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings June 5, 2026 15:06

Copilot started reviewing on behalf of Jakub Jareš (nohwnd) June 5, 2026 15:06 View session

Copilot AI reviewed Jun 5, 2026

View reviewed changes

Jakub Jareš (nohwnd) commented Jun 5, 2026

View reviewed changes

This comment has been minimized.

Sign in to view

This was referenced Jun 6, 2026

Daily Maintenance Digest — 2026-06-11 #16066

Closed

[aw] PR Iteration Agent 🔧 failed #16056

Closed

Jakub Jareš (nohwnd) added the 🚢 Ship it! Add to PRs where owner approves automated PR, but cannot approve because they "wrote it". label Jun 9, 2026

Jakub Jareš (nohwnd) merged commit 00db055 into main Jun 11, 2026
24 of 28 checks passed

Jakub Jareš (nohwnd) deleted the fix/issue-15623-94e20d569c0096b9 branch June 11, 2026 14:54

dotnet-maestro Bot mentioned this pull request Jun 12, 2026

[release/11.0.1xx-preview5] Source code updates from microsoft/vstest dotnet/dotnet#7209

Closed

This was referenced Jun 23, 2026

[main] Source code updates from microsoft/vstest dotnet/dotnet#7319

Closed

[release/11.0.1xx-preview6] Source code updates from microsoft/vstest dotnet/dotnet#7356

Merged

Uh oh!

Conversation

Jakub Jareš (nohwnd) commented Jun 5, 2026

Root Cause

What the Fix Does

Main channel (DataCollectionRequestSender / DataCollectionRequestHandler)

Test-case event channel (DataCollectionTestCaseEventSender / DataCollectionTestCaseEventHandler)

Backward compatibility

Tests

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Jakub Jareš (nohwnd) left a comment

Choose a reason for hiding this comment

Review: Fix data collection channels to use negotiated protocol version

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jakub Jareš (nohwnd) commented Jun 5, 2026

Uh oh!

Jakub Jareš (nohwnd) left a comment

Choose a reason for hiding this comment

Review: Iteration update (commit 4b99347)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jakub Jareš (nohwnd) commented Jun 5, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Jakub Jareš (nohwnd) left a comment

Choose a reason for hiding this comment

Review: Iteration update (commit a0f6309)

Uh oh!

This comment has been minimized.

Jakub Jareš (nohwnd) commented Jun 7, 2026

Data Collection Sub-Channel Protocol Negotiation

Backward Compatibility

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Main channel (`DataCollectionRequestSender` / `DataCollectionRequestHandler`)

Test-case event channel (`DataCollectionTestCaseEventSender` / `DataCollectionTestCaseEventHandler`)

Review: Iteration update (commit `4b99347`)

Review: Iteration update (commit `a0f6309`)