Seed session-recovery reconnect baseline from FEATUREEXTACK - #230
Seed session-recovery reconnect baseline from FEATUREEXTACK#230gargsaumya wants to merge 1 commit into
Conversation
The server delivers the recoverable session-state baseline in the SESSIONRECOVERY FEATUREEXTACK at login, but the driver captured that payload and dropped it instead of parsing it. The reconnect LOGIN7 therefore omitted those state entries and the server rejected it (error 17897, state 81: "session recovery feature data ... structurally or semantically invalid"), so transparent reconnect never succeeded. Parse the FEATUREEXTACK baseline (StateId, Length, Value entries) into initial_state so it is echoed on reconnect, alongside the initial database/language/collation captured at login. Also fix transparent_reconnect_after_kill: it asserted the SPID changes after recovery, but SQL Server recycles SPID numbers, so a recovered session often reuses the same one; rely on connection_recovery_count instead. Re-enable the previously skipped C++ e2e session recovery test.
e3b45ce to
dd774e5
Compare
|
Depends on: #148 |
|
Can you link the PR to these 2 tasks? |
David Engel (David-Engel)
left a comment
David Engel (David-Engel)
left a comment
There was a problem hiding this comment.
Summary
Seeds the reconnect baseline from the SESSIONRECOVERY FEATUREEXTACK and wires session_recovery_negotiated from the ack, which is what finally makes idle connection resiliency function — it was requested on the wire and acked by the server, but the client-side gate was never set, so check_and_reconnect short-circuited on every path. The parse/serialize format matches JDBC's parseInitialSessionStateData exactly (StateId | 1-byte len or 0xFF + DWORD | Value), and the initial-block field order (database, collation, language) matches both msodbcsql SerializeRecoveryData and JDBC.
Verified locally: cargo fmt --all --check, cargo clippy -p mssql-tds --all-targets, and cargo test -p mssql-tds --lib session_recovery (79 passed) are all clean.
One blocking item below, plus suggestions on robustness and coverage inline.
Blocking — second reconnect replays a stale baseline
mssql-tds/src/connection/tds_client.rs:375 (unchanged line, so commenting here rather than inline). After a successful reconnect, self.recovery_context.session_state_table.reset() clears delta while initial_state keeps the login-time baseline. That was harmless before this PR because initial_state was always empty; now it means:
connect → SET options accumulate in delta → reconnect #1 sends initial+delta ✅ → delta cleared → reconnect #2 sends the bare login baseline — every numbered session state acquired before the first reconnect is silently lost.
Neither reference driver does this. JDBC only allocates a table when null == sessionRecovery.getSessionStateTable(), so the delta survives reconnects; SessionStateTable.reset() is reserved for sp_reset_connection (ENVCHANGE 18). msodbcsql keeps m_InitialSessionStateTable as the permanent login baseline and only calls ResetCurrentState() from the SQL_RESET_CONNECTION path in sqlcmisc.cpp. Smallest fix is to drop the reset() call at line 375; alternatively fold delta into initial_state before clearing.
CI
The mssql-rs Pull request validation check is currently NEUTRAL (not completed) — worth a green run before merge, particularly the Windows enableOdbcE2E leg, since that is the only place the re-enabled session_recovery_test.cpp actually executes.
| fn seed_initial_state_from_feature_ack(&mut self, data: &[u8]) { | ||
| let mut i = 0; | ||
| while i < data.len() { | ||
| let state_id = data[i] as usize; | ||
| i += 1; | ||
| let Some(&len_byte) = data.get(i) else { break }; | ||
| i += 1; | ||
| let len = if len_byte == 0xFF { | ||
| let Some(bytes) = data.get(i..i + 4) else { | ||
| break; | ||
| }; | ||
| i += 4; | ||
| u32::from_le_bytes([bytes[0], bytes[1], bytes[2], bytes[3]]) as usize | ||
| } else { | ||
| len_byte as usize | ||
| }; | ||
| let Some(value) = data.get(i..i + len) else { | ||
| break; | ||
| }; | ||
| i += len; | ||
| self.initial_state[state_id] = Some(SessionStateRecord { | ||
| recoverable: true, | ||
| sequence: 0, | ||
| data: value.to_vec(), | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Suggestion. Every break here drops the remaining entries silently. The consequence is exactly the failure this PR is fixing: an incomplete baseline goes out in the reconnect LOGIN7 and the server rejects it with 17897/81, with nothing driver-side to explain why. JDBC calls throwInvalidTDS() on the equivalent condition.
At minimum a warn! with the byte offset; better would be self.master_recovery_disabled = true so recovery fails fast with a driver error rather than an opaque server rejection. Note that seed_from_feature_ack_truncated_is_safe currently locks in the silent behavior, so it would need updating too.
Minor, same block: data.get(i..i + len) at line 368 can have len up to u32::MAX. Safe on 64-bit and there are no 32-bit targets in .pipeline, but i.checked_add(len) is free.
| for entry in &token.states { | ||
| if !entry.recoverable { | ||
| self.unrecoverable_state_count += 1; | ||
| } | ||
| self.initial_state[entry.state_id as usize] = Some(SessionStateRecord { | ||
| recoverable: entry.recoverable, | ||
| sequence: token.sequence_number, | ||
| data: entry.data.clone(), | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| // The server delivers the recoverable baseline as a packed FEATUREEXTACK | ||
| // payload, not SESSIONSTATE tokens. Each entry must be echoed in the | ||
| // reconnect LOGIN7 or the server rejects it as semantically invalid | ||
| // (error 17897, state 81). | ||
| if let Some(data) = feature_ack_initial_state { |
There was a problem hiding this comment.
Suggestion. Two bookkeeping inconsistencies between the token pass and the ack pass:
- The ack pass overwrites
initial_state[id]withrecoverable: truebut never decrementsunrecoverable_state_count, so a state the token pass counted stays counted while its record is now recoverable — the session is permanently un-recoverable with nothing in the table to show for it. reset()zeroesunrecoverable_state_countwholesale, which wipes the initial-state contribution while the records themselves survive (they are deliberately not cleared).
Tracking initial-state recoverability separately from the delta count would resolve both. Low practical impact since login-time SESSIONSTATE tokens are rare, but the counter is the sole gate in is_session_recoverable().
| /// needed for reconnection validation and orchestration. | ||
| /// needed for reconnection validation and orchestration, and to seed the | ||
| /// baseline session state a reconnect's LOGIN7 must replay. | ||
| #[allow(clippy::too_many_arguments)] |
There was a problem hiding this comment.
Suggestion. Five of the six new parameters come straight off NegotiatedSettings, which this module already imports for validate_reconnection. Taking &NegotiatedSettings instead would collapse them and remove the need for #[allow(clippy::too_many_arguments)].
| } | ||
|
|
||
| #[test] | ||
| fn seed_initial_state_forwards_feature_ack() { |
There was a problem hiding this comment.
Suggestion. The tokens branch of seed_initial_state is untested — all four new tests pass &[]. Worth covering the sequence_number == u32::MAX → master_recovery_disabled path and the !entry.recoverable increment, since those are the paths that can permanently disable recovery.
A round-trip test would also be valuable here: seed from an ack blob → snapshot() → SessionRecoveryFeature::serialize() → assert the emitted state entries byte-match the original blob. That pins reader and writer against each other in one assertion.
| negotiated_settings.is_session_recovery_acknowledged(), | ||
| negotiated_settings.database.clone(), | ||
| negotiated_settings.language.clone(), | ||
| negotiated_settings.database_collation, | ||
| &login_session_state_tokens, | ||
| negotiated_settings.session_recovery_initial_state(), |
There was a problem hiding this comment.
Suggestion. is_session_recovery_acknowledged() on line 197 is the single highest-impact line in the diff — on the base branch session_recovery_negotiated was only ever assigned inside #[cfg(test)] code, so check_and_reconnect returned Ok(Duration::ZERO) on all seven call sites and reconnect never ran despite the feature being requested and acked. Two asks:
- Please name this in the description. It reads as being only about FEATUREEXTACK seeding, but this line is what takes idle connection resiliency from inert to functional, and that belongs in the release notes.
- Nothing asserts this wiring. A unit test building
NegotiatedSettingswith an ackedSessionRecoveryFeaturecarrying ack bytes, passing it throughTdsClient::new, and checking bothrecovery_context.session_recovery_negotiatedandsession_state_table.initial_state[..]would be cheap viacreate_test_negotiated_settings_internal()and is the strongest regression guard available. Worth noting the base branch already assertedis_session_recovery_enabled()inconnectivity.rs:428and would have caught this — but every stage in.pipeline/templates/validation-stages.ymlpasses-E 'not (test(connectivity))', so a dead feature gate sat undetected.
| new_settings, | ||
| new_exec_ctx, | ||
| info_messages, | ||
| _session_state_tokens, |
There was a problem hiding this comment.
Suggestion. Worth a comment explaining why the reconnect's session state is discarded, because msodbcsql deliberately does not discard it. In sqlctokn.cpp the FEACK_SESSIONRECOVERY handler branches on FIsRecoveryInProgress(): first login goes to ReadDefaultSessionState() → initial table, recovery login goes to ReadUpdatedSessionState(recoverable=true, seq=0, ...) → current table. The initial table is written exactly once and stays the original login baseline.
Combined with the reset() issue in the top-level comment, mssql-rs currently does the inverse on both halves. If dropping these is intentional, a one-line note would save the next reader the trip through the ODBC source.
| let language = change_props | ||
| .language | ||
| .clone() | ||
| .unwrap_or_else(|| self.factory.context.language.clone()); |
There was a problem hiding this comment.
Nit. Falling back to context.language means that when the server sends no language ENVCHANGE we seed the baseline with the requested language rather than the server's actual one, and replay that in the reconnect initial block. Low risk in practice since SQL Server sends ENVCHANGE 2 at login, but "" may be the safer fallback.
Separately, this changes public TdsClient::language() from "" to the negotiated value at login. That makes it match its own doc comment, so it is a fix rather than a break — but it is an observable behavior change and deserves a CHANGELOG line.
| // numbers, so a recovered session frequently reuses the same one. | ||
| // connection_recovery_count is the reliable signal that a reconnect | ||
| // actually occurred. | ||
| get_spid(&mut client).await?; |
There was a problem hiding this comment.
Nit. The reasoning for dropping the SPID assertion is sound (SQL Server recycles SPID numbers, and connection_recovery_count() is the reliable signal). let _ = get_spid(&mut client).await?; would read more clearly as "called for its side effect."
|
What is the dynamics of this PR going to be ? :) This is sent to another dev branch. |
Yeah it was stacked on my PR. gargsaumya, my PR is merged now. Pls change the target branch of this PR to main |
4 participants
Description
This pull request implements critical fixes and enhancements to the SQL Server session recovery feature in the TDS client, ensuring that session state is correctly seeded and replayed during reconnects. The main changes include capturing and forwarding the session state baseline provided by the server at login, updating the session recovery context and table to handle this state, and extending the connection and client initialization code to manage session state tokens throughout the connection lifecycle. This allows transparent reconnects to succeed where they previously failed due to missing or incomplete session state.
Session Recovery State Handling:
Added
SessionStateTokensupport throughout the connection stack, ensuring that the baseline session state (including database, language, collation, and server-provided tokens) is captured at login and replayed during reconnects. This includes passing session state tokens from the connection provider to the TdsClient and into the session recovery context. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13] [14] [15] [16] [17]Enhanced
RecoveryContext::initializeto take and store all relevant session state information, and to seed the session state table with the baseline provided at login. [1] [2]Session State Table Improvements:
SessionStateTable::seed_initial_stateandseed_initial_state_from_feature_ackto correctly parse and store the session state baseline from server-provided FEATUREEXTACK payloads, ensuring reconnect attempts include all required state and are accepted by the server.Testing and Validation:
Test Enablement:
These changes collectively ensure that session recovery is robust and compliant with SQL Server requirements, enabling transparent reconnects to succeed in scenarios where they previously failed.
Related Issues
Checklist
cargo bfmtpassescargo bclippypassescargo btestpasses