fix(batch): throw when a synchronous processor gets a promise-returning handler

[vahidshaik1901]

Summary

BatchProcessorSync and SqsFifoPartialProcessor only support synchronous record
handlers, but nothing enforced that at runtime. processRecordSync() passed the
handler's return value straight to successHandler(), so when the handler was
async that value was a pending promise and every record was recorded as a
success — batchItemFailures came back empty and the event source deleted
messages that were never actually processed. Rejections surfaced afterwards as
unhandled rejections, once the response had already been returned.

The handler is typed as CallableFunction, so TypeScript does not reject an
async function here either.

Changes

• Added a thenable guard in BatchProcessorSync.processRecordSync(). If the
  handler returns a promise-like value, it throws the new
  AsyncHandlerNotSupportedError, which points at BatchProcessor /
  SqsFifoPartialProcessorAsync with processPartialResponse().
• Before throwing, the guard attaches a no-op rejection handler to the abandoned
  promise. Without it a rejecting handler also produces an unhandled rejection,
  which the Lambda Node runtime reports as Runtime.UnhandledPromiseRejection
  in place of the useful error.
• The throw is deliberately placed outside the try/catch so it fails the
  whole invocation rather than being captured as a per-record failure. Misusing a
  synchronous processor is a programming error, not a record-level one, and this
  mirrors how BatchProcessor.processRecordSync() already rejects misuse.
• Genuine handler errors and toBatchType() errors still go through
  failureHandler() exactly as before.
• SqsFifoPartialProcessor delegates to processRecordSync(), so the
  non-deprecated FIFO class is covered by the same guard.
• Added AsyncHandlerNotSupportedError extends BatchProcessingError in
  errors.ts, exported from the package entry point.
• Added packages/batch/tests/unit/BatchProcessorSync.test.ts. BatchProcessorSync
  previously had no unit tests, which is why this went unnoticed; the new tests
  cover the sync processor, the FIFO subclass, and the rejecting-handler case.
• Fixed the JSDoc examples that paired an async record handler with a
  synchronous processor, in processPartialResponseSync.ts (three) and
  SqsFifoPartialProcessor.ts (one), since they would now throw at runtime.
• Documented the behaviour in docs/features/batch.md, in the SQS migration note
  and in the FIFO section.

Verified locally: all 145 unit tests in packages/batch pass with 100% coverage,
npm run lint and npm run build:tests are clean. E2E tests were not run as they
require live AWS credentials.

Issue number: closes #5627

---

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[svozza]

Thanks for picking this up, and for adding the first unit tests for BatchProcessorSync. The guard is in the right place and the throw sitting outside the try/catch is exactly what we want.

A few things need to change before we can merge. Some of these should have been spelled out more clearly in the issue, so apologies for that.

1. Silence the abandoned promise before throwing

The issue mentioned the unhandled rejections but did not say the fix needs to deal with them. Right now the guard throws and leaves the handler's promise dangling. With a rejecting handler you still get an unhandled rejection after the BatchProcessingError, and in the Lambda Node runtime that shows up as Runtime.UnhandledPromiseRejection and hides the useful message. Attaching a no-op rejection handler before throwing fixes it:

if (isThenable(result)) {
  result.then(undefined, () => undefined);
  throw new AsyncHandlerNotSupportedError();
}

2. Add a test with a rejecting handler

All three tests use a handler that resolves. The rejecting case is the one users actually hit, and Vitest fails the file on an unhandled rejection, so that test also guards point 1. The second test can fold into the first as an extra assertion on processor.successMessages rather than swallowing the throw in an empty catch. Please also add the // Prepare / // Act / // Assess phase comments to match the rest of the suite.

3. Introduce a dedicated error class

Please add AsyncHandlerNotSupportedError extends BatchProcessingError in errors.ts, export it from index.ts, and throw that instead of a bare BatchProcessingError. It gives users something to match on and gives the docs something to name. Keep the message plain text without backticks, since it ends up in CloudWatch.

