fix: edge adapter correctness + adapter test coverage (F29, F46, F9)#242
fix: edge adapter correctness + adapter test coverage (F29, F46, F9)#242pi0x wants to merge 1 commit into
Conversation
Cloudflare (F29, locked: keep the global fetch listener, fix the bugs):
- Guard against double-registration: repeated `serve()` calls no longer
stack duplicate `fetch` listeners (which caused a double `respondWith`).
- `close()` now removes exactly the listener `serve()` added.
- Thread the event's env through the service-worker listener instead of a
hardcoded `{}`, and document that env bindings are only available in
module-worker syntax.
- Make `request.ip` `configurable` (like bun/deno) so `trustProxy` can
override it; still populated from `cf-connecting-ip`.
Bunny:
- Guard the `waitUntil` closure with `typeof Bunny` so it no longer throws
a `ReferenceError` outside the Bunny runtime (e.g. `manual: true`).
Service worker:
- `close()` now unregisters only the registration `serve()` created,
instead of every service worker on the origin.
Tests (F46 + F9):
- Add in-process tests for the `generic`, `bunny`, and `cloudflare`
adapters (cloudflare via mocked service-worker/module-worker globals —
best-effort in place of a @cloudflare/vitest-pool-workers job, which
needs a separate worker runtime/config).
- Add one unhandled-throw test per runtime (F9): Deno and Bun answer 500
and keep serving; Node's current behavior (an uncaughtException that
would crash the process) is captured and documented — the fix belongs
to the Node adapter scope.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 48 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
commit: |
pi0x
left a comment
pi0x
left a comment
There was a problem hiding this comment.
Review: PR #242 — edge adapter correctness + adapter test coverage
Verdict: Approve with minor findings. No blocking bugs. Scope is clean (only cloudflare.ts, bunny.ts, service-worker.ts + test files; no deno.ts/bun.ts/_plugins.ts source, no prose docs). Full suite green: pnpm vitest run = 1002 passed / 35 skipped; lint + typecheck (post-build) clean; new files stable over 5 isolated runs; bun/deno error-path additions verified locally (both return 500 and stay alive).
Spec checklist
| Item | Status |
|---|---|
F29(a) no double-registration on repeated serve() |
PASS — per-instance #fetchListener guard, early-return |
F29(b) close() removes exactly that listener |
PASS — removeEventListener + clears the field; serve() after close() re-registers |
F29(c) real env threaded instead of {} |
PARTIAL — see F-1 (module-worker path already correct on main; the SW-listener change is a no-op) |
| F29(d) JSDoc: bindings module-syntax-only | PASS — accurate and honest that SW-mode env is {} |
CF ip configurable:true from cf-connecting-ip |
PASS |
Bunny waitUntil typeof Bunny guard |
PASS — no ReferenceError under manual:true; short-circuits cleanly when Bunny exists but unstable doesn't |
SW close() unregisters only its own registration |
PASS for the over-unregister bug; see F-2 for the async race |
| F46 generic/bunny/cloudflare tests | PASS — genuinely exercise the guard + close paths (not just mirrors); caveat in F-1 |
| F9 bun/deno exec 500 + alive | PASS |
| F9 node uncaughtException documented not fixed | PASS — honest (independently verified below) |
Findings
F-1 (low / accuracy) — cloudflare.ts:88: the service-worker env thread is dead code. event.respondWith(this.fetch(event.request, (event as any).env || {}, event)) — a service-worker FetchEvent never carries .env (bindings are globals), so this is always {}. The meaningful env threading is the module-worker path (this.fetch = (request, env, context) => … { env, context }), which already existed on main — this PR does not change it. So F29(c) is effectively already satisfied for the case that matters, and the listener edit neither can nor does improve it. Not a bug (the JSDoc is honest), but the PR description oversells "threads env through the service-worker listener." Consider dropping the (event as any).env read for a plain {} to avoid implying a binding source that cannot exist.
F-2 (low / browser-only race) — service-worker.ts:125-137: close() does not await #listeningPromise. #registration is only assigned inside the register().then() at :74-75. If close() runs before that promise resolves (serve() immediately followed by close()), #registration is still undefined, await this.#registration?.unregister() no-ops, and the registration completes afterward — leaving a worker registered that close() was meant to remove. The spec explicitly asked to check this race; the fix is one line: await this.#listeningPromise before the unregister block. Narrow (browser-window mode, tight timing) so non-blocking, but it is a real gap against the stated spec.
F-3 (informational) — cloudflare.ts:35,81: the double-registration guard is per-instance. Two separate serve() calls (two CloudflareServer instances) each add a global fetch listener → double respondWith() on the same event → Cloudflare throws. The guard only covers same-instance repeat calls (which is what the spec asked for). Pre-existing footgun for SW syntax; noting for awareness, not a blocker.
F-4 (informational) — node-error-paths.test.ts: weak OR assertion. expect(uncaught || (status !== undefined && status >= 500)) passes whether Node hangs (uncaughtException) or returns 500, so it would not catch a future change in either direction. I verified independently that today the escape genuinely happens — a sync throw yields uncaughtException:"unhandled sync error" and the client fetch times out (~1.5s, no response); the async path yields unhandledRejection + the same hang. So the test is honest now, but the assertion doesn't lock the documented behavior. Containment is adequate: prependListener("uncaughtException") + afterEach cleanup; no flake across isolated and full-suite runs, vitest worker survives.
Escalation recommendation (Node uncaughtException)
Yes — escalate to a code fix in the Node scope. This is a genuine cross-runtime reliability defect, not just a doc note: an unhandled handler throw on Node hangs the socket and, on an unguarded process, crashes via uncaughtException — while Deno and Bun both answer 500 and keep serving. That is a DoS-shaped hang and a portability trap (code written on Node "works" until moved). node.ts should catch handler throws/rejections and emit 500 like the other runtimes. This PR correctly documents-and-defers (node.ts is out of its scope); recommend filing it as a Node-scope T1/T2 item rather than leaving it only as a test comment.
Nice work overall — the guard/close/env-doc trio is coherent, the bunny guard is correct, and the F9 exec tests are a clean, zero-CI-wiring addition.
2 participants
Edge-adapter batch of the v1 stabilization plan.
Fixes
Cloudflare — F29 (locked: keep the global
fetchlistener, fix the bugs)serve()retains its listener; repeated calls (ormanual: truethenserve()) no longer stack a second listener, which previously caused a doublerespondWith()error.close()removes the listener it registered.envthrough the service-worker listener instead of a hardcoded{}. Cloudflare exposes bindings as globals in service-worker syntax, soenvis only meaningfully available via module-worker syntax — documented in the class JSDoc.request.ipis nowconfigurable: true(matching bun/deno) sotrustProxycan override it; still populated fromcf-connecting-ip.Bunny
waitUntilclosure withtypeof Bunny !== "undefined"so it no longer throws aReferenceErrorwhen the adapter runs outside the Bunny runtime (e.g.manual: true).Service worker
close()unregisters only the registrationserve()created, instead of every service worker on the origin.Tests
F46 — adapter coverage
test/generic.test.ts— in-process tests (middleware, plugins,error,waitUntil/close).test/bunny.test.ts— mockedBunnyglobal (registration, single-registration,waitUntildelegation,ip, and the no-runtimeReferenceErrorregression).test/cloudflare.test.ts— best-effort in-process test with mocked service-worker globals + direct module-workerfetch(request, env, context)calls, covering all three F29 behaviors and the configurableip.@cloudflare/vitest-pool-workerssmoke job needs its own worker runtime + wrangler + a separate vitest project, which conflicts with the existing single-config Node suite, so the in-process fallback was used (as the plan allows).F9 — error paths (one unhandled-throw test per runtime)
test/_error-fixture.ts— a server with noerroroption (soerrorPluginno-ops and throws propagate to the runtime).test/_error-tests.ts, wired intodeno.test.ts/bun.test.ts— zero extra CI wiring): both answer 500 and keep serving.test/node-error-paths.test.ts, in-process): documents Node's current behavior — an unhandled throw escapes as anuncaughtExceptionand leaves the connection hanging (an unguarded server would crash). Making Node answer 500 like Deno/Bun belongs to the Node adapter scope, so it is documented, not fixed here.Verification
pnpm lintclean,pnpm typecheckclean (afterpnpm build), fullvitest run: 1002 passed / 35 skipped.🤖 Generated with Claude Code