Skip to content
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

fix(core): Handle SIGTERM for graceful shutdowns #16882

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

Conversation

AndesKrrrrrrrrrrr
Copy link
Member

@AndesKrrrrrrrrrrr AndesKrrrrrrrrrrr commented Nov 14, 2024

Stop forceful shutdowns in-cluster.

See https://expressjs.com/en/advanced/healthcheck-graceful-shutdown.html

Nest.js documentation has a section explicitly about this issue:

  • For these events, if you're not calling app.close() explicitly, you must opt-in to make them work with system signals such as SIGTERM. See Application shutdown below.

Summary by CodeRabbit

  • New Features
    • Introduced a graceful exit handling mechanism for the server, improving robustness during shutdown scenarios.
    • Added a utility function for managing server shutdown processes in response to termination signals.
    • Implemented logging for shutdown events to enhance visibility during server exits.
  • Bug Fixes
    • Improved error handling during server closure to ensure proper logging and exit status.
  • Refactor
    • Updated function signatures in route handlers to clarify unused parameters, improving code readability.

Copy link
Contributor

coderabbitai bot commented Nov 14, 2024

Warning

Rate limit exceeded

@andes-it has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 17 minutes and 47 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 4974ecb and 1ec4c5a.

📒 Files selected for processing (22)
  • apps/auth-admin-web/components/Admin/nav/AdminTabNav.tsx (1 hunks)
  • apps/auth-admin-web/components/Client/nav/ClientTabNav.tsx (1 hunks)
  • apps/contentful-apps/components/Layout.tsx (1 hunks)
  • apps/contentful-apps/components/LocalhostWarning.tsx (1 hunks)
  • apps/contentful-apps/components/MideindTranslationSidebar/fieldUtils/index.tsx (1 hunks)
  • apps/contentful-apps/pages/fields/active-translations-field.tsx (1 hunks)
  • apps/contentful-apps/pages/fields/admin-only-json-field.tsx (1 hunks)
  • apps/contentful-apps/pages/fields/admin-only-short-text-checkbox-list-field.tsx (1 hunks)
  • apps/contentful-apps/pages/fields/admin-only-short-text-dropdown-field.tsx (1 hunks)
  • apps/contentful-apps/pages/fields/admin-only-short-text-field.tsx (1 hunks)
  • apps/contentful-apps/pages/fields/admin-only-single-reference-field.tsx (1 hunks)
  • apps/contentful-apps/pages/fields/article-related-articles-field.tsx (2 hunks)
  • apps/contentful-apps/pages/fields/article-subarticles-field.tsx (1 hunks)
  • apps/contentful-apps/pages/fields/content-type-notifications-field.tsx (1 hunks)
  • apps/contentful-apps/pages/fields/embed-link-field.tsx (1 hunks)
  • apps/contentful-apps/pages/fields/form-default-namespace-field.tsx (1 hunks)
  • apps/contentful-apps/pages/fields/formatted-location-input-field.tsx (1 hunks)
  • apps/contentful-apps/pages/fields/publish-date-field.tsx (1 hunks)
  • apps/contentful-apps/pages/fields/theme-properties-field.tsx (1 hunks)
  • apps/contentful-apps/pages/fields/time-duration-field.tsx (1 hunks)
  • apps/contentful-apps/pages/fields/vacancy-contacts-field.tsx (1 hunks)
  • apps/financial-aid/backend/src/app/modules/staff/test/numberOfUsersForMunicipality.spec.ts (1 hunks)

Walkthrough

The pull request introduces a new function, setupShutdownHooks, to manage graceful server termination by listening for specific signals (SIGHUP, SIGINT, SIGTERM). This function is imported and utilized in various server files to enhance shutdown handling. Additionally, several route handler and middleware function signatures are updated to indicate unused parameters by prefixing them with an underscore. These changes improve the server's shutdown capabilities and clarify code intent regarding unused parameters.

Changes

File Path Change Summary
libs/infra-express-server/src/lib/infra-express-server.ts - Added import for setupShutdownHooks.
- Updated function signatures to use _req and _next for unused parameters in route handlers and middleware.
- Called setupShutdownHooks(server) in runServer.
libs/infra-next-server/src/lib/bootstrap.ts - Added import for setupShutdownHooks.
- Captured server instance in startServer and passed it to setupShutdownHooks.
- Removed setupExitHook function.
libs/infra-nest-server/src/lib/bootstrap.ts - Added app.enableShutdownHooks() in createApp for graceful shutdown handling.
libs/node/utils/.eslintrc.json - Added new ESLint configuration file extending base settings with ignore patterns.
libs/node/utils/README.md - Documented setupShutdownHooks function and its parameters.
libs/node/utils/jest.config.ts - Added Jest configuration for the node-utils library.
libs/node/utils/project.json - Introduced project configuration for the node-utils library with targets for linting and testing.
libs/node/utils/src/index.ts - Re-exported all entities from ./lib/setupShutdownHooks.
libs/node/utils/src/lib/setupShutdownHooks.spec.ts - Added test suite for setupShutdownHooks function using Jest.
libs/node/utils/src/lib/setupShutdownHooks.ts - Introduced setupShutdownHooks function to manage server shutdown processes.
libs/node/utils/tsconfig.json - Added new TypeScript configuration file extending from base configuration.
libs/node/utils/tsconfig.lib.json - Added new TypeScript configuration file for library compilation settings.
libs/node/utils/tsconfig.spec.json - Added new TypeScript configuration file for testing specifications.
tsconfig.base.json - Added path mapping for @island.is/node-utils to libs/node/utils/src/index.ts.

