Vector acks messages sent by S3 source even when delivery failed

[tanushri-sundar]

A note for the community

• Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
• If you are interested in working on this issue or have submitted a pull request, please leave a comment

Problem

I am sending messages from S3 to GCP PubSub using Vector.

I have observed that messages rejected by the PubSub sink for non-retryable errors (ex: 400, 403) are dropped.

2024-01-18T21:35:42.649078Z ERROR vector_common::internal_event::component_events_dropped: Internal log [Events dropped] is being suppressed to avoid flooding.

The dropped messages are deleted from SQS, even with a DLQ configured on my SQS.

I would expect that the SQS message not be deleted per Vector’s end-to-end-acknowledgement guarantee.

---

Expected behavior: Vector should delete messages in S3 only if delivery to the sink is successful. I have traced this issue back to the S3 source code here:

match result {
    BatchStatus::Delivered => Ok(()),
    BatchStatus::Errored => Err(ProcessingError::ErrorAcknowledgement),
    BatchStatus::Rejected => {
        // Sinks are responsible for emitting ComponentEventsDropped.
        // Failed events cannot be retried, so continue to delete the SQS source message.
        Ok(())
    }

The BatchStatus::Rejected case should call Err(ProcessingError::ErrorAcknowledgement).

This would allow failed messages to be DLQed, as the user can configure a DLQ on their SQS and set custom values for maximum retries and visibility timeout.

This fix could potentially be a configuration based approach, with a parameter such as strict_ack = true preventing Vector's overeager deletion.

Configuration

No response

Version

0.33.0

Debug Output

No response

Example Data

No response

Additional Context

No response

References

#14899
#14708
#10870

[jszwedko]

This is one of the issues intended to be addressed by #14708.

[tanushri-sundar]

@jszwedko I agree, this could definitely be solved by the RFC #14708.

A smaller scope fix for this particular source would be very helpful, and it could potentially be a low-touch code change. Since there isn't a timeline for implementing the RFC, perhaps starting with this fix can help unblock some users more quickly.

[jszwedko]

Yeah, after reflecting a bit I agree that this seems like a safe incremental change to make even in the context of that RFC. I'd be happy to see a PR for it if you like.

[tanushri-sundar]

@jszwedko That sounds great! I've opened PR #19748

[jszwedko]

Closed by #19748