diff --git a/app/jobs/application_job.rb b/app/jobs/application_job.rb index 25727230f..104f5cd39 100644 --- a/app/jobs/application_job.rb +++ b/app/jobs/application_job.rb @@ -1,4 +1,18 @@ class ApplicationJob < ActiveJob::Base + # Defer enqueuing until the surrounding Active Record transaction commits, so a + # job is never picked up by a worker before the records it references (by + # GlobalID) are committed and visible on the worker's connection — and is + # dropped entirely if the transaction rolls back. Without this, a job enqueued + # inside a transaction can be dequeued before COMMIT, fail to load its + # arguments (ActiveJob::DeserializationError), and be silently dropped by the + # `discard_on` below — the "stuck sync" regression for SyncJob enqueued in + # Syncable#sync_later. + # + # This is the Rails 8.2 default. On 8.1 the global + # `config.active_job.enqueue_after_transaction_commit` toggle is non-functional, + # so we set the class attribute on the base job, which every job inherits. + self.enqueue_after_transaction_commit = true + retry_on ActiveRecord::Deadlocked discard_on ActiveJob::DeserializationError queue_as :low_priority # default queue diff --git a/app/jobs/destroy_job.rb b/app/jobs/destroy_job.rb index ba937e61f..8d9483536 100644 --- a/app/jobs/destroy_job.rb +++ b/app/jobs/destroy_job.rb @@ -1,6 +1,10 @@ class DestroyJob < ApplicationJob queue_as :low_priority - self.enqueue_after_transaction_commit = :never + # Inherits enqueue_after_transaction_commit = true from ApplicationJob. (This + # previously read `= :never`, the removed Rails 7.2 symbol API; under 8.1 that + # symbol is truthy, so it already deferred — the explicit line was dead and + # misleading.) Deferring is correct here: destroy after the enclosing + # transaction commits, never against an uncommitted/rolled-back record. def perform(model) model.destroy diff --git a/test/interfaces/syncable_interface_test.rb b/test/interfaces/syncable_interface_test.rb index df1420529..a972f375d 100644 --- a/test/interfaces/syncable_interface_test.rb +++ b/test/interfaces/syncable_interface_test.rb @@ -18,6 +18,18 @@ module SyncableInterfaceTest @syncable.perform_sync(mock_sync) end + test "sync_later does not enqueue SyncJob while a surrounding transaction is still open" do + job_enqueued_mid_transaction = false + + ActiveRecord::Base.transaction do + @syncable.sync_later + job_enqueued_mid_transaction = queue_adapter.enqueued_jobs.any? { |j| j[:job] == SyncJob } + end + + assert_not job_enqueued_mid_transaction, "SyncJob was enqueued inside an open transaction (GlobalID race)" + assert_enqueued_with(job: SyncJob) + end + test "second sync request widens existing pending window" do later_start = 2.days.ago.to_date first_sync = @syncable.sync_later(window_start_date: later_start, window_end_date: later_start) diff --git a/test/jobs/application_job_test.rb b/test/jobs/application_job_test.rb new file mode 100644 index 000000000..75fffcb3d --- /dev/null +++ b/test/jobs/application_job_test.rb @@ -0,0 +1,36 @@ +require "test_helper" + +class ApplicationJobTest < ActiveJob::TestCase + # Throwaway subclass used only to exercise ApplicationJob's enqueue policy + # without depending on any real job's side effects. + class CanaryJob < ApplicationJob + def perform; end + end + + test "defers enqueue until the surrounding transaction commits" do + enqueued_mid_transaction = nil + + ActiveRecord::Base.transaction do + CanaryJob.perform_later + enqueued_mid_transaction = enqueued_jobs.any? { |job| job[:job] == CanaryJob } + end + + assert_not enqueued_mid_transaction, "job was enqueued before the transaction committed" + assert_enqueued_with job: CanaryJob + end + + test "drops the enqueue when the surrounding transaction rolls back" do + assert_no_enqueued_jobs do + ActiveRecord::Base.transaction do + CanaryJob.perform_later + raise ActiveRecord::Rollback + end + end + end + + test "enqueues immediately when there is no surrounding transaction" do + assert_enqueued_with job: CanaryJob do + CanaryJob.perform_later + end + end +end diff --git a/test/jobs/destroy_job_test.rb b/test/jobs/destroy_job_test.rb new file mode 100644 index 000000000..54633495e --- /dev/null +++ b/test/jobs/destroy_job_test.rb @@ -0,0 +1,32 @@ +require "test_helper" +require "ostruct" + +class DestroyJobTest < ActiveJob::TestCase + test "destroys the model" do + model = mock + model.expects(:destroy).once + + DestroyJob.perform_now(model) + end + + test "resets scheduled_for_deletion when the destroy fails" do + model = OpenStruct.new(scheduled_for_deletion: true) + model.stubs(:destroy).raises(ActiveRecord::RecordNotDestroyed.new("nope")) + model.expects(:update!).with(scheduled_for_deletion: false).once + + DestroyJob.perform_now(model) + end + + test "inherits the deferred-enqueue policy from ApplicationJob" do + account = accounts(:depository) + enqueued_mid_transaction = nil + + ActiveRecord::Base.transaction do + DestroyJob.perform_later(account) + enqueued_mid_transaction = enqueued_jobs.any? { |job| job[:job] == DestroyJob } + end + + assert_not enqueued_mid_transaction, "DestroyJob enqueued before the transaction committed" + assert_enqueued_with job: DestroyJob + end +end diff --git a/test/models/snaptrade_account_test.rb b/test/models/snaptrade_account_test.rb index 48efc0f30..68172bde3 100644 --- a/test/models/snaptrade_account_test.rb +++ b/test/models/snaptrade_account_test.rb @@ -1,6 +1,8 @@ require "test_helper" class SnaptradeAccountTest < ActiveSupport::TestCase + include ActiveJob::TestHelper + setup do @family_a = families(:dylan_family) @family_b = families(:empty) @@ -71,4 +73,32 @@ class SnaptradeAccountTest < ActiveSupport::TestCase ) end end + + # Regression: the after_destroy callback enqueues SnaptradeConnectionCleanupJob, + # which references the account/item by id. If it is enqueued before the destroy + # transaction commits (Rails 8.1's immediate-enqueue default), a worker can run + # before COMMIT: its "do other accounts still share this authorization?" guard + # then sees the not-yet-deleted row, skips the provider call, and leaks the + # SnapTrade connection. Enqueuing must be deferred until commit. + test "connection cleanup job is deferred until the destroy transaction commits" do + account = SnaptradeAccount.create!( + snaptrade_item: @item_a, + snaptrade_account_id: "cleanup_uuid", + snaptrade_authorization_id: "auth_cleanup", + name: "Brokerage", + currency: "USD", + current_balance: 1000 + ) + + enqueued_mid_transaction = nil + + ActiveRecord::Base.transaction do + account.destroy! + enqueued_mid_transaction = enqueued_jobs.any? { |job| job[:job] == SnaptradeConnectionCleanupJob } + end + + assert_not enqueued_mid_transaction, + "SnaptradeConnectionCleanupJob was enqueued before the destroy committed (would race COMMIT and leak the provider connection)" + assert_enqueued_with job: SnaptradeConnectionCleanupJob + end end