4. Update the JSDoc examples that this change breaks

Several examples currently pair an async record handler with a synchronous processor, and after this PR they throw at runtime:

• packages/batch/src/processPartialResponseSync.ts (three examples)
• packages/batch/src/SqsFifoPartialProcessor.ts (one example, which also constructs a BatchProcessor instead of SqsFifoPartialProcessor)

Please switch those handlers to synchronous functions.

5. Docs

Yes please, fold the docs note into this PR. Two places in docs/features/batch.md:

• Extend the existing migration note in the SQS section to say the synchronous processors throw AsyncHandlerNotSupportedError if the handler returns a Promise.
• Add a note in the FIFO section stating that SqsFifoPartialProcessor requires a synchronous handler and pointing to SqsFifoPartialProcessorAsync with processPartialResponse.

Once those are in I am happy to approve. Thanks again.

[vahidshaik1901]

All five are in, pushed as 8c97878.

1. Silencing the abandoned promise. Confirmed the behaviour first: with the
rejecting-handler test added and no .then(undefined, ...), vitest reports
Unhandled Rejection: Error: failed and exits non-zero even though the three
assertions pass. The guard now takes ownership of the rejection before throwing.

2. Rejecting-handler test. Added leaves no unhandled rejection behind when the promise rejects. The successMessages assertion folded into the first test
and the empty catch is gone.

3. AsyncHandlerNotSupportedError. Added in errors.ts extending
BatchProcessingError, exported from index.ts, thrown by the guard. Message is
plain text now:

The record handler returned a promise, but this batch processor is synchronous and cannot await it. Use BatchProcessor together with processPartialResponse(), or SqsFifoPartialProcessorAsync for FIFO queues.

4. JSDoc examples. All four handlers are synchronous now. One thing beyond
what you listed: the first example in processPartialResponseSync.ts also
constructs BatchProcessor rather than BatchProcessorSync, and
BatchProcessor.processRecordSync() throws Not implemented. Use asyncProcess() instead., so that example was already broken before this PR. I switched it to
BatchProcessorSync — happy to revert that hunk if you would rather keep it out
of this change.

5. Docs. Extended the existing migration note in the SQS section, and added a
note in the FIFO section pointing at SqsFifoPartialProcessorAsync with
processPartialResponse.

Verification: 145 unit tests pass with 100% coverage on src/**, npm run lint
and npm run build:tests clean, markdownlint-cli2 clean on the changed doc.

[svozza]

The guard itself makes sense, and I agree the rejection has to be claimed before the throw or the Lambda runtime reports Runtime.UnhandledPromiseRejection instead of this error. Two things about how it's claimed though.

The no-op handler discards the customer's actual failure. By this point the async handler has already been invoked for the first record, so if it rejects, that rejection is the only evidence of what went wrong inside their code. With this line they see "wrong processor" and nothing else. I'd rather log the reason so the guard doesn't hide a second bug:

Suggested change

	// The promise is abandoned here, so take ownership of its rejection first:
	// otherwise a rejecting handler also surfaces as an unhandled rejection, which
	// the Lambda runtime reports instead of the error thrown below.
	result.then(undefined, () => undefined);
	// The promise cannot be awaited here. Claim its rejection so a failing handler
	// doesn't also surface as Runtime.UnhandledPromiseRejection and mask the error
	// thrown below; log the reason so the handler's own failure is not lost.
	result.then(undefined, (reason) => {
	console.error(
	'Record handler returned a promise to a synchronous batch processor and later rejected',
	reason
	);
	});

Separately, the comment says the code "takes ownership" of the rejection, which reads as if it's handled. It's suppressed, and the comment is clearer if it says that. The suggestion above rewords it.

One more nit, not on this line: since the handler has already run once when this throws, it'd help if AsyncHandlerNotSupportedError said so. Something like "The record handler was invoked for the first record and returned a promise, but this batch processor is synchronous and cannot await it..." tells the reader that side effects may already be in flight.