Repository navigation
SharedDuckDbFixture's cross-class parallelism is handed back by the process-wide store lock #2376
Description
SharedDuckDbFixture chose IClassFixture over a collection fixture specifically to keep test classes running in parallel, and DuckDbInitializer's lock is process-wide, so most of that parallelism is handed straight back. Split out of #2374, which this contributed to but is not the same bug.
The contradiction
The fixture states the tradeoff it is making:
Deliberately IClassFixture (one database per class), NOT a collection fixture: a single shared collection would serialize these classes against each other and give back most of the win. Each class owns its own database file, keeping xUnit's cross-class parallelism intact.
The file separation is real — each instance builds its database under its own GUID temp directory. But every read and write goes through:
private static readonly ReaderWriterLockSlim s_dbLock = new(LockRecursionPolicy.NoRecursion);That is one lock for the whole process, so all 42 classes taking this fixture contend on it regardless of owning separate files. They serialize anyway — through a lock instead of through a collection, and without the ordering a collection would give.
Why the lock is right and should not be narrowed
Worth stating so nobody "fixes" this the fast way. Production creates many DuckDbInitializer instances against the same App.DatabasePath — MainWindow, DatabaseStateOverridesWindow, and fourteen in DuckDbAlertHistoryStore. The static lock is exactly what makes those mutually exclusive over one file. Making it per-instance would trade slow tests for a real data race in the app.
So the lock stays. The question is what the test topology should be given that it stays.
What it costs
Lite.Tests runs 500 s for 2483 tests. Some of that is 42 classes each executing ~80 DDL statements to build a schema, but the lock contention is on top of it, and it is the part the fixture believes it has avoided.
It also has a correctness cost, which is how it surfaced. LocalDataService.GetDatabaseStateDeviationsAsync does its maintenance under a 5-second write-lock acquisition and skips the whole block on timeout — correct for a monitoring app, and reachable in tests precisely because unrelated classes are holding that same static lock. The method's own comment records it happening before:
That is how this surfaced — an unrelated server-tags test timed out acquiring the write lock while this method held the read lock, on a static lock shared by the whole process.
#2374 was another instance. #2375 makes the affected tests wait for their precondition rather than assume it, which is right on its own terms, but it treats the symptom.
Worth considering
- Measure first. If the classes are already effectively serialized by the lock, moving them into one collection may cost far less than the comment assumes — and would replace nondeterministic contention with deterministic queuing, which is the difference between "slow" and "intermittently wrong".
- Or reduce lock hold times. Holding the read lock across four statements starves writers, which the [BUG] Lite's database-state deviation read performs four writes under a READ lock, starving writers and racing archival #2208 comment already identifies as the mechanism.
- Either way the fixture's doc comment should stop claiming a property the lock does not permit.
Not a release blocker and not a product defect — the shipped app has one database and one process, where all of this is correct. It is a test-topology problem that costs wall-clock and manufactures flakes.
A smaller thing in the same method
catch (LockRecursionException)
{
/* The current thread already owns a read lock — likely leaked by an unhandled
exception that prevented Dispose(). ... */
return NoOpDisposable.Instance;
}With LockRecursionPolicy.NoRecursion this fires for any nesting on a thread, not only the leak the comment describes, and the caller then proceeds unsynchronized believing it holds a lock. The comment's justification ("we're already protected by a read lock") holds for the leak case and not for a legitimate nested acquisition. Flagging as a hazard, not a diagnosis — I have not tied it to an observed failure.
Activity
erikdarlingdata commentedon Aug 20, 2026
Correcting my own issue: the 500-second figure is wrong, and with it the size of the problem.
I wrote that
Lite.Teststakes 500 s and offered lock contention as the likely reason. That number came from run 32283591556 — the one run that FAILED, on the flake this issue was split out of. Successful runs on the same tree:So the suite normally runs ~190–222 s, and I generalised from a single outlier reading — the same mistake I have now made twice this week, and exactly what a "measure the series, not one point" rule exists to prevent. Apologies for putting it in the issue as fact.
What survives, and what does not
Does not survive: "this is costing us 500 seconds a run." It is not. Whatever the contention costs, it is inside a ~200 s baseline, and I have not isolated how much of that is the lock versus 42 classes each running ~80 DDL statements to build their own schema.
Survives, and is still worth fixing: the fixture's doc comment asserts a property the lock does not permit.
DuckDbInitializer's lock isprivate static readonly— process-wide — so the classes serialize on it regardless of owning separate files. The file separation is real; the cross-class parallelism the comment claims to be protecting is not fully there. A comment that states a false invariant is worse than no comment, because the next person reasons from it — I did, and it sent me down a wrong path for several minutes while diagnosing #2374.Also survives: the outlier itself is a clue rather than noise. A run that produced one flaky failure took 2.5x the normal duration. That is consistent with a scheduling-pressure window that both slowed the suite and starved the 5-second write-lock acquisition that #2374 traced. It does not prove causation, and I am not going to claim it does on one occurrence.
Revised scope
Restructuring 42 classes into a shared collection is not justified by a ~200 s suite, and #2375 already made the affected tests wait for their precondition rather than assume it. So I am not proposing the restructure.
What I would still do, small and safe:
LockRecursionException→ no-op-disposable swallow flagged but untouched — withNoRecursionit fires for any nesting, not only the leak its comment describes, but I have not tied it to an observed failure and changing lock semantics to fix a comment-level concern is a bad trade.Retitling this in my head from "the fixture's parallelism is a lie costing us 500 s" to "the fixture's doc comment claims a property the process-wide lock does not permit". Lower severity, still real, still worth a few lines.
Describe what the fixture's class-per-database actually buys (#2385)
erikdarlingdata commentedon Aug 21, 2026
Closing. What survived my own correction was the doc comment, and it is now accurate — but it took two passes, and the second one is worth recording because the first overcorrected.
#2385 replaced the false claim ("keeps xUnit's cross-class parallelism intact") with a different false claim: that every read and write in the suite queues on one process-wide lock. #2396 fixes that.
DuckDbInitializer's lock is aReaderWriterLockSlim, so readers do not queue behind each other, and this suite is overwhelmingly readers — roughly 184AcquireReadLockcall sites against 20 write sites. Readers block only while a writer holds the lock or is queued for it, so the cost is bursty around archival, compaction, CHECKPOINT and the mute/alert-history stores rather than charged to every database call.That distinction is the whole practical value of the paragraph. "Every read and write queues on one lock" points a reader straight at a collection fixture, which serializes the readers too — so it cannot help writer-driven contention and can only make things worse. The comment now says that outright.
Why the topology question closes unanswered rather than open
The original issue proposed changing the test topology. Three things now argue against keeping that open:
The cost was never measured. The 500 s figure was one failed run; successful runs are ~190–222 s, and nobody has isolated how much of that is lock contention versus 42 classes each executing ~80 DDL statements. An issue proposing a fix for an unquantified cost is how the 500 s number got written down as fact in the first place.
The obvious fix is now known to be wrong, and that is written down where someone would actually find it — in the fixture's own comment, not in an issue.
And the lock cannot be narrowed regardless: production creates several
DuckDbInitializerinstances over the sameApp.DatabasePath, and the static lock is exactly what keeps those mutually exclusive. Per-instance would trade a slow suite for a real data race.If suite time becomes a problem, the reopening move is to measure first — which class-level timings would show — rather than to act on the reasoning in this issue, which was wrong twice.
The correctness consequence this was split out of (#2374, a test assuming maintenance had run rather than waiting for it) was fixed by #2375 and is documented in the same comment.
The fixture's lock contention is writer-driven, not a flat tax (erikd…