Wait for an in-flight replay before closing the log watcher by ikhoon · Pull Request #1347 · line/centraldogma

ikhoon · 2026-08-03T07:48:40Z

Motivation

When a replica shuts down, PathChildrenCache.close() cancels its in-flight tasks with an interrupt.
If a replay was waiting on delegate.execute(...) at that moment, the command had already been applied
to the local data but lastReplayedRevision was never advanced, because it is updated only on the
success path. The replica is then left with local data ahead of <dataDir>/last_revision, and the next
start-up replays the same revision again. Re-applying a command whose effect is already in the local
data fails, so the replica enters read-only mode and needs a manual re-sync.

[INFO ] [command-executor-shutdown] Closing the log watcher
[ERROR] [zookeeper-log-watcher-1-1] Failed to replay a log at revision N; entering read-only mode.
java.lang.InterruptedException
    at java.base/java.util.concurrent.CompletableFuture.reportGet(CompletableFuture.java:386)
    at java.base/java.util.concurrent.CompletableFuture.get(CompletableFuture.java:2073)
    at ...ZooKeeperCommandExecutor.replayLogs(ZooKeeperCommandExecutor.java:820)
    at ...ZooKeeperCommandExecutor.childEvent(ZooKeeperCommandExecutor.java:885)
    at ...PathChildrenCache.lambda$callListeners$1(PathChildrenCache.java:529)

Modifications

Wait for an in-flight replay before closing the log watcher, by acquiring the same monitor that
replayLogs() holds. Releasing it is safe because listenerInfo is already null, so a replay that
starts afterwards returns before executing anything.
Move the log watcher shutdown ahead of delegate.stop(). The barrier waits for a replay that is
itself waiting on the delegate, so the delegate must still be running. This also matches the drain
that shutdown(executor) already performs for the command executor threads.
logWatcher.close() still runs before shutdown(logWatcherExecutor): reversing the two would let
PathChildrenCache keep submitting to an already shut-down executor, because submitToExecutor()
only guards on its own state, which flips in close().
Wait uninterruptibly for the replay result, as a safeguard on the line where the failure occurred.
Log the last replayed revision once the replay is drained.

Note this covers a graceful shutdown only. A SIGKILL between the local apply and the progress update
leaves the same divergence; making the replay idempotent is a separate topic.

Result

A replica that shuts down while replaying no longer leaves its local data ahead of its recorded
replication progress, so it no longer enters read-only mode on the next start-up.

Motivation: When a replica shuts down, PathChildrenCache.close() cancels its in-flight tasks with an interrupt. If a replay was waiting on delegate.execute(...) at that moment, the command had already been applied to the local data but lastReplayedRevision was never advanced, because it is updated only on the success path. The replica is then left with local data ahead of <dataDir>/last_revision, and the next start-up replays the same revision again. Re-applying a command whose effect is already in the local data fails, so the replica enters read-only mode and needs a manual re-sync. Modifications: - Wait for an in-flight replay before closing the log watcher, by acquiring the same monitor that replayLogs() holds. Releasing it is safe because listenerInfo is already null, so a replay that starts afterwards returns before executing anything. - Move the log watcher shutdown ahead of delegate.stop(). The barrier waits for a replay that is itself waiting on the delegate, so the delegate must still be running. This also matches the drain that shutdown(executor) already performs for the command executor threads. - Wait uninterruptibly for the replay result, as a safeguard on the line where the failure occurred. - Log the last replayed revision once the replay is drained. Result: - A replica that shuts down while replaying no longer leaves its local data ahead of its recorded replication progress, so it no longer enters read-only mode on the next start-up.

coderabbitai · 2026-08-03T07:55:07Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 2051a9bd-7ddf-432c-85e0-873e982e4868

📥 Commits

Reviewing files that changed from the base of the PR and between ad4e3de and 78f3670.

📒 Files selected for processing (1)

server/src/main/java/com/linecorp/centraldogma/server/internal/replication/ZooKeeperCommandExecutor.java

🚧 Files skipped from review as they are similar to previous changes (1)

server/src/main/java/com/linecorp/centraldogma/server/internal/replication/ZooKeeperCommandExecutor.java

📝 Walkthrough

Walkthrough

Shutdown now rejects new replay logs, waits for in-flight replay to finish, and closes replay resources before final cleanup. A regression test verifies durable progress, restart behavior, later revisions, and writes.

Changes

Replica shutdown and replay persistence

