fix: retry WAL conversion on lock contention at connection setup - #361
Conversation
7e90eec to
88a235f
Compare
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
88a235f to
c2d7709
Compare
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Converting a rollback-journal database to WAL needs the exclusive lock, and SQLite can return SQLITE_BUSY for that upgrade without consulting the busy handler while sibling connections are mid-setup on the same file. Concurrent process startup (gateway + CLI + sub-agents) on a not-yet-WAL database therefore crashed sporadically with 'database is locked' from configure_connection. Wrap the journal_mode pragma in a bounded exponential-backoff retry (budget = SQLITE_BUSY_TIMEOUT_MS) and set busy_timeout first. Once the database is in WAL mode the pragma is a plain read, so steady state is unaffected. Also harden the concurrent-migration regression test that exposed this: its barrier had no timeout, so the pre-barrier failure parked the seven surviving threads forever and deadlocked the whole test run instead of reporting the error. The barrier now times out and aborts on failure, join is bounded, and the assert surfaces the root-cause exception. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
c2d7709 to
09a2307
Compare
|
Merge-ready on Self-contained — one retry path in |
Tosko4
left a comment
There was a problem hiding this comment.
Reviewed 09a2307 against current main.
I reproduced the startup failure on the exact base with a rollback-journal database held under an exclusive lock: the base failed immediately with database is locked, while this head waited for release and completed in WAL mode.
Validation on the exact head:
tests/test_crash_safe_wal.py: 14 passed- concurrent migration regression: 30 consecutive passes
- full suite: 2297 passed, 12 xfailed
- full release validation: passed
The retry remains scoped to lock-related OperationalErrors, non-lock failures still propagate, and the hardened barrier can no longer hide a setup failure behind a deadlocked test run. I found no blocking issue in this change.
Summary
Follow-up to #346 (concurrent-startup migration race), one phase earlier in the same failure class.
configure_connectionrunsPRAGMA journal_mode=WALas its first statement. Converting a rollback-journal database to WAL needs the exclusive lock, and SQLite can returnSQLITE_BUSYfor that upgrade without consulting the busy handler while sibling connections are mid-setup on the same file. Concurrent process startup (gateway + CLI + sub-agents — the documented topology) on a not-yet-WAL database therefore crashed sporadically withsqlite3.OperationalError: database is lockedduring store construction. Steady state is immune: once the database is WAL, the pragma is a plain read.Two changes:
db_bootstrap.configure_connection: setbusy_timeoutfirst, then run the WAL conversion through_execute_wal_conversion_with_lock_retry— a bounded exponential-backoff retry (5ms→250ms steps, total budgetSQLITE_BUSY_TIMEOUT_MS) that only swallows lock-contention errors and re-raises everything else or on budget exhaustion.tests/test_crash_safe_wal.py::TestConcurrentStartupMigration: the regression test that exposed this had a second bug of its own —barrier.wait()without timeout. The thread that crashed pre-barrier left the seven survivors parked forever, so instead of a red test the whole pytest run deadlocked (observed twice as avalidate_release --fullhang at 4%, 8 threads in futex wait, zero CPU). The barrier now uses a timeout andabort()on failure,joinis bounded with a liveness assert, and the final assert reports the root-cause exception instead of hiding it.Why
The hang variant is worse than the crash variant: it silently stalls any CI/validation run that executes the suite, with no error to act on. And the underlying crash is the same first-boot scenario #346 fixed for column DDL — a fresh install or an upgrade from a pre-WAL database with several Hermes processes starting together.
Validation
Reproduction & proof (Linux, 8-thread barrier race on a rollback-journal seed DB):
database is lockedfromconfigure_connection(PRAGMA journal_mode=WAL) within 15 attempts; the unhardened test hung 2/10 runs (killed only by an external timeout).Notes
Refs
db_bootstrap.py::configure_connection,SQLITE_BUSY_TIMEOUT_MS