Standardized shutdown processing with Claude Code 1 +test#151
Standardized shutdown processing with Claude Code 1 +test#151
Conversation
WalkthroughCentralizes shutdown handling via a new shutdown utility. The server and shutdown route now delegate to gracefulShutdown(). The server registers its instance for shutdown management and handles SIGTERM by invoking the utility. New tests cover the utility and update route tests to assert delegation. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as /shutdown route
participant Shutdown as gracefulShutdown()
participant Server
participant Process
Client->>API: POST /shutdown
API->>Shutdown: gracefulShutdown()
Shutdown->>Server: server.close(callback)
Note over Shutdown: Start 10s force-exit timer
Server-->>Shutdown: callback(err?)
Shutdown->>Shutdown: clear timer
alt success
Shutdown->>Process: exit(0)
else error
Shutdown->>Process: exit(1)
end
sequenceDiagram
participant OS as OS
participant Node as Process
participant App as server.ts
participant Shutdown as gracefulShutdown()
participant Server
OS-->>Node: SIGTERM
Node->>App: emit SIGTERM
App->>Shutdown: gracefulShutdown(server)
Shutdown->>Server: server.close(callback)
Server-->>Shutdown: callback(err?)
Shutdown->>Node: exit(code)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
ryo8000
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @ryo8000, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request refactors the server shutdown mechanism to be more standardized and robust. It extracts the shutdown logic into a new utility module, allowing for a consistent and graceful termination process across different parts of the application, such as when a shutdown route is hit or a SIGTERM signal is received. This change improves the reliability and maintainability of the server's lifecycle management.
Highlights
- Centralized Shutdown Logic: The core server shutdown logic has been extracted from
src/server.tsinto a new, dedicated utility file atsrc/utils/shutdown.ts. This centralizes the logic, making it more maintainable and reusable. - New Graceful Shutdown Utility: A new
gracefulShutdownfunction has been introduced, which encapsulates the process of closing the HTTP server, handling potential errors, and implementing a timeout to force exit if the server doesn't close within a specified duration. - Standardized Shutdown Triggering: The
src/routes/shutdown.tsendpoint and the mainsrc/server.tsfile now consistently utilize the newgracefulShutdownutility, ensuring a standardized approach to server termination across the application. - Enhanced Test Coverage: Comprehensive unit tests have been added for the new
shutdownutility, covering various scenarios including successful shutdown, error conditions, and state management, significantly improving the robustness of the shutdown process.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Server bids adieu, Graceful exit, tasks complete, Code sleeps, peace descends.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request effectively standardizes the server shutdown process by centralizing the logic into a new shutdown utility. This is a great refactoring that improves code organization and reusability. The new unit tests for the shutdown utility are also comprehensive. I have a couple of minor suggestions to further improve the code's maintainability and test coverage.
| const forceExitTimeout = setTimeout(() => { | ||
| log.error('Server close timed out, forcing exit'); | ||
| process.exit(1); | ||
| }, 10_000); |
There was a problem hiding this comment.
The timeout value 10_000 is a magic number. It's better to extract it into a named constant at the top of the file (e.g., const SHUTDOWN_TIMEOUT_MS = 10_000;) to improve readability and make it easier to change in the future. This also makes it easier to configure if needed later.
| it('should warn and return early if shutdown already in progress', () => { | ||
| // Mock successful server close | ||
| mockServer.close.mockImplementation((callback) => { | ||
| if (callback) { | ||
| callback(); | ||
| } | ||
| return mockServer; | ||
| }); | ||
|
|
||
| // First call should work | ||
| expect(() => gracefulShutdown(mockServer)).toThrow('process.exit called'); | ||
|
|
||
| // Reset mocks to check second call | ||
| jest.clearAllMocks(); | ||
|
|
||
| // Second call should warn and return early | ||
| gracefulShutdown(mockServer); | ||
| expect(mockServer.close).not.toHaveBeenCalled(); | ||
| expect(mockExit).not.toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
This test case is missing an assertion to verify that a warning is logged when a shutdown is already in progress. Adding this check ensures that the logging behavior, which is important for diagnostics, is working as expected. The test has been made async to accommodate a dynamic import of the logger.
it('should warn and return early if shutdown already in progress', async () => {
// Mock successful server close
mockServer.close.mockImplementation((callback) => {
if (callback) {
callback();
}
return mockServer;
});
// First call should work
expect(() => gracefulShutdown(mockServer)).toThrow('process.exit called');
// Reset mocks to check second call
jest.clearAllMocks();
const { log } = await import('../../../src/logger.js');
// Second call should warn and return early
gracefulShutdown(mockServer);
expect(log.warn).toHaveBeenCalledWith('Shutdown already in progress...');
expect(mockServer.close).not.toHaveBeenCalled();
expect(mockExit).not.toHaveBeenCalled();
});![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
src/utils/shutdown.ts (3)
25-31: Avoid hard process exit when no server instance is availableCalling
process.exit(1)from a utility makes the whole process terminate even if the caller could recover or decide differently. Consider settingprocess.exitCodeand returning, or throwing, so the caller decides the exit strategy.Apply this minimal change to keep behavior observable without force-exiting:
if (!targetServer) { log.error('No server instance available for shutdown'); - process.exit(1); + process.exitCode = 1; + return; }
41-45: Make the force-exit timeout configurableHard-coding 10s is inflexible across environments. Allow an optional timeout to be passed (or sourced from config) so CI/dev/prod can tune it.
One way (non-breaking default):
-export function gracefulShutdown(server?: Server): void { +export function gracefulShutdown(server?: Server, opts: { timeoutMs?: number } = {}): void { const targetServer = server || serverInstance; @@ - const forceExitTimeout = setTimeout(() => { + const timeoutMs = opts.timeoutMs ?? 10_000; + const forceExitTimeout = setTimeout(() => { log.error('Server close timed out, forcing exit'); process.exit(1); - }, 10_000); + }, timeoutMs);Callers that care can pass
{ timeoutMs }; existing calls remain valid.
46-56: Consider separating process termination concerns from server shutdownEmbedding
process.exit(0|1)in the callback couples shutdown mechanics to process lifecycle. Exposing a Promise-based API (e.g.,gracefulShutdownAsync) that resolves with a success flag/code would let route handlers and signal handlers decide if/when to exit. Tests would also be simpler without mockingprocess.exit.If helpful, I can provide a Promise-returning variant and update call sites and tests.
src/routes/shutdown.ts (1)
18-20: Optionally defer shutdown to the next tick to ensure response flushesWhile the response is already sent, deferring can avoid any edge-case interleaving and keeps the handler fully synchronous.
- gracefulShutdown(); + // Defer to allow the response to flush + setImmediate(gracefulShutdown);src/server.ts (1)
17-20: Useoncefor signals and also handle SIGINTUsing
onceprevents accidental multiple registrations from triggering repeated shutdown attempts. Handling SIGINT improves local/dev UX.-process.on('SIGTERM', () => { - log.info('SIGTERM signal received'); - gracefulShutdown(server); -}); +process.once('SIGTERM', () => { + log.info('SIGTERM signal received'); + gracefulShutdown(server); +}); + +process.once('SIGINT', () => { + log.info('SIGINT signal received'); + gracefulShutdown(server); +});tests/ut/routes/shutdown.test.ts (2)
7-7: Type the mocked function asjest.Mock
jest.fn()returns ajest.Mock, not ajest.SpyInstance. This fixes typing and IDE hints.- let mockGracefulShutdown: jest.SpyInstance; + let mockGracefulShutdown: jest.Mock; @@ - mockGracefulShutdown = jest.fn(); + mockGracefulShutdown = jest.fn();Also applies to: 37-38
74-83: Strengthen assertion: also verify call countAsserting a single invocation tightens the contract for the route handler.
- expect(mockGracefulShutdown).toHaveBeenCalledWith(); + expect(mockGracefulShutdown).toHaveBeenCalledWith(); + expect(mockGracefulShutdown).toHaveBeenCalledTimes(1);tests/ut/utils/shutdown.test.ts (1)
33-39: Prefer fake timers over stubbing global timers for timeout path coverageUsing Jest fake timers gives you deterministic control and lets you test the forced-exit path (timeout firing) without manual stubs.
You can add a test like this (no need to change existing tests):
it('forces exit when close never calls back (timeout)', () => { jest.useFakeTimers(); // server.close never invokes callback mockServer.close.mockImplementation(() => mockServer); expect(() => gracefulShutdown(mockServer)).not.toThrow(); // no immediate exit // Fast-forward timers to trigger force exit expect(() => { jest.advanceTimersByTime(10_000); }).toThrow('process.exit called'); expect(mockExit).toHaveBeenCalledWith(1); jest.useRealTimers(); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
src/routes/shutdown.ts(2 hunks)src/server.ts(2 hunks)src/utils/shutdown.ts(1 hunks)tests/ut/routes/shutdown.test.ts(5 hunks)tests/ut/utils/shutdown.test.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
src/routes/shutdown.ts (1)
src/utils/shutdown.ts (1)
gracefulShutdown(25-57)
tests/ut/utils/shutdown.test.ts (1)
src/utils/shutdown.ts (3)
resetShutdownState(10-13)gracefulShutdown(25-57)setServerInstance(18-20)
src/utils/shutdown.ts (1)
src/logger.ts (1)
log(22-29)
src/server.ts (2)
src/utils/shutdown.ts (2)
setServerInstance(18-20)gracefulShutdown(25-57)src/logger.ts (1)
log(22-29)
🔇 Additional comments (4)
src/utils/shutdown.ts (1)
33-38: Good idempotency guard to prevent concurrent shutdownsEarly-return on in-progress shutdown avoids double-closing and repeated exits. This is a solid defensive check.
src/routes/shutdown.ts (1)
18-20: Delegation to centralized gracefulShutdown is correctResponding first and then delegating to
gracefulShutdown()keeps the route lean and reuses the shared shutdown flow.src/server.ts (1)
14-15: Registering the live server instance for shutdown is a good moveThis ensures routes and other code paths can initiate shutdown without threading the server everywhere.
tests/ut/utils/shutdown.test.ts (1)
105-124: Good validation of in-flight shutdown guardVerifying that the second call neither closes nor exits confirms the
isShuttingDownprotection works as intended.
1 participant
Summary by CodeRabbit