From d83a6cf057a2a0ae09491e7797661e570752e694 Mon Sep 17 00:00:00 2001
From: Andrea Barbadoro <andij.cr@gmail.com>
Date: Wed, 8 May 2024 11:42:12 +0200
Subject: [PATCH] utils/file_io: fix double closing of ss::file in write_fully

ss::output_stream internally keeps track of in-flight exceptions. In the
close() method, it will close the underlying ss::file and rethrow the
exception.

ss::with_file_close_on_failure will too close the underlying ss::file if
the future fails, this can result in a double close triggering an
assertion like

```
seastar-prefix/src/seastar/include/seastar/core/future.hh:1917: future<T> seastar::promise<>::get_future() [T = void]: Assertion `!this->_future && this->_state && !this->_task' failed
```

This unit test shows how this assert could be triggered, if
ss::output_stream as an active exception:

```
SEASTAR_THREAD_TEST_CASE(test_with_file_close_on_failure) {
    auto flags = ss::open_flags::rw | ss::open_flags::create
                 | ss::open_flags::truncate;
    ss::with_file_close_on_failure(
      ss::open_file_dma("/tmp/tmp.YuupbuphlR", flags),
      [](ss::file f) mutable {
          return f.close().then([] { throw "any value"; });
       })
      .get();
}
```

This commit moves out ss::output_stream::close() from
ss::with_file_close_on_failure.

The method is coroutinized for clarity.
---
 src/v/utils/file_io.cc | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

diff --git a/src/v/utils/file_io.cc b/src/v/utils/file_io.cc
index 9d9b164839ab..8d0e8821908f 100644
--- a/src/v/utils/file_io.cc
+++ b/src/v/utils/file_io.cc
@@ -14,6 +14,7 @@
 #include "bytes/iobuf.h"
 #include "bytes/iostream.h"
 
+#include <seastar/core/coroutine.hh>
 #include <seastar/core/file.hh>
 #include <seastar/core/fstream.hh>
 #include <seastar/core/temporary_buffer.hh>
@@ -57,17 +58,11 @@ ss::future<> write_fully(const std::filesystem::path& p, iobuf buf) {
                  | ss::open_flags::truncate;
     /// Closes file on failure, otherwise file is expected to be closed in the
     /// success case where the ss::output_stream calls close()
-    return ss::with_file_close_on_failure(
-      ss::open_file_dma(p.string(), flags),
-      [buf = std::move(buf)](ss::file f) mutable {
-          return ss::make_file_output_stream(std::move(f), buf_size)
-            .then([buf = std::move(buf)](ss::output_stream<char> out) mutable {
-                return ss::do_with(
-                  std::move(out),
-                  [buf = std::move(buf)](ss::output_stream<char>& out) mutable {
-                      return write_iobuf_to_output_stream(std::move(buf), out)
-                        .then([&out]() mutable { return out.close(); });
-                  });
-            });
+    auto out = co_await ss::with_file_close_on_failure(
+      ss::open_file_dma(p.string(), flags), [](ss::file f) {
+          return ss::make_file_output_stream(std::move(f), buf_size);
       });
+    co_await write_iobuf_to_output_stream(std::move(buf), out).finally([&out] {
+        return out.close();
+    });
 }