Issue 4855: fail closed on entry log flush failure by yangxianjungree · Pull Request #4860 · apache/bookkeeper

yangxianjungree · 2026-08-04T07:12:31Z

Descriptions of the changes in this PR:

This PR makes entry-log write/flush failures fail closed instead of allowing a bookie to continue after the entry-log write position becomes uncertain. The issue was reproduced on the 4.16.7 line with DefaultEntryLogger + BufferedChannel + DbLedgerStorage, and this PR targets branch-4.16. This is the backport of #4863.

Motivation

A partial entry-log write/flush/force failure can leave the physical log file behind the logical BufferedChannel position. If the bookie continues accepting writes after that point, ledger storage may persist locations that point past the bytes actually written to the entry log, making entries unreadable after restart or recovery.

Changes

Introduce EntryLogWriteException as the fatal boundary for entry-log-level write failures.
Poison BufferedChannel after entry-log write, flush, or force-write failure so later writes cannot continue on an uncertain channel.
Harden entry-log header and ledgers-map writes with full-write checks and fatal failure propagation.
Propagate entry-log write failures through DefaultEntryLogger, EntryLogManager, SyncThread, BookieImpl, DbLedgerStorage, and SingleDirectoryDbLedgerStorage.
Make startup requestFlush() fail on fatal entry-log flush failure instead of racing with asynchronous shutdown.
Fail closed for InterleavedLedgerStorage and SortedLedgerStorage entry-log write failures while keeping NoWritableLedgerDirException behavior unchanged.
Harden the entryLogPerLedgerEnabled eviction path so appendLedgersMap() failure triggers fatal shutdown and the failed channel is not treated as a normal rotated log.
Clean up newly allocated log channels if allocation fails before publication.
Add unit coverage plus a real BookKeeper client e2e that verifies client-visible write failure and bookie shutdown after an entry-log flush failure.

Scope notes:

The DirectEntryLogger / direct I/O path is not changed in this PR.
This PR does not implement a two-phase header fsync / log-id publication protocol for new entry-log allocation. Allocation failure cleanup is included; stronger allocation lifecycle handling can be addressed separately.

Master Issue: #4855

Tests:

mvn -pl bookkeeper-server -am -Dtest=DbLedgerStorageEntryLogFlushFailureE2ETest -Dsurefire.failIfNoSpecifiedTests=false test
mvn -pl bookkeeper-server -am -Dtest=BufferedChannelTest,SyncThreadTest,BookieImplTest,DefaultEntryLogTest,DbLedgerStorageWriteCacheTest,DbLedgerStorageEntryLogFlushFailureE2ETest -Dsurefire.failIfNoSpecifiedTests=false test

(cherry picked from commit 200818f)

(cherry picked from commit 17042aa)

(cherry picked from commit 4058ab5)

(cherry picked from commit acff463)

(cherry picked from commit 8988984)

(cherry picked from commit 9088500)

The ImportOrder rule requires lexicographic ordering, so EntryLocation must precede EntryLogWriteException and BookieImpl must precede BufferedChannel. Rewrapping the two comments in DefaultEntryLogTest keeps them under 120 characters after the try-with-resources refactor added a level of indentation.

StevenLuMT

#4863 This code hasn't even been merged into the master branch yet, so there is no need to submit a separate merge request for a different branch. If a release from a specific branch is required later, a new release version can simply be added—provided, of course, that the feature has already been merged into master.

Copilot

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

This PR backports a “fail-closed” strategy for entry-log write/flush/fsync failures, preventing bookies from continuing after the entry-log write position becomes uncertain and adding tests to validate shutdown + client-visible errors.

Changes:

Introduces EntryLogWriteException and propagates it through entry-log write/flush paths to trigger fatal shutdown behavior.
Poisons BufferedChannel after partial/failed flush/forceWrite/header writes to prevent subsequent writes on an uncertain channel.
Adds unit + e2e tests covering flush failure propagation, shutdown idempotency, and client-observable failure behavior.

Reviewed changes

Copilot reviewed 23 out of 23 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieImpl.java	Triggers shutdown when `EntryLogWriteException` escapes add-entry paths; wires fatal listener for DB storage.
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BufferedChannel.java	Adds channel “poisoning” on write/flush/force failures to prevent further writes.
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/DefaultEntryLogger.java	Hardens ledgers-map header updates with full-write checks and adds idempotent close.
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogManager.java	Adds a fatal-error listener hook for entry-log managers.
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogManagerBase.java	Wraps entry-log IO failures into `EntryLogWriteException` and notifies fatal listeners.
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogManagerForEntryLogPerLedger.java	Makes eviction / appendLedgersMap failures fatal and uses hardened flush wrappers.
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogManagerForSingleEntryLog.java	Routes flush/force through hardened wrapper methods.
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogWriteException.java	Introduces fatal exception type for entry-log-level write uncertainty.
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLoggerAllocator.java	Cleans up partially allocated channels and avoids executor rejection races on stop.
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/InterleavedLedgerStorage.java	Re-throws `EntryLogWriteException` to fail closed instead of being handled as generic IO.
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/SortedLedgerStorage.java	Treats entry-log write failure as fatal (shutdown if possible, else RO).
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/SyncThread.java	Makes `requestFlush()` fail on fatal entry-log errors and improves shutdown idempotency.
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/DbLedgerStorage.java	Adds a fatal-error listener fanout to underlying per-directory storages.
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/SingleDirectoryDbLedgerStorage.java	On DB background flush failure, notifies fatal listener and hardens shutdown cleanup.
bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieImplTest.java	Adds tests ensuring bookie shuts down on flush failure (startup and background).
bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BufferedChannelTest.java	Adds tests for poisoning behavior on partial flush and force/header failures.
bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/DefaultEntryLogTest.java	Adds/adjusts tests for idempotent close and resource release improvements.
bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/SyncThreadTest.java	Adds tests for shutdown idempotency and fatal propagation on checkpoint/flush failure.
bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/storage/ldb/DbLedgerStorageEntryLogFlushFailureE2ETest.java	Adds e2e test verifying client-visible failure + bookie shutdown after flush failure.
bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/storage/ldb/DbLedgerStorageTest.java	Adds eviction-failure fatal propagation test and improves ByteBuf releasing in assertions.
bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/storage/ldb/DbLedgerStorageWriteCacheTest.java	Adds test asserting fatal listener invoked on cache-flush entry-log failure.
bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/storage/ldb/FailOnFlushDbLedgerStorage.java	Introduces a test-only DbLedgerStorage that can inject flush failures.
bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/storage/ldb/SingleDirectoryDbLedgerStorageShutdownTest.java	Adds test ensuring shutdown cleanup continues even after flush failure.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

