From e40f32c331b69ec49baebf5c5215dcfeee5e93fa Mon Sep 17 00:00:00 2001 From: Darek Date: Sat, 24 Oct 2026 16:39:01 +0200 Subject: [PATCH 1/4] fix(storage): recover pinned checkpoints that a crash interrupts A checkpoint taken while a read-only transaction is open keeps its CHECKPOINT record in the active WAL. Recovery treated a shadow file left next to such a WAL as a checkpoint in progress: it replayed only the records before the first CHECKPOINT (applying them twice and dropping every later commit), replayed the next checkpoint's unfinished shadow, or required partition shadows that cleanup had already removed. - A whole transaction after the last CHECKPOINT record proves that checkpoint completed. Recovery then skips through the record and removes the leftover shadow, in the active and the frozen WAL. Other records after a CHECKPOINT keep the prefix replay. - The pinned-checkpoint marker now means that every CHECKPOINT record in the active WAL is applied: WAL::logAndFlushCheckpoint removes it before logging a record, and pinned cleanup writes it (content 2) before removing shadows. With it, a WAL ending in a CHECKPOINT discards leftover main and partition shadows instead of replaying or requiring them. - A WAL with nothing committed after its last CHECKPOINT is recovered as one ending in it, so it is retired instead of kept. - A marker from an older build (content 1) only proves that a missing shadow was applied. Named graphs are not covered: pinned checkpoints keep their own WALs too. Co-Authored-By: Claude Opus 5.5 --- docs/checkpoint_recovery.md | 21 ++ src/include/storage/checkpointer.h | 2 + src/include/storage/storage_utils.h | 18 +- src/include/storage/wal/wal.h | 7 + src/include/storage/wal/wal_replayer.h | 12 +- src/storage/checkpointer.cpp | 35 +-- src/storage/wal/wal.cpp | 27 ++ src/storage/wal/wal_replayer.cpp | 177 +++++++------ test/transaction/checkpoint_test.cpp | 345 +++++++++++++++++++++++++ 9 files changed, 530 insertions(+), 114 deletions(-) diff --git a/docs/checkpoint_recovery.md b/docs/checkpoint_recovery.md index bed775f34d..e82bc7a136 100644 --- a/docs/checkpoint_recovery.md +++ b/docs/checkpoint_recovery.md @@ -32,6 +32,27 @@ reached the OS — but power-loss durability of those directory entries is not g If a step fails after the commit point, the database refuses further writes until it is reopened, and recovery finishes the checkpoint. +## Pinned checkpoints + +A checkpoint taken while a read-only transaction is open keeps the WAL that transaction may still +need: it does not rotate the WAL, so its `CHECKPOINT` record stays in the active WAL and later +commits follow it. Its cleanup writes the marker file `.pinned.tmp` before removing any shadow +file, and every `CHECKPOINT` record logged into the active WAL first removes the marker. The marker +therefore means that every `CHECKPOINT` record in the active WAL has been applied. + +A transaction committed after a `CHECKPOINT` record proves that checkpoint completed, since the +write gate is held until a checkpoint's cleanup ends and a failure after its record poisons the +WAL. Recovery then replays only the records after the last `CHECKPOINT` and removes a remaining +shadow file, which belongs to a later checkpoint that never logged its record. Records after a +`CHECKPOINT` that form no whole transaction leave the earlier rule in place: with a shadow file, +only the records before the first `CHECKPOINT` replay. + +When nothing was committed after the last `CHECKPOINT` record, the marker decides. With it, +recovery discards the shadow files of the main database and partition children. Without it, a +remaining shadow is replayed as the committed checkpoint's, and a missing one is an error. A marker +written by an older build holds `1` instead of `2` and only proves that a missing shadow was +applied. Named graphs still recover through their own shadow files. + ## Checkpoint bundle format `CheckpointRecord::bundleFormatVersion` is 2 for checkpoints written by current builds, including diff --git a/src/include/storage/checkpointer.h b/src/include/storage/checkpointer.h index fa829e9881..088bf0d454 100644 --- a/src/include/storage/checkpointer.h +++ b/src/include/storage/checkpointer.h @@ -81,6 +81,8 @@ class Checkpointer { // Runs only when WAL retirement actually executes; skipped alongside retirement while // the horizon pins the WAL. virtual void onWALRetired(bool walRotated); + // Runs after the pinned-checkpoint marker is written, before cleanup removes any shadow file. + virtual void beforeShadowRemoval(); virtual void beforeGraphShadowApply(StorageManager& storageManager); void applyShadowPagesForPartitionChildren(); void flushGraphShadowFiles(); diff --git a/src/include/storage/storage_utils.h b/src/include/storage/storage_utils.h index 22daa83790..140da1faf4 100644 --- a/src/include/storage/storage_utils.h +++ b/src/include/storage/storage_utils.h @@ -77,15 +77,21 @@ class StorageUtils { static std::string getShadowFilePath(const std::string& path) { return std::format("{}.{}", path, common::StorageConstants::SHADOWING_SUFFIX); } - // Marker proving a pinned checkpoint (CHECKPOINT record in the active WAL with no WAL - // rotation) completed and its shadow was applied before cleanup removed the shadow. - // Recovery only treats a trailing/mid-file active-WAL CHECKPOINT with no shadow as - // durable when this marker exists; otherwise a missing shadow is an error, never a - // silent WAL delete (out-of-band shadow loss must stay loud). Uses a `.tmp` suffix so - // the file-deletion allowlist permits cleanup. + // Marker proving that every CHECKPOINT record in the active WAL (left there by pinned + // checkpoints, which skip WAL rotation) has been applied: WAL::logAndFlushCheckpoint removes + // it before logging a record, and a pinned checkpoint's cleanup writes it again after + // applying its shadows, before removing them. With the marker, any shadow file recovery finds + // belongs to a later checkpoint that never logged its record, or to an interrupted cleanup. + // A marker from an older build (see below) only proves that a missing shadow was applied. + // Without any marker, CHECKPOINT records with no shadow are an error, never a silent WAL + // delete (out-of-band shadow loss must stay loud). Uses a `.tmp` suffix so the file-deletion + // allowlist permits cleanup. static std::string getPinnedCheckpointMarkerPath(const std::string& path) { return std::format("{}.pinned.tmp", path); } + // Content of the marker above. Older builds wrote 1 and kept the marker while logging a new + // CHECKPOINT record, so their marker does not prove a trailing record was applied. + static constexpr uint8_t PINNED_CHECKPOINT_MARKER_APPLIED = 2; static std::string getCheckpointIntentLockFilePath(const std::string& path) { return std::format("{}.checkpoint.intent.lock", path); } diff --git a/src/include/storage/wal/wal.h b/src/include/storage/wal/wal.h index c0aaebaa42..65b208612e 100644 --- a/src/include/storage/wal/wal.h +++ b/src/include/storage/wal/wal.h @@ -39,6 +39,8 @@ class WAL { void logCommittedWAL(LocalWAL& localWAL, main::ClientContext* context, uint64_t& commitSequence); + // Removes the pinned-checkpoint marker first, so it never vouches for a record whose shadow is + // not applied yet. void logAndFlushCheckpoint(main::ClientContext* context); // Renames the active WAL to the frozen checkpoint WAL. Returns false if there is nothing to @@ -62,6 +64,9 @@ class WAL { // source for WAL pin observability alongside getFileSize() (active). uint64_t getFrozenWALFileSize(); bool mayHaveCheckpointRecord(); + // See StorageUtils::getPinnedCheckpointMarkerPath. + void writePinnedCheckpointMarker(main::ClientContext* context); + void removePinnedCheckpointMarker(main::ClientContext* context); void throwIfPoisoned(); void poison(const std::string& reason); @@ -76,11 +81,13 @@ class WAL { void waitForDurabilityNoLock(uint64_t commitSequence, std::unique_lock& lck); void flushAndSyncNoLock(); void writeHeader(main::ClientContext& context); + void removePinnedCheckpointMarkerNoLock(main::ClientContext* context); private: std::mutex mtx; std::string walPath; std::string checkpointWalPath; + std::string pinnedCheckpointMarkerPath; bool inMemory; [[maybe_unused]] bool readOnly; common::VirtualFileSystem* vfs; diff --git a/src/include/storage/wal/wal_replayer.h b/src/include/storage/wal/wal_replayer.h index 69c8f2a7b4..c6de662d52 100644 --- a/src/include/storage/wal/wal_replayer.h +++ b/src/include/storage/wal/wal_replayer.h @@ -51,15 +51,15 @@ class WALReplayer { bool isLastRecordCheckpoint = false; // End offset of the last CHECKPOINT record, or 0 when the file holds none. A // pinned checkpoint skips WAL rotation, so its record can sit mid-file (with - // later commits after it); only the suffix past this offset replays, and only - // when no shadow file remains (see below). + // later commits after it); usually only the suffix past this offset replays. uint64_t lastCheckpointOffset = 0; // offsetDeserialized as of just before the first CHECKPOINT record (meaningful - // only when lastCheckpointOffset != 0). When a shadow file remains, some - // checkpoint may never have completed, so recovery replays exactly the - // pre-checkpoint prefix ending here and discards everything from the first - // CHECKPOINT record onward. + // only when lastCheckpointOffset != 0). A WAL whose CHECKPOINT records are not + // shown to be complete replays exactly the prefix ending here. uint64_t offsetBeforeFirstCheckpoint = 0; + // A BEGIN..COMMIT transaction follows the last CHECKPOINT record, which proves every + // CHECKPOINT in the file completed (see WALReplayer::replayActiveWAL). + bool hasTransactionAfterLastCheckpoint = false; common::uuid walDatabaseID{}; uint64_t checkpointFormatVersion = 0; }; diff --git a/src/storage/checkpointer.cpp b/src/storage/checkpointer.cpp index 7f6e9e1818..c2c5698758 100644 --- a/src/storage/checkpointer.cpp +++ b/src/storage/checkpointer.cpp @@ -443,6 +443,13 @@ void Checkpointer::postCheckpointCleanup(bool canResetPageManagerToCurrent) try } onWALRetired(mainWALRotated); } + // A pinned checkpoint keeps its CHECKPOINT record in the active WAL. The marker must be + // durable before any shadow is removed: recovery then discards the shadows instead of + // replaying a half-removed one. + if (pinWALRetirement) { + mainStorageManager->getWAL().writePinnedCheckpointMarker(&clientContext); + } + beforeShadowRemoval(); for (const auto& target : checkpointTargets) { target.storageManager->getShadowFile().clear(*bufferManager, false); target.storageManager->getShadowFile().reset(); @@ -458,29 +465,9 @@ void Checkpointer::postCheckpointCleanup(bool canResetPageManagerToCurrent) try common::VirtualFileSystem::GetUnsafe(clientContext) ->syncParentDirectory( StorageUtils::getShadowFilePath(mainStorageManager->getDatabasePath())); - // Durability marker for pinned checkpoints: the active WAL holds CHECKPOINT records - // with no rotation, and cleanup above removed the shadow. Recovery treats such - // CHECKPOINTs as durable only when this marker exists; a missing shadow without the - // marker is an out-of-band loss and must throw, never silently delete the WAL. - if (!isInMemory) { - auto* vfs = common::VirtualFileSystem::GetUnsafe(clientContext); - const auto markerPath = - StorageUtils::getPinnedCheckpointMarkerPath(mainStorageManager->getDatabasePath()); - if (pinWALRetirement) { - auto markerFile = vfs->openFile(markerPath, - common::FileOpenFlags( - common::FileFlags::WRITE | common::FileFlags::CREATE_IF_NOT_EXISTS), - &clientContext); - const uint8_t done = 1; - markerFile->writeFile(&done, 1, 0); - markerFile->truncate(1); - markerFile->syncFile(); - markerFile.reset(); - vfs->syncParentDirectory(markerPath); - } else { - vfs->removeFileIfExists(markerPath, &clientContext); - vfs->syncParentDirectory(markerPath); - } + // An unpinned checkpoint retired the WAL above, so no CHECKPOINT record is left to vouch for. + if (!pinWALRetirement) { + mainStorageManager->getWAL().removePinnedCheckpointMarker(&clientContext); } bufferManager->removeEvictedCandidates(); releaseCheckpointLocks(); @@ -576,6 +563,8 @@ void Checkpointer::beforeWALRetirement(bool) {} void Checkpointer::onWALRetired(bool) {} +void Checkpointer::beforeShadowRemoval() {} + void Checkpointer::beforeGraphShadowApply(StorageManager&) {} void Checkpointer::logCheckpointAndApplyShadowPages(bool walRotated_) { diff --git a/src/storage/wal/wal.cpp b/src/storage/wal/wal.cpp index a4328e5e2f..8356678406 100644 --- a/src/storage/wal/wal.cpp +++ b/src/storage/wal/wal.cpp @@ -33,6 +33,7 @@ WAL::WAL(const std::string& dbPath, bool readOnly, bool enableChecksums, Virtual StorageManager* storageManager) : walPath{StorageUtils::getWALFilePath(dbPath)}, checkpointWalPath{StorageUtils::getCheckpointWALFilePath(dbPath)}, + pinnedCheckpointMarkerPath{StorageUtils::getPinnedCheckpointMarkerPath(dbPath)}, inMemory{main::DBConfig::isDBPathInMemory(dbPath)}, readOnly{readOnly}, vfs{vfs}, storageManager{storageManager}, enableChecksums(enableChecksums) {} @@ -56,6 +57,7 @@ void WAL::logCommittedWAL(LocalWAL& localWAL, main::ClientContext* context, void WAL::logAndFlushCheckpoint(main::ClientContext* context) { std::unique_lock lck{mtx}; throwIfPoisonedNoLock(); + removePinnedCheckpointMarkerNoLock(context); initWriter(context); CheckpointRecord walRecord{CHECKPOINT_BUNDLE_FORMAT_VERSION}; checkpointRecordMayExist = true; @@ -362,6 +364,31 @@ bool WAL::mayHaveCheckpointRecord() { return checkpointRecordMayExist; } +void WAL::writePinnedCheckpointMarker(main::ClientContext* context) { + std::unique_lock lck{mtx}; + auto markerFile = vfs->openFile(pinnedCheckpointMarkerPath, + FileOpenFlags(FileFlags::WRITE | FileFlags::CREATE_IF_NOT_EXISTS), context); + const uint8_t applied = StorageUtils::PINNED_CHECKPOINT_MARKER_APPLIED; + markerFile->writeFile(&applied, 1, 0); + markerFile->truncate(1); + markerFile->syncFile(); + markerFile.reset(); + vfs->syncParentDirectory(pinnedCheckpointMarkerPath); +} + +void WAL::removePinnedCheckpointMarker(main::ClientContext* context) { + std::unique_lock lck{mtx}; + removePinnedCheckpointMarkerNoLock(context); +} + +void WAL::removePinnedCheckpointMarkerNoLock(main::ClientContext* context) { + if (!vfs->fileOrPathExists(pinnedCheckpointMarkerPath, context)) { + return; + } + vfs->removeFileIfExists(pinnedCheckpointMarkerPath, context); + vfs->syncParentDirectory(pinnedCheckpointMarkerPath); +} + void WAL::throwIfPoisoned() { std::unique_lock lck{mtx}; throwIfPoisonedNoLock(); diff --git a/src/storage/wal/wal_replayer.cpp b/src/storage/wal/wal_replayer.cpp index c924a7824a..9445435068 100644 --- a/src/storage/wal/wal_replayer.cpp +++ b/src/storage/wal/wal_replayer.cpp @@ -73,8 +73,27 @@ class ReplayOwnerScope { main::DatabaseManager& dbManager; catalog::Catalog* previous; }; +enum class PinnedCheckpointMarker : uint8_t { NONE, LEGACY, APPLIED }; } // namespace +// See StorageUtils::getPinnedCheckpointMarkerPath. +static PinnedCheckpointMarker readPinnedCheckpointMarker(main::ClientContext& clientContext) { + auto* vfs = VirtualFileSystem::GetUnsafe(clientContext); + const auto markerPath = + StorageUtils::getPinnedCheckpointMarkerPath(clientContext.getDatabasePath()); + if (!vfs->fileOrPathExists(markerPath, &clientContext)) { + return PinnedCheckpointMarker::NONE; + } + auto fileInfo = vfs->openFile(markerPath, FileOpenFlags(FileFlags::READ_ONLY), &clientContext); + uint8_t content = 0; + if (fileInfo->getFileSize() == 1) { + fileInfo->readFromFile(&content, 1, 0); + } + return content == StorageUtils::PINNED_CHECKPOINT_MARKER_APPLIED ? + PinnedCheckpointMarker::APPLIED : + PinnedCheckpointMarker::LEGACY; +} + static void removePartitionChildShadowFiles(main::ClientContext& clientContext) { auto* dbManager = main::DatabaseManager::Get(clientContext); if (dbManager == nullptr) { @@ -623,16 +642,13 @@ void WALReplayer::replayFrozenWAL(Checkpointer& checkpointer, bool throwOnWalRep throwIfReadOnlyCheckpointState(clientContext, true /* hasFrozenWAL */, shadowFilePath); replayCommittedCheckpoint(checkpointer, fileInfo, checkpointWalPath, replayInfo); } else { - // A missing shadow proves every CHECKPOINT in the file completed (each - // cleanup clears it only after the checkpoint is applied and synced), so the - // pre-checkpoint prefix is already durable and only the suffix after the - // last CHECKPOINT replays. A remaining shadow means some checkpoint may never - // have completed: replay the pre-checkpoint prefix exactly as before and - // discard everything from the first CHECKPOINT record onward. Check before - // deleting below. - const bool discardCheckpointPrefix = + // See replayActiveWAL. A record after a CHECKPOINT reaches a frozen WAL only when the + // active WAL rotated with records of pinned checkpoints in it; nothing is appended + // after the frozen WAL's own CHECKPOINT. Check before deleting below. + const bool skipThroughLastCheckpoint = !VirtualFileSystem::GetUnsafe(clientContext) - ->fileOrPathExists(shadowFilePath, &clientContext); + ->fileOrPathExists(shadowFilePath, &clientContext) || + replayInfo.hasTransactionAfterLastCheckpoint; removeFileAndSyncParentDirectory(shadowFilePath); checkpointer.readCheckpoint(); removePartitionChildShadowFiles(clientContext); @@ -646,15 +662,13 @@ void WALReplayer::replayFrozenWAL(Checkpointer& checkpointer, bool throwOnWalRep walHeader.databaseID); deserializer.getReader()->onObjectEnd(); } - // Without a shadow the prefix is durable (skip it); with one, stop replay at - // the first CHECKPOINT record, whose checkpoint may be incomplete. Both - // bounds exclude every CHECKPOINT record structurally, so none ever reaches + // Both bounds exclude every CHECKPOINT record structurally, so none ever reaches // replayWALRecord, which rejects them. const auto replayEnd = - !discardCheckpointPrefix && replayInfo.lastCheckpointOffset != 0 ? + !skipThroughLastCheckpoint && replayInfo.lastCheckpointOffset != 0 ? replayInfo.offsetBeforeFirstCheckpoint : offsetDeserialized; - const auto skipBefore = discardCheckpointPrefix ? replayInfo.lastCheckpointOffset : 0; + const auto skipBefore = skipThroughLastCheckpoint ? replayInfo.lastCheckpointOffset : 0; while (getReadOffset(deserializer, enableChecksums) < replayEnd) { DASSERT(!deserializer.finished()); auto walRecord = WALRecord::deserialize(deserializer, clientContext); @@ -683,8 +697,7 @@ void WALReplayer::replayFrozenWAL(Checkpointer& checkpointer, bool throwOnWalRep // goes to the frozen WAL, so a crash at any point either replays the frozen WAL again // or applies the checkpoint, and the active WAL is replayed on top in both cases. // Drop any torn tail first, so the CHECKPOINT record directly follows the last commit. - // With a shadowed (possibly incomplete) checkpoint this also drops the - // CHECKPOINT record onward, matching the replay bound above. + // When replay stopped at the first CHECKPOINT record, this also drops it onward. truncateWALFile(*fileInfo, replayEnd); fileInfo.reset(); completeInterruptedCheckpoint(); @@ -727,40 +740,49 @@ void WALReplayer::replayActiveWAL(Checkpointer& checkpointer, bool throwOnWalRep try { const auto replayInfo = dryReplay(*fileInfo, throwOnWalReplayFailure, enableChecksums); const auto offsetDeserialized = replayInfo.offsetDeserialized; - if (replayInfo.isLastRecordCheckpoint) { + // Records after the last CHECKPOINT that no COMMIT ends are a torn tail. Treating such a + // WAL as ending in that record retires it below instead of keeping it, so a later + // checkpoint never rotates a WAL that ends in an applied CHECKPOINT into the frozen WAL. + const bool endsInCheckpoint = replayInfo.lastCheckpointOffset != 0 && + offsetDeserialized == replayInfo.lastCheckpointOffset; + if (endsInCheckpoint) { throwIfReadOnlyCheckpointState(clientContext, true /* hasFrozenWAL */, shadowFilePath); - auto vfs = VirtualFileSystem::GetUnsafe(clientContext); - if (!vfs->fileOrPathExists(shadowFilePath, &clientContext)) { + auto* vfs = VirtualFileSystem::GetUnsafe(clientContext); + // With the marker the trailing CHECKPOINT is applied, so a remaining shadow belongs + // to a later checkpoint that never logged its record, or to an interrupted cleanup. + if (!vfs->fileOrPathExists(shadowFilePath, &clientContext) || + readPinnedCheckpointMarker(clientContext) == PinnedCheckpointMarker::APPLIED) { replayPinnedActiveCheckpoint(checkpointer, fileInfo, replayInfo); } else { replayCommittedCheckpoint(checkpointer, fileInfo, walPath, replayInfo); } } else { - // A missing shadow proves every CHECKPOINT in the file completed only when the - // pinned-checkpoint marker proves it: cleanup clears the shadow only after the - // checkpoint is applied and synced, and writes the marker in the same cleanup. - // With no shadow and pinned CHECKPOINT records but no marker, the shadow was - // lost out of band and the prefix may not be durable: throw instead of silently - // deleting committed records. A remaining shadow means some checkpoint may never - // have completed: replay the pre-checkpoint prefix exactly as before and - // discard everything from the first CHECKPOINT record onward. Check before - // deleting below. - auto* vfsNoShadow = VirtualFileSystem::GetUnsafe(clientContext); - const bool noShadow = !vfsNoShadow->fileOrPathExists(shadowFilePath, &clientContext); - if (noShadow && replayInfo.lastCheckpointOffset != 0) { - const auto markerPath = - StorageUtils::getPinnedCheckpointMarkerPath(clientContext.getDatabasePath()); - if (!vfsNoShadow->fileOrPathExists(markerPath, &clientContext)) { - throw RuntimeException( - std::format("Cannot recover WAL {}: it holds CHECKPOINT records but its " - "shadow file {} is missing " - "with no pinned-checkpoint marker. Do not delete the WAL or " - "shadow files; they hold " - "committed pages. Restore both files they were written for.", - walPath, shadowFilePath)); - } + // A transaction committed after a CHECKPOINT record proves that checkpoint completed: + // it holds the write gate until its cleanup is done, and a failure after logging the + // record poisons the WAL before anything else can be appended. So does a missing + // shadow, which cleanup removes only after applying it. Then the records up to the + // last CHECKPOINT are in the data file and only the suffix replays; a remaining + // shadow belongs to a later checkpoint that never logged its record. Records after a + // CHECKPOINT that form no whole transaction are not ones the engine writes (it logs a + // COMMIT only after a BEGIN), so the checkpoint may not have completed: replay the + // prefix before the first CHECKPOINT record and discard everything from it onward. + // Cleanup writes the pinned-checkpoint marker before removing the shadow, so with + // neither of them the shadow was lost out of band: throw instead of silently deleting + // committed records. Check before deleting below. + auto* vfs = VirtualFileSystem::GetUnsafe(clientContext); + const bool shadowExists = vfs->fileOrPathExists(shadowFilePath, &clientContext); + const bool skipThroughLastCheckpoint = + !shadowExists || replayInfo.hasTransactionAfterLastCheckpoint; + if (replayInfo.lastCheckpointOffset != 0 && !shadowExists && + readPinnedCheckpointMarker(clientContext) == PinnedCheckpointMarker::NONE) { + throw RuntimeException( + std::format("Cannot recover WAL {}: it holds CHECKPOINT records but its " + "shadow file {} is missing " + "with no pinned-checkpoint marker. Do not delete the WAL or " + "shadow files; they hold " + "committed pages. Restore both files they were written for.", + walPath, shadowFilePath)); } - const bool discardCheckpointPrefix = noShadow; removeFileAndSyncParentDirectory(shadowFilePath); if (!checkpointRead) { checkpointer.readCheckpoint(); @@ -776,15 +798,13 @@ void WALReplayer::replayActiveWAL(Checkpointer& checkpointer, bool throwOnWalRep walHeader.databaseID); deserializer.getReader()->onObjectEnd(); } - // Without a shadow the prefix is durable (skip it); with one, stop replay at - // the first CHECKPOINT record, whose checkpoint may be incomplete. Both - // bounds exclude every CHECKPOINT record structurally, so none ever reaches + // Both bounds exclude every CHECKPOINT record structurally, so none ever reaches // replayWALRecord, which rejects them. const auto replayEnd = - !discardCheckpointPrefix && replayInfo.lastCheckpointOffset != 0 ? + !skipThroughLastCheckpoint && replayInfo.lastCheckpointOffset != 0 ? replayInfo.offsetBeforeFirstCheckpoint : offsetDeserialized; - const auto skipBefore = discardCheckpointPrefix ? replayInfo.lastCheckpointOffset : 0; + const auto skipBefore = skipThroughLastCheckpoint ? replayInfo.lastCheckpointOffset : 0; while (getReadOffset(deserializer, enableChecksums) < replayEnd) { DASSERT(!deserializer.finished()); auto walRecord = WALRecord::deserialize(deserializer, clientContext); @@ -801,9 +821,7 @@ void WALReplayer::replayActiveWAL(Checkpointer& checkpointer, bool throwOnWalRep cachedOwnerName.clear(); cachedOwnerCatalog = nullptr; recoverGraphCheckpoints(clientContext, false, false); - // With a shadowed (possibly incomplete) checkpoint this drops the CHECKPOINT - // record onward, matching the replay bound above; otherwise it only drops a - // torn tail. + // Drops a torn tail, and the CHECKPOINT record onward when replay stopped before it. truncateWALFile(*fileInfo, replayEnd); } } catch (const std::exception&) { @@ -818,29 +836,23 @@ void WALReplayer::replayActiveWAL(Checkpointer& checkpointer, bool throwOnWalRep void WALReplayer::replayPinnedActiveCheckpoint(Checkpointer& checkpointer, std::unique_ptr& fileInfo, const WALReplayInfo& replayInfo) const { // A checkpoint taken while a reader pins the WAL skips rotation, so its CHECKPOINT record - // lands in the active WAL while cleanup still removes the already-applied shadow file and - // writes the pinned-checkpoint marker in the same cleanup. A trailing CHECKPOINT with no - // shadow is durable only when that marker exists: without it the shadow was lost out of - // band and the prefix may not be durable, so throw instead of silently deleting the WAL. - // Only the active-WAL path uses this; a frozen WAL with no shadow keeps throwing in - // replayCommittedCheckpoint. + // lands in the active WAL and its cleanup writes the pinned-checkpoint marker before removing + // the applied shadows. The trailing CHECKPOINT is therefore applied, and the main and + // partition children's shadows left are discarded below; named graphs still recover through + // their own shadows. Without the marker the shadow was lost out of band and the prefix may + // not be durable, so throw instead of silently deleting the WAL. Only the active-WAL path + // uses this; a frozen WAL with no shadow keeps throwing in replayCommittedCheckpoint. if (replayInfo.checkpointFormatVersion > WAL::CHECKPOINT_BUNDLE_FORMAT_VERSION) { throw RuntimeException(std::format("Cannot recover checkpoint WAL {}: {}", walPath, unsupportedCheckpointFormatMessage(replayInfo.checkpointFormatVersion))); } - { - auto* vfs = VirtualFileSystem::GetUnsafe(clientContext); - const auto markerPath = - StorageUtils::getPinnedCheckpointMarkerPath(clientContext.getDatabasePath()); - if (!vfs->fileOrPathExists(shadowFilePath, &clientContext) && - !vfs->fileOrPathExists(markerPath, &clientContext)) { - throw RuntimeException(std::format("Cannot recover checkpoint WAL {}: shadow file {} " - "is missing with no pinned-checkpoint " - "marker. Do not delete the WAL or shadow files; " - "they hold committed pages. Restore the " - "database file they were written for.", - walPath, shadowFilePath)); - } + if (readPinnedCheckpointMarker(clientContext) == PinnedCheckpointMarker::NONE) { + throw RuntimeException(std::format("Cannot recover checkpoint WAL {}: shadow file {} " + "is missing with no pinned-checkpoint " + "marker. Do not delete the WAL or shadow files; " + "they hold committed pages. Restore the " + "database file they were written for.", + walPath, shadowFilePath)); } auto* mainStorageManager = clientContext.getDatabase()->getStorageManager(); if (mainStorageManager->getDataFH() == nullptr) { @@ -863,7 +875,7 @@ void WALReplayer::replayPinnedActiveCheckpoint(Checkpointer& checkpointer, if (checkpointBundle) { checkpointDatabaseID = ShadowFile::CHECKPOINT_BUNDLE_DATABASE_ID; } - checkpointer.readCheckpoint(true, checkpointDatabaseID, checkpointBundle); + checkpointer.readCheckpoint(); recoverGraphCheckpoints(clientContext, true, checkpointBundle, checkpointDatabaseID); truncateWALFile(*fileInfo, 0); @@ -938,6 +950,7 @@ WALReplayer::WALReplayInfo WALReplayer::dryReplay(FileInfo& fileInfo, bool throw replayInfo.walDatabaseID = walHeader.databaseID; deserializer.getReader()->onObjectEnd(); + bool beganAfterLastCheckpoint = false; bool finishedDeserializing = deserializer.finished(); while (!finishedDeserializing) { auto walRecord = WALRecord::deserialize(deserializer, clientContext); @@ -946,14 +959,11 @@ WALReplayer::WALReplayInfo WALReplayer::dryReplay(FileInfo& fileInfo, bool throw case WALRecordType::CHECKPOINT_RECORD: { // A CHECKPOINT record need not be final: a pinned checkpoint skips WAL // rotation, so its record lands mid-file in the active WAL (and - // consecutive pins leave several). When no shadow file remains, every - // checkpoint in the file completed, so records up to and including the - // last one are already durable and only the suffix after it replays. - // When a shadow remains, some checkpoint may never have completed, so - // recovery instead replays the pre-checkpoint prefix (see the replay - // loops below). Remember the offsets and keep scanning instead of - // failing; a later COMMIT (or any other record) clears the - // last-record flag below. + // consecutive pins leave several). Records up to and including the last one + // are usually durable and only the suffix after it replays; a frozen WAL may + // instead replay the prefix before the first one (see the replay loops). + // Remember the offsets and keep scanning instead of failing; a later COMMIT + // (or any other record) clears the last-record flag below. replayInfo.isLastRecordCheckpoint = true; replayInfo.checkpointFormatVersion = walRecord->constCast().bundleFormatVersion; @@ -962,11 +972,20 @@ WALReplayer::WALReplayInfo WALReplayer::dryReplay(FileInfo& fileInfo, bool throw } replayInfo.lastCheckpointOffset = getReadOffset(deserializer, enableChecksums); replayInfo.offsetDeserialized = replayInfo.lastCheckpointOffset; + replayInfo.hasTransactionAfterLastCheckpoint = false; + beganAfterLastCheckpoint = false; + } break; + case WALRecordType::BEGIN_TRANSACTION_RECORD: { + replayInfo.isLastRecordCheckpoint = false; + beganAfterLastCheckpoint = true; } break; case WALRecordType::COMMIT_RECORD: { // Update the offset to the end of the last commit record. replayInfo.isLastRecordCheckpoint = false; replayInfo.offsetDeserialized = getReadOffset(deserializer, enableChecksums); + if (beganAfterLastCheckpoint && replayInfo.lastCheckpointOffset != 0) { + replayInfo.hasTransactionAfterLastCheckpoint = true; + } } break; default: { // Any other record after a checkpoint also ends the checkpoint suffix. diff --git a/test/transaction/checkpoint_test.cpp b/test/transaction/checkpoint_test.cpp index 269af81455..3a173169a5 100644 --- a/test/transaction/checkpoint_test.cpp +++ b/test/transaction/checkpoint_test.cpp @@ -5633,6 +5633,351 @@ TEST_F(ReviewFixesTest, ConsecutivePinnedCheckpointsRecoverAfterCrash) { } #endif // __SINGLE_THREADED__ +// A checkpoint taken while a read-only transaction is open (pinned) keeps its CHECKPOINT record +// in the active WAL. Each test below crashes after such a checkpoint, leaving a different shadow +// file behind, and reopens: every commit must be there exactly once. +class PinnedCheckpointCrashTest : public FlakyCheckpointerTest { +protected: + void SetUp() override { + FlakyCheckpointerTest::SetUp(); + if (inMemMode || systemConfig->checkpointThreshold == 0) { + GTEST_SKIP(); + } + // Tearing down without a closing checkpoint models the process dying. + ASSERT_TRUE(conn->query("CALL force_checkpoint_on_close=false;")->isSuccess()); + ASSERT_TRUE(conn->query("CALL auto_checkpoint=false;")->isSuccess()); + } + + void TearDown() override { + reader.reset(); + FlakyCheckpointerTest::TearDown(); + } + + void query(const std::string& statement) { + auto res = conn->query(statement); + ASSERT_TRUE(res->isSuccess()) << statement << ": " << res->getErrorMessage(); + } + + void createRows(int64_t first, int64_t last) { + query(std::format("UNWIND range({}, {}) AS i CREATE (:P {{id: i}});", first, last)); + } + + void pinWAL() { + reader = std::make_unique(database.get()); + ASSERT_TRUE(reader->query("BEGIN TRANSACTION READ ONLY;")->isSuccess()); + } + + template + void failCheckpoint() { + auto* context = getClientContext(*conn); + FlakyCheckpointer flakyCheckpointer( + [](main::ClientContext& clientContext) { return std::make_unique(clientContext); }); + flakyCheckpointer.setCheckpointer(*context); + auto res = conn->query("CHECKPOINT;"); + FlakyCheckpointer::resetCheckpointer(*context); + ASSERT_FALSE(res->isSuccess()); + } + + void crash() { + reader.reset(); + conn.reset(); + database.reset(); + } + + std::vector ids(const std::string& table = "P") { + auto res = conn->query(std::format("MATCH (p:{}) RETURN p.id ORDER BY p.id;", table)); + EXPECT_TRUE(res->isSuccess()) << res->getErrorMessage(); + std::vector result; + while (res->isSuccess() && res->hasNext()) { + result.push_back(res->getNext()->getValue(0)->getValue()); + } + return result; + } + + // Appends a well-formed record to the active WAL of a crashed database. + void appendToWAL(const WALRecord& record) const { + const auto bytes = serializeCheckpointRecord(record); + std::ofstream wal{StorageUtils::getWALFilePath(databasePath), + std::ios::binary | std::ios::app}; + wal.write(reinterpret_cast(bytes.data.get()), bytes.size); + if (systemConfig->enableChecksums) { + const auto recordChecksum = checksum(bytes.data.get(), bytes.size); + wal.write(reinterpret_cast(&recordChecksum), sizeof(recordChecksum)); + } + } + + void writeMarker(const std::string& content) const { + std::ofstream{markerPath(), std::ios::binary | std::ios::trunc} << content; + } + + std::string shadowPath() const { return StorageUtils::getShadowFilePath(databasePath); } + std::string markerPath() const { + return StorageUtils::getPinnedCheckpointMarkerPath(databasePath); + } + + std::unique_ptr reader; +}; + +// The next checkpoint dies after its storage phase wrote shadow pages, before it logged a record. +TEST_F(PinnedCheckpointCrashTest, LeftoverShadowKeepsCommitsAfterPinnedCheckpoint) { + query("CREATE NODE TABLE P(id INT64 PRIMARY KEY);"); + query("CHECKPOINT;"); + createRows(1, 3); + pinWAL(); + query("CHECKPOINT;"); + createRows(4, 5); + failCheckpoint(); + ASSERT_TRUE(std::filesystem::exists(shadowPath())); + crash(); + createDBAndConn(); + EXPECT_EQ(ids(), (std::vector{1, 2, 3, 4, 5})); +} + +TEST_F(PinnedCheckpointCrashTest, LeftoverShadowKeepsTableCreatedBeforePinnedCheckpoint) { + query("CREATE NODE TABLE P(id INT64 PRIMARY KEY);"); + createRows(1, 3); + pinWAL(); + query("CHECKPOINT;"); + createRows(4, 5); + failCheckpoint(); + ASSERT_TRUE(std::filesystem::exists(shadowPath())); + crash(); + createDBAndConn(); + EXPECT_EQ(ids(), (std::vector{1, 2, 3, 4, 5})); +} + +// No commit between the pinned checkpoint and the one that dies: the WAL ends in the applied +// CHECKPOINT record, and the shadow on disk is the next checkpoint's, cut short by a crash while +// it was being written. +TEST_F(PinnedCheckpointCrashTest, NextCheckpointShadowAfterPinnedCheckpointIsDiscarded) { + query("CREATE NODE TABLE P(id INT64 PRIMARY KEY);"); + createRows(1, 3); + pinWAL(); + query("CHECKPOINT;"); + failCheckpoint(); + ASSERT_TRUE(std::filesystem::exists(shadowPath())); + crash(); + std::filesystem::resize_file(shadowPath(), LBUG_PAGE_SIZE); + createDBAndConn(); + EXPECT_EQ(ids(), (std::vector{1, 2, 3})); +} + +// The state a crash leaves while cleanup removes the applied shadow: ShadowFile::clear truncates +// the file before unlinking it. +TEST_F(PinnedCheckpointCrashTest, EmptyShadowAfterPinnedCheckpointIsDiscarded) { + query("CREATE NODE TABLE P(id INT64 PRIMARY KEY);"); + createRows(1, 3); + pinWAL(); + query("CHECKPOINT;"); + crash(); + std::ofstream{shadowPath()}; + createDBAndConn(); + EXPECT_EQ(ids(), (std::vector{1, 2, 3})); +} + +class CheckpointerCrashesBeforeShadowRemoval final : public Checkpointer { +public: + explicit CheckpointerCrashesBeforeShadowRemoval(main::ClientContext& context) + : Checkpointer(context) {} + + static inline std::string markerAtHook; + static inline bool shadowAtHook = false; + +protected: + void beforeShadowRemoval() override { + const auto& databasePath = mainStorageManager->getDatabasePath(); + std::ifstream marker{StorageUtils::getPinnedCheckpointMarkerPath(databasePath), + std::ios::binary}; + markerAtHook = {std::istreambuf_iterator{marker}, {}}; + shadowAtHook = std::filesystem::exists(StorageUtils::getShadowFilePath(databasePath)); + throw RuntimeException("simulated crash before removing the shadow files."); + } +}; + +TEST_F(PinnedCheckpointCrashTest, MarkerIsDurableBeforeShadowsAreRemoved) { + query("CREATE NODE TABLE P(id INT64 PRIMARY KEY);"); + createRows(1, 3); + pinWAL(); + failCheckpoint(); + EXPECT_EQ(CheckpointerCrashesBeforeShadowRemoval::markerAtHook, + std::string(1, static_cast(StorageUtils::PINNED_CHECKPOINT_MARKER_APPLIED))); + EXPECT_TRUE(CheckpointerCrashesBeforeShadowRemoval::shadowAtHook); + ASSERT_TRUE(std::filesystem::exists(shadowPath())); + crash(); + createDBAndConn(); + EXPECT_EQ(ids(), (std::vector{1, 2, 3})); +} + +// The marker of an earlier pinned checkpoint must not vouch for a later CHECKPOINT record whose +// shadow was never applied. +TEST_F(PinnedCheckpointCrashTest, EarlierMarkerDoesNotSkipUnappliedCheckpoint) { + query("CREATE NODE TABLE P(id INT64 PRIMARY KEY);"); + createRows(1, 3); + pinWAL(); + query("CHECKPOINT;"); + ASSERT_TRUE(std::filesystem::exists(markerPath())); + createRows(4, 5); + failCheckpoint(); + EXPECT_FALSE(std::filesystem::exists(markerPath())); + ASSERT_TRUE(std::filesystem::exists(shadowPath())); + crash(); + createDBAndConn(); + EXPECT_EQ(ids(), (std::vector{1, 2, 3, 4, 5})); +} + +// Older builds kept the marker while logging the next CHECKPOINT record. +TEST_F(PinnedCheckpointCrashTest, LegacyMarkerDoesNotSkipUnappliedCheckpoint) { + query("CREATE NODE TABLE P(id INT64 PRIMARY KEY);"); + createRows(1, 3); + pinWAL(); + query("CHECKPOINT;"); + createRows(4, 5); + failCheckpoint(); + ASSERT_TRUE(std::filesystem::exists(shadowPath())); + crash(); + writeMarker("\x01"); + createDBAndConn(); + EXPECT_EQ(ids(), (std::vector{1, 2, 3, 4, 5})); +} + +// Copies the database's files once the storage phase has run (WAL rotated, shadow pages written), +// then fails: restoring the copy gives the files a process killed at that point leaves behind, +// which a failed checkpoint's rollback would otherwise tidy up. +class CheckpointerImagedAfterCheckpointStorage final : public Checkpointer { +public: + explicit CheckpointerImagedAfterCheckpointStorage(main::ClientContext& context) + : Checkpointer(context) {} + + static inline std::map> image; + +protected: + bool checkpointStorage() override { + Checkpointer::checkpointStorage(); + image.clear(); + const auto& databasePath = mainStorageManager->getDatabasePath(); + for (const auto& path : {databasePath, StorageUtils::getWALFilePath(databasePath), + StorageUtils::getCheckpointWALFilePath(databasePath), + StorageUtils::getShadowFilePath(databasePath), + StorageUtils::getPinnedCheckpointMarkerPath(databasePath)}) { + if (std::filesystem::exists(path)) { + std::ifstream file{path, std::ios::binary}; + image[path] = {std::istreambuf_iterator{file}, {}}; + } + } + throw RuntimeException("simulated crash after checkpoint storage phase."); + } +}; + +// A recovery that skipped the prefix of a pinned WAL keeps its CHECKPOINT record mid-file; the next +// unpinned checkpoint rotates that WAL into the frozen WAL and dies before logging its record. +TEST_F(PinnedCheckpointCrashTest, LeftoverShadowKeepsCommitsInFrozenWAL) { + query("CREATE NODE TABLE P(id INT64 PRIMARY KEY);"); + createRows(1, 3); + pinWAL(); + query("CHECKPOINT;"); + createRows(4, 5); + crash(); + createDBAndConn(); + query("CALL force_checkpoint_on_close=false;"); + query("CALL auto_checkpoint=false;"); + createRows(6, 7); + failCheckpoint(); + crash(); + auto& image = CheckpointerImagedAfterCheckpointStorage::image; + ASSERT_TRUE(image.contains(StorageUtils::getCheckpointWALFilePath(databasePath))); + ASSERT_TRUE(image.contains(shadowPath())); + for (const auto& path : {StorageUtils::getWALFilePath(databasePath), + StorageUtils::getCheckpointWALFilePath(databasePath), shadowPath(), markerPath()}) { + std::filesystem::remove(path); + } + // The commits after the inherited CHECKPOINT record prove it completed, without the marker. + image.erase(markerPath()); + for (const auto& [path, bytes] : image) { + std::ofstream{path, std::ios::binary}.write(bytes.data(), bytes.size()); + } + createDBAndConn(); + EXPECT_EQ(ids(), (std::vector{1, 2, 3, 4, 5, 6, 7})); +} + +// Without the shadow and without the marker, the pinned checkpoint's pages were lost out of band. +TEST_F(PinnedCheckpointCrashTest, MissingShadowWithoutMarkerRefusesToOpen) { + query("CREATE NODE TABLE P(id INT64 PRIMARY KEY);"); + createRows(1, 3); + pinWAL(); + query("CHECKPOINT;"); + crash(); + std::filesystem::remove(markerPath()); + try { + createDBAndConn(); + FAIL() << "reopened without the marker"; + } catch (const std::exception& e) { + EXPECT_NE(std::string{e.what()}.find("no pinned-checkpoint marker"), std::string::npos) + << e.what(); + } +} + +TEST_F(PinnedCheckpointCrashTest, MissingShadowWithoutMarkerRefusesToOpenWithLaterCommits) { + query("CREATE NODE TABLE P(id INT64 PRIMARY KEY);"); + createRows(1, 3); + pinWAL(); + query("CHECKPOINT;"); + createRows(4, 5); + crash(); + std::filesystem::remove(markerPath()); + try { + createDBAndConn(); + FAIL() << "reopened without the marker"; + } catch (const std::exception& e) { + EXPECT_NE(std::string{e.what()}.find("no pinned-checkpoint marker"), std::string::npos) + << e.what(); + } +} + +// A crash while a commit after the pinned checkpoint was being written leaves its BEGIN record. +// Recovery must retire the WAL: kept, it would end in an applied CHECKPOINT that the next +// checkpoint could rotate into the frozen WAL. +TEST_F(PinnedCheckpointCrashTest, TornCommitAfterPinnedCheckpointRetiresWAL) { + query("CREATE NODE TABLE P(id INT64 PRIMARY KEY);"); + createRows(1, 3); + pinWAL(); + query("CHECKPOINT;"); + crash(); + appendToWAL(BeginTransactionRecord{}); + createDBAndConn(); + EXPECT_EQ(ids(), (std::vector{1, 2, 3})); + const auto walPath = StorageUtils::getWALFilePath(databasePath); + EXPECT_TRUE(!std::filesystem::exists(walPath) || std::filesystem::file_size(walPath) == 0); +} + +// A marker written by an older build still proves that a missing shadow was applied. +TEST_F(PinnedCheckpointCrashTest, LegacyMarkerWithoutShadowReopens) { + query("CREATE NODE TABLE P(id INT64 PRIMARY KEY);"); + createRows(1, 3); + pinWAL(); + query("CHECKPOINT;"); + createRows(4, 5); + crash(); + writeMarker("\x01"); + createDBAndConn(); + EXPECT_EQ(ids(), (std::vector{1, 2, 3, 4, 5})); +} + +TEST_F(PinnedCheckpointCrashTest, PartitionedTableReopensAfterPinnedCheckpoint) { + query("CREATE NODE TABLE L(id INT64, cluster INT64, PRIMARY KEY(id)) PARTITION BY LIST " + "(cluster);"); + query("CREATE (:L {id: 5, cluster: 1});"); + query("CHECKPOINT;"); + pinWAL(); + query("CHECKPOINT;"); + crash(); + for (const auto& entry : + std::filesystem::directory_iterator{std::filesystem::path{databasePath}.parent_path()}) { + ASSERT_NE(entry.path().extension(), ".shadow") << entry.path(); + } + createDBAndConn(); + EXPECT_EQ(ids("L"), (std::vector{5})); +} + // Hash index basic recovery // // HashIndexLocalStorage has no per-entry timestamps; a correct fix for post-snapshotTS From f0efda6b17443a8b3b0c0c9ef837751be845d0c5 Mon Sep 17 00:00:00 2001 From: Darek Date: Sun, 25 Oct 2026 12:53:10 +0100 Subject: [PATCH 2/4] docs(storage): say which records follow a pinned CHECKPOINT, and the read-only refusal Only a COMMIT without its BEGIN keeps the prefix replay; records that end in no COMMIT are a torn tail of the last CHECKPOINT. A read-only open refuses a WAL that ends in a CHECKPOINT record, now also one followed by a torn commit. Co-Authored-By: Claude Opus 5.5 --- docs/checkpoint_recovery.md | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/docs/checkpoint_recovery.md b/docs/checkpoint_recovery.md index e82bc7a136..4ed6a8538f 100644 --- a/docs/checkpoint_recovery.md +++ b/docs/checkpoint_recovery.md @@ -43,9 +43,16 @@ therefore means that every `CHECKPOINT` record in the active WAL has been applie A transaction committed after a `CHECKPOINT` record proves that checkpoint completed, since the write gate is held until a checkpoint's cleanup ends and a failure after its record poisons the WAL. Recovery then replays only the records after the last `CHECKPOINT` and removes a remaining -shadow file, which belongs to a later checkpoint that never logged its record. Records after a -`CHECKPOINT` that form no whole transaction leave the earlier rule in place: with a shadow file, -only the records before the first `CHECKPOINT` replay. +shadow file, which belongs to a later checkpoint that never logged its record. A `COMMIT` after a +`CHECKPOINT` without its `BEGIN`, which the engine never writes, leaves the earlier rule in place: +with a shadow file, only the records before the first `CHECKPOINT` replay. Records that end in no +`COMMIT`, such as a torn commit, are a torn tail of the last `CHECKPOINT`, and the WAL counts as +ending in that record. + +Recovering a WAL that ends in a `CHECKPOINT` record writes: it applies or removes the shadow files +and retires the WAL. A read-only open therefore refuses such a database with `Cannot open database +in read-only mode while checkpoint recovery is pending`, also when the `CHECKPOINT` is followed by +a torn commit; opening it read-write once recovers it. When nothing was committed after the last `CHECKPOINT` record, the marker decides. With it, recovery discards the shadow files of the main database and partition children. Without it, a From e5923840b5c99cd4858e65b4eb6129fed5480ef3 Mon Sep 17 00:00:00 2001 From: Darek Date: Sun, 25 Oct 2026 13:19:59 +0100 Subject: [PATCH 3/4] test(storage): crash between removing the pinned marker and logging the CHECKPOINT A kill after WAL::logAndFlushCheckpoint removes the pinned-checkpoint marker and before it writes the CHECKPOINT record leaves the WAL ending in the earlier, applied CHECKPOINT, no marker, and the next checkpoint's complete shadow. Three tests cover it: nothing committed in between, commits in between, and a partitioned table whose shadows the dying checkpoint never wrote. Co-Authored-By: Claude Opus 5.5 --- test/transaction/checkpoint_test.cpp | 66 ++++++++++++++++++++++++++++ 1 file changed, 66 insertions(+) diff --git a/test/transaction/checkpoint_test.cpp b/test/transaction/checkpoint_test.cpp index 3a173169a5..3fb7bca998 100644 --- a/test/transaction/checkpoint_test.cpp +++ b/test/transaction/checkpoint_test.cpp @@ -5718,6 +5718,23 @@ class PinnedCheckpointCrashTest : public FlakyCheckpointerTest { std::unique_ptr reader; }; +// Dies where WAL::logAndFlushCheckpoint has removed the pinned-checkpoint marker and not yet +// written the CHECKPOINT record: the shadow is complete, and the marker no longer vouches for the +// WAL's last CHECKPOINT. +class FlakyCheckpointerKilledAfterMarkerRemoval final : public Checkpointer { +public: + explicit FlakyCheckpointerKilledAfterMarkerRemoval(main::ClientContext& context) + : Checkpointer(context) {} + + void logCheckpointAndApplyShadowPages(bool /*walRotated*/) override { + const auto storageManager = mainStorageManager; + auto& shadowFile = storageManager->getShadowFile(); + shadowFile.flushAll(storageManager->getOrInitDatabaseID(clientContext)); + WAL::Get(clientContext)->removePinnedCheckpointMarker(&clientContext); + throw RuntimeException("checkpoint failed."); + } +}; + // The next checkpoint dies after its storage phase wrote shadow pages, before it logged a record. TEST_F(PinnedCheckpointCrashTest, LeftoverShadowKeepsCommitsAfterPinnedCheckpoint) { query("CREATE NODE TABLE P(id INT64 PRIMARY KEY);"); @@ -5978,6 +5995,55 @@ TEST_F(PinnedCheckpointCrashTest, PartitionedTableReopensAfterPinnedCheckpoint) EXPECT_EQ(ids("L"), (std::vector{5})); } +// Nothing committed since the pinned checkpoint, and the next one dies after removing the marker: +// the WAL ends in the applied CHECKPOINT, and the complete shadow beside it is the next one's. +TEST_F(PinnedCheckpointCrashTest, CrashAfterMarkerRemovalReopens) { + query("CREATE NODE TABLE P(id INT64 PRIMARY KEY);"); + createRows(1, 3); + pinWAL(); + query("CHECKPOINT;"); + ASSERT_TRUE(std::filesystem::exists(markerPath())); + failCheckpoint(); + EXPECT_FALSE(std::filesystem::exists(markerPath())); + ASSERT_TRUE(std::filesystem::exists(shadowPath())); + crash(); + createDBAndConn(); + EXPECT_EQ(ids(), (std::vector{1, 2, 3})); + createRows(4, 4); + crash(); + createDBAndConn(); + EXPECT_EQ(ids(), (std::vector{1, 2, 3, 4})); +} + +TEST_F(PinnedCheckpointCrashTest, CrashAfterMarkerRemovalKeepsLaterCommits) { + query("CREATE NODE TABLE P(id INT64 PRIMARY KEY);"); + createRows(1, 3); + pinWAL(); + query("CHECKPOINT;"); + createRows(4, 5); + failCheckpoint(); + EXPECT_FALSE(std::filesystem::exists(markerPath())); + ASSERT_TRUE(std::filesystem::exists(shadowPath())); + crash(); + createDBAndConn(); + EXPECT_EQ(ids(), (std::vector{1, 2, 3, 4, 5})); +} + +// The dying checkpoint wrote no partition shadows; recovery must not require them. +TEST_F(PinnedCheckpointCrashTest, CrashAfterMarkerRemovalWithPartitionedTableReopens) { + query("CREATE NODE TABLE L(id INT64, cluster INT64, PRIMARY KEY(id)) PARTITION BY LIST " + "(cluster);"); + query("CREATE (:L {id: 5, cluster: 1});"); + query("CHECKPOINT;"); + pinWAL(); + query("CHECKPOINT;"); + failCheckpoint(); + EXPECT_FALSE(std::filesystem::exists(markerPath())); + crash(); + createDBAndConn(); + EXPECT_EQ(ids("L"), (std::vector{5})); +} + // Hash index basic recovery // // HashIndexLocalStorage has no per-entry timestamps; a correct fix for post-snapshotTS From 4ac98aa50bf253b145f89933104aa31723219547 Mon Sep 17 00:00:00 2001 From: Darek Date: Sun, 25 Oct 2026 18:31:07 +0100 Subject: [PATCH 4/4] docs(storage): correct two comments on pinned CHECKPOINT records The replay comment named records that form no whole transaction where only a COMMIT without its BEGIN reaches that branch; a torn tail counts as ending in the CHECKPOINT. The checkpointer comment said a WAL holding a CHECKPOINT record is never rotated and that frozen recovery needs the record to be final; the check is per process, so after a restart such a WAL can rotate, and frozen recovery skips through its last CHECKPOINT. Co-Authored-By: Claude Opus 5.5 --- src/storage/checkpointer.cpp | 10 +++++----- src/storage/wal/wal_replayer.cpp | 9 +++++---- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/src/storage/checkpointer.cpp b/src/storage/checkpointer.cpp index c2c5698758..e6dbb98a07 100644 --- a/src/storage/checkpointer.cpp +++ b/src/storage/checkpointer.cpp @@ -278,11 +278,11 @@ void Checkpointer::beginCheckpoint(common::transaction_t snapshotTimestamp, // snapshot may still need, and the next checkpoint refuses to overwrite a leftover // frozen WAL. The checkpoint proceeds without rotation; the CHECKPOINT record lands in // the active WAL and retirement below is skipped the same way. - // Independently, an active WAL that already holds a CHECKPOINT record (left by an - // earlier pinned checkpoint) is never rotated into a frozen WAL: the frozen recovery - // path requires the CHECKPOINT record to be final, so a rotated mid-file CHECKPOINT - // would break crash recovery. The record stays in the active WAL, whose recovery - // replays the suffix after the last CHECKPOINT. + // Independently, an active WAL holding a CHECKPOINT record this process logged (left by + // an earlier pinned checkpoint) is not rotated either: the record stays in the active + // WAL, whose recovery replays only the suffix after the last CHECKPOINT. The flag does + // not survive a restart, after which such a WAL can be rotated; frozen recovery then + // skips through its last CHECKPOINT the same way (see replayFrozenWAL). const bool pinWAL = retentionHorizon != common::INVALID_TRANSACTION; const bool activeWALHasCheckpoint = mainStorageManager->getWAL().mayHaveCheckpointRecord(); for (const auto& target : checkpointTargets) { diff --git a/src/storage/wal/wal_replayer.cpp b/src/storage/wal/wal_replayer.cpp index 9445435068..1846595a76 100644 --- a/src/storage/wal/wal_replayer.cpp +++ b/src/storage/wal/wal_replayer.cpp @@ -762,10 +762,11 @@ void WALReplayer::replayActiveWAL(Checkpointer& checkpointer, bool throwOnWalRep // record poisons the WAL before anything else can be appended. So does a missing // shadow, which cleanup removes only after applying it. Then the records up to the // last CHECKPOINT are in the data file and only the suffix replays; a remaining - // shadow belongs to a later checkpoint that never logged its record. Records after a - // CHECKPOINT that form no whole transaction are not ones the engine writes (it logs a - // COMMIT only after a BEGIN), so the checkpoint may not have completed: replay the - // prefix before the first CHECKPOINT record and discard everything from it onward. + // shadow belongs to a later checkpoint that never logged its record. A COMMIT after a + // CHECKPOINT without its BEGIN is not one the engine writes (it logs a COMMIT only + // after a BEGIN), so the checkpoint may not have completed: replay the prefix before + // the first CHECKPOINT record and discard everything from it onward. Records that end + // in no COMMIT do not get here: they are a torn tail of the last CHECKPOINT, above. // Cleanup writes the pinned-checkpoint marker before removing the shadow, so with // neither of them the shadow was lost out of band: throw instead of silently deleting // committed records. Check before deleting below.