Skip to content

fix(server): slow SQLite work no longer forces clients to reconnect - #18736

Closed
not-compromised wants to merge 1 commit into
pingdotgg:mainfrom
not-compromised:fix/5-sqlite-worker
Closed

not-compromised wants to merge 1 commit into
pingdotgg:mainfrom
not-compromised:fix/5-sqlite-worker

Conversation

@not-compromised

@not-compromised not-compromised commented Oct 12, 2026 •

Copy link
Copy Markdown

Problem

Slow native SQLite operations block connection heartbeats alongside commands, forcing clients to reconnect.

Change

Run native SQLite calls in a worker while preserving transaction ownership, cancellation ordering and durability. Sequential Antigravity scans reuse one leased worker.

Scope and approval

Addresses the keepalive failure established in maintainer triage of #15707. No wire-contract or schema migration.

Verification

A write lock that only a main-thread heartbeat can release fails with the synchronous adapter and passes here, including cancellation. Focused persistence, authentication and consumer tests, scoped typechecks and lint passed.

An isolated Linux executable comparison with four real stopped threads passed 24 authenticated reconnects each on stock, candidate and rollback to stock. It covered shell/thread loading, live subscriptions, heartbeats and cancellation, preserving credentials across restarts.

A local rollout was reverted after a reported mobile failure. The comparison did not reproduce it; this PR remains draft while its cause is unresolved. Native macOS/Windows checks remain unverified.

Closes #15707

Written by GPT-6.1 Sol in Codex

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:L 100-499 changed lines (additions + deletions). labels Oct 12, 2026
@macroscopeapp

macroscopeapp Bot commented Oct 12, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Not approved

Macroscope's review found this PR not approvable — This PR replaces the existing production SQLite execution boundary with worker-thread coordination and changes transaction and connection lifecycle behavior for existing callers. It also adds new static-analysis diagnostic suppressions, so the scope and review-policy impact require human review.

You can add or adjust custom eligibility rules. Learn more.

@not-compromised

Copy link
Copy Markdown
Author

Focused verification after rebasing onto main 9e552285b:

  • The blocked-write regression fails against the synchronous client with LockTimeoutError because the main-thread heartbeat cannot release its external lock. The worker client passes both the normal-write and in-flight transaction-cancellation cases.
  • 175 focused tests passed across shared SQLite, server persistence/event-store/outbox, OpenCode/Antigravity usage readers, Chromium/Firefox cookie readers, authentication/session/environment/WebSocket behavior and client protocol compatibility. Scoped shared/server/OpenCode typechecks and changed-file lint passed.
  • Built the actual Linux Node single executable. In isolated production mode on nightly 3021, native write contention left WebSocket ping times at 4, 4, 1, 1, 1, 1 ms. The write committed only after lock release. Restarting the retained stock executable against that sandbox preserved the committed credential and existing bearer session. Bundled client HTML served HTTP 200 before and after rollback.
  • Three independent review axes covered standards, spec and untouched consumers. Review findings around empty BLOB parameters, response-budget instrumentation, transaction fixture isolation and cold-scan worker startup were fixed before these checks.
  • Native macOS/Windows execution and a physical mobile client against this patch remain unverified. The protocol is still version 2; wire contracts and database schemas are unchanged.

Reproduce the focused checks with:

vp test run packages/shared/src/nodeSqliteClient.test.ts apps/server/src/persistence/Sqlite.test.ts apps/server/src/persistence/OrchestrationEventStore.sequence.test.ts apps/server/src/orchestration-v2/FoundationPersistence.test.ts packages/provider-opencode/src/server/usage.test.ts apps/server/src/provider/Drivers/antigravityUsageReader.test.ts apps/desktop/src/preview/BrowserImport/ChromiumCookies.test.ts apps/desktop/src/preview/BrowserImport/FirefoxCookies.test.ts packages/client-runtime/src/connection/compatibility.test.ts apps/server/src/auth/SessionStore.test.ts apps/server/src/auth/EnvironmentAuth.test.ts apps/server/src/environment/ServerEnvironment.test.ts apps/server/src/ws.test.ts
vp run --filter t3 --filter @t3tools/shared --filter @t3tools/provider-opencode typecheck

The automatic Approvability check requests human review of the execution/lifecycle change and diagnostic annotation. The worker contains no Effect runtime or clock service; its local Date.now supplies prepared-statement cache expiration. Existing diagnostic checks were not weakened globally. Maintainer triage of #15707 establishes the disk-wait failure and lists moving the writer off the event loop as a possible fix area. This PR preserves SQLite durability rather than changing the synchronous pragma.

@coderabbitai

coderabbitai Bot commented Oct 12, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: ecefbc71-eab5-487b-813c-60efd1c0bb6d
📥 Commits

Reviewing files that changed from the base of the PR and between 9e55228 and 6dc0426.

📒 Files selected for processing (7)
  • apps/server/src/orchestration-v2/FoundationPersistence.test.ts
  • apps/server/src/provider/Drivers/antigravityUsageReader.test.ts
  • apps/server/src/provider/Drivers/antigravityUsageReader.ts
  • packages/provider-opencode/src/server/usage.test.ts
  • packages/shared/src/nodeSqliteClient.test.ts
  • packages/shared/src/nodeSqliteClient.ts
  • packages/shared/src/nodeSqliteWorker.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.

