Skip to content

fix(server): harden SQLite durability under streaming writes - #5104

Closed
nihar5hah wants to merge 2 commits into
pingdotgg:mainfrom
nihar5hah:fix/sqlite-auto-rollback-cleanup
Closed

nihar5hah wants to merge 2 commits into
pingdotgg:mainfrom
nihar5hah:fix/sqlite-auto-rollback-cleanup

Conversation

@nihar5hah

@nihar5hah nihar5hah commented Jul 31, 2026 •

Copy link
Copy Markdown

Refs #961

What Changed

Set PRAGMA synchronous = FULL for the production and in-memory SQLite persistence layers, and add focused coverage that the SQLite client is running with FULL durability.

Why

T3's persisted state is a WAL SQLite database receiving a high volume of small streaming writes. When an I/O or transaction-state failure occurs during that stream, the transaction cleanup path can surface a later cannot rollback - no transaction is active error instead of the original SQLite error and leave the shared connection unusable until restart. In one local repro, the database passed PRAGMA integrity_check but every subsequent orchestration command failed at OrchestrationCommandReceiptRepository.getByCommandId with disk I/O error.

SQLite's WAL default is synchronous = NORMAL, which does not sync on every commit. Setting FULL makes committed transactions durable before SQLite reports success, reducing the window where WAL checkpoint/checkpoint-restart behavior can leave the main database inconsistent under heavy write streams. This keeps the existing WAL mode and schema unchanged.

Related upstream context:

Validation

  • corepack pnpm exec vp test run apps/server/src/persistence/NodeSqliteClient.test.ts
  • corepack pnpm exec vp test run apps/server/src/persistence/Layers/Sqlite.test.ts
  • corepack pnpm exec vp lint apps/server/src/persistence/Layers/Sqlite.ts apps/server/src/persistence/Layers/Sqlite.test.ts apps/server/src/persistence/NodeSqliteClient.test.ts
  • corepack pnpm --filter t3 exec tsgo --noEmit

Checklist

  • This PR is small and focused
  • I explained what changed and why
  • I included before/after screenshots for any UI changes
  • I included a video for animation/interaction changes

Kimi K3 via Cursor


Note

Medium Risk
Changes core persistence durability and write behavior (more disk sync per commit); scope is small and localized to SQLite setup with test coverage.

Overview
Hardens SQLite persistence by setting PRAGMA synchronous = FULL during layer setup in Sqlite.ts, immediately after WAL journal mode and before foreign keys. WAL mode stays the same; commits are flushed more aggressively than the WAL default (NORMAL), targeting fewer I/O / transaction failures under heavy small writes.

Adds Sqlite.test.ts with a layer test that reads PRAGMA synchronous and asserts the value is 2 (FULL) on SqlitePersistenceMemory.

Reviewed by Cursor Bugbot for commit 4f0a464. Bugbot is set up for automated code reviews on this repo. Configure here.

Note

Set PRAGMA synchronous = FULL on SQLite connections to harden durability

Adds PRAGMA synchronous = FULL to the SQLite setup in Sqlite.ts, applied between the existing WAL journal mode and foreign keys pragmas during layer initialization. A new test in Sqlite.test.ts asserts the pragma value is set to 2. Risk: FULL sync mode flushes to disk on every write, which reduces throughput compared to the previous default.

Macroscope summarized 4f0a464.

Copilot AI review requested due to automatic review settings July 31, 2026 11:32
@coderabbitai

coderabbitai Bot commented Jul 31, 2026 •

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: f558cf7a-be83-4f62-ad9a-8ad6df50fe60

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added the vouch:unvouched PR author is not yet trusted in the VOUCHED list. label Jul 31, 2026
@github-actions github-actions Bot added the size:XS 0-9 changed lines (additions + deletions). label Jul 31, 2026

@macroscopeapp macroscopeapp Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One finding on the added test in apps/server/src/persistence/NodeSqliteClient.test.ts. The PRAGMA synchronous = FULL change in Layers/Sqlite.ts is already covered by the new Sqlite.test.ts.

Posted via Macroscope — Effect Service Conventions

Comment thread apps/server/src/persistence/NodeSqliteClient.test.ts Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens the server’s SQLite durability configuration by explicitly setting PRAGMA synchronous = FULL in the shared persistence setup layer, and adds focused tests to verify the setting is in effect.

Changes:

  • Set PRAGMA synchronous = FULL alongside existing WAL/foreign key PRAGMAs in the SQLite persistence setup layer.
  • Add a new persistence-layer test to assert the runtime PRAGMA value is FULL for the in-memory persistence layer.
  • Add a PRAGMA synchronous assertion in the low-level NodeSqliteClient test suite.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
apps/server/src/persistence/Layers/Sqlite.ts Sets PRAGMA synchronous = FULL in the shared SQLite persistence setup.
apps/server/src/persistence/Layers/Sqlite.test.ts Adds a focused test that the persistence setup results in synchronous = FULL.
apps/server/src/persistence/NodeSqliteClient.test.ts Adds a PRAGMA synchronous assertion for the raw Node sqlite client layer.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +46 to +54
it.effect("uses full synchronous durability", () =>
Effect.gen(function* () {
const sql = yield* SqlClient.SqlClient;

const rows = yield* sql<{ readonly synchronous: number }>`PRAGMA synchronous;`;

assert.equal(rows[0]?.synchronous, 2);
}),
);

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 4f0a464 — removed the test from NodeSqliteClient.test.ts; that file is untouched now. The pragma is verified only in Layers/Sqlite.test.ts via SqlitePersistenceMemory, which runs the setup layer that sets it.

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Want fixes drafted automatically? Bugbot Autofix can create code changes for findings. A team admin can enable Autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 9119a52e09ef41f76b3a5da133ab1d0279c06459. Configure here.

Comment thread apps/server/src/persistence/NodeSqliteClient.test.ts Outdated
@macroscopeapp

macroscopeapp Bot commented Jul 31, 2026 •

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved 4f0a464

Adds a single SQLite PRAGMA configuration for enhanced write durability, following the existing pattern of PRAGMA statements in the setup function. The change is small, self-contained, and includes appropriate test coverage.

You can customize Macroscope's approvability policy. Learn more.

@nihar5hah
nihar5hah force-pushed the fix/sqlite-auto-rollback-cleanup branch from cad7a6c to 4f0a464 Compare July 31, 2026 12:13
@nihar5hah

Copy link
Copy Markdown
Author

All three review findings (same root concern: the durability test sat in the wrong layer) are addressed in 4f0a464:

  • Removed the assertion from apps/server/src/persistence/NodeSqliteClient.test.ts entirely — that file is no longer part of the diff
  • Coverage now lives only in apps/server/src/persistence/Layers/Sqlite.test.ts, where the persistence setup actually sets the pragma
  • Rebased onto latest main per repo convention

PR is now 2 files, +20/-0. Re-verified: vp test (both files, 4 passed), targeted vp lint, and tsgo --noEmit on the server package.

@t3dotgg

t3dotgg commented Aug 27, 2026

Copy link
Copy Markdown
Member

Note

🤖 GPT-5.6 Sol responding on behalf of Theo

We're closing this PR as we clean up the T3 Code backlog. Thank you for taking the time to put this together.

Closing this global durability change for now. The test proves that the pragma was set, but not that it fixes the reported failure under real disk writes or interruption. A replacement should reproduce the failure with a disk-backed database and show the streaming-write cost of the chosen setting.

If you believe we closed this in error, please reopen the PR and leave a comment explaining what we missed. If GitHub does not let you reopen it, leave a comment here and we'll take another look.

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

Labels

size:XS 0-9 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.

3 participants