Skip to content

Commit 89a6b16

Browse files
Docs | Trim RCA note for #4001
Drops the alternative-approach rationale, the residual-risk discussion, and the failed-reproduction-variants section, and renumbers the remainder. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: cadc8f9e-e4ac-4074-92ef-88e90df96091
1 parent 80e7582 commit 89a6b16

1 file changed

Lines changed: 3 additions & 74 deletions

File tree

doc/design-notes/4001-delegated-transaction-reset.md

Lines changed: 3 additions & 74 deletions
Original file line numberDiff line numberDiff line change
@@ -151,43 +151,9 @@ evaluate `false` while `B` is `true`. The guarantee is structural, not empirical
151151
The condition is a strict **superset** of both prior behaviors. There is no input on
152152
which it returns `false` where either predecessor returned `true`.
153153

154-
### Why the union, and not something more precise
155-
156-
An earlier attempt checked whether the transaction was still *alive* before preserving
157-
it — inspecting `TransactionInformation.Status`, mirroring the pattern in
158-
`DbConnectionInternal.DetachCurrentTransactionIfEnded`. It worked, and it was arguably
159-
more correct.
160-
161-
It was discarded deliberately, because it **changed the value in the #2970 row**. That
162-
would have meant relying on tests to prove #2970 had not regressed — and as section 8
163-
shows, the test suite cannot currently prove that.
164-
165-
A guarantee derived from the shape of the condition is worth more here than a green
166-
check from tests that do not exercise the line. If the liveness refinement is
167-
desirable, it should land as its own change, on its own merits, with test coverage
168-
that actually discriminates.
169-
170-
---
171-
172-
## 6. The residual risk, stated plainly
173-
174-
The risk in this change is **not** reintroducing #2970. It is the opposite direction:
175-
this fix preserves in the delegated-root row where #3019 chose not to. If a connection
176-
were the root of a transaction that had already ended, this would preserve state that
177-
did not need preserving.
178-
179-
Two reasons that is acceptable:
180-
181-
- **It is not new behavior.** This is precisely what 6.0.5 and every prior release did,
182-
in production, for years. This restores known-good behavior rather than introducing
183-
untested behavior.
184-
- **The failure modes are not symmetric.** Under-preserving *destroys a live
185-
transaction* and permanently breaks a pooled connection — the bug being fixed.
186-
Over-preserving leaves state that is cleaned up when the transaction completes.
187-
188154
---
189155

190-
## 7. How the root cause was established
156+
## 6. How the root cause was established
191157

192158
The cause was **proven at runtime, not inferred**. The driver was temporarily
193159
instrumented at the reset site and at `DoomThisConnection()`. The captured state at the
@@ -218,7 +184,7 @@ All instrumentation was removed before commit.
218184

219185
---
220186

221-
## 8. What the existing tests actually verify
187+
## 7. What the existing tests actually verify
222188

223189
This section is deliberately blunt, because the intuitive answer is wrong.
224190

@@ -269,44 +235,7 @@ matrix above and the structural argument in section 5. Those should carry the we
269235

270236
---
271237

272-
## 9. Why no new automated test ships with this fix
273-
274-
The only known reliable reproduction requires NHibernate.
275-
276-
The bug needs the connection caught in a narrow half-state: still the delegated root,
277-
but with `EnlistedTransaction` already detached by `DetachCurrentTransactionIfEnded()`
278-
(called from `DbConnectionInternal.CloseConnection`). NHibernate's `StatelessSession`
279-
reaches this window because it returns the connection to the pool after **every
280-
statement**. Hand-written SqlClient code typically holds a connection across statements
281-
and steps straight over it.
282-
283-
Eight NHibernate-free variants were attempted. Every one landed in a non-reproducing
284-
state:
285-
286-
| Variant | Shape | Observed |
287-
|---|---|---|
288-
| A | c1 open/query/close, c2 open + query | `enlisted=set` — buggy condition accidentally true |
289-
| B | as A, c2 does nothing | `enlisted=set` |
290-
| C | c2 does `BeginTransaction()` then query | `enlisted=set` |
291-
| D | default pool size, c2 forces promotion failure, then `c1.Close()` | `deleg.IsActive=False` — already torn down |
292-
| E | c1 parked in pool, then `Transaction.Current.Rollback()` | `deleg.IsActive=False` |
293-
| F | c1 park → reopen → promotion fail → close | `deleg.IsActive=False` |
294-
| G | c2 from a different pool forces promotion | `deleg.IsActive=False` |
295-
| H | mirrors NHibernate: park, reopen, local tx inside ambient scope | `enlisted=set` |
296-
297-
Either the connection still held its `EnlistedTransaction` (so the buggy condition
298-
happened to evaluate `true`), or delegation had already been torn down synchronously.
299-
Neither reproduces the failure.
300-
301-
This is a genuine coverage gap and is recorded here rather than papered over. Given
302-
section 8, closing it properly would mean adding a test that *fails* against both
303-
buggy conditions — which, today, likely requires either an NHibernate-backed manual
304-
test or a targeted harness that drives a connection into the delegated-root-with-
305-
detached-enlistment state directly.
306-
307-
---
308-
309-
## 10. Related
238+
## 8. Related
310239

311240
- **#2970** — the issue #3019 was fixing. Fully preserved by this change (section 5).
312241
- **#2285** — reports the same exception with no reproduction. Plausibly the same root

0 commit comments

Comments
 (0)