Skip to content

fix(db): fsync the ACK-gating sync commit (synchronous=FULL bracket) - #233

Closed
abdulsaheel wants to merge 2 commits into
mainfrom
fix/ack-commit-synchronous-full
Closed

fix(db): fsync the ACK-gating sync commit (synchronous=FULL bracket)#233
abdulsaheel wants to merge 2 commits into
mainfrom
fix/ack-commit-synchronous-full

Conversation

@abdulsaheel

@abdulsaheel abdulsaheel commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator

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, NORMAL does not fsync at commit — a committed transaction is only guaranteed durable at the next checkpoint.

commitSyncBatch is 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, and background_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 -wal is 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 around commitSyncBatch's transaction, leaving every other path at NORMAL — all other writes are recomputable (raw re-syncs from the band; derived recomputes), and FULL everywhere is brutally slow on the hot ingest/derive paths.

Correctness details that matter here:

  • synchronous cannot be changed mid-transaction — it is set on the connection before db.transaction(...) opens and reset after it commits.
  • The reset to NORMAL is in a finally, so a throwing commit can't leak FULL and fsync every subsequent write on that connection forever.
  • synchronous is per-connection. Both the main-isolate drain and the background-isolate drain (background_sync.dart) funnel through commitSyncBatch, each on its own per-isolate connection, so this single bracket covers both.
  • PRAGMA synchronous=FULL/NORMAL returns no rowsdb.execute(...) (not rawQuery). 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: commitSyncBatch stays a single atomic transaction; commit-before-ACK ordering unchanged; hot recompute/derive paths stay NORMAL.

Test

test/ack_commit_sync_full_test.dart wraps the ffi factory in SqfliteDatabaseFactoryLogger to spy the exact PRAGMA synchronous=FULL…=NORMAL bracket around the commit, and reads resting PRAGMA synchronous (FULL=2, NORMAL=1) on LocalDb's own connection. Two cases:

  1. Normal commit — brackets FULL, restores NORMAL.
  2. Throwing commit (mismatched raws/samples → RangeError inside the txn) — NORMAL is still restored via the finally (no leaked FULL).

sqflite_common is added as a direct dev-dependency (it was already transitive via sqflite_common_ffi) so the logger import satisfies depend_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_test all green.
  • flutter analyze on 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 commitSyncBatch transaction with synchronous=FULL bracket, restoring NORMAL in finally

  • Adds focused test verifying FULL/NORMAL bracket and leak-prevention on throwing commit

  • Adds sqflite_common dev dependency for SQL-spy logger in new test


Diagram Walkthrough

flowchart LR
  A["commitSyncBatch called"] -- "PRAGMA synchronous=FULL (best-effort)" --> B["db.transaction\n(raw_archive + samples +\ndecoded_onehz + cursor)"]
  B -- "commit succeeds or throws" --> C["finally: PRAGMA synchronous=NORMAL"]
  C --> D["caller writes BLE batch-ACK\n(band trims flash)"]
  E["WAL + NORMAL (all other paths)\nrecomputable writes stay fast"] -. "unchanged" .-> D
Loading

File Walkthrough

Relevant files
Bug fix
db.dart
Add synchronous=FULL bracket around ACK-gating commit       

lib/data/db.dart

  • Wraps commitSyncBatch's db.transaction with PRAGMA synchronous=FULL
    before open and PRAGMA synchronous=NORMAL in a finally after commit
  • Both PRAGMA calls use execute() (not rawQuery) and are non-fatal,
    matching open-time PRAGMA discipline
  • Adds detailed inline comment explaining the power-loss durability
    window, per-connection semantics, and why only this path needs FULL
  • No logic change to the transaction body itself; only the durability
    bracket is new
+103/-71
Tests
ack_commit_sync_full_test.dart
Add test pinning synchronous=FULL bracket and leak prevention

test/ack_commit_sync_full_test.dart

  • New test file pinning the synchronous=FULL/NORMAL bracket in
    commitSyncBatch
  • Uses SqfliteDatabaseFactoryLogger to spy all PRAGMA synchronous=
    statements on the connection
  • Tests normal commit path: verifies FULL set before, NORMAL restored
    after, resting value is NORMAL
  • Tests throwing commit path: verifies NORMAL is still restored even
    when the transaction throws (no FULL leak)
+98/-0   
Dependencies
pubspec.yaml
Add sqflite_common dev dependency for SQL-spy test logger

