Skip to content

[fix][test] Fix flaky AdminApiTest caused by leaked brokerShutdownTimeoutMs - #26249

Merged
nodece merged 1 commit into
apache:masterfrom
lhotari:lh-fix-flaky-adminapitest-shutdown
Jul 27, 2026
Merged

[fix][test] Fix flaky AdminApiTest caused by leaked brokerShutdownTimeoutMs#26249
nodece merged 1 commit into
apache:masterfrom
lhotari:lh-fix-flaky-adminapitest-shutdown

Conversation

@lhotari

@lhotari lhotari commented Jul 27, 2026

Copy link
Copy Markdown
Member

Motivation

AdminApiTest can fail with the whole class collapsing after a single test method: the
@AfterMethod (resetClusters) fails with Connection refused and TestNG then skips every
remaining test method of the class.

The same failure was observed on branch-4.0 in
V1AdminApiTest,
which is the copy of this class that PIP-457 (#25304) removed from master. The bug itself is still
present in AdminApiTest on master.

[ERROR] Tests run: 47, Failures: 1, Errors: 0, Skipped: 20 <<< FAILURE! - in ...V1AdminApiTest
[ERROR] V1AdminApiTest.reset:193 » PulsarAdmin ...RetryException: Could not complete the
        operation. Number of retries has been exhausted.
        Failed reason: Connection refused: localhost/[0:0:0:0:0:0:0:1]:36193

The causal chain:

  1. testGetDynamicLocalConfiguration lowers the brokerShutdownTimeoutMs dynamic
    configuration to 10 ms and never restores it. It only asserts the stored value; the broker
    applies dynamic configuration changes asynchronously, so whether the value has reached
    pulsar.getConfiguration() by the time the next test method runs is a race — that is where the
    flakiness comes from.

  2. The next test method in alphabetical order, testInvalidDynamicConfigContentInMetadata, starts
    with stopBroker(). With a 10 ms budget PulsarService#close fails:

    PulsarService - Shutdown timed out after 10 ms
    org.apache.pulsar.broker.PulsarServerException:
        org.apache.pulsar.common.util.FutureUtil$LowOverheadTimeoutException: Timeout in close
        at org.apache.pulsar.broker.PulsarService.close(PulsarService.java:478)
        at ...MockedPulsarServiceBaseTest.stopBroker(MockedPulsarServiceBaseTest.java:391)
    
  3. MockedPulsarServiceBaseTest.stopBroker() runs pulsar.close(); pulsar = null;, so the thrown
    exception skips the assignment and the test is left holding a broker that is no longer
    listening, together with the admin client pointing at its web service port.

  4. resetClusters() then calls admin.tenants().getTenants() and fails with Connection refused.
    Because it is a configuration method, TestNG skips all remaining test methods of the class
    (19 in the CI run above) and the build fails. TestRetrySupport does recreate the test context
    for the retry, and the retried test passes, but the configuration failure has already been
    recorded.

Modifications

In AdminApiTest:

  • Add restoreBrokerShutdownTimeout() and call it at the start of resetClusters(). It removes
    the brokerShutdownTimeoutMs dynamic configuration (which makes the broker restore the value it
    was started with) and waits until the value the broker uses is no longer the override, so no
    test method can leave a ~10 ms shutdown budget behind for a later stopBroker() /
    restartBroker().
  • Return early from resetClusters() when the broker isn't running, with a warning. Throwing from
    a configuration method skips every remaining test method of the class; TestRetrySupport
    already recreates the shared test context before the next test method runs.
  • Make testGetDynamicLocalConfiguration wait until the broker has applied the value it sets.
    This is a small strengthening of the test itself, and it turns the race into a deterministic
    case that the @AfterMethod has to handle, so a regression fails consistently instead of
    flaking.

No production code is changed.

Verifying this change

  • Make sure that the change passes the CI checks.

This change is already covered by existing tests, such as AdminApiTest.

The failure was reproduced deterministically before fixing it, by making the asynchronous
propagation in testGetDynamicLocalConfiguration explicit and running
testGetDynamicLocalConfiguration, testInvalidDynamicConfigContentInMetadata and
testJacksonWithTypeDifferencies together. On master that run fails with exactly the CI
signature — FutureUtil$LowOverheadTimeoutException: Timeout in close on the test method and
PulsarAdminException ... Connection refused on resetClusters, with the following test method
skipped — and it passes with this change.

The full AdminApiTest class was run before and after the change: 100 tests, 0 failures in both
runs. The single retried testUpdateDynamicLoadBalancerSheddingIntervalMinutes behaves identically
on unmodified master (it asserts on the conf field that resetConfig() replaces in every
@AfterMethod, so it only passes once TestRetrySupport has re-run the setup); it is unrelated to
this change.

branch-4.0 still carries V1AdminApiTest with the same problem and will need a separate fix; it
cannot inherit this one.

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

…eoutMs

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)
@nodece
nodece merged commit 6a109ac into apache:master Jul 27, 2026
43 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants