feat(devframe): add onServerError for post-bind server errors - #165
feat(devframe): add onServerError for post-bind server errors#165dvcolomban wants to merge 2 commits into
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
Adds an onServerError callback option to startHttpAndWs so callers can observe owned HTTP server 'error' events beyond the initial bind window, without exposing the raw httpServer (and risking incorrect shutdown paths).
Changes:
- Introduces
onServerError?: (error: Error) => voidonStartHttpAndWsOptionsand wires it into the owned-server lifecycle. - Ensures bind-time listen failures still reject with structured diagnostic
DF0052, while also forwarding the underlying bind error toonServerError. - Adds tests covering listen failure rejection and bind-error forwarding, and adds documentation for
DF0052.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/devframe/src/node/server.ts | Adds onServerError option and attaches HTTP server error listeners around listen + runtime. |
| packages/devframe/src/node/diagnostics.ts | Defines new structured diagnostic DF0052 for listen failures. |
| packages/devframe/src/node/tests/server.test.ts | Adds regression tests for port-in-use failures and onServerError forwarding. |
| docs/errors/DF0052.md | Documents DF0052 cause/fix/source. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
startHttpAndWs's owned httpServer has no error listener once past the bind window, so a later runtime error (e.g. a transient EMFILE while accepting a connection) crashes the process. StartedServer exposes no handle a caller could attach their own listener to either, unlike the shared-server path where the caller already owns the object. Add onServerError instead of exposing the raw httpServer — handing out the raw object would let a caller call close() on it directly, bypassing the wrapper's own close() and leaking the WS transport. Attached only after the bind has already succeeded, so this never touches bind-time crash/hang semantics — only what happens afterward. No test: StartedServer deliberately doesn't expose the raw server, so there's no way to trigger a genuine post-bind error through the public API without a test-only seam. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
f8a28a7 to
e5a717e
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (1)
packages/devframe/src/node/server.ts:272
onServerErroris attached only after theawaitresolves, leaving a small race window between thelistencallback firing and the handler being registered. Anerroremitted in that gap would still be unhandled and could crash the process. You can avoid this by registering the handler inside thelistencallback (still only after bind success) before resolving the promise.
await new Promise<void>((resolveListen) => {
httpServer.listen(port, bindHost, () => resolveListen())
})
The option silently does nothing when a `server` is supplied, so lead the JSDoc with that constraint (and why — the caller owns that object and its crash semantics are not devframe's to change) instead of mentioning it mid-paragraph. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (1)
packages/devframe/src/node/server.ts:275
onServerErroris attached only after theawaitresolves, which leaves a small race window where the server is already listening/accepting connections but still has no'error'listener. If an error is emitted in that window, the process can still crash, undermining the intent of this option. Attach the handler inside thelisten()callback before resolving the promise to avoid the gap (still only after a successful bind).
await new Promise<void>((resolveListen) => {
httpServer.listen(port, bindHost, () => resolveListen())
})
// Attached only now that the bind has already succeeded — this never
// changes bind-time crash/hang semantics, only what happens afterward.
if (options.onServerError)
httpServer.on('error', options.onServerError)
}
What
Once
startHttpAndWs's ownedhttpServeris past its bind, it has zero error listeners for the rest of its life — a later runtime error (a transientEMFILEwhile accepting a connection, say) still crashes the process, and there's no way for a caller to defend against it:StartedServerdoesn't expose the underlying server, unlike the shared-server path where the caller already owns that object.Adds
onServerError?: (error: Error) => void, attached only after the bind has already succeeded — so it doesn't touch bind-time crash/hang semantics at all, just gives the server's remaining lifetime an escape hatch.Why not just expose httpServer
Considered it, but handing out the raw object would let a caller call
.close()on it directly, bypassing the wrapper's ownclose()and leaking the WS transport — basically the same class of bug #163 fixes, through a different door. A narrow callback matches the shape the file already uses (onReady,onPeerConnect,rpcOptions.onFunctionError/onGeneralError) without that risk.Why it's gated on
ownsHttpServerDeliberate, and the JSDoc leads with it: passing
onServerErroralongsideserverdoes nothing.A
net.Serverwith zero'error'listeners crashes the process; with one, it doesn't. Attaching to a caller-supplied server would flip that server's failure mode process-wide for as long as the devframe is mounted — on a Next or Vite host that's the host's own server, and its crash semantics aren't devframe's to change.close()also deliberately leaves a caller-owned server running, so the listener would outlive the devframe and keep firing; making that symmetric would need aclose()-time detach on top. And on that path the caller already holds the object, soserver.on('error', handler)is one line on their side — the owned path having no such handle is the entire gap here. Matches howlisten,destroyUnmatched,closeandseparateWsPortare already gated in this file.Note for maintainers — no test here, on purpose
StartedServerdeliberately doesn't expose the raw server (that's the whole point of this over just exposing it), so there's no way to trigger a genuine post-bind error through the public API without adding a test-only seam. The wiring itself is a one-line pass-through of Node's own documented'error'event, so I'm leaving it to review rather than inventing something fragile to cover it.I also considered forwarding bind-time errors to the same callback, so it'd be one place to observe every server error instead of two — but dropped it. That's only safe once #163's promise-settlement fix is also in: attaching any listener during the bind window changes whether Node treats a failed bind as fatal, and without #163 that trades a loud crash for a silent hang for anyone who opts in early. Independent of #163 (this PR doesn't depend on it, and applies cleanly to
maineither way), so not worth that risk here.Tests
pnpm lint && pnpm knip && pnpm test && pnpm typecheck && pnpm build— all green (1054 tests, unchanged — see above for why there's no new one).