Skip to content

fix(storage): harden checkpoint crash recovery - #1099

Merged
adsharma merged 2 commits into
LadybugDB:mainfrom
aikins01:fix/checkpoint-recovery-hardening
Oct 5, 2026
Merged

adsharma merged 2 commits into
LadybugDB:mainfrom
aikins01:fix/checkpoint-recovery-hardening

Conversation

@aikins01

@aikins01 aikins01 commented Oct 3, 2026

Copy link
Copy Markdown
Contributor

Description

A crash inside a checkpoint could leave on-disk state that recovery
misinterpreted, and several failure windows could silently lose or corrupt
data. This change makes the WAL checkpoint record the single commit point of
the protocol: everything recovery needs (main, graph, and partition-child
shadow files, plus each child's database header and page manager) is flushed
and fsynced before the commit record is written, and once that record is
durable, a failure anywhere makes the database refuse further writes until
restart instead of rolling back over a committed checkpoint. Shadow replay is
bounded to the page extent committed in the checkpoint header, locates the
last database-header record and validates its database ID before applying
anything, and rejects any record that targets a page outside that extent.

Shadow files are stamped with the ID of the database running the checkpoint,
using a bundle sentinel for the new format, so a graph's or partition child's
shadow names the parent that owns its pending bundle. A standalone open of a
file whose shadow belongs to another database's bundle refuses to decide that
bundle's fate instead of discarding the shadow. A graph whose file is missing
while its shadow is present under a committed checkpoint fails loudly instead
of being skipped, and a graph whose active and frozen WALs both carry
checkpoint markers fails instead of half-recovering. The WAL CHECKPOINT
record is versioned: this build writes a bundle-format marker, parses records
without it as legacy v0, and rejects unknown future versions. WAL rotation
failures poison the database instead of continuing on a half-rotated WAL,
retired WALs are removed only after the checkpoint's shadow state is fully
applied, and directory renames and removals are fsynced.

Two windows that previously corrupted data on retry are fixed. A checkpoint
retried after a failed attempt used to skip hash-index header pages that the
failed attempt had already advanced past, which corrupted the header chain
and made recovery fail with "disk array header page 0"; rollback now restores
the staged header-page count so the retry rewrites the chain. And databases
written by the legacy checkpoint protocol, which applied and removed each
partition child's shadow before retiring the main WAL, used to fail recovery
when a child shadow was legitimately absent; legacy markers now treat a
missing child shadow as already applied, while bundle-format checkpoints
still require every shadow. Each window has a regression test that fails on
the old behavior.

docs/checkpoint_recovery.md documents the resulting protocol: the commit
order, what each marker means, and what recovery does for each crash point.

Types of changes

  • Bug fix
  • New feature
  • Breaking change
  • Documentation Update

Checklist

  • I have changed storage version if on disk format has changed.
    Not needed: the database file layout is unchanged. The additions are
    confined to transient recovery artifacts and are self-describing: a
    trailing ownerDatabaseID in the shadow-file header (zero reads as the
    file's own bundle) and a trailing versioned field in the WAL CHECKPOINT
    record (absent reads as legacy v0), so existing databases and older shadow
    files open unchanged.
  • I have requested a review from a maintainer.
  • I have updated the documentation (if needed).

@aikins01

aikins01 commented Oct 3, 2026

Copy link
Copy Markdown
Contributor Author

Why minimal test (linux) failed, and what changed

Both GraphShadow/LegacyMarkerUnflushedGraphShadowTest.DiscardsGraphShadow cases failed on Value::getValue<std::string>(): in an ANY graph, n.name is JSON-typed by design (ANY-graph properties live in the generic _nodes table's data column), so the string accessor asserts on a JSON Value. The assert only fires in RUNTIME_CHECKS builds, which the minimal job enables, so the mismatch stayed hidden in dev builds while the recovery behavior itself was correct (the value was Alice either way).

fd199c1 switches the assertion to Value::toString(), which is how ANY-graph property reads already surface in the E2E suite (test/test_files/graph/any.test expects RETURN u.name to give Alice). Both cases pass in a RUNTIME_CHECKS build, and the full transaction suite is 148/148 with the default config.

One pre-existing limitation I ran into while validating this

TornGraphWALHeaderTest.FollowsReplayFailureMode/NonStrict takes a different path depending on force_checkpoint_on_close. With the default true, Bob's insert is checkpointed into the graph file when the connection closes, so the final reopen reads checkpointed data. With the close checkpoint disabled, the reopen replays the main WAL instead and hits a pre-existing gap: replayUpdateSequenceRecord looks up the ANY-graph's internal _nodes_id_serial (OID 0 in the graph's catalog) through the main catalog, and the _nodes insert record has the same problem in resolveNodeTableByID, since table and sequence IDs are per-catalog. Graph catalogs are loaded before the replay loop, so the records can be routed to the owning catalog, but the replay-side lookups don't consult it yet. CI doesn't hit this because the workflow always runs with force_checkpoint_on_close=true. I left it out of this PR since it changes WAL replay behavior and deserves its own review; I'll put that up as a separate PR.

@adsharma

adsharma commented Oct 5, 2026

Copy link
Copy Markdown
Contributor

This looks great! Ready to merge. A few thoughts I'm jotting down for future reference:

  • Why have a bundle version when we already have a storage format version? The former versions the WAL, the latter versions the disk format of data files. Entirely justified.

  • Child graph formats using parent graphs database ID in the WAL records: as a part of the distributed database work, we want to bulk migrate a child partition from one parent database ID to another as we rebalance partitions. This rebalance work needs more design. I want to make sure nothing here treats the parent database ID as immutable. https://github.com/orgs/Latentpedia/discussions/4.

  • Even though this PR allows going to back to an older version of the database after replaying WAL, our stance on not allowing downgrade of storage format (once new WAL has been applied and new WAL records have been written that old versions don't understand) remains unchanged. Let me know if that was not your intention.

@adsharma
adsharma force-pushed the fix/checkpoint-recovery-hardening branch from e8253de to 7111ecd Compare October 5, 2026 16:07
@adsharma
adsharma merged commit a456025 into LadybugDB:main Oct 5, 2026
4 checks passed
@aikins01

aikins01 commented Oct 7, 2026

Copy link
Copy Markdown
Contributor Author

On the two notes:

Parent database ID: nothing in the recovery path keys a child graph to its parent's database ID. Graph WAL headers verify against the graph's own persisted ID (replayGraphWAL passes the graph's own storage manager to verifyDatabaseID), and checkpoint shadows carry the bundle sentinel and replay against the graph's own storage manager and data file. A child partition bulk-migrated to a different parent under a distributed rebalance should therefore keep recovering against its own ID, so rebalance is free to move children between parents as far as this path is concerned.

Downgrade stance: that matches the intent. The documented gate is a successful CHECKPOINT on a version 2 build, the step that empties the WAL of version 2 records, before an older build opens the database. Storage-format downgrades remain unsupported.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants