Skip to content

provideEvent's contract is unenforced: calling fn twice double-commits a mutation under a 200 #3172

Description

@frenzzy

Summary

provideEvent's contract — "run this function with event visible to getRequestEvent()" — is not enforced in any way. Every violation answers HTTP 200, and one of them executes the server function's side effects twice.

Tested against next @ a536e29b, built from source, Node 24.19.

Reproduction

{ provideEvent: (event, fn) => { fn(); return fn(); } }   // calls fn twice
status = 200   side effects ran = 2 times

A non-idempotent mutation commits twice, and the caller is told it succeeded once. Nothing is logged.

The full table, from a production build, with a server function that reads the event and writes a cookie the way real integration code does:

provideEvent implementation HTTP what the function observed Set-Cookie on the wire
default (global ALS fallback) 200 event present, invocation id present both cookies
healthy ALS.run(event, fn) 200 event present, invocation id present both cookies
calls fn outside its scope 200 getRequestEvent() undefined the function's cookie silently dropped
runs fn under a different event 200 event present but wrong; invocation id null the function's cookie silently dropped
never calls fn 200 function never ran
calls fn twice 200 ran twice ["int=1","fn=1","fn=1"] — the double commit is visible on the wire

Why this is worth a guard

provideEvent exists so an integration can supply its own AsyncLocalStorage — that is the documented use, and it is the hook an enterprise deployment is most likely to implement by hand, because it is where request context is established. Every way of getting it wrong produces a successful-looking response:

  • Never calling fn returns 200 with undefined and the body never runs. A caller cannot distinguish that from a function that legitimately returned nothing.
  • Calling fn twice double-commits. This is the one I would rank first: it is silent, it is a data-integrity failure rather than an availability one, and the natural bug that causes it (a retry wrapper, a .then that re-invokes, a misplaced await) is invisible in review.
  • Calling fn outside the scope is the subtlest: the function runs and returns a correct-looking value, but getRequestEvent() is undefined, so every header and cookie it writes is discarded. The response is a 200 with the work done and the response state thrown away.
  • Running fn under a different event is worse still, because nothing looks wrong from inside — the function sees an event, so a defensive if (!event) throw does not fire; it is simply the wrong request's event, and getServerFunctionInvocation() returns null because the invocation map is keyed on the handler's own event object.

One correction to my own filing: the outside-scope case is not entirely silent — getRequestEvent() reports through the existing RequestEvent is missing… path whenever the function actually reads the event. It is app-triggered rather than hook-detected, and silent for a function that never reads the event, and the request still answers 200 either way. The other four failure modes produce nothing at all.

Note also that the docblock never says "exactly once" — it says the function passed "runs with event visible to getRequestEvent()". So options (1) and (3) are a contract clarification as much as an enforcement.

Options

  1. Assert single invocation. Count calls to fn inside the handler; a second call throws (dev) or is refused with a 500 naming the hook (production). Directly closes the double-commit, which is the only one of these with a data-integrity consequence.
  2. Assert the scope took effect. After provideEvent returns, check that the invocation the handler registered is the one the function saw — the machinery already exists, since INVOCATIONS is keyed by the event object and comes back null in exactly the broken cases. Catches the outside-scope and wrong-event variants, which are otherwise undetectable.
  3. Assert fn was called at all — cheapest of the three, catches the never-called case, and is a one-line flag.
  4. Dev-only diagnostics for all three, production unchanged. Lowest risk to existing deployments; catches the mistakes during development, where these are made.
  5. Document the contract's failure modes. Necessary regardless, insufficient alone: the docblock states what the hook must do, and none of the ways of failing it are visible at runtime.

(3) plus (1) measured clean: never calls fn and calls fn twice both become HTTP 500, the double-call is stopped after the first invocation (one fn=1 cookie instead of two), the other four rows are untouched, suite unchanged (48 files / 524 passed / 2 skipped).

One honest cost of (1): the double-call case answers 500 after the first invocation has already committed. Strictly better than a silent double-commit, but the caller is now told a committed mutation failed — and a retry would commit it again. If that trade is unacceptable, (4) is the shape to take.

Happy to send a PR for whichever direction you prefer. The regression test shape is the table above: six provideEvent implementations, asserting what the function observed and what reached the wire, with the healthy implementation as the control.

Related

Same shape at three seams — an integration hook's return is accepted unvalidated where the mistake is invisible: #3170 and #3174. Different packages and different fixes, so filed separately, but the enforcement question is one question and is cheaper to settle once.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions