[Bug]: Not change visibility timeout error failed messages when process message batch

[doonpy]

Describe the bug

When processing the message batch, the processMessageBatch method does not change visibility timeout process failed messages (when terminateVisibilityTimeout is true)

Your minimal, reproducible example

...

Steps to reproduce

For example, I consuming batch 10 messages with handleMessageBatch is:

for (const message of messages) {
    if (message.Body) {
       await handle(JSON.parse(message.Body);
    }
}

Simulate it:

- Message 1: Success
- Message 2: Success
- Message 3: Failed -> throw an error

Current behavior: Not change the visibility timeout message from 3 to 10

Expected behavior

• Visibility timeout of message 3 to 10 should be changed to 0

How often does this bug happen?

None

Screenshots or Videos

No response

Platform

...

Package version

v6.1.0

AWS SDK version

No response

Additional context

No response

[nicholasgriffintn]

I'm not really sure that I understand what you are saying, but looking at the code, changeVisibilityTimeoutBatch should be called whenever any error occurs within the process message batch functionality if terminateVisibilityTimeout is true.

If you are saying that this is not happening then this would have to be investigated as to why.

I'll add a help-wanted on this for others find the problem if I don't have the time to.

[github-actions[bot]]

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

[nicholasgriffintn]

Still not sure what the issue is here, so I think we should close this issue, please reopen if some further clarification can be provided.

[nick-kang]

I believe the request is that when batch processing, there's no way to change visibility timeout for a subset of the messages. If we have 3 messages in a batch and only 1 fails, we want to set the 1 failed message's visibility timeout to zero. With the current implementation, all 3 messages's visbility timeout will be set to zero.

I think the best approach would be to change this to use Promise.allSettled instead of Promise.all https://github.com/bbc/sqs-consumer/blob/7991a77180ec7481d3bc1a27cb84e5280afef0d2/src/consumer.ts#L253

edit: looking at the code some more I think the best approach would actually be to update this method to figure out which messages are not part of ackedMessages and only set visbility timeout to zero for those (we can put this feature behind a configuration option as it'd be a breaking change otherwise). https://github.com/bbc/sqs-consumer/blob/7991a77180ec7481d3bc1a27cb84e5280afef0d2/src/consumer.ts#L303-L333