[fix][test] Drop racy concurrent-delete assertion in CompactionConcurrencyTest

[merlimat]

Motivation

CompactionConcurrencyTest.testDisableCompactionConcurrently fires two threads that both call deleteSubscriptionAsync(\"__compaction\") on the same topic and asserts that at least one of them fails:

assertTrue(f1.get().isCompletedExceptionally() || f2.get().isCompletedExceptionally());

That assertion has been flaky since the test was migrated from ProducerConsumerBase to SharedPulsarBaseTest in #25381.

The original test relied on MockZooKeeper.delay(...) to inject a delay into the cursor delete operation. This forced the two calls to overlap inside PersistentTopic#asyncDeleteCursorWithCleanCompactionLedger's synchronized block — the second one then hit the disablingCompaction.compareAndSet(false, true) == false branch and failed with SubscriptionBusyException.

SharedPulsarBaseTest uses an in-memory metadata store and there is no equivalent delay hook. The first delete can complete (and remove the subscription) before the second thread even reaches the broker. The second call then runs through asyncDeleteCursorWithClearDelayedMessage, sees subscriptions.get(\"__compaction\") == null, and returns success as a no-op:

PersistentSubscription persistentSubscription = subscriptions.get(subscriptionName);
if (persistentSubscription == null) {
    log.warn().attr(...).log(\"Can't find subscription, skip delete cursor\");
    unsubscribeFuture.complete(null);
    return;
}

Both futures complete successfully — perfectly valid behavior — and the assertion fails.

Example failure: https://scans.gradle.com/s/2odpjlsucvuly/tests/task/:pulsar-broker:test/details/org.apache.pulsar.broker.service.persistent.CompactionConcurrencyTest/testDisableCompactionConcurrently/2/output

Modifications

Drop the "at least one fails" assertion — the broker handles either outcome correctly (concurrent overlap → SubscriptionBusyException; sequential → no-op).

Keep the deadlock check (both futures complete in bounded time) and add a verification that the compaction subscription ends up removed, which is the actual invariant we care about.

Verifying this change

This change is already covered by CompactionConcurrencyTest.testDisableCompactionConcurrently. Locally I ran 5 times with --rerun-tasks; all passed.

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

• 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

[void-ptr974]

This makes sense to me. This is a simpler and more direct fix for the flaky assertion. Thanks for the update.

[merlimat]

Closing as superseded by #25794, which was merged earlier and fixes the same flakiness in CompactionConcurrencyTest.testDisableCompactionConcurrently with a more robust approach.

#25794 deterministically forces the concurrent-delete race by blocking the currentCompaction future, so it preserves the original test intent (the second delete reliably fails with PreconditionFailedException, the first succeeds, and disablingCompaction is verified to reset). My PR instead dropped the "at least one fails" assertion, which loses that coverage. Their fix is better — thanks @void-ptr974.

[void-ptr974]

Thanks @merlimat, appreciate it!