Skip to content

Fix .NET in-process E2E transport coverage#1986

Open
roji wants to merge 1 commit into
mainfrom
roji-fix-inprocess-e2e-transport
Open

Fix .NET in-process E2E transport coverage#1986
roji wants to merge 1 commit into
mainfrom
roji-fix-inprocess-e2e-transport

Conversation

@roji

@roji roji commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator

The .NET E2E fixture explicitly selected TCP even in the in-process matrix, so those tests never exercised the FFI transport and hid FFI-specific behavior.

This change:

  • Allows resuming the same session in .NET SDK, aligning it with the other SDK's behavior (.NET specifically had a check against doing that). This was a blocker for some .NET E2E tests in inproc mode.
  • selects RuntimeConnection.ForInProcess() for the in-process matrix while preserving TCP coverage in the other matrix
  • restores the native host environment before each test because the shared fixture outlives per-test environment isolation
  • aligns same-client session resume with the other SDKs by replacing the routed session wrapper before the resume RPC, without adding operation-wide locking or concurrent-resume rejection
  • keeps TCP-only multi-client behavior intact and adds fixture policy and session lifetime coverage
  • uses a checkpoint number valid for the native runtime's u32 domain

The complete .NET in-process suite passes through FFI with 672 tests passed and 3 existing skips. Targeted TCP resume coverage and all .NET target-framework builds also pass.

Copilot AI review requested due to automatic review settings July 14, 2026 16:44
@roji
roji requested a review from a team as a code owner July 14, 2026 16:44
@github-actions

This comment has been minimized.

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

Updates .NET E2E coverage to exercise the FFI transport and supports same-client session resumption.

Changes:

  • Selects in-process or TCP transport according to the test matrix.
  • Restores host environment state between in-process tests.
  • Adds same-client resume replacement behavior and related coverage.
Show a summary per file
File Description
dotnet/src/Client.cs Replaces tracked wrappers during resume.
dotnet/test/Harness/E2ETestFixture.cs Selects matrix-appropriate transport.
dotnet/test/Harness/E2ETestContext.cs Restores in-process environment state.
dotnet/test/Harness/E2ETestBase.cs Prepares tests and chooses resume client.
dotnet/test/E2E/SessionE2ETests.cs Covers same-client resume replacement.
dotnet/test/E2E/RpcWorkspaceCheckpointsE2ETests.cs Uses a native-compatible checkpoint value.
dotnet/test/Unit/E2ETestFixtureTests.cs Tests fixture transport selection.
dotnet/test/Unit/ClientSessionLifetimeTests.cs Tests replacement and failure lifetimes.

Review details

  • Files reviewed: 8/8 changed files
  • Comments generated: 1
  • Review effort level: Medium

Comment thread dotnet/src/Client.cs Outdated
@github-actions

This comment has been minimized.

@roji
roji enabled auto-merge July 14, 2026 17:13
@roji

roji commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator Author

@SteveSandersonMS noticed this while working on other stuff, take a look - we weren't doing actual FFI in the .NET SDK, plus the .NET SDK was unique in not allowing the same session ID to be resumed with a new session object (which obstructed fixing the former).

@roji
roji force-pushed the roji-fix-inprocess-e2e-transport branch from ad67cd3 to ce76fb0 Compare July 17, 2026 06:41
Make the in-process E2E matrix use the FFI runtime, restore the native host environment between tests, and align same-client session resume with the other SDKs while preserving routing after failed resumes.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: c71188a1-1445-46aa-9faf-3b73cf6a6dd9
@roji
roji force-pushed the roji-fix-inprocess-e2e-transport branch from ce76fb0 to e3f21ae Compare July 17, 2026 06:46
@github-actions

Copy link
Copy Markdown
Contributor

Cross-SDK Consistency Review ✅

This PR fixes a .NET-specific bug in ResumeSessionAsync: the old code used _sessions.TryAdd() which threw InvalidOperationException when resuming a session ID already tracked by the client. The fix replaces the session via AddOrUpdate and restores the previous session on RPC failure.

This change brings .NET into alignment with all other SDKs:

SDK Session registration in resumeSession Behavior on duplicate
Node.js this.sessions.set(sessionId, session) Silent overwrite ✅
Python self._sessions[session_id] = session Silent overwrite ✅
Go c.sessions[sessionID] = session Silent overwrite ✅
Java sessions.put(sessionId, session) Silent overwrite ✅
Rust self.sessions.lock().insert(...) Silent overwrite ✅
.NET (before) _sessions.TryAdd() ❌ Threw InvalidOperationException
.NET (after) _sessions.AddOrUpdate() Silent overwrite ✅

No cross-SDK consistency issues found. The PR correctly aligns .NET behavior with all other implementations.

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

  • awmgmcpg

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "awmgmcpg"

See Network Configuration for more information.

Generated by SDK Consistency Review Agent for #1986 · 64.1 AIC · ⌖ 7.56 AIC · ⊞ 5K ·

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.

2 participants