Possibly related PRs

Suggested reviewers

  • lodmfjord
  • thorkellmani
  • baering

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

codecov bot commented Nov 14, 2024

Codecov Report

Attention: Patch coverage is 39.13043% with 14 lines in your changes missing coverage. Please review.

Project coverage is 35.32%. Comparing base (3a496da) to head (1ec4c5a).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
...-ui/core/src/lib/DatePicker/DatePicker.stories.tsx 0.00% 9 Missing ⚠️
...app/modules/samgongustofa/samgongustofa.service.ts 50.00% 4 Missing ⚠️
...st-server/src/lib/httpRequestDurationMiddleware.ts 0.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #16882      +/-   ##
==========================================
- Coverage   35.57%   35.32%   -0.25%     
==========================================
  Files        7030     7029       -1     
  Lines      150490   151974    +1484     
  Branches    42974    43571     +597     
==========================================
+ Hits        53536    53687     +151     
- Misses      96954    98287    +1333     
Flag Coverage Δ
air-discount-scheme-backend 48.14% <0.00%> (-0.02%) ⬇️
air-discount-scheme-web 0.00% <ø> (ø)
application-system-api 38.68% <50.00%> (-0.01%) ⬇️
application-templates-new-primary-school ?
auth-admin-web 2.43% <ø> (ø)
auth-nest-tools 31.69% <ø> (ø)
auth-shared 75.00% <ø> (ø)
clients-charge-fjs-v2 28.88% <ø> (ø)
clients-driving-license 40.68% <ø> (ø)
clients-driving-license-book 43.75% <ø> (ø)
clients-financial-statements-inao 49.48% <ø> (ø)
clients-license-client 1.26% <ø> (ø)
clients-middlewares 73.39% <ø> (+0.32%) ⬆️
clients-regulations 42.75% <ø> (ø)
clients-rsk-company-registry 31.18% <ø> (ø)
clients-rsk-personal-tax-return 38.32% <ø> (ø)
clients-smartsolutions 12.77% <ø> (ø)
clients-syslumenn 49.18% <ø> (ø)
clients-zendesk 49.88% <ø> (ø)
cms 0.39% <ø> (ø)
cms-translations 38.81% <ø> (ø)
content-search-index-manager 95.65% <ø> (ø)
content-search-toolkit 8.16% <ø> (ø)
contentful-apps 4.56% <ø> (ø)
dokobit-signing 61.66% <ø> (ø)
email-service 59.68% <ø> (ø)
feature-flags 90.40% <ø> (ø)
file-storage 45.32% <ø> (ø)
icelandic-names-registry-backend 54.40% <0.00%> (-0.05%) ⬇️
infra-nest-server 48.13% <50.00%> (+0.06%) ⬆️
infra-tracing 69.94% <ø> (ø)
island-ui-core 30.32% <18.18%> (ø)
judicial-system-api 20.07% <ø> (ø)
judicial-system-audit-trail 68.53% <ø> (ø)
judicial-system-formatters 78.86% <ø> (ø)
judicial-system-message 66.29% <ø> (ø)
judicial-system-message-handler 47.89% <ø> (ø)
judicial-system-scheduler 71.24% <ø> (ø)
license-api 42.90% <0.00%> (-0.09%) ⬇️
localization 10.15% <ø> (ø)
logging 58.02% <ø> (ø)
message-queue 67.05% <ø> (-0.71%) ⬇️
nest-audit 65.78% <ø> (ø)
nest-aws 51.93% <ø> (ø)
nest-config 76.05% <ø> (ø)
nest-core 53.16% <ø> (ø)
nest-feature-flags 50.69% <ø> (ø)
nest-problem 45.60% <0.00%> (-0.05%) ⬇️
nest-swagger 51.13% <ø> (ø)
nova-sms 60.96% <ø> (ø)
services-auth-delegation-api 58.37% <0.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...uth-admin-web/components/Admin/nav/AdminTabNav.tsx 0.00% <ø> (ø)
...h-admin-web/components/Client/nav/ClientTabNav.tsx 0.00% <ø> (ø)
apps/contentful-apps/components/Layout.tsx 0.00% <ø> (ø)
...ps/contentful-apps/components/LocalhostWarning.tsx 0.00% <ø> (ø)
...nts/MideindTranslationSidebar/fieldUtils/index.tsx 0.00% <ø> (ø)
...ul-apps/pages/fields/active-translations-field.tsx 0.00% <ø> (ø)
...entful-apps/pages/fields/admin-only-json-field.tsx 0.00% <ø> (ø)
...elds/admin-only-short-text-checkbox-list-field.tsx 0.00% <ø> (ø)
...es/fields/admin-only-short-text-dropdown-field.tsx 0.00% <ø> (ø)
...-apps/pages/fields/admin-only-short-text-field.tsx 0.00% <ø> (ø)
... and 21 more

... and 606 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3a496da...1ec4c5a. Read the comment docs.

@datadog-island-is
Copy link

datadog-island-is bot commented Nov 14, 2024

Datadog Report

All test runs 65dd19e 🔗

