Sign up
View stargazersSign up

fix(shared): let SQLite reads proceed beside slow reads#24762

closed3
main
fix/issue-24660
opened7d agobyJJulius Marminge
2 files changed
View on GitHubTwo-way sync enabledPR last imported 1d ago

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 https://github.com/pingdotgg/t3code/issues/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

ProofBefore: c79ef8c454After: 37df35aff6
Lookup during shell reader transactionWaited 181–220 ms to acquire the readerAcquired and completed 166–186 ms before the overlapping shell transaction committed
Two batches of 120 canceled subscriptionsAll 240 terminatedAll 240 terminated on open sockets; immediate health probes passed
Cancellation while all four readers were heldNot applicable to the one-reader implementation120 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.

Checks

22 successful · 12 skipped · 1 neutral
All checks passed
37df35a

Added 3 commits

  • fix(shared): an interrupted read no longer leaks a reader permit37df35a
  • fix(shared): an open read transaction no longer stalls every other read67df903
  • test(shared): reproduce issue 24660 reader head-of-line blockingf2e63fd
MmacroscopeappBot7d ago
Synced from GitHub

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.

Ggithub-actionsBot7d ago
Synced from GitHub

Thread transfer impact

✅ Thread transfer remains within every enforced ceiling.

ProviderMetricMain baselineThis PRImpactPR ceiling
CodexTotal thread wire5.0 KiB5.0 KiB0 B (0.0%)6.8 KiB✅
CodexThread snapshot wire3.8 KiB3.8 KiB0 B (0.0%)4.9 KiB✅
CodexLive turn WebSocket wire1.2 KiB1.2 KiB0 B (0.0%)2.0 KiB✅
CodexLive turn WebSocket decoded20.9 KiB20.9 KiB0 B (0.0%)29.3 KiB✅
CodexLive turn messages220 (0.0%)8✅
ClaudeTotal thread wire5.0 KiB5.0 KiB0 B (0.0%)6.8 KiB✅
ClaudeThread snapshot wire3.8 KiB3.8 KiB0 B (0.0%)4.9 KiB✅
ClaudeLive turn WebSocket wire1.2 KiB1.2 KiB0 B (0.0%)2.0 KiB✅
ClaudeLive turn WebSocket decoded21.2 KiB21.2 KiB0 B (0.0%)29.3 KiB✅
ClaudeLive turn messages220 (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.

CcoderabbitaiBot7d ago
Synced from GitHub

Review in Change Stack →

No actionable comments were generated in the recent review.

External image Recent review info
External image Run configuration
External image Commits

Reviewing files that changed from the base of the PR and between c79ef8c454e1268ff79da5a9dcb74572e86f7861 and 37df35aff6b2da81ab99b2219365e45860c7a700.

External image 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: External image Normal

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

Change: Bug fix · Severity of issue fixed: Medium

Sequence Diagram(s)

mermaid
1sequenceDiagram
2 participant Caller
3 participant borrower
4 participant makeReader
5 participant SQLiteReaderWorker
6 Caller->>borrower: Run read-only operation
7 borrower->>makeReader: Acquire reader lease
8 makeReader->>SQLiteReaderWorker: Start or reuse worker for slot
9 SQLiteReaderWorker-->>borrower: Provide reader connection
10 borrower-->>Caller: Return operation result and release lease

Possibly related issues

  • pingdotgg/t3code#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: External image 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: Passed 3 · Failed 1

Failed Failed checks: 1 warning

Check nameStatusExplanationResolution
Linked Issues checkWarningIssue #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 Passed checks: 3 passed
Check nameStatusExplanation
Description checkPassedThe description covers the problem, implementation, scope and approval, focused verification, limitations, and agent attribution. It provides specific test results and runtime evidence.
Title checkPassedThe 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 checkPassedThe 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.

LLuneDevelops4d ago
Synced from GitHub

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 p9911.5 s0.34 s
Reader lock wait max42 s0.62 s
Reader duration p9911.6 s0.94 s
Reader duration max42 s1.85 s
Reads over 6 smany0
/api/auth/session wall timetimed out on the phone0.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.

Ttheo1d ago

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.