Batch support: fix completion races, accounting, and API polish - #14
Merged
Conversation
Batches could get permanently stuck or lose callbacks through several races, all fixed without locking the batch row outside a single once-per-batch moment: - Completion keeps the single-statement CAS (exactly one finisher matches; losers resolve with a 0-row update) but shares its transaction with counters and callback enqueueing, so a crash can't leave a finished batch without callbacks - After winning the CAS, executions are re-checked with a current snapshot: on PostgreSQL READ COMMITTED a blocked finisher re-evaluates NOT EXISTS against its original snapshot and could otherwise finish a batch that just gained jobs from a concurrent adder - Adding jobs to a finished batch is prevented lock-free: the existing total_jobs increment gains an `unfinished` condition, so it and the completion CAS contend on the same row and exactly one wins; losing rolls back the enqueue transaction with AlreadyFinished - The increment runs before inserting tracking rows: execution inserts take a shared lock on the batch row (FK validation) and upgrading to exclusive deadlocked concurrent adders on MySQL (163/200 in stress tests); exclusive-first serializes them cleanly - start_batch marks the batch started before enqueueing the empty job and sweeps for completion afterwards, closing the window where jobs finish before enqueued_at is set and nothing ever finishes the batch; the start transition is single-winner so concurrent sweepers can't enqueue duplicate empty jobs - Batch.sweep_stalled is the safety net for work that can't finish through the normal flow: bulk-discarded jobs, processes that died before starting their batch, and completions whose callback enqueueing failed - Completion and sweeps emit finish_batch and sweep_stalled_batches events Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P39PSB74nP3a8B2F7Z5xvz
In-flight counters double counted failures: failed jobs have also lost their batch execution, so completed_jobs (total - pending) included them while failed_jobs counted them again, letting progress_percentage exceed 100% and report completion with jobs still running. Progress is now (total - pending) / total, which needs one COUNT instead of three and is consistent before and after finishing. Also: failed counts reuse the Job.failed scope, status returns symbols like Job#status, and the empty_executions scope stays join-free so update_all keeps it in the completion CAS's own WHERE clause. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P39PSB74nP3a8B2F7Z5xvz
Jobs join the batch that's active when they're enqueued, not when they're instantiated: a job built outside the block and enqueued inside it joins the batch, and vice versa. The active context takes precedence so reused instances rebind to the current batch, falling back to prior membership so retries stay in theirs. Bulk enqueues bypass per-job enqueue, so Job.enqueue_all captures the context itself. Capture must run before Rails 7.2's enqueue_after_transaction_commit deferral or deferred jobs would silently miss their batch — the include is nested like Rails' own initializer so BatchId deterministically wraps outside EnqueueAfterTransactionCommit. This hazard is why capture originally lived in initialize; with the ordering guaranteed, enqueue time keeps the better semantics without the initialize override. Also: serialize only writes batch keys when set (sparing dead JSON on every non-batch job), and the batch accessor memoizes by id so a nil read before enqueue doesn't hide a batch assigned later. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P39PSB74nP3a8B2F7Z5xvz
- Tracking rows are created before dispatching in the bulk path, so jobs discarded by concurrency conflicts clean up via dependent: :destroy and count the same as in the single-job path (previously perform_later counted them and perform_all_later didn't) - Batch executions get on_delete: :cascade foreign keys like every other execution table; bulk discards delete jobs without callbacks, and the cascade removes their tracking rows declaratively so the sweep can finish the batch (Execution itself stays untouched) - The failure-side concern moves to FailedExecution::Batchable: single-owner concerns live under their owner like Job::Batchable, and failed executions are the only terminal execution type, so they're the only place batch tracking ends - Tracking failures emit batch_progress_error events instead of swallowing errors with a bare log line, and rescue only ActiveRecord::ActiveRecordError so unexpected errors propagate Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P39PSB74nP3a8B2F7Z5xvz
ConcurrencyMaintenance generalizes into Dispatcher::Maintenance running both concurrency and batch routines on one timer at concurrency_maintenance_interval, individually toggleable via the concurrency_maintenance and batch_maintenance options — batch support adds no maintenance thread or worst-case database connection to the dispatcher. ConcurrencyMaintenance remains as a compatibility shim with its original signature and behavior, so no shipped API is removed. The LogSubscriber renders the new batch events: finish_batch, sweep_stalled_batches and batch_progress_error. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P39PSB74nP3a8B2F7Z5xvz
Every load-bearing line in batch completion has a test that fails if it's removed, verified by sabotage (breaking each guard makes its test go red): - Concurrent completion checks finish a batch exactly once, with exactly one set of callbacks (kills the CAS conditions if weakened; also caught a where.missing refactor that silently moved `unfinished` out of the UPDATE's own WHERE on PostgreSQL) - Concurrent adders keep exact accounting and hit no deadlocks (the reverted statement order fails with 33/40 adds deadlocked on MySQL) - A completion check racing a concurrent adder does not finish the batch (deterministically reproduces the PostgreSQL stale-snapshot interleaving; removing the post-CAS re-check wrongly finishes it) - start_batch sweeps up jobs that finished before the batch was started, and is single-winner against stale instances - sweep_stalled finishes bulk-discarded batches and starts batches whose creating process died - In-flight counters stay bounded and consistent with failures present - Batch membership follows enqueue-time context in both directions, reused instances rebind, the accessor doesn't cache a stale nil, and BatchId stays ahead of EnqueueAfterTransactionCommit in the ancestor chain - Conflict-discarded jobs count identically for single and bulk enqueues - Dispatcher batch maintenance is optional and rides the shared timer; ConcurrencyMaintenance keeps its original signature Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P39PSB74nP3a8B2F7Z5xvz
Fixes the callback examples — callbacks are enqueued with their original arguments and use the batch accessor; the documented perform(batch) signature raised ArgumentError. Documents enqueue-time batch membership, counter semantics (retry attempts count toward total_jobs), discard and manual-retry behavior, callback set() timing, batch maintenance and the recurring-task alternative, clearing batches, and a migration snippet for existing installations. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P39PSB74nP3a8B2F7Z5xvz
Found by exercising real batches in a Resque-default dummy app: Batch::EmptyJob inherits the host's ApplicationJob and enqueued to Redis, and callback jobs enqueue via their class's current adapter in the worker process, so they leaked to the app default too. EmptyJob now pins its adapter, and callbacks enqueue directly through SolidQueue::Job so they stay in Solid Queue and atomic with the finishing transaction regardless of adapter configuration. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P39PSB74nP3a8B2F7Z5xvz
test_empty_batches_fire_callbacks waited 2 seconds where every other test in the file waits 5, and it's the one batch test that flakes on CI: four empty batches each need an empty job claimed, performed and finished, then a callback job claimed and performed, which doesn't reliably fit 2 seconds on shared runners with SQLite's single writer (the outermost batch's callback misses the window). The only batch-owned failure across the full starburstlabs CI matrix; the remaining red legs are upstream (continuation test on rails_main, concurrency-controls and forked-lifecycle timing flakes) and fail on the base branch too. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P39PSB74nP3a8B2F7Z5xvz
The persistent rails-7.1 + SQLite CI failures (present on the base branch for months) fit one mechanism: removing a batch's tracking row raises inside the finishing transaction (SQLite can't retry busy errors mid-transaction), the batchable rescue swallows it, and the resolved job leaves a live tracking row behind — the batch can never finish, its callback never fires, and it's never clearable. Widening test waits didn't help because the batch wasn't slow, it was stuck. sweep_stalled now repairs the leak: a tracking row whose job is finished or failed is always a leak (they only resolve together in one transaction), so the sweep destroys such rows with no staleness threshold and the destroy re-triggers the completion check. The lifecycle tests run dispatcher maintenance every second so repairs land within their wait windows on busy CI runners; a deterministic regression test constructs both leak flavors directly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P39PSB74nP3a8B2F7Z5xvz
Two fixes from evaluating every CI failure across the matrix: - Edge Rails defers bulk enqueues past all open transactions (enqueue_after_transaction_commit now covers enqueue_all), so perform_all_later inside a batch block pushed its jobs after the block's transaction committed and the batch context was gone: the jobs silently lost their batch, which looked empty and completed with just its empty job. Membership is now seeded at instantiation again — the original design, whose enqueue-timing hazard instinct keeps being right — and rebound at enqueue time when a context is active. Deterministically reproduced and fixed under the rails_main gemfile. - A failed finishing transaction (SQLite can't retry busy errors mid-transaction) left batches empty-but-unfinished, and the sweep's 5-minute staleness threshold kept it from repairing them in any useful window. Completion checks are idempotent and single-winner, so that threshold is now a 3-second grace covering only the started-but-empty-job-still-deferred gap. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P39PSB74nP3a8B2F7Z5xvz
Enqueueing callback jobs directly through SolidQueue::Job skipped their before/around/after_enqueue hooks and ignored aborts — the one regression an independent review found in the cumulative diff. The callback chain now wraps the direct enqueue, so hooks run and :abort prevents the enqueue, while callbacks keep their solid_queue pinning and atomicity with the finishing transaction. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P39PSB74nP3a8B2F7Z5xvz
tblais1224
approved these changes
Jul 29, 2026
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.
The batch review-fix series (#13 / jpcamara#3), rebased directly onto
main— which already carries the batch feature at exactly the vintage these fixes were built against, so this diff is purely the fixes: no staging branch, no external pins needed once merged.All 12 commits replayed with zero conflicts (main's batch files are byte-identical to the
batch-pocbase). Verified on this base: batch model + lifecycle suites green on SQLite (including theenqueue_after_transaction_commitpath, which runs under main's Rails 7.2+ lockfile), MySQL green, rubocop clean.See #13 for the full commit-by-commit rationale, stress-test numbers, and CI history — that PR went fully green across the matrix on the equivalent tree. Supersedes #13 as the integration path if merging to
maindirectly is preferred.🤖 Generated with Claude Code