Repository navigation
fix(storage): keep the data file a checkpoint image; journal checkpoints - #6
Merged
Merged
Conversation
Crash recovery replays the WAL from the meta page's checkpoint LSN on top of whatever pages are in the data file. That replay is logical and not idempotent: once a dirty page was evicted after a checkpoint, the data file already held later changes, and replaying them again (tuple inserts, slot reuse, compaction, splits) lost writes, resurrected deleted tuples or panicked in SlottedPage.Compact. A new lost-write crash test failed 15 of 20 seeds on the previous code, with or without the per-page fsync. The data file now only ever holds a complete checkpoint image: - Evicted dirty pages go to a scratch spill file (<db>-spill): no fsync, no WAL barrier, discarded on restart. ReadPage prefers the spilled copy. - A checkpoint (BufferPoolManager.FlushAll -> DiskManager.WriteCheckpoint) writes every dirty and spilled page to a CRC-framed journal (<db>-journal), fsyncs it, copies the pages into the data file, fsyncs that, then empties the journal with truncate+fsync. On open (and before the next checkpoint) a complete journal is re-applied and an incomplete one discarded. - The WAL barrier now runs once per checkpoint instead of per page write. - WritePage and FlushPage are gone; the engine's checkpoint paths (Close, Checkpoint, CreateSnapshot, recovery) share persistCheckpointLocked. - A zero meta page means setup never completed, whatever the file length. - beastctl restore removes a stale journal beside the destination; v0 migration removes its temporary journal. This also removes the fsync per evicted page that made eviction-heavy work (TestLargeValuesReplicate: thousands of F_FULLFSYNCs) collide with the fsync-bound TestEngineSystemWorkloadBenchmark in parallel CI runs. Locally, in a full `go test ./... -race`: benchmark 790-980 -> ~1820-2070 ops/s, TestLargeValuesReplicate 17-38 s -> 7-9 s. Tests: internal/storage/simfile simulates power loss (any subset of unsynced writes and size changes lost). New tests cover checkpoint atomicity across a crash at any point, spill semantics, leftover-journal installation, and engine recovery after random crashes with a tiny pool and after checkpoints that fail mid-journal or mid-install. Both were checked to fail when evicted pages leak into the data file or pages are installed before journaling.
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 425847b48d
ℹ️ 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".
| // A zero meta page means initialization never reached its first sync | ||
| // (no write is accepted before that), so there is nothing to recover. The | ||
| // file may be longer than one page because growth is reserved in chunks. | ||
| if isAllZeros(data) { |
There was a problem hiding this comment.
If an existing database has a zeroed or corrupted page 0 while later pages still contain data, this branch closes and deletes the entire file, then creates a fresh database. A multi-page file can be corrupted without being an interrupted first initialization; if the WAL was already checkpoint-rotated, replay cannot reconstruct the earlier records, turning a detectable metadata error into permanent data loss. Restrict auto-reset to a state that proves initialization never completed; otherwise return the decode error.
Useful? React with 👍 / 👎.
Windows CI on this branch failed TestLeaderFollowerReplication: "follower missing replayed key 20" after a fixed 100 ms sleep. The benchmark passed there (2065 ops/s), but the replication tests ran about twice as slow as on main, because the new engine crash tests fsynced a real WAL file on every commit and ran in parallel with them. - Engine crash tests open their WAL without per-commit fsync (openEngine gains a syncWAL flag; production always passes true). They only simulate loss in the data and journal files and close the WAL normally, so the fsyncs added nothing but disk load. Under -race: ~34 s -> ~12 s. - Replication tests wait for the follower with a 5 s deadline instead of fixed 100/150 ms sleeps (waitUntil in wait_test.go). - That exposed a real follower bug the sleeps were hiding: the interval ACK was only checked when the next record arrived, so after fewer than ackBatchSize records followed by an idle stream the leader never learned of the applied progress. The interval ACK now runs on its own ticker, and ACKs are serialized and skipped when nothing new was applied, so the leader receives them in LSN order.
ChromaBeast
added a commit
that referenced
this pull request
Nov 13, 2026
fix(storage): keep the data file a checkpoint image; journal checkpoints
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
The data file now only ever holds a complete checkpoint image.
<db>-spill. It is never fsynced, is discarded on restart, and needs no WAL barrier.ReadPageprefers the spilled copy.BufferPoolManager.FlushAll→DiskManager.WriteCheckpoint:<db>-journal), which is fsynced.WritePageandFlushPageare removed.Close,Checkpoint,CreateSnapshotand recovery sharepersistCheckpointLocked.beastctl restoreremoves a stale journal beside the destination.docs/architecture.mdanddocs/upgrade-spec.mddescribe the new model.Why
Crash recovery could corrupt data. Recovery replays the WAL from the meta page's checkpoint LSN on top of whatever is in the data file. That replay is logical and not idempotent. Once a dirty page had been evicted after a checkpoint, the data file already held later changes. Replaying them again (tuple inserts, slot reuse, compaction, splits) lost writes, brought back deleted tuples, or panicked in
SlottedPage.Compact. A new lost-write crash test failed 15 of 20 seeds onmain, with or without the old fsync on every page write. The existing fault harness missed it because its staged crash happens after almost no evictions.It also explains the CI slowdown that PR #5 works around.
DiskManager.WritePagefsynced (F_FULLFSYNCon macOS) after every 4 KB page.TestLargeValuesReplicatepushes ~10 MB through a 50-frame pool and so issued thousands of full flushes. Under default package parallelism it overlappedTestEngineSystemWorkloadBenchmark, which is also fsync-bound, and dragged it under its floor. Under identical background load, head and the old base engine measured within noise of each other. Skipping just that test made them match (~1750–1850 ops/s). Evictions no longer fsync, so the collision goes away.Full
go test ./... -raceon one Apple-silicon Mac:TestEngineSystemWorkloadBenchmarkTestLargeValuesReplicateThese are single-machine, in-process numbers, not a comparison with other databases.
Tests
internal/storage/simfile: an in-memory file that simulates power loss. Any subset of writes since the last sync may be lost, and so may size changes.TestCheckpointIsAtomicAcrossCrash: 168 cases, failing the journal or the data install at each point, then crashing. Every reopen yields the whole old image or the whole new one.TestSpilledPagesAreScratchOnly: spilling costs no fsync and never reaches the data file.TestFailedCheckpointJournalIsInstalledFirst: a leftover journal is installed before the next checkpoint.TestCrashLosingUnsyncedPageWritesRecovers: random workloads through an 8-frame pool with repeated crashes.TestCheckpointCrashMidFlushKeepsCommittedData: a checkpoint fails mid-journal or mid-install, followed by a crash, or by more writes and another checkpoint.go build ./... && go vet ./... && go test ./... -racepasses (run twice).go vetis also clean withGOOS=windowsandGOOS=linux.Things a reviewer should know
-p 1workaround in ci: run Go test packages serially (fix Windows throughput failure) #5 should no longer be needed. The 1000 ops/s floor is unchanged.<db>-journalstays, empty, between checkpoints.<db>-spillis removed on clean close. Backups copy only the data file, written right after a checkpoint, so they stay consistent.internal/apiunder-race. The whole suite still finishes faster, because the replication package dropped from ~43 s to ~11 s.Second commit: Windows CI follow-up
On the first Windows run, the benchmark passed (2065 ops/s, against 314–398 before), but
TestLeaderFollowerReplicationfailed after a fixed 100 ms sleep. The replication tests ran about twice as slow because the new crash tests fsynced a real WAL file on every commit, in parallel with them.-race: about 34 s → 12 s.