Repository navigation
fix(wal): sync the WAL to disk before a commit is acknowledged - #280
CaffeinatedCoder wants to merge 1 commit into
Conversation
Every commit path awaits WriteAheadLog.FlushAsync before it acknowledges a commit and frees the pages the commit stopped using. FlushAsync called FileStream.FlushAsync, which hands the 64 KB buffer to the OS and does not fsync (.NET 6 to 10), and the stream has no WriteThrough. Since the async-first rewrite removed the synchronous Flush(flushToDisk: true), no commit synced the WAL. A power loss therefore dropped acknowledged commits while the frees those commits made, written straight to the page file, survived, so documents could come back pointing at freed or reused overflow pages. TruncateAsync had the same gap: the pages a truncation releases are freed right after it returns. FlushAsync now follows the buffer hand-off with Flush(flushToDisk: true), once per group-commit batch, and TruncateAsync syncs the truncation. The lazy checkpoint is unchanged. The sync costs a lot here: on macOS/APFS a sequential single-document commit goes from about 0.13 ms to about 4.5 ms. So there is a switch, PageFileConfig.WalSync, defaulting to WalSyncMode.Full (the behaviour above). WalSyncMode.Normal acknowledges a commit after the hand-off alone and syncs the WAL before a checkpoint copies pages to the data file, before the pages a commit stopped using are freed, and on truncation. A power loss can then lose the most recent commits but leaves a committed state, as SQLite's synchronous=NORMAL does in WAL mode. README's durability line now says which mode guarantees what. Found-By: OT Defect-Class: WAL Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
mrdevrobot
left a comment
There was a problem hiding this comment.
Changes requested. The durability gap in #260 is real, but a sync on every commit by default is not acceptable: on macOS it took commits from about 1.5 ms to about 53 ms (Flush(true) is F_FULLFSYNC there), and with the 500 ms default lock timeout 313–329 of 2,000 concurrent inserts timed out.
Keep WalSyncMode, with a default that leaves commit latency unchanged; full per-commit sync is opt-in. Independently of the mode, sync the WAL before the writes whose loss corrupts rather than drops: before a page freed by a committed transaction reaches the page file and before the WAL is truncated. Document what each mode survives (process crash vs. power loss). LockTimeout.Default stays at 500 ms (#321 is closed).
What was wrong
CommitAsyncand every auto-committing write were acknowledged once the WAL had been handed to the OS, not once it was on disk (#260).WriteAheadLog.FlushAsynccalledFileStream.FlushAsync, which does not fsync on .NET 6 to 10, and the WAL stream has noWriteThrough. Since63465aa(v4.0.0) removed the synchronousFlush(flushToDisk: true), no commit path synced the WAL.The pages a commit stopped using are freed right after that flush, straight to the page file. A power loss therefore kept the frees and dropped the commits, so documents could come back pointing at freed or reused overflow pages.
TruncateAsynchad the same gap: the pages a truncation releases are freed right after it returns.Fix
WriteAheadLog.FlushAsynchands the buffer to the OS (FlushAsync) and then callsFlush(flushToDisk: true)(fsync,FlushFileBufferson Windows). Group commit still flushes once per batch, so the sync is amortised over the batch, asd72b73aintended. The lazy checkpoint is unchanged.TruncateAsyncsyncs the truncation before returning.Cost, and the switch
I measured with a small console app against
BLite.Core(Release) on macOS 27 / Apple Silicon / APFS. Each run does 1000 sequentialInsertAsync+CommitAsynccalls on aBLiteEngineafter 50 warm-up commits, then 8 concurrent writers × 250 transactions. Two runs each:main(no sync)WalSyncMode.Full(default)WalSyncMode.NormalThat cost is large, mostly the cost of a real flush on this storage. So, as the issue's second option suggests, there is a switch:
PageFileConfig.WalSyncwithWalSyncMode.Full(the default,0, sodefault(PageFileConfig)and every preset stay durable) andWalSyncMode.Normal.Normalacknowledges a commit after the hand-off alone. It syncs the WAL before a checkpoint copies committed pages to the data file, before the pages a commit stopped using are freed (_freeOnCommitholds an entry for a transaction in the batch), and on truncation. A power loss can lose the most recent commits but leaves a committed state, never one pointing at freed pages. This matches SQLite'ssynchronous=NORMALin WAL mode.Normalmode the p99 and the mean include those syncs, from checkpoints when the WAL passesMaxWalSize.WriteAheadLogtakes the mode assyncOnFlush(constructor, defaulttrue) and gainsSyncAsync().IWriteAheadLogis unchanged, so the memory and WASM WALs are untouched.If you would rather not have the switch, drop
WalSyncMode,PageFileConfig.WalSyncandSyncDeferredWalAsync. The fix itself is the twoFlush(flushToDisk: true)lines inWriteAheadLog.Tests
WalCommitSyncTestsreplaces the WAL'sFileStreamwith a recording stream over the same handle (the seam the WAL crash test uses) and records each write, truncation, flush and sync in order:AutoCommit_Insert_Returns_Only_After_The_Wal_Is_Syncedwrite, write, write, write, flush, flushTransaction_CommitAsync_Returns_Only_After_The_Wal_Is_SyncedCheckpoint_Syncs_The_Wal_Truncation_Before_Returningset-length, flush, flushNormal_Mode_Acknowledges_A_Commit_Without_SyncingNormal_Mode_Syncs_Before_Freeing_The_Pages_A_Commit_ReleasedNormal_Mode_Syncs_Before_A_Checkpoint_Copies_PagesI checked the first three by reverting
src/toupstream/main. I checked the last two by removing each Normal-mode sync point in turn.WAL crash oracle (#263): I cherry-picked this onto
test/oracle-waland deleted the#260entry froms_candidates.Oracles.WalCrashTeststhen passes all 25 rows, includingPowerLoss / Durability,Atomicity,Pages.ReachableandContinue(default mode,Full). That test is the real guard for what survives each crash point; the unit tests above only pin the order of calls.dotnet test tests/BLite.Testsgave 3089 passed and 4 skipped. Nine tests failed under load from parallel runs (ConcurrentConsistency,BTreeSplit,BTreeRollbackFree,IndexPageRelease,StringIndexPrefixMatch,UnchangedIndexKeyUpdate, twoDocumentCollection*); all 115 tests of those classes pass when rerun on their own. The suite takes about a minute longer because commits now sync. BLite.Core builds for net10.0 and netstandard2.1 with 0 warnings, andtests/BLite.NetStandard21.Testsbuilds.Compatibility
WriteAheadLog's new constructor parameter is optional and appended at the end.Fixes #260
The WAL crash test PR (#263) lists this as a known candidate. Its
#260row ins_candidatescan go once this lands.Conflicts with other open PRs
Conflicts with #310 (write-write conflict detection, #304) in
src/BLite.Core/Storage/StorageEngine.GroupCommit.csandREADME.md. Resolution: the sync-before-free loop skips commits that #310 rejected as conflicts, and a batch whose commits were all rejected skips itsFlushAsync, so a rejected commit is never written or synced. The README keeps this PR's Durable line and #310's Isolated line. Test:WalCommitSyncTests.A_Commit_Rejected_As_A_Conflict_Writes_And_Syncs_Nothing. Either PR can land first; I will push the resolution to the second.This PR should land together with #321, the fix for #303. With a sync per batch, a commit waits for the previous batch's fsync while holding the locks, and the 500 ms
LockTimeout.Default(tuned for an unsynced WAL) fires more often.🤖 Generated with Claude Code