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.
This commit is contained in:
ghost
2026-06-15 12:43:08 -07:00
committed by GitHub
parent c29380ce57
commit 94d2ee908d
6 changed files with 129 additions and 1 deletions

View File

@@ -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)