-
Notifications
You must be signed in to change notification settings - Fork 7
fix: graceful bridge restarts — SIGTERM handling + EADDRINUSE retry #161
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -885,6 +885,36 @@ function getLogLinesForResponse(url) { | |||||||||||||||||||
| return lines; | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| /** Reference to the HTTP server so we can close it on shutdown. */ | ||||||||||||||||||||
| let apiServer = null; | ||||||||||||||||||||
| let shuttingDown = false; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| /** | ||||||||||||||||||||
| * Graceful shutdown: close the HTTP server (releases the port), then exit. | ||||||||||||||||||||
| * Called on SIGTERM/SIGINT so restarts don't fight over the port. | ||||||||||||||||||||
| */ | ||||||||||||||||||||
| function gracefulShutdown(signal) { | ||||||||||||||||||||
| if (shuttingDown) return; | ||||||||||||||||||||
| shuttingDown = true; | ||||||||||||||||||||
| logInfo(`🛑 received ${signal} — shutting down gracefully`); | ||||||||||||||||||||
| if (apiServer) { | ||||||||||||||||||||
| apiServer.close(() => { | ||||||||||||||||||||
| logInfo("🛑 HTTP server closed, exiting"); | ||||||||||||||||||||
| process.exit(0); | ||||||||||||||||||||
| }); | ||||||||||||||||||||
| // Force exit after 5s if connections don't drain | ||||||||||||||||||||
| setTimeout(() => { | ||||||||||||||||||||
| logWarn("🛑 forceful exit after 5s timeout"); | ||||||||||||||||||||
| process.exit(1); | ||||||||||||||||||||
| }, 5000).unref(); | ||||||||||||||||||||
| } else { | ||||||||||||||||||||
| process.exit(0); | ||||||||||||||||||||
| } | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| process.on("SIGTERM", () => gracefulShutdown("SIGTERM")); | ||||||||||||||||||||
| process.on("SIGINT", () => gracefulShutdown("SIGINT")); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| function startApiServer() { | ||||||||||||||||||||
| const server = createServer(async (req, res) => { | ||||||||||||||||||||
| const url = new URL(req.url, `http://localhost:${API_PORT}`); | ||||||||||||||||||||
|
|
@@ -1024,9 +1054,33 @@ function startApiServer() { | |||||||||||||||||||
| } | ||||||||||||||||||||
| }); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| server.listen(API_PORT, "127.0.0.1", () => { | ||||||||||||||||||||
| // Retry with backoff if the port is still held by a dying predecessor. | ||||||||||||||||||||
| const MAX_BIND_RETRIES = 5; | ||||||||||||||||||||
| const BIND_RETRY_DELAY_MS = 2000; | ||||||||||||||||||||
| let bindAttempt = 0; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| function tryListen() { | ||||||||||||||||||||
| bindAttempt++; | ||||||||||||||||||||
| server.listen(API_PORT, "127.0.0.1"); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| server.on("listening", () => { | ||||||||||||||||||||
| apiServer = server; | ||||||||||||||||||||
| logInfo(`📡 Outbound API listening on http://127.0.0.1:${API_PORT}`); | ||||||||||||||||||||
| }); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| server.on("error", (err) => { | ||||||||||||||||||||
| if (err.code === "EADDRINUSE" && bindAttempt < MAX_BIND_RETRIES) { | ||||||||||||||||||||
| logWarn(`⚠️ port ${API_PORT} in use, retrying in ${BIND_RETRY_DELAY_MS}ms (attempt ${bindAttempt}/${MAX_BIND_RETRIES})`); | ||||||||||||||||||||
| server.close(); | ||||||||||||||||||||
| setTimeout(tryListen, BIND_RETRY_DELAY_MS); | ||||||||||||||||||||
| } else { | ||||||||||||||||||||
|
Comment on lines
1072
to
1077
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing
Suggested change
Prompt To Fix With AIThis is a comment left during a code review.
Path: slack-bridge/broker-bridge.mjs
Line: 1072-1076
Comment:
**Missing `server.close()` before retry**
The [Node.js documentation](https://nodejs.org/api/net.html#event-error) recommends calling `server.close()` before retrying `server.listen()` after an EADDRINUSE error, to properly reset the server's internal state. While the current code may work in practice (the failed bind doesn't create resources), adding `server.close()` follows the documented pattern and avoids potential edge cases in future Node.js versions.
```suggestion
if (err.code === "EADDRINUSE" && bindAttempt < MAX_BIND_RETRIES) {
logWarn(`⚠️ port ${API_PORT} in use, retrying in ${BIND_RETRY_DELAY_MS}ms (attempt ${bindAttempt}/${MAX_BIND_RETRIES})`);
server.close();
setTimeout(tryListen, BIND_RETRY_DELAY_MS);
```
How can I resolve this? If you propose a fix, please make it concise. |
||||||||||||||||||||
| logError(`❌ HTTP server error: ${err.message}`); | ||||||||||||||||||||
| process.exit(1); | ||||||||||||||||||||
| } | ||||||||||||||||||||
| }); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| tryListen(); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| async function startPollLoop() { | ||||||||||||||||||||
|
|
||||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.