Fix Azure sync race and reject empty uploads

app/controllers/uploads_controller.rb

@@ -1,5 +1,9 @@
 class UploadsController < ApplicationController
   def create
+    if empty_documents.any?
+      return render json: { result: "error", message: empty_documents_message }, status: :unprocessable_content
+    end
+
     blobs = document_params.map do |document|
       ActiveStorage::Blob.create_and_upload!(**document)
     end
@@ -28,7 +32,7 @@ def destroy
   private
 
   def document_params
-    params.require(:documents).map do |document|
+    documents.map do |document|
       {
         io: document,
         filename: derived_filename(document),
@@ -37,15 +41,23 @@ def document_params
     end
   end
 
+  def documents
+    @documents ||= params.require(:documents)
+  end
+
+  def empty_documents
+    @empty_documents ||= documents.select { |document| document.size.zero? }
+  end
+
+  def empty_documents_message
+    names = empty_documents.map { |document| original_filename(document) }.to_sentence
+
+    "Nothing was uploaded because #{names} #{empty_documents.one? ? "is" : "are"} empty (0 bytes). " \
+      "Check the file on your computer and try again."
+  end
+
   def derived_filename(document)
-    filename =
-      if document.respond_to?(:original_filename)
-        document.original_filename
-      elsif document.respond_to?(:filename)
-        document.filename.to_s
-      else
-        "unnamed"
-      end
+    filename = original_filename(document)
 
     name = File.basename(filename, ".*")
     ext = File.extname(filename).delete_prefix(".")
@@ -54,4 +66,14 @@ def derived_filename(document)
       [ name.parameterize(separator: "_"), ext ].compact.join("."),
     ].join("_")
   end
+
+  def original_filename(document)
+    if document.respond_to?(:original_filename)
+      document.original_filename
+    elsif document.respond_to?(:filename)
+      document.filename.to_s
+    else
+      "unnamed"
+    end
+  end
 end

app/javascript/controllers/upload_controller.js

