Fix prepared handle ownership across streamed execution

[Saurabh Singh (saurabh500)]

Description

Fix the third-rebind panic and prepared-handle leak after streamed execution is cancelled and retried.

Streamed sp_prepexec carries the old handle in its by-reference @handle, avoiding a separate cleanup round trip. The orphan and its cached encryption metadata remain owned until the complete message is sent. Only then is the new statement identity assigned, even if a post-send timeout or response failure occurs. Cancelling a parked request retains the old orphan without leaving an inert new identity that can overwrite it on the next rebind.

The new end_execute_prepared_param API closes prepared streams with the caller's statement and orphan slot. SQLParamData uses it for both immediate and deferred prepared DAE, restoring ownership before handling success or failure; ad-hoc streams continue using end_streamed_param. Reusing an already-live sp_execute still releases a separate cross-identity orphan through standalone sp_unprepare, because that RPC has no piggyback drop slot. Deviation entry 19 is narrowed to that case.

PacketWriter now distinguishes an attempted send from a successfully completed packet. A per-send incomplete flag survives suspension; shared retraction, streamed chunk failures, and withdrawal failures retire interrupted writes without IGNORE, attention, or TLS shutdown. This also covers interruption after earlier packets completed. Cancellation between complete packets remains retractable, and cancellation before any send remains locally discardable. Standalone unprepare uses the shared retirement path.

The shared timeout helper deducts whole elapsed seconds rather than rounding every sub-second step up, preserving one-second command budgets. Multi-step callers use the original budget and cumulative elapsed time; rounding leaves less than one second of slack, while exact/over-budget exhaustion still fails before sending.

Merged main (cc7bb82e) in 503fc43f; the parity registry now lives in mssql-odbc/docs/parity-deviations.md. Streamed piggyback landed in a8602cc9; the interrupted-send follow-up is 6297939e.

Merged main (50a315de) in 7bf8fc32, preserving its entry 18 and renumbering this PR's entry to 19. Post-merge workspace and Python formatting/Clippy pass; 4,066 workspace library tests pass. Live integration tests were not rerun locally; CI must validate the new merge commit.

Validation

Local validation for 6297939e:

• cargo bfmt and separate mssql-py-core formatting check pass.
• cargo bclippy, production mssql-tds --lib Clippy, and separate mssql-py-core --all-features --all-targets Clippy pass with -D warnings.
• cargo btest -E 'kind(lib)': 4,022 workspace library tests passed under coverage. This is not a complete live integration-suite run.
• Repeated one-second rebind/cancel/retry coverage asserts one streamed sp_prepexec with a non-NULL old handle and no standalone sp_unprepare. Separate tests cover cross-identity release, multiple parameters, cached encryption metadata, cancelled/failed final writes, and errors after a completed send.
• The ODBC regression runs three cancel/rebind cycles and then a completed request with a failed response, checking that the pending slot is never overwritten.
• The interrupted-send regression reproduced unsafe connection reuse before the fix. It now covers both chunk and final-packet sends, with and without a prior completed packet, asserting no cleanup bytes, attention, or transport shutdown. Repeated cancellation/chunk calls cannot write again. The healthy between-packets cancellation test confirms the connection remains reusable.
• Mutation checks: reducing the partial-send payload below a packet fails the partial-send assertion; removing the injected write delay fails the timeout assertion; moving cancellation one send later fails the interrupted-send regression. All setups were restored. The pending-send double reproduces an interrupted write; the EOF response double tests ownership on an error, not absence of a network hang.
• Live SQL Server/ODBC C++ end-to-end tests, native TLS interruption reproduction, and build-specific retail-driver wire/diagnostic comparisons were not rerun. Required CI must rerun on the pushed head; prior-head green checks do not validate this revision.

Historical results: a8602cc9 passed 4,020 library tests and 503fc43f passed 4,014; before the main merge, 2,211 TDS library tests and 3,911 workspace library tests passed. Seven initial missing-certificate failures cleared after running scripts/generate_mock_tds_server_certs.ps1. Earlier debug/release/reference-driver regressions and CI passed on 0b0bee43 (99% diff coverage), while an original full local run reported 3,925 passed / 405 failed / 14 skipped, mostly missing credentials/certificates. These historical results do not establish current-head live integration coverage; the earlier unqualified 2,009-test statement is superseded.

Related Issues

Fixes #598

The pre-existing materialized sp_prepexec piggyback ownership issue is tracked separately in #647.

Checklist

• cargo bfmt passes
• cargo bclippy passes
• cargo btest passes (library-only selection passed; full live suite not rerun locally)
• New/changed functionality has tests
• Public API changes are documented

[Copilot]

🟡 Changes recommended

An unsent cancelled sp_unprepare can discard the orphan ID and leak the live server handle.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Updates streamed prepared execution to release superseded handles before opening cancellable RPC streams.

Changes:

• Adds standalone sp_unprepare handling with shared timeout accounting.
• Adds unit and ODBC regressions for cancellation, rebinding, cleanup, and reuse.
• Updates ownership documentation.

File summaries

File	Description
mssql-tds/src/connection/tds_client.rs	Implements orphan release and unit coverage.
mssql-odbc/src/api/execute.rs	Updates streamed-execution ownership comments.
mssql-odbc/tests/e2e/tests/execute_test.cpp	Adds immediate and deferred streaming regressions.

Review details

• Files reviewed: 4/4 changed files
• Comments generated: 1
• Review effort level: Balanced (auto)

Note

Copilot is running an experiment and ran this review at Balanced.

---

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

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

Note

Copilot is running an experiment and ran this review at Balanced.

[github-actions]

📊 Code Coverage Report

🔥 Diff Coverage 98%

🎯 Overall Coverage 94.3%

📦 Project: mssql-tds + mssql-odbc + mssql-py-core ℹ️ Note: diff coverage is reported, not enforced.

---

Diff Coverage

Diff: main...HEAD, staged and unstaged changes

• mssql-odbc/src/api/execute.rs (100%)
• mssql-odbc/src/api/param_data.rs (42.9%): Missing lines 307-309,312-315,329-331,334-339
• mssql-tds/src/connection/tds_client.rs (99.8%): Missing lines 2447,3721
• mssql-tds/src/io/packet_writer.rs (100%)

Summary

• Total: 1017 lines
• Missing: 18 lines
• Coverage: 98%