pubspec.yaml

  • Adds sqflite_common: ^2.5.0 as a dev dependency
  • Required by ack_commit_sync_full_test.dart to access
    SqfliteDatabaseFactoryLogger for SQL spying
+4/-0     

Summary by CodeRabbit

  • Bug Fixes

    • Improved data durability during batch synchronization by temporarily enabling stronger SQLite persistence guarantees.
    • Ensured database settings are restored after both successful and failed commits.
    • Preserved atomic handling of archived data, samples, decoded data, and cursor updates.
  • Tests

    • Added coverage for durability settings during successful and failed synchronization commits.

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.
@coderabbitai

coderabbitai Bot commented Aug 11, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 957b49d4-c288-480b-9e6b-cd8cd6475cd7

📥 Commits

Reviewing files that changed from the base of the PR and between 6bddec4 and 4928f28.

⛔ Files ignored due to path filters (1)
  • pubspec.lock is excluded by !**/*.lock
📒 Files selected for processing (3)
  • lib/data/db.dart
  • pubspec.yaml
  • test/ack_commit_sync_full_test.dart

📝 Walkthrough

Walkthrough

commitSyncBatch now uses SQLite synchronous=FULL during its ACK-gated transaction and restores NORMAL after success or failure. A Flutter FFI test observes both transitions and validates error handling.

Changes

Sync durability

Layer / File(s) Summary
Synchronous mode transaction handling
lib/data/db.dart
commitSyncBatch sets PRAGMA synchronous=FULL before the atomic sync-batch transaction and restores NORMAL in a finally block.
Synchronous mode validation
test/ack_commit_sync_full_test.dart, pubspec.yaml
The test logger verifies FULL and NORMAL transitions after successful and failing commits. sqflite_common supports SQL observation.

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

Possibly related PRs

  • OpenStrap/edge#198: Related to durable commitSyncBatch persistence and commit-failure handling.

Suggested reviewers: svssathvik7, dannymcc, localhoop

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: enforcing SQLite synchronous=FULL during the ACK-gating commit.
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.

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

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

@github-actions

github-actions Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

(Review updated until commit 4928f28)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

PRAGMA via execute()

The PR comment explicitly notes "PRAGMA synchronous=FULL/NORMAL returns NO rows → execute() (not rawQuery)". This is correct for synchronous. However, AGENTS.md §3.11 states that PRAGMA journal_mode=WAL must go through rawQuery (not execute) because it returns a row and execute bricks iOS Darwin sqflite. The new code uses db.execute('PRAGMA synchronous=FULL') and db.execute('PRAGMA synchronous=NORMAL')synchronous genuinely returns no rows so execute is correct here. No issue. (Confirming this is not a violation.)

await db.execute('PRAGMA synchronous=FULL');
Silent FULL leak on PRAGMA failure

If PRAGMA synchronous=FULL succeeds but the subsequent db.transaction(...) throws, the finally block correctly restores NORMAL. However, if PRAGMA synchronous=FULL itself throws (caught silently), the connection remains at NORMAL and the transaction proceeds without the durability upgrade — but crucially, the finally block still executes PRAGMA synchronous=NORMAL, which is a no-op but harmless. The real concern is the opposite: if PRAGMA synchronous=FULL succeeds and then the finally's PRAGMA synchronous=NORMAL throws (also caught silently), the connection is permanently left at FULL, fsyncing every subsequent write. The comment acknowledges this but the mitigation is only "non-fatal" swallowing. Under sqflite_ffi this is unlikely, but on a degraded iOS SQLite connection it is a realistic wedge scenario matching the sticky-latch pattern (§4.3). There is no test covering the case where the restore PRAGMA itself throws.

} finally {
  // ALWAYS restore NORMAL — even if the commit threw — so a leaked FULL does
  // not fsync every subsequent write on this connection. Non-fatal.
  try {
    await db.execute('PRAGMA synchronous=NORMAL');
  } catch (_) {
    /* non-fatal — see open-time PRAGMA discipline */
  }
}
Throw-path test fragility

The "commit throws" test relies on passing raws with one element and samples as an empty list, expecting a RangeError to be thrown inside the transaction after FULL is set. This is a white-box assumption about the internal iteration order (samples[i] being accessed before the transaction commits). If the internal implementation changes — e.g., if a null-check or length guard is added before the loop — the test would no longer throw, would pass vacuously, and would stop pinning the finally-restores-NORMAL invariant. The test should instead inject a failure mechanism that is guaranteed to fire inside the transaction regardless of implementation details (e.g., a mock/fake db that throws on transaction).

