Say which DuckDB lock a write takes, and why both answers were right (#2463)

[erikdarlingdata]

Closes #2463. Lite only. No behaviour changes anywhere — git diff on the five modified sources adds zero executable lines; every added line is a doc comment. The only new code is one test file.

Verifying the issue first

Mostly accurate, with one incomplete census and one prior ruling it did not account for.

Claim
LocalDataService.OpenWriteConnectionAsync doc states the house rule verbatim	✅
DuckDbAlertHistoryStore — 6 write-lock sites	✅
DuckDbMuteRuleStore — 5 write-lock sites	✅
FindingStore — 3 write paths on the read lock	✅
LocalDataService is the only caller passing a timeout (5 s)	✅
The maintenance-shaped set is not in question	✅
"eleven write sites"	❌ twenty-five. OpenWriteConnectionAsync has 14 callers of its own the census missed — LocalDataService.ServerTags.cs ×8, .DatabaseStates.cs ×3, .AlertHistory.cs ×3
"the read lock is right and the write-lock sites are over-locking"	❌ mostly not — see below

The prior ruling: #2208 already decided this axis, in the opposite direction, off a measured failure. LocalDataService.GetDatabaseStateDeviationsAsync moved from the read lock to the write lock, and says why at the site — an unrelated server-tags test timed out acquiring the write lock while this method held the read lock across four statements. So "unify on the read lock" would have reverted a deliberate ruling backed by an observed failure, on the strength of a measurement taken against a different file. That is precisely the "repo-wide convention altered without anyone deciding to" the issue was filed to prevent.

The two conventions are answers to two different questions

The resolution is neither "unify" nor "state both and shrug". There is one rule, and it explains both sets:

Which lock a caller takes is decided by what it must EXCLUDE, not by whether it reads or writes.

The read lock excludes MAINTENANCE. A held read lock blocks EnterWriteLock, so holding one is how a statement says "not while the file is reorganized under me". That is the whole of what an append needs.

The write lock additionally excludes OTHER WRITERS OF THE SAME ROWS. You need that because DuckDB's concurrency control is optimistic: it does not queue the second writer, it fails it.

Measured on DuckDB.NET 1.5.5, with the two transactions strictly interleaved — the second statement runs while the first transaction is still open and uncommitted:

overlap	result
two APPENDS of brand-new rows	both commit
a retention DELETE overlapping a disjoint append	both commit
two UPDATEs of the same row	second fails — TransactionContext Error: Conflict on update!
two upserts of the same key	second fails — Conflict on update!
two delete-all-then-reinsert compounds	second fails — Conflict on tuple deletion!

The interleave is load-bearing. My first pass let both connections merely start together, and every case came back "both committed" — a slow connection open lets one transaction commit before the other begins, so the probe measures nothing. That artefact is exactly how this reads as a tidiness complaint instead of a rule, and it is why the numbers above are reported with the barrier described.

What the rule says about the actual call sites

It describes the code as it stands, with two named exceptions.

Read lock, correct — FindingStore.InsertFindingsAsync and MuteStoryAsync append new rows (#2464 moved their ids to the process-wide CollectionIdGenerator, so they cannot even collide on the key), and CleanupOldFindingsAsync's retention DELETE is disjoint from them. Nothing else writes those tables.

Write lock, earned — the two watermark upserts, SaveIncidentOccurrencesAsync's delete-then-reinsert, and the two config_database_state_expected UPDATEs (which collide with #2208's maintenance block, writing the same table); the mute store's UPDATEs and DELETEs, which race a timer-driven expiry purge; the 14 OpenWriteConnectionAsync callers, which are UPDATE, DELETE and #2208's compound.

The pair that made this look like a contradiction resolves rather than persists. The issue's headline example — DuckDbMuteRuleStore.InsertAsync writing config_mute_rules under a write lock while FindingStore.MuteStoryAsync writes analysis_muted under a read lock, "same species of write, same store, opposite lock" — turns out not to be the same species. Nothing but the analysis pass ever writes analysis_muted. config_mute_rules is UPDATEd by an operator and DELETEd by a timer that can be running at the same instant.

Write lock, over-locked, and left that way — two appends that cannot collide: DuckDbAlertHistoryStore.RecordAlertAsync and DuckDbMuteRuleStore.InsertAsync. Each is one method on a store whose siblings genuinely need the write lock, both are alert- or operator-frequency rather than hot, and making two of eleven spell it differently costs more in legibility than it recovers in lock time. Recorded as over-locked so the next reader knows it was seen and decided rather than missed. A rule that names its own two exceptions is more useful than one quietly enforced over them.

Where it is written

On DuckDbInitializer.s_dbLock — the issue's own suggestion, and where every caller already looks. LocalDataService.OpenWriteConnectionAsync keeps its 5-second-timeout note (real, and its own contribution) and stops claiming to be the rule; the INSERT in its old sentence was the part that was wrong, since excluding archival is what the read lock already does. FindingStore's #2464 paragraph said "which is right for the other eleven call sites is #2463 … deliberately not answered here" — that is now answered, so it says so. Both write-lock stores get a class note pointing at the same rule, so whichever a reader lands on first is where they find it.

The latent hazard: not "documented and left"

LockReleaser.Dispose calls ExitReadLock unguarded across an await, safe today only because DuckDB.NET's async completes synchronously. Reproduced independently: thread id 4, 4, 4, 4, 4 across OpenAsync, DDL, 200 ExecuteNonQueryAsync and a reader drain.

The cheap mitigation the issue floated is a bug amplifier, and that is measured rather than argued. Guarding Dispose with IsReadLockHeld:

entered on main; IsReadLockHeld observed on ANOTHER thread = False   (the guard SKIPS the exit)
CurrentReadCount while still held = 1
ExitReadLock from another thread threw: SynchronizationLockException
with the read entry still held, TryEnterWriteLock(400ms) succeeded = False

A thread that did not enter sees false, so the guard skips the exit — and ReaderWriterLockSlim entries are per-thread, so the original thread's entry is then held forever. With it held, no writer can get in. The guard would trade a loud, attributable SynchronizationLockException for a silently leaked reader that permanently wedges every CHECKPOINT, archival and compaction for the life of the app. Throwing is the better failure, and the doc now says so rather than leaving the next reader to reach for the guard and make it worse.

A real fix has to make the releaser not thread-affine at all — replacing ReaderWriterLockSlim, which would also retire #2443's 50 ms poll. That is a change to the lock primitive, not to any caller, and it is not worth doing while the premise holds. So what ships is a tripwire on the premise: DuckDbAsyncStillCompletesOnTheCallingThread asserts the thread never moves across OpenAsync, DDL, 200 awaited inserts and a reader drain, under Task.Run with no SynchronizationContext. Two hundred awaits because one moved continuation could land back on the same pool thread by chance and two hundred could not. If a driver bump turns it red, the LockReleaser doc is the place to start.

Verification

• Full-solution rebuild on macOS (EnableWindowsTargeting, -t:Rebuild): 0 errors, 30 warnings, warning-code set byte-identical to dev's (CA1720 ×8, CA2016 ×20, xUnit1031 ×2, xUnit2031 ×30). The 20 CA2016 are Twenty CA2016 warnings, all in QueryStoreSliceRepairService: a token in scope that never reaches the lock #2465's, in a file this branch does not touch.

• git diff -U0 on the five modified sources yields zero added non-comment lines.

• Lite.Tests is net10.0-windows, so the real Lite.Tests/DuckDbLockModelTests.cs was compiled into a net10.0 xUnit shim with the real Lite.Tests/ParitySource.cs (no stubs needed) and run against both branches: 3/3 pass here, 1/3 fails on dev:

  TheLockRuleIsWrittenWhereEveryCallerAlreadyLooks [FAIL]
    Not found: "WHAT IT MUST EXCLUDE, NOT BY WHETHER IT READS OR"
  

  Only that one can be red, and the class doc says so. The other two assert facts that are already true — the driver's synchrony, and what a guard would cost. A test for a fact is a tripwire, not a regression test; writing one that failed today would mean breaking the thing first.

• Rebased onto dev at 9d1337a5 and every check above re-run after.

Deferred

• The two recorded over-locks are not changed. Changing them is a behaviour change on an unmeasured benefit, in the two files a reader is most likely to copy from.
• Replacing ReaderWriterLockSlim with a non-thread-affine primitive is left unfiled: it is a real option, it would retire The analysis token is armed but 167 Darling and ~138 Lite store calls never receive it #2443's poll, and it has no trigger yet. The tripwire is the trigger.

Co-Authored-By: Claude Opus 5 noreply@anthropic.com

[claude]

Reviewed. This is a clean, well-verified documentation PR — no correctness, security, or Lite/Darling parity concerns.

Verified independently (not just taken on faith from the PR description):

• Every + line in the five modified source files falls inside an XML doc comment (///) or block comment — no executable code changed, confirming the "zero behaviour change" claim.
• Lock-site counts cited in the new DuckDbInitializer.s_dbLock doc comment match the code exactly: 14 callers of OpenWriteConnectionAsync (3 in .AlertHistory.cs, 3 in .DatabaseStates.cs, 8 in .ServerTags.cs), 6 write-lock sites in DuckDbAlertHistoryStore (2 watermark upserts, 1 delete-then-reinsert, 2 config_database_state_expected UPDATEs, 1 over-locked append), 5 in DuckDbMuteRuleStore (4 earned + InsertAsync over-locked), and 3 write paths on the read lock in FindingStore (InsertFindingsAsync, MuteStoryAsync, CleanupOldFindingsAsync).
• Every exact string the new DuckDbLockModelTests.TheLockRuleIsWrittenWhereEveryCallerAlreadyLooks test asserts against (WHAT IT MUST EXCLUDE..., #2463, Conflict on update!, Conflict on tuple deletion!, IsReadLockHeld, bug amplifier, DuckDbInitializer.s_dbLock, and the removed old sentence in LocalDataService.cs) is present/absent exactly as expected — the test isn't asserting against strings that don't exist in the diff.
• ParitySource.ReadFile/repo-root resolution already existed pre-PR and is used correctly (no new coupling issues).
• The claim that DuckDbInitializer is constructed in multiple places (MainWindow, DatabaseStateOverridesWindow, DuckDbAlertHistoryStore) over a static lock field checks out — it is genuinely one process-wide lock.
• GuardingTheReleaserWouldTradeAnExceptionForAPermanentlyWedgedLock correctly releases its ReaderWriterLockSlim from the owning thread in finally, so it doesn't leak a held lock even though it deliberately fails an exit attempt from a foreign thread.

Lite/Darling parity: No concern — this is DuckDB-specific locking machinery. Darling has no DuckDB usage (grep for DuckDB/ReaderWriterLockSlim under Darling/ returns nothing), so there's no counterpart to keep in sync.

T-SQL style / SQL injection / missing-index folklore: N/A — no SQL or T-SQL touched in this PR.

No inline comments to add; nothing here needs a change.