fix: engine restart process for Deno used wrong status #33865
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
🦋 Changeset detectedLatest commit: ef8469f The changes in this PR will be included in the next version bump. This PR includes changesets to release 36 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #33865 +/- ##
========================================
Coverage 75.42% 75.42%
========================================
Files 493 493
Lines 21499 21499
Branches 5337 5337
========================================
Hits 16215 16215
Misses 4644 4644
Partials 640 640
Flags with carried forward coverage won't be shown. Click here to find out more. |
| @@ -10,7 +10,7 @@ const defaultOptions: LivenessManager['options'] = { | |||
| pingRequestTimeout: 10000, | |||
| pingFrequencyInMS: 10000, | |||
| consecutiveTimeoutLimit: 4, | |||
| maxRestarts: 3, | |||
| maxRestarts: Infinity, | |||
There was a problem hiding this comment.
Is this safe? If an app restarted more than 10 times, should we continue trying to restart?
There was a problem hiding this comment.
It is safe, but you're right to suggest that after some restarts it might mean that the app should not work at all. However, until we have a place where we can show the restart information it will just cause confusion as to why the app is not working properly. Mainly because a simple "enable" call will not make the engine create a new subprocess for the app
There was a problem hiding this comment.
Is there any chance of deno being "restarted" wrongly?
I ask because we can effectively then have a tremendous number of stale processes running
There was a problem hiding this comment.
Maybe after a small amount of restarts the log gets pushed to all installed apps'/affected app's individual log? So we dont need a way to expose it centrally but is available through all apps
There was a problem hiding this comment.
Is there any chance of deno being "restarted" wrongly?
You mean like, unnecessarily? I guess it's possible, but the "restart" procedure attempts to kill the living process before spawning a new one, and if we fail killing the process we just halt. So I'd think the chances are slim
Maybe after a small amount of restarts the log gets pushed to all installed apps'/affected app's individual log?
I can add a new warn log if the restart call happens after a threshold of 3 times
There was a problem hiding this comment.
What happens if a user attempts to "enable" an app while it's being restarted? Could it cause 2 attempts of start, and 2 loops of "app restarting"?
I think we can have a special log of "app restarting" every time it happens so users can also check on them, and we as well. Cause if an app is restarting 2 times every some time, some actions may not work but since we're not passing the threshold, we won't see what's going on.
Also, i think these restart logs should (even if we don't show on the UI) have all the information needed about the process failing and a why if possible. wdyt?
There was a problem hiding this comment.
What happens if a user attempts to "enable" an app while it's being restarted? Could it cause 2 attempts of start, and 2 loops of "app restarting"?
If a user tries to enable an app while it's being restarted it will fail because the process is not ready to take in requests
There was a problem hiding this comment.
I've modified the logging a bit. Please @tapiarafael @debdutdeb @KevLehman check it out again
| @@ -10,7 +10,7 @@ const defaultOptions: LivenessManager['options'] = { | |||
| pingRequestTimeout: 10000, | |||
| pingFrequencyInMS: 10000, | |||
| consecutiveTimeoutLimit: 4, | |||
| maxRestarts: 3, | |||
| maxRestarts: Infinity, | |||
There was a problem hiding this comment.
What happens if a user attempts to "enable" an app while it's being restarted? Could it cause 2 attempts of start, and 2 loops of "app restarting"?
I think we can have a special log of "app restarting" every time it happens so users can also check on them, and we as well. Cause if an app is restarting 2 times every some time, some actions may not work but since we're not passing the threshold, we won't see what's going on.
Also, i think these restart logs should (even if we don't show on the UI) have all the information needed about the process failing and a why if possible. wdyt?
Co-authored-by: Kevin Aleman <[email protected]>
Proposed changes (including videos or screenshots)
Whenever the Deno subprocess of an app crashes, the Apps-Engine tries to restart it so we can keep the operation going. However, the app wasn't correctly reporting its status afterwards, and stayed at the
constructedorinitializedstatus, which the engine does not recognize as enabled.Also, we're removing the limit for the number of times the Apps-Engine would restart an app. At this point, it is not possible to show the restart or process status information to the admin, so it would just cause confusion to see the app not working.
Issue(s)
SUP-690
Steps to test or reproduce
Further comments