Skip to content

Commit 8ca5816

Browse files
fix(control-plane): discard malformed SQS messages (github-aws-runners#5219)
## Description Treat malformed SQS message bodies as permanent, non-retryable failures. The scale-up handler logs and acknowledges malformed records so they do not block valid messages in the same batch or repeatedly return to the queue. Document the SQS event source mapping contract near `batchItemFailures` and update the focused handler tests for the new behavior. ## Test Plan 1. Send a scale-up SQS batch containing one valid message and one malformed JSON body. 2. Confirm the valid message is passed to `scaleUp`. 3. Confirm the malformed message is logged but omitted from `batchItemFailures`. 4. Confirm a message rejected by `scaleUp` is still returned in `batchItemFailures`. 5. Run `yarn vitest run functions/control-plane/src/lambda.test.ts --config functions/control-plane/vitest.config.ts --reporter=dot`. ## Related Issues None.
1 parent 67bfdf8 commit 8ca5816

2 files changed

Lines changed: 12 additions & 17 deletions

File tree

lambdas/functions/control-plane/src/lambda.test.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -129,12 +129,12 @@ describe('Test scale up lambda wrapper.', () => {
129129
await expect(scaleUpHandler({ Records: records }, context)).resolves.not.toThrow();
130130
});
131131

132-
it('Should report only the malformed message as a batch item failure', async () => {
132+
it('Should acknowledge a malformed message without retrying it', async () => {
133133
const records = [...createMultipleRecords(2), malformedRecord('message-bad')];
134134
vi.mocked(scaleUp).mockResolvedValue([]);
135135

136136
await expect(scaleUpHandler({ Records: records }, context)).resolves.toEqual({
137-
batchItemFailures: [{ itemIdentifier: 'message-bad' }],
137+
batchItemFailures: [],
138138
});
139139
});
140140

@@ -150,12 +150,12 @@ describe('Test scale up lambda wrapper.', () => {
150150
]);
151151
});
152152

153-
it('Should combine malformed and rejected messages in batch item failures', async () => {
153+
it('Should report rejected messages but acknowledge malformed messages', async () => {
154154
const records = [...createMultipleRecords(2), malformedRecord('message-bad')];
155155
vi.mocked(scaleUp).mockResolvedValue(['message-1']);
156156

157157
await expect(scaleUpHandler({ Records: records }, context)).resolves.toEqual({
158-
batchItemFailures: [{ itemIdentifier: 'message-bad' }, { itemIdentifier: 'message-1' }],
158+
batchItemFailures: [{ itemIdentifier: 'message-1' }],
159159
});
160160
});
161161

lambdas/functions/control-plane/src/lambda.ts

Lines changed: 8 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@ export async function scaleUpHandler(event: SQSEvent, context: Context): Promise
1616

1717
const sqsMessages: ActionRequestMessageSQS[] = [];
1818
const warnedEventSources = new Set<string>();
19-
const malformedMessageIds: string[] = [];
2019

2120
for (const { body, eventSource, messageId } of event.Records) {
2221
if (eventSource !== 'aws:sqs') {
@@ -32,18 +31,10 @@ export async function scaleUpHandler(event: SQSEvent, context: Context): Promise
3231
try {
3332
payload = JSON.parse(body) as ActionRequestMessage;
3433
} catch (e) {
35-
// Parsing happens outside the try/catch below, so an unparseable body used to
36-
// throw straight out of the handler. That failed the whole invocation and made
37-
// SQS redeliver the entire batch, so one malformed message held up every valid
38-
// message alongside it, indefinitely.
39-
//
40-
// Report it as an individual failure instead: the rest of the batch proceeds,
41-
// and the malformed message exhausts maxReceiveCount on its own. It cannot be
42-
// acknowledged and discarded here — reporting it is the only way to single it
43-
// out — so configure redrive_build_queue if these should be captured rather
44-
// than expire.
34+
// A malformed body is a permanent, non-retryable failure. Keep it out of
35+
// batchItemFailures so the event source mapping acknowledges and deletes it,
36+
// while valid records in the same batch continue to scale up normally.
4537
logger.error(`Ignoring message ${messageId}, body is not valid JSON`, { error: e, messageId });
46-
malformedMessageIds.push(messageId);
4738

4839
continue;
4940
}
@@ -57,7 +48,11 @@ export async function scaleUpHandler(event: SQSEvent, context: Context): Promise
5748
return (l.retryCounter ?? 0) - (r.retryCounter ?? 0);
5849
});
5950

60-
const batchItemFailures: SQSBatchItemFailure[] = malformedMessageIds.map((itemIdentifier) => ({ itemIdentifier }));
51+
// The SQS event source mapping owns message acknowledgement and deletion. Because
52+
// ReportBatchItemFailures is enabled, a successful handler response makes Lambda
53+
// delete every record not listed here; listed records remain in SQS and become
54+
// available for retry after their visibility timeout expires.
55+
const batchItemFailures: SQSBatchItemFailure[] = [];
6156

6257
try {
6358
const rejectedMessageIds = await scaleUp(sqsMessages);

0 commit comments

Comments
 (0)