Repository navigation
Conversation
A read that found every reader busy picked one and waited on that reader's own lock, even when another reader freed up first. Admission is now one semaphore with a permit per reader; a read waits for any permit and takes the lowest idle slot.
github-actions
Bot
added
the
vouch:unvouched
The semaphore stops a queued read from being tied to one busy reader; it does not make waiters first in, first out.
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
Pyseph
changed the title
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Reader slot acquisition packages/shared/src/nodeSqliteClient.ts |
Routing waits for a permit before claiming an available reader slot. A registered release clears the claim and returns the permit. If reader routing is unavailable, the request routes to the writer. |
Lock-wait timing and validation packages/shared/src/nodeSqliteClient.ts, packages/shared/src/nodeSqliteClient.test.ts |
Borrow and transaction timing starts before routing, so it includes reader-admission wait time. Transaction acquisition registers its lock-release finalizer without an intervening scheduler yield. The regression test checks that a queued read returns 1 and records a 1000 ms wait. |
Estimated code review effort: 3 (Moderate) | ~20 minutes
Change: Bug fix
Possibly related issues
pingdotgg/t3code#24660: The issue describes reads blocked behind a slow read. This change lets queued reads claim available reader slots.pingdotgg/t3code#17765: The issue describes snapshot reads queued behind a single reader permit. This change admits reads through the reader pool.pingdotgg/t3code#24895: The issue describes requests queued behind long reads. This change lets queued reads use available reader slots.pingdotgg/t3code#24192: The issue describes server-side lookups queued on one SQLite connection. This change updates reader admission, but does not address the reported client-side behavior.
Suggested reviewers: juliusmarminge
Merge Risk: 
62649
-
This change lets a queued SQLite read take whichever reader frees up first, and it counts reader-wait time in the lock-wait metric. No actionable merge-blocking risk was found. Whole-server behavior under real traffic was not measured, so a normal check after rollout is reasonable.
Pre-merge checks: 
-
Check name Status Explanation Title check The title clearly and concisely describes the main change: queued SQLite reads can use another available reader instead of remaining tied to one busy reader. Description check The description is mostly complete. It includes detailed Problem, Change, and Verification sections, with reproduction context, implementation details, test results, and load measurements. The Scope a… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Description check
-
Explanation
The description is mostly complete. It includes detailed Problem, Change, and Verification sections, with reproduction context, implementation details, test results, and load measurements. The Scope and approval section is not explicitly labeled and does not provide a direct approval link or comment, but the reference to issue
#26816and the focused bug-fix context provide relevant scope information.
-
Finishing Touches
-
Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Usage-based review receipt
- Mode: Continue automatically
- Reviewed files: 2
- Charged: $0.50
- View usage details
Note
This review was completed with usage-based billing: files reviewed beyond your plan's included limits are billed at $0.25/file. View usage-based billing.
Queueing moved from each reader's lock into routing, which ran before the lock-wait timer started, so a read that waited for a busy pool reported almost no db.lock_wait_ms. Both acquisition paths now start timing before routing.
…t-free # Conflicts: # packages/shared/src/nodeSqliteClient.test.ts
|
Note Written by merged current main without rebasing, preserving both sets of test imports. kept first-free reader admission and the contributor's lock-wait timing fix. fixed one cancellation edge: reader admission now uses the transaction's restore policy, matching lock acquisition. extended the existing regression to cancel a queued transaction before any of the four held readers is released. it timed out before the fix and passes afterward. blacksmith verification: all 20 sqlite tests pass; |
Problem
When a read finds all four SQLite readers busy, it picks the least-claimed one and waits on that reader's own lock. It stays pinned there: if a different reader frees up first, the read keeps waiting for its own, and later reads can finish ahead of it on the free one. A point read that lands behind a long shell read waits for the whole shell read while other readers sit idle.
PixPMusic found this while reviewing #26816 and reproduced it on main with real workers. The review asked for it to be tracked separately from that PR.
Change
Admission to the pool is now one semaphore with a permit per reader. A read waits for any permit, then takes the lowest idle slot. Holding a permit always leaves an idle slot, so a queued read is no longer tied to one busy reader: it waits for any of them. Idle slots still never start a worker. As before, waiters are not strictly first in, first out; a read that arrives just as a permit frees can take it first. The permit and slot are released through the same finalizers as before: when a standalone read ends, or when a read transaction's scope closes. A scheduler yield still can't separate taking the permit from registering its release.
The queueing now happens while routing, so both acquisition paths start the
db.lock_wait_mstimer before routing; otherwise a read that waited for a busy pool would report almost no lock wait.Verification
New case in
nodeSqliteClient.test.ts, "gives a queued read the first reader that frees up": four read-only transactions hold every reader, a fifth read queues, the second transaction ends, and the queued read must finish while the other three are still open. It usesDeferredmilestones and real reader workers, with no sleeps. While the read waits, the test clock moves 1 s, and the read's span must reportdb.lock_wait_msof exactly 1000.nodeSqliteClient.tsit fails at the 60 s test timeout, because the queued read waits on the first reader's transaction.vp run check:changedpasses format, lint, knip and typecheck across the 33 dependent packages; its only test failure is that same Windows case.Latency under load
To see the effect on real reads, I ran the SQLite client from main and from this branch with reader workers against a static copy of a real server's shell tables: 3,047 threads and 701,015 turn items, read-only. Three loops ran a shell-sized read (every thread's item count by run, about 0.5 s), with a 1 ms pause between runs. Point reads (one thread row by ID) arrived as new fibers in bursts of three every 5 ms, so at least one reader was always idle. Each arm ran 20 s, alternating main and branch, two rounds:
The heavy reads took the same time in both arms (p50 450 / 609 ms on main, 456 / 499 ms here). On main, a point read that arrived while the idle reader was busy queued behind a heavy read instead.
Not measured: a whole server under real traffic.
Claude Opus 5.5 in T3 Code (Claude Code harness).