[navidnadali]

Problem

Each read-only transaction on a SQLite reader worker costs a separate worker round trip for BEGIN, for every statement and for COMMIT, and each row comes back as a cloned object. On a busy server the time a read waits for a reader (reader-slot wait) is a visible part of dispatch and thread-shell latency: 22 ms mean at STRESS 80 on the benchmark below.

Change

• BEGIN travels with the transaction's first statement; an empty read transaction sends nothing.
• COMMIT/ROLLBACK is not awaited unless statements are still outstanding on the worker. An interrupted transaction still frees its reader only once the worker has answered what it abandoned, as today.
• Requests are posted at once and answered in order; rows come back as column names plus arrays and are rebuilt into the same objects as before.
• Only SQLite's exact plain BEGIN/COMMIT/END/ROLLBACK are handled client-side; anything else (BEGIN IMMEDIATE/EXCLUSIVE, savepoints, bound or raw controls, look-alikes such as BEGIN2) goes to SQLite and gets SQLite's answer. Each reply carries the connection's transaction state, so the bookkeeping follows what SQLite did.
• Readers stay read-only and now also refuse temporary-table writes: PRAGMA query_only = ON and, where Node has setAuthorizer, a statement authorizer that refuses ATTACH/DETACH, the query_only pragma and writes outside the main database. This is what makes the unawaited COMMIT safe: nothing on a reader can write, so its ending cannot fail. On a Node without setAuthorizer, endings are awaited as today, and on one without DatabaseSync.isTransaction no BEGIN is folded either.
• After a reader worker stops, transaction cleanup still succeeds as today, so a crash inside a read transaction fails it with the reader's error rather than a defect.

One file, packages/shared/src/nodeSqliteClient.ts, plus its tests. No server code, no client or contract change; the reader pool and routing are unchanged.

Scope and approval

An internal client–worker protocol change with no product behaviour, default, setting or contract change. The only statements that now behave differently on a reader are ones the server never issues there (temporary tables and views, ATTACH, PRAGMA query_only, VACUUM, a raw BEGIN IMMEDIATE). Overlap: #30067 (open) moves this reader's worker source into a shared connection worker for a writer worker; whichever lands second needs a rebase. #30818 (open) also edits nodeSqliteClient.ts, to bring the reader pool back after a failed start; the two conflict in a few small hunks, so whichever lands second needs a small rebase.

Background and measurements: #31002 (context, not an approval request).

Verification

vp test run packages/shared/src/nodeSqliteClient.test.ts: 62 pass in the file on this branch, merged with main @ 6eef4acce. The new tests assert message shapes, counts and ordering, never time. Failing first: with only the test file applied to main @ 6eef4acce, 18 of the 62 fail, all on assertions, including:

• -t "sends a read transaction's BEGIN with its first statement and nothing for an empty one" and -t "posts statements issued together at once, in order, and answers each on its own": main sends BEGIN, SELECT, SELECT, COMMIT as four messages; here begin+SELECT, SELECT, COMMIT.
• -t "finishes a read transaction without waiting for COMMIT and ignores its error"
• -t "answers reader statements with column names and row arrays"
• -t "refuses a write on a reader, temporary tables included" and -t "refuses what the read-only open does not, whatever the spelling or the database"
• -t "keeps a read transaction's BEGIN on the first statement that leaves, whichever fiber yields" and -t "never lets an interrupted statement take a read transaction's BEGIN with it": sweep every yield point.
• -t "keeps its bookkeeping when a sibling statement leaves before an awaited ending": a test scheduler releases a sibling statement at each turn of the transaction's ending, under five yield budgets (2 to 6 operations per turn); afterwards a stray COMMIT still gets SQLite's error and the next read transaction folds its BEGIN again.

