fix(store): make WORM/ledger appends atomic #266

2026-07-31T16:20:57Z

KofTwentyTwo commented

2026-07-31 16:20:57 +00:00

(Migrated from github.com)

Closes #245 (Refs #164)

Summary

DatabaseEngine::save_filter_execution_log and append_worm_audit_record read the latest chain hash and then insert the new row as two separate statements with no lock. Since DatabaseEngine is Arc-shared across the daemon, two concurrent appends could both read the same prev-hash and fork the tamper-evident hash chain.
Fixed by wrapping each read+insert in a single BEGIN IMMEDIATE transaction (pool.begin_with("BEGIN IMMEDIATE")), which acquires SQLite's write lock before any statement runs, so a concurrent append blocks until the in-flight one commits rather than racing on the same prev-hash. busy_timeout(5000ms) is already configured on the pool, so contending writers wait rather than error.
No change to the hash-chain format or WORM/ledger semantics — only the append is made atomic.

Verification that the regression tests are real

Before finalizing, I temporarily reverted just the BEGIN IMMEDIATE mechanism (keeping the new tests) and re-ran the two new tests 5 times: they failed every time, with either a forked prev_hash/previous_block_hash or a UNIQUE constraint failed: worm_audit_records.sequence error. With the fix restored, both tests passed consistently across repeated runs.

Tests added (crates/nuncio-store/src/db.rs)

append_worm_audit_record_is_atomic_under_concurrency — spawns 20 concurrent tokio::spawn appends against the same Arc<DatabaseEngine>, then asserts: exactly 20 records, contiguous sequence numbers with no duplicates, each record's previous_block_hash equals the immediately preceding record's record_hmac, and verify_worm_audit_chain() passes on the full persisted chain.
save_filter_execution_log_is_atomic_under_concurrency — same shape for the filter execution ledger: 20 concurrent appends, no duplicate row ids, each entry's prev_hash equals the prior entry's hash, and verify_execution_log_chain() passes.

Gate output

cargo fmt --all -- --check   -> clean
cargo check-all              -> Finished, 0 warnings
cargo test-all               -> 456 passed; 0 failed; 0 ignored (across all workspace crates)

nuncio-store alone: 66 passed; 0 failed; 0 ignored (includes the 2 new concurrency tests).

Notes / decisions

Chose a DB-level BEGIN IMMEDIATE transaction over an in-process tokio::sync::Mutex because it serializes correctly across the whole connection pool (and any future additional daemon processes/connections), not just within one process's in-memory lock.
No proto or CLI surface changes — this is a pure engine correctness fix, so no new CLI command or E2E test was needed per the DoD's "engine + proto + CLI + E2E" rule (there is no new capability, just a bug fix to an existing one).

Closes #245 (Refs #164) ## Summary - `DatabaseEngine::save_filter_execution_log` and `append_worm_audit_record` read the latest chain hash and then insert the new row as two separate statements with no lock. Since `DatabaseEngine` is `Arc`-shared across the daemon, two concurrent appends could both read the same prev-hash and fork the tamper-evident hash chain. - Fixed by wrapping each read+insert in a single `BEGIN IMMEDIATE` transaction (`pool.begin_with("BEGIN IMMEDIATE")`), which acquires SQLite's write lock before any statement runs, so a concurrent append blocks until the in-flight one commits rather than racing on the same prev-hash. `busy_timeout(5000ms)` is already configured on the pool, so contending writers wait rather than error. - No change to the hash-chain format or WORM/ledger semantics — only the append is made atomic. ## Verification that the regression tests are real Before finalizing, I temporarily reverted just the `BEGIN IMMEDIATE` mechanism (keeping the new tests) and re-ran the two new tests 5 times: they failed every time, with either a forked `prev_hash`/`previous_block_hash` or a `UNIQUE constraint failed: worm_audit_records.sequence` error. With the fix restored, both tests passed consistently across repeated runs. ## Tests added (crates/nuncio-store/src/db.rs) - `append_worm_audit_record_is_atomic_under_concurrency` — spawns 20 concurrent `tokio::spawn` appends against the same `Arc<DatabaseEngine>`, then asserts: exactly 20 records, contiguous sequence numbers with no duplicates, each record's `previous_block_hash` equals the immediately preceding record's `record_hmac`, and `verify_worm_audit_chain()` passes on the full persisted chain. - `save_filter_execution_log_is_atomic_under_concurrency` — same shape for the filter execution ledger: 20 concurrent appends, no duplicate row ids, each entry's `prev_hash` equals the prior entry's `hash`, and `verify_execution_log_chain()` passes. ## Gate output ``` cargo fmt --all -- --check -> clean cargo check-all -> Finished, 0 warnings cargo test-all -> 456 passed; 0 failed; 0 ignored (across all workspace crates) ``` `nuncio-store` alone: 66 passed; 0 failed; 0 ignored (includes the 2 new concurrency tests). ## Notes / decisions - Chose a DB-level `BEGIN IMMEDIATE` transaction over an in-process `tokio::sync::Mutex` because it serializes correctly across the whole connection pool (and any future additional daemon processes/connections), not just within one process's in-memory lock. - No proto or CLI surface changes — this is a pure engine correctness fix, so no new CLI command or E2E test was needed per the DoD's "engine + proto + CLI + E2E" rule (there is no new capability, just a bug fix to an existing one).

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: KofTwentyTwo/nuncio#266