fix: self-heal application-level discovery after transient metadata failure (#3615) - #3625
Merged
Merged
Conversation
AsperforMias
force-pushed
the
fix/3615-metadata-retry
branch
from
August 9, 2026 10:05
2883588 to
9231ce5
Compare
…ailure (apache#3615) Two stacked recovery gaps made a transient MetadataService fetch failure permanent for application-level service discovery: 1. ServiceInstancesChangedListenerImpl skipped instances whose metadata fetch failed and never retried; with no further registry event the consumer directory stayed empty forever. Add a shared retry timer per listener: unresolved revisions are re-resolved by replaying the latest instance snapshot with capped exponential backoff (1s..30s + jitter, unlimited attempts). Retries stop naturally when instances leave the snapshot, subscribers are detached, or the registry is destroyed. Metadata RPCs no longer run under the listener state mutex. 2. RegistryDirectory's closing tombstone vetoed the rebuild even after a successful retry when the provider restarted with the same address, because a stale pre-shutdown snapshot and a genuine restart were indistinguishable. The tombstone now records the export timestamp and only vetoes re-adds carrying the same timestamp; a different timestamp proves a genuine restart and clears the tombstone.
AsperforMias
force-pushed
the
fix/3615-metadata-retry
branch
from
August 9, 2026 10:25
9231ce5 to
a2e3561
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #3625 +/- ##
===========================================
+ Coverage 46.76% 56.00% +9.23%
===========================================
Files 295 475 +180
Lines 17172 37226 +20054
===========================================
+ Hits 8031 20848 +12817
- Misses 8287 14734 +6447
- Partials 854 1644 +790 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
Author
|
Note on CI: the |
Alanxtl
reviewed
Aug 10, 2026
3 tasks
- Drop the caller-less hasActiveClosingTombstone wrapper; production code uses activeClosingTombstone directly and the tests keep a local helper. - Demote the per-event "received instance notification" log to debug: it fires on every registry push and is too noisy at info level. - refreshServiceURLs now reports whether every revision resolved, and OnEvent surfaces unresolved revisions as an error so registry dispatchers log the partial failure instead of it staying silent. The retry loop is unchanged and still self-schedules.
Alanxtl
reviewed
Aug 10, 2026
A retry timer could outlive its listener in three ways: - scheduleMetadataRetry only checked unresolvedRevisions, so a refresh that failed before any subscriber attached (the SubscribeURL order) armed the timer, and a listener discarded after losing the install race kept retrying — and was kept alive by the timer closure. - RemoveListener racing with the timer callback still allowed the callback to refresh and re-schedule with no subscribers left. - Destroy raced with an in-flight refresh: stopMetadataRetry canceled the pending timer, but the refresh's trailing scheduleMetadataRetry armed a new one after the registry was gone. The listener now has a closed flag; the scheduler and the timer callback re-check closed and subscriber presence under the mutex, and a refresh that fails with no subscribers is re-armed by the next AddListenerAndNotify instead of running detached. SubscribeURL closes the listener that loses the install race.
closed covered listeners already installed when Destroy runs, but a SubscribeURL still in its initial GetInstances/metadata phase is invisible to Destroy's listener sweep: when the call finished after Destroy returned, it would install the listener, attach the subscriber and arm a fresh unlimited retry timer on a dead registry (a blocking fetch probe shows metadata fetches continuing to grow after Destroy). The registry now carries a destroyed flag. SubscribeURL bails early on destroyed registries and re-checks the flag under the write lock right before installing: a late listener is closed and discarded instead of installed, so no AddListener call and no retry timer can appear after Destroy. Adds the deterministic SubscribeURL x Destroy interleaving test requested in review (blocking fetch probe; verified to fail without the fix: listener installed, AddListener called, fetches continue) plus a post-Destroy subscribe no-op test.
Member
|
pls fix conflict |
…a-retry # Conflicts: # registry/servicediscovery/service_instances_changed_listener_impl.go
…e#3615) SubscribeURL registers the notify listener under url.ServiceKey()+":"+protocol, but UnSubscribe removed it with the bare url.ServiceKey(), so the last subscriber was never detached and the metadata retry timer kept probing after UnSubscribe. Derive both sides from a shared protocolSubscribeKey helper and cover the public SubscribeURL -> UnSubscribe lifecycle with a regression test.
|
Contributor
Author
done |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes #3615
Problem. With application-level service discovery, a single transient
MetadataServicefetch failure during a provider restart could leave the consumer directory permanently empty (No provider available) until the consumer was restarted. Root cause chain, confirmed by a deterministic docker reproduction (details in this comment):ServiceInstancesChangedListenerImpl.OnEventskipped instances whose metadata fetch failed, overwrote the service URLs with the (possibly empty) result, and returned success. No retry queue, no error propagation — and since Nacos only pushes on instance-list changes, no later event ever retried the fetch.RegistryDirectoryrefused to rebuild the invoker: the graceful shutdown of the provider had left a closing tombstone on the instance key (default TTL 30s), and a genuine same-address restart was indistinguishable from a stale pre-shutdown registry snapshot.Fix (two layers, both required — verified by e2e):
service_instances_changed_listener_impl.go): failed revisions are tracked inunresolvedRevisionsand re-resolved by replaying the latest instance snapshot through the existing build path, using one shared timer per listener with capped exponential backoff (1s→30s, 25% jitter, unlimited attempts — a retry cap would re-introduce the permanent failure this fixes). Retries stop naturally when instances leave the snapshot, when the last subscriber is removed, or on registryDestroy()(service_discovery_registry.go). Metadata RPCs no longer run under the listener state mutex (build is serialized by a separatebuildMu; the state mutex only guards field access), so a slow/dead provider cannot block event processing.directory.go): the closing tombstone now records the closing invoker's exporttimestamp. A re-add carrying the same timestamp is a stale pre-shutdown snapshot and stays vetoed (existing graceful-shutdown semantics unchanged); a different timestamp proves a genuine restart, so the tombstone is cleared and the invoker rebuilt immediately. URLs without a timestamp (older providers) keep the conservative veto.Known limitation: export
timestampis second-precision in dubbo-go (server/action.go), so a stop+restart+export+register completing within the same second would still be vetoed (degrading to pre-fix behavior until the next registry event). This is practically unreachable — graceful stop alone takes seconds. If it ever matters, the principled follow-up is discriminating by metadatarevisionor aligning the timestamp to milliseconds (Java Dubbo usesSystem.currentTimeMillis()), which would be a separate, framework-wide discussion.Out of scope (tracked separately): application-level
AddListenerfailure is likewise never retried — same recovery-gap family, different layer; see #3624. Observability for unresolved revisions (e.g. a gauge) belongs to #3356.Verification:
metadata_retry_test.go,directory_test.go): recovery without any further registry event; retries stop when the instance is removed; only the latest revision is retried; one shared timer across repeated events; timer cancelled on subscriber detach and resumed on re-attach; tombstone vetoes stale re-adds but allows genuine restarts.go test ./registry/...and the listener/directory tests with-racepass; broader regression overregistry/metadata/cluster/server/client/commonpasses.No provider availablepermanently.Checklist
develop