10 Total Test Services: 0 Failed, 10 Passed
🔻 Test Sessions change in coverage: 4 decreased, 3 increased, 192 no change

Test Services
This report shows up to 10 services
Service Name Failed Known Flaky New Flaky Passed Skipped Total Time Code Coverage Change Test Service View
air-discount-scheme-backend 0 0 0 63 0 23.31s 1 decreased (-0.01%) Link
air-discount-scheme-web 0 0 0 2 0 7.89s N/A Link
application-system-api 0 0 0 46 0 2m 26.5s 1 no change Link
auth-admin-web 0 0 0 1 0 19.32s N/A Link
auth-api-lib 0 0 0 20 0 26.68s N/A Link
auth-nest-tools 0 0 0 6 0 17.07s N/A Link
auth-shared 0 0 0 4 0 5.49s N/A Link
clients-charge-fjs-v2 0 0 0 1 0 21.09s 1 no change Link
clients-driving-license 0 0 0 9 0 47.68s 1 no change Link
clients-driving-license-book 0 0 0 5 0 38.59s 1 no change Link

🔻 Code Coverage Decreases vs Default Branch (4)

  • message-queue - jest 67.46% (-0.8%) - Details
  • nest-problem - jest 50.11% (-0.06%) - Details
  • icelandic-names-registry-backend - jest 59.75% (-0.05%) - Details
  • air-discount-scheme-backend - jest 47.83% (-0.01%) - Details

@AndesKrrrrrrrrrrr AndesKrrrrrrrrrrr changed the title fix(infra): Handle SIGTERM for graceful shutdowns fix(core): Handle SIGTERM for graceful shutdowns Nov 14, 2024
@AndesKrrrrrrrrrrr AndesKrrrrrrrrrrr added the deprecated:automerge (Disabled) Merge this PR as soon as all checks pass label Nov 28, 2024
@AndesKrrrrrrrrrrr AndesKrrrrrrrrrrr marked this pull request as ready for review November 28, 2024 09:27
@AndesKrrrrrrrrrrr AndesKrrrrrrrrrrr requested a review from a team as a code owner November 28, 2024 09:27
Copy link
Member

@eirikurn eirikurn left a comment

Choose a reason for hiding this comment

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

I was checking the other day and I'm not sure if any of our servers are using infra-express-server. It might have been a blueprint which went unused.

I also think this SIGTERM handling is worse then the current SIGKILL fallback. There's nothing graceful about process.exit(0). Afaik it will just abruptly exit the process and close all open connections, resulting in socket issues.

Currently at least there's a chance that current requests finish and the load balancer stops sending new requests to the pod before its eventual SIGKILL. 🤔

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (11)
libs/node/utils/src/lib/setupShutdownHooks.ts (3)

8-14: Consider adding SIGQUIT signal handler

While the current signal handling covers the most common scenarios, consider adding SIGQUIT for complete signal coverage, especially in Unix-like environments.

-  const terminationEvents = ['SIGHUP', 'SIGINT', 'SIGTERM']
+  const terminationEvents = ['SIGHUP', 'SIGINT', 'SIGTERM', 'SIGQUIT']

37-39: Consider cleanup of signal handlers

While the current implementation works, it's good practice to clean up signal handlers when the server closes to prevent memory leaks in long-running applications that might restart the server.

+  const cleanupListeners = () => {
+    for (const signal of terminationEvents) {
+      process.removeListener(signal, () => shutdown(signal))
+    }
+  }
+
   for (const signal of terminationEvents) {
     process.on(signal, () => shutdown(signal))
   }
+
+  server.on('close', cleanupListeners)

1-40: Add comprehensive documentation and testing guidelines

As this is a critical utility function in the shared library, consider:

  1. Adding JSDoc documentation with examples
  2. Creating integration tests with different signal scenarios
  3. Documenting recommended usage patterns in the README

This will help other developers properly integrate the shutdown hooks in their services.

libs/node/utils/README.md (3)

9-10: Improve parameter documentation clarity

The onShutdown parameter description should be restructured for better clarity.

-- `onShutdown` (Function): Optional callback function that will be executed before the server shuts down. Can be used to perform and wait for other cleanup tasks before the server shuts down.
++ `onShutdown` (Function): Optional callback function that will be executed before the server shuts down. It can be used to perform and wait for other cleanup tasks.
🧰 Tools
🪛 LanguageTool

[style] ~10-~10: To form a complete sentence, be sure to include a subject.
Context: ... executed before the server shuts down. Can be used to perform and wait for other c...

(MISSING_IT_THERE)


16-20: Add error handling details to behavior documentation

The behavior section should include information about error handling scenarios and potential error types.

Consider adding the following details:

  • Types of errors that might occur during shutdown
  • How errors are propagated
  • Whether the onShutdown callback errors are caught and handled

22-24: Enhance testing documentation

The testing section would benefit from examples of specific test cases that users should implement.

Consider adding:

  • Test scenarios for different termination signals
  • Examples of mocking the server and shutdown process
  • Test cases for error conditions
  • Examples of testing the onShutdown callback functionality
libs/infra-express-server/src/lib/infra-express-server.ts (1)

69-72: Consider enhancing the error handler implementation

While the parameter naming is correct, the error handler could benefit from improvements:

