From 94d2ee908dfe34ffe719e2e78a9664ec8f885b8a Mon Sep 17 00:00:00 2001
From: ghost <49853598+JSONbored@users.noreply.github.com>
Date: Mon, 15 Jun 2026 12:43:08 -0700
Subject: [PATCH] fix(jobs): enqueue jobs after transaction commit to fix
 SyncJob deserialization race (#2354)

* fix(jobs): enqueue jobs after transaction commit to fix SyncJob deserialization race

Syncable#sync_later creates a Sync row and enqueues SyncJob inside the
same transaction. Rails 7.2 deferred such enqueues until commit by
default; Rails 8.0 changed the default to enqueue immediately and 8.1
left the global config toggle non-functional, so a worker could dequeue
the job before COMMIT, fail to resolve the Sync GlobalID
(ActiveJob::DeserializationError), and have it silently dropped by
discard_on -- surfacing as stuck syncs after the Rails 8.1 upgrade.

Set enqueue_after_transaction_commit = true on ApplicationJob (the
Rails 8.2 default, inherited by every job) and drop the dead :never
symbol override on DestroyJob. Add regression and invariant tests.

* test(jobs): use OpenStruct for DestroyJob failure case

Replace the bare mock and the respond_to? expectation (an implementation
detail of how DestroyJob probes the model) with an OpenStruct that genuinely
responds to scheduled_for_deletion. Keeps only the command-facing assertions:
destroy raises and update! is called with scheduled_for_deletion: false.
Matches the repo convention of preferring OpenStruct for mock instances.

* test(snaptrade): assert connection-cleanup enqueue defers until commit

SnaptradeAccount#after_destroy enqueues SnaptradeConnectionCleanupJob, which
references the item/account by id. Before the ApplicationJob fix, Rails 8.1
enqueued it immediately inside the destroy transaction, so a worker could run
before COMMIT, see the not-yet-deleted row in its shared-authorization guard,
skip the provider call, and leak the SnapTrade connection. This regression
test destroys an account inside a transaction and asserts the job is not
enqueued until the transaction commits.
---
 app/jobs/application_job.rb                | 14 +++++++++
 app/jobs/destroy_job.rb                    |  6 +++-
 test/interfaces/syncable_interface_test.rb | 12 ++++++++
 test/jobs/application_job_test.rb          | 36 ++++++++++++++++++++++
 test/jobs/destroy_job_test.rb              | 32 +++++++++++++++++++
 test/models/snaptrade_account_test.rb      | 30 ++++++++++++++++++
 6 files changed, 129 insertions(+), 1 deletion(-)
 create mode 100644 test/jobs/application_job_test.rb
 create mode 100644 test/jobs/destroy_job_test.rb

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