Preserve prepexec orphan when the piggybacked drop never sends

[Tech Guy (lukiod)]

Closes #647.

Problem

TdsClient::execute_sp_prepexec evicts orphan's prepared_handles/
prepared_param_encryption entries unconditionally, before the packet
writer for the RPC is even created. A failure or cancellation before any
byte reaches the network still permanently discards the handle, even
though the server never saw the request and the piggybacked drop was
never sent - there is nothing ambiguous about that case, and the handle
should stay releasable.

Fix

Keep the removed entries in locals instead of dropping them immediately.
After the send attempt, check SuspendedMessage::nothing_sent() (already
used elsewhere in this file for exactly this kind of distinction, e.g.
retract_partial_request): if the failure happened before anything
reached the network, reinstate the orphan and both map entries exactly as
they were. Once any byte is sent, the existing behavior - evict either
way, since the server may have consumed the drop - is unchanged.

Testing

cargo test -p mssql-tds --lib: 2127 passed. (One log-buffering test in
an unrelated module is flaky under the default parallel test runner
regardless of this change - it passes both alone and with
--test-threads=1; confirmed by running it both ways before and after
this patch.)

Added execute_sp_prepexec_preserves_orphan_when_the_first_send_fails,
using the existing create_failing_capturing_client harness to fail the
very first wire write. Confirmed it fails against the pre-fix code (the
orphan and its prepared_handles entry are lost) and passes after the
fix.

The two existing prepexec orphan tests (..._preserves_orphan_on_pre_send_error,
..._consumes_orphan_after_send_begins) both still pass, but neither
actually exercises this ordering: the first fails at the
already-executing guard before orphan.take() ever runs, and the second
doesn't fail the send at all, so both hold under either the old or new
behavior. The new test isolates the specific gap the issue reports.

Branched from cc7bb82e.

[azure-pipelines]

Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

[Copilot]

Copilot review overview

🟡 Changes recommended

Interrupted sends can be misclassified as unsent, potentially restoring an ambiguously released handle.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 · 1

Open (2)

• Track send attempts before awaiting network writes · New
• Test restoration of prepared parameter encryption · New

What changed in this PR

Preserves piggybacked sp_prepexec orphan state when a request fails before reaching the network.

Changes:

• Retains removed handle and encryption metadata for conditional restoration.
• Adds a first-write-failure regression test.

File	Description
mssql-tds/​src/​connection/​tds_client.rs	Restores unsent orphan state and tests handle preservation.

---

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

[Tech Guy (lukiod)]

Real gap, but I don't think a flag set before/after network_writer.send() closes it cleanly. run_until_cancelled races the write against the token with no way to tell whether the underlying write_all already made real progress when cancellation wins - so an 'attempted' flag set before the call would ALSO fire for this PR's own test (a definitive, synchronous send failure), which is exactly the case #647 says should still restore.

This looks pre-existing rather than something this PR adds: retract_partial_request already makes its clean-discard-vs-withdraw decision off the same nothing_sent()/any_packet_flushed signal, for every RPC in the client, not just prepexec. If it's wrong under a true in-flight cancellation, it's wrong there too. A correct fix needs the transport to report whether a syscall was ever issued, independent of whether it completed - that's a PacketWriter-level change affecting every caller, not something I want to bolt onto this orphan fix. Want me to file that as a follow-up issue?

[Saurabh Singh (saurabh500)]

Unattended review sweep; this assessment was not checked by a human before posting.

Verdict: Issues remain. The existing interrupted-send finding remains blocking; this is not an approval.

Reviewed the complete one-file merge-base diff, the adjacent packet-writer/cancellation and send-cleanup paths, and prepared-execution callers against #647. I found no additional findings in that scope. The existing send-state and encryption-test threads are not duplicated here.

Blocking: No new findings; the existing blocker linked above remains open.

Suggestion: No new findings.

Nit: None.

Evidence: Source inspection only. Focused prepexec nextest and affected-crate Clippy attempts both stopped before compilation because the isolated checkout lacks Cargo.lock. The author's test results were not reproduced, and no mutation check, live-server test, or in-flight-cancellation/retry execution was established. No required CI checks were reported at the reviewed head.

Performance: Inspected the prepare/reprepare and retry paths, map ownership and encryption-metadata lifetime, plus affected Rust/ODBC/Python callers. No substantiated performance finding: the metadata is a moved Arc rather than a deep copy, and the change adds no SQL round trip or per-row work. No timing, allocation-count or retained-memory measurements were collected.

Independent critique: No new findings were added or withdrawn. I verified the critique's distinction between the definite pre-byte failure injected by the new mock test and cancellation of a pending send. The issue's named writer-creation cancellation hook is absent at this head, so it is not treated as available test evidence.

[Copilot]

Copilot review overview

🟢 Approval recommended

The implementation and regression test are sound; only a minor documentation-boundary correction remains.

Review effort: Balanced
Findings: 1

Open (1)

• Track send attempts before awaiting network writes

Resolved since last review (1)

• Test restoration of prepared parameter encryption

Previously missed (1)

In code that hasn't changed since last review

Clarify eviction boundary as first packet reaching the network

mssql-tds/​src/​connection/​tds_client.rs:4197

This says eviction becomes permanent once a send is attempted, but the new test attempts the first send and then restores the entries because nothing_sent() remains true. Describe the boundary as “once any packet reaches the network.” The orphan contract at lines 4087–4090 also still names serialization as the boundary, so it should be updated to the same wire-level semantics.

[Vahid Beiranvand (Vahid-b)]

Automated review — generated by GitHub Copilot on behalf of Vahid Beiranvand (@Vahid-b). 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

This change retains the superseded prepared handle and its encryption-metadata Arc during sp_prepexec, restoring them and the caller's orphan after a send failure classified as unsent. It changes client/RPC cleanup, not the wire format: previously the entries were discarded before the writer was created. The focused tests pass at this head, but the new regression fails in the conflict-free merge with current main; one integration-test blocker is below.

Verification

• Reviewed 7ab58489ebf08ec9de682bf1bc5de703142f56bc against merge base cc7bb82e73f1d0df3fc9aec2c901e56b75455274. Read #647, all review bodies, issue comments, and every review thread, including resolved/outdated discussion. Existing findings are not re-filed.
• Read .github/copilot-instructions.md, .github/instructions/pr-workflow.instructions.md, and .github/skills/code-review/SKILL.md + posting.md from fetched origin/main, plus the PR template and coverage/linked-issue workflows.
• Synced reference heads: mssql-rs main = 685f8bd0c20580c4ecd6c3938fbf1767d21bb6ef; msodbcsql master = bacdc8e78f2704b1fd95e28be3cfb39d06ab2e93; SqlClient main = 280ff92b856b09db7ca1e61cd884eadfdc43d9b6. Read the native BuildSPPrepExec/ExecSPPrepExec paths (Sql/Ntdbms/sqlncli/odbc/sqlccmd.cpp:8244-8263, 7044-7083) and MS-TDS RPC Request. Cancellation parity with a shipping native-driver build was not established; no SqlClient parity claim is made.
• Ran locally in a detached worktree with an exclusive CARGO_TARGET_DIR, Rust 1.95.0 on Windows x64. cargo fmt -p mssql-tds -- --check passed.
• cargo nextest run -p mssql-tds --lib -E 'test(execute_sp_prepexec_)' --no-fail-fast: 7 passed at the PR head. Replacing let send_never_reached_network = ... with false produced 6 passed, 1 failed, with the new regression detecting the disabled restoration.
• git merge-tree --write-tree --name-only origin/main 7ab58489ebf08ec9de682bf1bc5de703142f56bc produced conflict-free tree 665e61c8cf4f11dd71d22333ace0107efec1a6c9. Materializing that tree without a commit and rerunning the same command produced 6 passed, 1 failed: the new test observed None, expected Some(StatementId(0)).
• A temporary replacement using cancel_on_writer_creation passed 7/7 on that merged tree. It additionally checked zero captured bytes, a live connection, the identical encryption Arc, and a successful unprepare retry that emitted one unprepare RPC. Restored afterwards; the worktree is clean.
• CI: gh pr checks 648 --repo microsoft/mssql-rs reports only passing license/cla; --required reports no required checks. No build/test CI result is attached to this head.
• Not verified: the full crate/workspace suites, a live SQL Server, or retail-driver cancellation parity. The results above are focused mock-transport tests, not live-server measurements.