+interface ApiError extends Error {
+  status?: number;
+}

 app.use(function errorHandler(
-  err: any,
+  err: ApiError,
   _req: Request,
   res: Response,
   _next: NextFunction,
 ) {
   logger.error(`Status code: ${err.status}, msg: ${err.message}`)
   res.status(err.status || 500)
-  res.send(err.message)
+  res.json({
+    error: {
+      message: err.message,
+      status: err.status || 500
+    }
+  })
 }
libs/infra-nest-server/src/lib/bootstrap.ts (1)

49-51: Consider documenting shutdown behavior with keep-alive connections.

While the implementation is correct, the interaction between the 30-second KEEP_ALIVE_TIMEOUT and shutdown signals could affect shutdown timing. Long-lived connections might delay the shutdown process.

Consider:

  1. Adding a comment documenting this interaction
  2. Implementing a maximum shutdown timeout
  3. Logging when shutdown is initiated and completed

Here's a suggested comment addition:

  // Handle signals like SIGINT, SIGTERM and SIGHUP gracefully.
+ // Note: Active keep-alive connections (timeout: 30s) may delay shutdown
  app.enableShutdownHooks()

Also applies to: 18-19

libs/node/utils/src/lib/setupShutdownHooks.spec.ts (3)

18-37: Enhance type safety in test setup.

While the test setup is comprehensive, consider improving type safety:

-    server = {
-      close: jest.fn((callback) => callback()),
-    } as unknown as Server
+    server = {
+      close: jest.fn((callback?: (err?: Error) => void) => {
+        if (callback) callback();
+        return server;
+      }),
+    } as Partial<Server> as Server

39-86: Consider extracting common test patterns.

The signal handler extraction is repeated across multiple tests. Consider creating a helper function:

const getSignalHandler = (signal: string, spy: jest.SpyInstance): Function => {
  return spy.mock.calls.find(([event]) => event === signal)[1];
};

88-151: Comprehensive error handling test coverage.

The test cases thoroughly cover error scenarios and callback handling. Great job on including both successful and failure paths for the onShutdown callback.

However, consider adding one more test case:

it('should handle multiple signals in sequence', async () => {
  setupShutdownHooks(server);
  const sigTermHandler = getSignalHandler('SIGTERM', processOnSpy);
  const sigIntHandler = getSignalHandler('SIGINT', processOnSpy);
  
  await sigTermHandler();
  await sigIntHandler();
  
  expect(server.close).toHaveBeenCalledTimes(1); // Should only close once
  expect(processExitSpy).toHaveBeenCalledTimes(1);
});
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 75bdd29 and fdd8a2f.

📒 Files selected for processing (14)
  • libs/infra-express-server/src/lib/infra-express-server.ts (5 hunks)
  • libs/infra-nest-server/src/lib/bootstrap.ts (1 hunks)
  • libs/infra-next-server/src/lib/bootstrap.ts (2 hunks)
  • libs/node/utils/.eslintrc.json (1 hunks)
  • libs/node/utils/README.md (1 hunks)
  • libs/node/utils/jest.config.ts (1 hunks)
  • libs/node/utils/project.json (1 hunks)
  • libs/node/utils/src/index.ts (1 hunks)
  • libs/node/utils/src/lib/setupShutdownHooks.spec.ts (1 hunks)
  • libs/node/utils/src/lib/setupShutdownHooks.ts (1 hunks)
  • libs/node/utils/tsconfig.json (1 hunks)
  • libs/node/utils/tsconfig.lib.json (1 hunks)
  • libs/node/utils/tsconfig.spec.json (1 hunks)
  • tsconfig.base.json (1 hunks)
✅ Files skipped from review due to trivial changes (7)
  • libs/node/utils/.eslintrc.json
  • libs/node/utils/jest.config.ts
  • libs/node/utils/project.json
  • libs/node/utils/src/index.ts
  • libs/node/utils/tsconfig.json
  • libs/node/utils/tsconfig.lib.json
  • libs/node/utils/tsconfig.spec.json
🚧 Files skipped from review as they are similar to previous changes (1)
  • libs/infra-next-server/src/lib/bootstrap.ts
🧰 Additional context used
📓 Path-based instructions (5)
libs/infra-express-server/src/lib/infra-express-server.ts (1)

Pattern libs/**/*: "Confirm that the code adheres to the following:

  • Reusability of components and hooks across different NextJS apps.
  • TypeScript usage for defining props and exporting types.
  • Effective tree-shaking and bundling practices."
libs/infra-nest-server/src/lib/bootstrap.ts (1)

Pattern libs/**/*: "Confirm that the code adheres to the following:

  • Reusability of components and hooks across different NextJS apps.
  • TypeScript usage for defining props and exporting types.
  • Effective tree-shaking and bundling practices."
libs/node/utils/README.md (1)

Pattern libs/**/*: "Confirm that the code adheres to the following:

  • Reusability of components and hooks across different NextJS apps.
  • TypeScript usage for defining props and exporting types.
  • Effective tree-shaking and bundling practices."
libs/node/utils/src/lib/setupShutdownHooks.spec.ts (1)

Pattern libs/**/*: "Confirm that the code adheres to the following:

  • Reusability of components and hooks across different NextJS apps.
  • TypeScript usage for defining props and exporting types.
  • Effective tree-shaking and bundling practices."
libs/node/utils/src/lib/setupShutdownHooks.ts (1)

Pattern libs/**/*: "Confirm that the code adheres to the following:

  • Reusability of components and hooks across different NextJS apps.
  • TypeScript usage for defining props and exporting types.
  • Effective tree-shaking and bundling practices."
🪛 LanguageTool
libs/node/utils/README.md

[style] ~10-~10: To form a complete sentence, be sure to include a subject.
Context: ... executed before the server shuts down. Can be used to perform and wait for other c...

(MISSING_IT_THERE)

🔇 Additional comments (9)
libs/node/utils/src/lib/setupShutdownHooks.ts (1)

1-7: LGTM! Well-structured function signature with proper typing

The function signature is well-designed with TypeScript types and an optional callback for custom cleanup operations.

libs/node/utils/README.md (1)

14-15: Verify NX runner detection implementation

The documentation mentions NX runner detection using an environment variable. Let's verify this implementation.

✅ Verification successful

Let me gather more context about the implementation to ensure a complete verification.


NX runner detection and disconnect event handling are correctly implemented

The implementation in setupShutdownHooks.ts accurately matches the documentation:

  • Checks for NX runner using process.env.NX_INVOKED_BY_RUNNER === 'true'
  • Adds 'disconnect' event to termination events when running under NX
  • Implementation is well-tested with dedicated test cases verifying this behavior
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for the NX runner environment variable usage
rg -A 2 "NX_INVOKED_BY_RUNNER" 

# Look for the disconnect event handler implementation
ast-grep --pattern 'process.on("disconnect"'

Length of output: 1216


Script:

#!/bin/bash
# Get the full implementation of setupShutdownHooks
cat libs/node/utils/src/lib/setupShutdownHooks.ts

# Get the full test implementation
cat libs/node/utils/src/lib/setupShutdownHooks.spec.ts

Length of output: 5125

libs/infra-express-server/src/lib/infra-express-server.ts (3)

45-50: LGTM! Parameter naming follows best practices

The underscore prefix for unused parameters follows TypeScript conventions and improves code clarity.


88-88: Verify graceful shutdown behavior

The addition of setupShutdownHooks addresses the PR objective, but we should verify it properly handles in-flight requests during shutdown.

✅ Verification successful

Graceful shutdown implementation is properly handled

The setupShutdownHooks implementation correctly handles in-flight requests by:

  • Using server.close() which waits for existing connections to complete before shutting down
  • Preventing multiple shutdown attempts with the isShuttingDown flag
  • Supporting custom cleanup through the optional onShutdown callback
  • Properly handling termination signals (SIGHUP, SIGINT, SIGTERM)
  • Logging shutdown progress and errors for observability
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if setupShutdownHooks properly handles in-flight requests
# Look for timeout configurations and request handling during shutdown
ast-grep --pattern 'setupShutdownHooks($_) {
  $$$
}'