The other new tests pass on main too: they pin what the protocol must keep, such as SQLite's own error for a duplicate BEGIN or a missing savepoint, an interrupted transaction keeping its reader until the worker has drained (-t "leaves no abandoned statement unanswered behind a run of interrupted transactions"), an uncloneable binding, a worker that stops mid-transaction, nested or without an authorizer (-t "whose reader stops, then starts a new reader"), a BEGIN on a reserved reader whose worker stopped, a reader that cannot report its transaction state (-t "cannot report its state": BEGIN, statement and COMMIT each go to SQLite, and a transaction opened raw survives managed ones that cannot start inside it), and a one-shot process exiting cleanly.

Local probes on this branch (macOS, Node 26.12.1; throwaway, not committed), against main where it applies: interrupting a transaction mid-statement returns only after the worker has answered, and the next read waits 1 ms (main: 1 ms); 5 interrupts in a row leave 0 abandoned statements queued (main: 0); 0 of 20 interrupt-then-write cycles read a stale snapshot; 10,000 pipelined statements, two rounds interrupted mid-flight, gave 0 answers meant for another statement; errors for 5 kinds of failure (prepare, mid-transaction, execute, unsafe integer, read-only write) have the same tag, message, operation, cause and codes as on main. Read-only enforcement: a 63-statement comparison of a reader opened as on main with one opened as here returns identical results for every kind of read the server makes (sorts spilling to temp files, CTEs, window functions, JSON functions, 22 PRAGMA reads); the 7 that differ are the statements listed above. The authorizer costs about 0.1 µs per callback, on the worker thread at prepare time only (local micro-benchmark).

• vp run check:changed (macOS, Node 26.12.1): passes: format, lint, knip, typecheck of 35 packages, and the 2 test files beside the changed files (64 tests).
• The server's SQLite-backed suites (apps/server/src/persistence, and the thread search, foundation persistence, provider session, guest share, projection store, auth session store and CLI config tests): 36 files, 342 passed, 1 skipped. Server integration suites (vp test run apps/server/integration): 26 files, 341 pass. Both on this branch merged with main @ 6eef4acce.

Own A/B on the bench machines: the fork's RC2 vs RC2 + this change, synthetic database, median of 3 interleaved runs (min–max).

Metric	STRESS 80: RC2	+ this change	PRIMARY 250: RC2	+ this change
Dispatch p50 (ms)	282 (278–287)	252 (228–262), −11 %	127 (119–128)	110 (106–121), −13 %
Thread-shell read p95 (ms)	130	108, −17 %	50.1	44.3, −12 %
Reader-slot wait, mean (ms)	22.4	18.1, −19 %	6.58	5.69, −14 %
Thread-switch sync p95 (ms)	399	442, +11 %	156	104, −33 %
Live thread-event push p50 (ms)	73 (65–74)	138 (112–139), +89 %	28 (27–29)	32 (31–33), +14 %
Turns completed	509	495, −2.8 %	818	821, +0.4 %

Per-change ladder. The fork's RC3 changes were applied to its RC2 build one at a time, in merge order, and each cumulative step was measured at 25–250 sessions (PRIMARY, synthetic database), 3 interleaved runs per cell, each compared with the RC2 runs of its own job. A test-only step in the same ladder flagged 5 of 53 cells, so an isolated flag is weak evidence; a pattern across sizes is stronger. Here: dispatch p50 −30 % at 200 and 250 sessions vs RC2 (103 → 73 ms at 250), reader wait −17 % at 250; as its own step, flagged better at 150 and 250 sessions and worse at no size.

How this was measured

A server benchmark harness runs the T3 Code server from its single-executable build against fake provider CLIs (recorded Claude stream-JSON turns), with a load driver that keeps N agent sessions turning over the WebSocket API, UI clients that subscribe and switch threads, and MCP/HTTP pollers. PRIMARY: 60 s think time between a session's turns, one subscriber per 10 sessions switching threads every 5 minutes, one poller per 4 sessions, and a client polling the largest thread's full snapshot. STRESS: 1 s think time, one subscriber per session switching every 30 s, pollers at 1 call/s; built to saturate the server. The database is a synthetic one shaped like a real 39 GB database (about 4.3 M events), or a sanitised copy of that real database with all content blanked. Each arm runs 3 times, interleaved (A B, B A, A B), on fixed-shape 16-vCPU x86-64 Linux VMs, with the server and the load driver pinned to separate cores and the database restored before every run; 60 s warm-up, then a 5-minute window. Values are medians of valid runs (a run is valid only if every session produced provider output in the window). A change counts as better or worse only when it moves a metric by more than the baseline's run-to-run band ((max − min) / median) plus 5 points. "RC1", "RC2" and "RC3" are the fork's release-candidate builds. Full report, with every run and every failed run marked: benchmark report.

