Report batches that were never sealed instead of completing them - #13
Report batches that were never sealed instead of completing them#13jpcamara wants to merge 4 commits into
Conversation
Batch maintenance treated any batch older than the stalled window that had never been started as a process that died mid-creation, and started it on its behalf. Started with nothing outstanding, the batch immediately finished and fired its callbacks. That guess is wrong as often as it's right. An unsealed batch is one of two things, and nothing in the queue database tells them apart: a creator that died before sealing it, or a creator still filling it from a transaction that hasn't committed. Active Job defers those enqueues until commit, and with a separate queue database the batch row doesn't wait for them, so a transaction that outlives the window leaves a batch that looks abandoned and isn't. Completing it finishes the batch over whatever happened to have landed, fires on_success for work that never ran, and leaves the real enqueues to raise AlreadyFinished. Stop guessing. Complete only batches whose creator sealed them by calling start, since sealed is what makes an empty batch meaningfully complete rather than merely unfilled. Report the rest through a stalled_batch event and a Batch.stalled scope, and let an operator adopt one with Batch#start once they've established its creator is gone. This trades automatic recovery of genuinely crashed batches for never reporting success over work that didn't happen. Their jobs still run either way; only the batch's own completion waits for a decision. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Reporting unsealed batches instead of completing them left genuinely crashed creators to manual adoption. Narrow that: when Batch#enqueue runs with no transaction open anywhere, nothing outside it can roll the batch back or defer its jobs, so seal in the same transaction as the batch row. Either nothing commits, or a sealed batch and its jobs commit together—a creator dying mid-creation can no longer leave an unsealed batch behind, and crash recovery needs no operator. Only batches that received jobs in their own transaction seal this way. A batch with none may still be waiting on enqueues deferred to a commit that hasn't happened, and a sealed empty batch means complete, so those keep deferring to start. An unsealed batch therefore came from inside a transaction, or is waiting on deferred enqueues: exactly the cases where completing it loses work, which is why the sweep reports rather than adopts them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Revised per discussion: batches created outside any transaction now seal atomically with their own row, so a server dying mid-creation can't strand one — automatic crash recovery is back for that case without the sweeper ever guessing. The manual path now only covers batches created inside a transaction (where completing them was the data-loss bug) and all-deferred configurations. PR body rewritten to match. |
A transaction that outlives the stalled window, a rollback, and a creator dying right after enqueue: one test each, end to end with a dispatcher and worker, asserting what actually runs and what never fires. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Reporting unsealed batches left an expectation problem: nobody expects "sometimes batches get stuck forever, and if you don't monitor an event you'll accumulate rows that never complete" as default behaviour. A batch still unsealed a day after creation came from a transaction that will never commit. Its data rolled back, so the batch ends the same way: the sweep removes it, as if it was never created. No callbacks fire over rolled-back work, and nothing accumulates or needs monitoring. Jobs that reached the queue before the rollback already ran and stay untouched, matching jobs enqueued outside a batch in a rolled-back transaction. Between the stalled report at five minutes and expiry at a day, batches remain visible through Batch.stalled for anyone who wants to complete or discard them early. A transaction that outlives the expiry and then commits finds its batch gone and raises AlreadyFinished from its deferred enqueues: loud, and without success callbacks over lost work. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Addressed the expectations problem: unsealed batches no longer sit around forever waiting for someone to notice an event. They now expire — still unsealed a day after creation means the transaction is never committing, so the sweep removes the batch as if it was never created. Rolled-back batch data and its batch row now end the same way. Nothing to monitor by default; |
Draft — against my own fork for discussion, not upstream.
Maintenance could complete a batch whose transaction was still open or rolled back — firing
on_successfor work that never happened and losing the deferred enqueues toAlreadyFinished. This PR makes that impossible, without giving up crash recovery.The three scenarios, as integration tests in
batch_lifecycle_test.rb(each runs a real dispatcher and worker):1. Transaction outlives the stalled window → now works
2. Rollback → no phantom success
3. Server dies right after enqueue → nothing to repair
How
Two changes:
Maintenance completes only batches their creator sealed (called
starton). Unsealed ones follow a lifecycle instead of a guess:stalled_batch.solid_queueevent each, visible viaSolidQueue::Batch.stalled, resolvable early with.startor.destroy.expire_after): removed. A batch still unsealed then came from a transaction that will never commit; its data rolled back, so the batch ends the same way — as if never created. No callbacks, no accumulation, nothing to monitor.Jobs that reached the queue before a rollback already ran and stay untouched — the same behaviour as non-batch jobs enqueued in a rolled-back transaction. A transaction that outlives the expiry and then commits raises
AlreadyFinishedfrom its deferred enqueues: loud, and without success callbacks over lost work.A batch created outside any transaction seals itself atomically:
Crash → either nothing committed or a sealed batch and its jobs committed together, which is why scenario 3 needs no sweeper.
The
total_jobs != 0guard: deferred enqueues put no jobs in the batch's own transaction, and a sealed batch with nothing outstanding means complete — sealing there would let a concurrent sweep finish it before the deferred INSERTs land. So all-deferred configurations (Rails 7.2:default, 8.2 defaults) keep sealing via the deferredstart; a crash in that window goes to the reported path, since the jobs existed only in process memory and there is nothing durable to recover.Rails 7.1 (no
all_open_transactions) is unchanged.Verification
Suites green on Rails 7.1 / 7.2 / 8.0 / 8.1 / main, PostgreSQL, shared-pool and separate-pool layouts, rubocop clean. Companion: #7 deletes the leftover pending rows from scenario 2.
🤖 Generated with Claude Code