fix(db): fsync the ACK-gating sync commit (synchronous=FULL bracket) - #233
fix(db): fsync the ACK-gating sync commit (synchronous=FULL bracket)#233abdulsaheel wants to merge 2 commits into
Conversation
The DB runs WAL + synchronous=NORMAL, under which a commit is durable only at the next checkpoint, not at commit. commitSyncBatch persists the sync batch (raw_archive + samples + decoded + trim cursor) and returns; the caller then writes the BLE batch-ACK and the band trims its flash. A kernel panic / battery-yank AFTER the ACK but BEFORE the -wal is checkpointed lost those just-committed rows from the phone while they were already gone from the band. The commit-before-ACK ordering held; the durability did not. Raise durability to synchronous=FULL (fsync AT commit) for this one commit only, leaving every other path at NORMAL — they are all recomputable and FULL everywhere is brutally slow. synchronous is per-connection and cannot change mid-transaction, so it is set BEFORE db.transaction opens and reset to NORMAL in a finally (a leaked FULL would fsync every later write on the connection forever). Both the main and background-isolate drains funnel through commitSyncBatch, each on its own connection, so this single bracket covers both. PRAGMA synchronous returns no rows -> execute(), kept non-fatal like the open-time PRAGMAs. Adds a focused test (spies the FULL/NORMAL SQL bracket and reads resting PRAGMA synchronous) covering both a normal commit and a throwing one.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
📝 WalkthroughWalkthrough
ChangesSync durability
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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. Comment |
PR Reviewer Guide 🔍(Review updated until commit 4928f28)Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Latest suggestions up to 4928f28
Previous suggestionsSuggestions up to commit 04a887e
|
|
@coderabbitai review |
|
|
@coderabbitai review |
|
|
@coderabbitai review |
|
The headless drain (background_sync.dart) is the iOS CoreBluetooth-restoration recovery path and runs in the MAIN isolate on the same shared _db connection — not a separate per-isolate connection as the prior comment claimed. The bracket is safe not because of isolation but because BandOwnership + the single-flight offload processor guarantee the two drains never overlap on one connection. Document that as the load-bearing invariant so a future concurrent caller does not silently defeat the FULL window.
|
Persistent review updated to latest commit 4928f28 |
|
@coderabbitai review |
|
|
@coderabbitai review |
|
|
@coderabbitai review |
|
|
@coderabbitai review |
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Superseded by #235 — consolidated into the single |
User description
Problem — a power-loss durability window
The DB opens with
PRAGMA journal_mode=WAL+PRAGMA synchronous=NORMAL(lib/data/db.dart_open). Under WAL,NORMALdoes not fsync at commit — a committed transaction is only guaranteed durable at the next checkpoint.commitSyncBatchis the ACK-gating commit of the safe-trim invariant: it atomically persists the sync batch (raw_archive+samples+decoded_onehz/decoded_rr+ trim cursor) and returns; the caller (ble_engine/app_state, andbackground_sync) then writes the BLE batch-ACK, and the band trims its flash.So there is a window: a kernel panic / battery-yank after the ACK but before the
-walis checkpointed loses the just-committed rows from the phone while they are already gone from the band. The commit-before-ACK ordering was correct; the durability underneath it was not.Fix
Raise durability to
synchronous=FULL(fsync at commit) only aroundcommitSyncBatch's transaction, leaving every other path atNORMAL— all other writes are recomputable (raw re-syncs from the band; derived recomputes), andFULLeverywhere is brutally slow on the hot ingest/derive paths.Correctness details that matter here:
synchronouscannot be changed mid-transaction — it is set on the connection beforedb.transaction(...)opens and reset after it commits.NORMALis in afinally, so a throwing commit can't leakFULLand fsync every subsequent write on that connection forever.synchronousis per-connection. Both the main-isolate drain and the background-isolate drain (background_sync.dart) funnel throughcommitSyncBatch, each on its own per-isolate connection, so this single bracket covers both.PRAGMA synchronous=FULL/NORMALreturns no rows →db.execute(...)(notrawQuery). Kept inside the same non-fatal try/catch discipline as the open-time PRAGMAs so a PRAGMA throw can never brick the commit.Invariants preserved:
commitSyncBatchstays a single atomic transaction; commit-before-ACK ordering unchanged; hot recompute/derive paths stayNORMAL.Test
test/ack_commit_sync_full_test.dartwraps the ffi factory inSqfliteDatabaseFactoryLoggerto spy the exactPRAGMA synchronous=FULL→…=NORMALbracket around the commit, and reads restingPRAGMA synchronous(FULL=2, NORMAL=1) onLocalDb's own connection. Two cases:FULL, restoresNORMAL.RangeErrorinside the txn) —NORMALis still restored via thefinally(no leakedFULL).sqflite_commonis added as a direct dev-dependency (it was already transitive viasqflite_common_ffi) so the logger import satisfiesdepend_on_referenced_packages, matching the repo's existing convention for test-imported transitive deps.Verification
flutter test— new file +raw_archive_test,db_integrity_test,local_persistence_test,ble_safe_trim_testall green.flutter analyzeon changed files — no issues.PR Type
Bug fix, Tests
Description
Fixes power-loss data loss window in ACK-gating commit by fsyncing before BLE ACK
Wraps
commitSyncBatchtransaction withsynchronous=FULLbracket, restoringNORMALinfinallyAdds focused test verifying FULL/NORMAL bracket and leak-prevention on throwing commit
Adds
sqflite_commondev dependency for SQL-spy logger in new testDiagram Walkthrough
File Walkthrough
db.dart
Add synchronous=FULL bracket around ACK-gating commitlib/data/db.dart
commitSyncBatch'sdb.transactionwithPRAGMA synchronous=FULLbefore open and
PRAGMA synchronous=NORMALin afinallyafter commitexecute()(notrawQuery) and are non-fatal,matching open-time PRAGMA discipline
window, per-connection semantics, and why only this path needs FULL
bracket is new
ack_commit_sync_full_test.dart
Add test pinning synchronous=FULL bracket and leak preventiontest/ack_commit_sync_full_test.dart
synchronous=FULL/NORMALbracket incommitSyncBatchSqfliteDatabaseFactoryLoggerto spy allPRAGMA synchronous=statements on the connection
after, resting value is NORMAL
when the transaction throws (no FULL leak)
pubspec.yaml
Add sqflite_common dev dependency for SQL-spy test loggerpubspec.yaml
sqflite_common: ^2.5.0as a dev dependencyack_commit_sync_full_test.dartto accessSqfliteDatabaseFactoryLoggerfor SQL spyingSummary by CodeRabbit
Bug Fixes
Tests