Skip to content

add error handling and graceful shutdown #302

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

nikgraf
Copy link
Collaborator

@nikgraf nikgraf commented Jul 2, 2025

No description provided.

Copilot

This comment was marked as outdated.

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR enhances server robustness by adding middleware for request timeouts, centralized error handling, and graceful shutdown logic.

  • Introduces a 30s timeout middleware to send 408 responses on slow requests
  • Adds global error and 404 handlers for consistent API responses
  • Registers process-level handlers for unhandled rejections, uncaught exceptions, and OS signals to cleanly close HTTP and WebSocket servers
Comments suppressed due to low confidence (2)

apps/server/src/index.ts:71

  • Add tests to verify the 30s timeout triggers correctly and returns a 408, and to simulate long-running routes to confirm the behavior.
// Request timeout middleware

apps/server/src/index.ts:74

  • [nitpick] For consistency with other error responses, consider adding a message field alongside error, or align the JSON shape with your global error handler.
    res.status(408).json({ error: 'Request timeout' });

console.error('Unhandled Rejection at:', promise, 'reason:', reason);
});

process.on('uncaughtException', (error) => {
Copy link
Preview

Copilot AI Jul 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this handler, you close only the HTTP server. To fully shut down, also call webSocketServer.close() before exiting to avoid leaving socket connections open.

Copilot uses AI. Check for mistakes.

const server = app.listen(PORT, () => {
console.log(`Listening on port ${PORT}`);
});

// Global process error handlers
process.on('unhandledRejection', (reason, promise) => {
console.error('Unhandled Rejection at:', promise, 'reason:', reason);
Copy link
Preview

Copilot AI Jul 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uncaught promise rejections can leave the process in an inconsistent state. Consider invoking the same shutdown logic as for uncaughtException or at least logging and exiting after a grace period.

Suggested change
console.error('Unhandled Rejection at:', promise, 'reason:', reason);
console.error('Unhandled Rejection at:', promise, 'reason:', reason);
// Graceful shutdown
server.close(() => {
console.log('Server closed due to unhandled rejection');
webSocketServer.close(() => {
console.log('WebSocket server closed');
process.exit(1);
});
});
// Force close after 30 seconds
setTimeout(() => {
console.error('Could not close connections in time, forcefully shutting down');
process.exit(1);
}, 30000);

Copilot uses AI. Check for mistakes.

Comment on lines +73 to +75
res.setTimeout(30000, () => {
res.status(408).json({ error: 'Request timeout' });
});
Copy link
Preview

Copilot AI Jul 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After sending the timeout response, ensure no further handlers write to the response (e.g., by storing a flag or directly ending the response) to avoid double send errors.

Suggested change
res.setTimeout(30000, () => {
res.status(408).json({ error: 'Request timeout' });
});
let responseSent = false;
res.setTimeout(30000, () => {
if (!responseSent) {
responseSent = true;
res.status(408).json({ error: 'Request timeout' });
}
});
res.on('finish', () => {
responseSent = true;
});

Copilot uses AI. Check for mistakes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant