Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions mssql-odbc/tests/e2e/tests/session_recovery_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,6 @@
class SessionRecoveryLiveTest : public ODBCTest {
protected:
void SetUp() override {
GTEST_SKIP() << "Disabled pending mssql-tds transparent-reconnect fix: "
"the reconnection LOGIN7 is rejected by the server, so "
"SQLExecute-after-KILL cannot recover yet. Re-enable when "
"transparent reconnect succeeds end-to-end.";

ODBCTest::SetUp();
ASSERT_TRUE(ODBCTestConfig::Instance().HasConnection())
<< "No connection configured – set ODBC_TEST_SERVER or "
Expand Down
191 changes: 189 additions & 2 deletions mssql-tds/src/connection/session_recovery.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,20 +67,36 @@ impl RecoveryContext {

/// Initialize recovery context with connection-time settings.
/// Called after a successful login to capture the original connection parameters
/// 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)]

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.

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)].

pub fn initialize(
&mut self,
client_context: ClientContext,
tds_version: Option<TdsVersion>,
server_version: Option<Version>,
encryption_level: NegotiatedEncryptionSetting,
mars_enabled: bool,
session_recovery_negotiated: bool,
initial_database: String,
initial_language: String,
initial_collation: SqlCollation,
initial_state_tokens: &[SessionStateToken],
initial_state_ack: Option<&[u8]>,
) {
self.client_context = Some(Box::new(client_context));
self.original_tds_version = tds_version;
self.original_server_version = server_version;
self.original_encryption_level = Some(encryption_level);
self.original_mars_enabled = mars_enabled;
self.session_recovery_negotiated = session_recovery_negotiated;
self.session_state_table.seed_initial_state(
initial_database,
initial_language,
initial_collation,
initial_state_tokens,
initial_state_ack,
);
}

/// Check whether session recovery can be attempted.
Expand Down Expand Up @@ -285,6 +301,82 @@ impl SessionStateTable {
});
}

/// Seed the baseline snapshot captured at login.
///
/// Called once, right after a successful login, with the database/language/
/// collation negotiated at connect time and any SESSIONSTATE tokens the
/// server sent during login. Without this, `initial_*` stays empty and a
/// reconnect's LOGIN7 session-recovery block goes out blank, which servers
/// reject.
pub fn seed_initial_state(
&mut self,
database: String,
language: String,
collation: SqlCollation,
tokens: &[SessionStateToken],
feature_ack_initial_state: Option<&[u8]>,
) {
self.initial_database = database;
self.initial_language = language;
self.initial_collation = collation;

for token in tokens {
if token.sequence_number == u32::MAX {
self.master_recovery_disabled = true;
continue;
}
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 {
Comment on lines +328 to +344

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.

Suggestion. Two bookkeeping inconsistencies between the token pass and the ack pass:

  1. The ack pass overwrites initial_state[id] with recoverable: true but never decrements unrecoverable_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.
  2. reset() zeroes unrecoverable_state_count wholesale, 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().

self.seed_initial_state_from_feature_ack(data);
}
}

/// Parse the packed FEATUREEXTACK session-recovery baseline into
/// `initial_state`. Entry format matches the reconnect wire format:
/// StateId (1 byte), Length (1 byte, or `0xFF` followed by a u32), Value.
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(),
});
}
}
Comment on lines +352 to +378

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.

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.


/// Returns `true` if the session can be recovered after a disconnect.
///
/// Recovery is blocked when the server has globally disabled it or when
Expand Down Expand Up @@ -537,6 +629,96 @@ mod tests {
assert!(table.delta[5].is_none());
}

#[test]
fn seed_from_feature_ack_parses_server_baseline() {
// The 8-entry baseline the server sent in the SESSIONRECOVERY
// FEATUREEXTACK at login (captured from the wire). Dropping any of
// these makes the reconnect LOGIN7 fail with error 17897, state 81.
let ack = vec![
0x00, 0x09, 0x00, 0x60, 0x81, 0x14, 0xFF, 0xE7, 0xFF, 0xFF, 0x00, // id 0, len 9
0x02, 0x02, 0x07, 0x01, // id 2, len 2
0x04, 0x01, 0x00, // id 4, len 1
0x05, 0x04, 0xFF, 0xFF, 0xFF, 0xFF, // id 5, len 4
0x06, 0x01, 0x00, // id 6, len 1
0x07, 0x01, 0x02, // id 7, len 1
0x08, 0x08, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, // id 8, len 8
0x09, 0x04, 0xFF, 0xFF, 0xFF, 0xFF, // id 9, len 4
];
let mut table = SessionStateTable::new();
table.seed_initial_state_from_feature_ack(&ack);

for (id, expected) in [
(
0usize,
vec![0x00, 0x60, 0x81, 0x14, 0xFF, 0xE7, 0xFF, 0xFF, 0x00],
),
(2, vec![0x07, 0x01]),
(4, vec![0x00]),
(5, vec![0xFF, 0xFF, 0xFF, 0xFF]),
(6, vec![0x00]),
(7, vec![0x02]),
(8, vec![0x00; 8]),
(9, vec![0xFF, 0xFF, 0xFF, 0xFF]),
] {
let record = table.initial_state[id]
.as_ref()
.unwrap_or_else(|| panic!("state {id} missing"));
assert_eq!(record.data, expected);
assert!(record.recoverable);
assert_eq!(record.sequence, 0);
}
// Gaps stay empty and the session remains recoverable.
assert!(table.initial_state[1].is_none());
assert!(table.initial_state[3].is_none());
assert!(table.is_session_recoverable());
}

