(kafka) branch trunk updated: KAFKA-19334 MetadataShell execution unintentionally deletes lock file (#19817)

[frankvicky]

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

frankvicky pushed a commit to branch trunk
in repository https://gitbox.apache.org/repos/asf/kafka.git


The following commit(s) were added to refs/heads/trunk by this push:
     new e2500186cbf KAFKA-19334 MetadataShell execution unintentionally 
deletes lock file (#19817)
e2500186cbf is described below

commit e2500186cbf79d48f163268117e9e75f10a5e53c
Author: Okada Haruki <ocadar...@gmail.com>
AuthorDate: Mon Jun 9 13:24:26 2025 +0900

    KAFKA-19334 MetadataShell execution unintentionally deletes lock file 
(#19817)
    
    ## Summary
    - MetadataShell may deletes lock file unintentionally when it exists or
    fails to acquire lock. If there's running server, this causes unexpected
    result as below:
      * MetadataShell succeeds on 2nd run unexpectedly
      * Even worse, LogManager/RaftManager's lock also no longer work from
    concurrent Kafka process startup
    
    Reviewers: TengYao Chi <frankvi...@apache.org>
---
 .../main/java/org/apache/kafka/server/util/FileLock.java |  8 ++++++++
 .../main/java/org/apache/kafka/shell/MetadataShell.java  |  6 +++---
 .../apache/kafka/shell/MetadataShellIntegrationTest.java | 16 ++++++++++------
 3 files changed, 21 insertions(+), 9 deletions(-)

diff --git 
a/server-common/src/main/java/org/apache/kafka/server/util/FileLock.java 
b/server-common/src/main/java/org/apache/kafka/server/util/FileLock.java
index 4f55b4aebcd..b06f239183a 100644
--- a/server-common/src/main/java/org/apache/kafka/server/util/FileLock.java
+++ b/server-common/src/main/java/org/apache/kafka/server/util/FileLock.java
@@ -91,4 +91,12 @@ public class FileLock {
         }
         channel.close();
     }
+
+    /**
+     * Unlock the file and close the associated FileChannel
+     */
+    public synchronized void unlockAndClose() throws IOException {
+        unlock();
+        channel.close();
+    }
 }
diff --git a/shell/src/main/java/org/apache/kafka/shell/MetadataShell.java 
b/shell/src/main/java/org/apache/kafka/shell/MetadataShell.java
index 6812bd0cc62..7af9557381d 100644
--- a/shell/src/main/java/org/apache/kafka/shell/MetadataShell.java
+++ b/shell/src/main/java/org/apache/kafka/shell/MetadataShell.java
@@ -114,7 +114,7 @@ public final class MetadataShell {
                     "directory before proceeding.");
             }
         } catch (Throwable e) {
-            fileLock.destroy();
+            fileLock.unlockAndClose();
             throw e;
         }
         return fileLock;
@@ -186,9 +186,9 @@ public final class MetadataShell {
         Utils.closeQuietly(snapshotFileReader, "snapshotFileReader");
         if (fileLock != null) {
             try {
-                fileLock.destroy();
+                fileLock.unlockAndClose();
             } catch (Exception e) {
-                log.error("Error destroying fileLock", e);
+                log.error("Error cleaning up fileLock", e);
             } finally {
                 fileLock = null;
             }
diff --git 
a/shell/src/test/java/org/apache/kafka/shell/MetadataShellIntegrationTest.java 
b/shell/src/test/java/org/apache/kafka/shell/MetadataShellIntegrationTest.java
index 0b65d96ab43..6a9c1c769dd 100644
--- 
a/shell/src/test/java/org/apache/kafka/shell/MetadataShellIntegrationTest.java
+++ 
b/shell/src/test/java/org/apache/kafka/shell/MetadataShellIntegrationTest.java
@@ -97,12 +97,16 @@ public class MetadataShellIntegrationTest {
                 FileLock fileLock = new FileLock(new File(env.tempDir, 
".lock"));
                 try {
                     fileLock.lock();
-                    assertEquals("Unable to lock " + 
env.tempDir.getAbsolutePath() +
-                        ". Please ensure that no broker or controller process 
is using this " +
-                        "directory before proceeding.",
-                        assertThrows(RuntimeException.class,
-                            () -> env.shell.run(List.of())).
-                                getMessage());
+                    // We had a bug where the shell can lock the directory 
unintentionally
+                    // at the 2nd run, so we check that it fails (See 
KAFKA-19334)
+                    for (int i = 0; i < 2; i++) {
+                        assertEquals("Unable to lock " + 
env.tempDir.getAbsolutePath() +
+                                     ". Please ensure that no broker or 
controller process is using this " +
+                                     "directory before proceeding.",
+                                     assertThrows(RuntimeException.class,
+                                                  () -> 
env.shell.run(List.of())).
+                                             getMessage());
+                    }
                 } finally {
                     fileLock.destroy();
                 }