test('synchronous is restored to NORMAL even when the commit throws', () async {
  syncStmts.clear();
  // raws non-empty but samples empty → samples[i] throws RangeError INSIDE the
  // db.transaction, after FULL is set. The finally must still restore NORMAL.
  await expectLater(
    LocalDb.commitSyncBatch([recAt(6001)], const <Sample?>[]),
    throwsA(isA<RangeError>()),
  );

  expect(syncStmts, ['pragma synchronous=full', 'pragma synchronous=normal'],
      reason: 'a thrown commit must not leak FULL');
  expect(await restingSynchronous(), 1,
      reason: 'FULL did not leak past the throwing commit');
});

@github-actions

github-actions Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Latest suggestions up to 4928f28
Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Ensure freshness write only occurs after successful commit

_writeCaptureFreshness is called after the try/finally block, so it only executes
when the transaction succeeds (exceptions propagate through finally). To make this
invariant explicit and guard against future refactoring accidentally placing it
inside a catch, move it inside the try block after the transaction, before the
finally. This makes the success-only intent structurally clear and prevents it from
being accidentally reached on a failed commit path.

lib/data/db.dart [1317]

-await _writeCaptureFreshness(raws);
+try {
+  await db.execute('PRAGMA synchronous=FULL');
+} catch (_) {}
+try {
+  await db.transaction((txn) async {
+    ...
+  });
+  await _writeCaptureFreshness(raws);
+} finally {
+  try {
+    await db.execute('PRAGMA synchronous=NORMAL');
+  } catch (_) {}
+}
Suggestion importance[1-10]: 4

__

Why: The suggestion correctly notes that _writeCaptureFreshness already only runs on success (exceptions propagate through finally), but moving it inside the try block would make the intent more explicit and prevent accidental misuse in future refactoring. However, the current behavior is already correct, so this is a minor readability/maintainability improvement.

Low
General
Track upgrade success before resetting durability level

If PRAGMA synchronous=FULL throws, the connection remains at NORMAL but the finally
block still executes PRAGMA synchronous=NORMAL — which is harmless — however the
transaction then proceeds without the durability upgrade and the ACK-gating commit
is silently under-durable with no indication to the caller. The failure of the FULL
upgrade should be tracked so the finally reset can be skipped (avoiding a spurious
no-op) and, more importantly, so the caller can be informed that the fsync guarantee
was not obtained. Track whether the upgrade succeeded and only reset in finally when
it did.

lib/data/db.dart [1226-1316]

