From 5d02800e578ff2e65066dbe757f72c0f33274b93 Mon Sep 17 00:00:00 2001
From: eldritch horrors <pennae@lix.systems>
Date: Sat, 9 Nov 2024 01:17:28 +0100
Subject: [PATCH] libstore: do not retry FileTransfer uploads

this only ever worked for empty uploads, and there it worked only by
complete accident: curl was asked to send more data than the wrapper
would provide, which curl would not like and report as an error. the
error would cause a retry with even less data to send, until finally
failing by running into the retry limit. let's just forbid all this.

Change-Id: I229a94b3b8b33e2c6cdb8ea19edd57cd6740e6c6
---
 src/libstore/filetransfer.cc        |  1 +
 tests/unit/libstore/filetransfer.cc | 30 +++++++++++++++++++++++++----
 2 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/src/libstore/filetransfer.cc b/src/libstore/filetransfer.cc
index 88bcb3736..a713e7490 100644
--- a/src/libstore/filetransfer.cc
+++ b/src/libstore/filetransfer.cc
@@ -486,6 +486,7 @@ struct curlFileTransfer : public FileTransfer
                    sink, we can only retry if the server supports
                    ranged requests. */
                 if (err == Transient
+                    && !uploadData.has_value()
                     && attempt < tries
                     && (!this->dataCallback
                         || writtenToSink == 0
diff --git a/tests/unit/libstore/filetransfer.cc b/tests/unit/libstore/filetransfer.cc
index 0cb4de756..4a7f53e43 100644
--- a/tests/unit/libstore/filetransfer.cc
+++ b/tests/unit/libstore/filetransfer.cc
@@ -122,9 +122,10 @@ serveHTTP(std::vector<Reply> replies)
                 std::thread([=, conn{std::move(conn)}] {
                     auto send = [&](std::string_view bit) {
                         while (!bit.empty()) {
-                            auto written = ::write(conn.get(), bit.data(), bit.size());
+                            auto written = ::send(conn.get(), bit.data(), bit.size(), MSG_NOSIGNAL);
                             if (written < 0) {
-                                throw SysError(errno, "write() failed");
+                                debug("send() failed: %s", strerror(errno));
+                                return;
                             }
                             bit.remove_prefix(written);
                         }
@@ -164,13 +165,14 @@ serveHTTP(std::vector<Reply> replies)
                     ::shutdown(conn.get(), SHUT_WR);
                     for (;;) {
                         char buf[1];
-                        switch (read(conn.get(), buf, 1)) {
+                        switch (recv(conn.get(), buf, 1, MSG_NOSIGNAL)) {
                         case 0:
                             return; // remote closed
                         case 1:
                             continue; // connection still held open by remote
                         default:
-                            throw SysError(errno, "read() failed");
+                            debug("recv() failed: %s", strerror(errno));
+                            return;
                         }
                     }
                 }).detach();
@@ -369,4 +371,24 @@ TEST(FileTransfer, doesntRetryTransferForever)
     ASSERT_THROW(ft->download(fmt("http://[::1]:%d", port)).second->drain(), FileTransferError);
 }
 
+TEST(FileTransfer, doesntRetryUploads)
+{
+    auto ft = makeFileTransfer(0);
+
+    {
+        auto [port, srv] = serveHTTP({
+            {"429 try again later", "", [] { return ""; }},
+            {"200 ok", "", [] { return ""; }},
+        });
+        ASSERT_THROW(ft->upload(fmt("http://[::1]:%d", port), ""), FileTransferError);
+    }
+    {
+        auto [port, srv] = serveHTTP({
+            {"429 try again later", "", [] { return ""; }},
+            {"200 ok", "", [] { return ""; }},
+        });
+        ASSERT_THROW(ft->upload(fmt("http://[::1]:%d", port), "foo"), FileTransferError);
+    }
+}
+
 }