From ba113c8337ae6467a54addf14f592730626883ba Mon Sep 17 00:00:00 2001
From: guybe7 <guy.benoish@redislabs.com>
Date: Thu, 28 Sep 2023 16:05:53 +0200
Subject: [PATCH] WAITAOF: Update fsynced_reploff_pending just before starting
 the initial AOFRW fork (#12620)

If we set `fsynced_reploff_pending` in `startAppendOnly`, and the fork doesn't start
immediately (e.g. there's another fork active at the time), any subsequent commands
will increment `server.master_repl_offset`, but will not cause a fsync (given they were
executed before the fork started, they just ended up in the RDB part of it)
Therefore, any WAITAOF will wait on the new master_repl_offset, but it will time out
because no fsync will be executed.

Release notes:
```
WAITAOF could timeout in the absence of write traffic in case a new AOF is created and
an AOFRW can't immediately start.
This can happen by the appendonly config is changed at runtime, but also after FLUSHALL,
and replica full sync.
```

(cherry picked from commit bfa3931a04bddec8eb37c91c25d3a77c70e33000)
---
 src/aof.c           | 28 ++++++++++++++++------------
 tests/unit/wait.tcl | 31 +++++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+), 12 deletions(-)

diff --git a/src/aof.c b/src/aof.c
index 468d577f8..a89142b30 100644
--- a/src/aof.c
+++ b/src/aof.c
@@ -976,18 +976,6 @@ void stopAppendOnly(void) {
 int startAppendOnly(void) {
     serverAssert(server.aof_state == AOF_OFF);
 
-    /* Wait for all bio jobs related to AOF to drain. This prevents a race
-     * between updates to `fsynced_reploff_pending` of the worker thread, belonging
-     * to the previous AOF, and the new one. This concern is specific for a full
-     * sync scenario where we don't wanna risk the ACKed replication offset
-     * jumping backwards or forward when switching to a different master. */
-    bioDrainWorker(BIO_AOF_FSYNC);
-
-    /* Set the initial repl_offset, which will be applied to fsynced_reploff
-     * when AOFRW finishes (after possibly being updated by a bio thread) */
-    atomicSet(server.fsynced_reploff_pending, server.master_repl_offset);
-    server.fsynced_reploff = 0;
-
     server.aof_state = AOF_WAIT_REWRITE;
     if (hasActiveChildProcess() && server.child_type != CHILD_TYPE_AOF) {
         server.aof_rewrite_scheduled = 1;
@@ -2454,7 +2442,23 @@ int rewriteAppendOnlyFileBackground(void) {
         server.aof_lastbgrewrite_status = C_ERR;
         return C_ERR;
     }
+
+    if (server.aof_state == AOF_WAIT_REWRITE) {
+        /* Wait for all bio jobs related to AOF to drain. This prevents a race
+         * between updates to `fsynced_reploff_pending` of the worker thread, belonging
+         * to the previous AOF, and the new one. This concern is specific for a full
+         * sync scenario where we don't wanna risk the ACKed replication offset
+         * jumping backwards or forward when switching to a different master. */
+        bioDrainWorker(BIO_AOF_FSYNC);
+
+        /* Set the initial repl_offset, which will be applied to fsynced_reploff
+         * when AOFRW finishes (after possibly being updated by a bio thread) */
+        atomicSet(server.fsynced_reploff_pending, server.master_repl_offset);
+        server.fsynced_reploff = 0;
+    }
+
     server.stat_aof_rewrites++;
+
     if ((childpid = redisFork(CHILD_TYPE_AOF)) == 0) {
         char tmpfile[256];
 
diff --git a/tests/unit/wait.tcl b/tests/unit/wait.tcl
index 8c6010afb..bd0bcedc2 100644
--- a/tests/unit/wait.tcl
+++ b/tests/unit/wait.tcl
@@ -140,6 +140,37 @@ tags {"wait aof network external:skip"} {
             assert_error {ERR WAITAOF cannot be used when numlocal is set but appendonly is disabled.} {$master waitaof 1 0 0}
         }
 
+        test {WAITAOF local if AOFRW was postponed} {
+            r config set appendfsync everysec
+
+            # turn off AOF
+            r config set appendonly no
+
+            # create an RDB child that takes a lot of time to run
+            r set x y
+            r config set rdb-key-save-delay 100000000  ;# 100 seconds
+            r bgsave
+            assert_equal [s rdb_bgsave_in_progress] 1
+
+            # turn on AOF
+            r config set appendonly yes
+            assert_equal [s aof_rewrite_scheduled] 1
+
+            # create a write command (to increment master_repl_offset)
+            r set x y
+
+            # reset save_delay and kill RDB child
+            r config set rdb-key-save-delay 0
+            catch {exec kill -9 [get_child_pid 0]}
+
+            # wait for AOF (will unblock after AOFRW finishes)
+            assert_equal [r waitaof 1 0 10000] {1 0}
+
+            # make sure AOFRW finished
+            assert_equal [s aof_rewrite_in_progress] 0
+            assert_equal [s aof_rewrite_scheduled] 0
+        }
+
         $master config set appendonly yes
         waitForBgrewriteaof $master