fix(sdk): reject a factory class at registration

plugins=[MyFactory] -- the factory class rather than an instance of it --
passed the registration check, because create_plugin read off the class
is a plain function and therefore callable. The per-invocation call
supplies only the info, Python binds it to self, and the resulting
TypeError is contained like any other factory failure: instrumentation is
silently absent for the lifetime of the function.

Registration now binds one positional argument to the member's signature,
which no factory code runs. A bound method, a classmethod, a staticmethod
and a __call__ on an instance all bind; an instance method read off the
class does not. A callable whose signature cannot be read is accepted on
the member alone, because a missing description of a factory is not
evidence of a broken one.

Author: ParidelPooya

packages/aws-durable-execution-sdk-python/src/aws_durable_execution_sdk_python/plugin_discovery.py

@@ -1,5 +1,6 @@
 from __future__ import annotations
 
+import inspect
 import logging
 import os
 from collections.abc import Mapping, Sequence
@@ -17,6 +18,10 @@
 PLUGIN_ENTRY_POINT_GROUP = "aws_durable_execution.plugins"
 PLUGIN_ENVIRONMENT_VARIABLE = "DURABLE_EXECUTION_PLUGINS"
 
+# Stands in for the InvocationStartInfo when a factory's signature is checked at
+# registration. Only the bind is performed, so nothing reads it.
+_ARGUMENT_PROBE = object()
+
 
 def _parse_configured_plugin_names(environment: Mapping[str, str]) -> list[str]:
     configured_plugins = environment.get(PLUGIN_ENVIRONMENT_VARIABLE)
@@ -58,21 +63,57 @@ def _is_plugin_factory(value: object) -> bool:
     """Report whether a value has the shape of a plugin factory.
 
     :class:`DurableInstrumentationPluginFactory` declares one method, so the
-    shape is one member: a callable ``create_plugin``. The attribute is fetched
-    and tested for callability rather than merely for presence, because an
-    object carrying a non-callable ``create_plugin`` would otherwise pass here
-    and fail at invocation time.
+    shape is one member: a ``create_plugin`` that can be called with one
+    positional argument. The attribute is fetched and tested rather than merely
+    checked for presence, because an object carrying a non-callable
+    ``create_plugin`` would otherwise pass here and fail at invocation time.
+
+    Callability alone is not enough. ``plugins=[MyFactory]`` -- the factory
+    *class* rather than an instance of it -- resolves ``create_plugin`` to a
+    plain function whose first parameter is ``self``, which is callable. The
+    per-invocation call supplies only the info, Python binds it to ``self``, and
+    the resulting :exc:`TypeError` is contained like any other factory failure:
+    telemetry is silently absent for the lifetime of the function. Binding one
+    positional argument to the signature rejects that at registration instead.
+    The bind is a signature operation, so no factory code runs.
+
+    A callable with no introspectable signature -- a C-implemented callable, for
+    example -- is accepted on the member alone. ``inspect.signature`` raises for
+    it, and refusing a factory because its signature could not be read would
+    reject a usable factory over a missing description of it.
 
     Structural rather than nominal, so a factory need not import the SDK
     protocol to satisfy it. The protocol is deliberately not
     ``@runtime_checkable``; see its docstring.
 
-    The check cannot go further than one member. Whether ``create_plugin``
-    accepts the info, and whether it returns a plugin, is only knowable by
-    calling it, and calling it at load time is what the per-invocation factory
-    design avoids: there is no invocation yet.
+    The check stops there. Whether ``create_plugin`` returns a plugin is only
+    knowable by calling it, and calling it at load time is what the
+    per-invocation factory design avoids: there is no invocation yet. That case
+    is checked per invocation by :meth:`PluginExecutor._create_plugins`.
     """
