Skip to content

Merge latest batch refinements from rails/solid_queue#142 - #16

Open
jpcamara wants to merge 12 commits into
mainfrom
batch-refinements
Open

Merge latest batch refinements from rails/solid_queue#142#16
jpcamara wants to merge 12 commits into
mainfrom
batch-refinements

Conversation

@jpcamara

@jpcamara jpcamara commented Aug 20, 2026

Copy link
Copy Markdown
Member

Summary

Brings main up to date with the newest batch work from rails/solid_queue#142, on top of the review-fixes already merged in #14.

main already had the original batch feature (#8) and the Jul 29 race/accounting series (#14). This PR cherry-picks the follow-up from jpcamara/batch-poc (and the upstream solid_queue:update generator those commits depend on). Fork-specific work (concurrency max_blocked, prune/semaphore guards, security bumps) is left alone.

Batch-file tree matches current jpcamara/batch-poc. Not included: fiber workers, supervisor/pool refactors, and the 1.6.0 bump that also live on that branch.

Notable changes:

  • Count logical jobs in batch counters, not attempts/retries
  • Finish empty batches at start (no more EmptyJob)
  • Optional batches schema via rails solid_queue:update until 2.0 (if_not_exists, so existing installs that already have the tables no-op)
  • BatchExecution rebuilt on Execution; sweep split into named phases
  • Explicit metadata: hash in Batch.enqueue
  • Batch class cleanup (Callbacks / Sweepable / Status)

Test plan

  • bundle exec rubocop --parallel — clean
  • TARGET_DB=sqlite bin/rails test — 322 runs, 0 failures, 0 errors
  • CI green across the matrix (mysql/postgres/sqlite)
  • Existing apps that already have batch tables: rails solid_queue:update + migrate is a no-op (if_not_exists)
  • Empty batch finishes immediately and fires callbacks without a worker draining a no-op job
  • SolidQueue::Batch.enqueue(metadata: { stage: 1 }) stores metadata at the top level, not nested under "metadata"
  • A job that fails then succeeds increments total_jobs once

Made with Cursor

rosa and others added 12 commits August 20, 2026 17:37
@rosa @claude @jpcamara
Add an update generator to copy new migrations
49e7137 
Future schema changes will ship as regular migrations, optional at
first and required in the next major version. `rails solid_queue:update`
(or the solid_queue:update generator directly) copies any new migration
files from the gem to the application, honoring the database the app
uses for Solid Queue via the --database option.

Together with this, add a deprecator for Solid Queue, registered with
the application so it follows the app's deprecation behavior settings,
and a warning helper that features guarded behind pending migrations
can use to instruct users to update.

Extracted from earlier work on linking claimed executions to processes
by name, which ended up not being needed.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@jpcamara @claude
Align the batches README section with the house style
13793bc 
Unwrap paragraphs to single lines, use ruby fences and Solid Queue in
prose, document description: and metadata handling and instance-callback
arguments, nest the section in the ToC, and add a recurring.yml entry
for clearing finished batches that mirrors the installer's template.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P39PSB74nP3a8B2F7Z5xvz
@rosa @claude @jpcamara
Count logical jobs in batch counters instead of attempts
2fcaa92 
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>
@rosa @claude @jpcamara
Ship the batches schema as an optional migration until 2.0
f64a105 
`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>
@rosa @claude @jpcamara
Document why the batch completion re-check is database-proof
3c37d2a 
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>
@rosa @claude @jpcamara
Extract batch sweeping into a Sweepable concern
ae0b20b 
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>
@rosa @claude @jpcamara
Finish empty batches at start instead of enqueueing an EmptyJob
0cf601f 
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>
@rosa @claude @jpcamara
Clean up the Batch class
ce47869 
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>
@rosa @claude @jpcamara
Rebuild BatchExecution on top of Execution
280bb77 
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>
@rosa @claude @jpcamara
Split the batch sweep into named phases with self-describing metrics
fe99ba9 
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>
@rosa @claude @jpcamara
Accept an explicit metadata: hash in Batch.enqueue
2dcc882 
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>
@jpcamara @claude
Reset pooled connections when flipping the batches schema in tests
7a2b0a3 
Dropping and re-adding the jobs table's batch_id invalidates cached
prepared statements whose SQL text didn't change, and PostgreSQL raises
PreparedStatementCacheExpired when one is reused inside a transaction,
where the adapter can't silently replan. Which statements are cached
depends on which tests ran first, so this only failed on some CI seeds.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@olivier-wb
olivier-wb self-requested a review August 20, 2026 21:51
olivier-wb
olivier-wb approved these changes Aug 21, 2026
View reviewed changes

@olivier-wb olivier-wb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Comment thread app/models/solid_queue/batch/callbacks.rb
end

def enqueue_callback_job(callback_name)
if callback = send(callback_name)

@olivier-wb olivier-wb Aug 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use public_send here?

Suggested change
if callback = send(callback_name)
if callback = public_send(callback_name)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@olivier-wb olivier-wb olivier-wb approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jpcamara @olivier-wb @rosa