Skip to content

fix(queue): do not receive a new message after stop - #262

Merged
loks0n merged 2 commits into
mainfrom
fix/queue-swoole-stop-before-receive
Sep 11, 2026
Merged

fix(queue): do not receive a new message after stop#262
loks0n merged 2 commits into
mainfrom
fix/queue-swoole-stop-before-receive

Conversation

@loks0n

@loks0n loks0n commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Problem

Both Swoole consume loops (consumeBound() and run()) check isStopped() only at the top of the loop, before blocking on the concurrency slot. At maxCoroutines=1 the push blocks for the whole handler, so a SIGTERM that lands mid-job is first noticed after the slot comes free, and by then the loop has already gone back to receive() and claimed the next message.

Measured on cloud's compose stack with _APP_WORKERS_NUM=3 and six 5s jobs queued: SIGTERM during the first three, and each process still took one more, so the drain ran for 10s instead of 5s. Every handler process accepts one job after being told to stop. With cloud's 30s terminationGracePeriodSeconds and no reap() on most Redis queues, a long build or delete accepted after SIGTERM is SIGKILLed and its claim is stranded on processing.

Fix

Re-check the flag once the slot is held; pop it and leave without receiving. A message that receive() already returned before the flag flipped is still processed and committed as before.

Test

SwooleRestartTest::testShutdownDrainsJobWithoutRestartingWorkers now publishes a second slow job behind the in-flight one and asserts it is still on the queue after the drain. Seen red on both SIGTERM and SIGINT before the fix (actual size 2 matches expected size 1), green after. bin/monorepo test queue passes; Pint passes.

Supersedes utopia-php/queue#88, closed.

🤖 Generated with Claude Code

The Swoole consume loops checked the stop flag only at the top of the
loop, before blocking on the concurrency slot. At maxCoroutines=1 the push
blocks for the whole handler, so a SIGTERM that landed mid-job was first
noticed only after the slot came free, and by then the loop had already
gone back to receive() and claimed the next message. Every handler
process took one more job after being told to stop, so a rolling restart
drained for two job durations per process, and a long job accepted after
SIGTERM was SIGKILLed at the pod's grace period with its claim stranded.

Re-check the flag once the slot is held and leave without receiving.

The restart test now publishes a second slow job behind the in-flight one
and asserts it is still on the queue after the drain; seen red on both
SIGTERM and SIGINT before this change.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@greptile-apps

greptile-apps Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 5/5

The PR appears safe to merge; no blocking or non-blocking correctness issue remains.

Summary

  • Re-checks the stop flag immediately after acquiring a concurrency slot in both consumption paths.
  • Releases the acquired slot before leaving during shutdown.
  • Verifies that active work drains while queued work remains pending for both signals and one-queue or multi-queue configurations.

Reviews (2) · Last reviewed commit: "test(queue): cover the multi-queue drain..."

Comment thread packages/queue/src/Queue/Adapter/Swoole.php
Comment thread packages/queue/tests/Queue/E2E/Adapter/SwooleRestartTest.php
Comment thread packages/queue/tests/Queue/E2E/Adapter/SwooleRestartTest.php
…roker

The restart fixture takes a queue count, so the drain test runs once
against the single-queue loop and once against the multi-queue one; the
multi-queue cases go red when only the run() check is removed. Remaining
depth is read through Broker\Redis::getQueueSize() rather than the list
key it happens to use.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Sep 11, 2026

Copy link
Copy Markdown

Benchmark results

queue — workload shapes across both concurrency axes (4 cores, 600 messages, median of 3)

workload shape redis msg/s nats msg/s
io 1p x 1c 37 38
io 1p x 4c 149 153
io 4p x 1c 148 152
cpu 1p x 1c 160 172
cpu 1p x 4c 175 178
cpu 4p x 1c 384 403
mixed 1p x 1c 62 64
mixed 1p x 4c 164 174
mixed 4p x 1c 242 252

Np x Mc = N consumer processes x M handler coroutines. io yields and should follow
coroutines; cpu does not and should follow processes; mixed is in between.

Shared CI runners — treat absolute numbers as rough, compare modes within a run. Commit c123375.

@loks0n
loks0n merged commit da25acf into main Sep 11, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant