Skip to content

fix(amqp): skip ack when using exchange as ack is only meaningful for queues#1069

Open
sergio-u-simspace wants to merge 3 commits into
conductor-oss:mainfrom
Simspace:fix/amqp-skip-ack-on-exchange
Open

fix(amqp): skip ack when using exchange as ack is only meaningful for queues#1069
sergio-u-simspace wants to merge 3 commits into
conductor-oss:mainfrom
Simspace:fix/amqp-skip-ack-on-exchange

Conversation

@sergio-u-simspace
Copy link
Copy Markdown
Contributor

Problem

When a workflow is terminated, Conductor correctly calls Event.cancel() for any in-progress EVENT task. The Event.cancel() implementation attempts to clean up by calling queue.ack() with a synthetic Message using task.getTaskId() (a UUID) as the receipt.

For AMQP exchanges this is semantically wrong: the EVENT task publishes to an exchange — once the message is routed it is gone, and there is no consumer relationship, no delivery tag, and nothing to acknowledge. The ack attempt should never be made.

On top of the semantic issue, AMQPObservableQueue.ackMsg() calls Long.parseLong(message.getReceipt()) to obtain the AMQP delivery tag. Since the receipt is a UUID, this throws a NumberFormatException. The internal retry loop in ackMsg() exhausts before the cancellation proceeds, causing significant delay.

Fix

Skip ack entirely when useExchange is true. This is correct by definition — exchange publishers have no delivery tag to acknowledge — and eliminates the retry loop for the exchange case.

Note: the NumberFormatException also affects the queue path (wrong receipt type in Event.cancel()), but that is a separate concern tracked in #779.

Tests

  • Renamed testAcktestAckOnExchangeIsSkipped and added verify(channel, never()).basicAck(...) to make the intent explicit.
  • Added testAckOnQueueCallsBasicAck to confirm ACK is still invoked correctly on the queue path.
  • Also included a fix for pre-existing test state leakage in AMQPObservableQueueTest (resetAMQPConnectionState in @Before) that caused two unrelated tests to fail when run in certain orders.

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