Fix instance push loss during initial subscribe in ServiceDiscoveryRegistry

[hnpkso]

PR: Fix instance push loss during initial subscribe in ServiceDiscoveryRegistry

Base branch: apache/dubbo:3.3 (also applies cleanly to 3.2 and 3.4)
Commits: 2 (test-only f982a15294, then fix 8e0f8f2d81). Check out either to see the failing-then-passing tests.

---

What this fixes

Fixes #16392.

ServiceDiscoveryRegistry.subscribeURLs() currently:

1. calls serviceDiscovery.getInstances(serviceName) — the initial pull,
2. runs listener.onEvent(...) which synchronously fetches metadata (can take seconds),
3. only then calls serviceDiscovery.addServiceInstancesChangedListener(...) to arm the push callback.

Any push arriving between step 1 and step 3 is absorbed by the discovery SDK's local cache and never reaches Dubbo. allInstances is then permanently stale until either (a) a later, independent push happens for the same appName, or (b) the process restarts.

We hit this in production (Nacos 2.x, Dubbo 3.2.19): a provider Nacos had removed 92 ms after our subscribe started remained a ghost invoker for the entire process lifetime, producing recurring Fail to decode request errors.

Timeline and full analysis are in the linked issue.

The change

dubbo-registry-api/…/ServiceDiscoveryRegistry.java — subscribeURLs():

1. Register the push callback before the initial pull. addServiceInstancesChangedListener(...) moves inside the if (listener == null) guard, ahead of the pull loop. Any push arriving during/after the pull now flows through the listener's already-armed doOnEvent path.

2. Serialize the pull with the push callback. Wrap the pull loop in synchronized(serviceInstancesChangedListener). doOnEvent is a synchronized method on the same object; both now share the same intrinsic lock, so a push arriving during initialization queues cleanly instead of interleaving writes to allInstances.

3. Push wins over stale pull. Inside the synchronized region, check listener.getAllInstances().containsKey(serviceName) before pulling. If a push has already populated this service, treat push as authoritative and skip the pull. This prevents a slow pull (reading a stale SDK cache) from overwriting a fresher push snapshot.

Diff is under 30 lines.

Before/after verification

Two commits are separated intentionally so the failing-then-passing tests can be observed directly:

git remote add hnpkso https://github.com/hnpkso/dubbo.git
git fetch hnpkso fix/subscribe-push-loss

git checkout f982a15294       # test commit only

mvn -pl dubbo-registry/dubbo-registry-api test \
    -Dtest=ServiceDiscoveryRegistryTest
# Tests run: 6, Failures: 3
#   ✗ testSubscribeURLs (times(2) assertion on register)
#   ✗ testSubscribeURLsRegistersPushCallbackBeforePull  ← MAIN
#   ✗ testSubscribeURLsSkipsPullWhenPushAlreadyPopulated

git checkout 8e0f8f2d81       # fix on top

mvn -pl dubbo-registry/dubbo-registry-api test \
    -Dtest=ServiceDiscoveryRegistryTest
# Tests run: 6, Failures: 0

The red-to-green transition is the reproduction — no Nacos or external system needed.

Full module test suite: Tests run: 111, Failures: 0.

Notes on behavior changes

1. addServiceInstancesChangedListener cardinality: now called once per listener lifetime, previously once per subscribeURLs() call. All in-tree ServiceDiscovery implementations already treat duplicate calls as no-ops via instanceListeners.add() early-return, so the change is observationally equivalent. The testSubscribeURLs assertion updates from times(2) to times(1) accordingly.

2. RegistryEvent.toSsEvent metric cardinality rides along with (1): it now fires once per listener creation instead of once per subscribe. If per-subscribe metric cardinality is preferred, MetricsEventBus.post(...) can be split back out of the register lambda without affecting correctness — happy to adjust.

3. Push executor threads may briefly park on the listener intrinsic lock during initial subscribe, bounded by the metadata fetch duration (~500 ms in our production). Previously such pushes were silently dropped, so this is a strict improvement.

Scope

This PR only fixes the push-loss race in subscribeURLs(). NacosServiceDiscovery and other ServiceDiscovery implementations are untouched; no new SPI contract. Any unrelated concerns in the same code path are out of scope for this change.

Related

• Update initial mapping apps of service discovery MappingListener #14851 fixed a sibling race on the MappingListener side of the same subscribe path (initial-apps was null when the listener was constructed). The instance-list side, addressed here, has the same shape of bug (populate-before-arm) but a different failure mode (silent instance loss vs. spurious mapping event).

Commits

• f982a15294 — Add failing tests for instance push loss during initial subscribe. Tests only, fails on unmodified 3.3.
• 8e0f8f2d81 — Fix instance push loss during initial subscribe in ServiceDiscoveryRegistry. Source fix, turns tests green.

Feel free to squash on merge; the two commits exist purely to make the red→green transition observable during review.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 92.85714% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 60.91%. Comparing base (eb1d8ab) to head (8e0f8f2).

Files with missing lines	Patch %	Lines
...ubbo/registry/client/ServiceDiscoveryRegistry.java	92.85%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##                3.3   #16393      +/-   ##
============================================
+ Coverage     60.90%   60.91%   +0.01%     
- Complexity    11763    11766       +3     
============================================
  Files          1953     1953              
  Lines         89266    89269       +3     
  Branches      13471    13472       +1     
============================================
+ Hits          54367    54378      +11     
+ Misses        29319    29315       -4     
+ Partials       5580     5576       -4     

Flag	Coverage Δ
integration-tests-java21	32.15% <92.85%> (-0.01%)	⬇️
integration-tests-java8	32.21% <92.85%> (-0.08%)	⬇️
samples-tests-java21	32.11% <85.71%> (-0.05%)	⬇️
samples-tests-java8	29.78% <85.71%> (-0.04%)	⬇️
unit-tests-java11	59.11% <92.85%> (-0.01%)	⬇️
unit-tests-java17	58.55% <92.85%> (-0.01%)	⬇️
unit-tests-java21	58.57% <92.85%> (-0.01%)	⬇️
unit-tests-java25	58.50% <92.85%> (-0.06%)	⬇️
unit-tests-java8	59.10% <92.85%> (-0.06%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Copilot]

Pull request overview

Fixes a race in ServiceDiscoveryRegistry.subscribeURLs() where instance-change push notifications could be lost during the initial subscribe window (between the initial pull and listener registration), leaving allInstances stale for the lifetime of the process.

Changes:

• Register addServiceInstancesChangedListener(...) before the initial getInstances(...) pull to avoid missing pushes during initialization.
• Serialize the initialization pull path with the listener’s push-handling path by synchronizing on the ServiceInstancesChangedListener, and skip the pull when a push has already populated allInstances for a service.
• Update/add unit tests to reproduce the race deterministically and assert correct ordering/behavior (including adjusted listener-registration call count).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
dubbo-registry/dubbo-registry-api/src/main/java/org/apache/dubbo/registry/client/ServiceDiscoveryRegistry.java	Reorders listener registration ahead of initial pulls and synchronizes initialization to prevent push-loss and stale-overwrite.
dubbo-registry/dubbo-registry-api/src/test/java/org/apache/dubbo/registry/client/ServiceDiscoveryRegistryTest.java	Adds/updates tests to enforce registration-before-pull ordering and “push wins over stale pull” behavior.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.