Skip to content

fix(aws): stabilization batch for AWS Lambda + generic adapters#239

Open
pi0x wants to merge 1 commit into
mainfrom
fix/aws-batch
Open

fix(aws): stabilization batch for AWS Lambda + generic adapters#239
pi0x wants to merge 1 commit into
mainfrom
fix/aws-batch

Conversation

@pi0x

@pi0x pi0x commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Implements the AWS scope (T2/T3) of the v1 stabilization plan. Does not touch F7/trust-proxy (owned by #229).

Findings fixed

  • F19 — Duplicate Set-Cookie delivered to clients. set-cookie is no longer emitted via headers when it is already carried in cookies (v2) / multiValueHeaders (v1), on both the buffered and streaming paths, so API Gateway no longer merges both and sends the last cookie twice. Dead Array.isArray branch removed. The masking MockHeaders test was replaced with real Headers.
  • F20 — v1 (REST API) repeated headers were lost because event.multiValueHeaders was ignored. awsEventHeaders now merges it, avoiding double-counting keys present in both maps.
  • F21text/html + content-encoding: gzip bodies were mangled through toString("utf8"). awsResponseBody now forces base64 whenever a non-identity content-encoding is present.
  • F22awsResultToResponse passed an empty string body into null-body statuses, throwing for 204/304 handlers in the local-testing flow. It now passes null for 101/204/205/304.
  • F23 — Added toLambdaStreamHandler, a streaming counterpart to toLambdaHandler that goes through the full server contract (plugins/middleware/error/trustProxy). The AWS Lambda docs streaming section now teaches it.
  • F30 — The generic adapter now sets request.runtime (name generic) and request.ip, matching every other adapter.
  • F8 (AWS round-trip half)requestToAwsEvent now strips the cookie header from the synthesized v2 event (matching real API Gateway), so handlers no longer see cookie: "a=1, a=1" after awsEventHeaders re-appends event.cookies.
  • F24request.waitUntil now works on Lambda: registered promises are awaited before the invocation returns (buffered and streaming paths).
  • F15 — Degenerate events (headers: null, GET/HEAD-with-body) now surface a clear error / null-body instead of opaque TypeErrors.

Tests

  • Replaced the MockHeaders mock (which stored cookies in a side array that never entered header iteration, masking F19) with real web Headers.
  • Added regression tests for: v1 multi-value headers, set-cookie dedup on buffered and streaming paths, gzip body base64, 204 round-trip, streaming middleware/error application, waitUntil completion, cookie round-trip without doubling, and the F15 guards.

pnpm test (lint + typecheck + vitest w/ coverage): 989 passed, 35 pre-existing skips.

🤖 Generated with Claude Code

Address the AWS-scope T2/T3 findings from the v1 stabilization plan:

- F19: stop emitting `set-cookie` via `headers` when it is already carried in
  `cookies` (v2) / `multiValueHeaders` (v1) on both the buffered and streaming
  paths, so API Gateway no longer sends the last cookie twice; drop the dead
  `Array.isArray` branch.
- F20: merge v1 `multiValueHeaders` in `awsEventHeaders` (repeated REST API
  headers were lost), without double-counting keys present in both maps.
- F21: force base64 in `awsResponseBody` whenever a non-identity
  `content-encoding` is present, so gzip'd text bodies are not mangled.
- F22: pass a `null` body for 101/204/205/304 in `awsResultToResponse` so the
  documented `invokeLambdaHandler(toLambdaHandler(...))` flow no longer throws.
- F23: add `toLambdaStreamHandler`, a streaming counterpart to
  `toLambdaHandler` that goes through the server contract so middleware,
  plugins, error and trustProxy apply; update the AWS Lambda docs.
- F30: set `runtime` (name "generic") and `ip` on the generic adapter request.
- F8: strip the `cookie` header from the synthesized v2 event so it is not
  doubled once `awsEventHeaders` re-appends `event.cookies` on the round trip.
- F24: implement `request.waitUntil` for Lambda by awaiting collected promises
  before the invocation returns (buffered and streaming paths).
- F15: guard degenerate events (`headers: null`, GET/HEAD-with-body) with a
  clear error / null-body instead of opaque TypeErrors.

Replace the `MockHeaders` mock in the AWS test suite (which masked F19) with
real web `Headers`, and add regression tests for every finding above.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@pi0x pi0x requested a review from pi0 as a code owner July 14, 2026 12:47
@coderabbitai

coderabbitai Bot commented Jul 14, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@pi0x, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 50 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 2e697b73-0c93-4fa0-8f86-4f342ff0bcc8

📥 Commits

Reviewing files that changed from the base of the PR and between d8523dc and 42220a9.

📒 Files selected for processing (5)
  • docs/1.guide/9.aws-lambda.md
  • src/adapters/_aws/utils.ts
  • src/adapters/aws-lambda.ts
  • src/adapters/generic.ts
  • test/aws.test.ts
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/aws-batch

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@pkg-pr-new

pkg-pr-new Bot commented Jul 14, 2026

Copy link
Copy Markdown

Open in StackBlitz

npm i https://pkg.pr.new/srvx@239

commit: 42220a9

@pi0x pi0x left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Review: PR #239 — AWS stabilization batch

Verdict: Approve (non-blocking). Every finding in the AWS scope is implemented correctly and against real AWS v1/v2 semantics. I re-derived the bugs, confirmed the new tests are load-bearing (they fail against the old logic), and ran the full suite: pnpm vitest run test/aws.test.ts → 72 passed; pnpm test989 passed, 35 skipped, no type errors (after a pnpm build — the self-srvx import typecheck errors on a fresh worktree are a build-ordering artifact, not this diff). Two low-severity notes and one merge-conflict flag below; none warrant blocking.

Per-finding checklist

  • F19 — set-cookie no longer duplicated: PASS. awsResponseHeaders skips set-cookie during iteration (utils.ts:202) and delivers it only via cookies (v2) / multiValueHeaders["set-cookie"] (v1). Applies to awsStreamResponse too (spreads the same helper, utils.ts:250). Dead Array.isArray(value) branch removed. MockHeaders replaced with real Headers. Verified real Headers iteration yields each set-cookie individually, so the fix is required and the tests genuinely exercise it (old logic leaks set-cookie: "theme=dark" and fails the new toBeUndefined() assertion).
  • F20 — v1 multiValueHeaders merged without double-counting: PASS. awsEventHeaders (utils.ts:149-167) appends the multi-value form and records covered keys (lowercased) so the single headers map doesn't re-add them. get("x-test") === "a, b" and host not doubled.
  • F21 — force base64 on non-identity content-encoding: PASS. utils.ts:234-236 forces base64 when content-encoding is non-empty and not identity; identity still uses utf8 for text. Byte-exact gzip test passes.
  • F22 — null body for 101/204/205/304: PASS for 204/205/304. See low note on 101.
  • F23 — streaming counterpart through the server contract: PASS. toLambdaStreamHandler routes through AWSLambdaServer (wrapFetch + errorPlugin + trustProxy), so middleware/plugins/error/trustProxy all apply. Tests prove middleware short-circuit (299) and error option (503) reach the streaming prelude. Docs updated to teach it, with a TIP noting the lower-level handleLambdaEventWithStream bypasses them.
  • F30 — generic runtime/ip: PASS. generic.ts:35-45 sets runtime.name = "generic" and an ip getter that is configurable: true.
  • F8 — cookie stripped in round trip: PASS. requestToAwsEvent pushes cookie into cookies and continues, keeping it out of both headers and the new multiValueHeaders (utils.ts:345-353); awsEventHeaders re-appends it once. Round-trip handler sees cookie: "a=1" (no doubling).
  • F24 — waitUntil on Lambda: PASS on both paths. handleLambdaEvent/handleLambdaEventWithStream attach waitUntil (configurable) and await wait.wait() in finally, after the response/stream is produced, so no deadlock and response bytes aren't delayed — only the invocation return is. createWaitUntil swallows rejections (.catch(console.error)), so the finally-await can't crash the invocation.
  • F15 — degenerate events: PASS. Null/non-object headers throws a clear TypeError (utils.ts:40-42); GET/HEAD-with-body is null-body per the locked decision (utils.ts:51-57), raw bytes still reachable via runtime.awsLambda.event.

Findings by severity

  • Low — _aws/utils.ts:509-512 (F22, 101 branch): new Response(null, {status: 101}) still throws RangeError (Response status must be 200–599), verified empirically. The null-body guard therefore does nothing for 101 — only 204/205/304 are actually rescued. Not blocking: a statusCode: 101 Lambda proxy result is nonsensical and unreachable in practice, but the guard/comment overpromise "101". Consider dropping 101 from the list or short-circuiting it earlier.
  • Low / informational — generic.ts:42-44 (F30): ip is derived from the leftmost x-forwarded-for unconditionally (client-spoofable), and the generic adapter never wires trustProxyPlugin, so the trustProxy option is inert there. Acceptable — a transport-less adapter has no peer address, XFF is the only signal, and configurable: true satisfies the spec so a user can override — but worth a doc line that generic ip is untrusted by default.
  • Low / edge — _aws/utils.ts:151-160 (F20): an empty-array multiValueHeaders[key] marks the key covered before the value loop, so a same-key single-map value would be dropped. Not real API Gateway behavior (it never emits empty arrays); no action needed.

Merge collision with #229 (flagged per task)

Both PRs rewrite the body of awsRequest and the _trust-proxy import line in src/adapters/_aws/utils.ts. #229 replaces isTrustedProxy/firstForwardedValue/trusted with hop-aware resolveClientIP/trustedHops/forwardedHopValue and changes awsEventURL's signature; #239 inserts the headers guard, method/hasBody, and the body: change into the same lines. A textual conflict is guaranteed. #239 correctly does NOT touch trust-proxy logic (respects the #229 boundary). Whoever merges second must keep #229's hop-aware resolution AND #239's header-guard/hasBody/body edits — a naive "take #239" resolution would silently reintroduce the F7 leftmost-XFF regression. Recommend the second author resolve by hand and re-run test/aws.test.ts + the trust-proxy suites.

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