From 150bd05e5d767ddedbb38d9ee909e20b899bfbe8 Mon Sep 17 00:00:00 2001 From: Darek Date: Sun, 25 Oct 2026 09:58:07 +0100 Subject: [PATCH 1/3] fix(wal): refuse writes after a WAL write fails instead of hanging WAL::waitForDurabilityNoLock flushed the WAL writer before entering the try that poisons the WAL when a sync fails. On a full disk the flush threw without poisoning the WAL, so syncInProgress stayed set and the next commit waited for that sync forever. The bytes also stayed in BufferedFileWriter's buffer: its destructor wrote them when the database closed, so a commit reported as failed could reach the WAL, and when that write failed too, it threw out of the destructor and aborted the process. - A failed write while appending a commit, flushing it or flushing a checkpoint record now poisons the WAL, as a failed sync does; poisoning also wakes the commits waiting for the sync. - BufferedFileWriter::flush drops its bytes even when the write fails, so neither a later flush nor the destructor writes the bytes of a commit reported as failed. Co-Authored-By: Claude Opus 5.5 --- src/common/serializer/buffered_file.cpp | 9 +- src/include/common/serializer/buffered_file.h | 2 + src/include/storage/wal/wal.h | 3 + src/storage/wal/wal.cpp | 31 +++- test/transaction/wal_test.cpp | 137 ++++++++++++++++++ 5 files changed, 176 insertions(+), 6 deletions(-) diff --git a/src/common/serializer/buffered_file.cpp b/src/common/serializer/buffered_file.cpp index 47a1d7c66c..973ef8575a 100644 --- a/src/common/serializer/buffered_file.cpp +++ b/src/common/serializer/buffered_file.cpp @@ -1,6 +1,7 @@ #include "common/serializer/buffered_file.h" #include +#include #include "common/assert.h" #include "common/exception/runtime.h" @@ -52,9 +53,11 @@ void BufferedFileWriter::flush() { if (bufferOffset == 0) { return; } - fileInfo.writeFile(buffer.get(), bufferOffset, fileOffset); - fileOffset += bufferOffset; - bufferOffset = 0; + // The bytes leave the buffer even when the write fails: neither a later flush nor the + // destructor may write them after the caller has seen the failure. + const auto size = std::exchange(bufferOffset, 0); + fileInfo.writeFile(buffer.get(), size, fileOffset); + fileOffset += size; memset(buffer.get(), 0, BUFFER_SIZE); } diff --git a/src/include/common/serializer/buffered_file.h b/src/include/common/serializer/buffered_file.h index 3279561b44..22025db4c0 100644 --- a/src/include/common/serializer/buffered_file.h +++ b/src/include/common/serializer/buffered_file.h @@ -18,6 +18,8 @@ class BufferedFileWriter final : public Writer { void write(const uint8_t* data, uint64_t size) override; void clear() override; + // The bytes leave the buffer even when the write fails; fileOffset advances only after it + // succeeds. void flush() override; void sync() override; diff --git a/src/include/storage/wal/wal.h b/src/include/storage/wal/wal.h index c0aaebaa42..76490c5b21 100644 --- a/src/include/storage/wal/wal.h +++ b/src/include/storage/wal/wal.h @@ -73,6 +73,9 @@ class WAL { void retireFileNoLock(const std::string& path, const char* walName); void throwIfPoisonedNoLock() const; void poisonNoLock(const std::string& reason); + // A failed write can leave the WAL ending in part of a record, which later appends would + // follow, so writes are refused until a restart, whose recovery drops that tail. + [[noreturn]] void failWriteNoLock(const std::string& error); void waitForDurabilityNoLock(uint64_t commitSequence, std::unique_lock& lck); void flushAndSyncNoLock(); void writeHeader(main::ClientContext& context); diff --git a/src/storage/wal/wal.cpp b/src/storage/wal/wal.cpp index a4328e5e2f..4de78515df 100644 --- a/src/storage/wal/wal.cpp +++ b/src/storage/wal/wal.cpp @@ -48,7 +48,13 @@ void WAL::logCommittedWAL(LocalWAL& localWAL, main::ClientContext* context, std::unique_lock lck{mtx}; throwIfPoisonedNoLock(); initWriter(context); - localWAL.inMemWriter->flush(*serializer->getWriter()); + try { + localWAL.inMemWriter->flush(*serializer->getWriter()); + } catch (const std::exception& e) { + failWriteNoLock(e.what()); + } catch (...) { + failWriteNoLock("unknown exception"); + } commitSequence = ++appendedCommitSequence; waitForDurabilityNoLock(commitSequence, lck); } @@ -281,7 +287,13 @@ void WAL::waitForDurabilityNoLock(uint64_t commitSequence, std::unique_lockgetWriter()->flush(); + try { + serializer->getWriter()->flush(); + } catch (const std::exception& e) { + failWriteNoLock(e.what()); + } catch (...) { + failWriteNoLock("unknown exception"); + } auto* fileToSync = fileInfo.get(); lck.unlock(); try { @@ -317,7 +329,13 @@ void WAL::waitForDurabilityNoLock(uint64_t commitSequence, std::unique_lockgetWriter()->flush(); + try { + serializer->getWriter()->flush(); + } catch (const std::exception& e) { + failWriteNoLock(e.what()); + } catch (...) { + failWriteNoLock("unknown exception"); + } try { serializer->getWriter()->sync(); if (!activeWALDirectorySynced) { @@ -338,6 +356,13 @@ void WAL::flushAndSyncNoLock() { } } +void WAL::failWriteNoLock(const std::string& error) { + poisonNoLock("WAL write failed: " + error); + throw RuntimeException("WAL write failed; database is in a panic state and refuses further " + "writes until restart. Original error: " + + error); +} + uint64_t WAL::getFileSize() { std::unique_lock lck{mtx}; if (!serializer) { diff --git a/test/transaction/wal_test.cpp b/test/transaction/wal_test.cpp index a7bde256a5..bcedf3d42d 100644 --- a/test/transaction/wal_test.cpp +++ b/test/transaction/wal_test.cpp @@ -61,6 +61,17 @@ class FailingSyncFileSystem final : public FileSystem, public DirectorySyncFileS failDirectorySync = fail; } + void setFailWrite(bool fail) { + std::unique_lock lck{mtx}; + failWrite = fail; + } + + uint64_t getFileSize(const std::string& path) const { + std::unique_lock lck{mtx}; + const auto file = files.find(path); + return file == files.end() ? 0 : file->second.size(); + } + bool canHandleFile(const std::string_view path) const override { return path.starts_with("failing-sync://"); } @@ -167,6 +178,9 @@ class FailingSyncFileSystem final : public FileSystem, public DirectorySyncFileS void writeFile(FileInfo& fileInfo, const uint8_t* buffer, uint64_t numBytes, uint64_t offset) const override { std::unique_lock lck{mtx}; + if (failWrite) { + throw IOException{"Injected WAL write failure."}; + } auto& file = files[fileInfo.path]; if (file.size() < offset + numBytes) { file.resize(offset + numBytes); @@ -196,6 +210,7 @@ class FailingSyncFileSystem final : public FileSystem, public DirectorySyncFileS bool failRemove = false; bool failAfterRename = false; bool failDirectorySync = false; + bool failWrite = false; mutable uint64_t parentDirectorySyncCount = 0; mutable std::string parentDirectorySyncPath; mutable std::string truncatedPath; @@ -236,6 +251,128 @@ TEST_F(WalTest, WALSyncFailurePoisonsWALAndReturnsAllocatedCommitSequence) { EXPECT_EQ(walCommitSequence, 0); } +// A commit whose WAL write fails must not leave the WAL waiting for that write (#1397). +TEST_F(WalTest, WALWriteFailurePoisonsWAL) { + 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(); + failingFSPtr->setFailWrite(true); + uint64_t walCommitSequence = 0; + std::string error; + try { + wal.logCommittedWAL(localWAL, conn->getClientContext(), walCommitSequence); + } catch (const Exception& e) { + error = e.what(); + } + failingFSPtr->setFailWrite(false); + ASSERT_NE(error.find("panic state"), std::string::npos) << error; + // The flush failed after the commit took its sequence, as a failed sync does. + EXPECT_EQ(walCommitSequence, 1); + + lbug::storage::LocalWAL secondLocalWAL( + *lbug::storage::MemoryManager::Get(*conn->getClientContext()), false /* enableChecksums */); + secondLocalWAL.logLoadExtension("" /* main */, "dummy2"); + secondLocalWAL.logCommit(); + EXPECT_THROW(wal.logCommittedWAL(secondLocalWAL, conn->getClientContext(), walCommitSequence), + RuntimeException); + EXPECT_EQ(walCommitSequence, 0); +} + +// A commit larger than the writer's buffer reaches the file while it is appended, before the +// flush that waits for durability. +TEST_F(WalTest, WALAppendFailurePoisonsWAL) { + 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 */, true /* enableChecksums */, + &vfs); + lbug::storage::LocalWAL localWAL(*lbug::storage::MemoryManager::Get(*conn->getClientContext()), + true /* enableChecksums */); + localWAL.logLoadExtension("" /* main */, std::string(3 * LBUG_PAGE_SIZE, 'x')); + localWAL.logCommit(); + failingFSPtr->setFailWrite(true); + uint64_t walCommitSequence = 0; + std::string error; + try { + wal.logCommittedWAL(localWAL, conn->getClientContext(), walCommitSequence); + } catch (const Exception& e) { + error = e.what(); + } + failingFSPtr->setFailWrite(false); + ASSERT_NE(error.find("panic state"), std::string::npos) << error; + EXPECT_EQ(walCommitSequence, 0); + + lbug::storage::LocalWAL secondLocalWAL( + *lbug::storage::MemoryManager::Get(*conn->getClientContext()), true /* enableChecksums */); + secondLocalWAL.logLoadExtension("" /* main */, "dummy2"); + secondLocalWAL.logCommit(); + EXPECT_THROW(wal.logCommittedWAL(secondLocalWAL, conn->getClientContext(), walCommitSequence), + RuntimeException); +} + +// The bytes of a commit whose WAL write failed must not reach the file later, when closing. +TEST_F(WalTest, FailedWALWriteIsNotWrittenOnClose) { + VirtualFileSystem vfs; + auto failingFS = std::make_unique(false /* failSync */); + auto* failingFSPtr = failingFS.get(); + vfs.registerFileSystem(std::move(failingFS)); + + auto wal = std::make_unique("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(); + failingFSPtr->setFailWrite(true); + uint64_t walCommitSequence = 0; + try { + wal->logCommittedWAL(localWAL, conn->getClientContext(), walCommitSequence); + } catch (const Exception&) {} // NOLINT(bugprone-empty-catch): the failure is expected. + failingFSPtr->setFailWrite(false); + wal.reset(); + EXPECT_EQ(failingFSPtr->getFileSize( + lbug::storage::StorageUtils::getWALFilePath("failing-sync://db")), + 0); +} + +TEST_F(WalTest, CheckpointRecordWriteFailurePoisonsWAL) { + 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); + failingFSPtr->setFailWrite(true); + std::string error; + try { + wal.logAndFlushCheckpoint(conn->getClientContext()); + } catch (const Exception& e) { + error = e.what(); + } + failingFSPtr->setFailWrite(false); + ASSERT_NE(error.find("panic state"), std::string::npos) << error; + EXPECT_TRUE(wal.mayHaveCheckpointRecord()); + + lbug::storage::LocalWAL localWAL(*lbug::storage::MemoryManager::Get(*conn->getClientContext()), + false /* enableChecksums */); + localWAL.logLoadExtension("" /* main */, "dummy"); + localWAL.logCommit(); + uint64_t walCommitSequence = 0; + EXPECT_THROW(wal.logCommittedWAL(localWAL, conn->getClientContext(), walCommitSequence), + RuntimeException); +} + TEST_F(WalTest, CheckpointSyncFailureReportsPossibleCommitRecord) { VirtualFileSystem vfs; auto failingFS = std::make_unique(true /* failSync */); From 3944148bccd4bcc4f0f30ec2a6d031b802638c23 Mon Sep 17 00:00:00 2001 From: Darek Date: Sun, 25 Oct 2026 11:43:15 +0100 Subject: [PATCH 2/3] style(test): clang-format the WAL write-failure test Co-Authored-By: Claude Opus 5.5 --- test/transaction/wal_test.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/transaction/wal_test.cpp b/test/transaction/wal_test.cpp index bcedf3d42d..63c79b922d 100644 --- a/test/transaction/wal_test.cpp +++ b/test/transaction/wal_test.cpp @@ -340,8 +340,8 @@ TEST_F(WalTest, FailedWALWriteIsNotWrittenOnClose) { } catch (const Exception&) {} // NOLINT(bugprone-empty-catch): the failure is expected. failingFSPtr->setFailWrite(false); wal.reset(); - EXPECT_EQ(failingFSPtr->getFileSize( - lbug::storage::StorageUtils::getWALFilePath("failing-sync://db")), + EXPECT_EQ( + failingFSPtr->getFileSize(lbug::storage::StorageUtils::getWALFilePath("failing-sync://db")), 0); } From c050270b158cd4ebb3d76a874beb9ef962b05368 Mon Sep 17 00:00:00 2001 From: Darek Date: Sun, 25 Oct 2026 12:10:24 +0100 Subject: [PATCH 3/3] fix(wal): report a failed frozen-WAL write as a write failure logAndFlushCheckpointToFrozen poisoned the WAL when the flush of its CHECKPOINT record failed, but reported a sync failure. That flush now goes through failWriteNoLock like the active WAL's writes. BufferedFileWriter::flush also stops zeroing its whole buffer after every write: only the bytes before bufferOffset are written. Co-Authored-By: Claude Opus 5.5 --- src/common/serializer/buffered_file.cpp | 1 - src/storage/wal/wal.cpp | 8 +++++++ test/transaction/wal_test.cpp | 29 +++++++++++++++++++++++++ 3 files changed, 37 insertions(+), 1 deletion(-) diff --git a/src/common/serializer/buffered_file.cpp b/src/common/serializer/buffered_file.cpp index 973ef8575a..6c77c92c0f 100644 --- a/src/common/serializer/buffered_file.cpp +++ b/src/common/serializer/buffered_file.cpp @@ -58,7 +58,6 @@ void BufferedFileWriter::flush() { const auto size = std::exchange(bufferOffset, 0); fileInfo.writeFile(buffer.get(), size, fileOffset); fileOffset += size; - memset(buffer.get(), 0, BUFFER_SIZE); } void BufferedFileWriter::sync() { diff --git a/src/storage/wal/wal.cpp b/src/storage/wal/wal.cpp index 4de78515df..fb9f1d09b1 100644 --- a/src/storage/wal/wal.cpp +++ b/src/storage/wal/wal.cpp @@ -192,6 +192,14 @@ void WAL::logAndFlushCheckpointToFrozen(main::ClientContext* context) { frozenSerializer->getWriter()->onObjectEnd(); try { frozenSerializer->getWriter()->flush(); + } catch (const std::exception& e) { + std::unique_lock lck{mtx}; + failWriteNoLock(e.what()); + } catch (...) { + std::unique_lock lck{mtx}; + failWriteNoLock("unknown exception"); + } + try { frozenSerializer->getWriter()->sync(); } catch (const std::exception& e) { std::unique_lock lck{mtx}; diff --git a/test/transaction/wal_test.cpp b/test/transaction/wal_test.cpp index 63c79b922d..8dd684e459 100644 --- a/test/transaction/wal_test.cpp +++ b/test/transaction/wal_test.cpp @@ -373,6 +373,35 @@ TEST_F(WalTest, CheckpointRecordWriteFailurePoisonsWAL) { RuntimeException); } +// A failed write of the CHECKPOINT record into the frozen WAL is reported as a write failure. +TEST_F(WalTest, FrozenCheckpointRecordWriteFailurePoisonsWAL) { + 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; + ASSERT_NO_THROW(wal.logCommittedWAL(localWAL, conn->getClientContext(), walCommitSequence)); + ASSERT_NO_THROW(wal.rotateForCheckpoint(conn->getClientContext())); + + failingFSPtr->setFailWrite(true); + std::string error; + try { + wal.logAndFlushCheckpointToFrozen(conn->getClientContext()); + } catch (const Exception& e) { + error = e.what(); + } + failingFSPtr->setFailWrite(false); + EXPECT_NE(error.find("WAL write failed"), std::string::npos) << error; + EXPECT_THROW(wal.throwIfPoisoned(), RuntimeException); +} + TEST_F(WalTest, CheckpointSyncFailureReportsPossibleCommitRecord) { VirtualFileSystem vfs; auto failingFS = std::make_unique(true /* failSync */);