+
+    final void checkWritable() throws IOException {
+        IOException failure = writeFailure;
+        if (failure != null) {


+                log.debug("Skipping preallocated entry log cleanup because allocator is stopping", e);
+            }


+        BookieImpl bookie = (BookieImpl) serverByIndex(0).getBookie();
+        LedgerHandle lh = bkc.createLedger(1, 1, 1, DigestType.CRC32, PASSWD);
+        byte[] payload = new byte[100 * 1024];
+        BKException clientFailure = null;
+
+        FailOnFlushDbLedgerStorage.injectFailureOnNextFlush();
+        try {


+            Thread.sleep(TimeUnit.SECONDS.toMillis(2));
+            invoke(entryLogManager, "doEntryLogMapCleanup", new Class<?>[] { });
+
+            assertTrue("Per-ledger eviction failure should propagate through DbLedgerStorage fatal listener",
+                    fatalLatch.await(10, TimeUnit.SECONDS));


+    private static final AtomicBoolean failNextFlushWithEntryLogWriteException = new AtomicBoolean(false);
+
+    public static void injectFailureOnNextFlush() {
+        failNextFlushWithEntryLogWriteException.set(true);
+    }
+
+    public static void resetFailure() {
+        failNextFlushWithEntryLogWriteException.set(false);
+    }


+    private boolean isGcThreadRunning() throws Exception {
+        Field gcThreadField = SingleDirectoryDbLedgerStorage.class.getDeclaredField("gcThread");
+        gcThreadField.setAccessible(true);
+        GarbageCollectorThread gcThread = (GarbageCollectorThread) gcThreadField.get(storage);
+
+        Field runningField = GarbageCollectorThread.class.getDeclaredField("running");
+        runningField.setAccessible(true);
+        return runningField.getBoolean(gcThread);
+    }
+
+    private ExecutorService getCleanupExecutor() throws Exception {
+        Field cleanupExecutorField = SingleDirectoryDbLedgerStorage.class.getDeclaredField("cleanupExecutor");
+        cleanupExecutorField.setAccessible(true);
+        return (ExecutorService) cleanupExecutorField.get(storage);
+    }


yangxianjungree added 3 commits August 4, 2026 15:01

fix: fail closed after buffered entrylog write failure

b37aa0a

(cherry picked from commit 200818f)

fix: fail closed on entry log flush failure

9bd0858

(cherry picked from commit 17042aa)

test: add e2e coverage for entry log flush failure

a00d96b

(cherry picked from commit 4058ab5)

yangxianjungree mentioned this pull request Aug 4, 2026

Issue 4855: fail closed on entry log flush failure yangxianjungree/bookkeeper#1

Closed

yangxianjungree added 3 commits August 4, 2026 16:58

fix: close entrylog failure propagation gaps

0c6c801

fix: keep fatal entrylog notifications compatible

e5b080c

test: release per-ledger eviction test entry

cf7be85

yangxianjungree mentioned this pull request Aug 5, 2026

Issue 4855: fail closed on entry log flush failure #4863

Open

yangxianjungree and others added 4 commits August 5, 2026 15:59

fix: continue db storage shutdown cleanup after flush failure

db53108

(cherry picked from commit acff463)

test: release entrylog test buffers

2dff579

(cherry picked from commit 8988984)

fix: make entrylog failure shutdown cleanup idempotent

cc086d7

(cherry picked from commit 9088500)

StevenLuMT requested a lite review from Copilot September 10, 2026 01:51

StevenLuMT requested changes Sep 10, 2026

View reviewed changes

StevenLuMT closed this Sep 10, 2026

Copilot AI reviewed Sep 10, 2026

View reviewed changes

Copilot started reviewing on behalf of StevenLuMT September 10, 2026 02:34 View session

Navigation Menu

Uh oh!

Issue 4855: fail closed on entry log flush failure - #4860

Issue 4855: fail closed on entry log flush failure#4860
yangxianjungree wants to merge 10 commits into
apache:branch-4.16from
yangxianjungree:issue-4855-branch-4.16

yangxianjungree commented Aug 4, 2026 •

edited

Loading

Uh oh!

StevenLuMT left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		log.debug("Skipping preallocated entry log cleanup because allocator is stopping", e);
		}

Uh oh!

Conversation

yangxianjungree commented Aug 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Changes

Uh oh!

StevenLuMT left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

yangxianjungree commented Aug 4, 2026 •

edited

Loading