Layer / File(s)	Summary
Drain in-flight replay during shutdown `server/src/main/java/com/linecorp/centraldogma/server/internal/replication/ZooKeeperCommandExecutor.java`	Shutdown clears listener state, drains replay work, closes the log watcher and executor, then completes Curator and ZooKeeper cleanup. Replay execution uses an uninterruptible wait.
Validate restart and revision progress `server/src/test/java/com/linecorp/centraldogma/server/internal/replication/ZooKeeperCommandExecutorTest.java`	The regression test verifies durable progress, no duplicate replay after restart, later revision processing, and continued replica writes.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Replica
  participant ReplayExecutor
  participant LogWatcher
  participant ZooKeeper
  Replica->>ReplayExecutor: wait for in-flight replay
  ReplayExecutor-->>Replica: replay completes
  Replica->>LogWatcher: close watcher
  Replica->>ReplayExecutor: close executor
  Replica->>ZooKeeper: shut down Curator and ZooKeeper

Possibly related PRs

line/centraldogma#1266: Both changes update ZooKeeperCommandExecutor shutdown handling around replay activity.
line/centraldogma#1313: Both changes test shutdown-time persistence of in-flight replay progress.
line/centraldogma#1314: Both changes modify shutdown and replay handling with in-flight replay coverage.

Suggested reviewers: jrhee17

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly states the main change: waiting for in-flight replay work before closing the log watcher.
Description check	✅ Passed	The description explains the shutdown race, the implemented changes, the regression prevention, and the graceful-shutdown scope.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In
`@server/src/test/java/com/linecorp/centraldogma/server/internal/replication/ZooKeeperCommandExecutorTest.java`:
- Around line 793-796: Update the replay future’s get path in this test to
record when it is interrupted, then await that signal after
commandExecutor().stop() and before proceed.countDown(). Keep the existing
stopFuture.join() synchronization, ensuring the replay is released only after
the shutdown boundary has exercised the interrupted-replay failure.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 81502200-6f21-42d9-bc32-cc33dabb8177

📥 Commits

Reviewing files that changed from the base of the PR and between 5afa110 and ad4e3de.

📒 Files selected for processing (2)

server/src/main/java/com/linecorp/centraldogma/server/internal/replication/ZooKeeperCommandExecutor.java
server/src/test/java/com/linecorp/centraldogma/server/internal/replication/ZooKeeperCommandExecutorTest.java

coderabbitai · 2026-08-03T07:57:08Z

+            assertThat(replayEntered.await(10, TimeUnit.SECONDS)).isTrue();
+            final CompletableFuture<Void> stopFuture = replaying.commandExecutor().stop();
+            proceed.countDown();
+            stopFuture.join();


🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Synchronize the test with the shutdown boundary.

stop() is asynchronous. Line 795 can release the replay before the pre-fix code closes the log watcher. The test can then pass without exercising the interrupted-replay failure.

Make the replay future record an interrupt in its get() path. Wait for that signal before releasing proceed. This makes the test fail with the previous interruptible wait and pass with getUninterruptibly(...).

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/test/java/com/linecorp/centraldogma/server/internal/replication/ZooKeeperCommandExecutorTest.java` around lines 793 - 796, Update the replay future’s get path in this test to record when it is interrupted, then await that signal after commandExecutor().stop() and before proceed.countDown(). Keep the existing stopFuture.join() synchronization, ensuring the replay is released only after the shutdown boundary has exercised the interrupted-replay failure.

coderabbitai Bot reviewed Aug 3, 2026

View reviewed changes

ikhoon added the defect label Aug 3, 2026

ikhoon added this to the 0.86.0 milestone Aug 3, 2026

Explain why the replay result is awaited uninterruptibly

78f3670

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Wait for an in-flight replay before closing the log watcher - #1347

Wait for an in-flight replay before closing the log watcher#1347
ikhoon wants to merge 2 commits into
line:mainfrom
ikhoon:fix-replay-interrupt-on-shutdown

ikhoon commented Aug 3, 2026

Uh oh!

coderabbitai Bot commented Aug 3, 2026 •

edited

Loading

Walkthrough

Changes

Sequence Diagram(s)

Uh oh!

coderabbitai Bot left a comment

Uh oh!

coderabbitai Bot Aug 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

ikhoon commented Aug 3, 2026

Motivation

Modifications

Result

Uh oh!

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

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Aug 3, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

coderabbitai Bot commented Aug 3, 2026 •

edited

Loading