feat: support using process.on to listen SIGTERM #10
Conversation
There was a problem hiding this comment.
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
test/index.test.ts:86
- [nitpick] The test description 'should always listen sigterm work' is ambiguous and may confuse readers. Consider rephrasing it to 'should consistently handle SIGTERM signals' for clarity.
it('should always listen sigterm work', async () => {
test/index.test.ts:127
- [nitpick] The test name is duplicated in multiple describe blocks, which might lead to confusion when reading test outputs. Consider using unique, descriptive test names for different scenarios.
it('should always listen sigterm work', async () => {
commit: |
WalkthroughThe changes introduce an optional Changes
Sequence Diagram(s)sequenceDiagram
participant OS as OS/Signal
participant Process as Cluster Process
participant Worker as Worker Process
participant killWorkers as killWorkers Function
OS->>Process: Send SIGTERM
Process->>Process: Check ALWAYS_ON_SIGTERM env variable
alt ALWAYS_ON_SIGTERM set
Process->>killWorkers: Invoke to terminate workers
killWorkers->>Worker: Send termination signal
Process->>Process: Wait 50ms and re-trigger killWorkers
Process->>Process: Schedule exit after 2100ms
else Not set
Process->>Process: Wait 100ms and exit
end
sequenceDiagram
participant OS as OS/Signal
participant Master as Master Process
participant Child as Child Process
OS->>Master: Send SIGTERM
Master->>Child: Send SIGTERM to child
Master->>Master: Wait 50ms
Master->>Child: Send second SIGTERM
Master->>Master: Schedule exit after 1500ms
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
test/fixtures/child.cjsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-eggache". (The package "eslint-plugin-eggache" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-eggache" was referenced from the config file in ".eslintrc » eslint-config-egg/typescript » ./index.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. test/fixtures/master-sigterm.cjsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-eggache". (The package "eslint-plugin-eggache" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-eggache" was referenced from the config file in ".eslintrc » eslint-config-egg/typescript » ./index.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. test/fixtures/cluster.cjsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-eggache". (The package "eslint-plugin-eggache" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-eggache" was referenced from the config file in ".eslintrc » eslint-config-egg/typescript » ./index.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team.
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🧹 Nitpick comments (4)
test/fixtures/cluster.cjs (1)
18-33: Good extraction of worker termination logicExtracting the worker termination logic into a separate
killWorkersfunction improves code readability and maintainability. The conditional behavior based onALWAYS_ON_SIGTERMeffectively supports testing both SIGTERM handling modes.Consider extracting the timeout values as constants to make them more maintainable:
process.once('SIGTERM', () => { + const SECOND_SIGTERM_DELAY = 50; + const EXIT_DELAY_ONCE = 100; + const EXIT_DELAY_ALWAYS = 2100; + const killWorkers = () => { for (const id in cluster.workers) { cluster.workers[id].process.kill('SIGTERM'); } }; killWorkers(); if (process.env.ALWAYS_ON_SIGTERM) { - setTimeout(killWorkers, 50); - setTimeout(() => process.exit(0), 2100); + setTimeout(killWorkers, SECOND_SIGTERM_DELAY); + setTimeout(() => process.exit(0), EXIT_DELAY_ALWAYS); } else { setTimeout(() => { process.exit(0); - }, 100); + }, EXIT_DELAY_ONCE); } });src/index.ts (2)
13-13: Good addition of flexible SIGTERM handling optionAdding the
sigtermoption to theOptionsinterface provides users with more flexibility in handling SIGTERM signals.Consider adding a JSDoc comment to document this new option:
export interface Options { logger?: Logger; logLevel?: string; label?: string; timeout?: number; beforeExit?: BeforeExit; + /** + * Controls how SIGTERM signals are handled: + * - 'once': Process the first SIGTERM and ignore subsequent ones (default) + * - 'always': Process every SIGTERM, but only execute exit logic once + */ sigterm?: 'always' | 'once'; }
47-63: Well-implemented conditional SIGTERM handlingThe implementation for the new
sigtermoption is clean and robust:
- Uses
process.onwhensigtermis 'always' to handle multiple signals- Uses a
calledflag to track whether exit logic has been executed- Provides informative log messages for repeated signals
- Maintains backward compatibility with the default behavior
Consider making 'once' the explicit default to improve code clarity:
-if (options.sigterm === 'always') { +const sigtermMode = options.sigterm || 'once'; +if (sigtermMode === 'always') {test/index.test.ts (1)
86-106: Consider more comprehensive tests for the new SIGTERM handlingThe new tests verify that SIGTERM signals are received and processes exit gracefully after a delay, but there could be more scenarios to test.
Consider adding tests for the following scenarios:
- Testing with
sigterm: 'once'option (current tests only check'always'via environment variable)- Testing multiple SIGTERM signals with different timing between them
- Verifying behavior when SIGTERM is followed by another signal type
These additional tests would ensure the feature is robust under various conditions.
Also applies to: 127-147
🧰 Tools
🪛 GitHub Actions: CI
[error] 96-96: error TS2339: Property 'proc' does not exist on type 'Coffee'.
🛑 Comments failed to post (2)
test/index.test.ts (2)
127-147:
⚠️ Potential issueFix TypeScript type error
This test case contains the same TypeScript error as the previous one, causing pipeline failures.
Fix the TypeScript type error by adding a type annotation:
- const child = coffee.fork(startFile, [], { + const child: any = coffee.fork(startFile, [], {The test is well-structured and includes appropriate platform checks for Windows, which aligns with the existing test patterns.
📝 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.it('should always listen sigterm work', async () => { const startFile = path.join(fixtures, 'master-sigterm.js'); - const child = coffee.fork(startFile, [], { + const child: any = coffee.fork(startFile, [], { env: { ...process.env, ALWAYS_ON_SIGTERM: 'Y', }, }) .debug(); await sleep(waitStart); // the worker exit by graceful-process child.proc.kill('SIGTERM'); await sleep(2000); if (process.platform !== 'win32') { // windows can't handle SIGTERM signal child.expect('stdout', /\[test-child\] receive signal SIGTERM, exiting with code:0/); child.expect('stdout', /\[test-child\] receive signal SIGTERM again, waiting for exit/); child.expect('stdout', /exit after 1000ms/); child.expect('stdout', /\[test-child\] exit with code:0/); } });🧰 Tools
🪛 GitHub Check: Node.js / Test (macos-latest, 22)
[failure] 138-138:
Property 'proc' does not exist on type 'Coffee'.🪛 GitHub Check: Node.js / Test (ubuntu-latest, 22)
[failure] 138-138:
Property 'proc' does not exist on type 'Coffee'.🪛 GitHub Check: Node.js / Test (ubuntu-latest, 20)
[failure] 138-138:
Property 'proc' does not exist on type 'Coffee'.🪛 GitHub Check: Node.js / Test (macos-latest, 18)
[failure] 138-138:
Property 'proc' does not exist on type 'Coffee'.🪛 GitHub Check: Node.js / Test (ubuntu-latest, 18)
[failure] 138-138:
Property 'proc' does not exist on type 'Coffee'.🪛 GitHub Check: Node.js / Test (macos-latest, 20)
[failure] 138-138:
Property 'proc' does not exist on type 'Coffee'.🪛 GitHub Check: Node.js / Test (macos-latest, 18.19.0)
[failure] 138-138:
Property 'proc' does not exist on type 'Coffee'.🪛 GitHub Check: Node.js / Test (ubuntu-latest, 18.19.0)
[failure] 138-138:
Property 'proc' does not exist on type 'Coffee'.🪛 GitHub Actions: CI
[error] 138-138: error TS2339: Property 'proc' does not exist on type 'Coffee'.
86-106:
⚠️ Potential issueFix TypeScript type error and consider consistency in test setup
The test case is missing a type annotation for
child, causing the TypeScript error in the pipeline. Note also that this test uses.jsextension while other tests use.cjs.To fix the TypeScript error shown in pipeline failures, add a type annotation like in other tests:
- const child = coffee.fork(startFile, [], { + const child: any = coffee.fork(startFile, [], {Additionally, for consistency with the second test case, consider using the same sleep duration (2000ms instead of 2200ms). The current sleep (2200ms) seems to be chosen to accommodate the 1000ms exit delay, but it should be consistent across similar tests.
📝 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.it('should always listen sigterm work', async () => { const startFile = path.join(fixtures, 'cluster.js'); const child: any = coffee.fork(startFile, [], { env: { ...process.env, ALWAYS_ON_SIGTERM: 'Y', }, }) .debug(); await sleep(waitStart); child.proc.kill('SIGTERM'); await sleep(2200); if (process.platform !== 'win32') { // windows can't handle SIGTERM signal child.expect('stdout', /\[app-worker-\d\] receive signal SIGTERM, exiting with code:0/); child.expect('stdout', /\[app-worker-\d\] receive signal SIGTERM again, waiting for exit/); child.expect('stdout', /exit after 1000ms/); } child.expect('stdout', /\[app-worker-1\] exit with code:0/); child.expect('stdout', /\[app-worker-2\] exit with code:0/); });🧰 Tools
🪛 GitHub Actions: CI
[error] 96-96: error TS2339: Property 'proc' does not exist on type 'Coffee'.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
test/fixtures/cluster.cjs (2)
17-34: Improved SIGTERM handling with conditional behavior.The new implementation properly segregates worker termination logic into a reusable function and adds support for different termination behaviors based on the
ALWAYS_ON_SIGTERMenvironment variable. The code structure is clean and logical.However, I have a few concerns:
- The timeout values (50ms, 2100ms, 100ms) lack documentation explaining their significance.
- There's potential for a race condition if workers take longer than 2100ms to shut down.
Consider adding comments explaining the timeout values and their purpose. You might also want to implement a check to ensure all workers have terminated before calling
process.exit().killWorkers(); if (process.env.ALWAYS_ON_SIGTERM) { + // Call killWorkers again after a short delay to handle any newly forked workers setTimeout(killWorkers, 50); + // Allow sufficient time for workers to gracefully shut down before forcing exit setTimeout(() => process.exit(0), 2100); } else { + // Single SIGTERM mode: exit after a brief delay setTimeout(() => { process.exit(0); }, 100);
48-59: Well-structured options object with conditional configuration.The implementation cleanly encapsulates configuration in an options object and conditionally adds SIGTERM-specific settings based on the environment variable. The
beforeExithandler provides a useful delay for testing purposes.However, a minor improvement could be made to the code structure:
Consider defining the options object more concisely by conditionally spreading additional properties:
const options = { label: 'app-worker-' + cluster.worker.id, logLevel: process.env.NODE_LOG_LEVEL, + ...(process.env.ALWAYS_ON_SIGTERM ? { + sigterm: 'always', + beforeExit: async () => { + await new Promise(r => setTimeout(r, 1000)); + console.log('exit after 1000ms'); + } + } : {}) }; - if (process.env.ALWAYS_ON_SIGTERM) { - options.sigterm = 'always'; - options.beforeExit = async () => { - await new Promise(r => setTimeout(r, 1000)); - console.log('exit after 1000ms'); - }; - } graceful(options);This approach makes it clearer that these properties are conditionally included as a group.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/index.ts(2 hunks)test/fixtures/child.cjs(1 hunks)test/fixtures/cluster.cjs(2 hunks)test/fixtures/master-sigterm.cjs.js(1 hunks)test/index.test.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- test/fixtures/master-sigterm.cjs.js
- test/fixtures/child.cjs
- src/index.ts
- test/index.test.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
test/fixtures/cluster.cjs (1)
26-33: Be explicit about environment variable check.The current check only tests if the environment variable exists, but doesn't validate its value. Consider using a more explicit check to ensure expected behavior.
- if (process.env.ALWAYS_ON_SIGTERM) { + if (process.env.ALWAYS_ON_SIGTERM === 'Y' || process.env.ALWAYS_ON_SIGTERM === 'true') {test/index.test.ts (1)
127-136: Consider adding a comment about test purpose.Adding a brief comment about what specifically this test is verifying compared to other SIGTERM tests would improve readability.
- it('should always listen sigterm work', async () => { + it('should always listen sigterm work in child_process fork mode', async () => { + // This test verifies that the child process properly handles SIGTERM signals + // when the sigterm='always' option is enabled in fork mode🧰 Tools
🪛 GitHub Actions: CI
[error] 133-133: AssertionError [ERR_ASSERTION]: should match stdout expected
/\[test-child\] receive signal SIGTERM, exiting with code:0/(RegExp)but actualmaster fork /home/runner/work/graceful-process/graceful-process/test/fixtures/child.js done\n(String)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/index.ts(2 hunks)test/fixtures/child.cjs(1 hunks)test/fixtures/cluster.cjs(2 hunks)test/fixtures/master-sigterm.cjs(1 hunks)test/index.test.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- test/fixtures/child.cjs
- src/index.ts
🧰 Additional context used
🧬 Code Definitions (1)
test/index.test.ts (1)
test/fixtures/master-spawn.js (1)
path(3-3)
🪛 GitHub Actions: CI
test/index.test.ts
[error] 133-133: AssertionError [ERR_ASSERTION]: should match stdout expected /\[test-child\] receive signal SIGTERM, exiting with code:0/(RegExp) but actual master fork /home/runner/work/graceful-process/graceful-process/test/fixtures/child.js done\n(String)
🔇 Additional comments (5)
test/fixtures/master-sigterm.cjs (1)
1-14: Simple implementation for SIGTERM testing.The script correctly sets up a parent-child process hierarchy with SIGTERM signal handling, using a one-time listener for the SIGTERM signal.
test/fixtures/cluster.cjs (2)
18-22: Good refactoring of worker termination logic.Extracting the worker termination logic into a
killWorkersfunction improves code organization and allows for reuse when sending multiple SIGTERM signals.
48-59: Good use of options object for configuration.The implementation cleanly handles the conditional configuration of the
gracefulfunction based on environment variables.test/index.test.ts (2)
86-106: Well-structured test for cluster mode SIGTERM handling.The test adequately verifies the behavior of worker processes when the master receives a SIGTERM signal in the 'always' mode.
127-147:❓ Verification inconclusive
Test is failing in CI pipeline.
The test expects specific output that isn't being produced. The pipeline error shows a mismatch between expected and actual output.
- if (process.platform !== 'win32') { - // windows can't handle SIGTERM signal - child.expect('stdout', /\[test-child\] receive signal SIGTERM, exiting with code:0/); - child.expect('stdout', /\[test-child\] receive signal SIGTERM again, waiting for exit/); - child.expect('stdout', /exit after 1000ms/); - child.expect('stdout', /\[test-child\] exit with code:0/); - } + // Ensure test works on all platforms + // Check for master process output first + child.expect('stdout', /master fork .* done/); + + if (process.platform !== 'win32') { + // Then check for child process output on non-Windows platforms + child.expect('stdout', /\[test-child\] receive signal SIGTERM, exiting with code:0/); + child.expect('stdout', /\[test-child\] receive signal SIGTERM again, waiting for exit/); + child.expect('stdout', /exit after 1000ms/); + child.expect('stdout', /\[test-child\] exit with code:0/); + }
🏁 Script executed:
#!/bin/bash # Examine the test fixture to understand what might be causing the issue echo "Checking the imported test-child module in master-sigterm.cjs" cat test/fixtures/child.js # Check if the expected log messages are in the code echo "Searching for the expected log patterns in the codebase" rg -F "[test-child] receive signal SIGTERM, exiting with code:0"Length of output: 404
Action Required: Verify Test Logging and Fixture Files
- The CI failure appears to stem from the test’s expected output not matching what’s produced. The revised diff now additionally expects a "master fork … done" message before the child process logs.
- It also seems that the file referenced for log verification (i.e.
test/fixtures/child.js) does not exist. Please confirm whether the missing file is intentional (for example, if its responsibilities have been merged intomaster-sigterm.cjs) or if the test expectations need further adjustments.- Ensure that on non‑Windows platforms the child process still emits the expected logs:
[test-child] receive signal SIGTERM, exiting with code:0[test-child] receive signal SIGTERM again, waiting for exitexit after 1000ms[test-child] exit with code:0Please verify these points manually in the CI environment and adjust the test fixture or expected logs as necessary.
🧰 Tools
🪛 GitHub Actions: CI
[error] 133-133: AssertionError [ERR_ASSERTION]: should match stdout expected
/\[test-child\] receive signal SIGTERM, exiting with code:0/(RegExp)but actualmaster fork /home/runner/work/graceful-process/graceful-process/test/fixtures/child.js done\n(String)
| child.kill('SIGTERM'); | ||
| setTimeout(() => child.kill('SIGTERM'), 50); | ||
| setTimeout(() => process.exit(0), 100); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider passing the ALWAYS_ON_SIGTERM environment variable to the child process.
Since the test in index.test.ts sets this environment variable and the cluster implementation uses it, this script should probably propagate it to ensure consistent behavior.
-const child = childProcess.fork(childFile);
+const child = childProcess.fork(childFile, [], {
+ env: {
+ ...process.env,
+ ALWAYS_ON_SIGTERM: process.env.ALWAYS_ON_SIGTERM,
+ },
+});Committable suggestion skipped: line range outside the PR's diff.
| process.once('SIGTERM', () => { | ||
| child.kill('SIGTERM'); | ||
| setTimeout(() => child.kill('SIGTERM'), 50); | ||
| setTimeout(() => process.exit(0), 100); | ||
| }); |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Pipeline failure indicates an issue with expected test output.
The test in index.test.ts (line 143) expects the output to include [test-child] receive signal SIGTERM, exiting with code:0, but the actual output only shows master fork /home/runner/work/graceful-process/graceful-process/test/fixtures/child.js done.
This suggests that either:
- The child process isn't properly configured to handle SIGTERM signals with the expected output
- The child.js script isn't reporting the expected message format
🏁 Script executed:
#!/bin/bash
# Check the content of the child.js script to see if it has proper SIGTERM handling
cat test/fixtures/child.js
# Check if there are other child.js implementations that might be referenced
find test/fixtures -name "child*.js" -type fLength of output: 130
Action Required: Verify and Correct Child Process Handling
The pipeline failure indicates that the expected SIGTERM signal handling output from the child process is missing. The test in index.test.ts (line 143) expects to see:
[test-child] receive signal SIGTERM, exiting with code:0
but only shows:
master fork /home/runner/work/graceful-process/graceful-process/test/fixtures/child.js done
Our verification revealed that the file test/fixtures/child.js does not exist in the repository. This suggests that either the child process handler is missing or misnamed, or the SIGTERM handling logic isn’t implemented as expected.
Please verify whether:
- The child process should be handled by a file (perhaps under a different name or location) that output the expected message.
- The test expectation should be adjusted based on the current implementation.
Once confirmed, update the child process setup and/or test expectations accordingly.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test/fixtures/master-sigterm.cjs (1)
7-7: 🛠️ Refactor suggestionConsider passing environment variables to the child process.
The ALWAYS_ON_SIGTERM environment variable is being set in the test but isn't being passed to the child process here, which could lead to inconsistent behavior between master and child processes.
-const child = childProcess.fork(childFile); +const child = childProcess.fork(childFile, [], { + env: { + ...process.env, + ALWAYS_ON_SIGTERM: process.env.ALWAYS_ON_SIGTERM, + }, +});
🧹 Nitpick comments (3)
test/fixtures/master-sigterm.cjs (1)
1-16: Add error handling for the child process.The script doesn't handle potential errors when forking the child process or if the child process exits unexpectedly.
/* eslint-disable @typescript-eslint/no-var-requires */ const childProcess = require('node:child_process'); const path = require('node:path'); const childFile = path.join(__dirname, 'child.cjs'); const child = childProcess.fork(childFile); + +child.on('error', (err) => { + console.error('Child process error:', err); + process.exit(1); +}); + +child.on('exit', (code, signal) => { + console.log(`Child process exited with code ${code} and signal ${signal}`); +}); process.once('SIGTERM', () => { child.kill('SIGTERM'); setTimeout(() => child.kill('SIGTERM'), 50); setTimeout(() => process.exit(0), 1500); }); console.log('master fork %s done', childFile);test/index.test.ts (2)
127-147: Consider using a distinct test name.The test name is identical to the previous test at line 86, which could lead to confusion. Consider making the test name more specific to indicate it's testing the child_process.fork context.
- it('should always listen sigterm work', async () => { + it('should always listen sigterm work in child_process.fork mode', async () => {
140-146: Add assertions for Windows platform.The test conditionally checks behavior for non-Windows platforms but doesn't have any assertions for Windows. Consider adding a platform-specific assertion for Windows or at least a comment explaining what's expected on Windows.
if (process.platform !== 'win32') { // windows can't handle SIGTERM signal child.expect('stdout', /\[test-child\] receive signal SIGTERM, exiting with code:0/); child.expect('stdout', /\[test-child\] receive signal SIGTERM again, waiting for exit/); child.expect('stdout', /exit after 1000ms/); child.expect('stdout', /\[test-child\] exit with code:0/); + } else { + // On Windows, verify the process exits normally even though SIGTERM isn't handled + child.expect('stdout', /\[test-child\] exit with code:0/); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/index.ts(2 hunks)test/fixtures/child.cjs(1 hunks)test/fixtures/cluster.cjs(2 hunks)test/fixtures/master-sigterm.cjs(1 hunks)test/index.test.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- test/fixtures/child.cjs
- test/fixtures/cluster.cjs
- src/index.ts
🧰 Additional context used
🧬 Code Definitions (1)
test/index.test.ts (1)
test/fixtures/master-spawn.js (1)
path(3-3)
🔇 Additional comments (2)
test/index.test.ts (2)
86-106: Test case has the correct implementation.The test correctly verifies the "always listen sigterm" behavior by checking for specific log messages when SIGTERM is received multiple times. The platform-specific check is appropriate since Windows handles signals differently.
127-139:✅ Verification successful
Verify that child.cjs exists.
The test relies on
master-sigterm.cjswhich forkschild.cjs. Let's ensure this file exists and properly handles SIGTERM signals with the expected output format.
🏁 Script executed:
#!/bin/bash # Check if child.cjs exists and contains proper SIGTERM handling echo "Checking if child.cjs exists in the fixtures directory:" ls -la test/fixtures/child.cjs || echo "child.cjs not found" # If it exists, check its content for SIGTERM handling if [ -f test/fixtures/child.cjs ]; then echo "Checking child.cjs content for SIGTERM handling:" grep -A 10 "SIGTERM" test/fixtures/child.cjs else # List all files in fixtures to see what might be used instead echo "Listing all files in fixtures directory:" ls -la test/fixtures/ fiLength of output: 669
SIGTERM handling verified in child.cjs
The verification confirms that
test/fixtures/child.cjsexists and correctly implements SIGTERM handling by settingoptions.sigtermto"always"and attaching abeforeExithandler that logs"exit after 1000ms". No further changes are necessary.
| process.once('SIGTERM', () => { | ||
| child.kill('SIGTERM'); | ||
| setTimeout(() => child.kill('SIGTERM'), 50); | ||
| setTimeout(() => process.exit(0), 1500); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using persistent SIGTERM listener when ALWAYS_ON_SIGTERM is set.
Since this PR adds support for always listening to SIGTERM, this test fixture should demonstrate that behavior when the environment variable is set.
-process.once('SIGTERM', () => {
+const sigtermHandler = () => {
child.kill('SIGTERM');
setTimeout(() => child.kill('SIGTERM'), 50);
setTimeout(() => process.exit(0), 1500);
-});
+};
+
+if (process.env.ALWAYS_ON_SIGTERM === 'Y') {
+ process.on('SIGTERM', sigtermHandler);
+} else {
+ process.once('SIGTERM', sigtermHandler);
+}📝 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.
| process.once('SIGTERM', () => { | |
| child.kill('SIGTERM'); | |
| setTimeout(() => child.kill('SIGTERM'), 50); | |
| setTimeout(() => process.exit(0), 1500); | |
| }); | |
| const sigtermHandler = () => { | |
| child.kill('SIGTERM'); | |
| setTimeout(() => child.kill('SIGTERM'), 50); | |
| setTimeout(() => process.exit(0), 1500); | |
| }; | |
| if (process.env.ALWAYS_ON_SIGTERM === 'Y') { | |
| process.on('SIGTERM', sigtermHandler); | |
| } else { | |
| process.once('SIGTERM', sigtermHandler); | |
| } |
[skip ci] ## [2.1.0](v2.0.0...v2.1.0) (2025-04-08) ### Features * support using process.on to listen SIGTERM ([#10](#10)) ([b3d3626](b3d3626))
Summary by CodeRabbit