Something claude flagged related to batch failures which I think will probably end up being out of scope for this PR. In the context of batches a single message failure rejects the entire batch and SQS would retry all of them.
Claude suggested that we could do better by adding support for batch failure reporting:
export const handler: SQSHandler = Sentry.wrapHandler(
async (event: SQSEvent) => {
const s3Client = new S3Client({ region: process.env["AWS_REGION"] ?? "" });
const results = await Promise.allSettled(
event.Records.map(async (message) => {
// ... existing per-message logic, but throw on failure ...
// (return message.messageId from .catch handlers, or just let throw propagate)
}),
);
const batchItemFailures = results
.map((r, i) =>
r.status === "rejected"
? { itemIdentifier: event.Records[i]!.messageId }
: null,
)
.filter((x): x is { itemIdentifier: string } => x !== null);
return { batchItemFailures };
},
);
Which apparently would also require some terraform changes to handle batch item failure reports:
resource "aws_lambda_event_source_mapping" "file_copier_dev_event_source_mapping" {
event_source_arn = aws_sqs_queue.file_copier_dev_queue.arn
function_name = aws_lambda_function.file_copier_dev_lambda.arn
batch_size = 10
maximum_batching_window_in_seconds = 0
function_response_types = ["ReportBatchItemFailures"]
}
If this was a pattern we wanted to actually pursue I think it would apply to other lambdas as well.
Anyway, thought I'd share in case it was worth a followup issue! Not suggesting a change in this PR.
Originally posted by @slifty in #757 (comment)
Something claude flagged related to batch failures which I think will probably end up being out of scope for this PR. In the context of batches a single message failure rejects the entire batch and SQS would retry all of them.
Claude suggested that we could do better by adding support for batch failure reporting:
Which apparently would also require some terraform changes to handle batch item failure reports:
If this was a pattern we wanted to actually pursue I think it would apply to other lambdas as well.
Anyway, thought I'd share in case it was worth a followup issue! Not suggesting a change in this PR.
Originally posted by @slifty in #757 (comment)