fix(server): route projection reads through the reader connection #23867 · pingdotgg/t3code

Proof	Before: `2c8bdf6177`	After: `7ca41e9da5`
HTTP MCP thread list under write lock	Failure after 5,027 ms	Success after 54 ms
Server transaction trace	Writer routing; contended transaction failed after 5,013.65 ms	Reader transaction completed in 11.66 ms

MmacroscopeappBotapproved these changes18h ago

Approvability

Verdict: Approved at 7ca41e9

Macroscope's review found this PR approvable — This focused server fix routes five pure projection reads through the existing read-only SQLite worker, reducing contention while leaving write behavior and returned data unchanged. It also adds regression coverage verifying each affected operation uses the reader connection.

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

CcoderabbitaiBot18h ago

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Source mode: Auto
Repository: pingdotgg/t3code
Branch: fix/issue-23687
File: .coderabbit.config.ts
Commit: 7ca41e9
Layers: Repository TS config
Review profile: CHILL
Plan: Team
Run ID: 10db9138-1810-4e4d-a310-9666757b86d9

📥 Commits

Reviewing files that changed from the base of the PR and between 2c8bdf6177159d8fcd12a897d7c9311f6eb93237 and 7ca41e9da546c518d1b48d7190dca0998bdcf434.

📒 Files selected for processing: 2

apps/server/src/orchestration-v2/ProjectionStore.test.ts
apps/server/src/orchestration-v2/ProjectionStore.ts

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

📝 Walkthrough

Five ProjectionStore read operations now use the reader connection. A parameterized test checks that each operation uses that connection.

Changes: ProjectionStore reads

Layer / File(s)	Summary
Route reads through the reader connection `apps/server/src/orchestration-v2/ProjectionStore.ts`	`getRecoveryThreadIds`, `getThreadRecords`, `getSettlementCandidates`, `getShellSnapshot`, and `getThreadShell` now run through `NodeSqliteClient.readOnly`. Existing result mapping and error handling remain unchanged.
Test reader connection selection `apps/server/src/orchestration-v2/ProjectionStore.test.ts`	A parameterized test uses a file-backed SQLite database and span attributes to verify that all five reads use the reader connection.

Priority: ⬆️ High

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: High

Possibly related issues

Issue 23378: This change routes getShellSnapshot through the reader, but does not change the desktop bootstrap retry budget.
Issue 21840: This change routes five ProjectionStore reads through the reader; the getThreadSnapshotWindow path described in that issue remains outside this change.

Suggested reviewers: t3dotgg

Merge Risk: ⚪ Minimal · up to 7ca41

The reads can use the reader connection without a demonstrated new failure path. No merge-blocking issue remains after normal checks.

Pre-merge checks: ^Passed 4

Check name	Status	Explanation
Title check	Passed	The title "fix(server): route projection reads through the reader connection" matches the main change. The raw summary shows five projection-store read methods now use NodeSqliteClient.readOnly, which…
Description check	Passed	The description follows the template and covers the problem, change, scope and approval, and verification. The Problem section explains that pure projection reads use the writer connection, which make…
Linked Issues check	Passed	Issue `#23687` asks that five pure ProjectionStore reads use the reader connection instead of the writer. The diff at head (versus merge base 2c8bdf6177) applies NodeSqliteClient.readOnly to all five: g…
Out of Scope Changes check	Passed	The whole-PR changes touch apps/server/src/orchestration-v2/ProjectionStore.ts and its test file, ProjectionStore.test.ts. The ProjectionStore.ts diff adds one import of NodeSqliteClient and wraps onl…

Full details: Title check

Explanation

The title "fix(server): route projection reads through the reader connection" matches the main change. The raw summary shows five projection-store read methods now use NodeSqliteClient.readOnly, which is the reader connection. The title is a conventional commit with a clear scope and a specific action. It does not cover the multi-query snapshot handling or the write-transaction exception, and that detail is not required.

Full details: Description check

Explanation