@@ -7,6 +7,7 @@ export default class extends Controller {
     "filesContainer",
     "hiddenField",
     "submitButton",
+    "errorMessage",
   ];
 
   uploadFile(event) {
@@ -15,6 +16,7 @@ export default class extends Controller {
     this.submitButtonTarget.disabled = true;
     this.submitButtonTarget.classList.add("disabled:opacity-75");
     this.submitButtonTarget.value = "Uploading...";
+    this.showError("");
 
     const filesInput = this.filesInputTarget;
     let files = Array.from(filesInput.files);
@@ -40,15 +42,28 @@ export default class extends Controller {
       .then((data) => {
         if (data.result === "success") {
           this.filesContainerTarget.innerHTML += data.html;
-          filesInput.value = "";
+        } else {
+          this.showError(data.message || "The upload failed. Please try again.");
         }
-
+      })
+      .catch(() => {
+        this.showError("The upload failed. Please try again.");
+      })
+      .finally(() => {
+        filesInput.value = "";
         this.submitButtonTarget.disabled = false;
         this.submitButtonTarget.classList.remove("disabled:opacity-75");
         this.submitButtonTarget.value = "Save Topic";
       });
   }
 
+  showError(message) {
+    if (!this.hasErrorMessageTarget) return;
+
+    this.errorMessageTarget.textContent = message;
+    this.errorMessageTarget.hidden = !message;
+  }
+
   removeFile(event) {
     event.preventDefault();
 

app/jobs/application_job.rb

@@ -5,6 +5,10 @@ class ApplicationJob < ActiveJob::Base
   # Most jobs are safe to ignore if the underlying records are no longer available
   # discard_on ActiveJob::DeserializationError
 
+  # Solid Queue lives in its own database, so a job enqueued inside a transaction
+  # can be picked up before the records it needs are committed.
+  self.enqueue_after_transaction_commit = true
+
   around_perform do |job, block|
     start_time = Time.current
     Rails.logger.debug "[Job Start] #{job.class.name} ID=#{job.job_id} Args=#{job.arguments}"

app/jobs/documents_sync_job.rb

@@ -1,4 +1,6 @@
 class DocumentsSyncJob < ApplicationJob
+  retry_on AzureFileShares::Errors::ApiError, Timeout::Error, wait: :polynomially_longer, attempts: 5
+
   def perform(topic_id:, document_id:, action:, share: ENV["AZURE_STORAGE_SHARE_NAME"])
     return if ENV["AZURE_MEDIA_FILES_SYNC_DISABLED"].present?
 

app/services/file_worker.rb

@@ -1,6 +1,8 @@
 class FileWorker
   UPLOAD_TIMEOUT = 300
 
+  class EmptyFileError < StandardError; end
+
   def initialize(share:, name:, path:, file:, new_path: nil)
     @share = share
     @name = name
@@ -28,7 +30,8 @@ def copy
   def send_file
     validate_filename
 
-    return if file.blank?
+    # Skipping quietly left the file listed in File.csv but missing from Azure
+    raise EmptyFileError, "#{name} is empty and was not uploaded to #{path}" if file.blank?
 
     create_subdirs(path)
 

app/views/topics/_form.html.erb

@@ -140,6 +140,8 @@
           </div>
         </label>
 
+        <p data-upload-target="errorMessage" role="alert" hidden class="bg-red-50 border border-red-200 text-red-900 p-4 rounded-lg mt-4 text-sm"></p>
+
 
         <div class="flex justify-between items-center pt-8 border-t border-gray-100 mt-8">
           <div class="flex gap-4">

spec/jobs/documents_sync_job_spec.rb

@@ -6,6 +6,44 @@
   let(:document) { topic.documents.first }
   let(:file_name) { document.filename }
 
+  describe "enqueueing" do
+    let(:job_args) { { topic_id: topic.id, document_id: document.id, action: "update" } }
+
+    it "waits for the surrounding transaction to commit" do
+      expect {
+        ActiveRecord::Base.transaction do
+          expect { described_class.perform_later(**job_args) }.not_to have_enqueued_job(described_class)
+        end
+      }.to have_enqueued_job(described_class).with(**job_args)
+    end
+
+    it "is dropped when the surrounding transaction rolls back" do
+      expect {
+        ActiveRecord::Base.transaction do
+          described_class.perform_later(**job_args)
+
+          raise ActiveRecord::Rollback
+        end
+      }.not_to have_enqueued_job(described_class)
+    end
+  end
+
+  describe "retries" do
+    let(:job_args) { { topic_id: topic.id, document_id: document.id, action: "update" } }
+
+    it "retries when Azure returns an API error" do
+      allow(FileManager).to receive(:new).and_raise(AzureFileShares::Errors::ApiError.new("ServerBusy"))
+
+      expect { described_class.perform_now(**job_args) }.to have_enqueued_job(described_class).with(**job_args)
+    end
+
+    it "retries when the Azure request times out" do
+      allow(FileManager).to receive(:new).and_raise(Timeout::Error)
+
+      expect { described_class.perform_now(**job_args) }.to have_enqueued_job(described_class).with(**job_args)
+    end
+  end
+
   describe "#perform" do
     let(:file_worker) { instance_double(FileWorker) }
 

spec/requests/uploads/create_spec.rb

@@ -42,5 +42,26 @@
         expect(ActiveStorage::Blob.last.filename.to_s).to eq("[skillrx_internal_upload]_who_was_there.pdf")
       end
     end
+
+    context "when one of the files is empty" do
+      let(:document_params) do
+        [
+          Rack::Test::UploadedFile.new(
+            Rails.root.join("spec", "fixtures", "files", "dummy.pdf"),
+            "application/pdf",
+            original_filename: "dummy.pdf"
+          ),
+          Rack::Test::UploadedFile.new(StringIO.new(""), "application/pdf", original_filename: "empty.pdf"),
+        ]
+      end
+
+      it "rejects the upload and names the empty file" do
+        post uploads_url, params: { documents: document_params }
+
+        expect(response).to have_http_status(:unprocessable_content)
+        expect(JSON.parse(response.body)).to include("result" => "error", "message" => /empty\.pdf/)
+        expect(ActiveStorage::Blob.count).to eq(0)
+      end
+    end
   end
 end

spec/services/file_worker_spec.rb

@@ -70,7 +70,7 @@
       expect { worker.send }.to raise_error(AzureFileShares::Errors::ApiError)
     end
 
-    it "does not upload blank files" do
+    it "refuses to upload blank files and fails loudly" do
       worker_with_blank_file = described_class.new(
         share: "skillrx-test",
         file: "",
@@ -80,7 +80,7 @@
 
       expect(files).not_to receive(:upload_file)
 
-      worker_with_blank_file.send
+      expect { worker_with_blank_file.send }.to raise_error(FileWorker::EmptyFileError, /#{name}/)
     end
   end
 

spec/system/upload_management_spec.rb

@@ -26,6 +26,14 @@
       click_button("remove-button-1")
       expect(page).not_to have_text("logo_ruby_for_good.png")
     end
+
+    it "explains why an empty file was not uploaded" do
+      page.attach_file(Rails.root.join("spec/fixtures/files/empty.pdf")) do
+        page.find("#documents").click
+      end
+      expect(page).to have_text("empty.pdf is empty (0 bytes)")
+      expect(page).to have_button("Save Topic", disabled: false)
+    end
   end
 
   context "when updating a topic " do