Skip to content

feat(sqlite): add deferred commit mode with flush sequencing - #5676

Draft
NathanFlurry wants to merge 1 commit into
sync-sqlitefrom
sqlite-deferred-commits
Draft

NathanFlurry wants to merge 1 commit into
sync-sqlitefrom
sqlite-deferred-commits

Conversation

@NathanFlurry

Copy link
Copy Markdown
Member

Stacked on #5659 (sync-sqlite). Opt-in deferred SQLite commit mode: a commit returns after updating actor-local page state, a background flusher ships pages to the engine in order, and callers wait on a monotonic flush sequence. This is the storage-layer primitive for Durable Object style output gates. Design: docs-internal/engine/sqlite/deferred-commits/SPEC.md.

  • Add CommitMode::Deferred to the native SQLite VFS: committed pages land in a pinned overlay, a single flusher batches and ships them in order with the existing head fence, and commitSeq/flushedSeq/waitForFlush expose durability with snapshot semantics.
  • Keep read-your-own-writes across the overlay, in-flight batch, caches, prefetch, and engine reads.
  • Retry indeterminate commits with a byte-identical resend; a fence mismatch, wrong head, retry deadline, worker-close timeout, or flusher failure breaks the database and stops the actor generation so no waiter can observe a lost write as durable.
  • Bound the overlay with byte backpressure and a hard page cap that drains before merging, stage size-only truncates, and drain the flusher on close.
  • Add sqliteCommitMode actor config, SqliteDb sequence accessors, a database failure channel, sync-call fail-fast on JavaScript-owned transaction leases, and SQLite auto-rollback detection in the transaction coordinator.
  • Expose db({ commitMode }), commitSeq(), flushedSeq(), waitForFlush(), flushError(), executeSyncRaw() with readonly, and a handle-form beginTransactionSync() in the TypeScript client, Drizzle wrapper, NAPI bindings, and runtime interfaces; WebAssembly and remote SQLite reject the mode at actor start.
  • Fix the client action proxy so action.bind(...) is not dispatched as a nested remote action, and unlink the Actor Runtime Socket before joining its listener so a destroyed generation cannot expose its socket path.
  • Add VFS, randomized, coordinator, client unit, and driver tests plus public docs.

Awaited mode is unchanged. The five fault::* compaction tests in rivet-depot-client fail identically on the base branch (in-process engine pubsub "invalid version: 0") and are unrelated.

@railway-app

railway-app Bot commented Sep 6, 2026 •

Copy link
Copy Markdown

🚅 Deployed to the actors-pr-5676 environment in rivet-frontend

Service Status Web Updated
frontend-cloud 😴 Sleeping (View Logs) Web Sep 23, 2026 at 2:49 pm UTC
kitchen-sink 😴 Sleeping (View Logs) Web Sep 8, 2026 at 3:22 am UTC
frontend-inspector 😴 Sleeping (View Logs) Web Sep 8, 2026 at 3:19 am UTC
ladle ✅ Success (View Logs) Web Sep 6, 2026 at 9:08 pm UTC
mcp-hub ✅ Success (View Logs) Web Sep 6, 2026 at 9:07 pm UTC
website ❌ Build Failed (View Logs) Web Sep 6, 2026 at 9:06 pm UTC

@claude

claude Bot commented Oct 14, 2026

Copy link
Copy Markdown
Contributor

Review: deferred SQLite commit mode

I did a partial review. The diff is about 11k lines, and I read the flusher retry, backpressure and waitForFlush paths. I did not trace the full VFS overlay or read-your-own-writes logic, and I ran no tests. Treat this as targeted feedback, not a full audit.

Design. This looks well thought out.

  • The monotonic commitSeq/flushedSeq split is clear.
  • Failing closed on fence mismatch, wrong head, retry deadline or flusher failure matches the single-writer invariant in CLAUDE.md.
  • Awaited mode is untouched, and WASM and remote SQLite reject the mode at actor start, which is the right fail-by-default behavior.
  • Test coverage is broad: VFS, randomized, coordinator, client unit and driver tests.

CLAUDE.md conformance issues

  1. Wildcard enum arms. CLAUDE.md says never to use _ => on a Rust enum.
    • In the flusher retry path, match &error { Other, Response, _ => "unknown" } is guarded by #[allow(unreachable_patterns)]. That attribute hides the problem. Enumerate the variants of CommitBufferError so a new variant fails to compile.
    • In wait_for_flush in rivetkit-core, _ => SqliteRuntimeError::FlushFailed matches depot_client::vfs::FlushError. List the variants explicitly.
  2. Parsing an error message. parse_actual_head_txid(&message) pulls the actual head out of a fence-mismatch string. This is brittle, because rewording the engine message silently turns actual into None. Carry the head as a structured field on CommitBufferError::FenceMismatch if the source can provide it.
  3. Mutex use. The new code uses self.flush.progress.lock() and self.state.read() from both sync VFS callbacks and async code. That is acceptable under the parking_lot exception for VFS callbacks. Confirm no guard is held across an .await. wait_for_flush clones the progress before awaiting, which is good.

Potential issues

  • Backpressure and stalls. apply_deferred_backpressure calls runtime.block_on(changed.changed()) inside a sync VFS callback. If the flusher is stalled on retries, the commit blocks until the retry deadline. That is probably intended, but a metric or log when a commit blocks on backpressure would help diagnosis.
  • Backpressure exit conditions. The loop returns Ok(()) when the progress channel closes or closing is set. In both cases the commit proceeds with an overlay over the limit. That is fine if close drains the overlay afterwards, but a test pinning that behavior would help.
  • Retry classification. Retry metrics classify as transport or engine only. unknown is unreachable by construction (see item 1), so drop it.

Tests

  • Coverage is strong. Several of the new tests wait on timing. CLAUDE.md requires a one-line justification comment on every vi.waitFor, enforced by pnpm run check:wait-for-comments. Make sure that check passes in CI.
  • The tests are inline under tests/inline/, which fits the shim convention.
  • The PR says five fault::* tests fail on the base branch for unrelated reasons. Please link a tracking issue so they don't hide regressions here.

Docs and housekeeping

  • The SPEC is 1,143 lines. Good, but make sure it contains no em dashes, per the CLAUDE.md prose rule.
  • Docs were added to sqlite.mdx. Check the docs-sync table in .claude/reference/docs-sync.md for the new actor config option sqliteCommitMode and the new error codes.
  • The new error artifacts under rivetkit-rust/engine/artifacts/errors/ are committed, as required.

Overall this is solid work. Items 1 and 2 are worth fixing before merge.

🤖 Generated with Claude Code

This branch is being deployed

1 in progress deployment
rivet-frontend / actors-pr-5676 — 9e0ad73a Deployed Oct 14, 2026 by railway-app[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant