Skip to content

fix: Close both apps to allow for process restart #885

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

Merged
merged 1 commit into from
Feb 5, 2025

Conversation

kamilogorek
Copy link
Member

@kamilogorek kamilogorek commented Feb 5, 2025

Currently when the main app is closed, or any node.js termination signal is sent (which is hijacked by closeWithGrace https://github.com/mcollina/close-with-grace?tab=readme-ov-file#api), an adminApp instance will keep it hostage and won't allow ec2 to restart everything correctly.

This can be tested by updating /health endpoint to call app.close() and observing that process is still running due to adminApp server being bound.

This will hopefully kill server instantly and won't let other requests come in to this instance as observed in our ec2 logs now.

Sidenote: why do we expose prometheus metrics on a separate port/app and not just /metrics endpoint within same app instead? @soedirgo

@kamilogorek kamilogorek requested review from a team as code owners February 5, 2025 15:07
@coveralls
Copy link

Pull Request Test Coverage Report for Build 13160373616

Details

  • 0 of 7 (0.0%) changed or added relevant lines in 2 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.06%) to 75.116%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/server/admin-app.ts 0 1 0.0%
src/server/server.ts 0 6 0.0%
Files with Coverage Reduction New Missed Lines %
src/server/server.ts 1 0.0%
Totals Coverage Status
Change from base Build 13149849112: -0.06%
Covered Lines: 4848
Relevant Lines: 6368

💛 - Coveralls

@pcnc
Copy link
Member

pcnc commented Feb 5, 2025

Sidenote: why do we expose prometheus metrics on a separate port/app and not just /metrics endpoint within same app instead

I think it's so that we don't expose any type of metrics (not aware of the types of metrics we're exposing) - but eitherway those aren't being scraped either atm

@kamilogorek kamilogorek merged commit fc69684 into master Feb 5, 2025
5 checks passed
@kamilogorek kamilogorek deleted the close-both-apps branch February 5, 2025 15:19
avallete pushed a commit that referenced this pull request May 13, 2025
fix: Close both apps to allow for process restart
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.

3 participants