Confusing documentation on acknowledgement

[matiasgarcia]

Summary

Hi team,
First of all, thank you for maintaining this library! We’ve been using sqs-consumer at scale and recently ran into an unexpected issue during graceful shutdown implementation that I believe highlights a confusing point in the documentation and behavior of message acknowledgement.

The Problem

We noticed some SQS messages were getting deleted even though our handler didn’t explicitly acknowledge them. This happened during the shutdown phase of a worker, where we were trying to not process or delete any more messages so they could be retried later.

Here’s a simplified version of what we were doing in our handler:

function handleMessage(message: Message) {
  if (this.abortController.signal.aborted) {
    this.warn(`consumer closed, skipping message to reprocess later`, { message });
    return;
  }
  await callNestJsHandler(message)
  return message;
}

We assumed that returning undefined would signal “don’t acknowledge, reprocess later”. However, the message was actually deleted. Looking into the library’s internals, I found this logic:

sqs-consumer/src/consumer.ts

Lines 372 to 378 in ca76326

	const ackedMessage: Message = await this.executeHandler(message);
	if (ackedMessage?.MessageId === message.MessageId) {
	await this.deleteMessage(message);
	this.emit("message_processed", message);
	}

const ackedMessage: Message = await this.executeHandler(message);

if (ackedMessage?.MessageId === message.MessageId) {
  await this.deleteMessage(message);
  this.emit("message_processed", message);
}

And the simplified implementation of executeHandler is:

sqs-consumer/src/consumer.ts

Lines 554 to 560 in ca76326

	} else {
	result = await this.handleMessage(message);
	}
	return !this.alwaysAcknowledge && result instanceof Object
	? result
	: message;

So if alwaysAcknowledge is false (the default), and the return value from handleMessage is not an object, it defaults to returning the original message, which results in deletion.

In our case, returning undefined led the consumer to assume the message had been successfully processed and delete it.

The fix was simple but not intuitive:

function handleMessage(message: Message) {
  if (this.abortController.signal.aborted) {
    this.warn(`consumer closed, skipping message to reprocess later`, { message });
    return {}; <-- avoid deletion of message
  }
  await callNestJsHandler(message)
  return message;
}

Returning an empty object avoids the match on MessageId, so the message is not deleted.

I believe the current documentation is a bit misleading. Here’s the relevant excerpt:

/**
 * An `async` function (or function that returns a `Promise`) to be called whenever
 * a message is received.
 *
 * In the case that you need to acknowledge the message, return an object containing
 * the MessageId that you'd like to acknowledge.
 */
handleMessage?(message: Message): Promise<Message | void>;

It reads as though a message is only acknowledged if you explicitly return a Message, but in reality, returning undefined also leads to deletion due to the internal fallback logic.

However, the alwaysAcknowledge documentation explains what we have to do quite well:

/**
 * By default, the consumer will treat an empty object or array from either of the
 * handlers as an acknowledgement of no messages and will not delete those messages as
 * a result. Set this to `true` to always acknowledge all messages no matter the returned
 * value.
 * @defaultvalue `false`
 */
alwaysAcknowledge?: boolean;

This subtle behavior can lead to data loss if misunderstood, especially during critical flows like graceful shutdown or error handling.

I think we need to clarify in the documentation that returning undefined will implicitly acknowledge and delete the message (unless alwaysAcknowledge = true).

Example

No response

[nicholasgriffintn]

Thank you for creating this issue, you are totally right and it's a great reminder for us to come back to this.

Originally, we had implemented this graceful shutdown and acknowledgement in a manner that was backwards compatible.

We should potentially look at making a breaking change release that takes undefined into account as you've noted here.

[nicholasgriffintn]

Hey, sorry for the delay, I've been busy with some other work and training for the last few weeks, we also needed to prioritise an AWS SDK upgrade before this as this would be a major change.

I've now raised a PR, I'm going to spend a bit of time thinking about this, it is quite a big, breaking change that will not be immediately obvious so we will need to be careful about the release. #583

It is available as 14.0.0.canary-1 now, I don't know when we will release this to main just yet but if anyone really wants it, it is available here: https://github.com/bbc/sqs-consumer/releases/tag/v14.0.0-canary.1