(gravitino) branch main updated: [#10272] Improvement : nsure JobManager.close() always shuts down background executors when jobExecutor.close() fails (#10440)

[jmclean]

This is an automated email from the ASF dual-hosted git repository.

jmclean pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/gravitino.git


The following commit(s) were added to refs/heads/main by this push:
     new 526b320cce [#10272] Improvement : nsure JobManager.close() always 
shuts down background executors when jobExecutor.close() fails (#10440)
526b320cce is described below

commit 526b320cce8cc68c14626eb612ffec4751706897
Author: AmitaWhite <49367874+amitawh...@users.noreply.github.com>
AuthorDate: Wed Apr 8 13:46:14 2026 +0900

    [#10272] Improvement : nsure JobManager.close() always shuts down 
background executors when jobExecutor.close() fails (#10440)
    
    ### What changes were proposed in this pull request?
    
    This PR fixes a potential thread leak in JobManager.close(). Previously,
    if jobExecutor.close() threw an IOException, the method would terminate
    early, skipping the shutdown calls for statusPullExecutor and
    cleanUpExecutor.
    The fix wraps the jobExecutor.close() call in a try block and ensures
    that both background executors are shut down in the finally block.
    
    
    ### Why are the changes needed?
    
    To guarantee that background resources are properly cleaned up even when
    the main job executor fails to close, preventing resource leaks.
    
    Improvement : #10272
    
    
    ### Does this PR introduce _any_ user-facing change?
    
    No
    
    
    ### How was this patch tested?
    
    Added a new unit test TestJobManager to simulate jobExecutor.close()
    failure and verify that background executors are still shut down.
        Ran the specific unit test:
    
    ```sh
    ./gradlew :core:test --tests "org.apache.gravitino.job.TestJobManager"
    ```
    
    Co-authored-by: AmitaWhite <amitawhite@AmitaWhites-MacBook-Pro-13.local>
---
 .../main/java/org/apache/gravitino/job/JobManager.java   |  9 ++++++---
 .../java/org/apache/gravitino/job/TestJobManager.java    | 16 ++++++++++++++++
 2 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/core/src/main/java/org/apache/gravitino/job/JobManager.java 
b/core/src/main/java/org/apache/gravitino/job/JobManager.java
index 17246645c7..95b90e89dc 100644
--- a/core/src/main/java/org/apache/gravitino/job/JobManager.java
+++ b/core/src/main/java/org/apache/gravitino/job/JobManager.java
@@ -528,9 +528,12 @@ public class JobManager implements JobOperationDispatcher {
 
   @Override
   public void close() throws IOException {
-    jobExecutor.close();
-    statusPullExecutor.shutdownNow();
-    cleanUpExecutor.shutdownNow();
+    try {
+      jobExecutor.close();
+    } finally {
+      statusPullExecutor.shutdownNow();
+      cleanUpExecutor.shutdownNow();
+    }
   }
 
   @VisibleForTesting
diff --git a/core/src/test/java/org/apache/gravitino/job/TestJobManager.java 
b/core/src/test/java/org/apache/gravitino/job/TestJobManager.java
index 9bf06875ca..2dc4af741f 100644
--- a/core/src/test/java/org/apache/gravitino/job/TestJobManager.java
+++ b/core/src/test/java/org/apache/gravitino/job/TestJobManager.java
@@ -913,4 +913,20 @@ public class TestJobManager {
     Assertions.assertThrows(
         RuntimeException.class, () -> JobManager.fetchFileFromUri(uri, 
stagingDir, 1000));
   }
+
+  @Test
+  public void testCloseShouldShutdownExecutorsWhenJobExecutorCloseFails() 
throws IOException {
+    JobExecutor failingJobExecutor = Mockito.mock(JobExecutor.class);
+    doThrow(new IOException("close failed")).when(failingJobExecutor).close();
+
+    JobManager manager = new JobManager(config, entityStore, idGenerator, 
failingJobExecutor);
+    try {
+      Assertions.assertThrows(IOException.class, manager::close);
+      Assertions.assertTrue(manager.statusPullExecutor.isShutdown());
+      Assertions.assertTrue(manager.cleanUpExecutor.isShutdown());
+    } finally {
+      manager.statusPullExecutor.shutdownNow();
+      manager.cleanUpExecutor.shutdownNow();
+    }
+  }
 }