Skip to content

fix(server): shell reads run on a SQLite reader worker and no longer freeze the server - #328

Merged
bk-agent-01 merged 5 commits into
stagefrom
fix/bkt3-shell-reads-reader-worker
Oct 20, 2026
Merged

bk-agent-01 merged 5 commits into
stagefrom
fix/bkt3-shell-reads-reader-worker

Conversation

@bk-agent-01

@bk-agent-01 bk-agent-01 commented Oct 19, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

bkt3 runs every SQLite statement on the Node event loop. The thread-list ("shell") query takes about 0.8 s for the 1,228 active threads with warm pages, and 4–8 s with cold pages or a held write lock. While it runs, the whole server freezes, and desktop clients show "Connecting".

Measured on bkt3 on 2026-10-19, after the sweep fix (#325/#326):

  • The two largest freezes were shell reads: 8.3 s in ThreadPullRequestServiceV2.synchronize, and 5.0 s in a one-thread read for claudeAccounts.setThreadMode. The second matches busy_timeout = 5000: writer transactions use BEGIN IMMEDIATE, so a read on the writer waits on the event loop while another process holds the write lock.
  • In 11 minutes, the shell query used 262 s per hour of main-thread time (7%), across 8 callers.

Fix

  1. Cherry-pick of upstream 8659fcf145 (fix(server): thread list reads no longer freeze the server on large databases pingdotgg/t3code#19226). It adds a reader worker to packages/shared/src/nodeSqliteClient.ts: a second connection in a worker thread. Transactions marked with NodeSqliteClient.readOnly run there with a deferred BEGIN, which does not wait on writers in WAL mode. Sqlite.ts turns it on, and the PR sweep's shell read uses it.

    • ShellStream.ts conflicted, because stage does not have upstream's loadShellSnapshotParts. I kept the fork side and wrapped the fork's own read sites instead (next point).
  2. The fork's full shell reads now run on the reader:

    • the HTTP shell for the Linear bridge (http.ts)
    • the client shell subscription snapshot and getArchivedShellSnapshot (ws.ts)
    • ProjectionSnapshotQuery.shell(), which covers threadUsage.get, the MCP control tools and other full-shell callers.

    One-thread reads (getThreadShell, threadShell(), the per-event projectShellItems read) stay on the writer, as upstream does. A review found that putting them on the one reader worker makes them wait behind multi-second full snapshots, which can fill the 1,000-event live buffers after a restart. Inside an open write transaction, readOnly has no effect and the read stays on the writer (upstream's test covers this). The reader sees every committed write.

  3. Cherry-pick of upstream 62c74a92f9 (fix(server): stop the branch rename before cancelling worktree setup pingdotgg/t3code#18443, "stop the branch rename before cancelling worktree setup"). The ported client's extra Effect steps change fiber timing, and that exposed a race that the fork already had: a background branch rename could write the old worktreePath back after a cancel cleared it. CI failed on ThreadLaunchService.test.ts ("cancels tracked setup before provider work is released") on both earlier heads. The cherry-pick interrupts the rename before cleanup and adds upstream's regression test.

  4. Fork markers on the cherry-picked regions, because upstream changed them again later (perf(shared): cached SQLite statements no longer hold large parameters pingdotgg/t3code#19305, perf(server): SQLite checkpoints no longer block the event loop pingdotgg/t3code#19324, fix(shared): SQLite checkpoints no longer fall back to the main thread silently pingdotgg/t3code#19417, feat(observability): SQL spans show lock wait and connection; stalls keep their own history pingdotgg/t3code#19425, refactor: prune narration and history comments pingdotgg/t3code#19559). On the next upstream merge, take upstream's side there.

If the worker cannot start, reads fall back to the writer and the server logs one warning. The worker code is an inline source string, so it needs no new bundle entry, and it works under the desktop app's ELECTRON_RUN_AS_NODE=1.

How I tested

  • Upstream's nodeSqliteClient.test.ts comes with the cherry-pick. It covers the worker, nested transactions, and readOnly inside a write transaction.
  • CI runs the tests and the typecheck. This host does not run them.
  • A fresh-context review found no correctness bug, deadlock, leak or crash path, and checked Effect 4.0.1, Node 24.16 and Electron compatibility. Its one medium risk (one-thread reads on the reader) is fixed in 605efd6d1b. Low risk, upstream behaviour: if the worker does not start within 10 s, reads use the writer until the next restart (one SQLite reader worker unavailable warning).
  • node scripts/check-fork-markers.ts passes (FORK_UPSTREAM_REF=upstream-ro/main).
  • After the merge to stage, I will check stagebkt3 for the reader-worker start log and errors. After the promotion to bkmain, I will check the bkt3 freeze log and the 250 ms probe.

🤖 Generated with Claude Code — Claude Opus 5.5 in T3 Code.

t3dotgg and others added 3 commits October 19, 2026 15:08
…atabases (pingdotgg#19226)

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-authored-by: Alex Southwell <saphid@gmail.com>
(cherry picked from commit 8659fcf)

Conflict resolution: ShellStream.ts keeps the fork side. This base has no
loadShellSnapshotParts (upstream pingdotgg#17141, 1f31906, is not on stage), so
the reader wrap for the shell snapshot routes lands in the follow-up fork
commit at the routes' own transactions in http.ts and ws.ts.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The cherry-picked reader worker (upstream pingdotgg#19226) only moves reads marked
NodeSqliteClient.readOnly. Upstream marks its shell snapshot routes inside
loadShellSnapshotParts, which stage does not have, so this marks the
fork's equivalents and the fork-only shell readers that froze bkt3:

- http.orchestration.loadShellSnapshot (GET /api/orchestration/shell, the
  Linear bridge) and ws.orchestrationV2.loadShellSnapshot (shell
  subscription): the whole read transaction, enrichment after it.
- ws getArchivedShellSnapshot: the whole read transaction.
- ws.orchestrationV2.projectShellItems: the per-thread shell read. Events
  are published after their commit, so the reader already sees them.
- Fork ProjectionSnapshotQuery shell() and threadShell(): only the
  ProjectionStore read, not the provider name lookup. Covers
  threadUsage.get, claudeAccounts.setThreadMode (access guard and owner
  lookup) and the other compatibility callers.

Every marked read is a pure read of committed state. Inside an open
transaction readOnly is ignored and the read stays on the writer. On the
reader a deferred BEGIN does not wait on another process's write lock,
where the writer's BEGIN IMMEDIATE blocks the event loop for up to
busy_timeout (the 5 s setThreadMode stall).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The fork marker check treats a line as upstream's only when it matches
upstream main. Upstream changed the reader worker after pingdotgg#19226 (pingdotgg#19305,
pingdotgg#19324, pingdotgg#19417, pingdotgg#19425, pingdotgg#19559), so the cherry-picked lines differ from
upstream main even though they are upstream's code.

- nodeSqliteClient.ts: BEGIN/END markers around the three cherry-picked
  regions, saying the next upstream merge takes upstream's side. The code
  stays byte-identical to 8659fcf.
- ThreadPullRequestService.ts: drop the two comment lines that upstream
  itself removed in pingdotgg#19559, so the wrap matches upstream main exactly.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions github-actions Bot added size:L vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. labels Oct 19, 2026
bk-agent-01 and others added 2 commits October 19, 2026 15:42
A review of the reader-worker port found that the fork-only wraps of
projectShellItems' per-event getThreadShell and ProjectionSnapshotQuery's
threadShell() put frequent small reads on the single reader worker, where
each waits behind multi-second full snapshots. After a restart that can
fill the 1,000-event live buffers and make shell streams resubscribe.
Upstream keeps these reads on the writer; only full snapshots move.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ingdotgg#18443)

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Co-authored-by: SAPHID <saphid@users.noreply.github.com>
(cherry picked from commit 62c74a9)

Conflict resolution: ThreadLaunchService.ts keeps the fork's cleanup
condition (`cancelled || !workspaceRecorded`, terminal close inside it).
Stage has no `runReleased` flag, so only the rename fiber handle and its
interrupt are ported. The test applies unchanged.

Why now: "cancels tracked setup before provider work is released" failed
on this branch because the background rename's thread.metadata.update
(worktreePath: worktreeCwd) landed after the cancel's update to null.
The race was already in the code; the reader-worker client adds a few
effect steps per SQL statement, which moved the fiber interleaving.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@bk-agent-01
bk-agent-01 marked this pull request as ready for review October 19, 2026 19:38
@bk-agent-01
bk-agent-01 merged commit 8772bc8 into stage Oct 20, 2026
31 of 33 checks passed
@bk-agent-01

Copy link
Copy Markdown
Collaborator Author

Merged as bk-agent-01, pinned to the approved head ee73689d09f057b7f66b57435e144c84c5286324. Merge commit: 8772bc88f3.

  • Go: the approving review of @tusharbhardwaj-bk on ee73689d09 (2026-10-20 10:02 UTC). Tushar's Manager session relayed it as "328 approved".
  • Checks on the head: all checks passed or were skipped. "Native fingerprint diff" has one cancelled run, and a newer run of the same workflow passed. "Transfer report artifact" stays queued on the Blacksmith runner, and this job never runs on this repository.
  • The stage branch has no ruleset and no required checks.

Next: I will confirm that stagebkt3 serves the merge and that its logs do not show "SQLite reader worker unavailable". This merge does not change bkmain.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:L vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants