Skip to content

Commit c3aaa9f

Browse files
paulmedynskipaulmedynski
committed
Add tests reproducing database context loss on reconnection (#4108)
Adds unit tests that reproduce the scenario described in issue #4108: after switching the database via USE [db] or ChangeDatabase(), a transparent reconnection silently reverts connection.Database to InitialCatalog instead of preserving the switched database. Tests use a custom DatabaseContextQueryEngine (handles USE [db] with proper EnvChange tokens) and a DisconnectableTdsServer that can sever TCP connections while keeping the listener up for reconnection. Three failing tests demonstrate the bug: - UseDatabase_ConnectionDropped_DatabaseContextPreservedAfterReconnect - ChangeDatabase_ConnectionDropped_DatabaseContextPreservedAfterReconnect - UseDatabase_ConnectionDropped_Pooled_DatabaseContextPreservedAfterReconnect Three passing tests verify baselines: - UseDatabaseCommand_UpdatesConnectionDatabaseProperty - ChangeDatabase_UpdatesConnectionDatabaseProperty - UseDatabase_ConnectionDropped_NoRetry_ThrowsOnNextCommand Also adds: - DisconnectAllClients() to GenericTdsServer<T> / DisconnectAll() to ServerEndPointHandler<T> for test infrastructure - Analysis documents under plans/database_context/ - Coding style updates (regions, line width guidance)
1 parent a11341f commit c3aaa9f

11 files changed

Lines changed: 1570 additions & 3 deletions

File tree

‎.markdownlint.jsonc‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
// Licensed to the .NET Foundation under one or more agreements.
2+
// The .NET Foundation licenses this file to you under the MIT license.
3+
// See the LICENSE file in the project root for more information.
4+
5+
// Configuration for the markdownlint VS Code extension.
6+
// See https://github.com/DavidAnson/markdownlint for rule documentation.
7+
{
8+
// MD013 - Line length: enforce a maximum line length of 100 characters
9+
"MD013": {
10+
"line_length": 100,
11+
"tables": false
12+
},
13+
// MD024 - No duplicate headings: only flag duplicates among sibling headings
14+
// (allows the same heading text under different parents)
15+
"MD024": {
16+
"siblings_only": true
17+
}
18+
}

‎plans/database_context/00-overview.md‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
# Database Context Preservation Across Internal Reconnections
2+
3+
## Issue
4+
5+
[dotnet/SqlClient#4108](https://github.com/dotnet/SqlClient/issues/4108) — `SqlConnection` doesn't
6+
restore database in the new session if connection is lost.
7+
8+
When a user changes the active database via `USE [db]` through `SqlCommand.ExecuteNonQuery()`, and
9+
the physical connection subsequently breaks and is transparently reconnected, the recovered session
10+
may land on the **initial catalog** from the connection string instead of the database the user
11+
switched to.
12+
13+
## Scope
14+
15+
This analysis covers every code path where an internal reconnection can occur and evaluates whether
16+
the current database context (`CurrentDatabase`) is correctly maintained. The assumption is:
17+
18+
> Any internal reconnection within `SqlConnection` must maintain the current database context.
19+
20+
## Documents
21+
22+
| File | Contents |
23+
| ---- | -------- |
24+
| [01-architecture.md](01-architecture.md) | How database context is tracked and how session recovery works |
25+
| [02-flows.md](02-flows.md) | Every reconnection flow, annotated with database context behaviour |
26+
| [03-issues.md](03-issues.md) | Identified bugs and gaps, ranked by severity |
27+
| [04-recommendations.md](04-recommendations.md) | Proposed fixes |
28+
| [05-reconnection-and-retry-mechanisms.md](05-reconnection-and-retry-mechanisms.md) | All retry/reconnection mechanisms with public doc cross-references |

‎plans/database_context/01-architecture.md‎

Lines changed: 140 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,140 @@
1+
# Architecture: Database Context Tracking and Session Recovery
2+
3+
## Key Data Structures
4+
5+
### `SqlConnectionInternal` fields
6+
7+
| Field | Type | Set during | Purpose |
8+
| ----- | ---- | ---------- | ------- |
9+
| `CurrentDatabase` | `string` | Login, ENV_CHANGE | The database the server considers active right now |
10+
| `_originalDatabase` | `string` | Login, constructor | Reset target for pool recycling (`ResetConnection()` restores to this value) |
11+
| `_currentSessionData` | `SessionData` | Constructor | Live session state, snapshotted before reconnection |
12+
| `_recoverySessionData` | `SessionData` | Constructor (param) | Saved session from the broken connection, used to build the recovery login packet |
13+
| `_fConnectionOpen` | `bool` | `CompleteLogin()` | Guards whether ENV_CHANGE updates `_originalDatabase` |
14+
| `_sessionRecoveryAcknowledged` | `bool` | `OnFeatureExtAck()` | Whether the server supports session recovery |
15+
16+
### `SessionData` fields
17+
18+
| Field | Mutated by | Cleared by `Reset()` | Purpose |
19+
| ----- | ---------- | -------------------- | ------- |
20+
| `_initialDatabase` | `CompleteLogin()` (first login only) | No | Immutable baseline from the login the server confirmed |
21+
| `_database` | `CurrentSessionData` getter | Yes (set to `null`) | Current database, written just-in-time before snapshot |
22+
| `_initialLanguage` | `CompleteLogin()` | No | Immutable baseline language |
23+
| `_language` | `CurrentSessionData` getter | Yes | Current language |
24+
| `_initialCollation` | `CompleteLogin()` | No | Immutable baseline collation |
25+
| `_collation` | `OnEnvChange()` | Yes | Current collation |
26+
| `_delta[]` | `OnFeatureExtAck()`, `SQLSESSIONSTATE` token handler | Yes | Per-stateID session variable changes |
27+
| `_initialState[]` | `OnFeatureExtAck()` (first login only) | No | Per-stateID session variable baselines |
28+
| `_unrecoverableStatesCount` | `SQLSESSIONSTATE` token handler | Yes | Count of non-recoverable session states |
29+
30+
`Reset()` is called when `ENV_SPRESETCONNECTIONACK` arrives (server acknowledged
31+
`sp_reset_connection`). It clears delta/current state but preserves the immutable baselines.
32+
33+
## How `CurrentDatabase` is set
34+
35+
### During login
36+
37+
```text
38+
Login() → CurrentDatabase = server.ResolvedDatabaseName
39+
(= ConnectionOptions.InitialCatalog)
40+
Server login response → ENV_CHANGE(ENV_DATABASE) → CurrentDatabase = newValue
41+
CompleteLogin() → _currentSessionData._initialDatabase = CurrentDatabase
42+
(only when _recoverySessionData == null, i.e. first login)
43+
```
44+
45+
`SqlConnectionInternal.cs` line 2976—sets `CurrentDatabase` to `InitialCatalog` immediately. The
46+
server then confirms (or overrides) via ENV_CHANGE before `CompleteLogin()` captures it.
47+
48+
### During normal operation
49+
50+
```text
51+
USE [MyDb] via SqlCommand → server response → ENV_CHANGE(ENV_DATABASE)
52+
OnEnvChange() → CurrentDatabase = "MyDb"
53+
_originalDatabase NOT updated (guarded by _fConnectionOpen)
54+
```
55+
56+
`SqlConnectionInternal.cs` lines 1155–1164. After the connection is open, `_originalDatabase` is frozen.
57+
58+
### During pool reset
59+
60+
```text
61+
Deactivate() → ResetConnection()
62+
→ _parser.PrepareResetConnection() (sets TDS header flag for sp_reset_connection)
63+
→ CurrentDatabase = _originalDatabase (resets to initial catalog immediately)
64+
```
65+
66+
`SqlConnectionInternal.cs` lines 3895–3907.
67+
68+
### `CurrentSessionData` getter (just-in-time snapshot)
69+
70+
```csharp
71+
internal SessionData CurrentSessionData
72+
{
73+
get
74+
{
75+
if (_currentSessionData != null)
76+
{
77+
_currentSessionData._database = CurrentDatabase;
78+
_currentSessionData._language = _currentLanguage;
79+
}
80+
return _currentSessionData;
81+
}
82+
}
83+
```
84+
85+
`SqlConnectionInternal.cs` lines 530–537. This is called by `ValidateAndReconnect()` right before
86+
saving recovery data for reconnection.
87+
88+
## Session Recovery Protocol
89+
90+
When `ConnectRetryCount > 0` (default: **1**), the driver negotiates `FEATUREEXT_SRECOVERY` with the
91+
server during login. On reconnection, `WriteSessionRecoveryFeatureRequest()` encodes:
92+
93+
1. **Initial state**: `_initialDatabase`, `_initialCollation`, `_initialLanguage`, `_initialState[]`
94+
2. **Current deltas**: `_database` (if different from `_initialDatabase`), `_language`,
95+
`_collation`, `_delta[]`
96+
97+
The server uses the initial state + deltas to rebuild the session. If `_database !=
98+
_initialDatabase`, the server switches to `_database` after login.
99+
100+
### `WriteSessionRecoveryFeatureRequest` — relevant excerpt
101+
102+
```text
103+
TdsParser.cs line 8963:
104+
initialLength += ... _initialDatabase ...
105+
TdsParser.cs line 8966:
106+
currentLength += ... (_initialDatabase == _database ? 0 : _database) ...
107+
TdsParser.cs line 9017:
108+
WriteIdentifier(_database != _initialDatabase ? _database : null, ...)
109+
```
110+
111+
When `_database` equals `_initialDatabase`, a zero-length identifier is written (meaning "no
112+
change"). When they differ, the current database name is written and the server applies it.
113+
114+
## Flow: How `ValidateAndReconnect` triggers recovery
115+
116+
```text
117+
SqlCommand.RunExecuteNonQueryTds()
118+
→ SqlConnection.ValidateAndReconnect()
119+
check _connectRetryCount > 0
120+
check _sessionRecoveryAcknowledged
121+
check !stateObj.ValidateSNIConnection() ← physical connection broken?
122+
SessionData cData = tdsConn.CurrentSessionData ← snapshot (writes _database = CurrentDatabase)
123+
_recoverySessionData = cData ← save for new connection
124+
tdsConn.DoomThisConnection()
125+
Task.Run → ReconnectAsync()
126+
→ ForceNewConnection = true
127+
→ OpenAsync()
128+
→ TryReplaceConnection()
129+
→ SqlConnectionFactory.CreateConnection()
130+
→ new SqlConnectionInternal(..., recoverySessionData)
131+
constructor: _originalDatabase = recoverySessionData._initialDatabase
132+
Login()
133+
→ CurrentDatabase = InitialCatalog
134+
→ login.database = CurrentDatabase
135+
→ TdsLogin(..., _recoverySessionData, ...)
136+
→ WriteSessionRecoveryFeatureRequest(recoverySessionData, ...)
137+
Server processes login + recovery → ENV_CHANGE(ENV_DATABASE) → CurrentDatabase updated
138+
CompleteLogin()
139+
→ _recoverySessionData = null
140+
```

0 commit comments

Comments
 (0)