Not checked

• The delta on this branch's own base (main @ 8b44f42b1) has not been measured. The numbers above come from the fork's builds, whose RC2 base already carried other reader changes that are not in this PR (reader pool recovery and sizing, shorter reader transactions, a reader scheduler).
• Live thread-event push latency got worse in the own A/B, with non-overlapping run ranges at both sizes; the cause is unknown. In the ladder (PRIMARY only) it stays within noise.
• The benchmarked revision is the protocol alone. The read-only enforcement, the review fixes and the cleanup-after-stop fix were tested, not re-benchmarked; on the measured path they send the same messages.
• A Node without setAuthorizer or DatabaseSync.isTransaction was covered by tests that make the reader report neither; on Node 22.16.0 and 23.11.0 only what the reader worker announces at startup was checked, not the whole client. On 23.11 the reader on main already fails every statement that returns rows (setReturnArrays is missing there), with or without this change.
• Known residual: a raw transaction-control statement issued inside an interrupted read transaction, in a one-turn gap, can take a request id out of order. The server never issues one.

Made with Claude Fable 5.1 and Claude Opus 5.5 via Claude Code. Reviewed by GPT-6.1 Sol via Codex.

🤖 Generated with Claude Code

[macroscopeapp]

Approvability

Verdict: Approved at c49bf34

Macroscope's review found this PR approvable — Reduces SQLite reader-worker round trips and row-transfer overhead while retaining read-only enforcement and awaited transaction endings where guarding is unavailable. The change leaves writer behavior and product defaults unchanged.

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

[coderabbitai]

No actionable comments were generated in the recent review.

Walkthrough

The reader-worker path adds read-only safeguards and transaction-state reporting. The client dispatches transaction-aware requests and handles worker replies and stops. Tests cover transaction controls, interruptions, read-only behavior, result shapes, and snapshot consistency.

Changes: Reader worker execution

Layer / File(s)	Summary
Reader safeguards and reply protocol packages/shared/src/nodeSqliteClient.ts, packages/shared/src/nodeSqliteClient.test.ts	The worker enables query_only and optionally installs an authorizer. Replies include column names and transaction state. Tests cover read-only enforcement and query result shapes.
Transaction dispatch and state tracking packages/shared/src/nodeSqliteClient.ts, packages/shared/src/nodeSqliteClient.test.ts	The client recognizes plain transaction controls, folds eligible BEGIN statements into reads, and handles transaction endings based on worker capabilities and request state. Tests cover ordering, errors, interruptions, worker stops, and snapshot behavior.

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

Change: Refactor

Sequence Diagram(s)

sequenceDiagram
  participant nodeSqliteClient
  participant readerWorker
  participant SQLite
  nodeSqliteClient->>readerWorker: Send statement with deferred BEGIN
  readerWorker->>SQLite: Begin transaction and execute statement
  SQLite-->>readerWorker: Return rows, columns, and transaction state
  readerWorker-->>nodeSqliteClient: Reply with results and state

Loading

Possibly related PRs

• pingdotgg/t3code#30067: Both PRs change the SQLite reader-worker request protocol and deferred BEGIN handling.

Suggested reviewers: juliusmarminge, t3dotgg

Merge Risk: Minimal · up to fff59

This change reduces SQLite reader round trips without altering product behavior, and no merge-blocking issue was found in the supplied material. The one known residual ordering edge is not triggered by the server.

