db: configurable commit durability and fullfsync, pg test-DB reuse - #726
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces configurable database durability levels for both SQLite and Postgres backends, defaulting to a "normal" durability level. It also adds a macOS-specific knob to disable the SQLite fullfsync pragma for higher write throughput, and implements a Postgres test fixture caching mechanism to correctly simulate daemon restarts in tests. Feedback highlights a performance bottleneck in tests where a global mutex is held during Docker container startup, a shared configuration mutation bug, and case-sensitivity issues when parsing synchronous level configurations.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| func resolveSqliteSynchronous(value string) (string, error) { | ||
| if value == "" { | ||
| return defaultSqliteSynchronous, nil | ||
| } | ||
|
|
||
| switch value { | ||
| case SqliteSynchronousFull, SqliteSynchronousNormal, | ||
| SqliteSynchronousOff: | ||
| return value, nil |
There was a problem hiding this comment.
Case-Sensitivity in Configuration Parsing
The resolveSqliteSynchronous function performs a case-sensitive check against the allowed values (full, normal, off). If an operator configures Synchronous using uppercase or mixed-case values (e.g., Normal or FULL), the daemon will fail to start. Normalizing the input to lowercase (e.g., using strings.ToLower) before validation would make the configuration parsing more robust and user-friendly.
| func resolveSqliteSynchronous(value string) (string, error) { | |
| if value == "" { | |
| return defaultSqliteSynchronous, nil | |
| } | |
| switch value { | |
| case SqliteSynchronousFull, SqliteSynchronousNormal, | |
| SqliteSynchronousOff: | |
| return value, nil | |
| func resolveSqliteSynchronous(value string) (string, error) { | |
| level := strings.ToLower(value) | |
| if level == "" { | |
| return defaultSqliteSynchronous, nil | |
| } | |
| switch level { | |
| case SqliteSynchronousFull, SqliteSynchronousNormal, | |
| SqliteSynchronousOff: | |
| return level, nil |
| func resolvePostgresSynchronousCommit(value string) (string, error) { | ||
| level := value | ||
| if level == "" { | ||
| level = defaultPostgresSynchronous | ||
| } | ||
|
|
||
| switch level { | ||
| case SqliteSynchronousFull: | ||
| return "on", nil | ||
|
|
||
| case SqliteSynchronousNormal: | ||
| return "local", nil | ||
|
|
||
| case SqliteSynchronousOff: | ||
| return "off", nil |
There was a problem hiding this comment.
Case-Sensitivity in Configuration Parsing
The resolvePostgresSynchronousCommit function performs a case-sensitive check against the allowed values (full, normal, off). If an operator configures Synchronous using uppercase or mixed-case values (e.g., Normal or FULL), the daemon will fail to start. Normalizing the input to lowercase (e.g., using strings.ToLower) before validation would make the configuration parsing more robust and user-friendly.
| func resolvePostgresSynchronousCommit(value string) (string, error) { | |
| level := value | |
| if level == "" { | |
| level = defaultPostgresSynchronous | |
| } | |
| switch level { | |
| case SqliteSynchronousFull: | |
| return "on", nil | |
| case SqliteSynchronousNormal: | |
| return "local", nil | |
| case SqliteSynchronousOff: | |
| return "off", nil | |
| func resolvePostgresSynchronousCommit(value string) (string, error) { | |
| level := strings.ToLower(value) | |
| if level == "" { | |
| level = defaultPostgresSynchronous | |
| } | |
| switch level { | |
| case SqliteSynchronousFull: | |
| return "on", nil | |
| case SqliteSynchronousNormal: | |
| return "local", nil | |
| case SqliteSynchronousOff: | |
| return "off", nil |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6430a8a649
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| sqliteCfg := db.DefaultSqliteConfig(networkDir) | ||
| sqliteCfg.NoFullfsync = s.cfg.DBNoFullfsync |
There was a problem hiding this comment.
For the actual darepod client path, DefaultSqliteConfig now defaults Synchronous to normal, but initDatabase only copies the new fullfsync option from daemon config and never exposes or forwards a synchronous setting. In standalone or embedded darepod deployments that need the previous FULL power-loss durability, there is no CLI/config/env way to restore it even though db.SqliteConfig now supports "full"; the only reachable behavior is the weaker default.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6430a8a649
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| sqliteCfg := db.DefaultSqliteConfig(networkDir) | ||
| sqliteCfg.NoFullfsync = s.cfg.DBNoFullfsync |
There was a problem hiding this comment.
For the actual darepod client path, DefaultSqliteConfig now defaults Synchronous to normal, but initDatabase only copies the new fullfsync option from daemon config and never exposes or forwards a synchronous setting. In standalone or embedded darepod deployments that need the previous FULL power-loss durability, there is no CLI/config/env way to restore it even though db.SqliteConfig now supports "full"; the only reachable behavior is the weaker default.
Useful? React with 👍 / 👎.
6430a8a to
dedb777
Compare
819feee to
efaceb1
Compare
27a1bf4 to
1047332
Compare
1047332 to
d48f908
Compare
|
@claude review this |
|
Claude finished @Roasbeef's task in 1m 57s —— View job PR Review:
|
d48f908 to
bcad772
Compare
The systest restart harness (CrashRestartClient) simulates a daemon restart by reopening the SAME dbPath and expects the previously persisted state to survive. On sqlite this works for free because NewTestDBHandleFromPath reattaches to the on-disk file at dbPath and the restart opens a fresh handle over it. The postgres variant ignored dbPath and called NewTestPostgresDB on every invocation, spinning up a brand-new empty database each time. A restart therefore silently dropped all persisted state (OOR sessions, wallet, boarding), so four restart-recovery systests passed on sqlite but failed on postgres. CI never caught it because the systest job only runs on sqlite. Memoize the postgres fixture (the docker container and its database) per dbPath behind a mutex-guarded map. The first handle for a path creates the database, runs migrations, and registers teardown. Later handles for the same path reuse that database but open a fresh connection pool with migrations skipped, since the prior store's pool is closed on shutdown and the schema already exists. Each reuse also registers a cleanup that closes its pool (sql.DB.Close is idempotent), and the same-testing.TB / unique-dbPath invariant is documented so the memoization is only ever reused within the test that first opened it. dbPaths are unique per client per test (temp dirs), so reuse only happens within a single test (the original handle plus its restart).
bcad772 to
34e4c93
Compare
The per-commit fsync forced by synchronous=full was the throughput ceiling for the persistence stack: under multi-actor write contention it ran roughly 3.9x slower than NORMAL in benchmarks, since every commit blocked on an extra disk sync. NORMAL is safe here because the OOR/outbox/serverconn stack is at-least-once, idempotent, and deterministic. Under WAL mode NORMAL omits the per-commit WAL fsync (the WAL is synced only before a checkpoint), so a process crash loses zero durably-committed work, a power loss is recoverable by deterministic replay of the dropped tail, and NORMAL is not OFF, so the database is never corrupted. The second knob is macOS-specific. The fullfsync pragma only matters on darwin, where a plain fsync does not guarantee data reached stable storage. Under NORMAL it governs the WAL checkpoint sync rather than any per-commit work, but checkpoints recur continuously under sustained write load and each F_FULLFSYNC waits on a full hardware cache flush: the stress benchmark measures it at roughly a third of attainable throughput (10.4 -> 14.2 payments/s with it disabled). In this commit, we expose both as configurable knobs on the SQLite backend. The synchronous level (full/normal/off) defaults to NORMAL and rejects unknown values at startup; fullfsync stays enabled by default. On the daemon they live under the db.sqlite.* namespace (db.sqlite.synchronous, db.sqlite.nofullfsync), matching the dotted config convention used elsewhere; the db.postgres.* namespace is reserved. Postgres-side durability tuning is deferred to a separate change.
34e4c93 to
56ca927
Compare
2 participants
In this PR, we expose the SQLite storage durability levers that the OOR
benchmarking campaign showed were the first two ceilings on payment
throughput, and we make the postgres test harness reuse its database across
daemon restarts so restart-style systests work on both backends.
The headline knob is commit durability:
synchronousis now configurable,defaulting to NORMAL on the client (was FULL). Under WAL, NORMAL omits the
per-commit WAL fsync (the WAL is synced only before a checkpoint), so a power
loss can roll back the last few commits but can never corrupt the database; an
OS or app crash loses nothing. That contract composes cleanly with the durable
mailbox model, where every cross-actor message is redelivered until acked, so a
dropped tail commit replays rather than strands. On the high-contention stress
shape this single default was worth 1.3-2.3x throughput with p50 falling from
~6s to ~700ms: the per-commit fsync was the latency tail.
The second knob is macOS specific: sqlite on darwin issues F_FULLFSYNC (a full
disk-barrier flush), a hidden second fsync tax that profiling showed cost ~35%
throughput on the bench host. Under NORMAL it governs the WAL checkpoint sync
rather than a per-commit fsync, but checkpoints recur continuously under
sustained write load, so the
fullfsyncpragma is now configurable. Thedefault is unchanged.
Both knobs are exposed on the daemon under the
db.sqlite.*namespace(
db.sqlite.synchronous,db.sqlite.nofullfsync), matching the dotted configconvention used elsewhere; the
db.postgres.*namespace is reserved.Postgres-side durability tuning (
synchronous_commit) is deliberately deferredto a separate change, and the operator-side default is a separate decision that
stays FULL over there (darepo#553).
The test-infra commit keys the postgres test database off the dbPath so a
restarted daemon lands on the same database rather than a fresh one, which the
restart-recovery systests depend on.
This PR builds on #693 and is part of the OOR optimization train.