From 49cf4ecaa3869102904f7650dd72bf80f8c4f2b2 Mon Sep 17 00:00:00 2001 From: Isra Jazairi Date: Thu, 17 Sep 2026 09:51:03 -0700 Subject: [PATCH 1/7] Guard against nil attachments in processing form Why these changes are being introduced: `ThesisController#deleted_file_list` calls `.blob` on a potentially nil Active Storage attachment. The processing form can trigger a race condition when a background process deletes a file that is still visible on the form. If a user attempts to delete that file and the attachment no longer exists, the app throws a 422 error. Relevant ticket(s): - [USE-710](https://mitlibraries.atlassian.net/browse/ETD-710) How this addresses that need: This adds a guard clause to check for the presence of the attachment before calling `.blob`. Side effects of this change: None. --- app/controllers/thesis_controller.rb | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/app/controllers/thesis_controller.rb b/app/controllers/thesis_controller.rb index c1390d61..d3cadc77 100644 --- a/app/controllers/thesis_controller.rb +++ b/app/controllers/thesis_controller.rb @@ -178,7 +178,10 @@ def deleted_file_list Rails.logger.debug('TRANSFER_COUNTS: Files count changed on thesis, expect updated Transfer count logs') thesis_params['files_attachments_attributes'].values.select { |item| item['_destroy'] == '1' }.each do |file| - needle = ActiveStorage::Attachment.find_by(id: file['id']).blob + attachment = ActiveStorage::Attachment.find_by(id: file['id']) + next unless attachment + + needle = attachment.blob list.append({ 'filename' => needle.filename, 'transfer_id' => needle.attachments.select { |att| att.record_type == 'Transfer' }.first.record_id From 68d767efc8c875a3bdc27e6786fd74cffd1d94ff Mon Sep 17 00:00:00 2001 From: Isra Jazairi Date: Fri, 18 Sep 2026 08:20:42 -0700 Subject: [PATCH 2/7] Add regression coverage for processing form bug and guard against stale delete rows --- app/controllers/thesis_controller.rb | 12 +++++++ test/controllers/thesis_controller_test.rb | 41 ++++++++++++++++++++++ 2 files changed, 53 insertions(+) diff --git a/app/controllers/thesis_controller.rb b/app/controllers/thesis_controller.rb index d3cadc77..1382d4a3 100644 --- a/app/controllers/thesis_controller.rb +++ b/app/controllers/thesis_controller.rb @@ -119,6 +119,18 @@ def process_thesis def process_thesis_update thesis = Thesis.find(params[:id]) + + # A file may be deleted after the form loads. Drop stale delete rows to avoid RecordNotFound. + if params[:thesis]&.[](:files_attachments_attributes) + params[:thesis][:files_attachments_attributes].delete_if do |_k, attrs| + marked_for_delete = attrs['_destroy'] == '1' + attachment_id = attrs['id'] + missing_attachment = attachment_id.present? && !ActiveStorage::Attachment.exists?(attachment_id) + + marked_for_delete && missing_attachment + end + end + removed = deleted_file_list params[:thesis][:files_complete] = false if removed.count.positive? if thesis.update(thesis_params) diff --git a/test/controllers/thesis_controller_test.rb b/test/controllers/thesis_controller_test.rb index 04d50e92..f0aa4d32 100644 --- a/test/controllers/thesis_controller_test.rb +++ b/test/controllers/thesis_controller_test.rb @@ -993,4 +993,45 @@ def attach_files_to_records(tr, th) error_count = Thesis.where(publication_status: 'Publication error').count assert error_count == 0 end + + # ~~~~~~~~~~~~~~~~~~~~~ process_thesis_update race condition regression ~~~~~~~~~~~~~~~~~~~~~ + test 'process_thesis_update handles gracefully when an attachment marked for deletion no longer exists' do + sign_in users(:processor) + thesis = theses(:one) + f1 = Rails.root.join('test', 'fixtures', 'files', 'a_pdf.pdf') + thesis.files.attach(io: File.open(f1), filename: 'a_pdf.pdf') + thesis.save + thesis.reload + + # Get the attachment ID to mark for deletion + attachment_id = thesis.files.first.id + + # Simulate the race condition: Delete the attachment from the database + # (This could happen if another process deletes it between form open and submit) + ActiveStorage::Attachment.find(attachment_id).delete + + # Attempt to update the thesis with the deleted attachment marked for deletion + # This previously would crash with "undefined method 'blob' for nil:NilClass" + patch "/thesis/#{thesis.id}/process", + params: { + thesis: { + title: thesis.title, + files_attachments_attributes: { + '0' => { + id: attachment_id, + _destroy: '1' + } + }, + files_complete: false, + metadata_complete: false, + issues_found: false + } + } + + # Verify the update succeeded (redirects to thesis_process_path with success message) + assert_response :redirect + assert_redirected_to thesis_process_path + follow_redirect! + assert_select '.alert-banner.success', text: /changes.*have been saved/ + end end From a27b409aa1eb981c36aecc18e197239728e01864 Mon Sep 17 00:00:00 2001 From: Isra Jazairi Date: Fri, 18 Sep 2026 09:28:50 -0700 Subject: [PATCH 3/7] Address copilot code review feedback --- app/controllers/thesis_controller.rb | 44 +++++++++++++++++++++------- 1 file changed, 33 insertions(+), 11 deletions(-) diff --git a/app/controllers/thesis_controller.rb b/app/controllers/thesis_controller.rb index 1382d4a3..5ec7ae5e 100644 --- a/app/controllers/thesis_controller.rb +++ b/app/controllers/thesis_controller.rb @@ -120,20 +120,25 @@ def process_thesis def process_thesis_update thesis = Thesis.find(params[:id]) - # A file may be deleted after the form loads. Drop stale delete rows to avoid RecordNotFound. - if params[:thesis]&.[](:files_attachments_attributes) - params[:thesis][:files_attachments_attributes].delete_if do |_k, attrs| - marked_for_delete = attrs['_destroy'] == '1' - attachment_id = attrs['id'] - missing_attachment = attachment_id.present? && !ActiveStorage::Attachment.exists?(attachment_id) - - marked_for_delete && missing_attachment - end - end + # A file may be deleted after the form loads. Drop stale delete rows before update. + drop_stale_deleted_attachment_rows! removed = deleted_file_list params[:thesis][:files_complete] = false if removed.count.positive? - if thesis.update(thesis_params) + updated = false + retried_for_missing_attachment = false + + begin + updated = thesis.update(thesis_params) + rescue ActiveRecord::RecordNotFound => e + raise unless missing_deleted_attachment_race?(e) && !retried_for_missing_attachment + + retried_for_missing_attachment = true + drop_stale_deleted_attachment_rows! + retry + end + + if updated flash[:success] = "

Your changes to '#{thesis.title}' have been saved.

".html_safe if removed.count.positive? flash[:success] += '

The following files were removed from this thesis. They can still be found attached to their original transfer, via the following links:

    '.html_safe @@ -202,6 +207,23 @@ def deleted_file_list list end + def drop_stale_deleted_attachment_rows! + return unless params[:thesis]&.[](:files_attachments_attributes) + + params[:thesis][:files_attachments_attributes].delete_if do |_k, attrs| + marked_for_delete = attrs['_destroy'] == '1' + attachment_id = attrs['id'] + missing_attachment = attachment_id.present? && !ActiveStorage::Attachment.exists?(attachment_id) + + marked_for_delete && missing_attachment + end + end + + def missing_deleted_attachment_race?(error) + params[:thesis]&.[](:files_attachments_attributes).present? && + error.message.include?('ActiveStorage::Attachment') + end + def publication_candidates filter_theses_by_term(Thesis.in_review).to_a end From cc130a7372fb21fb17cd014c960b68fa03c0a82f Mon Sep 17 00:00:00 2001 From: Isra Jazairi Date: Fri, 18 Sep 2026 09:39:47 -0700 Subject: [PATCH 4/7] Add regression coverage for stale deleted attachment rows in thesis controller update --- test/controllers/thesis_controller_test.rb | 73 ++++++++++++++++++++++ 1 file changed, 73 insertions(+) diff --git a/test/controllers/thesis_controller_test.rb b/test/controllers/thesis_controller_test.rb index f0aa4d32..51f76514 100644 --- a/test/controllers/thesis_controller_test.rb +++ b/test/controllers/thesis_controller_test.rb @@ -1034,4 +1034,77 @@ def attach_files_to_records(tr, th) follow_redirect! assert_select '.alert-banner.success', text: /changes.*have been saved/ end + + # This regression test targets the narrow race window where: + # 1) the pre-update stale-row filter runs while the attachment still exists, and + # 2) the attachment disappears before nested attribute processing inside update. + # + # To force that sequence deterministically, we temporarily wrap Thesis#update. + # On the first update call for this thesis, the wrapper deletes the target + # attachment, then calls the original update implementation. That first call + # should raise ActiveRecord::RecordNotFound from nested attributes, which the + # controller rescues, prunes stale rows again, and retries once. + # + # Assertions that prove retry-path behavior: + # - deleted_during_first_update is true (deletion happened in the race window) + # - update_calls == 2 (first call failed, second call succeeded) + # - request still redirects with success flash + test 'process_thesis_update retries when attachment disappears between stale check and update' do + sign_in users(:processor) + + transfer = transfers(:valid) + thesis = theses(:publication_review_except_hold) + attach_files_to_records(transfer, thesis) + + attachment_id = thesis.files.first.id + thesis_id = thesis.id + update_calls = 0 + deleted_during_first_update = false + + Thesis.class_eval do + alias_method :__original_update_for_attachment_race_test, :update + define_method(:update) do |*args| + if id == thesis_id + update_calls += 1 + + if update_calls == 1 + ActiveStorage::Attachment.find_by(id: attachment_id)&.delete + deleted_during_first_update = true + end + end + + __original_update_for_attachment_race_test(*args) + end + end + + begin + patch thesis_process_update_path(thesis), + params: { + thesis: { + title: thesis.title, + files_attachments_attributes: { + '0' => { + id: attachment_id, + _destroy: '1' + } + }, + files_complete: false, + metadata_complete: false, + issues_found: false + } + } + ensure + Thesis.class_eval do + alias_method :update, :__original_update_for_attachment_race_test + remove_method :__original_update_for_attachment_race_test + end + end + + assert deleted_during_first_update + assert_equal 2, update_calls + assert_response :redirect + assert_redirected_to thesis_process_path + follow_redirect! + assert_select '.alert-banner.success', text: /changes.*have been saved/ + end end From dcbb4666a77771c7bd8b21930b471ba2990d5d11 Mon Sep 17 00:00:00 2001 From: Isra Jazairi Date: Fri, 18 Sep 2026 12:47:06 -0400 Subject: [PATCH 5/7] Fix issue in regression test Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- test/controllers/thesis_controller_test.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/controllers/thesis_controller_test.rb b/test/controllers/thesis_controller_test.rb index 51f76514..7d64fa87 100644 --- a/test/controllers/thesis_controller_test.rb +++ b/test/controllers/thesis_controller_test.rb @@ -1030,7 +1030,7 @@ def attach_files_to_records(tr, th) # Verify the update succeeded (redirects to thesis_process_path with success message) assert_response :redirect - assert_redirected_to thesis_process_path + assert_redirected_to thesis_process_path(thesis) follow_redirect! assert_select '.alert-banner.success', text: /changes.*have been saved/ end From 25598a49c78df32e2070c30ca3c41c7a15a2cb54 Mon Sep 17 00:00:00 2001 From: Isra Jazairi Date: Fri, 18 Sep 2026 09:58:58 -0700 Subject: [PATCH 6/7] Address copilot code review feedback --- test/controllers/thesis_controller_test.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/controllers/thesis_controller_test.rb b/test/controllers/thesis_controller_test.rb index 7d64fa87..00c379fa 100644 --- a/test/controllers/thesis_controller_test.rb +++ b/test/controllers/thesis_controller_test.rb @@ -1004,7 +1004,7 @@ def attach_files_to_records(tr, th) thesis.reload # Get the attachment ID to mark for deletion - attachment_id = thesis.files.first.id + attachment_id = thesis.files_attachments.first.id # Simulate the race condition: Delete the attachment from the database # (This could happen if another process deletes it between form open and submit) @@ -1056,7 +1056,7 @@ def attach_files_to_records(tr, th) thesis = theses(:publication_review_except_hold) attach_files_to_records(transfer, thesis) - attachment_id = thesis.files.first.id + attachment_id = thesis.files_attachments.first.id thesis_id = thesis.id update_calls = 0 deleted_during_first_update = false @@ -1103,7 +1103,7 @@ def attach_files_to_records(tr, th) assert deleted_during_first_update assert_equal 2, update_calls assert_response :redirect - assert_redirected_to thesis_process_path + assert_redirected_to thesis_process_path(thesis) follow_redirect! assert_select '.alert-banner.success', text: /changes.*have been saved/ end From 3e61f7fe5de29576c86ebbb5a325f9057e5c1ae3 Mon Sep 17 00:00:00 2001 From: Isra Jazairi Date: Fri, 18 Sep 2026 10:09:10 -0700 Subject: [PATCH 7/7] Exercise nil guard in regression testing --- test/controllers/thesis_controller_test.rb | 49 ++++++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/test/controllers/thesis_controller_test.rb b/test/controllers/thesis_controller_test.rb index 00c379fa..0fbc5d48 100644 --- a/test/controllers/thesis_controller_test.rb +++ b/test/controllers/thesis_controller_test.rb @@ -1107,4 +1107,53 @@ def attach_files_to_records(tr, th) follow_redirect! assert_select '.alert-banner.success', text: /changes.*have been saved/ end + + test 'process_thesis_update handles attachment deleted after stale-row check and before deleted_file_list' do + sign_in users(:processor) + + transfer = transfers(:valid) + thesis = theses(:publication_review_except_hold) + attach_files_to_records(transfer, thesis) + + attachment_id = thesis.files_attachments.first.id + deleted_after_stale_check = false + + ThesisController.class_eval do + alias_method :__original_drop_stale_rows_for_nil_attachment_test, :drop_stale_deleted_attachment_rows! + define_method(:drop_stale_deleted_attachment_rows!) do + __original_drop_stale_rows_for_nil_attachment_test + ActiveStorage::Attachment.find_by(id: attachment_id)&.delete + deleted_after_stale_check = true + end + end + + begin + patch thesis_process_update_path(thesis), + params: { + thesis: { + title: thesis.title, + files_attachments_attributes: { + '0' => { + id: attachment_id, + _destroy: '1' + } + }, + files_complete: false, + metadata_complete: false, + issues_found: false + } + } + ensure + ThesisController.class_eval do + alias_method :drop_stale_deleted_attachment_rows!, :__original_drop_stale_rows_for_nil_attachment_test + remove_method :__original_drop_stale_rows_for_nil_attachment_test + end + end + + assert deleted_after_stale_check + assert_response :redirect + assert_redirected_to thesis_process_path(thesis) + follow_redirect! + assert_select '.alert-banner.success', text: /changes.*have been saved/ + end end