From 587840610c0466794dc96d5cdbc7d9bb0ee1142b Mon Sep 17 00:00:00 2001
From: Robin Karlsson <61623634+robinkar@users.noreply.github.com>
Date: Fri, 23 Aug 2024 18:27:01 +0300
Subject: [PATCH] Fix large remote file uploads (#3739)

* Handle remote uploads as file transfers

Apache expects a response to the request within 60 seconds
(ProxyTimeout). Avoid that by responding with a transfer for the upload,
which the browser will query the status of instead.

* Explicitly close RemoteTransfer tempfile

Prevents the Tempfile from staying around until garbage collection on
error.
---
 apps/dashboard/app/controllers/files_controller.rb | 11 +++++++++--
 apps/dashboard/app/javascript/files/file_ops.js    |  1 +
 apps/dashboard/app/javascript/files/uppy_ops.js    | 14 ++++++++++++++
 apps/dashboard/app/models/remote_file.rb           | 11 +++++++++--
 apps/dashboard/app/models/remote_transfer.rb       |  8 ++++----
 5 files changed, 37 insertions(+), 8 deletions(-)

diff --git a/apps/dashboard/app/controllers/files_controller.rb b/apps/dashboard/app/controllers/files_controller.rb
index c43be62547..8f8b7a37b0 100644
--- a/apps/dashboard/app/controllers/files_controller.rb
+++ b/apps/dashboard/app/controllers/files_controller.rb
@@ -154,9 +154,16 @@ def upload
     parse_path(upload_path)
     validate_path!
 
-    @path.handle_upload(params[:file].tempfile)
+    # Need to remove the tempfile from list of Rack tempfiles to prevent it
+    # being cleaned up once request completes since Rclone uses the files.
+    request.env[Rack::RACK_TEMPFILES].reject! { |f| f.path == params[:file].tempfile.path } unless posix_file?
 
-    render json: {}
+    @transfer = @path.handle_upload(params[:file].tempfile)
+    if @transfer
+      render "transfers/show"
+    else
+      render json: {}
+    end
   rescue AllowlistPolicy::Forbidden => e
     render json: { error_message: e.message }, status: :forbidden
   rescue Errno::EACCES => e
diff --git a/apps/dashboard/app/javascript/files/file_ops.js b/apps/dashboard/app/javascript/files/file_ops.js
index 082ab2233c..d6207baaa4 100644
--- a/apps/dashboard/app/javascript/files/file_ops.js
+++ b/apps/dashboard/app/javascript/files/file_ops.js
@@ -27,6 +27,7 @@ const EVENTNAME = {
 
 
 let fileOps = null;
+export {fileOps};
 
 jQuery(function() {
   fileOps = new FileOps();  
diff --git a/apps/dashboard/app/javascript/files/uppy_ops.js b/apps/dashboard/app/javascript/files/uppy_ops.js
index bd5c1053a2..5a38595a51 100755
--- a/apps/dashboard/app/javascript/files/uppy_ops.js
+++ b/apps/dashboard/app/javascript/files/uppy_ops.js
@@ -4,6 +4,7 @@ import XHRUpload from '@uppy/xhr-upload'
 import _ from 'lodash';
 import {CONTENTID, EVENTNAME as DATATABLE_EVENTNAME} from './data_table.js';
 import { maxFileSize, csrfToken, uppyLocale } from '../config.js';
+import { fileOps } from './file_ops.js';
 
 let uppy = null;
 
@@ -102,6 +103,7 @@ jQuery(function() {
 
   uppy.on('complete', (result) => {
     if(result.successful.length > 0){
+      result.successful.forEach(handleUploadSuccess);
       reloadTable();
     }
   });
@@ -179,3 +181,15 @@ function reloadTable() {
   $(CONTENTID).trigger(DATATABLE_EVENTNAME.reloadTable,{});
 }
 
+// Uploads may return the status of a transfer for remote uploads.
+function handleUploadSuccess(result) {
+  // These extra checks might not be needed.
+  const body = result?.response?.body;
+  if (!body || !(typeof body === "object" && !Array.isArray(body) && body !== null)) {
+    return;
+  }
+  if (Object.keys(body).length > 0 && !body.completed) {
+    fileOps.reportTransfer(body);
+  }
+}
+
diff --git a/apps/dashboard/app/models/remote_file.rb b/apps/dashboard/app/models/remote_file.rb
index 5c3d4ba03b..383f8edd0e 100644
--- a/apps/dashboard/app/models/remote_file.rb
+++ b/apps/dashboard/app/models/remote_file.rb
@@ -77,8 +77,15 @@ def write(content)
   end
 
   def handle_upload(tempfile)
-    # FIXME: upload to the remote asynchronously
-    RcloneUtil.moveto(remote, path, tempfile.path)
+    # Start a transfer that moves the Rack tempfile to the remote.
+    transfer = RemoteTransfer.build(
+      action: "mv",
+      files: { tempfile.path => path.to_s },
+      src_remote: RcloneUtil::LOCAL_FS_NAME,
+      dest_remote: remote,
+      tempfile: tempfile
+    )
+    transfer.tap(&:perform_later)
   end
 
   def mime_type
diff --git a/apps/dashboard/app/models/remote_transfer.rb b/apps/dashboard/app/models/remote_transfer.rb
index d24b0c2e82..2ad8b1c2eb 100644
--- a/apps/dashboard/app/models/remote_transfer.rb
+++ b/apps/dashboard/app/models/remote_transfer.rb
@@ -43,7 +43,7 @@ class RemoteTransfer < Transfer
     end
   end
 
-  attr_accessor :src_remote, :dest_remote, :filesizes, :transferred
+  attr_accessor :src_remote, :dest_remote, :tempfile, :filesizes, :transferred
 
   class << self
     def transfers
@@ -51,7 +51,7 @@ def transfers
       Transfer.transfers
     end
 
-    def build(action:, files:, src_remote:, dest_remote:)
+    def build(action:, files:, src_remote:, dest_remote:, tempfile: nil)
       if files.is_a?(Array)
         # rm action will want to provide an array of files
         # so if it is an Array we convert it to a hash:
@@ -59,8 +59,7 @@ def build(action:, files:, src_remote:, dest_remote:)
         # convert [a1, a2, a3] to {a1 => nil, a2 => nil, a3 => nil}
         files = Hash[files.map { |f| [f, nil] }].with_indifferent_access
       end
-
-      self.new(action: action, files: files, src_remote: src_remote, dest_remote: dest_remote)
+      self.new(action: action, files: files, src_remote: src_remote, dest_remote: dest_remote, tempfile: tempfile)
     end
   end
 
@@ -133,6 +132,7 @@ def perform
     errors.add :base, e.message
   ensure
     self.status = OodCore::Job::Status.new(state: :completed)
+    tempfile&.close(true)
   end
 
   def from