+var synchronousUpgraded = false;
 try {
   await db.execute('PRAGMA synchronous=FULL');
+  synchronousUpgraded = true;
 } catch (_) {
   /* durability upgrade is best-effort — NORMAL still commits correctly */
 }
 try {
   await db.transaction((txn) async {
     ...
   });
 } finally {
-  try {
-    await db.execute('PRAGMA synchronous=NORMAL');
-  } catch (_) {
-    /* non-fatal — see open-time PRAGMA discipline */
+  if (synchronousUpgraded) {
+    try {
+      await db.execute('PRAGMA synchronous=NORMAL');
+    } catch (_) {
+      /* non-fatal — see open-time PRAGMA discipline */
+    }
   }
 }
Suggestion importance[1-10]: 3

__

Why: The suggestion is logically sound but the concern is overstated. The finally block executing PRAGMA synchronous=NORMAL when already at NORMAL is a harmless no-op, and the code already documents the upgrade as "best-effort". The test file also verifies both FULL and NORMAL appear in the log, which would break with this change when the upgrade fails. The improvement is marginal.

Low

Previous suggestions

Suggestions up to commit 04a887e
CategorySuggestion                                                                                                                                    Impact
General
Reset FULL only when upgrade actually succeeded

If PRAGMA synchronous=FULL throws, the code still enters the try/finally block and
runs the transaction under NORMAL durability — but the finally then resets to NORMAL
anyway, which is correct. However, if the FULL upgrade silently fails, the
ACK-gating commit proceeds without the fsync guarantee, defeating the entire purpose
of this PR. The failure should be tracked so the finally reset is skipped (or at
minimum the transaction is still guarded), but more critically the transaction
should only run when the durability upgrade actually succeeded, or the caller should
be informed. Use a flag to track whether FULL was set, and only reset in finally if
it was actually applied.

lib/data/db.dart [1219-1309]

+var syncUpgraded = false;
 try {
   await db.execute('PRAGMA synchronous=FULL');
+  syncUpgraded = true;
 } catch (_) {
   /* durability upgrade is best-effort — NORMAL still commits correctly */
 }
 try {
   await db.transaction((txn) async {
     ...
   });
 } finally {
-  try {
-    await db.execute('PRAGMA synchronous=NORMAL');
-  } catch (_) {
-    /* non-fatal — see open-time PRAGMA discipline */
+  if (syncUpgraded) {
+    try {
+      await db.execute('PRAGMA synchronous=NORMAL');
+    } catch (_) {
+      /* non-fatal — see open-time PRAGMA discipline */
+    }
   }
 }
Suggestion importance[1-10]: 4

__

Why: The suggestion has merit in tracking whether FULL was actually set, but the PR's design explicitly treats the upgrade as best-effort and the finally reset to NORMAL is harmless even if FULL was never set (resetting NORMAL when already at NORMAL is a no-op). The improved code adds complexity without meaningful benefit since the PRAGMA synchronous=NORMAL in finally is safe regardless.

Low
Test mid-transaction failure not just pre-SQL failure

The test simulates a commit failure via a Dart-side RangeError (mismatched
raws/samples lengths), which fires before any SQL batch executes inside the
transaction. This means the test does not cover the case where PRAGMA
synchronous=FULL succeeds but the SQLite commit itself fails — the most important
failure path for the finally reset. Consider adding a test case that causes a
failure after at least one batch operation executes inside the transaction (e.g., a
duplicate primary key with ConflictAlgorithm.fail) to ensure NORMAL is restored
after a true mid-transaction failure.

test/ack_commit_sync_full_test.dart [84-97]

-// raws non-empty but samples empty → samples[i] throws RangeError INSIDE the
-// db.transaction, after FULL is set. The finally must still restore NORMAL.
-await expectLater(
-  LocalDb.commitSyncBatch([recAt(6001)], const <Sample?>[]),
-  throwsA(isA<RangeError>()),
-);
+test('synchronous is restored to NORMAL even when the commit throws mid-transaction', () async {
+  // Pre-insert a row so the second commitSyncBatch hits a real SQLite constraint
+  // failure inside the transaction (after FULL is set and SQL has executed).
+  await LocalDb.commitSyncBatch(
+    [recAt(7001)],
+    [Sample(tsEpoch: 1750007001, counter: 7001, hr: 55)],
+  );
 
+  syncStmts.clear();
+  // Inserting the same counter again with ConflictAlgorithm.fail (if exposed)
+  // or any path that throws inside db.transaction after FULL is set.
+  await expectLater(
+    LocalDb.commitSyncBatch([recAt(6001)], const <Sample?>[]),
+    throwsA(isA<RangeError>()),
+  );
+
+  expect(syncStmts, ['pragma synchronous=full', 'pragma synchronous=normal'],
+      reason: 'a thrown commit must not leak FULL');
+  expect(await restingSynchronous(), 1,
+      reason: 'FULL did not leak past the throwing commit');
+});
+
Suggestion importance[1-10]: 3

__

Why: The suggestion raises a valid point that the existing test uses a Dart-side RangeError before SQL executes, not a true SQLite failure. However, the finally block behavior is the same regardless of where the exception originates, and the improved code still uses the same RangeError path rather than a true mid-transaction SQLite failure, making it only marginally better than the existing test.

Low

@abdulsaheel

Copy link
Copy Markdown
Collaborator Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 11, 2026

Copy link
Copy Markdown
⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@abdulsaheel

Copy link
Copy Markdown
Collaborator Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 11, 2026

Copy link
Copy Markdown
⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@abdulsaheel

Copy link
Copy Markdown
Collaborator Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 11, 2026

Copy link
Copy Markdown
⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

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.
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 4928f28

@abdulsaheel

Copy link
Copy Markdown
Collaborator Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 11, 2026

Copy link
Copy Markdown
⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@abdulsaheel

Copy link
Copy Markdown
Collaborator Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 11, 2026

Copy link
Copy Markdown
⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@abdulsaheel

Copy link
Copy Markdown
Collaborator Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 11, 2026

Copy link
Copy Markdown
⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@abdulsaheel

Copy link
Copy Markdown
Collaborator Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 11, 2026

Copy link
Copy Markdown
⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@abdulsaheel

Copy link
Copy Markdown
Collaborator Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 11, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@abdulsaheel

Copy link
Copy Markdown
Collaborator Author

Superseded by #235 — consolidated into the single integration/gen4-data-integrity branch per request. Same commits, same reviews; closing to keep one PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant