From 313c9c5730dd1c32754a9267e8c124f46376cd0d Mon Sep 17 00:00:00 2001 From: Darek Date: Thu, 22 Oct 2026 18:59:54 +0200 Subject: [PATCH 1/6] fix(txn): don't fail a committed write when its automatic checkpoint fails A checkpoint that failed after a commit was raised from the statement that committed, so callers that retry failed writes applied them twice. Buffer pool pressure during the checkpoint, or another connection's open write transaction outlasting the checkpoint's wait, was enough. checkpointNoLock throws CheckpointException only with the checkpoint rolled back or not started, or with the WAL poisoned. commit() now drops that failure for the automatic checkpoint: the commit is in the WAL, and the next write commit retries. A poisoned WAL, any other exception, and the forced checkpoint after COPY FROM or CREATE INDEX on a large ART, whose data is not in the WAL, still fail the statement. Co-Authored-By: Claude Opus 5.5 --- src/include/storage/wal/wal.h | 1 + src/storage/wal/wal.cpp | 5 + src/transaction/transaction_manager.cpp | 22 +++- test/transaction/checkpoint_test.cpp | 149 ++++++++++++++++++++++++ 4 files changed, 173 insertions(+), 4 deletions(-) diff --git a/src/include/storage/wal/wal.h b/src/include/storage/wal/wal.h index c0aaebaa42..b421fe034d 100644 --- a/src/include/storage/wal/wal.h +++ b/src/include/storage/wal/wal.h @@ -63,6 +63,7 @@ class WAL { uint64_t getFrozenWALFileSize(); bool mayHaveCheckpointRecord(); void throwIfPoisoned(); + bool isPoisoned(); void poison(const std::string& reason); static WAL* Get(const main::ClientContext& context); diff --git a/src/storage/wal/wal.cpp b/src/storage/wal/wal.cpp index a4328e5e2f..e9050aefd5 100644 --- a/src/storage/wal/wal.cpp +++ b/src/storage/wal/wal.cpp @@ -367,6 +367,11 @@ void WAL::throwIfPoisoned() { throwIfPoisonedNoLock(); } +bool WAL::isPoisoned() { + std::unique_lock lck{mtx}; + return poisoned; +} + void WAL::poison(const std::string& reason) { std::unique_lock lck{mtx}; poisonNoLock(reason); diff --git a/src/transaction/transaction_manager.cpp b/src/transaction/transaction_manager.cpp index e469a575f0..0ac13b71f6 100644 --- a/src/transaction/transaction_manager.cpp +++ b/src/transaction/transaction_manager.cpp @@ -52,6 +52,13 @@ class QueryProgressScope { std::optional queryID; }; +[[noreturn]] void throwPostCommitCheckpointFailure(const std::exception& e) { + throw CheckpointException{ + std::format("Transaction committed successfully, but the post-commit checkpoint failed. " + "The committed data is durable and will be recovered on restart: {}", + e.what())}; +} + } // namespace Transaction* TransactionManager::beginTransaction(main::ClientContext& clientContext, @@ -181,11 +188,16 @@ void TransactionManager::commit(main::ClientContext& clientContext, Transaction* } else if (shouldAutoCheckpoint) { tryCheckpoint(clientContext); } + } catch (const CheckpointException& e) { + // Unless the WAL is poisoned, a failed automatic checkpoint left nothing behind: the commit + // is in the WAL and the next write commit retries. COPY FROM and CREATE INDEX on a large + // ART rely on their forced checkpoint instead of the WAL, so that failure still fails the + // statement. + if (shouldForceCheckpoint || wal.isPoisoned()) { + throwPostCommitCheckpointFailure(e); + } } catch (const std::exception& e) { - throw CheckpointException{std::format( - "Transaction committed successfully, but the post-commit checkpoint failed. " - "The committed data is durable and will be recovered on restart: {}", - e.what())}; + throwPostCommitCheckpointFailure(e); } } @@ -353,6 +365,8 @@ void TransactionManager::tryCheckpoint(main::ClientContext& clientContext) { checkpointNoLock(clientContext); } +// Throws CheckpointException only with the checkpoint rolled back or not started, or with the WAL +// poisoned; commit() relies on this to drop failed automatic checkpoints. void TransactionManager::checkpointNoLock(main::ClientContext& clientContext) { QueryProgressScope progress{clientContext, 0.01}; // Capture the retention horizon before acquiring the write gate. Taking the diff --git a/test/transaction/checkpoint_test.cpp b/test/transaction/checkpoint_test.cpp index 269af81455..4d71aa603e 100644 --- a/test/transaction/checkpoint_test.cpp +++ b/test/transaction/checkpoint_test.cpp @@ -814,6 +814,155 @@ TEST_F(CheckpointRetryAfterFailureTest, PartitionShadowsSurviveUntilMainWALRetir ASSERT_EQ(result->getNext()->getValue(0)->getValue(), 1); } +// Checkpoints automatically after every commit. +class PostCommitCheckpointFailureTest : public CheckpointRetryAfterFailureTest { +public: + void SetUp() override { + CheckpointRetryAfterFailureTest::SetUp(); + checkpointAfterEveryCommit(); + } + + void checkpointAfterEveryCommit() const { + ASSERT_TRUE(conn->query("CALL force_checkpoint_on_close=false;")->isSuccess()); + ASSERT_TRUE(conn->query("CALL auto_checkpoint=true;")->isSuccess()); + ASSERT_TRUE(conn->query("CALL checkpoint_threshold=0;")->isSuccess()); + } + + // Counts checkpoint attempts; each one fails while failCheckpoints is set. + template + void countCheckpoints() { + FlakyCheckpointer counting( + [this](main::ClientContext& context) -> std::unique_ptr { + numCheckpoints++; + if (failCheckpoints) { + return std::make_unique(context); + } + return std::make_unique(context); + }); + counting.setCheckpointer(*getClientContext(*conn)); + } + + uint64_t numCheckpoints = 0; + bool failCheckpoints = true; +}; + +TEST_F(PostCommitCheckpointFailureTest, FailedAutoCheckpointDoesNotFailCommit) { + if (inMemMode) { + GTEST_SKIP(); + } + countCheckpoints(); + insertNodes(0, 1); + ASSERT_TRUE(conn->query("BEGIN TRANSACTION;")->isSuccess()); + insertNodes(1, 2); + auto res = conn->query("COMMIT;"); + ASSERT_TRUE(res->isSuccess()) << res->getErrorMessage(); + ASSERT_EQ(numCheckpoints, 2u); + // Reopen without checkpointing: the WAL alone recovers both commits. + res.reset(); + createDBAndConn(); + checkNodes(2); +} + +TEST_F(PostCommitCheckpointFailureTest, NextCommitRetriesFailedAutoCheckpoint) { + if (inMemMode) { + GTEST_SKIP(); + } + countCheckpoints(); + insertNodes(0, 1); + EXPECT_GT(WAL::Get(*getClientContext(*conn))->getFileSize(), 0u); + failCheckpoints = false; + insertNodes(1, 2); + ASSERT_EQ(numCheckpoints, 2u); + EXPECT_EQ(WAL::Get(*getClientContext(*conn))->getFileSize(), 0u); + createDBAndConn(); + checkNodes(2); +} + +TEST_F(PostCommitCheckpointFailureTest, AutoCheckpointBlockedByOpenWriterDoesNotFailCommit) { + if (inMemMode) { + GTEST_SKIP(); + } + conn.reset(); + createDB(10000 /* checkpoint wait timeout in microseconds */); + conn = std::make_unique(database.get()); + checkpointAfterEveryCommit(); + ASSERT_TRUE(conn->query("CALL debug_enable_multi_writes=true;")->isSuccess()); + auto context = getClientContext(*conn); + main::Connection other{database.get()}; + ASSERT_TRUE(other.query("BEGIN TRANSACTION;")->isSuccess()); + ASSERT_TRUE(other.query("CREATE (a:test {id: 1, name: 'name_1'});")->isSuccess()); + // This commit's checkpoint times out waiting for the other connection's write transaction. + insertNodes(0, 1); + EXPECT_GT(WAL::Get(*context)->getFileSize(), 0u); + auto res = other.query("COMMIT;"); + ASSERT_TRUE(res->isSuccess()) << res->getErrorMessage(); + EXPECT_EQ(WAL::Get(*context)->getFileSize(), 0u); + checkNodes(2); +} + +TEST_F(PostCommitCheckpointFailureTest, AutoCheckpointThatPoisonsWALFailsCommit) { + if (inMemMode) { + GTEST_SKIP(); + } + countCheckpoints(); + auto res = conn->query("CREATE (a:test {id: 0, name: 'name_0'});"); + ASSERT_FALSE(res->isSuccess()); + EXPECT_NE(res->getErrorMessage().find("Transaction committed successfully"), std::string::npos) + << res->getErrorMessage(); + EXPECT_NE(res->getErrorMessage().find("refuses further writes until restart"), + std::string::npos) + << res->getErrorMessage(); + ASSERT_EQ(numCheckpoints, 1u); + res.reset(); + createDBAndConn(); + checkNodes(1); +} + +TEST_F(PostCommitCheckpointFailureTest, FailedForcedCheckpointFailsCopy) { + if (inMemMode) { + GTEST_SKIP(); + } + countCheckpoints(); + auto res = conn->query("COPY test FROM (UNWIND range(0, 9) AS i RETURN i, 'n');"); + ASSERT_FALSE(res->isSuccess()); + EXPECT_NE(res->getErrorMessage().find("Transaction committed successfully"), std::string::npos) + << res->getErrorMessage(); + ASSERT_EQ(numCheckpoints, 1u); +} + +TEST_F(PostCommitCheckpointFailureTest, FailedForcedCheckpointFailsCreateIndex) { + if (inMemMode) { + GTEST_SKIP(); + } + insertNodes(0, 2); + countCheckpoints(); + setenv("LBUG_CREATE_INDEX_WAL_THRESHOLD", "1", 1); + auto res = conn->query("CREATE ART INDEX test_name_idx FOR (n:test) ON (n.name);"); + unsetenv("LBUG_CREATE_INDEX_WAL_THRESHOLD"); + ASSERT_FALSE(res->isSuccess()); + EXPECT_NE(res->getErrorMessage().find("Transaction committed successfully"), std::string::npos) + << res->getErrorMessage(); + ASSERT_EQ(numCheckpoints, 1u); +} + +TEST_F(PostCommitCheckpointFailureTest, FailedCheckpointCleanupFailsCommit) { + if (inMemMode) { + GTEST_SKIP(); + } + countCheckpoints(); + auto res = conn->query("CREATE (a:test {id: 0, name: 'name_0'});"); + ASSERT_FALSE(res->isSuccess()); + EXPECT_NE(res->getErrorMessage().find("Transaction committed successfully"), std::string::npos) + << res->getErrorMessage(); + EXPECT_NE(res->getErrorMessage().find("checkpoint interrupted before WAL retirement"), + std::string::npos) + << res->getErrorMessage(); + ASSERT_EQ(numCheckpoints, 1u); + res.reset(); + createDBAndConn(); + checkNodes(1); +} + class CommittedPartitionMissingBaseTest : public CheckpointRetryAfterFailureTest, public ::testing::WithParamInterface {}; From 6048e521a53b3795ab40ee17d4444960ff8e6019 Mon Sep 17 00:00:00 2001 From: Darek Date: Thu, 22 Oct 2026 19:01:03 +0200 Subject: [PATCH 2/6] fix(txn): stop promising durability when a forced checkpoint fails A COPY FROM or a CREATE INDEX on a large ART writes its data without the WAL and relies on the checkpoint it forces at commit. When that checkpoint failed, the error still said the committed data was durable and would be recovered on restart, but a crash before a later successful checkpoint loses it. The forced-path error now says the changes may be lost on restart unless a later CHECKPOINT succeeds. The tests check that such a CHECKPOINT persists the COPY's rows and the index. Co-Authored-By: Claude Opus 5.5 --- src/transaction/transaction_manager.cpp | 13 ++++++++++--- test/transaction/checkpoint_test.cpp | 18 ++++++++++++++++-- 2 files changed, 26 insertions(+), 5 deletions(-) diff --git a/src/transaction/transaction_manager.cpp b/src/transaction/transaction_manager.cpp index 0ac13b71f6..814b4a2294 100644 --- a/src/transaction/transaction_manager.cpp +++ b/src/transaction/transaction_manager.cpp @@ -52,7 +52,14 @@ class QueryProgressScope { std::optional queryID; }; -[[noreturn]] void throwPostCommitCheckpointFailure(const std::exception& e) { +[[noreturn]] void throwPostCommitCheckpointFailure(const std::exception& e, bool forced) { + if (forced) { + throw CheckpointException{std::format( + "Transaction committed successfully, but the checkpoint it requires failed. Its " + "changes are not in the WAL and may be lost on restart unless a later CHECKPOINT " + "succeeds: {}", + e.what())}; + } throw CheckpointException{ std::format("Transaction committed successfully, but the post-commit checkpoint failed. " "The committed data is durable and will be recovered on restart: {}", @@ -194,10 +201,10 @@ void TransactionManager::commit(main::ClientContext& clientContext, Transaction* // ART rely on their forced checkpoint instead of the WAL, so that failure still fails the // statement. if (shouldForceCheckpoint || wal.isPoisoned()) { - throwPostCommitCheckpointFailure(e); + throwPostCommitCheckpointFailure(e, shouldForceCheckpoint); } } catch (const std::exception& e) { - throwPostCommitCheckpointFailure(e); + throwPostCommitCheckpointFailure(e, shouldForceCheckpoint); } } diff --git a/test/transaction/checkpoint_test.cpp b/test/transaction/checkpoint_test.cpp index 4d71aa603e..90a2cc85da 100644 --- a/test/transaction/checkpoint_test.cpp +++ b/test/transaction/checkpoint_test.cpp @@ -925,9 +925,15 @@ TEST_F(PostCommitCheckpointFailureTest, FailedForcedCheckpointFailsCopy) { countCheckpoints(); auto res = conn->query("COPY test FROM (UNWIND range(0, 9) AS i RETURN i, 'n');"); ASSERT_FALSE(res->isSuccess()); - EXPECT_NE(res->getErrorMessage().find("Transaction committed successfully"), std::string::npos) + EXPECT_NE( + res->getErrorMessage().find("may be lost on restart unless a later CHECKPOINT succeeds"), + std::string::npos) << res->getErrorMessage(); ASSERT_EQ(numCheckpoints, 1u); + failCheckpoints = false; + checkpoint(); + createDBAndConn(); + checkNodes(10); } TEST_F(PostCommitCheckpointFailureTest, FailedForcedCheckpointFailsCreateIndex) { @@ -940,9 +946,17 @@ TEST_F(PostCommitCheckpointFailureTest, FailedForcedCheckpointFailsCreateIndex) auto res = conn->query("CREATE ART INDEX test_name_idx FOR (n:test) ON (n.name);"); unsetenv("LBUG_CREATE_INDEX_WAL_THRESHOLD"); ASSERT_FALSE(res->isSuccess()); - EXPECT_NE(res->getErrorMessage().find("Transaction committed successfully"), std::string::npos) + EXPECT_NE( + res->getErrorMessage().find("may be lost on restart unless a later CHECKPOINT succeeds"), + std::string::npos) << res->getErrorMessage(); ASSERT_EQ(numCheckpoints, 1u); + failCheckpoints = false; + checkpoint(); + createDBAndConn(); + res = conn->query("CALL SHOW_INDEXES() WHERE index_name = 'test_name_idx' RETURN count(*);"); + ASSERT_TRUE(res->isSuccess()) << res->getErrorMessage(); + EXPECT_EQ(res->getNext()->getValue(0)->getValue(), 1); } TEST_F(PostCommitCheckpointFailureTest, FailedCheckpointCleanupFailsCommit) { From 82135e1c6c42d3878c4e2b42d97c61cf2fdf44f4 Mon Sep 17 00:00:00 2001 From: Darek Date: Fri, 23 Oct 2026 21:31:19 +0200 Subject: [PATCH 3/6] fix(storage): poison the WAL when a checkpoint rollback fails When a checkpoint failed and its rollback threw too, the free-page bookkeeping and the WAL rotation could stay half undone, and the engine kept accepting writes on that state. Checkpointer::rollback now releases the checkpoint lock files and poisons the WAL, as a failed cleanup already does, so the database refuses writes until restart. A beforeRollback hook, added after the existing virtual hooks, lets the test make the rollback fail. Co-Authored-By: Claude Opus 5.5 --- src/include/storage/checkpointer.h | 2 ++ src/storage/checkpointer.cpp | 19 ++++++++++++++- test/transaction/checkpoint_test.cpp | 36 ++++++++++++++++++++++++++++ 3 files changed, 56 insertions(+), 1 deletion(-) diff --git a/src/include/storage/checkpointer.h b/src/include/storage/checkpointer.h index fa829e9881..ffceee14fe 100644 --- a/src/include/storage/checkpointer.h +++ b/src/include/storage/checkpointer.h @@ -82,6 +82,8 @@ class Checkpointer { // the horizon pins the WAL. virtual void onWALRetired(bool walRotated); virtual void beforeGraphShadowApply(StorageManager& storageManager); + // Runs first in rollback(), so tests can make the rollback itself fail. + virtual void beforeRollback(); void applyShadowPagesForPartitionChildren(); void flushGraphShadowFiles(); void applyGraphShadowPages(); diff --git a/src/storage/checkpointer.cpp b/src/storage/checkpointer.cpp index 7f6e9e1818..e99283c6a8 100644 --- a/src/storage/checkpointer.cpp +++ b/src/storage/checkpointer.cpp @@ -578,6 +578,8 @@ void Checkpointer::onWALRetired(bool) {} void Checkpointer::beforeGraphShadowApply(StorageManager&) {} +void Checkpointer::beforeRollback() {} + void Checkpointer::logCheckpointAndApplyShadowPages(bool walRotated_) { auto& shadowFile = mainStorageManager->getShadowFile(); shadowFile.flushAll(mainStorageManager->getOrInitDatabaseID(clientContext)); @@ -633,10 +635,11 @@ void Checkpointer::applyShadowPagesForPartitionChildren() { } } -void Checkpointer::rollback() { +void Checkpointer::rollback() try { if (isInMemory) { return; } + beforeRollback(); // Any pages freed during the checkpoint are no longer freed for (const auto& target : checkpointTargets) { target.storageManager->rollbackCheckpoint(*target.catalog, &clientContext); @@ -661,6 +664,20 @@ void Checkpointer::rollback() { } } releaseCheckpointLocks(); +} catch (const std::exception& e) { + // A half-undone checkpoint leaves page and WAL state that later writes must not build on. + releaseCheckpointLocks(); + mainStorageManager->getWAL().poison(std::format("Checkpoint rollback failed: {}", e.what())); + throw common::RuntimeException(std::format( + "Checkpoint rollback failed; the database refuses further writes until restart. Original " + "error: {}", + e.what())); +} catch (...) { + releaseCheckpointLocks(); + mainStorageManager->getWAL().poison("Checkpoint rollback failed: unknown exception"); + throw common::RuntimeException( + "Checkpoint rollback failed; the database refuses further writes until restart. Original " + "error: unknown exception"); } bool Checkpointer::canAutoCheckpoint(const main::ClientContext& clientContext, diff --git a/test/transaction/checkpoint_test.cpp b/test/transaction/checkpoint_test.cpp index 90a2cc85da..6934507b80 100644 --- a/test/transaction/checkpoint_test.cpp +++ b/test/transaction/checkpoint_test.cpp @@ -366,6 +366,42 @@ TEST_F(CheckpointRetryAfterFailureTest, RetryFailsWithNewWritesThenReopen) { checkNodes(200); } +class FlakyCheckpointerFailsOnRollback final : public Checkpointer { +public: + explicit FlakyCheckpointerFailsOnRollback(main::ClientContext& context) + : Checkpointer(context) {} + +protected: + bool checkpointStorage() override { throw RuntimeException("checkpoint failed."); } + void beforeRollback() override { throw RuntimeException("rollback failed."); } +}; + +TEST_F(CheckpointRetryAfterFailureTest, FailedRollbackPoisonsWAL) { + if (inMemMode || systemConfig->checkpointThreshold == 0) { + GTEST_SKIP(); + } + insertNodes(0, 100); + FlakyCheckpointer flakyCheckpointer([](main::ClientContext& context) { + return std::make_unique(context); + }); + flakyCheckpointer.setCheckpointer(*getClientContext(*conn)); + auto res = conn->query("CHECKPOINT;"); + ASSERT_FALSE(res->isSuccess()); + EXPECT_NE(res->getErrorMessage().find("refuses further writes until restart"), + std::string::npos) + << res->getErrorMessage(); + checkNodes(100); + res = conn->query("CREATE (a:test {id: 100, name: 'name_100'});"); + ASSERT_FALSE(res->isSuccess()); + EXPECT_NE(res->getErrorMessage().find("panic state"), std::string::npos) + << res->getErrorMessage(); + EXPECT_NE(res->getErrorMessage().find("Checkpoint rollback failed"), std::string::npos) + << res->getErrorMessage(); + res.reset(); + createDBAndConn(); + checkNodes(100); +} + class FlakyCheckpointerFailsOnSerialization final : public Checkpointer { public: explicit FlakyCheckpointerFailsOnSerialization(main::ClientContext& context) From b9c44b4b296ad2cbfd943cb90ef48e818745a6af Mon Sep 17 00:00:00 2001 From: Darek Date: Fri, 23 Oct 2026 21:37:29 +0200 Subject: [PATCH 4/6] fix(txn): drop every failed automatic checkpoint after a commit A failed automatic checkpoint still failed the committed statement when it poisoned the WAL or when its cleanup failed, although the commit was in the WAL in both cases, so a client retrying after a restart applied the write twice. commit() now drops every failure of the automatic checkpoint and prints one warning to stderr with the WAL size and the reason. A rolled-back checkpoint is retried by a later write commit, and a poisoned WAL fails the next write, which really did not commit. The forced checkpoint after COPY FROM or CREATE INDEX on a large ART still fails the statement. WAL::isPoisoned() is removed again. Co-Authored-By: Claude Opus 5.5 --- src/include/storage/wal/wal.h | 1 - src/storage/wal/wal.cpp | 5 --- src/transaction/transaction_manager.cpp | 51 ++++++++++++------------- test/transaction/checkpoint_test.cpp | 24 +++++++----- 4 files changed, 39 insertions(+), 42 deletions(-) diff --git a/src/include/storage/wal/wal.h b/src/include/storage/wal/wal.h index b421fe034d..c0aaebaa42 100644 --- a/src/include/storage/wal/wal.h +++ b/src/include/storage/wal/wal.h @@ -63,7 +63,6 @@ class WAL { uint64_t getFrozenWALFileSize(); bool mayHaveCheckpointRecord(); void throwIfPoisoned(); - bool isPoisoned(); void poison(const std::string& reason); static WAL* Get(const main::ClientContext& context); diff --git a/src/storage/wal/wal.cpp b/src/storage/wal/wal.cpp index e9050aefd5..a4328e5e2f 100644 --- a/src/storage/wal/wal.cpp +++ b/src/storage/wal/wal.cpp @@ -367,11 +367,6 @@ void WAL::throwIfPoisoned() { throwIfPoisonedNoLock(); } -bool WAL::isPoisoned() { - std::unique_lock lck{mtx}; - return poisoned; -} - void WAL::poison(const std::string& reason) { std::unique_lock lck{mtx}; poisonNoLock(reason); diff --git a/src/transaction/transaction_manager.cpp b/src/transaction/transaction_manager.cpp index 814b4a2294..b0250a8fe8 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" @@ -52,18 +53,14 @@ class QueryProgressScope { std::optional queryID; }; -[[noreturn]] void throwPostCommitCheckpointFailure(const std::exception& e, bool forced) { - if (forced) { - throw CheckpointException{std::format( - "Transaction committed successfully, but the checkpoint it requires failed. Its " - "changes are not in the WAL and may be lost on restart unless a later CHECKPOINT " - "succeeds: {}", - e.what())}; +void warnAutoCheckpointFailed(WAL& wal, const std::exception& e) { + std::string walSize = "unknown"; + try { + walSize = std::to_string(wal.getFileSize()); + } catch (...) { // NOLINT: The statement has committed, so reporting must not fail it. } - throw CheckpointException{ - std::format("Transaction committed successfully, but the post-commit checkpoint failed. " - "The committed data is durable and will be recovered on restart: {}", - e.what())}; + std::cerr << "Warning: automatic checkpoint after a commit failed, WAL " << walSize + << " bytes: " << e.what() << std::endl; } } // namespace @@ -189,22 +186,26 @@ void TransactionManager::commit(main::ClientContext& clientContext, Transaction* // (commit/rollback) during the drain phase instead of deadlocking. The transaction has // already been removed from activeTransactions at this point, so any later failure is a // checkpoint failure, not a transaction failure that can be rolled back. - try { - if (shouldForceCheckpoint) { + if (shouldForceCheckpoint) { + try { checkpoint(clientContext); - } else if (shouldAutoCheckpoint) { - tryCheckpoint(clientContext); + } catch (const std::exception& e) { + // COPY FROM and CREATE INDEX on a large ART rely on this checkpoint instead of the WAL. + throw CheckpointException{std::format( + "Transaction committed successfully, but the checkpoint it requires failed. Its " + "changes are not in the WAL and may be lost on restart unless a later CHECKPOINT " + "succeeds: {}", + e.what())}; } - } catch (const CheckpointException& e) { - // Unless the WAL is poisoned, a failed automatic checkpoint left nothing behind: the commit - // is in the WAL and the next write commit retries. COPY FROM and CREATE INDEX on a large - // ART rely on their forced checkpoint instead of the WAL, so that failure still fails the - // statement. - if (shouldForceCheckpoint || wal.isPoisoned()) { - throwPostCommitCheckpointFailure(e, shouldForceCheckpoint); + } else if (shouldAutoCheckpoint) { + try { + tryCheckpoint(clientContext); + } catch (const std::exception& e) { + // The commit is in the WAL whatever the checkpoint did, so failing the statement would + // only invite a retry that applies it twice. A rolled-back checkpoint is retried by a + // later write commit; a poisoned WAL fails the next write instead. + warnAutoCheckpointFailed(wal, e); } - } catch (const std::exception& e) { - throwPostCommitCheckpointFailure(e, shouldForceCheckpoint); } } @@ -372,8 +373,6 @@ void TransactionManager::tryCheckpoint(main::ClientContext& clientContext) { checkpointNoLock(clientContext); } -// Throws CheckpointException only with the checkpoint rolled back or not started, or with the WAL -// poisoned; commit() relies on this to drop failed automatic checkpoints. void TransactionManager::checkpointNoLock(main::ClientContext& clientContext) { QueryProgressScope progress{clientContext, 0.01}; // Capture the retention horizon before acquiring the write gate. Taking the diff --git a/test/transaction/checkpoint_test.cpp b/test/transaction/checkpoint_test.cpp index 6934507b80..fda198f896 100644 --- a/test/transaction/checkpoint_test.cpp +++ b/test/transaction/checkpoint_test.cpp @@ -887,12 +887,18 @@ TEST_F(PostCommitCheckpointFailureTest, FailedAutoCheckpointDoesNotFailCommit) { GTEST_SKIP(); } countCheckpoints(); + ::testing::internal::CaptureStderr(); insertNodes(0, 1); ASSERT_TRUE(conn->query("BEGIN TRANSACTION;")->isSuccess()); insertNodes(1, 2); auto res = conn->query("COMMIT;"); + const auto warnings = ::testing::internal::GetCapturedStderr(); ASSERT_TRUE(res->isSuccess()) << res->getErrorMessage(); ASSERT_EQ(numCheckpoints, 2u); + EXPECT_NE(warnings.find("Warning: automatic checkpoint after a commit failed, WAL "), + std::string::npos) + << warnings; + EXPECT_NE(warnings.find("checkpoint failed."), std::string::npos) << warnings; // Reopen without checkpointing: the WAL alone recovers both commits. res.reset(); createDBAndConn(); @@ -936,19 +942,18 @@ TEST_F(PostCommitCheckpointFailureTest, AutoCheckpointBlockedByOpenWriterDoesNot checkNodes(2); } -TEST_F(PostCommitCheckpointFailureTest, AutoCheckpointThatPoisonsWALFailsCommit) { +TEST_F(PostCommitCheckpointFailureTest, AutoCheckpointThatPoisonsWALFailsNextWrite) { if (inMemMode) { GTEST_SKIP(); } countCheckpoints(); - auto res = conn->query("CREATE (a:test {id: 0, name: 'name_0'});"); + insertNodes(0, 1); + ASSERT_EQ(numCheckpoints, 1u); + auto res = conn->query("CREATE (a:test {id: 1, name: 'name_1'});"); ASSERT_FALSE(res->isSuccess()); - EXPECT_NE(res->getErrorMessage().find("Transaction committed successfully"), std::string::npos) - << res->getErrorMessage(); EXPECT_NE(res->getErrorMessage().find("refuses further writes until restart"), std::string::npos) << res->getErrorMessage(); - ASSERT_EQ(numCheckpoints, 1u); res.reset(); createDBAndConn(); checkNodes(1); @@ -995,19 +1000,18 @@ TEST_F(PostCommitCheckpointFailureTest, FailedForcedCheckpointFailsCreateIndex) EXPECT_EQ(res->getNext()->getValue(0)->getValue(), 1); } -TEST_F(PostCommitCheckpointFailureTest, FailedCheckpointCleanupFailsCommit) { +TEST_F(PostCommitCheckpointFailureTest, FailedCheckpointCleanupFailsNextWrite) { if (inMemMode) { GTEST_SKIP(); } countCheckpoints(); - auto res = conn->query("CREATE (a:test {id: 0, name: 'name_0'});"); + insertNodes(0, 1); + ASSERT_EQ(numCheckpoints, 1u); + auto res = conn->query("CREATE (a:test {id: 1, name: 'name_1'});"); ASSERT_FALSE(res->isSuccess()); - EXPECT_NE(res->getErrorMessage().find("Transaction committed successfully"), std::string::npos) - << res->getErrorMessage(); EXPECT_NE(res->getErrorMessage().find("checkpoint interrupted before WAL retirement"), std::string::npos) << res->getErrorMessage(); - ASSERT_EQ(numCheckpoints, 1u); res.reset(); createDBAndConn(); checkNodes(1); From ff0f53019282d0c5d125dde9b9100f14ce55ec1d Mon Sep 17 00:00:00 2001 From: Darek Date: Sat, 24 Oct 2026 17:30:45 +0200 Subject: [PATCH 5/6] docs(storage): describe checkpoint failures around a commit Co-Authored-By: Claude Opus 5.5 --- docs/checkpoint_recovery.md | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/docs/checkpoint_recovery.md b/docs/checkpoint_recovery.md index bed775f34d..0bc327bc07 100644 --- a/docs/checkpoint_recovery.md +++ b/docs/checkpoint_recovery.md @@ -30,7 +30,14 @@ directory sync. There the ordering still recovers from a process crash — file reached the OS — but power-loss durability of those directory entries is not guaranteed. If a step fails after the commit point, the database refuses further writes until it is reopened, -and recovery finishes the checkpoint. +and recovery finishes the checkpoint. A failure before it rolls the checkpoint back; if that rollback +fails too, the database refuses further writes the same way. + +A checkpoint that runs automatically after a commit never fails that commit, which is already in +the WAL: its failure is printed as a warning, and a later commit tries again. The checkpoint that +`COPY` or a large `CREATE INDEX` requires is different, since their data is not in the WAL: its +failure fails the statement, and the changes are lost on restart unless a later `CHECKPOINT` +succeeds. ## Checkpoint bundle format From 09134f137617ae02ebfba2290e623126a52fec88 Mon Sep 17 00:00:00 2001 From: Darek Date: Sun, 25 Oct 2026 12:10:24 +0100 Subject: [PATCH 6/6] fix(txn): don't promise a later CHECKPOINT after a forced one poisoned the WAL A COPY or large CREATE INDEX whose checkpoint failed reported that its changes are lost on restart unless a later CHECKPOINT succeeds. When that checkpoint failed after its record may have reached storage, or its rollback failed, the WAL is poisoned: no later CHECKPOINT can run before the restart, and recovery then keeps the changes only if the record reached storage. The error and docs/checkpoint_recovery.md now say so. Co-Authored-By: Claude Opus 5.5 --- docs/checkpoint_recovery.md | 10 ++++++---- src/include/storage/wal/wal.h | 1 + src/storage/wal/wal.cpp | 5 +++++ src/transaction/transaction_manager.cpp | 9 +++++++++ test/transaction/checkpoint_test.cpp | 16 ++++++++++++++++ 5 files changed, 37 insertions(+), 4 deletions(-) diff --git a/docs/checkpoint_recovery.md b/docs/checkpoint_recovery.md index 0bc327bc07..49a7e8dd3a 100644 --- a/docs/checkpoint_recovery.md +++ b/docs/checkpoint_recovery.md @@ -34,10 +34,12 @@ and recovery finishes the checkpoint. A failure before it rolls the checkpoint b fails too, the database refuses further writes the same way. A checkpoint that runs automatically after a commit never fails that commit, which is already in -the WAL: its failure is printed as a warning, and a later commit tries again. The checkpoint that -`COPY` or a large `CREATE INDEX` requires is different, since their data is not in the WAL: its -failure fails the statement, and the changes are lost on restart unless a later `CHECKPOINT` -succeeds. +the WAL: its failure is printed as a warning, and a later commit tries again, or fails if the +failure made the database refuse writes. The checkpoint that `COPY` or a large `CREATE INDEX` +requires is different, since their data is not in the WAL: its failure fails the statement. If the +checkpoint was rolled back, the changes are lost on restart unless a later `CHECKPOINT` succeeds; +if it made the database refuse writes, they survive the restart only if the checkpoint's record +reached storage. ## Checkpoint bundle format diff --git a/src/include/storage/wal/wal.h b/src/include/storage/wal/wal.h index c0aaebaa42..8ef1c64c1e 100644 --- a/src/include/storage/wal/wal.h +++ b/src/include/storage/wal/wal.h @@ -62,6 +62,7 @@ class WAL { // source for WAL pin observability alongside getFileSize() (active). uint64_t getFrozenWALFileSize(); bool mayHaveCheckpointRecord(); + bool isPoisoned(); void throwIfPoisoned(); void poison(const std::string& reason); diff --git a/src/storage/wal/wal.cpp b/src/storage/wal/wal.cpp index a4328e5e2f..3a8512f23b 100644 --- a/src/storage/wal/wal.cpp +++ b/src/storage/wal/wal.cpp @@ -362,6 +362,11 @@ bool WAL::mayHaveCheckpointRecord() { return checkpointRecordMayExist; } +bool WAL::isPoisoned() { + std::unique_lock lck{mtx}; + return poisoned; +} + void WAL::throwIfPoisoned() { std::unique_lock lck{mtx}; throwIfPoisonedNoLock(); diff --git a/src/transaction/transaction_manager.cpp b/src/transaction/transaction_manager.cpp index b0250a8fe8..a1b12eec0f 100644 --- a/src/transaction/transaction_manager.cpp +++ b/src/transaction/transaction_manager.cpp @@ -191,6 +191,15 @@ void TransactionManager::commit(main::ClientContext& clientContext, Transaction* checkpoint(clientContext); } catch (const std::exception& e) { // COPY FROM and CREATE INDEX on a large ART rely on this checkpoint instead of the WAL. + if (wal.isPoisoned()) { + // No later CHECKPOINT can run before the restart, whose recovery applies this one + // only if its record reached storage. + throw CheckpointException{std::format( + "Transaction committed successfully, but the checkpoint it requires failed. " + "The database refuses further writes until restart, and the changes survive " + "it only if the checkpoint's record reached storage: {}", + e.what())}; + } throw CheckpointException{std::format( "Transaction committed successfully, but the checkpoint it requires failed. Its " "changes are not in the WAL and may be lost on restart unless a later CHECKPOINT " diff --git a/test/transaction/checkpoint_test.cpp b/test/transaction/checkpoint_test.cpp index fda198f896..c6ea3aa87c 100644 --- a/test/transaction/checkpoint_test.cpp +++ b/test/transaction/checkpoint_test.cpp @@ -977,6 +977,22 @@ TEST_F(PostCommitCheckpointFailureTest, FailedForcedCheckpointFailsCopy) { checkNodes(10); } +TEST_F(PostCommitCheckpointFailureTest, ForcedCheckpointFailingAfterItsRecordRefusesWrites) { + if (inMemMode) { + GTEST_SKIP(); + } + countCheckpoints(); + auto res = conn->query("COPY test FROM (UNWIND range(0, 9) AS i RETURN i, 'n');"); + ASSERT_FALSE(res->isSuccess()); + const auto error = res->getErrorMessage(); + // A later CHECKPOINT cannot run: the failure after the record poisoned the WAL. + EXPECT_NE(error.find("refuses further writes until restart"), std::string::npos) << error; + EXPECT_EQ(error.find("unless a later CHECKPOINT succeeds"), std::string::npos) << error; + res.reset(); + createDBAndConn(); + checkNodes(10); +} + TEST_F(PostCommitCheckpointFailureTest, FailedForcedCheckpointFailsCreateIndex) { if (inMemMode) { GTEST_SKIP();