From bf1a9df0c56804e9d02f6bac531be718f6aa58f6 Mon Sep 17 00:00:00 2001
From: Lari Hotari <lhotari@apache.org>
Date: Mon, 27 Jul 2026 10:19:45 +0300
Subject: [PATCH] [fix][test] Fix flaky AdminApiTest caused by leaked
 brokerShutdownTimeoutMs

testGetDynamicLocalConfiguration lowers the brokerShutdownTimeoutMs dynamic
configuration to 10ms and never restores it. The broker applies dynamic
configuration changes asynchronously, so the value can become effective while a
later test method shuts the broker down. PulsarService#close then fails with
"Timeout in close", stopBroker() propagates the exception without clearing the
pulsar field, and the @AfterMethod fails with "Connection refused" against the
broker that is no longer listening. A failing configuration method makes TestNG
skip every remaining test method of the class.

Restore the dynamic configuration in resetClusters() and skip the cleanup when
the broker isn't running, so a single failure can no longer take down the whole
class.

Assisted-by: Claude Code (Opus 5)
---
 .../pulsar/broker/admin/AdminApiTest.java     | 31 +++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApiTest.java b/pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApiTest.java
index c08cc1cc1626a..30c6cc7d4d2c5 100644
--- a/pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApiTest.java
+++ b/pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApiTest.java
@@ -154,6 +154,8 @@
 @Test(groups = "broker-admin")
 public class AdminApiTest extends MockedPulsarServiceBaseTest {
 
+    private static final String BROKER_SHUTDOWN_TIMEOUT_MS = "brokerShutdownTimeoutMs";
+
     private MockedPulsarService mockPulsarSetup;
 
     private PulsarService otherPulsar;
@@ -216,6 +218,14 @@ private void setupConfigAndStart(java.util.function.Consumer<ServiceConfiguratio
 
     @AfterMethod(alwaysRun = true)
     public void resetClusters() throws Exception {
+        if (pulsar == null || !pulsar.isRunning()) {
+            // A test method left the broker stopped, for example because stopBroker() failed. Throwing from
+            // here is a TestNG configuration failure, which makes TestNG skip every remaining test method of
+            // this class. TestRetrySupport recreates the shared test context before the next test method.
+            log.warn().log("Broker isn't running, skipping the cleanup of the previous test method");
+            return;
+        }
+        restoreBrokerShutdownTimeout();
         pulsar.getConfiguration().setForceDeleteTenantAllowed(true);
         pulsar.getConfiguration().setForceDeleteNamespaceAllowed(true);
         for (String tenant : admin.tenants().getTenants()) {
@@ -237,6 +247,25 @@ public void resetClusters() throws Exception {
         setupClusters();
     }
 
+    /**
+     * Several test methods of this class lower the {@code brokerShutdownTimeoutMs} dynamic configuration to 10ms.
+     * The broker applies dynamic configuration changes asynchronously, so when the value is left behind it can
+     * become effective while a later test method is shutting the broker down in stopBroker() or restartBroker().
+     * PulsarService#close then fails with "Timeout in close" and leaves the test holding a broker that is no
+     * longer listening, which fails this @AfterMethod and skips the remaining test methods of the class.
+     */
+    private void restoreBrokerShutdownTimeout() throws Exception {
+        String overriddenValue = admin.brokers().getAllDynamicConfigurations().get(BROKER_SHUTDOWN_TIMEOUT_MS);
+        if (overriddenValue == null) {
+            return;
+        }
+        long overriddenTimeoutMs = Long.parseLong(overriddenValue);
+        // removing the dynamic configuration makes the broker restore the value it was started with
+        admin.brokers().deleteDynamicConfiguration(BROKER_SHUTDOWN_TIMEOUT_MS);
+        Awaitility.await().untilAsserted(
+                () -> assertNotEquals(pulsar.getConfiguration().getBrokerShutdownTimeoutMs(), overriddenTimeoutMs));
+    }
+
     private void setupClusters() throws PulsarAdminException {
         admin.clusters().createCluster("test", ClusterData.builder().serviceUrl(pulsar.getWebServiceAddress()).build());
         TenantInfoImpl tenantInfo = new TenantInfoImpl(Set.of("role1", "role2"), Set.of("test"));
@@ -754,6 +783,8 @@ public void testGetDynamicLocalConfiguration() throws Exception {
         admin.brokers().updateDynamicConfiguration(configName, Long.toString(shutdownTime));
         // Now, znode is created: updateConfigurationAndRegisterListeners and check if configuration updated
         assertEquals(Long.parseLong(admin.brokers().getAllDynamicConfigurations().get(configName)), shutdownTime);
+        // wait until the broker has applied the value, so that the @AfterMethod always has to restore it
+        Awaitility.await().until(() -> pulsar.getConfiguration().getBrokerShutdownTimeoutMs() == shutdownTime);
     }
 
     @Test