Address cubic review: multi-subscriber config-change callbacks + deterministic dispatch

- Provider now supports multiple on_config_change subscribers (add_on_config_change)
  so Logfire instances sharing a provider via with_settings() don't clobber each
  other's variable change notifications (cubic P2)
- Sort changed names before dispatch for deterministic callback order (cubic P3)
- Add regression test for the shared-provider scenario

Author: dmontagu

logfire/_internal/main.py

@@ -2586,12 +2586,12 @@ def _setup_variable_change_notifications(self) -> None:
         provider = self.config.get_variable_provider()
 
         def on_config_change(changed_names: set[str]) -> None:
-            for name in changed_names:
+            for name in sorted(changed_names):
                 variable = self._variables.get(name)
                 if variable is not None:
                     variable._notify_change()  # pyright: ignore[reportPrivateUsage]
 
-        provider.set_on_config_change(on_config_change)
+        provider.add_on_config_change(on_config_change)
 
     def variables_clear(self) -> None:
         """Clear all registered variables from this Logfire instance.

logfire/variables/abstract.py

@@ -640,25 +640,33 @@ def _update_variable_schema(
 class VariableProvider(ABC):
     """Abstract base class for variable value providers."""
 
-    _on_config_change: Callable[[set[str]], None] | None = None
+    _on_config_change_callbacks: list[Callable[[set[str]], None]] | None = None
 
-    def set_on_config_change(self, callback: Callable[[set[str]], None]) -> None:
-        """Set a callback that will be called when variable configurations change.
+    def add_on_config_change(self, callback: Callable[[set[str]], None]) -> None:
+        """Register a callback to be called when variable configurations change.
+
+        Multiple callbacks may be registered. This is important because several
+        `Logfire` instances can share a single provider (e.g. via `with_settings()`),
+        each with its own set of registered variables, and all of them need to be
+        notified of changes.
 
         Args:
             callback: Called with the set of variable names whose configs changed.
         """
-        self._on_config_change = callback
+        if self._on_config_change_callbacks is None:
+            self._on_config_change_callbacks = []
+        self._on_config_change_callbacks.append(callback)
 
     def _notify_config_change(self, changed_names: set[str]) -> None:
-        """Notify the registered callback about changed variables."""
-        if self._on_config_change and changed_names:
-            try:
-                self._on_config_change(changed_names)
-            except Exception:
-                import logging
-
-                logging.getLogger('logfire').exception('Error in on_config_change callback')
+        """Notify all registered callbacks about changed variables."""
+        if self._on_config_change_callbacks and changed_names:
+            for callback in self._on_config_change_callbacks:
+                try:
+                    callback(changed_names)
+                except Exception:
+                    import logging
+
+                    logging.getLogger('logfire').exception('Error in on_config_change callback')
 
     @abstractmethod
     def get_serialized_value(

tests/test_variables.py

@@ -3991,6 +3991,75 @@ def on_b_change():  # pyright: ignore[reportUnusedFunction]
         assert a_changes == ['a_changed']
         assert b_changes == []
 
+    def test_on_change_fires_for_variables_on_shared_provider_instances(self, config_kwargs: dict[str, Any]):
+        """Variables registered on different Logfire instances sharing a provider all get notified.
+
+        `with_settings()` returns a new Logfire instance that shares the same config (and
+        therefore the same variable provider) but has its own `_variables` map. Registering
+        variables on both instances must not cause one to clobber the other's change
+        notifications.
+        """
+        config = VariablesConfig(
+            variables={
+                'var_a': VariableConfig(
+                    name='var_a',
+                    labels={'x': LabeledValue(version=1, serialized_value='"a_value"')},
+                    rollout=Rollout(labels={'x': 1.0}),
+                    overrides=[],
+                ),
+                'var_b': VariableConfig(
+                    name='var_b',
+                    labels={'x': LabeledValue(version=1, serialized_value='"b_value"')},
+                    rollout=Rollout(labels={'x': 1.0}),
+                    overrides=[],
+                ),
+            }
+        )
+        config_kwargs['variables'] = LocalVariablesOptions(config=config)
+        lf = logfire.configure(**config_kwargs)
+        provider = lf.config.get_variable_provider()
+        assert isinstance(provider, LocalVariableProvider)
+
+        # var_a is registered on the default instance...
+        var_a = lf.var('var_a', default='default', type=str)
+        # ...and var_b on a `with_settings()` instance that shares the same provider.
+        lf2 = lf.with_settings(tags=['other'])
+        var_b = lf2.var('var_b', default='default', type=str)
+
+        a_changes: list[str] = []
+        b_changes: list[str] = []
+
+        @var_a.on_change
+        def on_a_change():  # pyright: ignore[reportUnusedFunction]
+            a_changes.append('a_changed')
+
+        @var_b.on_change
+        def on_b_change():  # pyright: ignore[reportUnusedFunction]
+            b_changes.append('b_changed')
+
+        # Updating var_a must still fire its callback even though lf2 registered later.
+        provider.update_variable(
+            'var_a',
+            VariableConfig(
+                name='var_a',
+                labels={'x': LabeledValue(version=1, serialized_value='"new_a"')},
+                rollout=Rollout(labels={'x': 1.0}),
+                overrides=[],
+            ),
+        )
+        provider.update_variable(
+            'var_b',
+            VariableConfig(
+                name='var_b',
+                labels={'x': LabeledValue(version=1, serialized_value='"new_b"')},
+                rollout=Rollout(labels={'x': 1.0}),
+                overrides=[],
+            ),
+        )
+
+        assert a_changes == ['a_changed']
+        assert b_changes == ['b_changed']
+
 
 # =============================================================================
 # Tests for additional coverage
@@ -4156,7 +4225,7 @@ def bad_callback(changed_names: set[str]) -> None:
             called = True
             raise RuntimeError('Callback failed!')
 
-        provider.set_on_config_change(bad_callback)
+        provider.add_on_config_change(bad_callback)
 
         # The _notify_config_change method catches exceptions and logs them
         provider._notify_config_change({'test'})
@@ -4381,7 +4450,7 @@ def test_change_detection_notifies_callback(self):
             )
             try:
                 changed_vars: list[set[str]] = []
-                provider.set_on_config_change(lambda names: changed_vars.append(names))
+                provider.add_on_config_change(lambda names: changed_vars.append(names))
 
                 # First fetch
                 provider.refresh(force=True)
@@ -4418,7 +4487,7 @@ def test_no_notification_when_config_unchanged(self):
             )
             try:
                 changed_vars: list[set[str]] = []
-                provider.set_on_config_change(lambda names: changed_vars.append(names))
+                provider.add_on_config_change(lambda names: changed_vars.append(names))
 
                 provider.refresh(force=True)
                 provider.refresh(force=True)