Skip to content

fix(aws-lambda): destroy the body when response stream setup throws - #126

Merged
dinwwwh merged 7 commits into
mainfrom
claude/peaceful-heisenberg-88d4hw
Sep 29, 2026
Merged

dinwwwh merged 7 commits into
mainfrom
claude/peaceful-heisenberg-88d4hw

Conversation

@dinwwwh

@dinwwwh dinwwwh commented Sep 29, 2026 •

Copy link
Copy Markdown
Member

The aws-lambda sendStandardResponse now releases the response body when setting up the response stream throws synchronously. Before, awslambda.HttpResponseStream.from() or the prelude-triggering res.write('') could throw. The promise rejected, but the body toNodeHttpBody had already built was never destroyed. An event-stream generator never ran its finally and its keep-alive interval kept firing; a ReadableStream body was never cancelled. On a warm Lambda container these leaks carried over into later invocations.

The setup now runs inside a try, like the node adapter. The catch destroys the body with destroyNodeHttpBody, destroys the response stream instead of leaving it half-open, and rejects with the original error.

Behaviour

  • Setup errors now clean up the body. Examples: the runtime's Cannot set content-type, too late. when the stream was already written to (canWriteToNodeResponse can't detect that on the Lambda stream), a missing awslambda global, or a throwing first write.
  • The error/close listeners that settle the promise are now attached to responseStream before the try. The Lambda runtime's from() returns the same stream it is given, so this is the same object as before. It also means a throwing from() can't leave the error event from destroy unhandled.
  • The success path is unchanged: the stream branch keeps its own close listener that destroys the body.
  • Removed the WARNING: errors that occur here are silently ignored comment that was copied from the node adapter. It isn't true here: res.destroy(error) emits error on the response stream, which rejects the promise, as the existing body-error test checks.

Testing

  • New it.each covering three cases: from throws, the first write throws, and awslambda is missing. Each asserts the rejection, responseStream.errored, and that the body was destroyed. All three fail against main.
  • vitest run packages/aws-lambda (100 passed), eslint packages/aws-lambda, and tsc -b are clean.

🤖 Generated with Claude Code

https://claude.ai/code/session_01AVoginFKKdE3QrCKUARJ98

… throws

`awslambda.HttpResponseStream.from()` and the prelude-triggering `write('')`
can throw synchronously (e.g. the runtime's "Cannot set content-type, too
late." on an already-written stream, or a missing `awslambda` global). The
promise rejected but the body was never destroyed, so event iterators never
ran their `finally`, the keep-alive interval kept firing, and ReadableStream
bodies were never cancelled.

Mirror the node adapter: on a synchronous throw, destroy the body, destroy
the response stream instead of leaving it half-open, and reject.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AVoginFKKdE3QrCKUARJ98
Attach the `error`/`close` listeners to the response stream before the
try, like the node adapter, so the catch no longer needs its own `error`
listener to cover a throwing `from`. Collapse the three throw-path tests
into one `it.each`.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AVoginFKKdE3QrCKUARJ98
Fold the stream branch's separate `close` listener into the one attached
before the try, as the fastify adapter now does. The body is released in
one place for every path, one fewer listener per streaming response, and
settle/cleanup no longer listen on differently named aliases of the same
stream. On the catch path the body is already destroyed, so the listener's
destroy is a no-op.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AVoginFKKdE3QrCKUARJ98
Replace the inline body teardown in the catch with the shared helper, as
the node and fastify adapters now do. Drop the "errors are silently
ignored" warning copied from the node adapter: here `res.destroy(error)`
emits `error` on the response stream, which rejects the promise.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AVoginFKKdE3QrCKUARJ98
@pkg-pr-new

pkg-pr-new Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
@standard-server/aws-lambda

npm i https://pkg.pr.new/@standard-server/aws-lambda@126

@standard-server/core

npm i https://pkg.pr.new/@standard-server/core@126

@standard-server/fastify

npm i https://pkg.pr.new/@standard-server/fastify@126

@standard-server/fetch

npm i https://pkg.pr.new/@standard-server/fetch@126

@standard-server/node

npm i https://pkg.pr.new/@standard-server/node@126

@standard-server/peer

npm i https://pkg.pr.new/@standard-server/peer@126

@standard-server/shared

npm i https://pkg.pr.new/@standard-server/shared@126

commit: fa4baf5

@codspeed

codspeed Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Merging this PR will not alter performance

✅ 26 untouched benchmarks
⏩ 108 skipped benchmarks1


Comparing claude/peaceful-heisenberg-88d4hw (fa4baf5) with main (ddd0a09)

Open in CodSpeed

Footnotes

  1. 108 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

@codecov

codecov Bot commented Sep 29, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

  • Response setup wrapped in try/catch: sendStandardResponse now catches synchronous failures from toLambdaHeaders, HttpResponseStream.from(), the prelude res.write(''), and end(), tears the body down via destroyNodeHttpBody, destroys the response stream, and rejects with the thrown error.
  • Listeners attached up front: the error/close handlers moved onto responseStream ahead of setup, so a throwing from() still has an error listener, and the single close handler now performs the guarded body teardown for every path instead of only the stream branch.
  • New failure tests: three it.each cases (from throws, first write throws, awslambda global missing) assert rejection, responseStream.errored === error, and a destroyed body.

I also confirmed the new cases are discriminating: reverting response.ts to b5b4035 while keeping the new tests makes all three fail with responseStream.errored still null, so they genuinely pin the cleanup. Once the tree is restored, the package tests, type:check, and lint are all clean.

Pullfrog  | View workflow run | Using DeepSeek Flash (default — pick a model for stronger reviews) | 𝕏

… listener

Restore the original stream-branch `close` listener for destroying the
body and keep the pre-try `close` listener to `resolve` only, matching
main and the node adapter.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AVoginFKKdE3QrCKUARJ98
@dinwwwh dinwwwh changed the title Improve error handling in Lambda HTTP response streaming fix(aws-lambda): destroy the body when response stream setup throws Sep 29, 2026

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

The only change since the prior pullfrog review (ed8585d) is one behavior-preserving refactor (fa4baf5):

  • Body teardown back in the stream-branch close listener: responseStream.once('close', …) now only resolves, while the guarded resBody.destroy(getNodeResponseError(res) ?? undefined) moved into the object-body else branch. This restores the success path to its pre-PR shape and mirrors the node adapter's structure.
  • Setup-error cleanup unchanged: the try/catch still calls destroyNodeHttpBody(resBody, error, reject), destroys the response stream, and rejects — so the fix the PR exists for is intact.

Since the runtime's from() returns the same stream it is handed, res === responseStream, making the relocation behavior-equivalent for every path. pnpm vitest run packages/aws-lambda/src/response.test.ts passes 13/13.

Pullfrog  | View workflow run | Using DeepSeek Flash (default — pick a model for stronger reviews) | 𝕏

@dinwwwh
dinwwwh merged commit 28db1dd into main Sep 29, 2026
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants