Fix unexpected behavior when turning appendonly on and off within a t…

…ransaction (#826)

If we do `config set appendonly yes` and `config set appendonly no`
in a multi, there are some unexpected behavior.

When doing appendonly yes, we will schedule a AOFRW, and when we
are doding appendonly no, we will call stopAppendOnly to stop it.
In stopAppendOnly, the aof_fd is -1 since the aof is not start yet
and the fsync and close will take the -1 and call it, so they will
all fail with EBADF. And stopAppendOnly will emit a server log, the
close(-1) should be no problem but it is still an undefined behavior.

This PR also adds a log `Background append only file rewriting
scheduled.` to bgrewriteaofCommand when it was scheduled. 
And adds a log in stopAppendOnly when a scheduled AOF is canceled,
it will print  `AOF was disabled but there is a scheduled AOF background, cancel it.`

Signed-off-by: Binbin <[email protected]>
Co-authored-by: Viktor Söderqvist <[email protected]>

src/aof.c

@@ -931,18 +931,23 @@ void killAppendOnlyChild(void) {
  * at runtime using the CONFIG command. */
 void stopAppendOnly(void) {
     serverAssert(server.aof_state != AOF_OFF);
-    flushAppendOnlyFile(1);
-    if (valkey_fsync(server.aof_fd) == -1) {
-        serverLog(LL_WARNING, "Fail to fsync the AOF file: %s", strerror(errno));
-    } else {
-        server.aof_last_fsync = server.mstime;
+    if (server.aof_fd != -1) {
+        flushAppendOnlyFile(1);
+        if (valkey_fsync(server.aof_fd) == -1) {
+            serverLog(LL_WARNING, "Fail to fsync the AOF file: %s", strerror(errno));
+        } else {
+            server.aof_last_fsync = server.mstime;
+        }
+        close(server.aof_fd);
     }
-    close(server.aof_fd);
 
     server.aof_fd = -1;
     server.aof_selected_db = -1;
     server.aof_state = AOF_OFF;
-    server.aof_rewrite_scheduled = 0;
+    if (server.aof_rewrite_scheduled) {
+        server.aof_rewrite_scheduled = 0;
+        serverLog(LL_NOTICE, "AOF was disabled but there is a scheduled AOF background, cancel it.");
+    }
     server.aof_last_incr_size = 0;
     server.aof_last_incr_fsync_offset = 0;
     server.fsynced_reploff = -1;
@@ -2453,6 +2458,7 @@ void bgrewriteaofCommand(client *c) {
         /* When manually triggering AOFRW we reset the count
          * so that it can be executed immediately. */
         server.stat_aofrw_consecutive_failures = 0;
+        serverLog(LL_NOTICE, "Background append only file rewriting scheduled.");
         addReplyStatus(c, "Background append only file rewriting scheduled");
     } else if (rewriteAppendOnlyFileBackground() == C_OK) {
         addReplyStatus(c, "Background append only file rewriting started");

tests/integration/aof.tcl

@@ -655,4 +655,21 @@ tags {"aof external:skip"} {
             }
         }
     }
+
+    start_server {} {
+        # This test is just a coverage test, it does not check anything.
+        test {Turning appendonly on and off within a transaction} {
+            r config set appendonly no
+            r multi
+            r config set appendonly yes
+            r config set appendonly no
+            r exec
+
+            r config set appendonly yes
+            r multi
+            r config set appendonly no
+            r config set appendonly yes
+            r exec
+        }
+    }
 }