feat(metadata): Clarify metadata report selection for multi-registry and multi-instance scenarios (#3369)

* fix(metadata): make GetMetadataReport() deterministic in multi-registry setup

* fix(metadata): Remove() fans out to all reports, consistent with Map()

* fix(metadata): thread registryId through GetMetadataFromMetadataReport call chain

* fix(metadata): scope revision calculation to per-registry services, thread registryId through createInstance

* fix(metadata): GetMetadataReportByRegistry returns nil for unknown registryId instead of silently falling back to wrong registry

When a caller provides a specific non-empty registryId that has no
registered report, the previous implementation silently fell back to
GetMetadataReport() and returned an arbitrary report from a different
registry. This undermined the per-registry scoping added in the earlier
commits: metadata for registry A could be fetched from registry B's
report without any indication.

Fix: return nil (with a Warnf log) when a specific registryId is not
found. The existing nil-guard in GetMetadataFromMetadataReport (client.go)
already surfaces this as an error to the caller, which is the correct
behaviour.

The empty-string path is unchanged: it still delegates to
GetMetadataReport() because callers that pass "" have no registry
context and should receive the stable default.

Also expose ClearMetadataReportInstances() to allow cross-package test
isolation of the package-level instances map.

* fix(metadata): ServiceNameMapping.Remove collects all errors instead of returning only the last one

The previous implementation used a lastErr variable that was overwritten
on each iteration. If the first report failed and the second succeeded,
the error was silently discarded and the caller saw no error despite a
partial failure — leaving a stale mapping in the first registry.

Fix: collect all errors with errors.Join so the caller receives the full
failure picture. The loop continues past individual failures (best-effort
fan-out) so a transient error in one registry does not prevent removal
from the others.

Add a comment explaining the intentional behavioural difference from
Map(), which is fail-fast: Map stops at the first report failure, while
Remove continues and collects all errors so every reachable registry gets
the removal attempt.

* test: expand coverage for per-registry metadata report selection

Three gaps identified in PR review, all addressed:

1. metadata/client_test.go — rewrite TestGetMetadataFromMetadataReport
   - Add sub-test: specific registryId routes to its own report (not default)
   - Add sub-test: unknown registryId returns error, not silent wrong-registry result
   - Each sub-test now resets instances independently to prevent state leakage

2. registry/servicediscovery/customizer/service_revision_customizer_test.go
   - Add TestExportedRevisionMissingRegistryIdYieldsZero: instance with no
     RegistryIdKey gets revision "0" and does not borrow another registry's
     service list
   - Add TestSubscribedRevisionMissingRegistryIdYieldsZero: same for subscribed
   - Add t.Cleanup(ClearMetadataReportInstances) + unique registry-id prefixes
     to all four tests to prevent cross-test global map pollution

3. registry/servicediscovery/service_instances_changed_listener_impl_test.go
   - Add TestListenerUsesRegistryIdToFetchRemoteMetadata: creates a listener
     with registryId="remote-reg-test", registers a mock MetadataReport under
     that id, triggers OnEvent with a RemoteMetadataStorageType instance, and
     asserts via mock.AssertExpectations that the correct per-registry report
     was called — end-to-end proof that registryId threads from
     NewServiceInstancesChangedListener through to GetMetadataFromMetadataReport

* chore: translate Chinese comments to English, remove UTF-8 BOM

- metadata/report_instance_test.go: translate inline comments and
  testify assertion messages in TestGetMetadataReportIsDeterministic
  from Chinese to English, consistent with the rest of the file

- registry/servicediscovery/customizer/service_revision_customizer.go:
  translate Customize doc-comments for both exported and subscribed
  customizers from Chinese to English

- registry/servicediscovery/service_instances_changed_listener_impl_test.go:
  strip the UTF-8 BOM (EF BB BF) that was introduced at line 1;
  Go toolchain accepts BOMs but they are not idiomatic and cause
  spurious diffs in some editors and CI tools

* style(metadata): fix import formatting

* fix(registry): upgrade missing-registryId log from Warn to Error with accurate consequence

The previous Warnf said 'revision will be empty', which is wrong on two
counts: the actual computed revision is "0" (not empty), and revision
"0" causes OnEvent to silently skip the instance entirely, making it
permanently invisible to all consumers.

Upgrade to Errorf and describe the actual outcome so operators can
diagnose misconfigurations immediately. Applies to both exported and
subscribed revision customizers.

* fix(registry): scope metaCache key to (registryId, revision) to prevent cross-registry metadata poisoning

The cache in GetMetadataInfo was keyed by revision string only. In a
multi-registry setup, if two registries happen to produce the same
revision value (e.g. identical service sets yielding the same CRC, or
test fixtures using literal revision strings), the second registry's
listener would get a cache hit and return MetadataInfo fetched via a
different registry's report — silently serving wrong endpoints with no
error surfaced.

Fix: use registryId+":"+revision as the cache key for both Get and Set,
so each registry's metadata is cached independently.

Update all test helpers and direct metaCache.Set/Delete calls to use
the same composite key (with constant.DefaultKey as the registryId for
all existing single-registry test listeners).

* docs(registry): restore TODO for multi-instance metadata service URL alignment

The previous commit removed the TODO without fixing the underlying issue.
GetMetadataService() still returns a global singleton, so all instances
across registries receive the same metadata service URL regardless of
which registry they belong to.

Restore the TODO with a more detailed explanation so the gap is visible
to future contributors. The fix requires per-registry MetadataService
support and is out of scope for this PR.

* Revert "docs(registry): restore TODO for multi-instance metadata service URL alignment"

This reverts commit 5d7bcc6.

* style(metadata): fix import formatting

* fix(debug) : fix review problems

* style(metadata): fix format issue && revert some changes

* fix(metadata): update tests and comment to match default fallback in GetMetadataReportByRegistry

GetMetadataReportByRegistry intentionally falls back to the 'default'
report when a specific registryId is not found. This supports standalone
metadata-report configs registered under 'default' being used by named
registries (e.g. nacos, zk). Update tests and doc comment to reflect
this behavior instead of the old 'return nil for unknown id' contract.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* fix: address review comments on clarify-metadata-report-selection

- fix(registry): unify registryId default to constant.DefaultKey before
  GetMetadataInfo lookup in RegisterService, eliminating empty-string
  fallback mismatch and redundant GetParam calls
- fix(metadata): scope metaCache key to app:registryId:revision to
  prevent cross-registry cache collisions
- fix(metadata/mapping): bind MappingListener to deterministic primary
  report via GetMetadataReport() instead of non-deterministic i==0
- fix(metadata): include registryId in GetMetadataFromMetadataReport
  error message for easier misconfiguration diagnosis
- test: add t.Cleanup to remove AddService/AddSubscribeURL entries and
  prevent global state leakage between tests
- docs: restore TODO comment explaining GetMetadataService() singleton
  limitation in multi-registry metadata service URL alignment

* fix: fix test isolation and cache key format after review changes

- fix(test): update metaCache keys in service_instances_changed_listener_impl_test
  to match new app:registryId:revision format
- test: use t.Name()+UnixNano as regID in service_discovery_registry_test
  to prevent cross-test global state pollution without expanding production API

---------

Co-authored-by: xnlemon <xianingawa@gmail.com>
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>

Author: 3 people

metadata/client.go

@@ -42,10 +42,10 @@ import (
 
 const defaultTimeout = "5s" // s
 
-func GetMetadataFromMetadataReport(revision string, instance registry.ServiceInstance) (*info.MetadataInfo, error) {
-	report := GetMetadataReport()
+func GetMetadataFromMetadataReport(revision string, instance registry.ServiceInstance, registryId string) (*info.MetadataInfo, error) {
+	report := GetMetadataReportByRegistry(registryId)
 	if report == nil {
-		return nil, perrors.New("no metadata report instance found,please check ")
+		return nil, perrors.Errorf("no metadata report instance found for registryId=%s, please check metadata-report configuration", registryId)
 	}
 	return report.GetAppMetadata(instance.GetServiceName(), revision)
 }

metadata/client_test.go

@@ -35,6 +35,7 @@ import (
 	"dubbo.apache.org/dubbo-go/v3/common/constant"
 	"dubbo.apache.org/dubbo-go/v3/common/extension"
 	"dubbo.apache.org/dubbo-go/v3/metadata/info"
+	"dubbo.apache.org/dubbo-go/v3/metadata/report"
 	"dubbo.apache.org/dubbo-go/v3/protocol/base"
 	"dubbo.apache.org/dubbo-go/v3/protocol/result"
 	_ "dubbo.apache.org/dubbo-go/v3/proxy/proxy_factory"
@@ -62,22 +63,64 @@ var (
 )
 
 func TestGetMetadataFromMetadataReport(t *testing.T) {
+	t.Cleanup(func() { instances = make(map[string]report.MetadataReport) })
+
 	t.Run("no report instance", func(t *testing.T) {
-		_, err := GetMetadataFromMetadataReport("1", ins)
+		instances = make(map[string]report.MetadataReport)
+		_, err := GetMetadataFromMetadataReport("1", ins, "default")
 		require.Error(t, err)
 	})
-	mockReport := new(mockMetadataReport)
-	defer mockReport.AssertExpectations(t)
-	instances["default"] = mockReport
-	t.Run("normal", func(t *testing.T) {
+
+	t.Run("default registry routes to default report", func(t *testing.T) {
+		instances = make(map[string]report.MetadataReport)
+		mockReport := new(mockMetadataReport)
+		defer mockReport.AssertExpectations(t)
+		instances["default"] = mockReport
+
 		mockReport.On("GetAppMetadata").Return(metadataInfo, nil).Once()
-		got, err := GetMetadataFromMetadataReport("1", ins)
+		got, err := GetMetadataFromMetadataReport("1", ins, "default")
+		require.NoError(t, err)
+		assert.Equal(t, metadataInfo, got)
+	})
+
+	t.Run("specific registryId routes to its own report", func(t *testing.T) {
+		instances = make(map[string]report.MetadataReport)
+		defaultReport := new(mockMetadataReport)
+		specificReport := new(mockMetadataReport)
+		defer defaultReport.AssertExpectations(t)
+		defer specificReport.AssertExpectations(t)
+		instances["default"] = defaultReport
+		instances["reg-a"] = specificReport
+
+		// specificReport must be called; defaultReport must NOT be called
+		specificReport.On("GetAppMetadata").Return(metadataInfo, nil).Once()
+		got, err := GetMetadataFromMetadataReport("1", ins, "reg-a")
+		require.NoError(t, err)
+		assert.Equal(t, metadataInfo, got)
+	})
+
+	t.Run("unknown registryId falls back to default report", func(t *testing.T) {
+		instances = make(map[string]report.MetadataReport)
+		defaultReport := new(mockMetadataReport)
+		defer defaultReport.AssertExpectations(t)
+		instances["default"] = defaultReport
+
+		// When the specific registryId is not found, it falls back to "default"
+		// so the default report's GetAppMetadata is called
+		defaultReport.On("GetAppMetadata").Return(metadataInfo, nil).Once()
+		got, err := GetMetadataFromMetadataReport("1", ins, "nonexistent-registry")
 		require.NoError(t, err)
 		assert.Equal(t, metadataInfo, got)
 	})
-	t.Run("error", func(t *testing.T) {
+
+	t.Run("report error propagated", func(t *testing.T) {
+		instances = make(map[string]report.MetadataReport)
+		mockReport := new(mockMetadataReport)
+		defer mockReport.AssertExpectations(t)
+		instances["default"] = mockReport
+
 		mockReport.On("GetAppMetadata").Return(metadataInfo, errors.New("mock error")).Once()
-		_, err := GetMetadataFromMetadataReport("1", ins)
+		_, err := GetMetadataFromMetadataReport("1", ins, "default")
 		require.Error(t, err)
 	})
 }

metadata/mapping/metadata/service_name_mapping.go

@@ -119,18 +119,56 @@ func backoff(attempt int) time.Duration {
 // Get will return the application-level services. If not found, the empty set will be returned.
 func (d *ServiceNameMapping) Get(url *common.URL, listener mapping.MappingListener) (*gxset.HashSet, error) {
 	serviceInterface := url.GetParam(constant.InterfaceKey, "")
-	metadataReport := metadata.GetMetadataReport()
-	if metadataReport == nil {
+	metadataReports := metadata.GetMetadataReports()
+	if len(metadataReports) == 0 {
 		return nil, perrors.New("can not get mapping in remote cause no metadata report instance found")
 	}
-	return metadataReport.GetServiceAppMapping(serviceInterface, DefaultGroup, listener)
+	// Attach the listener to the stable primary report only (GetMetadataReport uses
+	// a deterministic selection: prefer "default", otherwise lexicographic first).
+	// GetMetadataReports() iterates a map so its order is non-deterministic; using
+	// i==0 as the anchor would bind the listener to a random backend each run.
+	primaryReport := metadata.GetMetadataReport()
+	var result *gxset.HashSet
+	var errs []error
+	for _, metadataReport := range metadataReports {
+		var reportListener mapping.MappingListener
+		if metadataReport == primaryReport {
+			reportListener = listener
+		}
+		set, err := metadataReport.GetServiceAppMapping(serviceInterface, DefaultGroup, reportListener)
+		if err != nil {
+			errs = append(errs, err)
+			continue
+		}
+		if result == nil {
+			result = set
+		} else {
+			result.Add(set.Values()...)
+		}
+	}
+	if result == nil {
+		return nil, errors.Join(errs...)
+	}
+	return result, nil
 }
 
+// Remove removes the service-to-app mapping for the given URL from all
+// registered metadata reports. Unlike Map (which stops on the first failure),
+// Remove is best-effort: it attempts every report and returns all errors
+// joined together so the caller can see the full failure picture. The
+// intent is to avoid leaving stale entries in any registry due to a transient
+// error in one of the others.
 func (d *ServiceNameMapping) Remove(url *common.URL) error {
 	serviceInterface := url.GetParam(constant.InterfaceKey, "")
-	metadataReport := metadata.GetMetadataReport()
-	if metadataReport == nil {
+	metadataReports := metadata.GetMetadataReports()
+	if len(metadataReports) == 0 {
 		return perrors.New("can not remove mapping in remote cause no metadata report instance found")
 	}
-	return metadataReport.RemoveServiceAppMappingListener(serviceInterface, DefaultGroup)
+	var errs []error
+	for _, metadataReport := range metadataReports {
+		if err := metadataReport.RemoveServiceAppMappingListener(serviceInterface, DefaultGroup); err != nil {
+			errs = append(errs, err)
+		}
+	}
+	return errors.Join(errs...)
 }

metadata/mapping/metadata/service_name_mapping_test.go

@@ -19,6 +19,7 @@ package metadata
 
 import (
 	"errors"
+	"sync"
 	"testing"
 )
 
@@ -147,6 +148,105 @@ func initMock() (*mockMetadataReport, error) {
 	return metadataReport, err
 }
 
+func initMockWithId(t *testing.T, registryId string) *mockMetadataReport {
+	t.Helper()
+	mockReport := new(mockMetadataReport)
+	extension.SetMetadataReportFactory(registryId, func() report.MetadataReportFactory {
+		return mockReport
+	})
+	opts := metadata.NewReportOptions(
+		metadata.WithRegistryId(registryId),
+		metadata.WithProtocol(registryId),
+		metadata.WithAddress("127.0.0.1"),
+	)
+	require.NoError(t, opts.Init())
+	return mockReport
+}
+
+func TestServiceNameMappingRemoveFansOutToAllReports(t *testing.T) {
+	metadata.ClearMetadataReportInstances()
+	t.Cleanup(metadata.ClearMetadataReportInstances)
+	serviceNameMappingOnce = sync.Once{}
+	serviceNameMappingInstance = nil
+
+	r1 := initMockWithId(t, "reg-a")
+	r2 := initMockWithId(t, "reg-b")
+
+	ins := GetNameMappingInstance()
+	serviceUrl := common.NewURLWithOptions(
+		common.WithInterface("org.example.FooService"),
+		common.WithParamsValue(constant.ApplicationKey, "foo-app"),
+	)
+
+	r1.On("RemoveServiceAppMappingListener").Return(nil).Once()
+	r2.On("RemoveServiceAppMappingListener").Return(nil).Once()
+
+	err := ins.Remove(serviceUrl)
+	require.NoError(t, err)
+	r1.AssertExpectations(t)
+	r2.AssertExpectations(t)
+}
+
+func TestServiceNameMappingRemoveCollectsAllErrors(t *testing.T) {
+	metadata.ClearMetadataReportInstances()
+	t.Cleanup(metadata.ClearMetadataReportInstances)
+	serviceNameMappingOnce = sync.Once{}
+	serviceNameMappingInstance = nil
+
+	r1 := initMockWithId(t, "reg-c")
+	r2 := initMockWithId(t, "reg-d")
+
+	ins := GetNameMappingInstance()
+	serviceUrl := common.NewURLWithOptions(
+		common.WithInterface("org.example.BarService"),
+		common.WithParamsValue(constant.ApplicationKey, "bar-app"),
+	)
+
+	err1 := errors.New("r1 failure")
+	err2 := errors.New("r2 failure")
+
+	// both reports fail
+	r1.On("RemoveServiceAppMappingListener").Return(err1).Once()
+	r2.On("RemoveServiceAppMappingListener").Return(err2).Once()
+
+	err := ins.Remove(serviceUrl)
+	require.Error(t, err)
+	// both individual errors must be present in the returned error
+	require.ErrorIs(t, err, err1)
+	require.ErrorIs(t, err, err2)
+	r1.AssertExpectations(t)
+	r2.AssertExpectations(t)
+}
+
+func TestServiceNameMappingRemoveContinuesAfterPartialFailure(t *testing.T) {
+	metadata.ClearMetadataReportInstances()
+	t.Cleanup(metadata.ClearMetadataReportInstances)
+	serviceNameMappingOnce = sync.Once{}
+	serviceNameMappingInstance = nil
+
+	r1 := initMockWithId(t, "reg-e")
+	r2 := initMockWithId(t, "reg-f")
+
+	ins := GetNameMappingInstance()
+	serviceUrl := common.NewURLWithOptions(
+		common.WithInterface("org.example.BazService"),
+		common.WithParamsValue(constant.ApplicationKey, "baz-app"),
+	)
+
+	removeErr := errors.New("r1 partial failure")
+
+	// r1 fails, r2 succeeds — the loop must not short-circuit
+	r1.On("RemoveServiceAppMappingListener").Return(removeErr).Once()
+	r2.On("RemoveServiceAppMappingListener").Return(nil).Once()
+
+	err := ins.Remove(serviceUrl)
+	// the joined error only contains r1's error; the call should error
+	require.ErrorIs(t, err, removeErr)
+	// both reports must have been called despite r1's failure
+	r1.AssertExpectations(t)
+	r2.AssertExpectations(t)
+}
+
 type listener struct {
 }
 

metadata/report_instance.go

@@ -18,11 +18,12 @@
 package metadata
 
 import (
+	"sort"
 	"time"
 )
 
 import (
-	"github.com/dubbogo/gost/container/set"
+	gxset "github.com/dubbogo/gost/container/set"
 	"github.com/dubbogo/gost/log/logger"
 )
 
@@ -41,6 +42,12 @@ var (
 	instances = make(map[string]report.MetadataReport)
 )
 
+// ClearMetadataReportInstances resets the package-level instances map.
+// Intended for test isolation only; do not call in production code.
+func ClearMetadataReportInstances() {
+	instances = make(map[string]report.MetadataReport)
+}
+
 func addMetadataReport(registryId string, url *common.URL) error {
 	fac := extension.GetMetadataReportFactory(url.Protocol)
 	if fac == nil {
@@ -51,21 +58,46 @@ func addMetadataReport(registryId string, url *common.URL) error {
 	return nil
 }
 
+// GetMetadataReport returns a single metadata report for callers that lack
+// registry context. It prefers the "default" registry's report; when absent
+// it falls back to the lexicographically first registry id so the selection
+// is always stable across calls.
 func GetMetadataReport() report.MetadataReport {
-	for _, v := range instances {
-		return v
+	if r, ok := instances[constant.DefaultKey]; ok {
+		return r
+	}
+	keys := make([]string, 0, len(instances))
+	for k := range instances {
+		keys = append(keys, k)
+	}
+	sort.Strings(keys)
+	if len(keys) > 0 {
+		return instances[keys[0]]
 	}
 	return nil
 }
 
+// GetMetadataReportByRegistry returns the metadata report bound to the given
+// registry id. When the registry id is empty the caller has no registry context,
+// so the stable default returned by GetMetadataReport is used. When a specific
+// (non-empty) registry id is not found, it falls back to the "default" report
+// if one exists. This handles the common case where a standalone metadata-report
+// config is registered under "default" while named registries (e.g. nacos, zk)
+// need to use it. nil is returned only when neither the specific id nor "default"
+// is registered.
 func GetMetadataReportByRegistry(registry string) report.MetadataReport {
 	if len(registry) == 0 {
-		registry = constant.DefaultKey
+		return GetMetadataReport()
 	}
 	if r, ok := instances[registry]; ok {
 		return r
 	}
-	return GetMetadataReport()
+	if r, ok := instances[constant.DefaultKey]; ok {
+		logger.Infof("[Metadata] no metadata report bound to registryId=%s, falling back to default", registry)
+		return r
+	}
+	logger.Warnf("[Metadata] no metadata report found for registryId=%s", registry)
+	return nil
 }
 
 func GetMetadataReports() []report.MetadataReport {