-    return callable(getattr(value, "create_plugin", None))
+    create_plugin = getattr(value, "create_plugin", None)
+    if not callable(create_plugin):
+        return False
+    return _accepts_one_positional_argument(create_plugin)
+
+
+def _accepts_one_positional_argument(create_plugin: object) -> bool:
+    """Report whether one positional argument can be bound to a callable.
+
+    A bound method, a ``@classmethod`` or ``@staticmethod`` read off a class, and
+    a ``__call__`` on an instance all present the signature the SDK calls, so all
+    three bind. An instance method read off the class does not: its first
+    parameter is ``self``, so one argument leaves the info unbound.
+    """
+    try:
+        signature = inspect.signature(create_plugin)  # type: ignore[arg-type]
+    except (TypeError, ValueError):
+        return True
+    try:
+        signature.bind(_ARGUMENT_PROBE)
+    except TypeError:
+        return False
+    return True
 
 
 def _load_factory(
@@ -103,7 +144,7 @@ def _load_factory(
             "create_plugin(info) method returning a "
             "DurableInstrumentationPlugin -- but resolved to "
             f"{_qualified_type_name(factory)}. Name the factory instance, not a "
-            "plugin and not a plugin class."
+            "plugin, not a plugin class, and not the factory class."
         )
 
     return cast(DurableInstrumentationPluginFactory, factory)
@@ -126,11 +167,15 @@ def _validate_explicit_factories(
     A plugin *class* is rejected, and so is any bare callable. Both were
     accepted while the registration type was ``Callable``: a lambda satisfied it
     directly, and a class satisfied it because calling a class constructs an
-    instance. Neither carries ``create_plugin``, so ``plugins=[MyPlugin]`` and
-    ``plugins=[lambda info: MyPlugin()]`` now fail here. The replacement is a
-    small factory class, which is also where setup work that can fail belongs. A
-    class that declares ``create_plugin`` as a ``@classmethod`` is accepted,
-    because the requirement is the member and not the kind of object.
+    instance. Neither carries a ``create_plugin`` the SDK can call, so
+    ``plugins=[MyPlugin]`` and ``plugins=[lambda info: MyPlugin()]`` now fail
+    here. A *factory* class passed instead of an instance of it fails here too:
+    ``MyFactory.create_plugin`` is callable, but its first parameter is ``self``,
+    so the per-invocation call binds the info to ``self``. The replacement is a
+    small factory class, instantiated, which is also where setup work that can
+    fail belongs. A class that declares ``create_plugin`` as a ``@classmethod``
+    or a ``@staticmethod`` is accepted, because that member presents the
+    signature the SDK calls.
     """
     factories = list(explicit_plugins or [])
     for index, factory in enumerate(factories):
@@ -140,7 +185,8 @@ class that declares ``create_plugin`` as a ``@classmethod`` is accepted,
                 "plugin factory -- an object with a create_plugin(info) method "
                 "returning a DurableInstrumentationPlugin -- but is "
                 f"{_qualified_type_name(factory)}. Pass a factory rather than a "
-                "plugin, a plugin class, or a plain callable, for example "
+                "plugin, a plugin class, or a plain callable, and pass a factory "
+                "instance rather than the factory class, for example "
                 "plugins=[MyPluginFactory(exporter)]."
             )
     return factories

packages/aws-durable-execution-sdk-python/tests/plugin_discovery_test.py

@@ -1,7 +1,9 @@
 from __future__ import annotations
 
+import inspect
 import logging
 import os
+import time
 from unittest.mock import Mock, patch
 
 import pytest
@@ -577,6 +579,95 @@ def create_plugin(cls, info: InvocationStartInfo) -> _ClassFactoryPlugin:
     assert plugin.info is INVOCATION_START_INFO
 
 
+def test_explicit_factory_class_with_an_instance_method_is_rejected() -> None:
+    """The factory class is not the factory, and callability does not reveal it.
+
+    ``MyFactory.create_plugin`` read off the class is a plain function whose
+    first parameter is ``self``, so it is callable and used to pass. The
+    per-invocation call supplies only the info, Python binds it to ``self``, and
+    the resulting ``TypeError`` is contained like any other factory failure:
+    instrumentation is silently absent for the lifetime of the function. The
+    signature is bound at registration so the mistake fails here instead.
+    """
+    with pytest.raises(PluginLoadError) as error:
+        load_configured_plugins([_PluginAFactory], environment={})  # type: ignore[list-item]
+
+    assert "plugins[0]" in str(error.value)
+    assert "a factory instance rather than the factory class" in str(error.value)
+
+
+def test_discovery_rejects_a_factory_class_at_the_entry_point() -> None:
+    """The entry-point path applies the same signature check."""
+    entry_point = _FakeEntryPoint("a", _PluginAFactory)
+
+    with (
+        patch(
+            "aws_durable_execution_sdk_python.plugin_discovery.metadata.entry_points",
+            return_value=[entry_point],
+        ),
+        pytest.raises(PluginLoadError) as error,
+    ):
+        load_configured_plugins(
+            None,
+            environment={PLUGIN_ENVIRONMENT_VARIABLE: "a"},
+        )
+
+    assert "must resolve to a plugin factory" in str(error.value)
+    assert "not the factory class" in str(error.value)
+
+
+def test_explicit_class_declaring_a_static_create_plugin_is_accepted() -> None:
+    """A ``@staticmethod`` presents the signature the SDK calls, so it binds."""
+
+    class _StaticFactoryPlugin(DurableInstrumentationPlugin):
+        def __init__(self, info: InvocationStartInfo) -> None:
+            self.info = info
+
+        @staticmethod
+        def create_plugin(info: InvocationStartInfo) -> _StaticFactoryPlugin:
+            return _StaticFactoryPlugin(info)
+
+    result = load_configured_plugins([_StaticFactoryPlugin], environment={})
+
+    assert result == [_StaticFactoryPlugin]
+    assert isinstance(
+        result[0].create_plugin(INVOCATION_START_INFO), _StaticFactoryPlugin
+    )
+
+
+def test_explicit_factory_without_an_introspectable_signature_is_accepted() -> None:
+    """A signature that cannot be read is not evidence of a broken factory.
+
+    ``inspect.signature`` raises ``ValueError`` for some C-implemented callables,
+    ``time.strftime`` among them. Rejecting such a factory would refuse a usable
+    one over a missing description of it, so the member alone decides.
+    """
+
+    class _UnreadableSignatureFactory:
+        create_plugin = staticmethod(time.strftime)
+
+    factory = _UnreadableSignatureFactory()
+
+    with pytest.raises(ValueError, match="no signature"):
+        inspect.signature(time.strftime)
+
+    assert load_configured_plugins([factory], environment={}) == [factory]  # type: ignore[list-item, comparison-overlap]
+
+
+def test_explicit_factory_taking_no_argument_is_rejected() -> None:
+    """A ``create_plugin`` that takes nothing cannot receive the info."""
+
+    class _NoArgumentFactory:
+        def create_plugin(self) -> _PluginA:
+            return _PluginA()
+
+    with pytest.raises(PluginLoadError) as error:
+        load_configured_plugins([_NoArgumentFactory()], environment={})  # type: ignore[list-item]
+
+    assert "plugins[0]" in str(error.value)
+    assert "create_plugin(info) method" in str(error.value)
+
+
 def test_explicit_factory_object_is_accepted() -> None:
     result = load_configured_plugins([_plugin_a_factory], environment={})