feat(adapter/server): new Bunny.net Edge Scripting#179
feat(adapter/server): new Bunny.net Edge Scripting#179
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Bunny.net WebSocket support: new Bunny adapter and types, server plugin and serve wrapper, package exports and build config updates, documentation, and a basic test. New adapter entrypoints and WSOptions.bunny configuration were introduced. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
| }); | ||
|
|
||
| socket.addEventListener("error", (error) => { | ||
| peers.delete(peer); |
There was a problem hiding this comment.
Do we know that always error === disconnect? (
There was a problem hiding this comment.
Good point. No the docs do not specify this, which I would also assume they do not equal one another (since their websocket implementation lives within the CDN network regardless of PoP and where the edge script is being executed from). I did add it because I was noticing a strange behaviour while developing it, but since on idle they BOTH get fired I would remove the delete, let me fix it
There was a problem hiding this comment.
I might not have explained myself correctly, but in the topic of living in the CDN network I understood it as in case of error (like a PoP going down 😰) they should still be able to recover routing somewhere else. This should in theory end up in an error but not a disconnect, just connection hanging during transition
At least, this is how I understood it, but might not be the most experienced in this topic
There was a problem hiding this comment.
In 99c704f I decided to reintroduce peer.delete since:
- it seems like their upgrade is based on the official Deno one
- Edge Scripting only has 128MB of memory, personally I would prefer to be safe than sorry
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@docs/2.adapters/bunny.md`:
- Around line 75-77: The docs reference to test/fixture/bunny.ts is broken;
either add a demo fixture file matching that path (test/fixture/bunny.ts) that
demonstrates the adapter usage or remove/update the ::read-more block in
docs/2.adapters/bunny.md so it only links to src/adapters/bunny.ts (and/or to an
existing demo file); ensure the docs no longer point to the nonexistent
test/fixture/bunny.ts link.
In `@src/adapters/bunny.ts`:
- Around line 96-99: The error handler for the Bunny socket is passing the
entire ErrorEvent object into WSError; update the
socket.addEventListener("error", (error) => {...}) callback to extract and pass
the underlying error (use event.error) into new WSError so you preserve real
error details—i.e., change the parameter name to event (or keep error but
reference error.error) and call peers.delete(peer); hooks.callHook("error",
peer, new WSError(event.error)); to mirror the Cloudflare adapter's behavior.
🧹 Nitpick comments (1)
src/adapters/bunny.ts (1)
22-30: JSDoc says@default 30but no default is applied in code.The JSDoc documents
@default 30(seconds), but the adapter code (lines 62-64) only passesidleTimeoutthrough when explicitly set — it never applies a default of30. This is fine if the intent is to document Bunny's platform default, but readers may expect the adapter itself to enforce this default. Consider clarifying the JSDoc (e.g., "Defaults to30on the Bunny platform") to avoid ambiguity.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/adapters/bunny.ts`:
- Around line 51-55: The handleUpgrade function should destructure
upgradeHeaders from the result of hooks.upgrade() so custom headers aren’t
discarded; update the call in handleUpgrade to pull { endResponse, context,
namespace, upgradeHeaders } from hooks.upgrade(), then apply those headers when
completing the WebSocket upgrade via the platform API (or, if Bunny’s
upgradeWebSocket does not accept custom response headers, add a clear
comment/doc note in handleUpgrade explaining the limitation and why
upgradeHeaders cannot be applied). Ensure you reference handleUpgrade and
hooks.upgrade() and, if using the request.upgradeWebSocket API, attempt to pass
upgradeHeaders there or document the incompatibility.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/adapters/bunny.ts`:
- Around line 106-110: Add a peers.delete(peer) call to the Bunny adapter's
socket "error" event handler to avoid leaking peers when an "error" fires
without a subsequent "close"; specifically, in the error listener where
hooks.callHook("error", peer, new WSError(error)) is invoked, first call
peers.delete(peer) (it's idempotent so safe to also keep the existing delete in
the "close" handler) so the peer is removed from the peers Set regardless of
whether "close" follows.
🧹 Nitpick comments (1)
src/adapters/bunny.ts (1)
58-78:upgradeHeadersis only consumed for protocol negotiation; all other headers are silently discarded.You destructure
upgradeHeaders(good — addresses the prior review), but the only value extracted from them issec-websocket-protocol(line 67). Any other response headers theupgradehook returns are lost because they're never attached to theresponseobject returned byupgradeWebSocket. If this is a Bunny platform limitation, a brief comment would help future maintainers understand why.
| socket.addEventListener("error", (error) => { | ||
| // Note: on idle timeout Bunny.net will emit both "error" and "close" events, | ||
| // so the peer will already be deleted when we receive the "error" event. | ||
| hooks.callHook("error", peer, new WSError(error)); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all error event handlers in adapters to compare implementations
rg -n "addEventListener.*error" -A 10 --type=ts src/adapters/Repository: h3js/crossws
Length of output: 1864
Missing peers.delete(peer) in the error handler — potential peer leak.
The Deno and Cloudflare adapters both call peers.delete(peer) inside the error handler as a safety net. The comment in Bunny notes that idle-timeout errors are accompanied by a close event, but other error scenarios may fire error without close. In that case the peer is never removed from the set. Since Set.delete is idempotent, calling it in both error and close handlers is harmless and safe.
Proposed fix
socket.addEventListener("error", (error) => {
// Note: on idle timeout Bunny.net will emit both "error" and "close" events,
// so the peer will already be deleted when we receive the "error" event.
+ peers.delete(peer);
hooks.callHook("error", peer, new WSError(error));
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| socket.addEventListener("error", (error) => { | |
| // Note: on idle timeout Bunny.net will emit both "error" and "close" events, | |
| // so the peer will already be deleted when we receive the "error" event. | |
| hooks.callHook("error", peer, new WSError(error)); | |
| }); | |
| socket.addEventListener("error", (error) => { | |
| // Note: on idle timeout Bunny.net will emit both "error" and "close" events, | |
| // so the peer will already be deleted when we receive the "error" event. | |
| peers.delete(peer); | |
| hooks.callHook("error", peer, new WSError(error)); | |
| }); |
🤖 Prompt for AI Agents
In `@src/adapters/bunny.ts` around lines 106 - 110, Add a peers.delete(peer) call
to the Bunny adapter's socket "error" event handler to avoid leaking peers when
an "error" fires without a subsequent "close"; specifically, in the error
listener where hooks.callHook("error", peer, new WSError(error)) is invoked,
first call peers.delete(peer) (it's idempotent so safe to also keep the existing
delete in the "close" handler) so the peer is removed from the peers Set
regardless of whether "close" follows.
There was a problem hiding this comment.
But what if an error originates from the CDN network which is the one responsible for distributing and aggregating websocket connections? Like for example a PoP crashes and the clients that were using it get directed to a new PoP?
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Bunny.net Edge Scripting WebSocket error handling CDN failure PoP crash
💡 Result:
What this appears to refer to (based on public docs/posts)
1) Bunny.net WebSockets support (CDN / Pull Zone)
Bunny.net does support WebSockets on its CDN; you enable it per Pull Zone in the dashboard (Pull Zone → General → WebSockets). Their docs also note the included free tier of 500 concurrent connections (with paid upgrades for higher limits). [1] [2]
2) Edge Scripting “WebSocket is closed before the connection is established” (dashboard logging/preview)
A third-party Edge Scripting doc (Bunny Launcher) notes a known bug where Edge Script logging “breaks,” and the browser console shows an error like:
WebSocket connection to '<URL>' failed: WebSocket is closed before the connection is established.
This suggests the issue may be with the dashboard’s log streaming WebSocket (used by the editor/preview), not necessarily your own application WebSocket traffic. [3]
3) “CDN failure / PoP crash” scenario (real incident write-up)
Bunny.net published a detailed postmortem describing a failure chain where a corrupted file caused a stack overflow that bypassed exception handling and repeatedly crashed processes, knocking out large parts of their DNS fleet; knock-on effects included CDN/storage/deployment dependencies failing together. In the same incident, they describe requests being misrouted such that Madrid (a smaller PoP) got overwhelmed (“obliterated”). [4]
Practical error-handling implications (tying the above together)
- If you’re debugging Edge Scripting logs and you see that “WebSocket … closed before established” message, treat it as potentially a dashboard logging/preview transport issue (per the noted bug), and validate behavior with an external logger/telemetry path—not only the in-dashboard log pane. [3]
- If you’re building WebSocket apps behind Bunny CDN, plan for PoP-level failures and abrupt disconnects: clients should handle unexpected
closeand retry with backoff, because outages/misrouting/edge instability can terminate long-lived connections even if your origin is fine. Bunny’s own postmortems show how internal dependency failures can cascade and impact traffic routing/availability. [4] - Confirm your Pull Zone has WebSockets enabled and that you’re within any concurrency limits for your tier. [1] [2]
Sources:
[1] Bunny.net docs — WebSockets (enablement, pricing/limits)
[2] Bunny.net support — “Do you support WebSockets?”
[3] Bunny Launcher Edge Scripting docs — notes bug breaking logging with that WebSocket error
[4] Bunny.net blog — “The stack overflow of death. Why we were down for 2 hours” (DNS/CDN failure chain; Madrid PoP overwhelmed)
@sandros94 That's an excellent question and a very real concern! The web search found that Bunny.net has documented actual infrastructure failures, including a postmortem where PoP crashes and routing failures caused abrupt connection terminations.
The critical issue is: Bunny.net's Edge Scripting documentation doesn't provide explicit guarantees about which WebSocket events fire (or in what order) during infrastructure-level failures like:
- PoP crashes
- Network partitions between PoPs
- Edge script process terminations
- CDN routing failures
During these scenarios, your edge script might be terminated abruptly, and the normal event flow (error → close) may not complete. The "error" event might fire without a subsequent "close", or the process could be killed before either fires.
This is exactly why the defensive programming approach is appropriate:
Set.delete(peer)is idempotent — calling it multiple times is safe and has no side effects- Other adapters (Deno, Cloudflare) already do this — they call
peers.deletein both handlers as a safety net - Infrastructure failures are unpredictable — you can't rely on clean event ordering when the underlying infrastructure is failing
The proposed fix ensures that regardless of which event fires first (or if only one fires), the peer is removed from the set, preventing memory leaks during CDN infrastructure failures.
sandros94
changed the title
2 participants
Discussed privately, for Bunny.net Edge Scripting
Summary by CodeRabbit
New Features
Documentation
Exports
Tests