bug #61291 [Messenger] Remove fix NoAutoAckStamp handling in Worker::flush() (wazum)

nicolas-grekas · nicolas-grekas · commit 9aa072c1f30d · 2025-08-05T11:23:22.000+02:00
This PR was merged into the 6.4 branch. Discussion ---------- [Messenger] Remove fix `NoAutoAckStamp` handling in `Worker::flush()` | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | - | License | MIT ## Description This PR removes unused code in the `Worker::flush()` method where an envelope modification was never used. ## Changes - Removed the line `$envelope = $envelope->withoutAll(NoAutoAckStamp::class);` which was modifying a local variable that was immediately discarded - Moved the `NoAutoAckStamp` removal to the catch block where it's actually needed ## Why this change? The line was unused code because: 1. If `dispatch()` succeeds: The modified envelope is never used (foreach continues/ends) 2. If `dispatch()` throws: The line is never reached Moving the stamp removal to the `catch` block ensures that envelopes stored for acknowledgment have clean state - they shouldn't contain a "no auto ack" stamp when they're about to be acknowledged. Commits ------- 06b1d8f [Messenger] Fix NoAutoAckStamp handling in Worker::flush()
diff --git a/src/Symfony/Component/Messenger/Tests/Fixtures/DummyReceiver.php b/src/Symfony/Component/Messenger/Tests/Fixtures/DummyReceiver.php
@@ -63,4 +63,9 @@ public function getAcknowledgedEnvelopes(): array
     {
         return $this->acknowledgedEnvelopes;
     }
+
+    public function getRejectedEnvelopes(): array
+    {
+        return $this->rejectedEnvelopes;
+    }
 }
diff --git a/src/Symfony/Component/Messenger/Tests/WorkerTest.php b/src/Symfony/Component/Messenger/Tests/WorkerTest.php
@@ -38,6 +38,8 @@
 use Symfony\Component\Messenger\MessageBusInterface;
 use Symfony\Component\Messenger\Middleware\HandleMessageMiddleware;
 use Symfony\Component\Messenger\Stamp\ConsumedByWorkerStamp;
+use Symfony\Component\Messenger\Stamp\FlushBatchHandlersStamp;
+use Symfony\Component\Messenger\Stamp\NoAutoAckStamp;
 use Symfony\Component\Messenger\Stamp\ReceivedStamp;
 use Symfony\Component\Messenger\Stamp\SentStamp;
 use Symfony\Component\Messenger\Stamp\StampInterface;
@@ -584,6 +586,49 @@ public function testFlushBatchOnStop()
 
         $this->assertSame($expectedMessages, $handler->processedMessages);
     }
+
+    public function testFlushRemovesNoAutoAckStampOnException()
+    {
+        $envelope = new Envelope(new DummyMessage('Test'));
+        $receiver = new DummyReceiver([[$envelope]]);
+
+        $bus = new class implements MessageBusInterface {
+            public function dispatch(object $message, array $stamps = []): Envelope
+            {
+                $envelope = Envelope::wrap($message, $stamps);
+                if ($envelope->last(FlushBatchHandlersStamp::class)) {
+                    throw new \RuntimeException('Flush failed');
+                }
+
+                return $envelope;
+            }
+        };
+
+        $dispatcher = new EventDispatcher();
+        $dispatcher->addListener(WorkerRunningEvent::class, function (WorkerRunningEvent $event) {
+            static $calls = 0;
+            if (++$calls >= 2) {
+                $event->getWorker()->stop();
+            }
+        });
+
+        $worker = new Worker(['transport' => $receiver], $bus, $dispatcher, clock: new MockClock());
+
+        $reflection = new \ReflectionClass($worker);
+        $unacksProperty = $reflection->getProperty('unacks');
+        $unacks = $unacksProperty->getValue($worker);
+        $dummyHandler = new DummyBatchHandler();
+        $envelopeWithNoAutoAck = $envelope->with(new NoAutoAckStamp(new HandlerDescriptor($dummyHandler)));
+        $unacks->attach($dummyHandler, [$envelopeWithNoAutoAck, 'transport']);
+
+        $worker->run();
+
+        $this->assertSame(1, $receiver->getRejectCount());
+        $rejectedEnvelopes = $receiver->getRejectedEnvelopes();
+        $this->assertCount(1, $rejectedEnvelopes);
+        $rejectedEnvelope = $rejectedEnvelopes[0];
+        $this->assertNull($rejectedEnvelope->last(NoAutoAckStamp::class));
+    }
 }
 
 class DummyQueueReceiver extends DummyReceiver implements QueueReceiverInterface
diff --git a/src/Symfony/Component/Messenger/Worker.php b/src/Symfony/Component/Messenger/Worker.php
@@ -257,9 +257,9 @@ private function flush(bool $force): bool
             [$envelope, $transportName] = $unacks[$batchHandler];
             try {
                 $this->bus->dispatch($envelope->with(new FlushBatchHandlersStamp($force)));
-                $envelope = $envelope->withoutAll(NoAutoAckStamp::class);
                 unset($unacks[$batchHandler], $batchHandler);
             } catch (\Throwable $e) {
+                $envelope = $envelope->withoutAll(NoAutoAckStamp::class);
                 $this->acks[] = [$transportName, $envelope, $e];
             }
         }

Original file line number	Diff line number	Diff line change
`@@ -63,4 +63,9 @@ public function getAcknowledgedEnvelopes(): array`
`63`	`63`	`{`
`64`	`64`	`return $this->acknowledgedEnvelopes;`
`65`	`65`	`}`
	`66`	`+`
	`67`	`+ public function getRejectedEnvelopes(): array`
	`68`	`+ {`
	`69`	`+ return $this->rejectedEnvelopes;`
	`70`	`+ }`
`66`	`71`	`}`
Original file line number	Diff line number	Diff line change
`@@ -257,9 +257,9 @@ private function flush(bool $force): bool`
`257`	`257`	`[$envelope, $transportName] = $unacks[$batchHandler];`
`258`	`258`	`try {`
`259`	`259`	`$this->bus->dispatch($envelope->with(new FlushBatchHandlersStamp($force)));`
`260`		`- $envelope = $envelope->withoutAll(NoAutoAckStamp::class);`
`261`	`260`	`unset($unacks[$batchHandler], $batchHandler);`
`262`	`261`	`} catch (\Throwable $e) {`
	`262`	`+ $envelope = $envelope->withoutAll(NoAutoAckStamp::class);`
`263`	`263`	`$this->acks[] = [$transportName, $envelope, $e];`
`264`	`264`	`}`
`265`	`265`	`}`