fix(handler): wait for context.succeed when handler returns undefined#786
fix(handler): wait for context.succeed when handler returns undefined#786zarirhamza wants to merge 1 commit into
Conversation
|
There was a problem hiding this comment.
Pull request overview
Restores Lambda wrapper behavior for 2-arg handlers that implicitly return undefined by waiting for completion via context.succeed / context.done / context.fail (or callback), instead of resolving immediately with undefined. This aligns promisifiedHandler with the Node.js Lambda runtime contract and fixes regressions for “fire-and-forget then signal via context” handler shapes (e.g., aws-serverless-express proxy(server, event, context) in CONTEXT_SUCCEED mode).
Changes:
- Update
promisifiedHandlerto wait oncallbackPromwhenasyncProm === undefinedand the handler does not declare a callback parameter. - Harden the “artifact” heuristic by excluding
nullbefore attemptinginstanceof EventEmitter/ property checks. - Replace the prior “completes when handler returns undefined” test with new tests that assert waiting for
context.*completion in the no-return case (including anaws-serverless-express-like proxy simulation).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/utils/handler.ts |
Restores waiting semantics for implicit-undefined returns and tightens artifact detection against null. |
src/utils/handler.spec.ts |
Adds/updates unit tests to ensure context.* completion is respected when the handler returns undefined with < 3 parameters. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const looksLikeArtifact = | ||
| asyncProm !== undefined && | ||
| typeof asyncProm === "object" && | ||
| asyncProm !== null && |
There was a problem hiding this comment.
🤖
The added asyncProm !== null guard is both ineffective for its stated purpose and effectively dead code.
The PR body says this hardens against null so the code doesn't run instanceof EventEmitter / property reads on null. But a handler returning null never reaches this line — it throws earlier at line 60:
if (asyncProm !== undefined && typeof (asyncProm as any).then === "function") {For null: null !== undefined is true, so it evaluates typeof (null).then → TypeError: Cannot read properties of null (reading 'then') (verified). So a null return crashes the invocation regardless of this guard.
And at this line, asyncProm can never actually be null: undefined is intercepted by the new branch above, and null already threw at line 60 — so the else only ever sees defined, non-null values. The asyncProm !== null check therefore never evaluates false (dead).
If guarding null is a real goal, fix it at line 60 (e.g. asyncProm != null && typeof (asyncProm as any).then === "function"); otherwise drop the line-85 check and the null claim from the PR description so it doesn't overstate the fix.
There was a problem hiding this comment.
Good catch — you're right that the guard at line 85 is unreachable: null already throws at the line 60 typeof (asyncProm as any).then access before reaching the artifact heuristic, and even if it didn't, the new else if (asyncProm === undefined) branch would still let null fall into the else with typeof null === 'object' triggering the artifact reads.
I've dropped the asyncProm !== null change from this PR and removed the corresponding paragraph from the commit message and PR description — this PR is now strictly the undefined-return regression fix and nothing else. Final diff vs main is just the new else if (asyncProm === undefined) { promise = callbackProm; } branch in handler.ts plus the four regression tests in handler.spec.ts.
I'll send the line-60 null crash as a separate small PR with its own test.
Restore the pre-v11.126.0 semantics for the `(event, context) => { ... }` /
`length < 3` / no-return handler shape: when `asyncProm === undefined`, fall
back to `callbackProm` instead of resolving immediately with `undefined`.
PR #661 (v11.126.0) introduced an "eager-resolve on undefined" branch intended
to fix TypeScript-transpiled async handlers whose `return` was lost during
bundling. PR #665 (v12.127.0) refactored that branch and PR #683 (v12.130.0)
added a `looksLikeArtifact` heuristic for handlers that *return* a Server /
EventEmitter, but the `asyncProm === undefined` path was never re-guarded. As
a result every layer from v11.126.0 onwards silently truncates handlers that
return nothing and finish via `context.succeed` / `context.done` /
`context.fail`, most notably `aws-serverless-express@3`'s
`proxy(server, event, context)` with the default `CONTEXT_SUCCEED` resolution
mode.
Symptom: handler returns `undefined` synchronously, wrapper resolves
immediately with `undefined`, the Lambda runtime ships `undefined` back to the
caller and freezes the worker before any deferred work writes to stdout, so
nothing is flushed to CloudWatch.
Tests:
- Replace the test added by PR #661 that codified the regressive "return
undefined -> resolve immediately" behavior.
- Add four regression tests covering the actual contract: a handler with
`length < 3` and no return must wait for `context.succeed` / `context.done` /
`context.fail`, including a reproducer mirroring the aws-serverless-express
`proxy()` shape.
80e5f09 to
eccce76
Compare
Summary
Restore the pre-
v11.126.0semantics for the(event, context) => { ... }handler shape (no callback parameter, no explicit return). When the handler returnsundefined, the wrapper now waits forcontext.succeed/context.done/context.fail/callbackinstead of resolving the wrapping promise immediately withundefined.This fixes a regression for handlers using
aws-serverless-express@3'sproxy(server, event, context)pattern (defaultCONTEXT_SUCCEEDresolution mode) and any other "fire-and-forget then signal viacontext.*" handler shape.Root cause
#661 (
v11.126.0, "Fix Typescript Timeouts when Lambda handler returns undefined") added an eager-resolve branch:#665 (
v12.127.0) refactored this and #683 (v12.130.0) added alooksLikeArtifactheuristic that keeps waiting when the handler returns aServer/EventEmitter. However, the artifact heuristic gates onasyncProm !== undefined, so when the handler returns nothing it still falls through toPromise.resolve(undefined)— i.e. #683 never covered the no-return case.A handler that returns
undefinedsynchronously is indistinguishable from a handler that's mid-flight and intends to complete viacontext.succeed/context.done/context.fail. Eager-resolving onundefinedtruncates that work: the Lambda runtime shipsundefinedback to the caller and freezes the worker before pending stdout writes flush to CloudWatch.A representative handler shape that hits this:
handler.length === 2, no return, soasyncProm === undefined→ previously hit the eager-resolve branch and resolved before Express finished.Fix
Why this is safe to revert
v11.125.0. Reverting restores a long-stable contract.context.succeed/done/failis called, or the returned promise settles". It is not "the function ends when the synchronous body returns". (fix): Fix Typescript Timeouts when Lambda handler returns undefined #661's branch silently changed that contract.undefinedand does nothing else is now left waiting oncallbackProm, which is correct: it will hit the configured Lambda function timeout. That is the expected behavior — a function that never signals completion is a user bug, and conflating it with "the handler is done" was the wrong fix.Notes for users still hitting timeouts with TS-bundled async handlers
The original problem #661 set out to solve (TS compilation losing the implicit return of an async handler) is better addressed at the user level: ensure your transpilation target supports native
async(Node ≥ 12), orreturnthe work explicitly. The wrapper should not silently truncate execution to compensate.Tests
The four added tests would have caught #661 and #665. The existing
it("completes when handler returns undefined")(added by #661, codifying the regressive behavior) is replaced; the new tests assert the actual Lambda runtime contract.Test plan
jest src/utils/handler.spec.ts— 20/20 passtslint --project tsconfig.json— cleanprettier --check src/utils/handler.{ts,spec.ts}— cleantsc --noEmit— cleanjest) — only 2 pre-existing failures insrc/index.spec.ts(nock-based, unrelated to handler logic; reproduce onmainwithout this PR)Related