#[test]
fn seed_from_feature_ack_extended_length() {
// Values >= 0xFF use the 0xFF marker followed by a u32 length.
let mut ack = vec![10u8, 0xFF];
ack.extend_from_slice(&300u32.to_le_bytes());
ack.extend_from_slice(&[0xAB; 300]);

let mut table = SessionStateTable::new();
table.seed_initial_state_from_feature_ack(&ack);

let record = table.initial_state[10].as_ref().unwrap();
assert_eq!(record.data.len(), 300);
assert!(record.data.iter().all(|&b| b == 0xAB));
}

#[test]
fn seed_from_feature_ack_truncated_is_safe() {
// A complete entry followed by one whose declared length runs past the
// buffer: the good entry is kept, the truncated one dropped, no panic.
let ack = vec![3, 1, 0xAA, 5, 10, 0x01];
let mut table = SessionStateTable::new();
table.seed_initial_state_from_feature_ack(&ack);

assert_eq!(table.initial_state[3].as_ref().unwrap().data, vec![0xAA]);
assert!(table.initial_state[5].is_none());
}

#[test]
fn seed_initial_state_forwards_feature_ack() {

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.

Suggestion. The tokens branch of seed_initial_state is untested — all four new tests pass &[]. Worth covering the sequence_number == u32::MAXmaster_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.

let ack = vec![7, 2, 0x01, 0x02];
let mut table = SessionStateTable::new();
table.seed_initial_state(
"master".to_string(),
"us_english".to_string(),
SqlCollation::default(),
&[],
Some(&ack),
);

assert_eq!(table.initial_database, "master");
assert_eq!(
table.initial_state[7].as_ref().unwrap().data,
vec![0x01, 0x02]
);
}

#[test]
fn reset_preserves_master_recovery_disabled() {
let mut table = SessionStateTable::new();
Expand Down Expand Up @@ -707,8 +889,13 @@ mod tests {
Some(Version::new(16, 0, 1000, 0)),
NegotiatedEncryptionSetting::Mandatory,
false,
true,
"master".to_string(),
"us_english".to_string(),
SqlCollation::default(),
&[],
None,
);
ctx.session_recovery_negotiated = true;
ctx
}

Expand Down
20 changes: 18 additions & 2 deletions mssql-tds/src/connection/tds_client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ use crate::message::transaction_management::{
TransactionManagementType,
};
use crate::query::result::ReturnValue;
use crate::token::tokens::SqlCollation;
use crate::token::tokens::{SessionStateToken, SqlCollation};
use crate::{
connection::{
execution_context::{ALREADY_EXECUTING_ERROR, ExecutionContext},
Expand Down Expand Up @@ -183,6 +183,7 @@ impl TdsClient {
negotiated_settings: NegotiatedSettings,
execution_context: ExecutionContext,
client_context: ClientContext,
login_session_state_tokens: Vec<SessionStateToken>,
) -> Self {
let mut recovery_context = RecoveryContext::new();
recovery_context.initialize(
Expand All @@ -193,6 +194,12 @@ impl TdsClient {
.session_settings
.negotiated_encryption_settings,
negotiated_settings.session_settings.mars_enabled,
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(),
Comment on lines +197 to +202

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.

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 NegotiatedSettings with an acked SessionRecoveryFeature carrying ack bytes, passing it through TdsClient::new, and checking both recovery_context.session_recovery_negotiated and session_state_table.initial_state[..] would be cheap via create_test_negotiated_settings_internal() and is the strongest regression guard available. Worth noting the base branch already asserted is_session_recovery_enabled() in connectivity.rs:428 and would have caught this — but every stage in .pipeline/templates/validation-stages.yml passes -E 'not (test(connectivity))', so a dead feature gate sat undetected.

);

Self {
Expand Down Expand Up @@ -323,7 +330,13 @@ impl TdsClient {
)
.await;
match connect_result {
Ok((new_transport, new_settings, new_exec_ctx, info_messages)) => {
Ok((
new_transport,
new_settings,
new_exec_ctx,
info_messages,
_session_state_tokens,

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.

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.

)) => {
// Validate reconnection properties match original
if let Err(validation_err) =
self.recovery_context.validate_reconnection(&new_settings)
Expand Down Expand Up @@ -4421,6 +4434,7 @@ mod tests {
negotiated_settings,
execution_context,
client_context,
Vec::new(),
)
}

Expand Down Expand Up @@ -4465,6 +4479,7 @@ mod tests {
negotiated_settings,
execution_context,
client_context,
Vec::new(),
)
}

Expand All @@ -4482,6 +4497,7 @@ mod tests {
negotiated_settings,
execution_context,
client_context,
Vec::new(),
);
(client, sent)
}
Expand Down
Loading