[juliusmarminge]

Problem

On a large server, a slow shell read holds the only SQLite reader permit and delays unrelated thread reads. On current main with an isolated 4,122-thread fixture, a native-thread legacy lookup spent 181–220 ms acquiring the reader while a shell transaction held it for 203–241 ms.

Change

Use a bounded pool of four reader workers, started lazily as concurrent reads need them. Each transaction retains one worker until its scope closes; standalone reads return their lease on completion or interruption. Protect the permit handoff against scheduler yields so cancellation cannot lose a lease. Existing writer routing and fallback remain available.

Scope and approval

Fixes #24660

GitCafe: https://git.cafe/pingdotgg/t3code/issues/24660

Triaged bug via the automated issue flow. This PR addresses reader serialization. The separate native-V2 negative transcript cache fix remains in #24707.

Verification

• Regression test holds one read-only transaction open and requires an independent read to finish before releasing it. On the base implementation it deadlocks and fails at the test runner's 5-second timeout; on this head it passes without sleeps or elapsed-time assertions.
• vp test run packages/shared/src/nodeSqliteClient.test.ts apps/server/src/persistence/Sqlite.test.ts apps/server/src/persistence/DatabaseMaintenance.test.ts: 22 tests passed across three files.
• Shared typecheck and targeted lint passed. Two independent full-diff reviews approved the corrected head. Both reproduced the cancellation permit leak on the preceding implementation and verified its absence after the scheduler-yield protection.
• No schema, wire contract, client rendering, or provider change. Visual fixture matrix is empty.

Runtime report · Sanitized seed/reproduction drivers, focused spans, and transcripts

Proof	Before: c79ef8c	After: 37df35a
Lookup during shell reader transaction	Waited 181–220 ms to acquire the reader	Acquired and completed 166–186 ms before the overlapping shell transaction committed
Two batches of 120 canceled subscriptions	All 240 terminated	All 240 terminated on open sockets; immediate health probes passed
Cancellation while all four readers were held	Not applicable to the one-reader implementation	120 queued lookups interrupted before acquisition; all RPCs terminated and immediate health probes passed

Runtime verification used the real dev server and HTTP/WebSocket APIs on Linux with isolated synthetic state (about 270 MB, one million completed items), rather than the reporter's 16–25 GB database. Four occupied readers can still queue additional work. The missing legacy lookup still repeats; shell query cost is unchanged. The runtime workload does not force the narrow scheduler race, and its regression coverage is an external review probe rather than a committed test. No production-sized performance, universal latency, or permanent-hang claim is made. Other host operating systems were not run locally.

Model: GPT-6.1-Sol and Claude Opus 5.5. Harness: Codex and Claude Code through T3 Code.

[macroscopeapp]

Approvability

Verdict: Needs Human Approval

Macroscope's review found this PR requires human approval — This production change replaces the single SQLite reader with a lazily initialized four-worker pool and adds interruption-safe leasing across standalone reads and transactions. Although the regression test is focused and the intent is clear, the concurrency and resource-lifecycle changes are substantial enough to require human review.

^{You can add or adjust custom eligibility rules. Learn more.}

[github-actions]

Thread transfer impact

✅ Thread transfer remains within every enforced ceiling.

Provider	Metric	Main baseline	This PR	Impact	PR ceiling
Codex	Total thread wire	5.0 KiB	5.0 KiB	0 B (0.0%)	6.8 KiB	✅
Codex	Thread snapshot wire	3.8 KiB	3.8 KiB	0 B (0.0%)	4.9 KiB	✅
Codex	Live turn WebSocket wire	1.2 KiB	1.2 KiB	0 B (0.0%)	2.0 KiB	✅
Codex	Live turn WebSocket decoded	20.9 KiB	20.9 KiB	0 B (0.0%)	29.3 KiB	✅
Codex	Live turn messages	2	2	0 (0.0%)	8	✅
Claude	Total thread wire	5.0 KiB	5.0 KiB	0 B (0.0%)	6.8 KiB	✅
Claude	Thread snapshot wire	3.8 KiB	3.8 KiB	0 B (0.0%)	4.9 KiB	✅
Claude	Live turn WebSocket wire	1.2 KiB	1.2 KiB	0 B (0.0%)	2.0 KiB	✅
Claude	Live turn WebSocket decoded	21.2 KiB	21.2 KiB	0 B (0.0%)	29.3 KiB	✅
Claude	Live turn messages	2	2	0 (0.0%)	8	✅

