I for one think you're right and I'm using the same fix. I was writing my tests today and making sure the handler could fail and leave the item in the queue wasn't happening until I applied this.

Cool, yeah I think it's because the Promise.all gets executed right away before the first promise resolves. So I think in practice it's probably actually deleting messages before knowing if the processing of such message was successful or not.

I've been using this in a production lambda, it's working well. Let me know what you think about my latest changes. Basically I pass all the items to the item callback, and only delete the items that resolved successfully.

Do you think it'd be helpful to have some type of error callback? That way you handle caught promise rejections further down the chain if desired.

Also, what's the use case for handling the whole list of messages vs one message at a time? I.e. item(cb) vs list(cb)?

@geoffdutton thanks for this! I just finished doing nearly the exact same thing myself and was coming to make a PR when I saw this! One thing that I might suggest, as you are now tracking failed requests in the metrics, I added another count for success in order to present a clear picture of the results. eg { processed: x, resolved: x, rejected: x, iterations: x }. Just something I found useful

If this works can we get it on master, so people that are using NPM don't need to hunt this down :)

Agreed with @achadee.

Just checking in on the status of the code merge for this PR. Very useful so looking forward to seeing this merged. Thanks!