From c8cc04c86ee3c80de81e92a89d19baff33f915be Mon Sep 17 00:00:00 2001 From: Darek Date: Sat, 24 Oct 2026 19:32:39 +0200 Subject: [PATCH 1/8] fix(txn): log an ABORT record when a committed transaction cannot be applied TransactionManager::commit makes a transaction's COMMIT record durable before publishCommit() applies it. When publish failed (a full buffer pool, or a named graph dropped by a concurrent DROP GRAPH), the caller rolled the transaction back in memory while the WAL kept the record, so a crash before the next checkpoint made recovery apply a COMMIT the client saw fail; a retried primary-key insert then left a database that no longer opened. - After such a failure, append and sync an ABORT record (new WAL record type) naming the transaction by the WAL offset where its records start, then report the original error. If the ABORT cannot be written either, poison the WAL: writes are refused until a reopen, which applies the transaction. - Recovery's dry pass collects aborted transactions; the replay loops of the active, frozen and graph WALs skip their records. - Remove the unused Transaction::commit(WAL*), a second publish path without this handling; tools/wal_dump prints the new record; docs describe it. Co-Authored-By: Claude Opus 5.5 --- docs/checkpoint_recovery.md | 17 +- src/include/storage/wal/record/abort_record.h | 31 ++ .../storage/wal/record/wal_record_base.h | 1 + src/include/storage/wal/wal.h | 8 +- src/include/storage/wal/wal_record.h | 1 + src/include/storage/wal/wal_replayer.h | 9 + src/include/transaction/transaction.h | 3 +- src/include/transaction/transaction_manager.h | 2 + src/storage/wal/CMakeLists.txt | 1 + src/storage/wal/records/abort_record.cpp | 32 +++ .../wal/typespec/records/abort_record.tsp | 7 + src/storage/wal/wal.cpp | 15 +- src/storage/wal/wal_record.cpp | 3 + src/storage/wal/wal_replayer.cpp | 60 +++- src/transaction/transaction.cpp | 14 +- src/transaction/transaction_manager.cpp | 38 ++- test/transaction/transaction_test.cpp | 264 +++++++++++++++++- test/transaction/wal_test.cpp | 25 ++ tools/wal_dump/main.cpp | 7 + 19 files changed, 510 insertions(+), 28 deletions(-) create mode 100644 src/include/storage/wal/record/abort_record.h create mode 100644 src/storage/wal/records/abort_record.cpp create mode 100644 src/storage/wal/typespec/records/abort_record.tsp diff --git a/docs/checkpoint_recovery.md b/docs/checkpoint_recovery.md index bed775f34d..259a54b4ae 100644 --- a/docs/checkpoint_recovery.md +++ b/docs/checkpoint_recovery.md @@ -32,6 +32,14 @@ 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. +## Failed commits + +A transaction whose changes cannot be applied after its `COMMIT` record is durable, for example +because the buffer pool is full, is rolled back in memory and gets an `ABORT` record naming the WAL +offset where its records start. Recovery skips the transactions such records name, so a `COMMIT` +that reported an error is never applied. If the `ABORT` record cannot be written either, the +database refuses further writes until it is reopened, and recovery then applies the transaction. + ## Checkpoint bundle format `CheckpointRecord::bundleFormatVersion` is 2 for checkpoints written by current builds, including @@ -94,10 +102,11 @@ An ordinary version 2 WAL tail — owner trailers on the records but no pending quieter hazard: an older build does not reliably refuse it. A record type the older build knows decodes with the trailer skipped as unknown trailing bytes and replays against the main catalog, silently misapplying records meant for a graph. A record type the older build does not know, such -as `UPDATE_SEQUENCE_NAMED`, is no safer: with the default non-throwing replay configuration the -decoding error is treated like a corrupt WAL tail — the older build replays the committed prefix -and truncates the WAL there, silently discarding that transaction and every transaction recorded -after it. Setting `throwOnWalReplayFailure` to true rejects an unknown record before that WAL +as `UPDATE_SEQUENCE_NAMED` or `ABORT`, is no safer: with the default non-throwing replay +configuration the decoding error is treated like a corrupt WAL tail — the older build replays the +committed prefix and truncates the WAL there, silently discarding that transaction and every +transaction recorded after it. For `ABORT`, which marks a transaction that failed after its +`COMMIT` record was written, the older build also applies the transaction it undoes. Setting `throwOnWalReplayFailure` to true rejects an unknown record before that WAL is applied, but it still does not make downgrading safe: a WAL containing only recognized record types passes validation, and its owner trailers are ignored. This is why the successful `CHECKPOINT`, not a clean close, is the downgrade gate. diff --git a/src/include/storage/wal/record/abort_record.h b/src/include/storage/wal/record/abort_record.h new file mode 100644 index 0000000000..bb2418022d --- /dev/null +++ b/src/include/storage/wal/record/abort_record.h @@ -0,0 +1,31 @@ +//===----------------------------------------------------------------------===// +// This file is automatically generated by scripts/generate_wal_typespec.py. +// Do not edit this file manually. +//===----------------------------------------------------------------------===// + +#pragma once + +#include +#include +#include +#include + +#include "storage/wal/record/wal_record_base.h" + +namespace lbug { +namespace storage { + +struct AbortRecord final : WALRecord { + uint64_t abortedBlockStart; + + AbortRecord() : WALRecord{WALRecordType::ABORT_RECORD}, abortedBlockStart{0} {} + + explicit AbortRecord(uint64_t abortedBlockStart) + : WALRecord{WALRecordType::ABORT_RECORD}, abortedBlockStart{abortedBlockStart} {} + + void serialize(common::Serializer& serializer) const override; + static std::unique_ptr deserialize(common::Deserializer& deserializer); +}; + +} // namespace storage +} // namespace lbug diff --git a/src/include/storage/wal/record/wal_record_base.h b/src/include/storage/wal/record/wal_record_base.h index 0ea29543b4..002373169b 100644 --- a/src/include/storage/wal/record/wal_record_base.h +++ b/src/include/storage/wal/record/wal_record_base.h @@ -25,6 +25,7 @@ enum class WALRecordType : uint8_t { // accidentally read from an empty buffer. BEGIN_TRANSACTION_RECORD = 1, COMMIT_RECORD = 2, + ABORT_RECORD = 3, COPY_TABLE_RECORD = 13, CREATE_CATALOG_ENTRY_RECORD = 14, diff --git a/src/include/storage/wal/wal.h b/src/include/storage/wal/wal.h index c0aaebaa42..d154757a9f 100644 --- a/src/include/storage/wal/wal.h +++ b/src/include/storage/wal/wal.h @@ -37,8 +37,12 @@ class WAL { common::VirtualFileSystem* vfs, StorageManager* storageManager = nullptr); ~WAL(); - void logCommittedWAL(LocalWAL& localWAL, main::ClientContext* context, - uint64_t& commitSequence); + // blockStart, if given, receives the WAL offset where the transaction's records begin. + void logCommittedWAL(LocalWAL& localWAL, main::ClientContext* context, uint64_t& commitSequence, + uint64_t* blockStart = nullptr); + // Logs and syncs that the transaction whose records begin at abortedBlockStart was not + // applied after its COMMIT record became durable, so recovery skips it. + void logAbort(main::ClientContext* context, uint64_t abortedBlockStart); void logAndFlushCheckpoint(main::ClientContext* context); // Renames the active WAL to the frozen checkpoint WAL. Returns false if there is nothing to diff --git a/src/include/storage/wal/wal_record.h b/src/include/storage/wal/wal_record.h index 40782526cf..5aa0ef6cef 100644 --- a/src/include/storage/wal/wal_record.h +++ b/src/include/storage/wal/wal_record.h @@ -1,5 +1,6 @@ #pragma once +#include "storage/wal/record/abort_record.h" #include "storage/wal/record/alter_table_entry_record.h" #include "storage/wal/record/begin_transaction_record.h" #include "storage/wal/record/checkpoint_record.h" diff --git a/src/include/storage/wal/wal_replayer.h b/src/include/storage/wal/wal_replayer.h index 69c8f2a7b4..862677cadb 100644 --- a/src/include/storage/wal/wal_replayer.h +++ b/src/include/storage/wal/wal_replayer.h @@ -22,10 +22,17 @@ class Checkpointer; class StorageManager; class WALReplayer { public: + // A committed transaction's records [start, end) that a later ABORT record undid. + struct AbortedBlock { + uint64_t start; + uint64_t end; + }; + struct GraphRecoveryState { struct WALReplayRange { std::string path; uint64_t endOffset; + std::vector abortedBlocks; }; std::vector walReplayRanges; @@ -62,6 +69,8 @@ class WALReplayer { uint64_t offsetBeforeFirstCheckpoint = 0; common::uuid walDatabaseID{}; uint64_t checkpointFormatVersion = 0; + // Sorted by start; replay skips these transactions. + std::vector abortedBlocks; }; void replayWALRecord(WALRecord& walRecord) const; diff --git a/src/include/transaction/transaction.h b/src/include/transaction/transaction.h index 8ad137208d..1f85394bf3 100644 --- a/src/include/transaction/transaction.h +++ b/src/include/transaction/transaction.h @@ -121,8 +121,7 @@ class LBUG_API Transaction { bool shouldForceCheckpoint() const; - void commit(storage::WAL* wal); - void writeCommitToWAL(storage::WAL* wal, uint64_t& walCommitSequence); + void writeCommitToWAL(storage::WAL* wal, uint64_t& walCommitSequence, uint64_t& walBlockStart); void publishCommit(); void rollback(storage::WAL* wal); void pushCommitCallback(std::function callback); diff --git a/src/include/transaction/transaction_manager.h b/src/include/transaction/transaction_manager.h index e456039179..fc48a36e24 100644 --- a/src/include/transaction/transaction_manager.h +++ b/src/include/transaction/transaction_manager.h @@ -75,6 +75,8 @@ class TransactionManager { private: bool hasNoActiveTransactions() const; void checkpointNoLock(main::ClientContext& clientContext); + void logAbortOfDurableCommit(main::ClientContext& clientContext, uint64_t walBlockStart, + const std::string& publishError); // Try to checkpoint without blocking. Returns immediately if another checkpoint is in // progress. Used by auto-checkpoint after commit. void tryCheckpoint(main::ClientContext& clientContext); diff --git a/src/storage/wal/CMakeLists.txt b/src/storage/wal/CMakeLists.txt index b17d46b8ee..a6e19e49bd 100644 --- a/src/storage/wal/CMakeLists.txt +++ b/src/storage/wal/CMakeLists.txt @@ -24,6 +24,7 @@ add_library(lbug_storage_wal wal.cpp wal_record.cpp wal_replayer.cpp + records/abort_record.cpp records/begin_transaction_record.cpp records/commit_record.cpp records/checkpoint_record.cpp diff --git a/src/storage/wal/records/abort_record.cpp b/src/storage/wal/records/abort_record.cpp new file mode 100644 index 0000000000..ecc84a09db --- /dev/null +++ b/src/storage/wal/records/abort_record.cpp @@ -0,0 +1,32 @@ +//===----------------------------------------------------------------------===// +// This file is automatically generated by scripts/generate_wal_typespec.py. +// Do not edit this file manually. +//===----------------------------------------------------------------------===// + +#include "common/serializer/deserializer.h" +#include "common/serializer/serializer.h" +#include "storage/wal/wal_record.h" + +using namespace lbug::common; +namespace lbug { +namespace storage { + +void AbortRecord::serialize(Serializer& serializer) const { + WALRecord::serialize(serializer); + serializer.writeDebuggingInfo("abortedBlockStart"); + serializer.write(abortedBlockStart); +} + +std::unique_ptr AbortRecord::deserialize(Deserializer& deserializer) { + std::string key; + uint64_t abortedBlockStart = 0; + if (deserializer.hasRemainingData()) { + deserializer.validateDebuggingInfo(key, "abortedBlockStart"); + deserializer.deserializeValue(abortedBlockStart); + } + + return std::make_unique(std::move(abortedBlockStart)); +} + +} // namespace storage +} // namespace lbug diff --git a/src/storage/wal/typespec/records/abort_record.tsp b/src/storage/wal/typespec/records/abort_record.tsp new file mode 100644 index 0000000000..4bef69284f --- /dev/null +++ b/src/storage/wal/typespec/records/abort_record.tsp @@ -0,0 +1,7 @@ +import "../common.tsp"; +import ; + +// @explicit_constructor=true +model AbortRecord extends WalRecordBase { + abortedBlockStart: uint64_t; +} diff --git a/src/storage/wal/wal.cpp b/src/storage/wal/wal.cpp index a4328e5e2f..7e36f7b598 100644 --- a/src/storage/wal/wal.cpp +++ b/src/storage/wal/wal.cpp @@ -39,7 +39,7 @@ WAL::WAL(const std::string& dbPath, bool readOnly, bool enableChecksums, Virtual WAL::~WAL() {} void WAL::logCommittedWAL(LocalWAL& localWAL, main::ClientContext* context, - uint64_t& commitSequence) { + uint64_t& commitSequence, uint64_t* blockStart) { DASSERT(!readOnly); commitSequence = 0; if (inMemory || localWAL.getSize() == 0) { @@ -48,11 +48,24 @@ void WAL::logCommittedWAL(LocalWAL& localWAL, main::ClientContext* context, std::unique_lock lck{mtx}; throwIfPoisonedNoLock(); initWriter(context); + if (blockStart != nullptr) { + *blockStart = serializer->getWriter()->getSize(); + } localWAL.inMemWriter->flush(*serializer->getWriter()); commitSequence = ++appendedCommitSequence; waitForDurabilityNoLock(commitSequence, lck); } +// Holds the WAL lock across the sync, like the CHECKPOINT record: this runs only after a failure. +void WAL::logAbort(main::ClientContext* context, uint64_t abortedBlockStart) { + DASSERT(!readOnly); + std::unique_lock lck{mtx}; + throwIfPoisonedNoLock(); + initWriter(context); + addNewWALRecordNoLock(AbortRecord{abortedBlockStart}); + flushAndSyncNoLock(); +} + void WAL::logAndFlushCheckpoint(main::ClientContext* context) { std::unique_lock lck{mtx}; throwIfPoisonedNoLock(); diff --git a/src/storage/wal/wal_record.cpp b/src/storage/wal/wal_record.cpp index 59708e5bc9..0d83cbfff8 100644 --- a/src/storage/wal/wal_record.cpp +++ b/src/storage/wal/wal_record.cpp @@ -48,6 +48,9 @@ std::unique_ptr WALRecord::deserialize(Deserializer& deserializer, case WALRecordType::COMMIT_RECORD: { walRecord = CommitRecord::deserialize(deserializer); } break; + case WALRecordType::ABORT_RECORD: { + walRecord = AbortRecord::deserialize(deserializer); + } break; case WALRecordType::CREATE_CATALOG_ENTRY_RECORD: { walRecord = CreateCatalogEntryRecord::deserialize(deserializer); } break; diff --git a/src/storage/wal/wal_replayer.cpp b/src/storage/wal/wal_replayer.cpp index c924a7824a..e289c07035 100644 --- a/src/storage/wal/wal_replayer.cpp +++ b/src/storage/wal/wal_replayer.cpp @@ -1,7 +1,9 @@ #include "storage/wal/wal_replayer.h" +#include #include #include +#include #include "catalog/catalog.h" #include "common/constants.h" @@ -75,6 +77,13 @@ class ReplayOwnerScope { }; } // namespace +static bool isInAbortedBlock(const std::vector& abortedBlocks, + uint64_t offset) { + const auto next = std::upper_bound(abortedBlocks.begin(), abortedBlocks.end(), offset, + [](uint64_t value, const WALReplayer::AbortedBlock& block) { return value < block.start; }); + return next != abortedBlocks.begin() && offset < std::prev(next)->end; +} + static void removePartitionChildShadowFiles(main::ClientContext& clientContext) { auto* dbManager = main::DatabaseManager::Get(clientContext); if (dbManager == nullptr) { @@ -479,7 +488,8 @@ WALReplayer::GraphRecoveryState WALReplayer::prepareGraphCheckpoint(StorageManag shadowDatabaseID); if (frozenWAL.has_value() && frozenWAL->isLastRecordCheckpoint && activeWAL.has_value() && !activeWAL->isLastRecordCheckpoint) { - recoveryState.walReplayRanges.push_back({activeWALPath, activeWAL->offsetDeserialized}); + recoveryState.walReplayRanges.push_back( + {activeWALPath, activeWAL->offsetDeserialized, activeWAL->abortedBlocks}); } else if (activeWAL.has_value() && activeWAL->isLastRecordCheckpoint && frozenWAL.has_value() && !frozenWAL->isLastRecordCheckpoint) { throw RuntimeException(std::format( @@ -490,10 +500,12 @@ WALReplayer::GraphRecoveryState WALReplayer::prepareGraphCheckpoint(StorageManag } else { removeFileAndSyncParentDirectory(graphShadowPath); if (frozenWAL.has_value()) { - recoveryState.walReplayRanges.push_back({frozenWALPath, frozenWAL->offsetDeserialized}); + recoveryState.walReplayRanges.push_back( + {frozenWALPath, frozenWAL->offsetDeserialized, frozenWAL->abortedBlocks}); } if (activeWAL.has_value()) { - recoveryState.walReplayRanges.push_back({activeWALPath, activeWAL->offsetDeserialized}); + recoveryState.walReplayRanges.push_back( + {activeWALPath, activeWAL->offsetDeserialized, activeWAL->abortedBlocks}); } } return recoveryState; @@ -533,7 +545,11 @@ void WALReplayer::replayGraphWAL(StorageManager& storageManager, storageManager.getOrInitDatabaseID(clientContext), walHeader.databaseID); deserializer.getReader()->onObjectEnd(); while (getReadOffset(deserializer, *walEnableChecksums) < range.endOffset) { + const auto recordStart = getReadOffset(deserializer, *walEnableChecksums); auto walRecord = WALRecord::deserialize(deserializer, clientContext); + if (isInAbortedBlock(range.abortedBlocks, recordStart)) { + continue; + } replayWALRecord(*walRecord); } truncateWALFile(*fileInfo, range.endOffset); @@ -657,8 +673,10 @@ void WALReplayer::replayFrozenWAL(Checkpointer& checkpointer, bool throwOnWalRep const auto skipBefore = discardCheckpointPrefix ? replayInfo.lastCheckpointOffset : 0; while (getReadOffset(deserializer, enableChecksums) < replayEnd) { DASSERT(!deserializer.finished()); + const auto recordStart = getReadOffset(deserializer, enableChecksums); auto walRecord = WALRecord::deserialize(deserializer, clientContext); - if (getReadOffset(deserializer, enableChecksums) <= skipBefore) { + if (getReadOffset(deserializer, enableChecksums) <= skipBefore || + isInAbortedBlock(replayInfo.abortedBlocks, recordStart)) { continue; } replayWALRecord(*walRecord); @@ -787,8 +805,10 @@ void WALReplayer::replayActiveWAL(Checkpointer& checkpointer, bool throwOnWalRep const auto skipBefore = discardCheckpointPrefix ? replayInfo.lastCheckpointOffset : 0; while (getReadOffset(deserializer, enableChecksums) < replayEnd) { DASSERT(!deserializer.finished()); + const auto recordStart = getReadOffset(deserializer, enableChecksums); auto walRecord = WALRecord::deserialize(deserializer, clientContext); - if (getReadOffset(deserializer, enableChecksums) <= skipBefore) { + if (getReadOffset(deserializer, enableChecksums) <= skipBefore || + isInAbortedBlock(replayInfo.abortedBlocks, recordStart)) { continue; } replayWALRecord(*walRecord); @@ -929,6 +949,9 @@ void WALReplayer::replayCommittedCheckpoint(Checkpointer& checkpointer, WALReplayer::WALReplayInfo WALReplayer::dryReplay(FileInfo& fileInfo, bool throwOnWalReplayFailure, bool enableChecksums) const { WALReplayInfo replayInfo; + std::optional openBlockStart; + std::unordered_map committedBlockEnds; + std::vector abortedBlockStarts; try { Deserializer deserializer = initDeserializer(fileInfo, clientContext, enableChecksums); @@ -940,9 +963,20 @@ WALReplayer::WALReplayInfo WALReplayer::dryReplay(FileInfo& fileInfo, bool throw bool finishedDeserializing = deserializer.finished(); while (!finishedDeserializing) { + const auto recordStart = getReadOffset(deserializer, enableChecksums); auto walRecord = WALRecord::deserialize(deserializer, clientContext); finishedDeserializing = deserializer.finished(); switch (walRecord->type) { + case WALRecordType::BEGIN_TRANSACTION_RECORD: { + replayInfo.isLastRecordCheckpoint = false; + openBlockStart = recordStart; + } break; + case WALRecordType::ABORT_RECORD: { + // Ends a durable unit like a COMMIT, so a torn-tail truncation keeps it. + replayInfo.isLastRecordCheckpoint = false; + replayInfo.offsetDeserialized = getReadOffset(deserializer, enableChecksums); + abortedBlockStarts.push_back(walRecord->constCast().abortedBlockStart); + } break; 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 @@ -967,6 +1001,10 @@ WALReplayer::WALReplayInfo WALReplayer::dryReplay(FileInfo& fileInfo, bool throw // Update the offset to the end of the last commit record. replayInfo.isLastRecordCheckpoint = false; replayInfo.offsetDeserialized = getReadOffset(deserializer, enableChecksums); + if (openBlockStart.has_value()) { + committedBlockEnds[*openBlockStart] = replayInfo.offsetDeserialized; + openBlockStart.reset(); + } } break; default: { // Any other record after a checkpoint also ends the checkpoint suffix. @@ -981,6 +1019,15 @@ WALReplayer::WALReplayInfo WALReplayer::dryReplay(FileInfo& fileInfo, bool throw throw; } } + // An ABORT record naming no committed transaction in this file is ignored. + for (const auto start : abortedBlockStarts) { + const auto block = committedBlockEnds.find(start); + if (block != committedBlockEnds.end()) { + replayInfo.abortedBlocks.push_back({start, block->second}); + } + } + std::sort(replayInfo.abortedBlocks.begin(), replayInfo.abortedBlocks.end(), + [](const AbortedBlock& a, const AbortedBlock& b) { return a.start < b.start; }); return replayInfo; } @@ -1037,6 +1084,9 @@ void WALReplayer::replayWALRecord(WALRecord& walRecord) const { case WALRecordType::BEGIN_TRANSACTION_RECORD: { TransactionContext::Get(clientContext)->beginRecoveryTransaction(); } break; + case WALRecordType::ABORT_RECORD: { + // The replay loops skip the transaction it names (see dryReplay). + } break; case WALRecordType::COMMIT_RECORD: { TransactionContext::Get(clientContext)->commit(); // The enclosing recovery transaction just completed, so graph WALs queued by lazy diff --git a/src/transaction/transaction.cpp b/src/transaction/transaction.cpp index 41a6e94a25..a5270994b1 100644 --- a/src/transaction/transaction.cpp +++ b/src/transaction/transaction.cpp @@ -64,23 +64,15 @@ bool Transaction::shouldForceCheckpoint() const { return !clientContext->isInMemory() && forceCheckpoint; } -void Transaction::commit(storage::WAL* wal) { - if (commitTS == common::INVALID_TRANSACTION) { - throw common::RuntimeException{"Cannot commit with an invalid commit timestamp."}; - } - uint64_t walCommitSequence = 0; - writeCommitToWAL(wal, walCommitSequence); - publishCommit(); -} - -void Transaction::writeCommitToWAL(storage::WAL* wal, uint64_t& walCommitSequence) { +void Transaction::writeCommitToWAL(storage::WAL* wal, uint64_t& walCommitSequence, + uint64_t& walBlockStart) { walCommitSequence = 0; if (!shouldLogToWAL()) { return; } DASSERT(localWAL && wal); localWAL->logCommit(); - wal->logCommittedWAL(*localWAL, clientContext, walCommitSequence); + wal->logCommittedWAL(*localWAL, clientContext, walCommitSequence, &walBlockStart); localWAL->clear(); } diff --git a/src/transaction/transaction_manager.cpp b/src/transaction/transaction_manager.cpp index e469a575f0..974e11209d 100644 --- a/src/transaction/transaction_manager.cpp +++ b/src/transaction/transaction_manager.cpp @@ -1,6 +1,7 @@ #include "transaction/transaction_manager.h" #include +#include #include #include "common/exception/checkpoint.h" @@ -108,11 +109,33 @@ Transaction* TransactionManager::beginTransaction(main::ClientContext& clientCon } } +// The transaction's COMMIT record is durable, but its changes were not applied and the caller +// rolls them back: without an ABORT record recovery would apply them. If even that cannot be +// logged, refusing writes keeps a retry from applying them twice before a reopen does. +void TransactionManager::logAbortOfDurableCommit(main::ClientContext& clientContext, + uint64_t walBlockStart, const std::string& publishError) { + try { + wal.logAbort(&clientContext, walBlockStart); + } catch (const std::exception& e) { + // A failed sync has poisoned the WAL already; one that failed before it has not. + wal.poison(std::format("A transaction failed after its COMMIT record was written, and " + "logging its abort failed: {}", + e.what())); + throw TransactionManagerException(std::format( + "The transaction failed after its COMMIT record was written, and logging its abort " + "failed, so it is applied when the database reopens; until then the database refuses " + "further writes. Original error: {}. Abort error: {}", + publishError, e.what())); + } +} + void TransactionManager::commit(main::ClientContext& clientContext, Transaction* transaction) { bool shouldForceCheckpoint = false; bool shouldAutoCheckpoint = false; bool markedAsCommitting = false; uint64_t walCommitSequence = 0; + uint64_t walBlockStart = 0; + std::optional publishError; try { { std::unique_lock lck{mtxForSerializingPublicFunctionCalls}; @@ -126,7 +149,7 @@ void TransactionManager::commit(main::ClientContext& clientContext, Transaction* committingWriteTransactionCount.fetch_add(1, std::memory_order_release); markedAsCommitting = true; lck.unlock(); - transaction->writeCommitToWAL(&wal, walCommitSequence); + transaction->writeCommitToWAL(&wal, walCommitSequence, walBlockStart); lck.lock(); if (walCommitSequence != 0) { cvForPublishingCommit.wait(lck, @@ -134,7 +157,15 @@ void TransactionManager::commit(main::ClientContext& clientContext, Transaction* } lastTimestamp.fetch_add(1, std::memory_order_acq_rel); transaction->commitTS = lastTimestamp.load(std::memory_order_acquire); - transaction->publishCommit(); + try { + transaction->publishCommit(); + } catch (const std::exception& e) { + publishError = e.what(); + throw; + } catch (...) { + publishError = "unknown exception"; + throw; + } if (walCommitSequence != 0) { nextWALCommitSequenceToPublish++; cvForPublishingCommit.notify_all(); @@ -169,6 +200,9 @@ void TransactionManager::commit(main::ClientContext& clientContext, Transaction* committingWriteTransactionCount.fetch_sub(1, std::memory_order_release); cvForCommittingWriteTransaction.notify_all(); } + if (publishError && walCommitSequence != 0) { + logAbortOfDurableCommit(clientContext, walBlockStart, *publishError); + } throw; } // Checkpoint outside the public function lock so active writers can finish diff --git a/test/transaction/transaction_test.cpp b/test/transaction/transaction_test.cpp index 993a0ed6ff..fcb7b9ca73 100644 --- a/test/transaction/transaction_test.cpp +++ b/test/transaction/transaction_test.cpp @@ -2,12 +2,16 @@ #include "api_test/api_test.h" #include "api_test/private_api_test.h" +#include "common/checksum.h" #include "common/exception/runtime.h" +#include "common/serializer/buffer_writer.h" +#include "common/serializer/serializer.h" #include "graph_test/private_graph_test.h" #include "storage/buffer_manager/buffer_manager.h" #include "storage/buffer_manager/memory_manager.h" #include "storage/storage_utils.h" #include "storage/wal/wal.h" +#include "storage/wal/wal_record.h" #include "transaction/transaction.h" #include @@ -74,7 +78,7 @@ TEST_F(PrivateApiTest, CommitRollbackRemoveActiveTransaction) { TEST_F(PrivateApiTest, DirectTransactionCommitRequiresCommitTimestamp) { Transaction transaction(TransactionType::WRITE); - EXPECT_THROW(transaction.commit(nullptr), lbug::common::RuntimeException); + EXPECT_THROW(transaction.publishCommit(), lbug::common::RuntimeException); } TEST_F(PrivateApiTest, CloseConnectionWithActiveTransaction) { @@ -779,6 +783,10 @@ class FailOnceInCommitBufferManager final : public lbug::storage::BufferManager auto* transaction = ctx ? Transaction::Get(*ctx) : nullptr; if (transaction && transaction->getCommitTS() != INVALID_TRANSACTION && failNextCommit.exchange(false)) { + if (failAbortToo) { + // Logging the ABORT record then fails as well. + lbug::storage::WAL::Get(*ctx)->poison("injected failure"); + } return false; } return lbug::storage::BufferManager::reserve(sizeToReserve); @@ -786,8 +794,25 @@ class FailOnceInCommitBufferManager final : public lbug::storage::BufferManager lbug::main::ClientContext* ctx = nullptr; std::atomic failNextCommit = false; + bool failAbortToo = false; }; +// The bytes of one framed WAL record, with its checksum when the WAL has them. +static std::vector walRecordBytes(const lbug::storage::WALRecord& record, + bool enableChecksums) { + auto writer = std::make_shared(); + Serializer serializer{writer}; + lbug::storage::WALRecord::serializeWithLength(serializer, record); + auto data = writer->getData(); + std::vector bytes(data.data.get(), data.data.get() + data.size); + if (enableChecksums) { + const auto recordChecksum = checksum(data.data.get(), data.size); + const auto* checksumBytes = reinterpret_cast(&recordChecksum); + bytes.insert(bytes.end(), checksumBytes, checksumBytes + sizeof(recordChecksum)); + } + return bytes; +} + class FailedCommitTest : public EmptyDBTest { public: void TearDown() override { @@ -798,10 +823,247 @@ class FailedCommitTest : public EmptyDBTest { EmptyDBTest::TearDown(); } + void constructFailingDB() { + auto constructBM = [&](const lbug::main::Database& db) { + auto manager = std::make_unique(databasePath, + databasePath + ".tmp", systemConfig->bufferPoolSize, systemConfig->maxDBSize, + getFileSystem(db), systemConfig->readOnly); + bm = manager.get(); + return manager; + }; + database = BaseGraphTest::constructDB(databasePath, *systemConfig, constructBM); + conn = std::make_unique(database.get()); + bm->ctx = conn->getClientContext(); + query("CALL force_checkpoint_on_close=false"); + query("CALL auto_checkpoint=false"); + } + + void query(const std::string& statement) { + auto result = conn->query(statement); + ASSERT_TRUE(result->isSuccess()) << statement << ": " << result->getErrorMessage(); + } + + void commitFailingOnce() { + getMemoryManager(*database)->releaseReusableBuffers(); + bm->failNextCommit = true; + auto commit = conn->query("COMMIT"); + ASSERT_FALSE(commit->isSuccess()); + ASSERT_FALSE(hasActiveTransaction(*conn)); + } + + // Tears down without a closing checkpoint, as when the process dies, and reopens with the + // default buffer manager. + void crashAndReopen() { + if (bm) { + bm->ctx = nullptr; + bm = nullptr; + } + conn.reset(); + database.reset(); + createDBAndConn(); + } + + void appendAbort(const std::string& walPath, uint64_t abortedStart, uint64_t bytesToDrop) { + const auto bytes = + walRecordBytes(lbug::storage::AbortRecord{abortedStart}, systemConfig->enableChecksums); + std::ofstream wal{walPath, std::ios::binary | std::ios::app}; + wal.write(bytes.data(), static_cast(bytes.size() - bytesToDrop)); + } + + void crashAndReopenWithAbort(const std::string& walPath, uint64_t abortedStart, + uint64_t bytesToDrop) { + conn.reset(); + database.reset(); + appendAbort(walPath, abortedStart, bytesToDrop); + createDBAndConn(); + } + + int64_t count(const std::string& query) { + auto result = conn->query(query); + EXPECT_TRUE(result->isSuccess()) << result->getErrorMessage(); + return result->isSuccess() ? result->getNext()->getValue(0)->getValue() : -1; + } + FailOnceInCommitBufferManager* bm = nullptr; }; + } // namespace +// The COMMIT record is in the WAL when applying the transaction fails, so without its ABORT +// record recovery applied what the connection had rolled back. +TEST_F(FailedCommitTest, FailedCommitIsNotAppliedAfterCrash) { + if (inMemMode) { + GTEST_SKIP(); + } + constructFailingDB(); + query("CREATE NODE TABLE A(id INT64 PRIMARY KEY, b INT64)"); + query("CREATE REL TABLE T(FROM A TO A)"); + query("CREATE (:A {id: 0, b: 0}), (:A {id: 1, b: 0})"); + const auto insert = "MATCH (a:A {id: 0}), (c:A {id: 1}) CREATE (a)-[:T]->(c)"; + query(insert); + query("CHECKPOINT"); + query("BEGIN TRANSACTION"); + query("MATCH (a:A {id: 0}) SET a.b = 1"); + query(insert); + commitFailingOnce(); + crashAndReopen(); + EXPECT_EQ(count("MATCH (:A)-[t:T]->(:A) RETURN count(t)"), 1); + EXPECT_EQ(count("MATCH (a:A) WHERE a.b = 1 RETURN count(a)"), 0); +} + +// A client retrying the failed insert of a primary key left a database that no longer opened. +TEST_F(FailedCommitTest, RetriedPrimaryKeyInsertReopensAfterCrash) { + if (inMemMode) { + GTEST_SKIP(); + } + constructFailingDB(); + query("CREATE NODE TABLE A(id INT64 PRIMARY KEY)"); + query("CREATE (:A {id: 1})"); + query("CHECKPOINT"); + query("BEGIN TRANSACTION"); + query("CREATE (:A {id: 2})"); + commitFailingOnce(); + EXPECT_EQ(count("MATCH (a:A) WHERE a.id = 2 RETURN count(a)"), 0); + query("CREATE (:A {id: 2})"); + crashAndReopen(); + EXPECT_EQ(count("MATCH (a:A) RETURN count(a)"), 2); + EXPECT_EQ(count("MATCH (a:A) WHERE a.id = 2 RETURN count(a)"), 1); +} + +TEST_F(FailedCommitTest, FailedCommitOfTwoTablesLeavesNeither) { + if (inMemMode) { + GTEST_SKIP(); + } + constructFailingDB(); + query("CREATE NODE TABLE A(id INT64 PRIMARY KEY)"); + query("CREATE NODE TABLE B(id INT64 PRIMARY KEY)"); + query("CREATE REL TABLE T(FROM A TO B)"); + query("CREATE (:A {id: 0}), (:B {id: 0})"); + query("CHECKPOINT"); + query("BEGIN TRANSACTION"); + query("CREATE (:A {id: 1})"); + query("MATCH (a:A {id: 0}), (b:B {id: 0}) CREATE (a)-[:T]->(b)"); + commitFailingOnce(); + EXPECT_EQ(count("MATCH (a:A) RETURN count(a)"), 1); + EXPECT_EQ(count("MATCH ()-[t:T]->() RETURN count(t)"), 0); + crashAndReopen(); + EXPECT_EQ(count("MATCH (a:A) RETURN count(a)"), 1); + EXPECT_EQ(count("MATCH ()-[t:T]->() RETURN count(t)"), 0); +} + +// An ABORT record skips exactly the transaction whose records start at the offset it names, not +// the ones around it. +TEST_F(FailedCommitTest, AbortRecordSkipsOnlyTheNamedTransaction) { + if (inMemMode) { + GTEST_SKIP(); + } + createDBAndConn(); + query("CALL force_checkpoint_on_close=false"); + query("CALL auto_checkpoint=false"); + query("CREATE NODE TABLE A(id INT64 PRIMARY KEY)"); + query("CHECKPOINT"); + query("CREATE (:A {id: 0})"); + const auto walPath = lbug::storage::StorageUtils::getWALFilePath(databasePath); + const auto abortedStart = std::filesystem::file_size(walPath); + query("CREATE (:A {id: 1})"); + query("CREATE (:A {id: 2})"); + crashAndReopenWithAbort(walPath, abortedStart, 0 /* bytesToDrop */); + EXPECT_EQ(count("MATCH (a:A) WHERE a.id = 1 RETURN count(a)"), 0); + EXPECT_EQ(count("MATCH (a:A) RETURN count(a)"), 2); +} + +// A crash while the ABORT record was being written leaves it torn: the transaction stands, as +// after any crash during its COMMIT. +TEST_F(FailedCommitTest, TornAbortRecordLeavesTheTransaction) { + if (inMemMode) { + GTEST_SKIP(); + } + createDBAndConn(); + query("CALL force_checkpoint_on_close=false"); + query("CALL auto_checkpoint=false"); + query("CREATE NODE TABLE A(id INT64 PRIMARY KEY)"); + query("CHECKPOINT"); + query("CREATE (:A {id: 0})"); + const auto walPath = lbug::storage::StorageUtils::getWALFilePath(databasePath); + const auto abortedStart = std::filesystem::file_size(walPath); + query("CREATE (:A {id: 1})"); + crashAndReopenWithAbort(walPath, abortedStart, 3 /* bytesToDrop */); + EXPECT_EQ(count("MATCH (a:A) RETURN count(a)"), 2); +} + +// When even the ABORT record cannot be logged, the WAL refuses writes and the next open applies +// the transaction, as the error says. +TEST_F(FailedCommitTest, FailedAbortLeavesTheTransactionToRecovery) { + if (inMemMode) { + GTEST_SKIP(); + } + constructFailingDB(); + query("CREATE NODE TABLE A(id INT64 PRIMARY KEY)"); + query("CHECKPOINT"); + query("BEGIN TRANSACTION"); + query("CREATE (:A {id: 1})"); + getMemoryManager(*database)->releaseReusableBuffers(); + bm->failAbortToo = true; + bm->failNextCommit = true; + auto commit = conn->query("COMMIT"); + ASSERT_FALSE(commit->isSuccess()); + EXPECT_NE(commit->getErrorMessage().find("logging its abort failed"), std::string::npos) + << commit->getErrorMessage(); + auto write = conn->query("CREATE (:A {id: 2})"); + ASSERT_FALSE(write->isSuccess()); + EXPECT_NE(write->getErrorMessage().find("refuses further writes"), std::string::npos) + << write->getErrorMessage(); + crashAndReopen(); + EXPECT_EQ(count("MATCH (a:A) WHERE a.id = 1 RETURN count(a)"), 1); + EXPECT_EQ(count("MATCH (a:A) WHERE a.id = 2 RETURN count(a)"), 0); +} + +// A crash after a checkpoint rotated the WAL leaves the ABORT in the frozen WAL, whose recovery +// finishes the interrupted checkpoint; the transaction stays skipped across both opens. +TEST_F(FailedCommitTest, AbortRecordSkipsTheTransactionInAFrozenWAL) { + if (inMemMode) { + GTEST_SKIP(); + } + createDBAndConn(); + query("CALL force_checkpoint_on_close=false"); + query("CALL auto_checkpoint=false"); + query("CREATE NODE TABLE A(id INT64 PRIMARY KEY)"); + query("CHECKPOINT"); + query("CREATE (:A {id: 0})"); + const auto walPath = lbug::storage::StorageUtils::getWALFilePath(databasePath); + const auto abortedStart = std::filesystem::file_size(walPath); + query("CREATE (:A {id: 1})"); + conn.reset(); + database.reset(); + appendAbort(walPath, abortedStart, 0 /* bytesToDrop */); + std::filesystem::rename(walPath, + lbug::storage::StorageUtils::getCheckpointWALFilePath(databasePath)); + createDBAndConn(); + EXPECT_EQ(count("MATCH (a:A) RETURN count(a)"), 1); + conn.reset(); + database.reset(); + createDBAndConn(); + EXPECT_EQ(count("MATCH (a:A) RETURN count(a)"), 1); + EXPECT_EQ(count("MATCH (a:A) WHERE a.id = 0 RETURN count(a)"), 1); +} + +TEST_F(FailedCommitTest, AbortRecordNamingNoTransactionIsIgnored) { + if (inMemMode) { + GTEST_SKIP(); + } + createDBAndConn(); + query("CALL force_checkpoint_on_close=false"); + query("CALL auto_checkpoint=false"); + query("CREATE NODE TABLE A(id INT64 PRIMARY KEY)"); + query("CHECKPOINT"); + query("CREATE (:A {id: 0})"); + const auto walPath = lbug::storage::StorageUtils::getWALFilePath(databasePath); + const auto middleOfARecord = std::filesystem::file_size(walPath) - 1; + query("CREATE (:A {id: 1})"); + crashAndReopenWithAbort(walPath, middleOfARecord, 0 /* bytesToDrop */); + EXPECT_EQ(count("MATCH (a:A) RETURN count(a)"), 2); +} + TEST_F(FailedCommitTest, KeepsCheckpointedRels) { auto constructBM = [&](const lbug::main::Database& db) { auto manager = std::make_unique(databasePath, diff --git a/test/transaction/wal_test.cpp b/test/transaction/wal_test.cpp index a7bde256a5..f4b31e0cff 100644 --- a/test/transaction/wal_test.cpp +++ b/test/transaction/wal_test.cpp @@ -282,6 +282,31 @@ TEST_F(WalTest, OrdinaryCommitSyncsRegisteredNamespace) { lbug::storage::StorageUtils::getWALFilePath("failing-sync://db"))); } +// The ABORT record of a transaction that failed after its COMMIT record became durable must be +// synced too; when that fails, the WAL refuses writes instead. +TEST_F(WalTest, AbortSyncFailurePoisonsWAL) { + VirtualFileSystem vfs; + auto failingFS = std::make_unique(false /* failSync */); + auto* failingFSPtr = failingFS.get(); + vfs.registerFileSystem(std::move(failingFS)); + + lbug::storage::WAL wal("failing-sync://db", false /* readOnly */, false /* enableChecksums */, + &vfs); + lbug::storage::LocalWAL localWAL(*lbug::storage::MemoryManager::Get(*conn->getClientContext()), + false /* enableChecksums */); + localWAL.logLoadExtension("" /* main */, "dummy"); + localWAL.logCommit(); + uint64_t walCommitSequence = 0; + uint64_t blockStart = 0; + ASSERT_NO_THROW( + wal.logCommittedWAL(localWAL, conn->getClientContext(), walCommitSequence, &blockStart)); + EXPECT_GT(blockStart, 0); + + failingFSPtr->setFailSync(true); + EXPECT_THROW(wal.logAbort(conn->getClientContext(), blockStart), RuntimeException); + EXPECT_THROW(wal.throwIfPoisoned(), RuntimeException); +} + TEST_F(WalTest, FailureAfterCheckpointRenamePoisonsWAL) { VirtualFileSystem vfs; auto failingFS = std::make_unique(false /* failSync */); diff --git a/tools/wal_dump/main.cpp b/tools/wal_dump/main.cpp index 02910a1038..554d329475 100644 --- a/tools/wal_dump/main.cpp +++ b/tools/wal_dump/main.cpp @@ -58,6 +58,8 @@ static std::string walRecordTypeToString(WALRecordType type) { return "BEGIN_TRANSACTION_RECORD"; case WALRecordType::COMMIT_RECORD: return "COMMIT_RECORD"; + case WALRecordType::ABORT_RECORD: + return "ABORT_RECORD"; case WALRecordType::COPY_TABLE_RECORD: return "COPY_TABLE_RECORD"; case WALRecordType::CREATE_CATALOG_ENTRY_RECORD: @@ -123,6 +125,11 @@ static void dumpRecord(const WALRecord& record) { case WALRecordType::COMMIT_RECORD: std::cout << " Type: COMMIT\n"; break; + case WALRecordType::ABORT_RECORD: + std::cout << " Type: ABORT\n"; + std::cout << " AbortedBlockStart: " + << record.constCast().abortedBlockStart << "\n"; + break; case WALRecordType::CHECKPOINT_RECORD: std::cout << " Type: CHECKPOINT\n"; break; From 8363d7fb8332fb361d3db9407c1cb568140372b5 Mon Sep 17 00:00:00 2001 From: Darek Date: Fri, 30 Oct 2026 13:45:59 +0100 Subject: [PATCH 2/8] chore(txn): pin where a failed commit's ABORT record lands A failed commit's ABORT record must land in the WAL file that holds its COMMIT and name the offset dryReplay reads for the block's first record; otherwise recovery ignores the ABORT and applies the rolled-back transaction. WAL::logAbort now asserts that the WAL writer was not reset since the COMMIT: rotation and retirement reset it, and a checkpoint does either only after the failed transaction is rolled back, which happens after its ABORT is synced. WAL::logCommittedWAL asserts that the block grows the writer by exactly its own bytes. The crash test of an engine-written ABORT also runs with per-record checksums off, where the offsets differ. Co-Authored-By: Claude Opus 5.5 --- src/storage/wal/wal.cpp | 8 ++++ src/transaction/transaction_manager.cpp | 3 ++ test/transaction/transaction_test.cpp | 49 ++++++++++++++++++------- 3 files changed, 46 insertions(+), 14 deletions(-) diff --git a/src/storage/wal/wal.cpp b/src/storage/wal/wal.cpp index 7e36f7b598..edc4e67542 100644 --- a/src/storage/wal/wal.cpp +++ b/src/storage/wal/wal.cpp @@ -49,9 +49,13 @@ void WAL::logCommittedWAL(LocalWAL& localWAL, main::ClientContext* context, throwIfPoisonedNoLock(); initWriter(context); if (blockStart != nullptr) { + // No checksum entry is open between records, so the writer's size is the file offset of + // the block's first record, the offset WALReplayer::dryReplay reads back for it. *blockStart = serializer->getWriter()->getSize(); } localWAL.inMemWriter->flush(*serializer->getWriter()); + DASSERT(blockStart == nullptr || + serializer->getWriter()->getSize() == *blockStart + localWAL.getSize()); commitSequence = ++appendedCommitSequence; waitForDurabilityNoLock(commitSequence, lck); } @@ -61,6 +65,10 @@ void WAL::logAbort(main::ClientContext* context, uint64_t abortedBlockStart) { DASSERT(!readOnly); std::unique_lock lck{mtx}; throwIfPoisonedNoLock(); + // The aborted transaction counts as an active writer until it is rolled back, and a + // checkpoint rotates or retires the WAL only once no writer is active, so the file holding its + // COMMIT is still the open one: dryReplay matches an ABORT only within one file. + DASSERT(serializer != nullptr && abortedBlockStart < serializer->getWriter()->getSize()); initWriter(context); addNewWALRecordNoLock(AbortRecord{abortedBlockStart}); flushAndSyncNoLock(); diff --git a/src/transaction/transaction_manager.cpp b/src/transaction/transaction_manager.cpp index 974e11209d..96936e50db 100644 --- a/src/transaction/transaction_manager.cpp +++ b/src/transaction/transaction_manager.cpp @@ -200,6 +200,9 @@ void TransactionManager::commit(main::ClientContext& clientContext, Transaction* committingWriteTransactionCount.fetch_sub(1, std::memory_order_release); cvForCommittingWriteTransaction.notify_all(); } + // activeWriteTransactionCount still counts this transaction until the caller rolls it back, + // so no checkpoint can rotate the WAL between its COMMIT and this ABORT. A failed WAL sync + // logs no ABORT: it has poisoned the WAL, and a reopen applies the COMMIT if it is durable. if (publishError && walCommitSequence != 0) { logAbortOfDurableCommit(clientContext, walBlockStart, *publishError); } diff --git a/test/transaction/transaction_test.cpp b/test/transaction/transaction_test.cpp index fcb7b9ca73..98dc76e1af 100644 --- a/test/transaction/transaction_test.cpp +++ b/test/transaction/transaction_test.cpp @@ -884,9 +884,34 @@ class FailedCommitTest : public EmptyDBTest { return result->isSuccess() ? result->getNext()->getValue(0)->getValue() : -1; } + void expectFailedCommitNotAppliedAfterCrash() { + constructFailingDB(); + query("CREATE NODE TABLE A(id INT64 PRIMARY KEY, b INT64)"); + query("CREATE REL TABLE T(FROM A TO A)"); + query("CREATE (:A {id: 0, b: 0}), (:A {id: 1, b: 0})"); + const auto insert = "MATCH (a:A {id: 0}), (c:A {id: 1}) CREATE (a)-[:T]->(c)"; + query(insert); + query("CHECKPOINT"); + query("BEGIN TRANSACTION"); + query("MATCH (a:A {id: 0}) SET a.b = 1"); + query(insert); + commitFailingOnce(); + crashAndReopen(); + EXPECT_EQ(count("MATCH (:A)-[t:T]->(:A) RETURN count(t)"), 1); + EXPECT_EQ(count("MATCH (a:A) WHERE a.b = 1 RETURN count(a)"), 0); + } + FailOnceInCommitBufferManager* bm = nullptr; }; +class FailedCommitWithoutChecksumsTest : public FailedCommitTest { +public: + void SetUp() override { + FailedCommitTest::SetUp(); + systemConfig->enableChecksums = false; + } +}; + } // namespace // The COMMIT record is in the WAL when applying the transaction fails, so without its ABORT @@ -895,20 +920,16 @@ TEST_F(FailedCommitTest, FailedCommitIsNotAppliedAfterCrash) { if (inMemMode) { GTEST_SKIP(); } - constructFailingDB(); - query("CREATE NODE TABLE A(id INT64 PRIMARY KEY, b INT64)"); - query("CREATE REL TABLE T(FROM A TO A)"); - query("CREATE (:A {id: 0, b: 0}), (:A {id: 1, b: 0})"); - const auto insert = "MATCH (a:A {id: 0}), (c:A {id: 1}) CREATE (a)-[:T]->(c)"; - query(insert); - query("CHECKPOINT"); - query("BEGIN TRANSACTION"); - query("MATCH (a:A {id: 0}) SET a.b = 1"); - query(insert); - commitFailingOnce(); - crashAndReopen(); - EXPECT_EQ(count("MATCH (:A)-[t:T]->(:A) RETURN count(t)"), 1); - EXPECT_EQ(count("MATCH (a:A) WHERE a.b = 1 RETURN count(a)"), 0); + expectFailedCommitNotAppliedAfterCrash(); +} + +// Without per-record checksums the transaction's records start at other offsets, which the ABORT +// record must still name. +TEST_F(FailedCommitWithoutChecksumsTest, FailedCommitIsNotAppliedAfterCrash) { + if (inMemMode) { + GTEST_SKIP(); + } + expectFailedCommitNotAppliedAfterCrash(); } // A client retrying the failed insert of a primary key left a database that no longer opened. From 8c8d92b195df0114d206cc394e9b605e506d1113 Mon Sep 17 00:00:00 2001 From: Darek Date: Fri, 30 Oct 2026 14:05:46 +0100 Subject: [PATCH 3/8] fix(txn): ignore an ABORT record that names no committed block after the last CHECKPOINT dryReplay advanced the replay end and cleared the trailing-CHECKPOINT flag for every ABORT record. A stray one after a CHECKPOINT whose shadow was not applied yet therefore turned recovery into a second replay of the records before an earlier pinned CHECKPOINT, which fails on a duplicated primary key. The engine logs an ABORT only after its own COMMIT, with no CHECKPOINT between them; any other ABORT is now ignored entirely. WAL::logAbort refuses, rather than only asserting, an ABORT whose COMMIT is not in the open WAL file; the failed commit's caller then poisons the WAL. Co-Authored-By: Claude Opus 5.5 --- src/storage/wal/wal.cpp | 8 ++++--- src/storage/wal/wal_replayer.cpp | 22 +++++++++--------- test/transaction/checkpoint_test.cpp | 34 ++++++++++++++++++++++++++++ test/transaction/wal_test.cpp | 22 ++++++++++++++++++ 4 files changed, 72 insertions(+), 14 deletions(-) diff --git a/src/storage/wal/wal.cpp b/src/storage/wal/wal.cpp index edc4e67542..423a8c0d1f 100644 --- a/src/storage/wal/wal.cpp +++ b/src/storage/wal/wal.cpp @@ -61,15 +61,17 @@ void WAL::logCommittedWAL(LocalWAL& localWAL, main::ClientContext* context, } // Holds the WAL lock across the sync, like the CHECKPOINT record: this runs only after a failure. -void WAL::logAbort(main::ClientContext* context, uint64_t abortedBlockStart) { +void WAL::logAbort(main::ClientContext* /*context*/, uint64_t abortedBlockStart) { DASSERT(!readOnly); std::unique_lock lck{mtx}; throwIfPoisonedNoLock(); // The aborted transaction counts as an active writer until it is rolled back, and a // checkpoint rotates or retires the WAL only once no writer is active, so the file holding its // COMMIT is still the open one: dryReplay matches an ABORT only within one file. - DASSERT(serializer != nullptr && abortedBlockStart < serializer->getWriter()->getSize()); - initWriter(context); + if (serializer == nullptr || abortedBlockStart >= serializer->getWriter()->getSize()) { + throw RuntimeException( + "Cannot log an ABORT record: the WAL file holding its COMMIT is no longer open."); + } addNewWALRecordNoLock(AbortRecord{abortedBlockStart}); flushAndSyncNoLock(); } diff --git a/src/storage/wal/wal_replayer.cpp b/src/storage/wal/wal_replayer.cpp index e289c07035..364a00e08d 100644 --- a/src/storage/wal/wal_replayer.cpp +++ b/src/storage/wal/wal_replayer.cpp @@ -951,7 +951,6 @@ WALReplayer::WALReplayInfo WALReplayer::dryReplay(FileInfo& fileInfo, bool throw WALReplayInfo replayInfo; std::optional openBlockStart; std::unordered_map committedBlockEnds; - std::vector abortedBlockStarts; try { Deserializer deserializer = initDeserializer(fileInfo, clientContext, enableChecksums); @@ -972,10 +971,18 @@ WALReplayer::WALReplayInfo WALReplayer::dryReplay(FileInfo& fileInfo, bool throw openBlockStart = recordStart; } break; case WALRecordType::ABORT_RECORD: { - // Ends a durable unit like a COMMIT, so a torn-tail truncation keeps it. - replayInfo.isLastRecordCheckpoint = false; + // A failed commit logs its ABORT in the file of its COMMIT, and no CHECKPOINT + // record comes between them: a checkpoint waits for the rollback. An ABORT naming + // no committed block after the last CHECKPOINT is ignored and ends nothing, so a + // WAL ending in a CHECKPOINT still does. A valid one ends a durable unit like a + // COMMIT, so a torn-tail truncation keeps it. + const auto start = walRecord->constCast().abortedBlockStart; + const auto block = committedBlockEnds.find(start); + if (block == committedBlockEnds.end() || start < replayInfo.lastCheckpointOffset) { + break; + } replayInfo.offsetDeserialized = getReadOffset(deserializer, enableChecksums); - abortedBlockStarts.push_back(walRecord->constCast().abortedBlockStart); + replayInfo.abortedBlocks.push_back({start, block->second}); } break; case WALRecordType::CHECKPOINT_RECORD: { // A CHECKPOINT record need not be final: a pinned checkpoint skips WAL @@ -1019,13 +1026,6 @@ WALReplayer::WALReplayInfo WALReplayer::dryReplay(FileInfo& fileInfo, bool throw throw; } } - // An ABORT record naming no committed transaction in this file is ignored. - for (const auto start : abortedBlockStarts) { - const auto block = committedBlockEnds.find(start); - if (block != committedBlockEnds.end()) { - replayInfo.abortedBlocks.push_back({start, block->second}); - } - } std::sort(replayInfo.abortedBlocks.begin(), replayInfo.abortedBlocks.end(), [](const AbortedBlock& a, const AbortedBlock& b) { return a.start < b.start; }); return replayInfo; diff --git a/test/transaction/checkpoint_test.cpp b/test/transaction/checkpoint_test.cpp index 269af81455..94de3df6fb 100644 --- a/test/transaction/checkpoint_test.cpp +++ b/test/transaction/checkpoint_test.cpp @@ -1434,6 +1434,40 @@ INSTANTIATE_TEST_SUITE_P(ShadowID, LegacyMainWALDatabaseIDTest, return std::string{}; }); +// An ABORT record only ever follows its own COMMIT, after the last CHECKPOINT record. One that +// names no such block is ignored and does not end the WAL: a WAL that ends in a CHECKPOINT whose +// shadow is not applied yet still recovers through that shadow, instead of replaying the records +// before an earlier pinned CHECKPOINT a second time. +TEST_F(CheckpointRetryAfterFailureTest, StrayAbortRecordAfterCheckpointIsIgnored) { + if (inMemMode || systemConfig->checkpointThreshold == 0) { + GTEST_SKIP(); + } + insertNodes(0, 50); + auto reader = std::make_unique(database.get()); + ASSERT_TRUE(reader->query("BEGIN TRANSACTION READ ONLY;")->isSuccess()); + checkpoint(); + const auto walPath = StorageUtils::getWALFilePath(databasePath); + const auto blockBeforeLastCheckpoint = std::filesystem::file_size(walPath); + insertNodes(50, 100); + failCheckpointWith(); + const bool enableChecksums = getClientContext(*conn)->getDBConfig()->enableChecksums; + reader.reset(); + conn.reset(); + database.reset(); + std::ofstream wal{walPath, std::ios::binary | std::ios::app}; + for (const auto start : {uint64_t{1}, blockBeforeLastCheckpoint}) { + const auto record = serializeCheckpointRecord(AbortRecord{start}); + wal.write(reinterpret_cast(record.data.get()), record.size); + if (enableChecksums) { + const auto recordChecksum = checksum(record.data.get(), record.size); + wal.write(reinterpret_cast(&recordChecksum), sizeof(recordChecksum)); + } + } + wal.close(); + createDBAndConn(); + checkNodes(100); +} + TEST_F(CheckpointRetryAfterFailureTest, LegacyMainShadowRejectsReplacementDatabase) { if (inMemMode || systemConfig->checkpointThreshold == 0) { GTEST_SKIP(); diff --git a/test/transaction/wal_test.cpp b/test/transaction/wal_test.cpp index f4b31e0cff..44fd766818 100644 --- a/test/transaction/wal_test.cpp +++ b/test/transaction/wal_test.cpp @@ -307,6 +307,28 @@ TEST_F(WalTest, AbortSyncFailurePoisonsWAL) { EXPECT_THROW(wal.throwIfPoisoned(), RuntimeException); } +// Recovery matches an ABORT record only within the file of its COMMIT, so one whose COMMIT is not +// in the open WAL file is refused, and the failed commit's caller poisons the WAL. +TEST_F(WalTest, AbortOutsideTheOpenWALFileIsRefused) { + VirtualFileSystem vfs; + vfs.registerFileSystem(std::make_unique(false /* failSync */)); + lbug::storage::WAL wal("failing-sync://db", false /* readOnly */, false /* enableChecksums */, + &vfs); + EXPECT_THROW(wal.logAbort(conn->getClientContext(), 0), RuntimeException); + + lbug::storage::LocalWAL localWAL(*lbug::storage::MemoryManager::Get(*conn->getClientContext()), + false /* enableChecksums */); + localWAL.logLoadExtension("" /* main */, "dummy"); + localWAL.logCommit(); + uint64_t walCommitSequence = 0; + uint64_t blockStart = 0; + ASSERT_NO_THROW( + wal.logCommittedWAL(localWAL, conn->getClientContext(), walCommitSequence, &blockStart)); + EXPECT_THROW(wal.logAbort(conn->getClientContext(), blockStart + localWAL.getSize()), + RuntimeException); + EXPECT_NO_THROW(wal.logAbort(conn->getClientContext(), blockStart)); +} + TEST_F(WalTest, FailureAfterCheckpointRenamePoisonsWAL) { VirtualFileSystem vfs; auto failingFS = std::make_unique(false /* failSync */); From e1263628b999b4cb55313b120ae0cabf66a7b36b Mon Sep 17 00:00:00 2001 From: Darek Date: Fri, 30 Oct 2026 15:38:05 +0100 Subject: [PATCH 4/8] refactor(txn): say which ABORT records recovery ignores, and drop unused context parameters The dry-pass and test comments said "after the last CHECKPOINT" where they mean the last one before the ABORT. WAL::logAbort documents its refusal, and neither it nor logAbortOfDurableCommit needs the client context any more. docs/checkpoint_recovery.md states where an ABORT lands, which ones recovery ignores, and that a failed COMMIT stays unapplied once its ABORT is durable. Co-Authored-By: Claude Opus 5.5 --- docs/checkpoint_recovery.md | 8 ++++++-- src/include/storage/wal/wal.h | 6 ++++-- src/include/transaction/transaction_manager.h | 3 +-- src/storage/wal/wal.cpp | 2 +- src/storage/wal/wal_replayer.cpp | 6 +++--- src/transaction/transaction_manager.cpp | 8 ++++---- test/transaction/checkpoint_test.cpp | 2 +- test/transaction/wal_test.cpp | 9 ++++----- 8 files changed, 24 insertions(+), 20 deletions(-) diff --git a/docs/checkpoint_recovery.md b/docs/checkpoint_recovery.md index 259a54b4ae..a53228a3cf 100644 --- a/docs/checkpoint_recovery.md +++ b/docs/checkpoint_recovery.md @@ -37,8 +37,12 @@ and recovery finishes the checkpoint. A transaction whose changes cannot be applied after its `COMMIT` record is durable, for example because the buffer pool is full, is rolled back in memory and gets an `ABORT` record naming the WAL offset where its records start. Recovery skips the transactions such records name, so a `COMMIT` -that reported an error is never applied. If the `ABORT` record cannot be written either, the -database refuses further writes until it is reopened, and recovery then applies the transaction. +that reported an error is not applied once its `ABORT` is durable. If the `ABORT` record cannot be +written either, the database refuses further writes until it is reopened, and recovery then +applies the transaction. The `ABORT` always lands in the WAL file of its `COMMIT` with no +`CHECKPOINT` record between them, since a checkpoint waits for the failed transaction's rollback; +recovery ignores an `ABORT` that names no transaction committed in its file after the last +`CHECKPOINT` before it. ## Checkpoint bundle format diff --git a/src/include/storage/wal/wal.h b/src/include/storage/wal/wal.h index d154757a9f..ed4c0b42d2 100644 --- a/src/include/storage/wal/wal.h +++ b/src/include/storage/wal/wal.h @@ -41,8 +41,10 @@ class WAL { void logCommittedWAL(LocalWAL& localWAL, main::ClientContext* context, uint64_t& commitSequence, uint64_t* blockStart = nullptr); // Logs and syncs that the transaction whose records begin at abortedBlockStart was not - // applied after its COMMIT record became durable, so recovery skips it. - void logAbort(main::ClientContext* context, uint64_t abortedBlockStart); + // applied after its COMMIT record became durable, so recovery skips it. Throws when no WAL + // file is open or the offset lies past its end: recovery matches an ABORT only within the + // file of its COMMIT. + void logAbort(uint64_t abortedBlockStart); void logAndFlushCheckpoint(main::ClientContext* context); // Renames the active WAL to the frozen checkpoint WAL. Returns false if there is nothing to diff --git a/src/include/transaction/transaction_manager.h b/src/include/transaction/transaction_manager.h index fc48a36e24..bc01966dc1 100644 --- a/src/include/transaction/transaction_manager.h +++ b/src/include/transaction/transaction_manager.h @@ -75,8 +75,7 @@ class TransactionManager { private: bool hasNoActiveTransactions() const; void checkpointNoLock(main::ClientContext& clientContext); - void logAbortOfDurableCommit(main::ClientContext& clientContext, uint64_t walBlockStart, - const std::string& publishError); + void logAbortOfDurableCommit(uint64_t walBlockStart, const std::string& publishError); // Try to checkpoint without blocking. Returns immediately if another checkpoint is in // progress. Used by auto-checkpoint after commit. void tryCheckpoint(main::ClientContext& clientContext); diff --git a/src/storage/wal/wal.cpp b/src/storage/wal/wal.cpp index 423a8c0d1f..8cd5dbcbaa 100644 --- a/src/storage/wal/wal.cpp +++ b/src/storage/wal/wal.cpp @@ -61,7 +61,7 @@ void WAL::logCommittedWAL(LocalWAL& localWAL, main::ClientContext* context, } // Holds the WAL lock across the sync, like the CHECKPOINT record: this runs only after a failure. -void WAL::logAbort(main::ClientContext* /*context*/, uint64_t abortedBlockStart) { +void WAL::logAbort(uint64_t abortedBlockStart) { DASSERT(!readOnly); std::unique_lock lck{mtx}; throwIfPoisonedNoLock(); diff --git a/src/storage/wal/wal_replayer.cpp b/src/storage/wal/wal_replayer.cpp index 364a00e08d..22564f62f4 100644 --- a/src/storage/wal/wal_replayer.cpp +++ b/src/storage/wal/wal_replayer.cpp @@ -973,9 +973,9 @@ WALReplayer::WALReplayInfo WALReplayer::dryReplay(FileInfo& fileInfo, bool throw case WALRecordType::ABORT_RECORD: { // A failed commit logs its ABORT in the file of its COMMIT, and no CHECKPOINT // record comes between them: a checkpoint waits for the rollback. An ABORT naming - // no committed block after the last CHECKPOINT is ignored and ends nothing, so a - // WAL ending in a CHECKPOINT still does. A valid one ends a durable unit like a - // COMMIT, so a torn-tail truncation keeps it. + // no block committed after the last CHECKPOINT before it is ignored and ends + // nothing, so a WAL ending in a CHECKPOINT still does. A valid one ends a durable + // unit like a COMMIT, so a torn-tail truncation keeps it. const auto start = walRecord->constCast().abortedBlockStart; const auto block = committedBlockEnds.find(start); if (block == committedBlockEnds.end() || start < replayInfo.lastCheckpointOffset) { diff --git a/src/transaction/transaction_manager.cpp b/src/transaction/transaction_manager.cpp index 96936e50db..f2aa3966c2 100644 --- a/src/transaction/transaction_manager.cpp +++ b/src/transaction/transaction_manager.cpp @@ -112,10 +112,10 @@ Transaction* TransactionManager::beginTransaction(main::ClientContext& clientCon // The transaction's COMMIT record is durable, but its changes were not applied and the caller // rolls them back: without an ABORT record recovery would apply them. If even that cannot be // logged, refusing writes keeps a retry from applying them twice before a reopen does. -void TransactionManager::logAbortOfDurableCommit(main::ClientContext& clientContext, - uint64_t walBlockStart, const std::string& publishError) { +void TransactionManager::logAbortOfDurableCommit(uint64_t walBlockStart, + const std::string& publishError) { try { - wal.logAbort(&clientContext, walBlockStart); + wal.logAbort(walBlockStart); } catch (const std::exception& e) { // A failed sync has poisoned the WAL already; one that failed before it has not. wal.poison(std::format("A transaction failed after its COMMIT record was written, and " @@ -204,7 +204,7 @@ void TransactionManager::commit(main::ClientContext& clientContext, Transaction* // so no checkpoint can rotate the WAL between its COMMIT and this ABORT. A failed WAL sync // logs no ABORT: it has poisoned the WAL, and a reopen applies the COMMIT if it is durable. if (publishError && walCommitSequence != 0) { - logAbortOfDurableCommit(clientContext, walBlockStart, *publishError); + logAbortOfDurableCommit(walBlockStart, *publishError); } throw; } diff --git a/test/transaction/checkpoint_test.cpp b/test/transaction/checkpoint_test.cpp index 94de3df6fb..e87aef6b4b 100644 --- a/test/transaction/checkpoint_test.cpp +++ b/test/transaction/checkpoint_test.cpp @@ -1434,7 +1434,7 @@ INSTANTIATE_TEST_SUITE_P(ShadowID, LegacyMainWALDatabaseIDTest, return std::string{}; }); -// An ABORT record only ever follows its own COMMIT, after the last CHECKPOINT record. One that +// An ABORT record only ever follows its own COMMIT, with no CHECKPOINT between them. One that // names no such block is ignored and does not end the WAL: a WAL that ends in a CHECKPOINT whose // shadow is not applied yet still recovers through that shadow, instead of replaying the records // before an earlier pinned CHECKPOINT a second time. diff --git a/test/transaction/wal_test.cpp b/test/transaction/wal_test.cpp index 44fd766818..df0be3c09a 100644 --- a/test/transaction/wal_test.cpp +++ b/test/transaction/wal_test.cpp @@ -303,7 +303,7 @@ TEST_F(WalTest, AbortSyncFailurePoisonsWAL) { EXPECT_GT(blockStart, 0); failingFSPtr->setFailSync(true); - EXPECT_THROW(wal.logAbort(conn->getClientContext(), blockStart), RuntimeException); + EXPECT_THROW(wal.logAbort(blockStart), RuntimeException); EXPECT_THROW(wal.throwIfPoisoned(), RuntimeException); } @@ -314,7 +314,7 @@ TEST_F(WalTest, AbortOutsideTheOpenWALFileIsRefused) { vfs.registerFileSystem(std::make_unique(false /* failSync */)); lbug::storage::WAL wal("failing-sync://db", false /* readOnly */, false /* enableChecksums */, &vfs); - EXPECT_THROW(wal.logAbort(conn->getClientContext(), 0), RuntimeException); + EXPECT_THROW(wal.logAbort(0), RuntimeException); lbug::storage::LocalWAL localWAL(*lbug::storage::MemoryManager::Get(*conn->getClientContext()), false /* enableChecksums */); @@ -324,9 +324,8 @@ TEST_F(WalTest, AbortOutsideTheOpenWALFileIsRefused) { uint64_t blockStart = 0; ASSERT_NO_THROW( wal.logCommittedWAL(localWAL, conn->getClientContext(), walCommitSequence, &blockStart)); - EXPECT_THROW(wal.logAbort(conn->getClientContext(), blockStart + localWAL.getSize()), - RuntimeException); - EXPECT_NO_THROW(wal.logAbort(conn->getClientContext(), blockStart)); + EXPECT_THROW(wal.logAbort(blockStart + localWAL.getSize()), RuntimeException); + EXPECT_NO_THROW(wal.logAbort(blockStart)); } TEST_F(WalTest, FailureAfterCheckpointRenamePoisonsWAL) { From 8bd8876db13535a12031047968c42242149dc0a8 Mon Sep 17 00:00:00 2001 From: Darek Date: Sat, 31 Oct 2026 03:22:52 +0100 Subject: [PATCH 5/8] docs(storage): wrap a long line in the downgrade paragraph Co-Authored-By: Claude Opus 5.5 --- docs/checkpoint_recovery.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/checkpoint_recovery.md b/docs/checkpoint_recovery.md index 59f5e08169..24db25859d 100644 --- a/docs/checkpoint_recovery.md +++ b/docs/checkpoint_recovery.md @@ -123,7 +123,8 @@ as `UPDATE_SEQUENCE_NAMED` or `ABORT`, is no safer: with the default non-throwin configuration the decoding error is treated like a corrupt WAL tail — the older build replays the committed prefix and truncates the WAL there, silently discarding that transaction and every transaction recorded after it. For `ABORT`, which marks a transaction that failed after its -`COMMIT` record was written, the older build also applies the transaction it undoes. Setting `throwOnWalReplayFailure` to true rejects an unknown record before that WAL +`COMMIT` record was written, the older build also applies the transaction it undoes. Setting +`throwOnWalReplayFailure` to true rejects an unknown record before that WAL is applied, but it still does not make downgrading safe: a WAL containing only recognized record types passes validation, and its owner trailers are ignored. This is why the successful `CHECKPOINT`, not a clean close, is the downgrade gate. From b13d91fa10fe8ba733750daecf039c391ae7e744 Mon Sep 17 00:00:00 2001 From: Darek Date: Fri, 30 Oct 2026 13:50:03 +0100 Subject: [PATCH 6/8] test(storage): skip an aborted commit after a pinned checkpoint A COMMIT that a later ABORT undoes still proves the pinned checkpoint before it finished, so recovery skips through the CHECKPOINT record despite the next checkpoint's leftover shadow and leaves the aborted transaction out. Needs both the pinned-checkpoint recovery and the ABORT record; it goes into whichever of the two lands second. Co-Authored-By: Claude Opus 5.5 --- test/transaction/checkpoint_test.cpp | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/test/transaction/checkpoint_test.cpp b/test/transaction/checkpoint_test.cpp index 8bad96eb9d..1cc13c9ab1 100644 --- a/test/transaction/checkpoint_test.cpp +++ b/test/transaction/checkpoint_test.cpp @@ -6023,6 +6023,27 @@ TEST_F(PinnedCheckpointCrashTest, LeftoverShadowKeepsCommitsAfterPinnedCheckpoin EXPECT_EQ(ids(), (std::vector{1, 2, 3, 4, 5})); } +// A COMMIT that a later ABORT undoes still shows that the pinned checkpoint before it finished: +// its writer got past the write gate after the CHECKPOINT record. Recovery skips through that +// record despite the next checkpoint's leftover shadow, and the ABORT keeps the transaction out. +// The ABORT is appended after the crash, where a failed publish would have logged it. +TEST_F(PinnedCheckpointCrashTest, AbortedCommitAfterPinnedCheckpointIsSkipped) { + query("CREATE NODE TABLE P(id INT64 PRIMARY KEY);"); + query("CHECKPOINT;"); + createRows(1, 3); + pinWAL(); + query("CHECKPOINT;"); + const auto abortedStart = + std::filesystem::file_size(StorageUtils::getWALFilePath(databasePath)); + createRows(4, 5); + failCheckpoint(); + ASSERT_TRUE(std::filesystem::exists(shadowPath())); + crash(); + appendToWAL(AbortRecord{abortedStart}); + createDBAndConn(); + EXPECT_EQ(ids(), (std::vector{1, 2, 3})); +} + TEST_F(PinnedCheckpointCrashTest, LeftoverShadowKeepsTableCreatedBeforePinnedCheckpoint) { query("CREATE NODE TABLE P(id INT64 PRIMARY KEY);"); createRows(1, 3); From 664f62d9cef6e3c741004c78c85feca5fd01dea7 Mon Sep 17 00:00:00 2001 From: Darek Date: Mon, 2 Nov 2026 03:43:48 +0100 Subject: [PATCH 7/8] style(txn): drop a no-op std::move in AbortRecord::deserialize clang-tidy flags the move of a uint64_t (performance-move-const-arg). Co-Authored-By: Claude Opus 5.5 --- src/storage/wal/records/abort_record.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/storage/wal/records/abort_record.cpp b/src/storage/wal/records/abort_record.cpp index ecc84a09db..939bb9eb21 100644 --- a/src/storage/wal/records/abort_record.cpp +++ b/src/storage/wal/records/abort_record.cpp @@ -25,7 +25,7 @@ std::unique_ptr AbortRecord::deserialize(Deserializer& deserializer deserializer.deserializeValue(abortedBlockStart); } - return std::make_unique(std::move(abortedBlockStart)); + return std::make_unique(abortedBlockStart); } } // namespace storage From be48c8e555dfc3e59b60d0823c96a774773c20bc Mon Sep 17 00:00:00 2001 From: Darek Date: Mon, 2 Nov 2026 03:54:58 +0100 Subject: [PATCH 8/8] docs(txn): mention ABORT records in two recovery comments An ABORT never clears the flag that a WAL ends in a CHECKPOINT record, and the shared replay helper skips the transactions such records name. Co-Authored-By: Claude Opus 5.5 --- src/storage/wal/wal_replayer.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/storage/wal/wal_replayer.cpp b/src/storage/wal/wal_replayer.cpp index a936a0a7ee..d739999c51 100644 --- a/src/storage/wal/wal_replayer.cpp +++ b/src/storage/wal/wal_replayer.cpp @@ -659,7 +659,8 @@ void WALReplayer::removeGraphCheckpointShadow(StorageManager& storageManager) co // Replays the records dryReplay found committed, then the graph WALs they queued. With // skipThroughLastCheckpoint, only the records after the last CHECKPOINT replay; otherwise only // those before the first. Both bounds exclude every CHECKPOINT record structurally, so none ever -// reaches replayWALRecord, which rejects them. Returns the offset where replay stopped. +// reaches replayWALRecord, which rejects them. The records of a transaction an ABORT names are +// skipped. Returns the offset where replay stopped. uint64_t WALReplayer::replayCommittedRecords(FileInfo& fileInfo, const WALReplayInfo& replayInfo, bool skipThroughLastCheckpoint, bool enableChecksums) const { Deserializer deserializer = initDeserializer(fileInfo, clientContext, enableChecksums); @@ -1029,8 +1030,8 @@ WALReplayer::WALReplayInfo WALReplayer::dryReplay(FileInfo& fileInfo, bool throw // 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. + // Remember the offsets and keep scanning instead of failing; any later record + // except an ABORT clears the last-record flag below. replayInfo.isLastRecordCheckpoint = true; replayInfo.checkpointFormatVersion = walRecord->constCast().bundleFormatVersion;