mssql-odbc/src/api/param_data.rs

  303     }
  304 
  305     let (mut prepared, mut orphaned) = {
  306         let Ok(mut stmt_state) = stmt.inner.lock() else {
! 307             error!("SQLParamData: stmt mutex poisoned taking the streamed plan");
! 308             return_client_idle(dbc, statement_handle, client);
! 309             return SQL_ERROR;
  310         };
  311         let Some(dae) = stmt_state.dae.as_mut() else {
! 312             error!("SQLParamData: DAE sequence vanished before closing the parameter");
! 313             drop(stmt_state);
! 314             return_client_idle(dbc, statement_handle, client);
! 315             return SQL_ERROR;
  316         };
  317         (dae.take_prepared(), dae.take_orphaned())
  318     };
  319     let end_result = match prepared.as_mut() {

---

  325     // The send may have completed even when its response failed. Restore the
  326     // updated ownership before either completion or error tears down DAE.
  327     {
  328         let Ok(mut stmt_state) = stmt.inner.lock() else {
! 329             error!("SQLParamData: stmt mutex poisoned restoring the streamed plan");
! 330             return_client_idle(dbc, statement_handle, client);
! 331             return SQL_ERROR;
  332         };
  333         let Some(dae) = stmt_state.dae.as_mut() else {
! 334             error!("SQLParamData: DAE sequence vanished after closing the parameter");
! 335             stmt_state.prepared = prepared;
! 336             stmt_state.pending_unprepare = orphaned;
! 337             drop(stmt_state);
! 338             return_client_idle(dbc, statement_handle, client);
! 339             return SQL_ERROR;
  340         };
  341         dae.restore_plan(prepared, orphaned);
  342     }

---

mssql-tds/src/connection/tds_client.rs

  2443                 drop(message);
  2444                 if send_incomplete {
  2445                     self.retire_without_writing();
  2446                 } else {
! 2447                     self.abort_streamed_write().await;
  2448                 }
  2449                 Err(e)
  2450             }
  2451         }

---

  3717         orphaned: &mut Option<StatementId>,
  3718         options: ExecuteOptions<'_>,
  3719     ) -> TdsResult<()> {
  3720         let Some(id) = *orphaned else {
! 3721             return Ok(());
  3722         };
  3723         let result = self.execute_sp_unprepare(id, true, options).await;
  3724         if !self.prepared_handles.contains_key(&id) {
  3725             *orphaned = None;

---

---

🔗 Quick Links

View Azure DevOps Build · Coverage Report

[Copilot]

Copilot review overview

🔵 Needs a closer look

The cancellation-sensitive protocol and FFI ownership changes still lack a passing full post-merge validation run.

Review effort: Balanced
Findings: None

[ttk (Theekshna)]

Unattended hourly review sweep. Checked out at 11eff55a (merge-base b758050e against main, 5 files, +657/-51). Read all prior inline comments, reviews, and issue comments first — Copilot's earlier bot review flagged an unsent-cancellation handle leak on execute_sp_unprepare, and the author's reply plus the current diff show it fixed and mutation-tested (a failing zero-byte-cancellation test that a bytes-removed control makes pass): message.send_attempted()/nothing_sent() on SuspendedMessage distinguish "definitely never touched the wire" from "began writing, fate unknown," and I independently verified the mechanism against tokio-util 0.7.19's RunUntilCancelledFuture::poll (biased to the future; an already-cancelled token short-circuits before the future is polled at all, so send_attempted staying false really does prove nothing was sent). That thread is resolved and I did not re-file it.

Local build note: cargo nextest run -p mssql-tds --lib fails to even compile in this sandbox (E9012/E9013 codegen errors in crc32fast's AVX-512/vpclmulqdq path against this toolchain, reproduced both with and without -C target-feature=-avx512f,...) — the same pre-existing environment problem noted on PR #620's review, unrelated to this diff. I relied on the Code Coverage Report bot comment (99% diff coverage, 1 missing line) plus static/call-site analysis instead of a local mutation pass. gh pr checks is still fully pending on this exact head (11eff55a, pushed ~2h ago after merging main); no failures yet, nothing to flag there, and I'm not triggering or waiting on it further.

msodbcsql parity (check 1): the reference tree's DropPrepHandle (sqlcfunc.cpp:730-763) and the piggyback sites in sqlccmd.cpp:1642-1660 and :8244-8262 piggyback a same-statement superseded handle onto the next sp_prepare/sp_prepexec's by-reference @handle parameter — including for DAE/cursor-open RPCs (sqlccmd.cpp:6722-6726). I initially read this PR's new standalone sp_unprepare-before-stream as a divergence from that (an extra round trip), but @handle is a single by-reference slot carrying one statement's own old→new handle; it has no second slot for an unrelated statement's orphaned handle. Grepping this crate, orphaned: &mut Option<StatementId> is exactly that cross-identity case (a handle superseded by a different PreparedStatement id), and it is never piggybacked anywhere in this codebase, streamed or not — execute_prepared_batch_inner's existing None => arm (tds_client.rs:3830-3832, unchanged by this PR) already released a same-shaped orphaned via a standalone call before this PR touched the streamed path. So this isn't a new or streamed-specific divergence; it's the pre-existing convention extended consistently. No finding.

Six-check accounting: (3) Divergences — N/A per check 1, nothing to add to .github/instructions/mssql-odbc.instructions.md. (4) Description — see Nit below; linked issue #598 matches the diff's intent, checklist's unchecked cargo btest is honest given CI is still pending. (5) Verbose slop — none; new doc comments explain an invariant or the ownership contract (e.g. send_attempted's doc, unprepare's retry-safety note), and the e2e tests' Benefits-from-mock-tds: framing matches an existing convention (call_routing_test.cpp, describe_param_test.cpp, etc.), not new filler. (6) Evidence — re-derived the diff-coverage claim directly against the Code Coverage Report comment: 99%, exactly one missing line (tds_client.rs:3644), which is the same line my Suggestion below is about. I could not re-run the description's local full-suite numbers (2,009 unit tests; 3,925/405/14) — that requires a live SQL Server and local fixtures this sandbox doesn't have — so I'm not disputing them, only noting they're unverified here; 309 #[test]/#[tokio::test] functions in tds_client.rs alone make the total plausible.

Severity	Count
Blocking	0
Suggestion	1
Nit	1

[Nit] The description's checklist section ends with "Kept draft as requested," but this PR is not a draft (isDraft: false) and hasn't been since it was opened in this state. Leftover text like this reads as stale/contradictory to a reviewer checking currency — worth deleting.

This is an unattended automated review; findings may be incomplete or wrong, push back on anything that looks off.

[ttk (Theekshna)]

Reviewed at 11eff55a (merge-base b758050e). Read the prior Copilot and ttk (@Theekshna) reviews first; not re-filing the :3644 guard or the draft-text nit. One blocking finding, reproduced locally, plus two suggestions. Mechanism itself checks out — I mutation-tested the send_attempted guard, the retire_without_writing call, and the set-point of send_attempted relative to the await; each mutation fails a test, so the new tests are not vacuous.

[Vahid Beiranvand (Vahid-b)]

This is an unattended automated review. Findings were not checked by a human first — push back on anything that looks wrong.

Summary

Reviewed at 11eff55a (merge base b758050e against current origin/main, 5 files, +657/−51). The change is coherent and the mechanism holds up: execute_sp_unprepare stops evicting prepared_handles / prepared_param_encryption up front and instead evicts them only once SuspendedMessage::send_attempted() proves bytes were handed to the writer, retiring the connection when a send started but no packet flushed. begin_execute_prepared then releases a superseded handle in a standalone sp_unprepare before parking the cancellable stream, so the orphan is no longer carried into DaeState and restored by take_dae into an already-occupied pending_unprepare — which is exactly the orphan_prepared_handle: a pending unprepare already exists panic in #598. I traced that path end to end (stmt.rs:1398 → :1430, execute.rs:392-440) and the fix matches the reported repro.

No blocking findings from me. One suggestion concerns the sibling release path the PR deliberately did not change, and I confirmed it with a probe rather than by reading.

I did not re-file the four findings already open from ttk (@Theekshna) (:3644 redundant guard, :3950 deduct_timeout re-charge, :3895 deviation-registry entry, :16070 test gap) or the stale "Kept draft as requested" line. On :3950 I independently confirmed the arithmetic is real — deduct_timeout (tds_client.rs:1446-1451) rounds elapsed up to whole seconds, and CommandTimeoutBudget::Exhausted maps to TimeoutError in into_timeout (:1449, :1155) — so with timeout=1 any nonzero sub-second elapsed exhausts the budget. Worth noting that the same re-charge already ships on the materialized path (execute_prepared_batch_inner, unchanged by this PR), so it is a pre-existing shape this PR extends rather than introduces.

Verification performed

• Repos/policy. Fetched origin/main (120eab88) and refs/pull/599/head; merge base b758050e. Read .github/copilot-instructions.md, .github/instructions/pr-workflow.instructions.md, .github/instructions/mssql-odbc.instructions.md, and .github/skills/code-review/SKILL.md + posting.md from origin/main, not the checkout. Note: the brief for this run said the code-review skill file was absent — it is in fact present at origin/main:.github/skills/code-review/SKILL.md, so I applied it.
• Prior discussion. Read all three endpoints (pulls/599/comments, pulls/599/reviews, issues/599/comments) plus every review thread and its resolution state before drafting. Read issue #598 in full.
• Tests run locally (CARGO_TARGET_DIR redirected; main worktree cache untouched):
  cargo nextest run -p mssql-tds --lib -E 'test(orphan) + test(unprepare) + test(send_attempt) + test(retract_partial)' → 24 passed, 0 failed, including all six new ownership tests and io::packet_writer::tests::send_attempt_survives_suspend_and_resume.
• Probe (the basis for the Suggestion below). I added a temporary test driving execute_sp_prepexec_for_test with cancel_on_writer_creation — the same "cancelled before the writer's first send" hook this PR adds — and printed the resulting ownership state:
  PROBE err=true bytes_sent=0 orphan=None map_has_handle=false dead=false
  Restored afterwards; git status --porcelain is empty and the worktree is clean.
• Flush-ordering check. Confirmed any_packet_flushed / message_complete are set after send_data_fut.await? (packet_writer.rs:459-461) while send_attempted is set inside the cancellable future (:445-448), so the new send_attempted && nothing_sent pair genuinely distinguishes "never polled" from "polled, fate unknown". Verified the biased short-circuit semantics are what the cancel_on_writer_creation arm of begin_execute_prepared_release_cancellation_tracks_send_attempt relies on.
• Not verified: the two new C++ e2e tests — no live SQL Server or ODBC driver registration in this environment. They rest on the ADO validation leg, which passed on this head. I also did not re-run the description's local full-suite numbers (2,009 unit tests; 3,925/405/14); I am not disputing them, only noting they are unverified here.

Findings

Blocking

None.

Suggestion

1. execute_sp_prepexec still discards the orphan before the send boundary — the same window this PR closes for the streamed path. (mssql-tds/src/connection/tds_client.rs:4233-4236)

This PR's own rationale on begin_execute_prepared says it best:

Piggybacking the drop would let cancellation discard it while the server still held the plan.

That is a precise description of what the piggyback path still does. orphan.take() and both map evictions happen at RPC-build time — before the packet writer even exists — so a cancellation landing before the first flush loses the orphan. Measured, not inferred:

PROBE err=true bytes_sent=0 orphan=None map_has_handle=false dead=false

Zero bytes reached the server, yet the caller is told there is nothing left to release and the client has forgotten handle 7. The server still holds that plan, so it leaks until disconnect. Contrast the new execute_sp_unprepare behaviour on the identical hook, which begin_execute_prepared_release_cancellation_tracks_send_attempt pins: orphaned == Some(id), both map entries retained, connection reusable, release retryable.

Two supporting details:

• The doc contract at :4122-4128 says the orphan is "still Some if the call failed before serialization (the caller may retry the release)". The probe is a failure before anything was serialized to the wire, and it came back None — so the documented guarantee is narrower in practice than it reads.
• The pre-existing test execute_sp_prepexec_consumes_orphan_after_send_begins passes for a weaker reason than its name suggests. Because the take happens at build time, it would pass even if no send ever began; nothing currently pins the pre-send arm. execute_sp_prepexec_preserves_orphan_on_pre_send_error only covers the has_open_batch usage error, which returns before the take.

This is pre-existing on main and this PR strictly narrows the gap, so I am not treating it as blocking. But send_attempted() now exists precisely to close it, and leaving two of three release paths on the new contract and one on the old is the kind of asymmetry a later parity pass gets wrong. Suggest a follow-up issue rather than expanding this PR.

Nit

1. The retire comment describes only one of the two conditions that reach it. (mssql-tds/src/connection/tds_client.rs:3621-3623)

if serialize_result.is_err() && message.nothing_sent() {
    // A cancelled write may have left a partial packet on the wire.
    self.retire_without_writing();
}

Since any_packet_flushed is set only after send_data_fut.await? succeeds, this branch is also taken when network_writer.send() returns a plain I/O error, not just on cancellation. Retiring is the right action in both cases, so this is wording only — but "a cancelled write" reads as an exhaustive description of when the connection gets condemned, and it is not.

Required CI

coverage-report is failing on this head (run 35689467030). Reading the log, it is not a test or coverage regression — the job polls for the Cobertura artifact and gave up after 12 attempts:

Attempt 12: checking for coverage artifact...
⏳ Coverage artifact not ready yet (attempt 12)...
Available artifacts: none
❌ Timeout: coverage artifact not found within 75 minutes

Every other required check is green, including the full mssql-rs Pull request validation ADO pipeline (all five build legs, Test MacOS, Kerberos, and both cross-repo mssql-python legs) and Merge Coverage. Since the ADO validation now passed on this exact head, the unchecked cargo btest box in the description looks like it can be checked.

[Saurabh Singh (saurabh500)]

execute_sp_prepexec still discards the orphan before the send boundary.

Confirmed the build-time orphan.take() and map eviction ordering and tracked the pre-existing materialized-path issue in #647, rather than expanding this streamed-cleanup fix. The interrupted-write wording is corrected in f30e7a7 as well.

[Copilot]

Copilot review overview

🟡 Changes recommended

Pending handle ownership must be preserved when cleanup fails before sending.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1

Open (1)

• Restore pending unprepare IDs when cancellation fails before send · New

[David Engel (David-Engel)]

Automated review (unattended run)

Reviewed the full diff against merge-base cc7bb82e (10 files, +1438/-144), focused on the streamed sp_prepexec piggyback, the send-ownership boundary in PacketWriter, and the ODBC SQLParamData plan restoration. This came from an unattended run, so the findings below were not checked by a human first.

The design holds up: ownership settles on message_complete() rather than at park time, cancellation retains the orphan without minting an inert identity, and the ODBC layer restores the plan on both success and failure. Verified the new tests are not vacuous with two mutations (cargo nextest run -p mssql-tds --lib, 35 prepared/streamed tests):

• forcing the streamed @handle back to SqlType::Int(None) fails begin_execute_prepared_piggybacks_each_orphan_with_one_second_timeout;
• inverting the message.message_complete() gate fails 9 tests, including prepared_stream_partial_cancel_retains_orphan_and_metadata and prepared_stream_completed_send_consumes_orphan_on_error.

Also re-ran begin_execute_prepared_release_cancellation_tracks_send_attempt 40x to check the new send_attempted flag for a race when the handle is already cancelled: 40/40 deterministic, because CancellationToken::run_until_cancelled has an is_cancelled() fast path that never polls the inner future. No flakiness there.

No blocking findings. Two non-blocking items are left inline:

1. Suggestion — withdraw_sent_message samples send_incomplete only after writing the IGNORE packet (tds_client.rs:2644).
2. Nit — duplicated live_handle lookup in begin_execute_prepared (tds_client.rs:4034).

PR-wide (no attachable diff line): the two red checks are infrastructure, not this PR. The macOS job ran Run tests (mssql-tds and mssqlodbc packages) to success; it then failed on Publish Cobertura coverage artifact with ##[error]Artifact CoberturaCoverageRust_MacOS already exists for build 177888, which skipped the ODBC e2e steps and failed Publish ODBC e2e test results (macOS). coverage-report then failed only because it refuses to report against a failed ADO build. Worth a re-run rather than a code change — but the e2e C++ cases added here (RebindAfterCancelledStreamReleasesPriorHandle, RebindAfterDeferredStreamReleasesPriorHandle) have therefore not actually executed on macOS in this build.

[Saurabh Singh (saurabh500)]

/azp run

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[Vahid Beiranvand (Vahid-b)]

This is an unattended automated review. Findings were not checked by a human first — push back on anything that looks wrong.

Summary

Reviewed the full diff at 6297939e against merge base cc7bb82e (10 files, +1438/−144). The ownership redesign holds up under everything I could check here. The settle-on-message_complete() boundary in end_streamed_param_inner (tds_client.rs:2908-2923), the send_attempted / send_incomplete split in PacketWriter (packet_writer.rs:443-457), and the take/restore around end_execute_prepared_param in SQLParamData (param_data.rs:305-342) trace cleanly through the cancel, interrupted-send, completed-send-then-error, multi-parameter and cross-identity paths. I also confirmed the structural reason take_dae's unconditional self.pending_unprepare = dae.orphaned (stmt.rs:1444) is now safe: orphan_prepared_handle early-returns while a plan is parked in DAE because StmtState::prepared is None, so nothing can occupy the slot the panic in #598 tripped over.

No blocking findings. Two suggestions, both on the parity-registry entry this PR adds — the one ledger row no prior reviewer has audited.

Three reviews already cover this exact head (ttk (@Theekshna), David Engel (@David-Engel) with the :2644 suggestion and :4034 nit, and Copilot). I am not re-filing any of those, nor the resolved :3721, :4029, :4142 or :16591 threads. In particular I read David Engel (@David-Engel)'s :2644 thread before drafting, and I agree cancel_streamed_write → withdraw_sent_message is the path that relies on the invariant implicitly — nothing to add.

Verification

• Live state. Confirmed repository microsoft/mssql-rs, PR #599 open, non-draft, author Saurabh Singh (@saurabh500), head 6297939e, base main, immediately before posting. mergeable: true.
• Repos. Fetched origin/main (27707b21); merge base with this head is cc7bb82e. Read .github/copilot-instructions.md, .github/instructions/pr-workflow.instructions.md, .github/instructions/mssql-odbc.instructions.md and .github/skills/code-review/SKILL.md + posting.md from origin/main, not from the checkout.
• Prior discussion. Read all three endpoints (pulls/599/comments, pulls/599/reviews, issues/599/comments) plus every review thread and its resolution state before drafting, and confirmed #598 and #647 are both open.
• Tests run locally (detached worktree, CARGO_TARGET_DIR redirected so the main worktree cache is untouched):
  cargo nextest run -p mssql-tds --lib -E 'test(prepared_stream) + test(begin_execute_prepared) + test(deduct_timeout) + test(send_attempt) + test(retract_partial) + test(cancel_streamed_write)' → 37 passed, 0 failed.
• Mutation (novel — checking the description's claim that the interrupted-send regression "now covers both chunk and final-packet sends"). In write_streamed_chunk's error arm (tds_client.rs:2445) I forced send_incomplete to false, so an interrupted chunk write would fall back to abort_streamed_write() (which closes the transport) instead of retire_without_writing(). cargo nextest run -p mssql-tds --lib -E 'test(prepared_stream) + test(begin_execute_prepared) + test(streamed)' → 50 tests, 49 passed, 1 failed: prepared_stream_interrupted_send_retires_without_writing_again. The chunk arm is genuinely guarded, not just the finalize arm. Restored afterwards; git status --porcelain shows only the untracked cargo-target/ this review created.
• Branch coverage spot-checks. The new end_streamed_param_inner usage guard ("Prepared streams must be closed with end_execute_prepared_param", tds_client.rs:2850) is pinned — prepared_stream_partial_cancel_retains_orphan_and_metadata asserts it at :16263, and also that the guard leaves the stream usable afterwards. The attention-after-complete-send case is pinned by prepared_stream_completed_send_consumes_orphan_on_error (delayed_send = true, asserting attentions == 1 alongside orphaned.is_none()), which is what Suggestion 2 below is about.
• Required CI. Every check is green on this head after the /azp run re-trigger: ADO mssql-rs Pull request validation build 178213 passes on all five build legs, Test MacOS (48m18s), Kerberos, both cross-repo mssql-python legs and Merge Coverage; GitHub coverage-report, CodeQL, Analyze and check all pass. The macOS Artifact CoberturaCoverageRust_MacOS already exists collision David Engel (@David-Engel) reported on the earlier build is not present in 178213.
• Not verified. There is no msodbcsql checkout on this host (Test-Path on the three plausible roots returns False for all), so I could not re-derive the DropPrepHandle / BuildSPPrepExec / ProcessDAEParam citations in deviation entry 18 — Suggestion 2 is an internal-consistency argument against the entry's own stated reading, not a claim about msodbcsql. I also did not run the C++ e2e suite (no live SQL Server / driver registration here); RebindAfterCancelledStreamReleasesPriorHandle and RebindAfterDeferredStreamReleasesPriorHandle rest on ADO 178213. I did not re-run the description's 4,022-test figure.

Findings

Blocking

None.

Suggestion

Two, filed inline on mssql-odbc/docs/parity-deviations.md.

Nit

The cargo btest checklist box can be checked now. The description qualifies it as "library-only selection passed; full live suite not rerun locally", which was accurate when written — but ADO 178213 on this exact head runs Test MacOS, the Kerberos leg and both cross-repo mssql-python legs to green, so the full suite has run on this revision even though it did not run locally.

PR-wide note — the merge base has moved

origin/main is now 27707b21, nine commits ahead of this PR's base cc7bb82e, and three of those commits touch files this PR also edits:

File	Change on main since cc7bb82e
mssql-odbc/src/api/exec_common.rs	+294 (#643, #644)
mssql-odbc/docs/parity-deviations.md	+21/−10 (#643)
mssql-odbc/src/handles/stmt.rs	+24

I checked the two ways this normally bites and both are clear: GitHub reports mergeable: true, and main's registry still tops out at entry 17, so this PR's entry 18 does not collide. Nothing to do now — just worth a git fetch origin main and a re-diff right before merge rather than relying on 178213, which was computed against cc7bb82e.

[David Engel (David-Engel)]

Automated review — generated by GitHub Copilot on behalf of David Engel (@David-Engel). This is not an approval and does not satisfy the human review requirement. Findings may be incomplete or wrong; push back on anything that looks off.

Summary

Reviewed the full diff at 6297939e against merge base cc7bb82e (10 files, +1438/−144). The ownership redesign holds up. Settling on message.message_complete() in end_streamed_param_inner rather than at park time, the send_attempted / send_incomplete split in PacketWriter, and the take/end_execute_prepared_param/restore_plan window in SQLParamData trace cleanly through cancel, interrupted-send, completed-send-then-error, multi-parameter and cross-identity paths. I re-derived the deduct_timeout round-up→truncate change against every call site in tds_client.rs and cursor_ops.rs: each one passes either a single reconnect_elapsed or a cumulative started.elapsed() measured before check_and_reconnect, so no caller double-charges or forgets recovery time. I also checked the streamed_params.is_empty() early return, which delegates to execute_prepared and therefore still assigns statement.id and consumes the orphan even though begin_execute_prepared no longer does so itself.

No blocking findings. One suggestion and one nit, both filed inline. Neither is about the Rust ownership mechanism.

Prior reviews from ttk (@Theekshna), David Engel (@David-Engel), Vahid Beiranvand (@Vahid-b) and Copilot already cover this exact head. I am not re-filing the :2644 withdraw_sent_message sampling suggestion, the :4034 duplicated live_handle nit, the two parity-deviations.md content suggestions, the checklist nit, or any resolved thread.

Verification

• Live state. Confirmed author Saurabh Singh (@saurabh500), head 6297939e, base main, PR open and non-draft, immediately before posting.
• Repos. Fetched origin/main (now 50a315de); merge base with this head is cc7bb82e. Read .github/skills/code-review/SKILL.md, .github/copilot-instructions.md and .github/instructions/mssql-odbc.instructions.md from the checkout.
• Prior discussion. Read all reviews, issue comments and all 21 inline review comments before drafting.
• Tests run locally (detached worktree, CARGO_TARGET_DIR redirected): cargo nextest run -p mssql-tds --lib -E 'test(prepared_stream) + test(begin_execute_prepared) + test(deduct_timeout) + test(send_attempt) + test(retract_partial) + test(unprepare)' → 36 passed, 0 failed, including send_attempt_survives_suspend_and_resume and all the new prepared-stream ownership tests.
• Merge check. git merge-tree --write-tree HEAD origin/main reports exactly one conflict, in mssql-odbc/docs/parity-deviations.md. GitHub agrees: mergeable: CONFLICTING. Everything else auto-merges, including exec_common.rs, stmt.rs and parameters_plan.md.
• Required CI. All 18 checks are green on this head, including ADO build 178213 (all five build legs, Test MacOS, Kerberos, both cross-repo mssql-python legs, Merge Coverage) and GitHub coverage-report, CodeQL, Analyze and check. The earlier macOS Cobertura artifact collision is gone.
• Not verified. No live SQL Server or registered ODBC driver here, so RebindAfterCancelledStreamReleasesPriorHandle and RebindAfterDeferredStreamReleasesPriorHandle rest on ADO 178213. No msodbcsql checkout, so I could not re-derive the DropPrepHandle / BuildSPPrepExec / ProcessDAEParam citations in deviation entry 18. I did not re-run the description's 4,022-test figure.

Findings

Blocking

None.

Suggestion

1. The new parity registry entry is numbered 18, but main has since landed its own entry 18 — this is the sole merge conflict on the PR right now (mssql-odbc/docs/parity-deviations.md, inline).

Nit

1. The rewritten DaeState::orphaned doc omits the streamed sp_prepexec piggyback this PR is actually about (mssql-odbc/src/handles/stmt.rs, inline).

[Copilot]

Copilot review overview

🔵 Needs a closer look

It changes cancellation-sensitive network and ODBC ownership behavior, while current-head platform and live validation remains in progress.

Review effort: Balanced
Findings: 1

Open (1)

• Use correct method name in idle-state parameter error · New

[David Engel (David-Engel)]

Automated review — generated by GitHub Copilot on behalf of David Engel (@David-Engel). This is not an approval and does not satisfy the human review requirement. Findings may be incomplete or wrong; push back on anything that looks off.

Summary

The ownership model holds up. sp_prepexec now carries the orphan in its by-reference @handle, end_execute_prepared_param settles both the new statement identity and the orphan eviction from message.message_complete() rather than from the Result, and PacketWriter's split of send_attempted / send_incomplete correctly distinguishes "never reached the wire" (locally discardable), "complete packets only" (retractable with EOM | IGNORE) and "interrupted mid-packet" (retire without writing again). The deduct_timeout truncation now matches deduct_query_timeout, and every multi-step caller I checked passes the original budget with cumulative elapsed time, so the sub-second slack stays bounded at under one second rather than accumulating. Parity entry renumbering to 19 is clean — the registry reads 1–19 with no gaps or duplicates. One suggestion below, on a new ODBC error path.

Verification

• Merge base 50a315de; reviewed the full git diff $BASE..HEAD (10 files, +1438/-144) plus surrounding unchanged code in tds_client.rs, packet_writer.rs, exec_common.rs, stmt.rs and param_data.rs.
• Read all 20 prior reviews, 15 review threads and the top-level comments first; nothing below restates an already-answered thread.
• cargo nextest run -p mssql-tds --lib --no-fail-fast: 2234 run, 2227 passed, 7 failed — all 7 are the pre-existing certificate_validator / win_tls fixture failures that also fail on the merge base (missing generated tests/test_certificates/*.pem), not this PR.
• Mutation check on the core claim. Replaced if message.message_complete() with if true in end_streamed_param_inner (i.e. settle ownership from the Result instead of the wire state) and re-ran the prepared-stream tests: 6 of 19 failed — prepared_stream_failed_final_send_retains_ownership, prepared_stream_multiple_params_assigns_id_only_after_last, prepared_stream_partial_cancel_retains_orphan_and_metadata, begin_execute_prepared_cancelled_final_send_keeps_orphan_for_retry, prepared_stream_interrupted_send_retires_without_writing_again, prepared_stream_cancellation_between_packets_keeps_connection_reusable. The new tests genuinely guard the settle boundary; the source was restored afterwards.
• Traced every Self::deduct_timeout call site in tds_client.rs and cursor_ops.rs: all are either single-step (reconnect_elapsed) or cumulative from a started instant captured before recovery, so the truncation change cannot compound across steps.
• Verified execute_sp_unprepare's new send_attempted() gate is evaluated after SqlRpc::serialize (message/rpc.rs:257 finalizes and flushes inside serialize), so a successful release still evicts the map entry and unprepare_orphan still clears the caller's slot.
• Not run: live SQL Server / ODBC C++ e2e, TLS interruption reproduction, reference-driver wire comparison.

Findings

Blocking

None.

Suggestion

1. mssql-odbc/src/api/param_data.rs:308 — the two new bail-outs that take the streamed plan return the client to the idle pool without cancelling the still-open streamed write. Details inline.

Nit

None.

CI

mssql-rs Pull request validation (Build Stage Build Linux ARM) is failing on this head (7bf8fc32, build 178265); several other legs are still pending. I could not read the ADO job log from this environment, so I cannot say whether the failure is this PR's or infrastructure. Worth confirming before merge — the description's checklist is honest that the full suite was not rerun locally for this merge commit.

[Saurabh Singh (saurabh500)]

/azp run

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[Vahid Beiranvand (Vahid-b)]

This is an unattended automated review. Findings were not checked by a human first — push back on anything that looks wrong.

Summary

Re-reviewed at 7bf8fc32 (merge base 50a315de, which is current origin/main; 10 files, +1438/−144). I previously reviewed this PR at 6297939e.

The PR's own content is byte-identical to what I reviewed at 6297939e. I diffed the two contribution diffs (cc7bb82e...6297939e vs 50a315de...7bf8fc32) against each other: both are 2165 lines, and the only content difference is 18. → 19. on the parity entry. Everything else that moved is hunk offsets and blob hashes. So the main merge in 7bf8fc32 changed nothing about the ownership redesign, and my earlier assessment of the mechanism carries over unchanged.

The merge itself is clean in both directions: main's entry 18 (Authentication=ActiveDirectoryPassword is refused) is preserved, this PR's entry is renumbered to 19, and the registry reads 1–19 with no gaps or duplicates. Since the merge base is origin/main, git diff origin/main..HEAD touching only the PR's 10 files also confirms the merge reverted nothing from main.

No findings. Both suggestions I filed at 6297939e were answered and resolved, and all 16 review threads on this PR are resolved. One required check is red — see below; I believe it is a re-trigger timing artifact rather than a code defect, but it needs a human to re-run it.

Verification performed

• Live state. Confirmed microsoft/mssql-rs, PR #599 open, non-draft, author Saurabh Singh (@saurabh500), head 7bf8fc32, base main, labels empty — checked before reading the diff and again immediately before posting.
• Prior discussion. Read all three endpoints (pulls/599/comments, pulls/599/reviews, issues/599/comments) plus every review thread and its resolution state before drafting. All 16 threads are resolved, including David Engel (@David-Engel)'s :2644 / :4034 / param_data.rs:308 and both of my parity-deviations.md suggestions. Nothing below re-files an answered thread, and I am not restating David Engel (@David-Engel)'s review at this same head.
• Semantic merge check — the gap prior reviews left. main added +294 to exec_common.rs and +24 to stmt.rs since this PR's old base, and this PR edits both. A semantic conflict there would not show as a textual one. Prior reviewers ran mssql-tds only, so I built and ran the ODBC crate: cargo nextest run -p mssqlodbc --lib --no-fail-fast → 1796 run, 1796 passed, 0 failed. The merge is semantically clean for the crate main changed most.
• PR core tests at the merged head. cargo nextest run -p mssql-tds --lib -E 'test(prepared_stream) + test(begin_execute_prepared) + test(deduct_timeout) + test(send_attempt) + test(retract_partial) + test(cancel_streamed_write) + test(streamed)' → 61 run, 61 passed.
• The description's test count reproduces exactly. cargo nextest run --workspace --lib --no-fail-fast → 4066 run, 4059 passed, 7 failed — matching the description's "4,066 workspace library tests" precisely. All 7 failures are the documented pre-existing certificate_validator / win_tls::validate fixture failures (I did not run scripts/generate_mock_tds_server_certs.ps1), not this PR.
• deduct_timeout truncation, re-derived. The new body uses elapsed.as_secs() with no round-up. The zero-inversion hazard is handled correctly: a computed remaining of 0 becomes Exhausted (via NonZeroU32::new returning None), while a caller-supplied Some(0) returns None/unlimited from the earlier guard — the two zeros stay distinct. I independently traced every Self::deduct_timeout call site in tds_client.rs and cursor_ops.rs: the multi-step ones (:3899, :3910, :3932, :4029) all pass the original budget with started.elapsed() from a single Instant captured at :3896, so there is no per-step re-seeding and the slack stays bounded under one second rather than compounding. exec_common.rs's rewritten doc comment now matches that behaviour.
• Parity registry. Enumerated all top-level entries in parity-deviations.md: 1–19, no gaps, no duplicates; entry 19 is appended after main's entry 18 without disturbing it.
• Deferral tracking. Confirmed #598, #647 and #659 all exist and are open. The param_data.rs:308 bail-outs David Engel (@David-Engel) raised were deferred to #659 — a filed issue, which satisfies the repo's "deferrals need a filed item, not a code comment" rule.
• e2e harness (a ledger row no prior review covered). Read the +124 in execute_test.cpp. Both new tests carry the required Benefits-from-mock-tds: comments; RebindAfterCancelledStreamReleasesPriorHandle asserts payload.size() > packet_size so the "cancel must follow a packet flush" premise fails loudly rather than silently passing, and the SQLGetData loop has a forward-progress guard. The actual.append(chunk) NUL-stop is safe here because the generated payload is '!' + n % 90, so it contains no zero bytes — correct for a varchar(max) / SQL_C_CHAR case.
• Not verified. No live SQL Server or registered driver on this host, so RebindAfterCancelledStreamReleasesPriorHandle / RebindAfterDeferredStreamReleasesPriorHandle rest on CI. No msodbcsql checkout here, so entry 19's DropPrepHandle / BuildSPPrepExec / ProcessDAEParam citations are unre-derived — ttk (@Theekshna) reported confirming them against sqlccmd.cpp independently. I did not run the C++ e2e suite, the TLS interruption reproduction, or a retail-driver wire comparison.

Findings

Blocking

None.

Suggestion

None. Both suggestions I raised at 6297939e were answered and resolved by the author, and I found nothing new in the merge delta.

Nit

None.

Withdrawing one of my own: at 6297939e I nitted that the cargo btest checklist box could be ticked because CI had gone green on that head. That no longer holds — 7bf8fc32 is a new head whose validation build is still in flight, so the description's qualifier ("library-only selection passed; full live suite not rerun locally") is the accurate statement for this revision. The description already says CI must revalidate the merge commit, which is right.

Relevant required-CI failures

coverage-report — failure (run 36188613280), completed 22:12:38Z after 1h17m. This finished after every prior review on this head was posted, so it is not reflected in them.

It is a poll timeout, not a coverage regression. The job waits for the merged artifact named exactly CoberturaCoverage (.github/workflows/pr-code-coverage.yml:81) with a 75-minute deadline (:179, :207). The log's own "Available artifacts" list shows the per-leg artifacts present at every attempt — CoberturaCoverageRust_Linux, CoberturaCoverageRust_Windows, CoberturaCoverageRust_MacOS, CoberturaCoverageOdbcE2E_Linux, CoberturaCoverageOdbcE2E_Windows, CoberturaCoveragePyCoreRust, CoberturaCoveragePython — but never the merged one, because the ADO Merge Coverage job that publishes it had not run. The job polled from 20:55:30Z and gave up at 22:12:36Z on attempt 12.

The likely cause is the /azp run at 21:57Z, which started ADO build 178279 roughly 62 minutes into this job's 75-minute window, so the artifact it was pinned to was never going to arrive in time. Suggest re-running coverage-report once 178279 finishes rather than treating it as a code signal.

Two related notes on CI state:

• David Engel (@David-Engel)'s review at this head reported Build Stage Build Linux ARM failing on build 178265. That build is superseded — the same leg is now in_progress on 178279 (started 21:58:16Z), so the earlier red is stale and should be judged on 178279.
• Everything else on this head is currently pass or pending: Kerberos Authentication Tests, Evaluate PR duplicate, Analyze (actions), CodeQL, check and license/cla pass; the remaining ADO legs are still running.

Because a required check is currently red, I have not applied the ready for human review label (the PR carries no labels today, and I made no label change).

[David Engel (David-Engel)]

Automated review — generated by GitHub Copilot on behalf of David Engel (@David-Engel). This is not an approval and does not satisfy the human review requirement. Findings may be incomplete or wrong; push back on anything that looks off.

Summary

Re-reviewed the full diff at 7bf8fc32 against merge base 50a315de (10 files, +1438/−144). The ownership redesign holds up: end_streamed_param_inner settles the new statement identity and the piggybacked orphan from message.message_complete() rather than from the Result, so a post-send timeout or a failed response drain still transfers ownership while a cancellation before the send boundary retains it; PacketWriter's send_attempted / send_incomplete split correctly separates "never polled a send" (locally discardable), "only complete packets" (retractable with EOM | IGNORE) and "interrupted mid-packet" (retire without IGNORE, attention, or TLS shutdown); and SQLParamData's take/close/restore_plan window keeps the plan and orphan recoverable on every exit. No findings.

I read all three discussion endpoints first. Everything I independently arrived at was already filed and answered — the withdraw_sent_message sampling suggestion (tds_client.rs:2644), the duplicated live_handle lookup (tds_client.rs:4034), and the param_data.rs:308 bail-outs deferred to #659 — so nothing is re-filed here. A prior unattended review already covered this exact head; the two things this pass adds are a mutation check of the new regression tests and a refreshed CI reading.

Verification performed

• Live state. Author Saurabh Singh (@saurabh500), PR open, non-draft, head 7bf8fc32, base main, no labels — checked before reading the diff and again immediately before posting.

• Targeted tests (dedicated worktree, redirected CARGO_TARGET_DIR): cargo nextest run -p mssql-tds --lib -E 'test(/streamed|prepexec|deduct_timeout|send_attempt|unprepare|orphan/)' → 66 run, 66 passed.

• Mutation — do the new tests actually guard the change? Two independent mutations, then restored:

  • Removed self.send_incomplete = true from the send future (packet_writer.rs:448), so an interrupted write would look clean. → prepared_stream_interrupted_send_retires_without_writing_again fails.
  • Disabled the message.message_complete() settle in end_streamed_param_inner (tds_client.rs:2918). → prepared_stream_completed_send_consumes_orphan_on_error, prepared_stream_multiple_params_assigns_id_only_after_last, begin_execute_prepared_piggybacks_each_orphan_with_one_second_timeout, begin_execute_prepared_cancelled_final_send_keeps_orphan_for_retry, begin_execute_prepared_release_cancellation_tracks_send_attempt and begin_execute_prepared_streams_into_sp_prepexec_when_unmaterialized all fail.

  Both boundaries are genuinely covered, not asserted around. git status --porcelain is clean apart from this review's own scratch files.

• execute_sp_unprepare's new send_attempted() gate. Traced that SqlRpc::serialize finalizes internally (message/rpc.rs:257), so the flag is sampled after the whole message has gone out, not before finish_send — a successful unprepare still evicts both map entries, and only a pre-send failure retains them for retry. unprepare_orphan's !contains_key check derives orphan ownership from that same eviction.

• deduct_timeout truncation. Re-derived every call site in tds_client.rs and cursor_ops.rs: each passes either a single reconnect_elapsed or cumulative started.elapsed() from an Instant taken before recovery, so the sub-second slack stays bounded under one second and cannot compound. begin_execute_prepared recomputes from the original opts.timeout, so the orphan release is not charged twice.

• begin_execute_prepared routing. The streamed_params.is_empty() early return still delegates to execute_prepared, which assigns the id and consumes the orphan the old way; the sp_execute branch releases a cross-identity orphan through standalone sp_unprepare first and propagates its failure without opening a stream.

• Not verified. No live SQL Server or registered driver on this host, so the C++ e2e cases rest on CI; no msodbcsql checkout, so entry 19's DropPrepHandle / BuildSPPrepExec / ProcessDAEParam citations are unre-derived here.

Findings

Blocking

None.

Suggestion

None.

Nit

None.

It looks ready for human review — but this is not an approval, and one required check is still red (below).

Relevant required-CI failures

coverage-report — failure (run 36188613280), a 75-minute poll timeout waiting for the merged CoberturaCoverage artifact rather than a coverage regression; this was already diagnosed on this head. Worth noting what has changed since that diagnosis: ADO build 178279 has now finished and every ADO leg on this head passes, including Build Stage Build Linux ARM (26m30s), which supersedes the earlier red reported against build 178265. Analyze (actions), CodeQL, check and license/cla also pass. A re-run of coverage-report now that 178279 is complete should clear the last red check.

Because a required check is red, no label was applied and no label change was made (the PR carries no labels).

[ttk (Theekshna)]

Unattended hourly review sweep found no new findings. This pass was triggered by the merge-main commit (7bf8fc32) resolving the parity-deviations.md conflict David-Engel flagged; the PR's own net diff (vs current main) is unchanged from my prior two clean passes (10 files, +1438/-144).

Covered: verified the merge correctly renumbered/preserved the PR's divergence entry as #19 (diffed origin/main's copy against this head's copy of parity-deviations.md — pure addition, nothing dropped or duplicated) and that the PR description's own "entry 18/19" self-reference was updated to match; confirmed all 16 review threads (Copilot bot, David-Engel, Vahid-b, mine) are resolved, including the exact renumbering/wording concerns this merge commit addresses, and that the one deferred suggestion (param_data.rs:308, connection-stuck-after-internal-failure) was pushed to a real tracked follow-up (#659, open) rather than silently dropped; git diff origin/main:mssql-odbc/src/api/exec_common.rs and :mssql-odbc/src/handles/stmt.rs against this head show only the PR's own prior doc-comment wording, so the merge introduced no logic changes to already-reviewed code; cargo check -p mssql-tds --lib --tests --offline and cargo check -p mssqlodbc --lib --offline both compile clean post-merge (mutation-test build for cargo nextest still blocked in this sandbox by the pre-existing crc32fast AVX-512 codegen failure, unrelated to this PR). gh pr checks 599 is green on all 17 ADO/GitHub Actions stages except coverage-report, which times out after 75 minutes polling for a coverage artifact that the underlying ADO "Merge Coverage" stage itself reports as passed — an infra/polling issue, not a code regression; the PR's own "Code Coverage Report" bot comment is stale (2026-09-17, predates this session's pushes by over a week) and shouldn't be read as current-head coverage.

This is an unattended automated review; findings may be incomplete or wrong, push back on anything that looks off.

[David Engel (David-Engel)]

Automated review — generated by GitHub Copilot on behalf of David Engel (@David-Engel). This is not an approval and does not satisfy the human review requirement. Findings may be incomplete or wrong; push back on anything that looks off.

Summary

Re-reviewed the full diff at 7bf8fc32 against merge base 50a315de (10 files, +1438/−144). The ownership model holds up end to end: begin_execute_prepared piggybacks the orphan in sp_prepexec's by-reference @handle and defers both the new statement identity and the orphan eviction to message.message_complete() in end_streamed_param_inner, so a post-send timeout or failed response drain still transfers ownership while anything before the send boundary retains it; PacketWriter's send_attempted / send_incomplete split cleanly separates "no send polled" (locally discardable), "only complete packets" (retractable with EOM | IGNORE) and "interrupted mid-packet" (retire without IGNORE, attention, or TLS shutdown); and SQLParamData's take / close / restore_plan window keeps the plan and orphan recoverable on both success and error. No findings.

This is an unattended run at a head three prior passes already reviewed. Rather than restate their conclusions, this pass adds two mutation checks nobody had run and an independent trace of the state predicates. Nothing below re-files an answered thread — all 16 review threads are resolved, including tds_client.rs:2644, :4034, parity-deviations.md:515 / :531, stmt.rs:913 and param_data.rs:308 (deferred to #659).

Verification

• Live state. Author Saurabh Singh (@saurabh500), PR open, non-draft, head 7bf8fc32, base main, label ready for human review present — checked before reading the diff and again immediately before posting. Read all three discussion endpoints plus every review thread and its resolution state first.
• Predicate consumers, traced exhaustively. Every non-test use of nothing_sent() / send_incomplete() / send_attempted() / message_complete() in mssql-tds is accounted for: write_streamed_chunk:2442, cancel_streamed_write:2516, retract_partial_request:2556-2568, withdraw_sent_message:2644, end_streamed_param_inner:2918 and execute_sp_unprepare:3699. No call site was left reading the old any_packet_flushed semantics, and the cancel_streamed_write invariant the author defended on :2644 does hold: all three paths that re-park a message (Ok(Some(next)) after a successful write, and the two error paths that take the context out with mem::replace) leave send_incomplete == false or never re-park at all.
• Mutation 1 — is the new send_attempted() gate in execute_sp_unprepare actually guarded? (Not previously checked.) Replaced if message.send_attempted() with if true, so an unsent release would evict the map entries as before this PR. → begin_execute_prepared_release_cancellation_tracks_send_attempt and prepared_batch_unsent_release_keeps_orphan_for_retry_with_one_second_timeout both fail (64/66 pass). Restored.
• Mutation 2 — is the ODBC-side restore-on-error guarded? (Not previously checked; prior passes mutated only mssql-tds.) Made dae.restore_plan(prepared, orphaned) conditional on end_result.is_ok(), i.e. the pre-PR shape where a failed response loses the updated plan. → streamed_cancel_rebind_and_failed_response_preserve_ownership fails at execute.rs:2254. Restored.
• Targeted suites (assigned worktree, dedicated CARGO_TARGET_DIR): cargo nextest run -p mssql-tds --lib -E 'test(/streamed|prepexec|deduct_timeout|send_attempt|unprepare|orphan/)' → 66 run, 66 passed; cargo nextest run -p mssqlodbc --lib -E 'test(/streamed_cancel_rebind|param_data|deferred/)' → 20 run, 20 passed, and streamed_cancel_rebind_and_failed_response_preserve_ownership on its own → 1 passed. git status --porcelain is clean after both mutations were reverted.
• API-break surface. end_streamed_param now rejects a prepared stream with a usage error, so I checked every caller of begin_execute_prepared / end_streamed_param / end_execute_prepared_param outside tds_client.rs: only mssql-odbc's execute.rs and param_data.rs, and their routing key (dae.prepared.is_some()) matches the client's (ctx.prepared.is_some()) on all three open paths — immediate prepared execute, deferred open_deferred_rpc, and SQLExecDirect's ad-hoc sp_executesql. The JS, Python and CLI crates do not touch this surface.
• deduct_timeout truncation. Independently re-derived: Some(0) still means unlimited via the early guard while a computed remaining of 0 becomes Exhausted, and the multi-step call sites (:3899, :3910, :3932, :4029) all pass the original opts.timeout with cumulative started.elapsed(), so the sub-second slack stays bounded under one second instead of compounding. The orphan release in begin_execute_prepared recomputes from the original budget rather than the already-deducted one.
• Not verified. No live SQL Server or registered driver on this host, so the two new execute_test.cpp cases rest on CI; no msodbcsql checkout, so entry 19's DropPrepHandle / BuildSPPrepExec / ProcessDAEParam citations are unre-derived here. I did not rerun the C++ e2e suite, the TLS interruption reproduction, or a retail-driver wire comparison.

Findings

Blocking

None.

Suggestion

None.

Nit

None.

It looks ready for human review — but this is not an approval, and one check is still red (below).

CI

coverage-report — failure (run 36188613280). Confirmed from the log that this is the previously diagnosed 75-minute poll timeout waiting for the merged CoberturaCoverage artifact, not a coverage or test regression: the per-leg artifacts are listed at every attempt and the merged one never appears. Every ADO leg on this head now passes, including Build Stage Build Linux ARM, so a re-run of coverage-report should clear it. Because a check is red, no label was applied, removed, or otherwise changed by this run.