Uh oh!
There was an error while loading. Please reload this page.
fix(devframe): reject on server listen errors instead of hanging - #163
Conversation
✅ Deploy Preview for devfra ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull request overview
This PR fixes a failure mode in startHttpAndWs() where an owned HTTP server listen() bind error could previously cause an uncaught exception and leave the returned promise pending forever. It introduces a listen-scoped error handler that rejects properly, tears down the already-attached WS transport, and surfaces the failure via a new structured diagnostic DF0052.
Changes:
- Attach an
'error'listener during the owned-serverlisten()and reject/throwDF0052on bind failures (with WS teardown). - Add
DF0052to node diagnostics and document it underdocs/errors/. - Add a regression test ensuring a second server on the same port rejects instead of hanging.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/devframe/src/node/server.ts | Adds listen-scoped error handling + WS teardown, throwing DF0052 on bind failures. |
| packages/devframe/src/node/diagnostics.ts | Introduces the new DF0052 diagnostic definition. |
| packages/devframe/src/node/tests/server.test.ts | Adds a regression test for “port already taken” rejection behavior. |
| docs/errors/DF0052.md | Documents DF0052 message, cause, fix, and source. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| await new Promise<void>((resolve, reject) => { | ||
| const onError = (error: Error): void => reject(error) | ||
| // Without this listener a failed bind emits `error` with nobody | ||
| // attached — an uncaughtException — and the `listen` callback never | ||
| // fires, so this promise never settles. | ||
| httpServer.once('error', onError) | ||
| httpServer.listen(port, bindHost, () => { | ||
| httpServer.removeListener('error', onError) | ||
| resolve() | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Leaving this one as-is — I think it's inert here.
listen() only throws synchronously for ERR_SOCKET_BAD_PORT / ERR_SERVER_ALREADY_LISTEN. That throw happens inside the Promise executor, so the promise rejects, the catch below wraps it as DF0052, and startHttpAndWs throws — httpServer is never handed back to anyone. This block only runs under ownsHttpServer, so that server was created a few lines up and its only reference is the closure being unwound: the server and the listener become garbage together, and a listener on an unreachable object isn't a leak. Even if something did somehow emit 'error' on it afterwards, reject on a settled promise is a no-op.
The leak you're describing would be real on a caller-supplied server — but that's the branch this code never takes.
Uh oh!
There was an error while loading. Please reload this page.
startHttpAndWs's owned-server listen had no 'error' listener, so a
failed bind (e.g. EADDRINUSE) emitted 'error' with nobody attached —
an uncaughtException — while the listen promise never settled,
leaving createDevServer permanently pending. A caller doing
try { await createDevServer(...) } catch {} could not observe the
failure at all.
Attach a listen-scoped 'error' handler and reject with it. The WS RPC
transport is already attached by the time listen runs, so tear it
down via closeWs() before throwing to avoid leaking it and its peers.
The rejection is a new DF0052 diagnostic carrying the original node
error as `cause`, so callers can still branch on `error.cause.code`
(e.g. 'EADDRINUSE') while getting an actionable message and fix hint.
🤖 Generated with [Claude Code](https://claude.com/claude-code)c770b07 to
0041d0fCompareThe DF0052 catch assumed the caught value was an Error and cast both `message` and `cause`. `listen()` can throw arbitrary values, so derive the reason with the `error instanceof Error ? error.message : String(error)` pattern already used at both DF0045 call sites in instance-registry.ts, and pass the original value through as `cause` uncast. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Uh oh!
There was an error while loading. Please reload this page.
What
startHttpAndWs's owned-serverlisten()never had an'error'listener anywhere in the file. On a failed bind — most commonlyEADDRINUSE, another process (often a previous devframe instance) already on the port — two things happen at once:net.Serveremits'error'with nobody attached, which becomes anuncaughtException.await new Promise(...)around the listen call never settles, sostartHttpAndWs(andcreateDevServer) hangs forever.try { await createDevServer(...) } catch {}can't see the failure at all.This attaches a listen-scoped
'error'handler and rejects with it instead.The one subtlety
By the time
listen()runs,attachWsRpcTransporthas already bound the WS transport, and (throughcreateDevServer) the definition'ssetup()has already run. A bare reject would leak the WS server and its peers, so the fix awaitscloseWs()— already in scope — before throwing.Why a diagnostic, not the raw error
The rejection is a
DF0052diagnostic rather than the bare node error, to match the repo's structured-diagnostics convention (same{ ...identity, reason }+causeshape asDF0045/DF0046). The original error is still there aserror.cause, so branching onerror.cause.code === 'EADDRINUSE'keeps working, and the message already contains the code too (listen EADDRINUSE 127.0.0.1:9999).Note for maintainers
packages/devframe/src/rpc/transports/ws-server.tshas the identical shape (ownedServer.listen(port, host), unawaited, no handler) on its dedicated-port path. Left it alone here —attachWsRpcTransportis a sync public export with several call sites, so awaiting its listen is a signature change that's your call, not mine to fold into a fix PR. Happy to open a follow-up if useful.One more knock-on worth flagging:
packages/next/src/handler.tsbuilds itsreadypromise eagerly, so a host that never callsfetch()orclose()will now see an unhandled rejection logged instead of a silent hang — still better, but a visible change.Tests
New case in
packages/devframe/src/node/__tests__/server.test.ts: start onestartHttpAndWsinstance, start a second on the same resolved port, assertDF0052instead of a hang.pnpm lint && pnpm knip && pnpm test && pnpm typecheck && pnpm build— all green (1055 tests).