fix(idempotency): recognize registered wrappers through outer decorators (#1315)

Author: bokelley

src/adcp/server/idempotency/store.py

@@ -87,16 +87,29 @@ def _finish_supervised_operation(task: asyncio.Task[Any]) -> None:
 
 
 def is_wrapped(fn: Any) -> bool:
-    """Return True if ``fn`` was produced by :meth:`IdempotencyStore.wrap`.
+    """Return whether a callable's chain contains an SDK-registered wrapper.
 
-    Accepts bound methods (resolves to the underlying function before
-    the membership check) and plain callables. Used by the boot-time
-    validator at :mod:`adcp.decisioning.validate_idempotency`.
+    Accepts bound methods and plain callables. Outer decorators must use
+    ``functools.wraps`` (or set ``__wrapped__``) so the registered wrapper
+    remains reachable. Copied attributes alone do not establish membership.
+    Used by :mod:`adcp.decisioning.validate_idempotency`.
     """
-    if fn is None:
-        return False
-    target = fn.__func__ if hasattr(fn, "__func__") else fn
-    return target in _WRAPPED_FUNCTIONS
+    seen: set[int] = set()
+    # Bound the traversal to guard against pathological __wrapped__ cycles
+    # and chains that manufacture a new callable on every attribute access.
+    for _ in range(16):
+        target = getattr(fn, "__func__", fn)
+        if target is None or id(target) in seen:
+            return False
+        seen.add(id(target))
+        try:
+            if target in _WRAPPED_FUNCTIONS:
+                return True
+        except TypeError:
+            # Unhashable/non-weak-referenceable objects cannot be registered.
+            pass
+        fn = getattr(target, "__wrapped__", None)
+    return False
 
 
 # Spec bounds from capabilities.idempotency.replay_ttl_seconds (1h-7d).
@@ -364,13 +377,9 @@ def _log_late_failure(task: asyncio.Task[Any]) -> None:
         # registered, not the original handler: re-decorating a forked
         # copy of `handler` would otherwise falsely flag both.
         #
-        # Contract for future maintainers: ``is_wrapped()`` checks
-        # WeakSet membership of the closure object directly. Do NOT
-        # change it to ``inspect.unwrap()``-then-check — the
-        # ``@functools.wraps(handler)`` decorator above sets
-        # ``_wrapped.__wrapped__ = handler``, so ``inspect.unwrap``
-        # would return the original handler (not in the WeakSet) and
-        # the validator would silently regress.
+        # ``is_wrapped`` checks registry membership at each __wrapped__ link.
+        # Full inspect.unwrap()-then-check would reach the original handler,
+        # which is not registered, and miss this closure under outer decorators.
         _WRAPPED_FUNCTIONS.add(_wrapped)
         return _wrapped
 

tests/test_validate_idempotency_wiring.py

@@ -7,6 +7,9 @@
 
 from __future__ import annotations
 
+import functools
+from types import SimpleNamespace
+
 import pytest
 
 from adcp.decisioning import DecisioningCapabilities, DecisioningPlatform, SingletonAccounts
@@ -60,6 +63,77 @@ async def handler(self, params, context=None):
 
         assert is_wrapped(handler) is True
 
+    @pytest.mark.parametrize("depth", [1, 2, 15])
+    def test_outer_decorators_and_bound_methods(self, depth: int) -> None:
+        store = _store()
+
+        @store.wrap
+        async def handler(self, params, context=None):
+            return {}
+
+        for _ in range(depth):
+            inner = handler
+
+            @functools.wraps(inner)
+            async def outer(*args, _inner=inner, **kwargs):
+                return await _inner(*args, **kwargs)
+
+            handler = outer
+
+        class Seller:
+            capabilities = _caps_with_idempotency(supported=True)
+            create_media_buy = handler
+
+        assert is_wrapped(handler)
+        assert is_wrapped(Seller().create_media_buy)
+        validate_idempotency_wiring(Seller())
+
+    def test_mid_chain_bound_method(self) -> None:
+        store = _store()
+
+        class Seller:
+            @store.wrap
+            async def create_media_buy(self, params, context=None):
+                return {}
+
+        @functools.wraps(Seller().create_media_buy)
+        async def outer(*args, **kwargs):
+            return {}
+
+        assert is_wrapped(outer)
+
+    def test_outer_without_wrapped_link_is_not_registered(self) -> None:
+        store = _store()
+
+        @store.wrap
+        async def handler(self, params, context=None):
+            return {}
+
+        async def outer(*args, **kwargs):
+            return await handler(*args, **kwargs)
+
+        outer.__dict__.update(handler.__dict__)
+        outer.__dict__.pop("__wrapped__", None)
+        assert not is_wrapped(outer)
+
+    def test_cycle_and_depth_limit(self) -> None:
+        async def cycle():
+            return {}
+
+        cycle.__wrapped__ = cycle
+        assert not is_wrapped(cycle)
+        other = SimpleNamespace(__wrapped__=cycle)
+        cycle.__wrapped__ = other
+        assert not is_wrapped(cycle)
+        target = _store().wrap(cycle)
+        for _ in range(16):
+            target = SimpleNamespace(__wrapped__=target)
+        assert not is_wrapped(target)
+
+    @pytest.mark.parametrize("value", [[], {}, 42, object()])
+    def test_non_callable_values_are_not_registered(self, value) -> None:
+        assert not is_wrapped(value)
+
     def test_none_returns_false(self) -> None:
         assert is_wrapped(None) is False