Coverage ledger

Area	Result
Primary logic	Examined orphan/map removal, restoration, and capture cleanup.
Siblings	Examined standalone unprepare, materialized/batched/streamed prepared execution, and partial-send cleanup.
Callers & implementers	Examined execute_prepared, ODBC SQLExecute/deferred SQLParamData, Python prepared execution, and the production/mock writers.
Diagnostics	Examined send-error propagation and pending-handle capture cancellation; no diagnostic-format change.
Tests	Examined fixtures and assertions; mutation, current-main differential, and release-retry probe executed.
Build, packaging, pipelines	Examined unchanged manifests/toolchain and applicable CI configuration; reproduced the integration failure locally.
Docs & comments	Examined changed comments and adjacent ownership contracts; already-raised documentation feedback not duplicated.
Description & work items	Examined the description against the full substantive diff and #647.
CI evidence	Examined exact-head check runs/statuses; only CLA evidence is available.

R1 handoff: no new security finding in the changed ownership/metadata paths; cancellation and cache-state correctness were carried into R2. R2 concludes with the measured integration issue below.

Blocking

• [R2/Blocking] mssql-tds/src/connection/tds_client.rs:13606-13613 — update the regression for current main's no-send boundary. Measured: the new first-write-error test fails after the conflict-free merge with current main. See the inline comment for the source locations, observed values, and tested replacement fixture.

Suggestion

None newly identified.

Nit

None newly identified.

Deferred / to file

No new follow-up issue proposed. Existing interrupted-send and documentation discussions are left in their original threads, not duplicated or resolved by this run.

Severity	R1	R2	Total
Blocking	0	1	1
Suggestion	0	0	0
Nit	0	0	0

[Vahid Beiranvand (Vahid-b)]

[R2/Blocking] Update this fixture for current main's no-send boundary.

Measured with head 7ab58489ebf08ec9de682bf1bc5de703142f56bc: the seven prepexec tests pass on the branch, but a conflict-free merge with origin/main at 685f8bd0c20580c4ecd6c3938fbf1767d21bb6ef gives 6 passed, 1 failed. This test fails at the orphan assertion: left: None, right: Some(StatementId(0)).

#599 has already changed SuspendedMessage::nothing_sent() to !send_attempted (mssql-tds/src/io/packet_writer.rs:169-171 at that main SHA), and sets send_attempted before polling NetworkWriter::send (480-486). The injected error here is returned from send, so the merged implementation correctly does not enter the restoration branch. The old meaning of nothing_sent() is no longer the contract this test will run against.

After refreshing the branch from main, use TestTransport::cancel_on_writer_creation with ExecuteOptions::new().cancel(&cancel) for #647's genuinely unsent case. I temporarily made that replacement on the merged tree and got 7/7 passing, including added checks for zero captured bytes, both original map entries (same encryption Arc), a live connection, and a successful unprepare retry. Keep a returned-send-error case separately if useful, but expect retirement/eviction there rather than restoration. Please update the description's first-write-failure claim to the tested boundary too.

This is the newly reproduced integration-test failure, not a second filing of the existing interrupted-send thread. All probe edits were restored.