[Fix] Enhance server.js with graceful shutdown, env validation, and improved error handling#111
Open
zaibamachhaliya wants to merge 1 commit into
Conversation
…proved error handling
📝 WalkthroughWalkthroughThis PR restructures backend/.env.example with sectioned comments and a new CORS variable, and updates backend/src/server.js to add environment variable validation, unified CORS configuration, an async MongoDB/Redis-aware health check, AppError-based centralized error handling, and a graceful shutdown lifecycle with signal handlers. ChangesServer Hardening and Environment Configuration
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ServerJS
participant Mongoose
participant RedisClientInstance
Client->>ServerJS: GET /health
ServerJS->>Mongoose: check connection state
ServerJS->>RedisClientInstance: check ready state
alt MongoDB connected
ServerJS-->>Client: 200 with status details
else MongoDB not connected
ServerJS-->>Client: 503 with status details
end
sequenceDiagram
participant OS
participant ServerJS
participant Mongoose
participant RedisClientInstance
participant HTTPServer
OS->>ServerJS: SIGINT/SIGTERM/uncaughtException
ServerJS->>ServerJS: gracefulShutdown(signal)
ServerJS->>Mongoose: close() if connected
ServerJS->>RedisClientInstance: quit() if ready
ServerJS->>HTTPServer: close()
ServerJS->>OS: exit(0) or forced exit on timeout
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Contributor
Author
|
Hi please review my pr |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
backend/src/server.js (2)
175-210: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo re-entrancy guard for
gracefulShutdown.If
SIGINT/SIGTERMfires more than once (e.g., a user pressing Ctrl+C twice),gracefulShutdowncan run concurrently — closing an already-closing Mongo/Redis connection or racing onprocess.exit. Consider a simple guard flag.🔧 Suggested guard
+let isShuttingDown = false; + const gracefulShutdown = async (signal) => { + if (isShuttingDown) return; + isShuttingDown = true; console.log(`Received ${signal}. Shutting down gracefully...`);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/server.js` around lines 175 - 210, Add a re-entrancy guard to gracefulShutdown so repeated SIGINT/SIGTERM signals don’t trigger concurrent shutdown work or multiple process.exit paths. Use a simple module-level flag checked at the start of gracefulShutdown in backend/src/server.js; if shutdown has already started, return early, and set the flag before closing mongoose.default.connection, redisClientInstance, or server. Ensure the existing timeout cleanup and exit behavior still run only once.
34-63: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
ALLOWED_ORIGINSisn't validated and empty-string edge case bypasses the fallback.
requiredEnvVars(Line 34) doesn't includeALLOWED_ORIGINS. In production,process.env.ALLOWED_ORIGINS?.split(",") || [](Line 57) only falls back to[]when the var isundefined; if it's set but empty (""),?.still invokessplit, yielding[""](a truthy array), silently locking out all real origins with no startup warning. Similarly, unsetALLOWED_ORIGINSin production silently blocks all cross-origin requests instead of failing fast at startup.🔧 Suggested improvement
const requiredEnvVars = ["PORT", "MONGO_URI", "JWT_SECRET", "NODE_ENV"]; const validateEnv = () => { const missing = requiredEnvVars.filter((varName) => !process.env[varName]); if (missing.length > 0) { console.error("Missing required environment variables:"); missing.forEach((varName) => console.error(` - ${varName}`)); process.exit(1); } + if (process.env.NODE_ENV === "production" && !process.env.ALLOWED_ORIGINS) { + console.error("ALLOWED_ORIGINS must be set in production"); + process.exit(1); + } console.log("All required environment variables are present"); };origin: process.env.NODE_ENV === "production" - ? process.env.ALLOWED_ORIGINS?.split(",") || [] + ? process.env.ALLOWED_ORIGINS?.split(",").map((o) => o.trim()).filter(Boolean) || [] : ["http://localhost:5173", "http://localhost:3000"],🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/server.js` around lines 34 - 63, Update the startup env validation in server.js so production explicitly handles ALLOWED_ORIGINS alongside requiredEnvVars, using validateEnv to fail fast when it is missing or empty. In the corsOptions origin setup, normalize process.env.ALLOWED_ORIGINS by trimming and filtering empty entries before splitting, and fall back only when there are no valid origins; keep the fix localized around validateEnv and corsOptions so the empty-string case cannot produce a truthy [""] array.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/src/server.js`:
- Line 9: Remove the redundant dynamic mongoose imports in the health-check and
shutdown paths and use the existing statically imported mongoose instance
instead. Update the logic around the health-check handler and the
shutdown/cleanup code to reference mongoose.connection directly, eliminating the
extra await import("mongoose") calls and the shadowed local mongoose variable.
- Around line 116-128: The health check response currently hardcodes
healthStatus.status to "OK" in server.js, so the JSON body never reflects
failures even when the HTTP status is 503. Update the healthStatus object to
derive status from the same health decision used by isHealthy, and make sure the
response body in the health check handler reports an unhealthy value when
mongodbStatus or redisStatus is not connected. Use the existing healthStatus and
isHealthy logic in the server.js health endpoint to keep the body and HTTP
status consistent.
- Around line 183-204: Shutdown order in the server cleanup path is wrong: the
MongoDB and Redis clients are being closed before the HTTP server is drained.
Update the shutdown flow in server.js so server.close() happens first, then wait
for it to finish before calling mongoose.default.connection.close() and
redisClientInstance.quit(). Keep the existing cleanup logic in the server
shutdown block, but reorder it to preserve in-flight /api/notes and /api/auth
requests and the rate limiter’s Redis usage during graceful shutdown.
---
Nitpick comments:
In `@backend/src/server.js`:
- Around line 175-210: Add a re-entrancy guard to gracefulShutdown so repeated
SIGINT/SIGTERM signals don’t trigger concurrent shutdown work or multiple
process.exit paths. Use a simple module-level flag checked at the start of
gracefulShutdown in backend/src/server.js; if shutdown has already started,
return early, and set the flag before closing mongoose.default.connection,
redisClientInstance, or server. Ensure the existing timeout cleanup and exit
behavior still run only once.
- Around line 34-63: Update the startup env validation in server.js so
production explicitly handles ALLOWED_ORIGINS alongside requiredEnvVars, using
validateEnv to fail fast when it is missing or empty. In the corsOptions origin
setup, normalize process.env.ALLOWED_ORIGINS by trimming and filtering empty
entries before splitting, and fall back only when there are no valid origins;
keep the fix localized around validateEnv and corsOptions so the empty-string
case cannot produce a truthy [""] array.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b8b3a5a4-3003-48c0-9cf4-b27d1f7b6c53
📒 Files selected for processing (2)
backend/.env.examplebackend/src/server.js
| import { fileURLToPath } from "url"; | ||
| import dns from "dns"; | ||
| import jwt from "jsonwebtoken"; | ||
| import mongoose from "mongoose"; |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Redundant dynamic import("mongoose") despite static import.
mongoose is already statically imported at Line 9, but Lines 99 and 184 re-import it dynamically (await import("mongoose")) and access .default.connection. Since both resolve to the same cached module, this adds unnecessary async overhead on the health-check hot path (frequently polled by load balancers/orchestrators) and in the shutdown path, and creates a confusing shadowed mongoose variable.
♻️ Use the static import directly
app.get("/health", async (req, res) => {
let mongodbStatus = "Disconnected";
let redisStatus = "Disconnected";
try {
- const mongoose = await import("mongoose");
- mongodbStatus =
- mongoose.default.connection.readyState === 1
- ? "Connected"
- : "Disconnected";
+ mongodbStatus =
+ mongoose.connection.readyState === 1 ? "Connected" : "Disconnected";
} catch (error) {
mongodbStatus = "Error";
} try {
- const mongoose = await import("mongoose");
- if (mongoose.default.connection.readyState === 1) {
- await mongoose.default.connection.close();
+ if (mongoose.connection.readyState === 1) {
+ await mongoose.connection.close();
console.log("MongoDB connection closed");
}Also applies to: 99-99, 184-184
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/server.js` at line 9, Remove the redundant dynamic mongoose
imports in the health-check and shutdown paths and use the existing statically
imported mongoose instance instead. Update the logic around the health-check
handler and the shutdown/cleanup code to reference mongoose.connection directly,
eliminating the extra await import("mongoose") calls and the shadowed local
mongoose variable.
Comment on lines
+116
to
+128
| const healthStatus = { | ||
| status: "OK", | ||
| message: "Server is running", | ||
| timestamp: new Date().toISOString(), | ||
| }); | ||
| uptime: process.uptime(), | ||
| services: { | ||
| mongodb: mongodbStatus, | ||
| redis: redisStatus, | ||
| }, | ||
| environment: process.env.NODE_ENV || "development", | ||
| }; | ||
|
|
||
| const isHealthy = mongodbStatus === "Connected"; | ||
| res.status(isHealthy ? 200 : 503).json(healthStatus); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Health check status field is hardcoded to "OK" regardless of actual health.
Line 117 sets status: "OK" unconditionally, while isHealthy (Line 127) is computed afterward and only used for the HTTP status code. A client inspecting the JSON body (rather than the HTTP status) will always see "OK" even when MongoDB/Redis are disconnected — defeating the PR's goal of accurately reflecting service health.
🐛 Proposed fix
+ const isHealthy = mongodbStatus === "Connected";
+
const healthStatus = {
- status: "OK",
+ status: isHealthy ? "OK" : "ERROR",
timestamp: new Date().toISOString(),
uptime: process.uptime(),
services: {
mongodb: mongodbStatus,
redis: redisStatus,
},
environment: process.env.NODE_ENV || "development",
};
- const isHealthy = mongodbStatus === "Connected";
res.status(isHealthy ? 200 : 503).json(healthStatus);📝 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.
Suggested change
| const healthStatus = { | |
| status: "OK", | |
| message: "Server is running", | |
| timestamp: new Date().toISOString(), | |
| }); | |
| uptime: process.uptime(), | |
| services: { | |
| mongodb: mongodbStatus, | |
| redis: redisStatus, | |
| }, | |
| environment: process.env.NODE_ENV || "development", | |
| }; | |
| const isHealthy = mongodbStatus === "Connected"; | |
| res.status(isHealthy ? 200 : 503).json(healthStatus); | |
| const isHealthy = mongodbStatus === "Connected"; | |
| const healthStatus = { | |
| status: isHealthy ? "OK" : "ERROR", | |
| timestamp: new Date().toISOString(), | |
| uptime: process.uptime(), | |
| services: { | |
| mongodb: mongodbStatus, | |
| redis: redisStatus, | |
| }, | |
| environment: process.env.NODE_ENV || "development", | |
| }; | |
| res.status(isHealthy ? 200 : 503).json(healthStatus); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/server.js` around lines 116 - 128, The health check response
currently hardcodes healthStatus.status to "OK" in server.js, so the JSON body
never reflects failures even when the HTTP status is 503. Update the
healthStatus object to derive status from the same health decision used by
isHealthy, and make sure the response body in the health check handler reports
an unhealthy value when mongodbStatus or redisStatus is not connected. Use the
existing healthStatus and isHealthy logic in the server.js health endpoint to
keep the body and HTTP status consistent.
Comment on lines
+183
to
+204
| try { | ||
| const mongoose = await import("mongoose"); | ||
| if (mongoose.default.connection.readyState === 1) { | ||
| await mongoose.default.connection.close(); | ||
| console.log("MongoDB connection closed"); | ||
| } | ||
|
|
||
| if (redisClientInstance && redisClientInstance.isReady) { | ||
| await redisClientInstance.quit(); | ||
| console.log("Redis connection closed"); | ||
| } | ||
|
|
||
| if (server) { | ||
| server.close(() => { | ||
| console.log("Server closed successfully"); | ||
| clearTimeout(timeout); | ||
| process.exit(0); | ||
| }); | ||
| } else { | ||
| clearTimeout(timeout); | ||
| process.exit(0); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file first, then read the shutdown-related section with line numbers.
ast-grep outline backend/src/server.js --view expanded || true
wc -l backend/src/server.js
sed -n '1,260p' backend/src/server.js | cat -n
# Find where the HTTP server is created and how requests depend on Mongo/Redis.
rg -n "createServer|listen\\(|server\\.close|gracefulShutdown|redisClientInstance|mongoose|quit\\(|close\\(" backend/src/server.js backend/src -g '!**/node_modules/**'Repository: niharika-mente/ThinkBoard
Length of output: 13411
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the route handlers and controllers that run during requests.
ast-grep outline backend/src/routes/notesRoutes.js --view expanded || true
ast-grep outline backend/src/routes/authRoutes.js --view expanded || true
ast-grep outline backend/src/controllers/notesController.js --view expanded || true
ast-grep outline backend/src/controllers/authController.js --view expanded || true
# Read the relevant request-path code with line numbers.
sed -n '1,260p' backend/src/routes/notesRoutes.js | cat -n
sed -n '1,260p' backend/src/routes/authRoutes.js | cat -n
sed -n '1,260p' backend/src/controllers/notesController.js | cat -n
sed -n '1,260p' backend/src/controllers/authController.js | cat -nRepository: niharika-mente/ThinkBoard
Length of output: 7722
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect auth and Redis-related middleware used on request paths.
ast-grep outline backend/src/middleware/authMiddleware.js --view expanded || true
ast-grep outline backend/src/middleware/rateLimiter.js --view expanded || true
ast-grep outline backend/src/config/redis.js --view expanded || true
# Read the relevant files with line numbers.
sed -n '1,260p' backend/src/middleware/authMiddleware.js | cat -n
sed -n '1,260p' backend/src/middleware/rateLimiter.js | cat -n
sed -n '1,260p' backend/src/config/redis.js | cat -nRepository: niharika-mente/ThinkBoard
Length of output: 3109
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find all Redis client usages and whether request handlers depend on the server's redisClientInstance.
rg -n "redisClientInstance|connectRedis\\(|ratelimit|Upstash|`@upstash/redis`|redisClient\\." backend/src -g '!**/node_modules/**'
# Inspect the Upstash config referenced by the rate limiter.
ast-grep outline backend/src/config/upstash.js --view expanded || true
sed -n '1,220p' backend/src/config/upstash.js | cat -nRepository: niharika-mente/ThinkBoard
Length of output: 1843
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the auth controller used by backend/src/routes/authRoutes.js.
ast-grep outline backend/src/controllers/userAuth.js --view expanded || true
sed -n '1,260p' backend/src/controllers/userAuth.js | cat -nRepository: niharika-mente/ThinkBoard
Length of output: 4600
Drain HTTP before closing MongoDB/Redis. server.close() should happen first; /api/notes and /api/auth handlers still hit MongoDB, and the production rate limiter also uses Upstash Redis. Closing those clients before draining can make in-flight requests fail during shutdown.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/server.js` around lines 183 - 204, Shutdown order in the server
cleanup path is wrong: the MongoDB and Redis clients are being closed before the
HTTP server is drained. Update the shutdown flow in server.js so server.close()
happens first, then wait for it to finish before calling
mongoose.default.connection.close() and redisClientInstance.quit(). Keep the
existing cleanup logic in the server shutdown block, but reorder it to preserve
in-flight /api/notes and /api/auth requests and the rate limiter’s Redis usage
during graceful shutdown.
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
1 participant
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.
Fixes #88
Changes Made:
Summary by CodeRabbit
New Features
Bug Fixes