Baseline: c79ef8c · PR result: 37df35a · Source CI: success

Scenario and decoded snapshot size

10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.

• Codex decoded thread snapshot: 108.5 KiB
• Claude decoded thread snapshot: 108.8 KiB

Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed.

[coderabbitai]

No actionable comments were generated in the recent review.

Recent review info

Run configuration

• Source mode: Auto
• Repository: pingdotgg/t3code
• Branch: fix/issue-24660
• File: .coderabbit.config.ts
• Commit: 37df35a
• Layers: Repository TS config
• Review profile: CHILL
• Plan: Team
• Run ID: 9ec7e7f9-c3cd-4fcb-a001-f17b2626be6b

Commits

Reviewing files that changed from the base of the PR and between c79ef8c and 37df35a.

Files selected for processing: 2

• packages/shared/src/nodeSqliteClient.test.ts
• packages/shared/src/nodeSqliteClient.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review.

Walkthrough

SQLite read-only operations now lease connections from a four-worker pool instead of sharing one reader. The client releases each lease after use and routes reads to the writer when no reader is available. Transaction scopes register reader or writer cleanup. A regression test checks concurrent read-only access during an open read-only transaction.

Changes: SQLite read concurrency

Layer / File(s)	Summary
Reader pool and lease lifecycle packages/shared/src/nodeSqliteClient.ts	The client uses four reader-worker slots with a semaphore to manage leases. It starts or reuses workers by slot, clears stopped workers, terminates workers at shutdown, and releases the slot and permit after use.
Read routing and transaction acquisition packages/shared/src/nodeSqliteClient.ts, packages/shared/src/nodeSqliteClient.test.ts	Read-only operations lease a reader when available and otherwise use the writer. Writer borrowing retains checkpoint-worker batching and yield behavior. Transaction scopes register lease or writer-lock cleanup. The regression test checks that a second read-only query completes while a read-only transaction remains open.

Priority: Normal

Estimated code review effort: 4 (Complex) | ~45 minutes

Change: Bug fix · Severity of issue fixed: Medium

Sequence Diagram(s)

sequenceDiagram
  participant Caller
  participant borrower
  participant makeReader
  participant SQLiteReaderWorker
  Caller->>borrower: Run read-only operation
  borrower->>makeReader: Acquire reader lease
  makeReader->>SQLiteReaderWorker: Start or reuse worker for slot
  SQLiteReaderWorker-->>borrower: Provide reader connection
  borrower-->>Caller: Return operation result and release lease

Loading

Possibly related issues

• [Bug]: Thread snapshot HTTP handlers have no server-side timeout; cold-open herd times out at 6s and floods relay logs with context-canceled warnings #17765 — This change adds multiple reader workers, which addresses reads queuing behind one slow reader. It does not add the server timeout or client concurrency limit described in the issue.

Possibly related PRs

• pingdotgg/t3code#24586 — Both changes cover SQLite reader concurrency and checkpoint behavior while a read snapshot remains open.
• pingdotgg/t3code#20877 — Both changes use reader workers for SQLite operations; this PR changes the worker design to a bounded pool and updates transaction acquisition.

Suggested labels: size:M

Suggested reviewers: yash-singh1, t3dotgg

Merge Risk: Minimal · up to 37df3

This change lets independent SQLite reads run beside slow reads by using up to four reader workers, and it keeps the writer fallback. No concrete merge-blocking defect was identified. The reported tests and typecheck passed.

Pre-merge checks: 3 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Linked Issues check		Issue #24660 has two coding requests. Request 2 (more reader concurrency) is implemented. In packages/shared/src/nodeSqliteClient.ts, the single reader and Semaphore.make(1) are replaced by READER_WOR…	Either (a) implement Request 1 in this PR: in LegacyV1ThreadImporter.ts ensureTranscript, when no legacy-import row exists, add the thread id to confirmedTranscriptThreadIds before returning, and add a regression test that a second ensureTr…

Passed checks: 3 passed

