fix(push): report connect failures instead of leaving them unhandled - #232
Merged
Merged
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new .catch() handler can still reintroduce an unhandled rejection if client.log(...) throws (e.g., due to a user-provided log listener), undermining the crash-prevention goal.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR hardens the push connection startup (packages/linejs/base/push/conn.ts) so transport/stream-init failures don’t surface as unhandled promise rejections on Deno, allowing the existing reconnect loop to keep the host process alive.
Changes:
- Catch and report errors from the floating connect IIFE in
Conn.new()and resolve the connect race so the caller proceeds into the established reconnect flow. - Add a dedicated test suite for
Conn.new()covering fetch rejection, missing response body, and success behavior.
File summaries
| File | Description |
|---|---|
| packages/linejs/base/push/conn.ts | Adds a .catch() to the connect IIFE to log failures and prevent unhandled rejections from terminating the process. |
| packages/linejs/base/push/conn.test.ts | Adds tests validating that connect failures are reported (not unhandled) and that successful connects still set resStream. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+140
to
+144
| // reconnect loop in packages/linejs/base/polling/mod.ts retries the | ||
| // connection. | ||
| this.client.log("LegyPusherError", { error }); | ||
| resolve(); | ||
| }); |
`Conn.new` races an async IIFE against a 300 ms timer to get the push
stream started. Nothing awaits or catches that IIFE, so when the fetch
inside it failed -- a reset connection, DNS trouble, the endpoint
refusing the h2 stream -- the rejection had nowhere to go and became an
unhandled rejection. On Deno that kills the host process outright:
"Uncaught (in promise) TypeError: fetch failed". The same applies to the
`no body` throw a few lines below it.
Catch the rejection, resolve, and report it through
`client.log("LegyPusherError", ...)` like the other pusher error sites.
Resolve runs first and the log call is guarded because `log` is a
user-supplied listener: letting it throw would reject this handler and
put back the very unhandled rejection it exists to prevent. `resStream`
is left unset, so `read()` throws "no resStream" and the existing
reconnect loop in base/polling/mod.ts sleeps and retries, which is what
a dropped connection should do. The 300 ms race is untouched.
(cherry picked from commit 15c4142)
frankekn
force-pushed
the
upstream/push-connect-rejection
branch
from
September 5, 2026 16:28
27f1c6d to
fe00767
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A failed push connect takes the whole host process down on Deno, instead of reconnecting.
Conn.newstarts the push stream in an async IIFE that is raced against a 300 ms fallback timer (packages/linejs/base/push/conn.ts:120-137):Nothing awaits or catches that IIFE, and the outer promise is resolved by the timer regardless. When the
fetchinside it rejects — a reset connection, DNS trouble, the endpoint refusing the h2 stream — the rejection has nowhere to go and becomes an unhandled rejection:which terminates the process. The
no bodythrow two lines below has the same fate. Any network hiccup is enough, so a listener that runs for days eventually dies from a transient failure rather than reconnecting through the retry loop that already exists for exactly this.Fix
Attach a
.catch()to the IIFE: log the error throughclient.log("LegyPusherError", { error })— the same channelinitLegyPusheralready uses for pusher failures (packages/linejs/base/polling/mod.ts:170) — andresolve().resStreamis then simply left unset, so the nextread()throwsno resStreamand the existing reconnect loop ininitLegyPusher(packages/linejs/base/polling/mod.ts:157-173) sleeps 4 s and reconnects, which is what a dropped connection should do. The 300 ms race and the success path are untouched, andreqStreamis still handed back so the caller can close the conn.Testing
New
packages/linejs/base/push/conn.test.ts, 3 cases against a stub whosefetchis controlled per test:fetchrejects →new()resolves, oneLegyPusherErroris logged carrying the originalTypeError,resStreamstays unset. Onmainthe unhandled rejection is raised instead (Deno's test sanitizer reports it as a failure, the same way the runtime reports it as a crash).no bodypath is covered too.resStreamand logs nothing.cd packages/linejs && deno test -A— 219 passed, 0 failed (216 onmain@ 802f4c7).deno fmt --checkanddeno checkclean on both touched files.This change is independent of the other pull requests open from this fork; it touches only
base/push/conn.ts.