fix(lambda): return batch failures on unhandled errors instead of dropping messages

[vegardx]

Problem

The scaleUpHandler catch block returns empty batchItemFailures for non-ScaleError exceptions. This tells SQS that all messages were processed successfully, causing them to be permanently deleted. The corresponding workflow_job events are silently lost — no EC2 instances are launched, no retry occurs, and affected jobs remain queued in GitHub until they time out (24 hours).

Any transient error (GitHub API 404, rate limit, network timeout) that isn't a ScaleError triggers this path.

Fix

Return all message IDs as batchItemFailures when an unhandled exception occurs. SQS will retry delivery according to the queue's visibility timeout. The redrive policy / maxReceiveCount moves persistently-failing messages to the DLQ, preventing infinite retries.

} else {
  logger.error(
    `Error processing batch (size: ${sqsMessages.length}): ${(e as Error).message}, returning batch for retry`,
    { error: e },
  );
  batchItemFailures.push(...sqsMessages.map(({ messageId }) => ({ itemIdentifier: messageId })));
}

Changes

• lambdas/functions/control-plane/src/lambda.ts — return all messages as batch failures on non-ScaleError
• lambdas/functions/control-plane/src/lambda.test.ts — update tests to assert new behavior + add log message test

Impact

• Non-ScaleError exceptions → messages retried via SQS (previously: silently dropped)
• ScaleError path → unchanged
• DLQ catches persistently-failing messages (no infinite retry loop)

Fixes #5024
Also fixes #5105

[Copilot]

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Updates the SQS scale-up Lambda to retry the entire batch on unexpected (non-ScaleError) failures, aligning behavior with partial batch response semantics and adding/adjusting tests to verify retries and logging.

Changes:

• Return all SQS message IDs as batchItemFailures when scaleUp throws a non-ScaleError, causing SQS/Lambda to retry the batch.
• Update existing tests to assert the new retry behavior for single and multi-record batches.
• Add a test asserting the updated error log message for non-ScaleError failures.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
lambdas/functions/control-plane/src/lambda.ts	Changes non-ScaleError handling to return all messages for retry and updates the log message accordingly.
lambdas/functions/control-plane/src/lambda.test.ts	Updates expectations to match new retry behavior and adds a log assertion test.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[vegardx]

Good catch. Fixed in d949963 — switched to expect.arrayContaining with a length check.

[edersonbrilhante]

Won't this feature make scale error unnecessary?

[vegardx]

Good question, and no — ScaleError still carries something this fallback can't.

toBatchItemFailures() returns a subset: messages.slice(0, failedInstanceCount). Combined with the ascending retryCounter sort in lambda.ts, it retries the N least-retried messages and ACKs the rest. The fallback here returns everything.

That difference matters because scaleUp calls createRunners per runner group. A ScaleError from group 3 aborts after groups 1 and 2 already launched instances — returning every message ID would make SQS redeliver those and create a second runner for jobs that already have one. So I'd keep ScaleError as the precise path and treat this only as the net for unclassified errors.

Two things I noticed while digging, both arguably worse than what this PR fixes, happy to split into separate issues:

1. JSON.parse(record.body) in lambda.ts throws before sqsMessages is populated, so batchItemFailures comes back empty and the whole batch is silently dropped — the exact failure mode Scale-up Lambda silently drops entire SQS batch on non-ScaleError exceptions #5024 describes, on a path this PR doesn't cover.
2. redrive_build_queue defaults to enabled = false, so for most deployments a genuinely poison message now retries until retention expires instead of dropping once. Worth calling out in the docs if we merge this as-is.

Should I also exclude rejectedMessageIds from the returned set? Those are deliberately rejected and permanently unprocessable, so retrying them is pure waste.

Apologies for the slow reply.

[vegardx]

On Copilot's note about the order-sensitive assertion — addressed in test: use order-insensitive assertion for batchItemFailures, which is on the branch. The comparison no longer depends on the relative order of equal-retryCounter messages.

[vegardx]

Correction to point 1 in my earlier comment — I described the JSON.parse path wrongly, and in a way that undersold this PR rather than overselling it.

I said an unparseable body leaves batchItemFailures empty and silently drops the batch. That's not what happens. JSON.parse is outside the try, so it throws straight out of the handler, and Lambda treats a thrown exception as a complete batch failure — nothing is deleted and the whole batch is redelivered. Poison pill, not silent loss. I've verified that empirically against main rather than by reading.

What I'd conflated: the silent-drop behaviour I was describing is real, but it lives in the catch block — a non-ScaleError pushes nothing to batchItemFailures and returns { batchItemFailures: [] }, which AWS treats as complete success and deletes every message in the batch. That is exactly what this PR fixes. So the two paths fail in opposite directions, and my example belonged to this PR's scope rather than outside it.

The parse path is still worth fixing on its own — one malformed message blocks every valid message batched with it, and ReportBatchItemFailures disables Lambda's usual polling scale-down on failed invocations, so there's no backoff cushion. That's #5215, kept separate since it's a different failure mode. It touches the same file, so whichever of the two lands second will need a small rebase.

Point 2 stands unchanged: redrive_build_queue defaults to enabled = false, so on this PR's path a genuinely unprocessable message retries until retention expires instead of being dropped once. Still worth a docs note.

Apologies for the noise — should have checked that before writing it down.