Skip to content

[fix][broker] Prevent stale service unit callbacks from dropping active lookup and cleanup jobs#26146

Open
void-ptr974 wants to merge 2 commits into
apache:masterfrom
void-ptr974:fix/service-unit-channel-stale-futures
Open

[fix][broker] Prevent stale service unit callbacks from dropping active lookup and cleanup jobs#26146
void-ptr974 wants to merge 2 commits into
apache:masterfrom
void-ptr974:fix/service-unit-channel-stale-futures

Conversation

@void-ptr974

@void-ptr974 void-ptr974 commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

Motivation

ServiceUnitStateChannelImpl stores pending owner lookups and inactive-broker cleanup jobs in maps keyed by service unit or broker. These futures can be replaced when a lookup is retried, when an Owned event completes a waiting lookup, or when cleanup is rescheduled.

Some completion and cancellation paths removed entries by key only. A stale callback could remove or cancel a newer future for the same key, causing owner lookups to wait for timeout or delaying inactive-broker ownership cleanup.

Modifications

  • Guard getOwnerRequests removals with the captured request future.
  • Guard cleanupJobs removals with the captured cleanup future.
  • Guard broker-creation cleanup cancellation with the cleanup future observed before the async health check.
  • Add regression tests for stale owner-lookup, skipped Owned event, cleanup-job completion, and broker-creation cancellation callbacks.

Verifying this change

  • ./gradlew :pulsar-broker:test --tests org.apache.pulsar.broker.loadbalance.extensions.channel.ServiceUnitStateChannelTest.testCompletedGetOwnerRequestDoesNotRemoveNewRequest --tests org.apache.pulsar.broker.loadbalance.extensions.channel.ServiceUnitStateChannelTest.testSkippedEventDoesNotRemoveNewGetOwnerRequest --tests org.apache.pulsar.broker.loadbalance.extensions.channel.ServiceUnitStateChannelTest.testCompletedCleanupJobDoesNotRemoveNewCleanupJob --tests org.apache.pulsar.broker.loadbalance.extensions.channel.ServiceUnitStateChannelTest.handleBrokerCreationEventDoesNotCancelNewCleanupJobTest
  • ./gradlew :pulsar-broker:checkstyleMain :pulsar-broker:checkstyleTest
  • git diff --check

@void-ptr974 void-ptr974 force-pushed the fix/service-unit-channel-stale-futures branch 2 times, most recently from db37eac to ca16bea Compare July 4, 2026 04:25
…futures

ServiceUnitStateChannel keeps deferred owner lookups and inactive broker cleanup jobs in maps keyed by service unit or broker. Completion callbacks removed entries by key, so a stale completion could remove a newer future installed after a retry.

Guard getOwnerRequests and cleanupJobs removals with the captured future. Also guard broker-creation cleanup cancellation so a delayed health-check result cannot cancel a newer cleanup job.

Add tests that reproduce stale get-owner and cleanup-job completion removing newer futures.
@void-ptr974 void-ptr974 force-pushed the fix/service-unit-channel-stale-futures branch from ca16bea to 1605aee Compare July 4, 2026 04:28

CountDownLatch staleCallbackRunning = new CountDownLatch(1);
CountDownLatch releaseStaleCallback = new CountDownLatch(1);
oldRequest.whenComplete((__, ___) -> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test assumes that the whenComplete registered here runs before the cleanup callback registered by dedupeGetOwnerRequest. CompletableFuture does not specify the execution order of dependent actions. If the production cleanup runs first, the patched implementation can legitimately remove the old mapping before staleCallbackRunning is released, causing the assertion below to fail. Could we make this deterministic by installing newRequest before completing oldRequest, then completing the old request and asserting that the new mapping remains? That would reproduce the stale cleanup without relying on callback ordering or an executor.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in 12d970b. The test now installs the replacement request before completing the old one, so it deterministically exercises the stale cleanup without relying on dependent-action ordering or an executor.


CountDownLatch staleCallbackRunning = new CountDownLatch(1);
CountDownLatch releaseStaleCallback = new CountDownLatch(1);
oldJob.whenComplete((__, ___) -> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has the same dependent-action ordering assumption as testCompletedGetOwnerRequestDoesNotRemoveNewRequest: the blocking callback registered here must run before the cleanup callback registered by scheduleCleanup, but CompletableFuture does not guarantee that ordering. We can avoid the latches and executor by replacing the map entry with newJob before calling oldJob.complete(null). The old implementation will then deterministically remove newJob, while the conditional-remove implementation will preserve it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in 12d970b. This test now replaces the cleanup job before completing the old job, making the stale removal deterministic and eliminating the latches and executor.

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