Check name	Status	Explanation
Description check		The description covers the problem, implementation, scope and approval, focused verification, limitations, and agent attribution. It provides specific test results and runtime evidence.
Title check		The title is concise, uses the repository's conventional format, and accurately describes the primary change: allowing SQLite reads to proceed concurrently with slow reads.
Out of Scope Changes check		The changes are in packages/shared/src/nodeSqliteClient.ts and its test file, per the summary and the diff. The reader pool (slots, permit semaphore, lease release, PreventSchedulerYield around the pe…

Full details: Out of Scope Changes check

Explanation

The changes are in packages/shared/src/nodeSqliteClient.ts and its test file, per the summary and the diff. The reader pool (slots, permit semaphore, lease release, PreventSchedulerYield around the permit take) implements #24660 Request 2. The writer-path refactor (borrowWriter, transactionAcquirer, annotation helpers) supports this change. It lets reads and transactions choose reader or writer through one path and keeps writer fallback. The regression test for head-of-line blocking supports the reader-serialization objective. The diff output was truncated at 16000 bytes, so the transactionAcquirer hunk was read only from the summary. The summary describes it as reader leasing with writer fallback, which matches the pool design. No change outside the reader and writer path was found.

Full details: Linked Issues check

Explanation

Issue #24660 has two coding requests. Request 2 (more reader concurrency) is implemented. In packages/shared/src/nodeSqliteClient.ts, the single reader and Semaphore.make(1) are replaced by READER_WORKERS = 4 slots and a permit semaphore. Read-only effects lease a reader, and transactions keep one until their scope closes. Request 1 is not implemented. The issue asks to cache the negative result in ensureTranscript when no legacy-import row exists. The base-to-head diff for apps/server/src/orchestration-v2/legacy/LegacyV1ThreadImporter.ts is empty, so no negative cache was added. The PR objectives and description state that this fix belongs to #24707. A reference to #24707 does not show its code is in this head. The PR also says 'Fixes #24660', which would close the issue while Request 1 is still unmet. Four readers also still queue extra work, which the PR states; this is accepted under Request 2's 'small pool' option.

Resolution

Either (a) implement Request 1 in this PR: in LegacyV1ThreadImporter.ts ensureTranscript, when no legacy-import row exists, add the thread id to confirmedTranscriptThreadIds before returning, and add a regression test that a second ensureTranscript call for a V2-created thread does not re-run the legacy query; or (b) remove 'Fixes #24660' from the PR description, reference #24660 only as partial progress, and keep the issue open until the negative-cache change is merged.

Finishing Touches

📝 Generate docstrings

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}

[LuneDevelops]

Production-size measurement from a fork, since the description says no production-sized claim is made.

Setup: a personal fork of nightly v0.1.4-nightly.20261027.3796 with this PR (head 37df35aff6) merged, and #25728 cherry-picked on top. Server runs over SSH on a Linux desktop against an 8.6 GB statev2.sqlite (72 migrations applied, about 30 concurrent agent processes). The symptom that led here was the mobile app's 6 s /api/auth/session check timing out, which the client reports as "This connection cannot control this task".

Reader spans from the server's trace files, db.lock_wait_ms and durationMs:

	Before (single reader, 14 min window)	After (this PR, first 7 min)
Reader lock wait p99	11.5 s	0.34 s
Reader lock wait max	42 s	0.62 s
Reader duration p99	11.6 s	0.94 s
Reader duration max	42 s	1.85 s
Reads over 6 s	many	0
/api/auth/session wall time	timed out on the phone	0.09 to 0.26 s

Before the change, lock wait was nearly the whole span duration, so the latency was queueing rather than query cost.

A bench on a snapshot copy of the same database, with a 100 ms session-row probe running while thread-history scans loop, adds two points the description touches on but does not quantify:

• With one reader, the probe did not complete at all while scans were running.
• With four readers, the probe stays at p99 1.2 to 1.4 ms with one or two concurrent scans, and scan throughput doubles. Four concurrent scans saturate the pool and the probe queues again. The ceiling the description mentions is real at this size; a priority lane or a reserved worker for short reads might be worth a follow-up.

After the pool, individual reader spans still reach p90 424 ms, so the scans themselves remain expensive. #26240 looks like the complementary half.

Landing note: against the current nightly's Effect language-service rules this head trips three diagnostics, effectSucceedWithVoid twice and effectFnOpportunity once on the writer borrow. Disable comments with reasons were enough; nothing behavioral.

Nothing in the comment is derived from the reporter's database. Measurement script and bench are reproducible from the trace files alone.

[t3dotgg]

Note

🤖 Claude Opus 5.5 responding on behalf of Theo

Closing: this is covered by #27097, which landed. Reopen if a needed part is still missing.