The description follows the template and covers the problem, change, scope and approval, and verification. The Problem section explains that pure projection reads use the writer connection, which makes MCP thread listing fail under write contention, and it gives reproduction data. The Change section names the five reads, the NodeSqliteClient.readOnly helper, and the snapshot and writer-transaction exceptions. Scope links issue #23687 and the reused prepared work in #23689. Verification gives the failing and passing regression results, the scoped test command with its counts, lint and typecheck results, and before and after runtime evidence. It also states that the 25.5 GB macOS workload was not replicated and that no screenshots are needed. The Scope section does not explicitly state maintainer approval of the direction, and the template asks for that when it is a bug fix linked to an automated triage flow. This gap is minor, so the check passes.

Full details: Linked Issues check

Explanation

Issue #23687 asks that five pure ProjectionStore reads use the reader connection instead of the writer. The diff at head (versus merge base 2c8bdf6177) applies NodeSqliteClient.readOnly to all five: getRecoveryThreadIds (startup recovery query), getThreadRecords (readCanonicalProjection inside sql.withTransaction, used by MCP readThread), getSettlementCandidates (auto-settlement sweep), getShellSnapshot (thread list), and getThreadShell (single shell row). The helper is defined in packages/shared/src/nodeSqliteClient.ts and is unchanged by this PR. Its documented behavior matches the issue: reads run on the reader worker, and reads inside an open write transaction keep the writer. Each read keeps its existing transaction and error mapping. The issue's requirement that writer-bound reads remain on the writer is preserved by readOnly's design. The regression test file ProjectionStore.test.ts is changed at head, but its diff could not be read in this review. The PR description reports five regression cases that failed on main and pass with the fix. The code change alone establishes the routing requirement. Runtime evidence in the PR description is author-reported and was not independently verified here.

Full details: Out of Scope Changes check

Explanation

The whole-PR changes touch apps/server/src/orchestration-v2/ProjectionStore.ts and its test file, ProjectionStore.test.ts. The ProjectionStore.ts diff adds one import of NodeSqliteClient and wraps only the five named reads with NodeSqliteClient.readOnly. The surrounding error mapping and decoding remain unchanged. No unrelated read, write, or schema logic changed in the inspected diff. The test file change is supporting verification for the same five reads, per the raw summary. Its diff could not be read, so its exact contents are not verified here, but it is not a demonstrated unrelated change.

✨ Finishing Touches: 💡 1

📝 Generate docstrings

Commit to this branch
Create a new PR

🧪 Generate unit tests (beta)

Commit to this branch
Create a new PR

🛠️ Fix failing CI checks: 💡

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.}

Ggithub-actionsBot18h ago

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	−7 B (−0.1%)	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	−7 B (−0.6%)	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	1	1	0 (0.0%)	8	✅

Baseline: 2c8bdf6 · PR result: 7ca41e9 · Source CI: failure

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.

Vvifar16h ago

Thanks for taking this up, and for crediting me — glad it landed somewhere useful.

Two things from my side, both optional.

1. A companion test that isn't in this PR. The five cases here assert that the read runs on the reader connection. The case that actually matters in production is one step stronger: the read has to finish while a writer is holding the write lock. That is one file and one commit, and it passes on your head unchanged:

1700dd5263 — branch perf/lock-wait-test-for-23867 in the fork, so it cherry-picks as a single commit.

The writer holds its transaction open for the whole read, so the overlap is guaranteed by construction rather than by timing. With the lock held, the read completes on the reader in ~13 ms and the same read on the writer cannot acquire a connection at all:

1	MEASURE read-on-reader: connection=reader lock_wait_ms=0.026 lock_held_for_ms=11 uncommitted_rows_visible=0
2	MEASURE read-on-writer: outcome=TimeoutError lock_held_for_ms=3002 connections_acquired=0

2. A typecheck trap, in case you port anything else of mine. My version of the routing test needed twelve typecheck fixes, and the reason they went unnoticed locally is worth passing on: vp run --filter @t3tools/server typecheck matches no package and exits 0, so it reports nothing at all. The package is t3; cd apps/server && npx tsc --noEmit is the command that actually runs. I checked your head — 7ca41e9da5 is clean under it, with no errors and no warnings, so this should not affect anything you have here.

fix(server): route projection reads through the reader connection#23867

Problem

Change

Scope and approval

Verification

Checks

Approvability

Thread transfer impact