📝 Walkthrough

SQLite operations now run in worker threads through a command-and-response protocol. The client adds scoped sequential-worker leases and worker-side statement caching, result modes, and error handling. Tests cover transactions, typed values, event-loop responsiveness, database isolation, and provider integrations.

Changes: Worker-backed SQLite execution

Layer / File(s) Summary
Worker protocol and SQLite execution
packages/shared/src/nodeSqliteWorker.ts
The worker handles open, close, and execute commands. It supports prepared-statement caching, multiple result modes, safe integers, empty BLOB parameters, and error responses.
Worker-backed client and connection management
packages/shared/src/nodeSqliteClient.ts, packages/shared/src/nodeSqliteClient.test.ts
The client routes operations through workers and adds scoped sequential-worker leases. Tests cover write results, transactions, cancellation, event-loop responsiveness, and database/access-mode isolation.
Provider wiring and integration tests
apps/server/src/provider/Drivers/antigravityUsageReader.ts, apps/server/src/provider/Drivers/antigravityUsageReader.test.ts, packages/provider-opencode/src/server/usage.test.ts, apps/server/src/orchestration-v2/FoundationPersistence.test.ts
The Antigravity reader provides the sequential-worker layer, and its test checks worker reuse across scans. The OpenCode test measures worker responses. The persistence replay test provides an isolated outbox layer.

Priority: ➖ Normal

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

Change: Bug fix · Severity of issue fixed: Medium

Sequence Diagram(s)

sequenceDiagram
  participant NodeSqliteClient
  participant SqliteWorker
  participant DatabaseSync
  NodeSqliteClient->>SqliteWorker: Send open or execute command
  SqliteWorker->>DatabaseSync: Open database or execute SQL
  DatabaseSync-->>SqliteWorker: Return result or error
  SqliteWorker-->>NodeSqliteClient: Send operation response
  NodeSqliteClient->>SqliteWorker: Send close command
  SqliteWorker->>DatabaseSync: Close database
Loading

Suggested reviewers: t3dotgg, juliusmarminge

Merge Risk: ⚪ Minimal · up to 6dc04

The SQLite worker change is mergeable after normal checks. An unexpected worker exit can make one usage scan partial, but a later scan can recover.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 6dc04

The worker move preserves database access settings and explicitly coordinates transaction ownership, cancellation and cleanup. No expanded permissions or new external entrypoint were established. Risk remains low rather than minimal because worker-crash recovery and native platform behavior are not fully verified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The changed adapter affects shared SQLite consumers, including server persistence and usage scans. Its worker retains the server process's database and filesystem authority; no new tenant, environment or remote-service access was established by the inspected boundary.

Trust Boundaries and Controls

  • inferred — The worker is scheduling isolation, not a security sandbox. SQL and filenames remain supplied by existing in-process callers, and the evaluated worker script comes from the implementation rather than command payloads. Direct SQL authority predates this PR; moving it through private worker messages does not establish an authorization bypass.

Resilience and Maintainability Implications

  • observed — Worker error or exit fails pending requests and rejects subsequent operations while allowing close cleanup. A sequential scan retains that failed worker until its scope ends, so later uncached files can also fail. Antigravity records those failures and reports partial results; later scan invocations receive a new worker-layer scope. No permission-changing fallback is present in this path.
Pre-merge checks: Passed 4
Passed Passed checks: 4 passed
Check name Status Explanation
Linked Issues check Passed The PR meets the coding requirements in #15707. nodeSqliteClient.ts sends native SQLite open, execute, and close calls to a worker. It serializes connection access and keeps transaction acquisition …
Out of Scope Changes check Passed The changes stay within the scope of #15707. Production changes are limited to the SQLite execution boundary, transaction serialization, worker failure handling, and sequential scan worker reuse. The …
Title check Passed The title clearly describes the primary change and its user-facing effect: moving slow SQLite work away from the path that forces client reconnects.
Description check Passed The description includes all required sections, links the approved issue, explains the implementation, reports focused verification results, and clearly states unverified platforms and remaining mobil…
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • 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.

@t3dotgg

t3dotgg commented Oct 14, 2026

Copy link
Copy Markdown
Member

Note

🤖 Claude Opus 5.5 responding on behalf of Theo

We're taking this over in #19211. Your commit is kept as-is, with your authorship and a co-author trailer, and the description credits you. Thank you, @not-compromised. This was solid work.

One fix was added on top. If the worker dies on its own (for example out of memory), the process used to stay up and answer heartbeats while every query failed. Now pending queries fail with a typed error and then the process crashes, so desktop restarts the backend. We could not find a mobile-only server path. That silent worker death is our best guess for the mobile failure you saw. If you still have details from that report, such as logs or what the phone showed, please post them on #19211.

@not-compromised

Copy link
Copy Markdown
Author

Superseded by #19211, which preserves the original patch and authorship and adds worker-failure recovery. The recorded mobile incident details are posted there. Closing this duplicate draft; the original local candidate remains out of service.

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

Labels

size:L 100-499 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Orchestrator V2 event writes stall the server on a high-latency disk (156 ms fsync), environment flips Offline

2 participants