Batch review refinements: logical counters, optional migration, empty-batch completion - #6
Merged
Merged
Conversation
rosa
force-pushed
the
batch-refinements
branch
4 times, most recently
from
August 19, 2026 16:50
506f2d7 to
0eced2d
Compare
Sidekiq Pro and GoodJob both report logical jobs, and it matches what you enqueued: a job that fails twice and then succeeds is one job, not three. Retries re-enqueued via retry_on keep their active_job_id, so the increment can skip active_job_ids the batch has already counted without touching the completion machinery: every attempt still gets its own tracking row, and the batch still finishes when none are left. Only jobs that have executed before pay the already-counted lookup, so first enqueues stay as cheap as they were. And while a retry coexists with its not-yet-finished previous attempt, both attempts hold tracking rows, so the counters derived from them clamp at zero instead of dipping negative during that window. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
`rails solid_queue:update` now copies a migration that adds the batch tables and the jobs' batch_id, guarded with if_not_exists so it no-ops for fresh installs, which get all of it with the base schema. Until an existing installation runs it, everything works as before: jobs enqueue, finish, fail and get destroyed without any batch bookkeeping, starting a batch raises with instructions, and the dispatcher swaps the stalled-batches sweep for a deprecation warning, once per process. The schema check memoizes only success, so a deployment that migrates while running starts sweeping on the next tick without a restart. Replacing the tracking row's dependent: :destroy with a callback guarded like the others also spares every unbatched job destroy a query for a tracking row that can't exist. The tests recreate a not-yet-migrated app by reverting the actual migration users get, proving in passing that it's reversible and matches the base schema on all three databases. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Verified the completion CAS's cross-database behavior with a controlled two-connection interleaving: an adder holding the batch row lock with freshly inserted tracking rows while a completion check blocks on it. - PostgreSQL at READ COMMITTED wrongly wins the CAS (the lock-wait re-evaluation keeps the original snapshot for the NOT IN subquery), and the existing re-check catches it because a new statement gets a fresh snapshot. - PostgreSQL at REPEATABLE READ fails loudly with a serialization error instead, so nothing finishes wrongly. - MySQL declines the CAS correctly at both isolation levels, even with a deliberately staled transaction snapshot: InnoDB reads subqueries inside an UPDATE from the latest committed data. So the plain-SELECT re-check is sufficient everywhere. A FOR UPDATE re-check would be unconditionally fresh by construction, but on MySQL a locking read over the batch's empty executions range takes a gap lock that can briefly block other batches' adders, buying insurance nothing currently needs. Record all of this in the comment instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The Batch class mixes its lifecycle (enqueue, start, complete, fire callbacks) with the repair mechanics that fix batches stranded outside that happy path. Move the sweep and its completion grace period into a Sweepable concern, mirroring how Process keeps its analogous stalled cleanup in Prunable, so the core class tells one story. Also simplify AlreadyFinished to define its message at the raise site. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
start_batch already ends by checking completion — that's how a batch whose jobs all finished before it started gets finished — and a batch with no jobs at all can take the same exit. Empty batches now finish as soon as they start, without needing a worker to drain a no-op job first: callbacks fire right away, total_jobs stays honestly at zero instead of counting the EmptyJob, and the EmptyJob queue configuration goes away. Sidekiq Pro and GoodJob treat empty batches the same way. This also removes the sweeper's completion grace period, which existed to give the EmptyJob's transaction-deferred enqueue time to become visible after the start. Regular jobs can't recreate that window: their tracking rows are committed before the batch's enqueued_at is stamped. The only behavior removed is the window in which an empty batch sat unfinished until a worker performed the no-op, during which jobs could still join it. That window was racy — the moment the EmptyJob ran, late enqueues raised AlreadyFinished — so it didn't support deferred filling so much as let it work sometimes. Now a batch that starts empty is finished, deterministically, and a job instantiated in the batch's block needs the batch still running when it's finally enqueued. start_batch keeps a reload before its completion check: update_all doesn't refresh the instance, check_completion consults enqueued? in memory, and previously the refresh happened only incidentally, while reading total_jobs to decide on the EmptyJob. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Extract callback serialization and enqueueing into a Callbacks concern, move the without_executions scope next to the completion update that needs it join-free, and store metadata with store. Rename the lifecycle internals so the guarded plain verbs mirror each other and follow the batch's own finished vocabulary, which failing batches share too: start_batch → start, check_completion → finish, finalize_completion → finalize, with mark_as_enqueued extracted from start. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
BatchExecution is the batch family's execution table — a transient, job-keyed row per outstanding attempt — so make it one: subclassing Execution gives it the required belongs_to :job and lets creation reuse the base insert machinery via assumable_attributes_from_job, replacing a hand-rolled row builder. That builder also carried a dead branch for Active Job instances, a leftover from the old buffer-based design where tracking rows were created from buffered Active Jobs after enqueue_all stamped their provider_job_id; both call sites pass SolidQueue::Job rows today. Counting new logical jobs no longer queries the database: a job whose serialized executions is positive was already counted when it first joined the batch, since retries keep their active_job_id and batch across re-enqueues. Unlike looking prior attempts up, this stays correct when those attempts' rows have been cleared, and it accepts a small trade: an already-executed job enqueued into a different batch won't bump that batch's total_jobs. Also rename the leaked-row scopes to read as what they match (with_finished_jobs, with_failed_jobs) and drop a redundant foreign_key option on Job's side of the association. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Extract each of sweep_stalled's three passes into a method named after what it repairs — sweep_stale_executions, finish_stalled_batches, start_stalled_batches — and rename the instrumentation payload to match: repaired/size/started said nothing about what was counted, and mixed units besides (execution rows in the first, batches in the other two). Now each metric carries its unit: stale_executions, finished_batches, started_batches. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Extra keyword arguments still become the batch's metadata, but a user who intuitively passes metadata: directly — mirroring description: — used to get it silently nested under a "metadata" key. Now both styles work and merge cleanly if combined. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
rosa
force-pushed
the
batch-refinements
branch
from
August 19, 2026 17:09
0eced2d to
8361aba
Compare
jpcamara
reviewed
Aug 20, 2026
| # Fresh installs create all of this with the base schema, so skip | ||
| # anything that already exists | ||
| add_column :solid_queue_jobs, :batch_id, :bigint, if_not_exists: true | ||
| add_index :solid_queue_jobs, :batch_id, if_not_exists: true |
Owner
There was a problem hiding this comment.
This is the one thing that's stuck out to me in this PR. on PG, the safe way to run a migration for an index is to run it concurrently.
This was definitely an issue already, in principle, just sticks out more here. What do DB-based rails features normally do in this type of situation? For instance, if someone had a highly active jobs table, it's likely this would lock up the db without making it a concurrent index run, inside of a disabled ddl.
If someone is using a gem like strong_migrations, that shouldn't be a problem because it would catch it, so maybe it's just up to users to know?
Owner
Honestly, it seems like possibly an artifact of an earlier state where it couldn't handle finishing the batch without having some kind of job in it. I can't come up with a reason it needs to exist anymore. My only other thinking was something related to |
Owner
|
Great updates, ty @rosa ! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is my review round on top of
batch-poc, moved out of your branch (which I've reset to yourae396b1, sorry for the noise there!).Here's a quick overview of the interesting changes:
Count logical jobs in batch counters instead of attempts. We discussed this one briefly via Bluesky.
retry_onre-enqueues keep theiractive_job_id, socreate_all_from_jobsskips ids the batch has already counted: a job that fails twice and then succeeds contributes 1 tototal_jobs, not 3, matching Sidekiq Pro and GoodJob. The completion machinery is untouched.Ship the batches schema as an optional migration until 2.0 - this is also something we discussed, and expected. The migration is optional until 2.0.
Extract batch sweeping into a Sweepable concern — mirrors how
Processkeeps its stalled cleanup inPrunable.Finish empty batches at start instead of enqueueing an
EmptyJob- this is the one change that might not be right! I think it was a leftover from the initial buffer-based approach. At least I couldn't find a good reason why theEmptyJobwas necessary.start_batchalready ended by checking completion, which is how a batch whose jobs all drained before it started gets finished, so a batch with no jobs takes the same exit: callbacks fire right away,total_jobsstays at zero, vs. before, where it would report always 1 because of theEmptyJob. No worker (orEmptyJobqueue configuration) is needed, and the sweeper'sCOMPLETION_GRACE, which only existed to give theEmptyJob's deferred enqueue time to land, goes away.The one thing this removes is the window where an empty batch sat unfinished until a worker drained the no-op, during which jobs could still join it; that window was racy (the moment the
EmptyJobran, late enqueues raisedAlreadyFinished), so it didn't really support deferred filling; it was more like something working by chance. If you had a use case in mind for going through a job here, let me know. I have totally missed it 😅Some renaming around the
Batchclass — callback serialization and enqueueing extracted into aCallbacksconcern, and the lifecycle internals renamed to guarded plain verbs that mirror each other and match the schema's vocabulary:start,finish,finalize.