Length of output: 55


Script:

#!/bin/bash
# Search for setupShutdownHooks function definition
rg -A 10 "setupShutdownHooks" --type ts

Length of output: 10770


Script:

#!/bin/bash
# Get the full implementation of setupShutdownHooks
cat libs/node/utils/src/lib/setupShutdownHooks.ts

Length of output: 1102


1-1: Verify the implementation of setupShutdownHooks

Let's ensure the imported function properly handles graceful shutdowns.

✅ Verification successful

Implementation of setupShutdownHooks is properly integrated

The implementation is well-tested and correctly used in the Express server:

  • Handles standard termination signals (SIGHUP, SIGINT, SIGTERM)
  • Includes special handling for NX runner environments
  • Provides graceful shutdown with proper server closure
  • Supports optional cleanup through onShutdown callback
  • Correctly integrated at the end of runServer function
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check the implementation of setupShutdownHooks
# Look for the implementation and documentation
fd -e md -e ts "node-utils" | xargs rg -l "setupShutdownHooks"

Length of output: 314


Script:

#!/bin/bash
# Check the implementation and documentation of setupShutdownHooks
echo "=== Implementation of setupShutdownHooks ==="
ast-grep --pattern 'export function setupShutdownHooks($_) { $$$ }'

echo -e "\n=== README Documentation ==="
cat libs/node/utils/README.md

echo -e "\n=== Test Cases ==="
cat libs/node/utils/src/lib/setupShutdownHooks.spec.ts

Length of output: 5686


Script:

#!/bin/bash
# Check the actual implementation file and usage in express server
echo "=== Implementation File ==="
cat libs/node/utils/src/lib/setupShutdownHooks.ts

echo -e "\n=== Usage in Express Server ==="
rg -A 5 "setupShutdownHooks" libs/infra-express-server/src/lib/infra-express-server.ts

Length of output: 1709

libs/infra-nest-server/src/lib/bootstrap.ts (2)

49-51: LGTM! Graceful shutdown implementation follows best practices.

The addition of app.enableShutdownHooks() properly implements signal handling for graceful shutdowns, which aligns with the PR objectives. This ensures that the application can properly clean up resources and complete ongoing requests before terminating.


49-51: Verify shutdown cleanup in the close method.

The close method handles cleanup of both the main app and metrics server. Let's verify it properly awaits all cleanup operations.

Also applies to: 156-164

✅ Verification successful

Shutdown cleanup is properly implemented

The close method correctly handles the cleanup by:

  • Awaiting the NestJS application shutdown with app.close()
  • Properly closing the metrics server using a Promise-based approach
  • Following the correct order: first app, then metrics server
  • Using enableShutdownHooks() to handle system signals appropriately

