Skip to content

Discard batches whose enclosing transaction rolls back - #7

Open
jpcamara wants to merge 1 commit into
mainfrom
batch-rollback-cleanup
Open

Discard batches whose enclosing transaction rolls back#7
jpcamara wants to merge 1 commit into
mainfrom
batch-rollback-cleanup

Conversation

@jpcamara

Copy link
Copy Markdown
Owner

Draft for review — opened against my own fork, not upstream.

The problem

SolidQueue::Batch#enqueue persists the batch on Solid Queue's connection:

transaction do
  save! if new_record?
  #...
end

When Solid Queue has its own connection — the default, and what the install generator configures — that's a genuinely separate transaction. It commits while the surrounding application transaction is still open, so a rollback never reaches it.

ActiveRecord::Base.transaction do
  SolidQueue::Batch.enqueue { SomeJob.perform_later }
  raise ActiveRecord::Rollback
end

What's left behind, measured on PostgreSQL:

before after
batch row survives at :pending removed
jobs that weren't deferred survive and run removed

The leftovers aren't inert. Maintenance can't distinguish such a batch from one whose creating process crashed after enqueueing jobs, so once it's older than the stalled window it starts it, finds nothing pending, completes it, and fires on_success — for a transaction that rolled back:

before sweep    status=pending    total_jobs=1
job ran         status=pending    total_jobs=1  ran=["DocJob ran"]
after sweep     status=completed  total_jobs=1
after workers   ran=["DocJob ran", "on_success FIRED"]

enqueue_after_transaction_commit doesn't help — it defers jobs, but Solid Queue writes the batch row itself, before any of that applies.

The fix

Register cleanup on the open transactions the batch's own writes aren't part of:

def enclosing_transactions
  return [] unless ActiveRecord.respond_to?(:all_open_transactions)

  ActiveRecord.all_open_transactions.reject { |t| t.connection.pool == self.class.connection_pool }
end

Mirrors how ActiveRecord.after_all_transactions_commit enumerates transactions, using Transaction#after_rollback for the other side.

Transactions are compared by connection pool, not by database. A queue: entry pointed at the same database as the app still gets its own pool, so it still commits independently and still needs this. Conversely, when Solid Queue shares the app's connection the list is empty and this is a no-op — single-database setups behave exactly as before.

Verification

Four scenarios × both connection layouts × Rails 7.2 / 8.0 / 8.1 / 8.2, on PostgreSQL:

rollback, empty        -> batch=GONE
rollback, 1 job        -> batch=GONE jobs=0
commit, 1 job          -> batch=enqueued jobs=1   (unchanged)
no txn, 1 job          -> batch=enqueued jobs=1   (unchanged)
nested savepoint rb    -> batch=GONE

Identical output for shared-pool and separate-pool configurations on every version. Full suite green on 7.2/8.0/8.1/main; rubocop clean. Five regression tests added, all of which fail without the change.

Known gaps

  • Rails 7.1 isn't covered. Transaction#after_rollback arrived in 7.2. The older add_transaction_record could work but needs a sentinel implementing rolledback!/trigger_transactional_callbacks?, which is version-sensitive. Tests skip there. Worth noting 7.1 is where this is worst: after_commit :start, on: :create fires on the queue-side commit, so a rolled-back empty batch completes and enqueues on_success immediately, with no sweeper involved.
  • Jobs a worker already claimed are left alone — a running job can't be recalled. Reported as claimed_jobs in the discard_rolled_back_batch instrumentation payload.
  • Picks the transactions it can see. If an app rolls back a transaction on a connection Solid Queue never observed as open, cleanup won't run — the sweeper remains the backstop.

This is independent of the total_jobs != 0 sweeper guard on batch-deferred-enqueue-sweep; that one covers a batch in a transaction that commits but outlives the stalled window.

🤖 Generated with Claude Code

A batch created inside an application transaction is written on Solid
Queue's own connection. When that's a different connection—the default,
and what the install generator configures—the batch commits as soon as
it's enqueued, and an application rollback never reaches it. The batch row
survives, and so does every job in it that wasn't deferred until commit.

Those leftovers aren't inert. Maintenance can't tell such a batch from one
whose creating process crashed after enqueueing jobs, so once it's older
than the stalled window it starts it, finds nothing pending, completes it,
and fires its callbacks—reporting success for work that was rolled back.
The jobs that survived run too, against data that no longer exists.

Register cleanup on the open transactions the batch's own writes aren't
part of, so the batch and its jobs are removed if any of them rolls back.
Transactions are compared by connection pool rather than by database: a
queue database pointed at the same database as the app still gets its own
connection, and so still commits independently. When Solid Queue shares
the application's connection there's nothing to register—the batch is
already inside that transaction and rolls back with it—so this is a no-op
there and single-database setups keep behaving exactly as they did.

Jobs a worker already claimed are left alone, since a running job can't be
recalled; they're reported in the instrumentation payload instead.

Rails 7.1 has no transaction rollback hooks, so batches there keep the old
behaviour.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant