Split queue workers into pools, parked crashed messages instead of redelivering them, and dropped the Redis transport - #1247
Merged
Merged
Conversation
A single worker consumed every queue, so a long-running handler blocked everything behind it: a newsletter batch or a ten-minute feed build would hold up an order confirmation email for its whole runtime. Cron now keeps one detached worker per configured pool. Queues are routed to a tier under global/queue/routing, one node per queue so a module can route its own and local.xml can retarget a single one. Pools themselves carry only resourcing (count, memory, time limit, idle timeout, redelivery window) under global/queue/pools. Core ships two tiers: "fast" stays resident for transactional email, while "slow" is the catch-all and only runs while it has work, exiting after an idle grace period. An unrouted queue falls to the catch-all on purpose, so forgetting to declare a tier costs a message waiting behind a feed rather than a checkout email stuck behind one. Redelivery is per pool, which means stale claims are only requeued for the queues a worker owns; otherwise the fast worker's shorter window would requeue a feed the slow worker is still running. The watchdog probe counts only work that is actually due, plus claims abandoned by a dead worker, so a campaign scheduled for next week does not respawn a worker every minute and a crashed handler is not stranded. Queue filtering is unavailable on the Redis transport, which is not a QueueReceiverInterface, so pools collapse to a single worker there.
…ker count - PoolRegistry::build() falls back to the default pool when every declared pool is dropped, instead of leaving the queue with no workers - queue:work without --pool uses the widest redelivery window in play, so it cannot requeue another pool's in-flight claim early - On-demand pools spawn at most one worker per due message - Corrected the queue:list orphan hint and the undefined newsletter queue constant in the pool tests
A worker stamps claimed_by (machine:lock-name) as it claims a message, and only while it holds its machine-local worker lock. The kernel frees that flock however the process ends, so a claim from this machine is settled by reading the lock rather than the clock: free means the worker died and the row is requeued at once, held means the handler is still running and the row is never touched, however long it takes. redeliver_after stays as the fallback for a claim from another server, whose lock is not visible here. That removes both failure modes of the timer on a single-server install. Recovery no longer waits out the window, and a handler slower than the window is no longer requeued underneath itself and run a second time. Dropped the fast pool's 900s override. It only existed to reach a crashed worker sooner, which the lock now does immediately, and it silently ignored system/queue/redeliver_after for transactional email. Removed the Redis transport. It had no test coverage, was an optional suggest rather than a dependency, and each of its seven branch points disabled a queue feature: worker pools, the admin grid listing, queue:list, the on-demand spawn probe, redeliver_after and completed_retention. It still needed the database table for failures, so it ran alongside the database rather than in place of it. Also fixed, from review of the earlier commits: the watchdog counted only newly spawned workers against an on-demand pool's due budget, it ignored the poolless lock a hand-run exclusive worker takes, getMessageCount() skipped the queue filter, an out-of-range --index took a lock nobody probes, and the Redis pool notice logged on every install because core ships two pools.
fballiano
changed the title
A claim is now parked, not redelivered. Nothing infers a worker's death from elapsed time, so no handler is ever started a second time by a clock. Inferring death from a timer is wrong in both directions: it waits out the window before recovering, and it re-queues a handler that is merely slower than the window, running it alongside the first. The lock-based version of this traded the second fault for a third, since a hung worker holds its lock forever and would strand the message with no way back. The message is not lost. The row keeps its body, queue and claimed_at, and the admin grid lists it, so recovery is an operator decision: Retry now accepts a claimed row as well as a failed one, and Discard already took any row. That is the whole of the recovery path. Removed with it: claimed_by and WorkerIdentity, requeueStaleClaims(), redeliver_after and its admin field, the per-pool override, PoolRegistry::widestRedeliveryWindow() and QueueWork::adHocRedeliveryWindow(). countDue() is back to pending-and-due, so the watchdog no longer respawns an on-demand worker for a claim no worker will pick up. Note the queue is at-most-once for a crash only. The retry-on-exception path is untouched, so a handler that throws is still re-run up to max_retries.
fballiano
changed the title
Nothing re-queues an abandoned claim now, so the grid is the only way one comes back, and a message parked there is invisible until somebody opens the page. Every admin page carries a notice while such a row exists, linking to the grid to retry or discard it. The one-hour threshold drives display only, so a handler that honestly overruns costs a notice rather than a second run. The notice is skipped for users without the queue ACL, and added through addUniqueMessages() so browsing does not stack it up.
Retrying a message that a worker was still handling ran the handler a second time. Retry now accepts a claimed row only when the claim is older than the abandoned cut-off, applied in the update where-clause so a fresh claim wins the race, and the grid hides the button in the same case. A parked claim also kept its dedupe key in flight forever, turning every later dispatch of that key into a silent no-op. The in-flight check now ignores claims past the same cut-off.
A pool that fails to take its lock is now logged at ERROR unless it is an
on-demand pool with nothing left due, so a worker that fatals on boot no
longer hides behind a notice while its queues stop being consumed. The
spawn confirmation also waits once for the whole batch instead of five
seconds per pool in turn.
`queue:work --queue` now drops the pool's inherited exclusion list, so
`--pool=slow --queue=email` consumes email instead of idling on
`queue IN ('email') AND queue NOT IN ('email')`.
DbTransport implements Symfony's KeepaliveReceiverInterface and QueueWork wires SIGALRM the way ConsumeMessagesCommand does, so claimed_at means "a worker is alive here" rather than "a worker started here a while ago". The admin notice, the retry gate and the dedupe check now only fire on a worker that died, instead of on any handler that outran the window. The abandoned window drops from an hour to 300 seconds, next to the 5 second refresh it depends on. email:queue:process delegates to queue:work through the Application instead of building a second Worker, so the signal wiring is not duplicated and WorkerFactory::create() has a single caller. On-demand pools subtract their live claims from the roster, so a worker inside a long handler no longer counts as available and block a spawn for a due message. The admin notice skips AJAX requests and no longer probes the schema with isTableExists() on every admin request. Dropped the post-spawn worker start check: startup failures already land in var/log/queue-worker.log, and its wait blocked every cron tick that spawned.
- Disarmed the keepalive alarm after the worker run so a pending SIGALRM cannot kill the surrounding CLI process - Guarded ack() on processing status so a late ack cannot swallow a message an operator already retried - Made --exclusive refuse --queue and --exclude-queue, since a filtered worker cannot claim whole-roster coverage of the lock - Merged --exclude-queue with the pool's configured exclusions instead of replacing them - Parsed pool config booleans via Mage_Core_Model_Config_Element::is() so <active>true</active> works - Added a (status, queue, available_at) index for queue-filtered polls - Reported skipped non-retryable rows in the admin mass retry - Fetched only the status column in the retry pre-check - Warned at startup when pcntl is unavailable and keepalive is off
- Guarded the retry re-send UPDATE with status=processing so a stale worker's late re-send cannot overwrite a row an operator already retried, completed, or deleted - Skipped the keepalive refresh while the shared connection is inside a handler's transaction, so the refresh never locks the row against the admin retry or vanishes on rollback - Let memory_limit 0 mean unbounded instead of falling back to 256M, matching time_limit and the CLI path - Extracted the abandoned-claim predicate into DbTransport::isAbandonedClaim() and used it in the admin view - Documented that spawn counts are cluster-global while worker locks are machine-local
… pool validation - Added a claim_token stamp so ack/reject/keepalive/re-send only touch a row the worker still owns; an operator retry hands the row to a new token - Refused retry of a parked message while a newer in-flight copy of its dedupe key exists - Validated pool memory_limit at build time with a logged 256M fallback instead of a silent worker respawn loop - Cached the abandoned-message admin notice count for 60s; retry/discard drop the cache - Warned when a bare queue:work --exclusive starts alongside live pool workers - Consolidated transport count queries, hoisted shared test helpers into Pest.php, removed the transport-name vestige
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
1 participant
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.
A single worker consumed every queue, so a newsletter batch or a ten-minute feed build held up order confirmation emails for its whole runtime. Cron now keeps one detached worker per pool.
Queues are routed to a tier under
global/queue/routing, one node per queue so a module can route its own and local.xml can retarget a single one without clobbering the rest.global/queue/poolscarries only resourcing: count, memory, time limit, idle timeout. An invalidmemory_limitis logged and falls back to 256M instead of killing the worker at boot in a silent respawn loop.Two tiers ship:
faststays resident for transactional email,slowis the catch-all and runs only while it has work, exiting after an idle grace period. An unrouted queue falls to the catch-all deliberately, so forgetting to declare a tier costs a message waiting behind a feed rather than a checkout email stuck behind one.queue:workgains--pool,--exclude-queue,--idle-timeoutand a per-pool exclusive lock; a bare--exclusiverun warns when pool workers are still alive, since they hold per-pool locks and would keep claiming messages.queue:listshows each queue's pool and flags orphaned ones.A claim a dead worker left is parked, not redelivered
Nothing re-queues a claim automatically, and a slow handler is never mistaken for a dead worker by a fixed window: a live worker refreshes its claim every 5 seconds on Symfony's keepalive alarm, so a handler may run as long as it needs. A claim with no refresh for 5 minutes is reported abandoned, which means the worker actually stopped refreshing, not that a handler outran an estimate. (The refresh needs pcntl and cannot land while the handler holds an open transaction on the shared connection, so such a worker can be misreported; recovery stays manual either way, so the cost is a premature notice, never an automatic second run.)
The message is not lost when a worker is killed mid-handler. The row keeps its body, its queue, and its
claimed_at, and the admin grid lists it. Recovery is an operator decision: Retry accepts a failed row or an abandoned claim and refuses a fresh one, and Discard accepts any row. That is the entire recovery path.Every claim writes an ownership token, and
ack,reject, keepalive, and the retry re-send all prove ownership with it. After an operator retries an abandoned claim, the original worker, should it turn out to be alive after all, can no longer complete, fail, or re-queue the row the new worker owns; the worst case is one duplicate handler run, never a corrupted row. For deduped messages the same boundary holds in both directions: an abandoned claim stops suppressing a fresh dispatch of its key, and Retry refuses a parked row while a newer in-flight copy of its key exists, so an at-most-once job cannot end up with two live rows.This is a narrower exposure than it sounds, because every graceful exit is already handled.
SIGTERMandSIGINTare trapped, so a deploy finishes the in-flight message and exits, andStopWorkerOnMemoryLimitListenerfires between messages, after an ack. OnlySIGKILL, a hard OOM kill, or power loss parks a claim.A parked message would otherwise be invisible until somebody opened the grid, so every admin page carries a notice while one exists, linking straight to it. The count behind it is cached for 60 seconds, well under the abandonment threshold, and the retry and discard actions drop the cache so operator feedback is instant.
Removed with it:
redeliver_afterand its admin field, the per-pool override,PoolRegistry::widestRedeliveryWindow(),QueueWork::adHocRedeliveryWindow(), andDbTransport::requeueStaleClaims().countDue()is back to pending-and-due, so the watchdog no longer respawns an on-demand worker for a claim no worker will take.The queue is at-most-once for a crash only. The retry-on-exception path is untouched: a handler that throws is still re-run up to
max_retries, so handlers must still tolerate re-execution.Removed the Redis transport
Unreleased, so nothing to migrate. It had no test coverage and no CI job,
symfony/redis-messengerwas an optionalsuggestrather than a dependency, and each of its branch points turned a queue feature off:RedisTransportis not aQueueReceiverInterface, soWorker::run()rejects a queue filter and worker pools, this PR's feature, collapsed to a single catch-allqueue:listcould not list pending messagescompleted_retentionwas inertIt still needed
maho_queue_messagefor failures, so it ran alongside the database rather than in place of it. The rot was visible here: the pools added in this PR silently did nothing under it.Dropping it also let
workerTransport()returnDbTransportinstead ofTransportInterface, removing theinstanceoffallback in the cron probe. The Redis cache backend and session storage are untouched; this is only the queue transport.Covered by tests
queue:work --exclusive.Note
Developed with the help of AI.
As part of our commitment to GenAI transparency, we flag pull requests produced with AI assistance alongside human work. As with every change in Maho, a maintainer reviews and validates it before merge, we never merge purely AI-generated changes. See the GenAI transparency section for details.