The implementation ensures all resources are cleaned up gracefully during shutdown.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for potential shutdown-related issues in the codebase

# Look for other cleanup operations that might need to be included
rg -A 5 "beforeApplicationShutdown|onApplicationShutdown|close\(\)" --type typescript

# Check for any custom shutdown hooks that might need coordination
ast-grep --pattern 'app.enableShutdownHooks()' 

Length of output: 810


Script:

#!/bin/bash
# Let's check the close method implementation and any shutdown hooks
ast-grep --pattern 'close() {
  $$$
}'

# Also check for any beforeApplicationShutdown or onApplicationShutdown implementations
ast-grep --pattern 'beforeApplicationShutdown() {
  $$$
}'

ast-grep --pattern 'onApplicationShutdown() {
  $$$
}'

# Let's also check the specific file for the close method
cat libs/infra-nest-server/src/lib/bootstrap.ts

Length of output: 4705

libs/node/utils/src/lib/setupShutdownHooks.spec.ts (1)

1-11: LGTM! Clean test setup with proper mocking.

The test setup follows best practices with clear imports and focused mocking of the logger functionality.

tsconfig.base.json (1)

956-956: LGTM! Path mapping correctly configured for node-utils module.

The new path mapping for @island.is/node-utils is properly configured and aligns with the PR's objective of implementing graceful shutdowns.

Let's verify the module's usage:

✅ Verification successful

Path mapping correctly configured and actively used in the codebase

The verification confirms that:

  • The @island.is/node-utils module is properly imported in two server implementations:
    • libs/infra-next-server/src/lib/bootstrap.ts
    • libs/infra-express-server/src/lib/infra-express-server.ts
  • The implementation file exists at the correct location: libs/node/utils/src/lib/setupShutdownHooks.ts
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the usage of @island.is/node-utils module
# Expected: Find imports of setupShutdownHooks from this module

# Search for imports of the module
rg "from ['|\"]@island\.is/node-utils['|\"]"

# Search for the implementation file
fd "setupShutdownHooks.ts" "libs/node/utils/src"

Length of output: 363

libs/node/utils/src/lib/setupShutdownHooks.ts Outdated Show resolved Hide resolved
coderabbitai[bot]
coderabbitai bot previously approved these changes Nov 28, 2024
@AndesKrrrrrrrrrrr
Copy link
Member Author

@eirikurn, hmm 🤔

Having forceful teardowns of connections this way is only marginally better than the current situation.

Pods in terminating state shouldn't receive any new requests, yet it seems like they still will according to this issue. So, we still need to ensure graceful teardown by stopping accepting new connections

@AndesKrrrrrrrrrrr
Copy link
Member Author

Copy link
Contributor

@snaerseljan snaerseljan left a comment

Choose a reason for hiding this comment

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

Nice work 👏

