Skip to content

[KYUUBI #7301] Fix engine not killed after deregisterEngine #7302

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.

Sign up for GitHub

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

Open
taylor12805 wants to merge 2 commits into
base: master
Choose a base branch
from
+12 −0
Open

[KYUUBI #7301] Fix engine not killed after deregisterEngine #7302

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f6812d2
[KYUUBI #7301] Fix engine not killed after deregisterEngine
Jan 12, 2026
b1501cf
[KYUUBI #7301] update check condition
Jan 16, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions kyuubi-server/src/main/scala/org/apache/kyuubi/engine/EngineRef.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -367,6 +367,18 @@ private[kyuubi] class EngineRef(
val msg = s"Deleting engine node:$sn"
info(msg)
discoveryClient.delete(s"$engineSpace/${sn.nodeName}")

if (shareLevel != CONNECTION && builder != null) {
try {
val appMgrInfo = builder.appMgrInfo()
engineManager.killApplication(appMgrInfo, engineRefId, Some(appUser))
Copy link

Copilot AI Jan 15, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
info(s"Killed engine process for $engineRefId with share level $shareLevel")
} catch {
case e: Exception =>
warn(s"Error killing engine process for $engineRefId", e)
}
}
Comment on lines +371 to +380
Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

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

Critical bug: The condition builder != null will prevent killing engines in the most common deregister scenarios. The builder field is only set when this EngineRef instance creates a new engine (line 216 in the create method). However, when an existing engine is discovered via getOrCreate, the builder remains null. This means when deregister is called for discovered engines (the exact scenario described in issue #7301), the engine process won't be killed.

Additionally, even if builder were non-null, calling builder.appMgrInfo() is incorrect here because the appMgrInfo should be retrieved from the ServiceNodeInfo that was just fetched from ZooKeeper. The correct pattern (as shown in AdminResource.scala:312) is to get appMgrInfo from sn.attributes.get(KyuubiReservedKeys.KYUUBI_ENGINE_APP_MGR_INFO_KEY).

The fix should:

  1. Get the ServiceNodeInfo from ZooKeeper
  2. Extract appMgrInfo from sn.attributes
  3. Use that to kill the application, without depending on the builder at all

Copilot uses AI. Check for mistakes.
Comment on lines +371 to +380
Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

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

The existing test for deregister (line 377-398 in EngineRefTests.scala) doesn't verify that the engine process is actually killed when deregister is called. The test only verifies that the engine is removed from ZooKeeper. Since this PR's purpose is to fix the bug where engines are not killed after deregistration, a test should be added to verify that killApplication is called with the correct parameters when deregister succeeds. This could be done by mocking the engineManager and verifying the killApplication call.

Copilot uses AI. Check for mistakes.

(true, msg)
} else {
val msg = s"Engine node:$sn is not matched with host&port[$hostPort]"
Expand Down
Loading