Skip to content

Fix managed SNI MARS receive teardown race - #4680

Draft
cheenamalhotra wants to merge 2 commits into
mainfrom
dev/cheena/mars-receive-teardown
Draft

cheenamalhotra wants to merge 2 commits into
mainfrom
dev/cheena/mars-receive-teardown

Conversation

@cheenamalhotra

@cheenamalhotra cheenamalhotra commented Sep 11, 2026

Copy link
Copy Markdown
Member

Description

Prevent unobserved receive-continuation exceptions during managed SNI MARS teardown.

  • Capture TCP and named-pipe streams once and report disposal through existing SNI errors.
  • Preserve the error packet until all MARS sessions are notified; release incomplete payloads.
  • Invoke error callbacks only for pending async reads and balance callback counts.
  • Account for valid late callbacks and buffered reads after connection closure in debug assertions.
  • Keep existing lock scope and public APIs unchanged.

Issues

Fixes #4679

Testing

Deterministic TCP/named-pipe coverage includes disposed streams, idle and pending sessions, cleared tasks, repeated errors, and packet reuse. Fixed the fake peer's unread SYN writes and write/read ordering.

31 focused unit tests and eight live SQL Server 2025 teardown cases pass on .NET 8, 9, and 10 Debug, plus .NET 9 Release. SQL cases are reported separately instead of running an opaque 200-iteration loop. Windows CI is pending for the latest correction.

Guidelines

  • Tests added or updated
  • Public API changes documented (none)
  • Original customer harness rerun (equivalent SQL-backed regression exercised)
  • Ensure no breaking changes introduced

Handle disposed TCP and named-pipe streams through SNI errors and preserve MARS packet ownership during receive re-arm failure. Add deterministic and SQL-backed regression coverage for #4679.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings September 11, 2026 16:16
@github-project-automation github-project-automation Bot moved this to To triage in SqlClient Board Sep 11, 2026
@cheenamalhotra cheenamalhotra added this to the 8.0.0-preview1 milestone Sep 11, 2026

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 concurrency-sensitive packet ownership changes require human review, particularly because the live SQL regression remains unexecuted.

Pull request overview

Fixes #4679 by safely handling managed SNI disposal while MARS re-arms receives.

Changes:

  • Captures TCP/named-pipe streams and reports disposal as SNI errors.
  • Preserves packet ownership during MARS error propagation.
  • Adds transport-level and SQL-backed regression coverage.
File summaries
File Description
SniTcpHandle.netcore.cs Handles disposed TCP streams safely.
SniNpHandle.netcore.cs Handles disposed named-pipe streams safely.
SniMarsConnection.netcore.cs Corrects packet lifetime during receive failures.
SniReceiveTests.cs Adds deterministic receive and packet-reuse tests.
MarsReceiveTeardownTest.cs Adds SQL-backed MARS teardown coverage.
Review details
  • Files reviewed: 5/5 changed files
  • Comments generated: 0
  • Review effort level: Balanced

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

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings September 11, 2026 18:20

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 concurrency-sensitive teardown and packet-ownership changes require final human review and completion of pending Windows CI.

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

@cheenamalhotra cheenamalhotra moved this from To triage to In progress in SqlClient Board Sep 11, 2026
@codecov

codecov Bot commented Sep 11, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 64.59%. Comparing base (ce9f01b) to head (f3d1232).
⚠️ Report is 15 commits behind head on main.

❗ There is a different number of reports uploaded between BASE (ce9f01b) and HEAD (f3d1232). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (ce9f01b) HEAD (f3d1232)
CI-SqlClient 1 0
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4680      +/-   ##
==========================================
- Coverage   71.88%   64.59%   -7.30%     
==========================================
  Files         290      284       -6     
  Lines       45056    68050   +22994     
==========================================
+ Hits        32389    43954   +11565     
- Misses      12667    24096   +11429     
Flag Coverage Δ
CI-SqlClient ?
PR-SqlClient-Project 64.59% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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

Labels

None yet

Projects

Status: In progress

2 participants