libs/node/utils/README.md Show resolved Hide resolved
libs/node/utils/src/lib/setupShutdownHooks.spec.ts Outdated Show resolved Hide resolved
libs/node/utils/src/lib/setupShutdownHooks.ts Outdated Show resolved Hide resolved
libs/node/utils/src/lib/setupShutdownHooks.ts Outdated Show resolved Hide resolved
@gudjong gudjong removed the deprecated:automerge (Disabled) Merge this PR as soon as all checks pass label Dec 1, 2024
AndesKrrrrrrrrrrr and others added 2 commits January 22, 2025 15:33
…e-app,web,api-domains-secondary-school,api,application-template-api-modules,application-system-api,api-domains-application,application-templates-secondary-school,application-template-loader,application-system-form,application-ui-shell,clients-secondary-school,clients-ultraviolet-radiation,clients-statistics,api-domains-statistics,cms,api-domains-icelandic-government-institution-vacancies,api-domains-university-careers,api-domains-university-gateway,api-domains-social-insurance,api-domains-license-service,api-domains-communications,api-domains-content-search,api-domains-notifications,api-domains-email-signup,air-discount-scheme-api,content-search-indexer,services-search-indexer,api-domains-auth,api-domains-auth-admin,api-domains-sessions,cms-translations,api-domains-occupational-licenses-v2,services-user-notification,clients-user-notification,api-domains-law-and-order,application-api-history,judicial-system-backend,api-domains-vehicles,judicial-system-api,infra-express-server,infra-nest-server,services-auth-personal-representative-public,services-auth-personal-representative,judicial-system-digital-mailbox-api,regulations-admin-backend,clients-regulations-admin,api-domains-regulations-admin,download-service,icelandic-names-registry-backend,services-auth-delegation-api,clients-auth-delegation-api,services-user-profile,clients-user-profile,api-domains-user-profile,services-auth-ids-api,air-discount-scheme-backend,clients-air-discount-scheme,api-domains-air-discount-scheme,services-university-gateway,clients-university-gateway-api,application-templates-university,services-endorsements-api,api-domains-endorsement-system,services-auth-public-api,clients-auth-public-api,judicial-system-xrd-api,services-auth-admin-api,clients-auth-admin-api,financial-aid-open-api,financial-aid-backend,clients-municipalities-financial-aid,api-domains-municipalities-financial-aid,license-api,services-form-system,services-documents,api-domains-document-provider,clients-document-provider,financial-aid-api,reference-backend,services-sessions,clients-sessions,skilavottord-ws,clients-car-recycling,services-bff,testing-nest,services-auth-testing,auth-api-lib,nest-problem,application-api-payment,application-types,application-templates-social-insurance-administration-additional-support-for-the-elderly,application-templates-transport-authority-order-vehicle-registration-certificate,application-templates-social-insurance-administration-household-supplement,system-e2e,application-templates-transport-authority-digital-tachograph-workshop-card,application-templates-transport-authority-digital-tachograph-company-card,application-templates-transport-authority-digital-tachograph-drivers-card,application-templates-social-insurance-administration-pension-supplement,application-templates-transport-authority-anonymity-in-vehicle-registry,application-templates-transport-authority-transfer-of-vehicle-ownership,application-templates-parliamentary-list-creation,application-templates-parliamentary-list-signing,application-templates-presidential-list-creation,application-templates-social-insurance-administration-old-age-pension,application-templates-transport-authority-order-vehicle-license-plate,application-templates-presidential-list-signing,application-templates-social-insurance-administration-death-benefits,application-templates-transport-authority-change-co-owner-of-vehicle,application-templates-transport-authority-change-operator-of-vehicle,fiancial-statement-individual-election,application-templates-children-residence-change-v2,application-templates-social-insurance-administration-income-plan,application-templates-transport-authority-license-plate-renewal,application-templates-inao-financial-statement-political-party,application-templates-directorate-of-immigration-citizenship,application-templates-driving-license-book-update-instructor,application-templates-social-insurance-administration-core,application-templates-parental-leave,application-templates-aosh-transfer-of-machine-ownership,financial-statement-cemetery,application-templates-complaints-to-althingi-ombudsman,application-templates-driving-instructor-registrations,application-templates-aosh-work-accident-notification,application-templates-aosh-change-machine-supervisor,application-templates-european-health-insurance-card,application-templates-healthcare-license-certificate,application-templates-document-provider-onboarding,application-templates-health-insurance-declaration,application-templates-aosh-request-for-inspection,application-templates-driving-assessment-approval,application-templates-driving-school-confirmation,application-templates-funding-government-projects,application-templates-official-journal-of-iceland,application-templates-aosh-register-new-machine,application-templates-data-protection-complaint,application-templates-driving-license-duplicate,application-templates-financial-statements-inao,application-templates-institution-collaboration,application-templates-aosh-street-registration,application-templates-grindavik-housing-buyout,application-templates-public-debt-payment-plan,application-templates-aosh-deregister-machine,application-templates-driving-learners-permit,application-templates-general-fishing-license,application-templates-alcohol-tax-redemption,application-templates-healthcare-work-permit,application-templates-accident-notification,application-templates-announcement-of-death,application-templates-mortgage-certificate,application-templates-family-matters-core,application-templates-marriage-conditions,application-templates-no-debt-certificate,application-templates-inheritance-report,application-templates-new-primary-school,application-templates-passport-annulment,application-templates-reference-template,application-templates-operating-license,application-templates-general-petition,application-templates-health-insurance,application-templates-criminal-record,application-templates-driving-license,application-templates-example-payment,application-templates-car-recycling,application-templates-financial-aid,application-templates-login-service,application-templates-energy-funds,application-templates-home-support,portals-admin-application-system,portals-admin,application-templates-passport,application-templates-id-card,portals-my-pages-applications,service-portal,application-templates-estate,application-templates-p-sign,application-ui-components,application-ui-fields,island-ui-storybook,application-api-files,clients-charge-fjs-v2,api-domains-payment,application-utils,application-api-core,application-ui-forms,application-testing,application-core,api-mocks,air-discount-scheme-web,skilavottord-web,nest-swagger,infra-next-server,financial-aid-web-veita,financial-aid-web-osk,financial-aid-web-osk-e2e,consultation-portal,judicial-system-web,reference-next-app,contentful-apps,auth-admin-web,node-utils,contentful-role-permissions,services-contentful-entry-tagger,judicial-system-message-handler,judicial-system-scheduler,external-contracts-tests,services-xroad-collector,github-actions-cache,auth-shared,portals-admin-ids-admin,portals-admin-service-desk,clients-university-application-agricultural-university-of-iceland,clients-university-application-iceland-university-of-the-arts,api-domains-administration-of-occupational-safety-and-health,clients-transport-authority-digital-tachograph-drivers-card,api-domains-transport-authority,clients-administration-of-occupational-safety-and-health,clients-university-application-university-of-akureyri,clients-university-application-university-of-iceland,clients-university-application-bifrost-university,clients-university-application-holar-university,api-domains-financial-statement-individual-election,api-domains-official-journal-of-iceland-application,clients-health-insurance,api-domains-health-insurance,clients-university-application-reykjavik-university,clients-icelandic-government-institution-vacancies,clients-transport-authority-vehicle-plate-ordering,clients-transport-authority-vehicle-plate-renewal,clients-rights-portal,api-domains-rights-portal,clients-license-client,license,clients-transport-authority-vehicle-owner-change,clients-transport-authority-vehicle-operators,clients-official-journal-of-iceland-application,clients-transport-authority-vehicle-codetables,clients-transport-authority-vehicle-infolocks,portals-my-pages-education-student-assessment,portals-my-pages-education-career,portals-my-pages-education,portals-my-pages-social-insurance-maintenance,clients-transport-authority-vehicle-printing,clients-official-journal-of-iceland,api-domains-official-journal-of-iceland,api-domains-financial-statement-cemetery,clients-district-commissioners-licenses,clients-social-insurance-administration,api-domains-housing-benefit-calculator,portals-admin-icelandic-names-registry,portals-my-pages-occupational-licenses,api-domains-financial-statements-inao,clients-ums-cost-of-living-calculator,api-domains-umbodsmadur-skuldara,portals-my-pages-signature-collection,portals-my-pages-information,api-domains-icelandic-names-registry,api-domains-national-registry-x-road,api-domains-driving-license,api-domains-driving-license-book,api-domains-identity,api-domains-intellectual-properties,clients-directorate-of-immigration,clients-housing-benefit-calculator,portals-admin-signature-collection,portals-my-pages-education-license,portals-shared-modules-delegations,delegation-admin,api-domains-directorate-of-labour,api-domains-occupational-licenses,api-domains-watson-assistant-chat,clients-data-protection-complaint,clients-financial-statements-inao,portals-admin-air-discount-scheme,portals-my-pages-education-degree,api-domains-mortgage-certificate,api-domains-signature-collection,api-consultation-portal,clients-adr-and-machine-license,clients-intellectual-properties,clients-judicial-administration,clients-rsk-personal-tax-return,island-ui-vanilla-extract-utils,island-ui-core,portals-admin-document-provider,portals-admin-regulations-admin,portals-my-pages-law-and-order,my-pages-air-discount,portals-my-pages-restrictions,portals-my-pages-documents,portals-my-pages-petitions,portals-admin-form-system,portals-my-pages-licenses,portals-my-pages-sessions,portals-my-pages-consent,portals-my-pages-finance,portals-my-pages-assets,portals-my-pages-health,portals-admin-petition,portals-my-pages-core,financial-aid-shared,island-ui-contentful,portals-admin-core,shared-form-fields,shared-components,react-components,react-spa-bff,react-feature-flags,portals-core,react-spa-shared,shared-connected,api-domains-disability-license,api-domains-health-directorate,clients-vehicle-service-fjs-v1,icelandic-names-registry-types,api-domains-aircraft-registry,api-domains-national-registry,api-domains-company-registry,api-domains-housing-benefits,api-domains-payment-schedule,clients-driving-license-book,clients-hms-housing-benefits,clients-national-registry-v2,clients-national-registry-v3,api-domains-education,clients-mms-grade,clients-identity,clients-rsk-company-registry,clients-signature-collection,content-search-index-manager,content-search-indexer-types,content-search-toolkit,content-search-metrics,judicial-system-court-client,api-domains-criminal-record,api-domains-fishing-license,clients-consultation-portal,judicial-system-audit-trail,air-discount-scheme-consts,clients-althingi-ombudsman,clients-disability-license,clients-health-directorate,clients-judicial-system-sp,clients-smart-solutions-v2,clients-university-careers,judicial-system-formatters,portals-my-pages-constants,air-discount-scheme-types,aircraft-registry,api-domains-api-catalogue,api-domains-ship-registry,api-domains-work-machines,clients-rsk-relationships,clients-work-accident-ver,api-domains-energy-funds,clients-payment-schedule,clients-vehicles-mileage,clients-workpoint-arborg,portals-my-pages-graphql,api-domains-file-upload,api-domains-regulations,api/domains/form-system,clients-criminal-record,clients-driving-license,clients-firearm-license,clients-fishing-license,clients-hunting-license,judicial-system-lawyers,judicial-system-message,api-catalogue-services,api-domains-fiskistofa,clients-ehic-client-v1,clients-smartsolutions,judicial-system-consts,judicial-system-auth,api-catalogue-elastic,api-domains-documents,api-domains-hms-loans,api-domains-syslumenn,clients-work-machines,judicial-system-types,ship-registry,api-catalogue-consts,api-catalogue-types,api-domains-passport,clients-auth-ids-api,clients-documents-v2,clients-energy-funds,api-domains-finance,application-graphql,clients-middlewares,clients-regulations,form-system-client,clients-finance-v2,clients-fiskistofa,clients-hms-loans,clients-mms-frigg,clients-passports,clients-syslumenn,clients-islykill,clients-vehicles,clients-finance,clients-assets,api-domains-assets,clients-p-card,clients-inna,clients-cms,clients-mms,shared-translations,skilavottord-consts,nest-feature-flags,skilavottord-types,testing-containers,message-queue,university-gateway,clients-documents,residence-history,shared-constants,infra-tracing,nest-core,localization,shared-utils,clients-vmst,logging,auth-nest-tools,testing-fixtures,nest-dataloader,nest-audit,clients-zendesk,dokobit-signing,nest-sequelize,email-service,infra-metrics,nest-config,file-storage,nova-sms,nest-aws,cache,island-ui-theme,island-ui-utils,regulations,nest-pagination,user-monitoring,shared-mocking,shared-problem,feature-flags,next-ids-auth,nest-graphql,shared-babel,shared-types,auth-scopes,testing-e2e,api-schema,shared-pii,plausible update dirty files
@andes-it andes-it requested review from a team as code owners January 22, 2025 16:15
@andes-it andes-it requested a review from Tryggvig January 22, 2025 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deploy-feature Deploys features to dev
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants