fix(core): retry the WAL mode flip a concurrent opener wins (GH #111)

[samkeen]

On a vault with no index yet, a second b2 command racing the first-ever b2 reindex failed one of the two processes with "database is locked" — ~40% of the documented cold-vault flow (b2 reindex & then b2 status), with the reindex usually the loser, so a backgrounded cold index could silently not happen.

Fixes #111. Supersedes #113 (same first commit, plus the review follow-ups below).

The lock busy_timeout cannot cover

journal_mode = WAL is the one statement in db::open that takes a write lock, and only when it actually changes the mode — so exactly once per vault, on the first open. It is not covered by busy_timeout: the flip upgrades an already-open read transaction, and SQLite only consults the busy handler for a write lock taken from no transaction at all (btreeBeginTrans's retry loop is guarded on inTransaction == TRANS_NONE, and sqlite3PagerBegin documents the RESERVED lock as taken without the handler).

Reordering the pragmas therefore does not help — measured at 6 failures in 160 racing opens with busy_timeout set explicitly first, and rusqlite already arms the same 5 s at Connection::open regardless. So the retry is ours: enter_wal_mode reattempts the flip on SQLITE_BUSY/SQLITE_LOCKED with a doubling backoff (~4 s total). It converges immediately because it only has to outlast the other opener's flip — on the next attempt the database is already WAL, where the pragma is a no-op needing no lock.

The flip is verified, not assumed

PRAGMA journal_mode = WAL reports the mode it ended on as a row, and there is one case where it declines the flip with no error at all: a filesystem with no shared-memory support (sqlite3PagerWalSupported — a network share, or a synced folder on some setups, both plausible homes for a personal vault), where SQLite returns the old mode and SQLITE_OK. A row-discarding execute_batch would report success having changed nothing, so enter_wal_mode queries the mode back instead.

A decline is deliberately not an error — B2 is correct in rollback-journal mode, and refusing to open such a vault would be the worse bug — and deliberately not retried, since nothing is holding a lock and no amount of waiting would change the answer. It logs at WARN and returns.

Tests

first_open_waits_out_a_held_lock is the gate. A BEGIN IMMEDIATE holder pins RESERVED — readers in, writers out — which is exactly the lock the flip trips over, so it fails in ~200 µs unfixed and passes with the retry. (BEGIN EXCLUSIVE would block the flip's read half instead, which the busy handler does cover, and would assert nothing.) It also asserts the resulting mode really is wal, so the wait bought the real thing.

concurrent_openers_of_a_fresh_index_coexist is a coexistence smoke test, not a second gate, and its docstring says so — because the name would otherwise promise a regression it does not catch. The flip's race window is ~200 µs wide, so eight barrier-released threads land inside it only sometimes: measured at 3 failures in 25 runs against the unfixed open, i.e. green ~88% of the time on the very bug it appears to name. What it does buy is the property no deterministic single-lock test can state — that N concurrent openers finish at all, with no deadlock, no starved thread, and no error escaping the retry, which is what would break if open ever took a lock it holds rather than one it waits out.

Scope: where open's concurrency safety stops

This closes the SQLITE_BUSY half only. migrate() still runs its schema DDL unwrapped and unserialized, so concurrent openers of a stale-schema index can interleave drop-and-rebuild — filed as #114 with a reproducer: ~2.5% of opens fail with no such table, and one round in 40 left four tables missing while every opener returned Ok. That is a different error class, so this PR's retry neither catches it nor should. The substrate.rs module header records the boundary explicitly so the suite doesn't read as covering ground it doesn't.

Summary by CodeRabbit

• Bug Fixes

  • Improved database opening reliability when multiple processes or threads access a database simultaneously.
  • Added automatic retries for temporary lock contention when enabling WAL mode.
  • Prevented concurrent first-time opens from failing, deadlocking, or starving.
  • Databases now gracefully continue when WAL mode is unavailable due to filesystem limitations.

• Tests

  • Added coverage for concurrent database initialization and lock contention scenarios.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Lcfs1nfA17zSqQGwRn2NeF

[coderabbitai]

📝 Walkthrough

Walkthrough

open() now activates WAL separately from connection pragmas and retries lock-contention failures with exponential backoff. New tests cover a held rollback-journal lock and eight simultaneous fresh-index opens.

Changes

WAL initialization

Layer / File(s)	Summary
WAL activation and retry flow crates/b2-core/src/db.rs	open() applies connection pragmas before invoking enter_wal_mode, which retries SQLite lock contention with bounded exponential backoff and handles unsupported WAL mode.
Contention regression coverage crates/b2-core/tests/substrate.rs	Tests verify that a contended first open succeeds and that eight simultaneous fresh-index opens complete successfully.

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

Sequence Diagram(s)

sequenceDiagram
  participant Opener
  participant enter_wal_mode
  participant SQLite
  Opener->>SQLite: Configure connection pragmas
  Opener->>enter_wal_mode: Activate WAL
  enter_wal_mode->>SQLite: Execute journal_mode = WAL
  SQLite-->>enter_wal_mode: WAL mode or lock contention
  enter_wal_mode->>enter_wal_mode: Wait and retry when locked
  enter_wal_mode-->>Opener: Return initialization result

Loading

Possibly related issues

• Concurrent open of a *fresh* index fails with "database is locked" — journal_mode = WAL runs before busy_timeout is set #111 — Directly covers contention-safe WAL initialization and concurrent fresh-index opens.

Possibly related PRs

• AlteredCraft/B2#39 — Previously modified the same database opening and SQLite pragma setup.

Suggested reviewers: claude

Poem

I’m a rabbit guarding WAL,
Through locks and threads, we hop, not fall.
Backoff softly, try once more,
Eight fresh openers reach the door.
SQLite settles, calm and bright.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main change: retrying the SQLite WAL mode flip under concurrent open contention.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch claude/pr-113-review-yfswdx

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

🧹 Nitpick comments (1)

crates/b2-core/tests/substrate.rs (1)

177-187: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Bind the opener count once. Barrier::new(8) and (0..8) must stay in lockstep; if they ever drift the test hangs on the barrier rather than failing.

♻️ Proposed tweak

-    let start = Arc::new(Barrier::new(8));
-    let openers: Vec<_> = (0..8)
+    const OPENERS: usize = 8;
+    let start = Arc::new(Barrier::new(OPENERS));
+    let openers: Vec<_> = (0..OPENERS)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/b2-core/tests/substrate.rs` around lines 177 - 187, Bind the
concurrent opener count to a single local value and reuse it for both
Barrier::new and the opener range in this test, ensuring the synchronization
count cannot drift from the number of spawned threads.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@crates/b2-core/tests/substrate.rs`:
- Around line 177-187: Bind the concurrent opener count to a single local value
and reuse it for both Barrier::new and the opener range in this test, ensuring
the synchronization count cannot drift from the number of spawned threads.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 554eb6b4-a875-4006-a326-61f25832875f

📥 Commits

Reviewing files that changed from the base of the PR and between 488f035 and 5ab2816.

📒 Files selected for processing (2)

• crates/b2-core/src/db.rs
• crates/b2-core/tests/substrate.rs