Pre-merge checks: 3 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Description check		The description follows the template's main sections: Problem, Change, Scope and approval, and Verification. The Problem and Change sections are clear and specific. The Scope and approval section expl…	Link a triaged issue or a discussion that has explicit maintainer approval of the direction and scope, and include the approval comment. The current link (discussion #31002) is labelled as context, not an approval request. Also reconcile th…

Passed checks: 3 passed

Check name	Status	Explanation
Title check		The title 'perf(shared): SQLite reader transactions take fewer worker round trips' matches the main change in packages/shared/src/nodeSqliteClient.ts. The objectives confirm the change folds BEGIN int…
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: Title check

Explanation

The title 'perf(shared): SQLite reader transactions take fewer worker round trips' matches the main change in packages/shared/src/nodeSqliteClient.ts. The objectives confirm the change folds BEGIN into the first statement, skips empty transactions, and avoids awaiting endings, which reduces worker round trips. It is a conventional commit title in plain language and stays concise.

Full details: Description check

Explanation

The description follows the template's main sections: Problem, Change, Scope and approval, and Verification. The Problem and Change sections are clear and specific. The Scope and approval section explains why the change is an internal protocol change with no product behavior change, and links the background discussion. It states that the discussion is context, not an approval request, so the template's requirement for a triaged issue or explicit maintainer approval is not met. The Verification section is detailed. It lists the focused test file and results, failing-first evidence, local probes, and the benchmark data, and it states what was not checked. The description also identifies overlapping PRs (#30067, #30818) that need rebasing. The template requires a conventional commit title, and the PR title (perf(shared): ...) appears to follow that format. The template also asks for the model and harness used, and the description includes a Claude Code attribution line that names the models. The main gap is the missing triaged issue or maintainer approval for scope. The template says a focused fix of an obvious bug can qualify without prior approval, but this is a performance refactor of the protocol, not an obvious bug fix, so the approval requirement applies.

Resolution

Link a triaged issue or a discussion that has explicit maintainer approval of the direction and scope, and include the approval comment. The current link (discussion #31002) is labelled as context, not an approval request. Also reconcile the test counts: the description says 62 tests pass, while the objectives summary says 58 tests on main at 8b44f42. Confirm which base and count apply.

Finishing Touches

Generate unit tests (beta)

• Create a new PR

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

Recent review info

Run configuration

• Source mode: Auto
• Repository: pingdotgg/t3code
• Branch: main
• File: .coderabbit.config.ts
• Commit: 6eef4ac
• Layers: Repository TS config
• Review profile: CHILL
• Plan: Advanced
• Run ID: e62dbcb9-a769-45ed-b920-420b9a075796

Commits

Reviewing files that changed from the base of the PR and between 3ae9c7d and fff59f9.

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; 9 remain after this review.

[github-actions]

This PR changes the test suite by +39 test cases (browser 0, unit +39, server 0).

Keep tests that would fail for a real reason, and test logic below the UI where it can be. See Testing.

[t3dotgg]

Note

🤖 Claude Fable 5.2 responding on behalf of Theo

Closing: the gain does not show on current main. Re-measured through the real ProjectionStore and NodeSqliteClient, one fresh process per run, on a 1.57 GB copy and a 9 GB copy of real data.

Case	main	This PR
Worker messages per thread open	24	23
Warm open, median, 1.57 GB	33.7 ms	33.7 ms
Warm open, median, 9 GB	47.7 ms	47.8 ms
Shell read, warm	108.1 ms	107.1 ms
Mixed load (400 opens, shell reads, writes), 1.57 GB	2,591 ms	2,649 ms
2,000 small read transactions, 32 at a time	491 ms	529 ms

It saves one message per read transaction, and that is not visible in latency. For that it adds transaction bookkeeping, an unawaited COMMIT and a statement authorizer on readers.

Not run: your 80-session stress harness with fake providers. The mixed load here was 16 concurrent opens on 4 readers. If you have a workload where the round trip shows, post the numbers and we will look again.