fix(bb-async-job): batch 10 SQS messages per invocation with partial batch failures - #353
Open
soberm wants to merge 12 commits into
Open
fix(bb-async-job): batch 10 SQS messages per invocation with partial batch failures#353soberm wants to merge 12 commits into
soberm wants to merge 12 commits into
Conversation
…batch failures
The SQS event source shipped with BatchSize 1, so every queued message paid
for its own Lambda invocation — a 10x cost amplifier on high-volume queues.
Raising BatchSize alone would be unsafe: without partial batch responses SQS
treats a batch as fully handled once the invocation returns, so one failing
record silently deletes its nine healthy siblings. The three settings are
therefore changed together:
- batchSize default 1 -> 10
- reportBatchItemFailures: true (new option, default on)
- maxBatchingWindow: 5s (new maxBatchingWindowSeconds option)
The runtime handler already loops over event.Records and returns
{ batchItemFailures }, so no runtime change is needed. An explicitly passed
batchSize still wins over the new default.
Tests: new CDK synth suite pinning BatchSize, FunctionResponseTypes and
MaximumBatchingWindowInSeconds (plus the override paths), and core
lambda-handler tests proving a mixed-success 10-record batch reports exactly
the failed messages.
Review follow-up. The unsafe combination (partial batch reporting off while batchSize > 1) was only warned about in a doc comment, so it stayed reachable — and with the new default of 10 it was reachable by passing a single option. It now throws at synth time; batchSize: 1 remains a legitimate way to opt out. Also pins the exact CFN rendering of maxBatchingWindowSeconds: 0 (CDK emits an explicit 0, it does not omit the property) instead of accepting either form, and documents that batching leaves per-message retry/DLQ accounting unchanged.
🦋 Changeset detectedLatest commit: 082d652 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
…igurable reportBatchItemFailures must be true whenever batchSize > 1 (otherwise a single failing record silently discards the rest of its batch), and at batchSize 1 the two values are behaviorally equivalent for this block — so the option unlocked no legitimate use case. It is now an internal constant, which also removes the need for the synth-time guard that rejected the invalid combination. batchSize and maxBatchingWindowSeconds stay configurable.
… synth
AWS rejects an SQS event source whose batchSize exceeds 10 without a batching
window (10000 with one) or whose window is outside 0-300, but it only does so
once CloudFormation creates the mapping — minutes into a deployment, with an
error that does not name the AsyncJob that caused it.
The constructor now range-checks both options and throws
InvalidOptionException naming the construct, the option and the offending
value, following the package's existing blocksError/AsyncJobErrors pattern.
Also updates the DESIGN.md event source mapping entry, which still described
`SqsEventSource(queue, { batchSize })`, to document the current mapping —
always-on partial batch reporting (required for any batchSize > 1, since
otherwise a single failing record makes SQS delete the whole batch) and the
maxBatchingWindowSeconds default of 5.
…item 3 Review follow-ups. The window is validated before batchSize because the batchSize ceiling (10 vs 10000) is derived from it, which is easy to break by reordering the two checks — say so. Splits the DESIGN.md event source mapping entry into shorter sentences; no change in meaning.
The AsyncJob E2E polls waited at most 2s (20 x 100ms) for a handler result, but the maxBatchingWindowSeconds default of 5 means real SQS can hold a message for up to 5s before invoking the Lambda. Five AsyncJob tests failed on the real-AWS jobs while mock/local passed (mocks ignore the window). Replace the ad-hoc loops with a shared pollForResult helper and named constants documenting the batching-window dependency: 10s budget for plain polls, delaySeconds + 5s window + 3s margin for the delayed tests. Assertions and test intent are unchanged.
Record that pollForResult treats any falsy value as "not ready", and that the delaySeconds tests only assert non-execution at t~=0 rather than continuously throughout the delay window.
The new AsyncJob defaults (batchSize 10 / maxBatchingWindowSeconds 5) let SQS hold a message for up to 5s before invoking the handler. bb-agent submits an internal job per interactive turn (plus a second on HITL resume) and the caller is blocked on that job starting, so the window lands directly on a human-blocking path with no way to opt out. Construct the internal AsyncJob with batchSize 1 and maxBatchingWindowSeconds 0 at both sites — the runtime path in agent.ts and the CDK mirror in index.cdk.ts — so synth and runtime provision an identical event source mapping. batchSize 1 also keeps one failing turn from sharing a batch with others, which matters because the handler is not idempotent (it re-runs the agent and republishes chunks). Adds index.cdk.test.ts pinning the opt-out in the synthesized template (BatchSize 1, MaximumBatchingWindowInSeconds 0, partial batch reporting still on). It runs under --conditions=cdk, otherwise the internal Building Blocks resolve to mocks and synthesize nothing. A sweep of the other AsyncJob consumers found none that need the opt-out.
The AsyncJob E2E poll budget was an attempt count (100 x fetch+100ms), so the real budget scaled inversely with environment latency: fast Sandbox reads (~66ms) collapsed it to ~16s while real handler latency is 15-22s, failing four tests. Production only passed accidentally, its slower ~120ms fetches inflating the same 100 attempts to ~22s. Poll on an explicit wall-clock deadline with a deployed-aware budget, matching async-job-status.test.ts. Assertions are unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
bb-async-jobconfigured its SQS Lambda event source withbatchSize: 1, and offered no way to configure the batching window.Issue #, if available:
Changes
packages/bb-async-job:SqsEventSourcedefaultbatchSizechanged from1to10.Added
maxBatchingWindowSecondsoption (default5), mapped to the event sourcemaxBatchingWindow.Partial batch failure reporting (
ReportBatchItemFailures) is always enabled — it is an internal constant, not an option. It must stay on for anybatchSize > 1, since without it SQS deletes every message in a batch as soon as the invocation returns, so one failing record would silently discard the rest.batchSizeandmaxBatchingWindowSecondsremain overridable by callers; an explicitly passed value takes precedence over the default.The
AsyncJobconstructor now range-checks both options and throwsAsyncJobErrors.InvalidOption(InvalidOptionException) naming the construct, the option and the offending value.batchSizemust be an integer 1–10 with no batching window, or 1–10000 oncemaxBatchingWindowSeconds > 0;maxBatchingWindowSecondsmust be an integer 0–300. These are the limits AWS enforces when the event source mapping is created, so the failure now happens at synth instead of mid-deployment as an opaque CloudFormation error.Updated
README.md,DESIGN.md(the Infrastructure (CDK) event source mapping entry now documents the real mapping — always-on partial batch reporting, themaxBatchingWindowSecondsdefault of5, and the ranges the constructor enforces) and regeneratedAPI.md(npm run update:api).packages/bb-agent: opts out of the new defaults on its interactive path — the internalAsyncJobis now constructed withbatchSize: 1andmaxBatchingWindowSeconds: 0at both the runtime (src/agent.ts) and CDK (src/index.cdk.ts) sites.stream()submits one job per agent turn (plus a second on HITL resume) and the caller is blocked on that job starting, so a 5s batching window would land straight on a human-facing path;batchSize: 1also keeps one failing turn from sharing a batch with others, which matters because that handler is not idempotent. Both sites carry identical options so the runtime and CDK-synth paths provision the same event source mapping. A sweep of the remaining consumers found none that need the opt-out.No runtime handler changes:
handleEventSourceRecordsinpackages/core/src/lambda-handler.tsalready iteratesevent.Recordsand returns{ batchItemFailures }.Validation
packages/bb-async-job/src/index.cdk.test.ts— 12 CDK synth tests. Event source mapping: defaultBatchSize: 10,FunctionResponseTypes: ['ReportBatchItemFailures'],MaximumBatchingWindowInSeconds: 5, explicitbatchSizeoverride (partial batch reporting still on), batching window override, and window0. Option guards:batchSize0rejected,batchSize11rejected with no batching window,batchSize500accepted with a window (synthesizesBatchSize: 500),batchSize10001rejected even with a window,maxBatchingWindowSeconds301rejected, and a validbatchSize: 10/ window300combination still synthesizes.packages/core/src/lambda-handler.test.ts— 3 runtime partial-batch tests over 10-record SQS batches: mixed results report only the failedmessageIds, all-success returns{}, all-fail reports all 10.test-apps/comprehensive/test/async-job.test.ts— the E2E result polls now run on a wall-clock deadline instead of an attempt count. The original loops spent a fixed 100 iterations of fetch+100ms sleep, so the real budget scaled inversely with environment latency: fast Sandbox reads (~66ms) collapsed it to ~16s against 15-22s of real handler latency, failing 4 tests, while Production passed only accidentally because its slower ~120ms fetches inflated the same 100 attempts to ~22s.pollForResultnow loops whileDate.now() < deadlineon a deployed-aware budget —isDeployed ? 60s : 15s, mirroring theBLOCKS_TEST_ENVdetection inasync-job-status.test.ts— and thedelaySecondstests getdelaySeconds * 1000 + budgetso the defer window is fully covered. Assertions and test intent are unchanged, including theimmediate === nullchecks; no production defaults were touched.packages/bb-agent/src/index.cdk.test.ts— 3 new CDK synth tests pin the opt-out in the synthesized template:BatchSize: 1,MaximumBatchingWindowInSeconds: 0, and partial batch reporting still on. Confirmed non-vacuous: with the opt-out removed they fail with the inheritedBatchSize: 10/ window5. Run under--conditions=cdk, otherwise the internal BBs resolve to mocks and synthesize nothing.bb-async-job54/54 tests pass;bb-agent81/81 tests pass (78 existing + 3 new);core667/667 tests pass; comprehensive local E2E 386 pass / 0 fail (1 skipped), including all 9 AsyncJob tests.Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.