fix(sql): stop clearing the shared command builder outside the connection gate

[michaelstonis]

The bug

Every operation captured its command StringBuilder from a ThreadLocal on the calling thread, then wrote to it after awaiting the connection gate — on a pool thread. The ThreadLocal keyed the builder per thread while the state it holds is per operation, so two overlapping operations issued from one thread received the same instance. Worse, the ReusableStringBuilder property called Clear() at capture time, outside the gate, concurrently with an in-flight operation's Append.

Three field symptoms, one cause:

• SqliteException: near "JSON_EXTRACT" / "$fp0" / "AND": syntax error — SQL truncated mid-build, matching the Sentry signature reported from the field
• a permanent hang, spinning at ~119% CPU inside StringBuilder on a corrupted chunk chain. It never releases the gate, so every later operation on that database queues behind it forever. No exception, nothing in Sentry
• on 4.4.x, filtered reads silently returning 0 rows where 1 exists — data integrity, not a crash

Reproduced deterministically: 3 concurrent tasks × 30 filtered ReadObjectsAsync calls goes red 10/10 in ~100ms. One task × 50 reads is green, so concurrency is load-bearing. Also reproduced against tag v4.4.1.

The fix

The sharing was never the problem — the side effect at capture time was.

private readonly StringBuilder _commandBuilder = new(1024);

One builder per database, captured by a side-effect-free reference read, cleared by each block as its first statement (which every block already did).

This is safe because of three things I verified rather than assumed:

1. A gate-concurrency probe confirmed the SemaphoreSlim admits exactly one operation at a time — max simultaneous holders was 1 even while the SQL was being corrupted, which is what ruled out "the gate is broken" as the cause.
2. All 28 builder mutation sites (plus FilterBuilder.Build / SortBuilder.Build) are inside connection blocks.
3. Nothing escapes the gate holding the builder — results materialize into a List<T> before return, and there is no yield return anywhere in Tycho.cs.

Cost

Measured over 5000 filtered reads, single-threaded:

approach	alloc/read	time/read
broken ThreadLocal	4786 B	69.1–69.3 µs
this change	4786 B (+0)	67.9–71.3 µs
new StringBuilder(1024) per operation	6906 B (+2120)	68.0–72.3 µs

A fresh builder per operation also fixes the race, but costs 2120 B of gen0 per read — new StringBuilder(1024) reserves 1024 chars, not bytes.

Test

TychoDb_ConcurrentFilteredReads_ShouldNotCorruptCommands — 8 tasks × 250 filtered reads, both serializers. Red in <100ms against the previous implementation, green here. Full suite: 182 passed.

The [Timeout] is load-bearing: the original defect hangs rather than throws, so without it a regression wedges CI instead of failing it.

The test also pins the invariant this fix now depends on — move any command building back outside the gate and it goes red.

Trade-off

The builder is retained for the instance lifetime at its high-water-mark capacity, so a one-off query with a huge IN-list would leave that buffer resident where a per-operation builder would be collected. Trivial to bound later (reset capacity in the block above a threshold); left out as premature.

Notes

• Supersedes feature/per-operation-command-builder, which carried the per-operation-allocation variant against a stale develop base.
• release/4.4.2 deliberately keeps the per-operation allocation — its sync paths call rateLimiter.AttemptAcquire() without checking the lease, so a patch release for users already in production shouldn't rest on a gate invariant that branch only partially upholds.

🤖 Generated with Claude Code

[Copilot]

Pull request overview

Fixes a concurrency bug in Tycho’s SQL command construction where a ThreadLocal<StringBuilder> was being cleared on the caller thread before awaiting the connection gate, racing with an in-flight operation appending on a pool thread (leading to truncated SQL, SqliteException parse errors, and potential hangs from StringBuilder corruption).

Changes:

• Replaces the per-thread ThreadLocal<StringBuilder> with a single per-database StringBuilder instance and removes the “clear-on-capture” accessor.
• Updates affected connection-block call sites to pass the shared builder by reference (side-effect-free capture) and rely on in-gate Clear() at first mutation.
• Adds a regression test that runs many concurrent filtered reads and enforces the invariant that command building stays inside the gate (with a load-bearing timeout to catch hangs).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
TychoDB/Tycho.cs	Switches command builder to a per-instance StringBuilder and updates connection-block state passing to avoid pre-gate clearing races.
TychoDB.UnitTests/TychoDbTests.cs	Adds a concurrent filtered-reads regression test to detect command corruption/hangs.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.