[KYUUBI #7301] Fix engine not killed after deregisterEngine #7302
Conversation
(cherry picked from commit b6b05e9)
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug where non-CONNECTION share level engines were not killed when deregistered, leaving zombie engine processes running. The fix adds logic to kill the engine application process when deregistering engines with share levels USER, GROUP, SERVER_LOCAL, or SERVER.
Changes:
- Added engine process killing logic in the
deregistermethod for non-CONNECTION share level engines - The kill attempt is wrapped in a try-catch block to handle failures gracefully
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (shareLevel != CONNECTION && builder != null) { | ||
| try { | ||
| val appMgrInfo = builder.appMgrInfo() | ||
| engineManager.killApplication(appMgrInfo, engineRefId, Some(appUser)) |
There was a problem hiding this comment.
Missing null check for engineManager. The condition should be updated to check both builder and engineManager for null before attempting to call killApplication. Without this check, a NullPointerException will occur when engineManager is null, such as in test scenarios where EngineRef is instantiated with a null engineManager parameter.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7302 +/- ##
======================================
Coverage 0.00% 0.00%
======================================
Files 698 698
Lines 43646 43654 +8
Branches 5894 5895 +1
======================================
- Misses 43646 43654 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I suppose the failing test is not correlated to this PR |
|
It might be better to make the Engine can know whether it is zombie, and stop itself. Since the |
3 participants
(cherry picked from commit b6b05e9)
Why are the changes needed?
Fix old engine not killed bug: #7301
How was this patch tested?
Was this patch authored or co-authored using generative AI tooling?
No