diff --git a/docs/handler-authoring.md b/docs/handler-authoring.md index 54c8430fc..f7b3534c0 100644 --- a/docs/handler-authoring.md +++ b/docs/handler-authoring.md @@ -629,14 +629,18 @@ top-level `adcp.server`): ```python from adcp.server.idempotency import PgBackend -idempotency = IdempotencyStore(backend=PgBackend(pool=pg_pool), ttl_seconds=86_400) +idempotency = IdempotencyStore( + backend=PgBackend(pool=pg_pool, lock_pool=idempotency_lock_pool), + ttl_seconds=86_400, +) ``` -The Pg-backed store survives restarts and is shared across workers. -`PgBackend` commits the cached response atomically with your handler's -business write when both run inside the same transaction — no window -where the side effect lands -but the cache entry doesn't. +The Pg-backed store survives restarts and is shared across workers. Size the +dedicated `lock_pool` for the maximum number of concurrently executing unique +idempotent operations; cache-hit replays do not acquire it. The SDK commits +the cache entry while holding the per-key advisory lock, but it does not share +a transaction with unrelated handler business writes. Protect non-idempotent +business effects with a matching database uniqueness constraint. **`caller_identity` + `tenant_id` must be populated.** The store keys its cache on `(tenant_id, caller_identity, idempotency_key)`. If @@ -1079,14 +1083,23 @@ serve( "/var/lib/myagent/push_configs.db", allowed_destination_hosts=None, # public-HTTPS mode ), + push_sender=MyPushNotificationSender(...), ) ``` +The store controls subscription registration and discovery; the sender +delivers task updates. Configure both for built-in delivery. Supplying a +store without a sender remains supported for custom delivery pipelines, but +the SDK emits a startup warning because subscriptions would otherwise be +accepted without any notification being sent. Implement the a2a-sdk +`PushNotificationSender` interface when its base sender does not match your +HTTP-client lifecycle or tenant-isolation model. + Choose the destination policy explicitly: | Mode | Wiring | Behavior | |---|---|---| -| Disabled | Omit `push_config_store` | Agent card does not advertise push support; registration is unsupported. | +| Disabled | Omit `push_config_store` and `push_sender` | Agent card does not advertise push support; registration is unsupported. | | Public HTTPS | Pass a store with `allowed_destination_hosts=None` | Accept any HTTPS hostname that resolves only to public, non-reserved addresses. | | Allowlist | Pass a non-empty `frozenset` | Apply the public HTTPS/SSRF checks, then require an exact canonical hostname match. | diff --git a/release-please-config.json b/release-please-config.json index fcfc23ccf..cee59b352 100644 --- a/release-please-config.json +++ b/release-please-config.json @@ -4,9 +4,6 @@ "release-type": "python", "package-name": "adcp", "changelog-path": "CHANGELOG.md", - "versioning": "prerelease", - "prerelease-type": "rc", - "prerelease": true, "bump-minor-pre-major": true, "bump-patch-for-minor-pre-major": false, "include-component-in-tag": false diff --git a/scripts/generate_ergonomic_coercion.py b/scripts/generate_ergonomic_coercion.py index 3d95dbd79..ff0e126fe 100644 --- a/scripts/generate_ergonomic_coercion.py +++ b/scripts/generate_ergonomic_coercion.py @@ -7,7 +7,7 @@ Coercion patterns detected: 1. Enum fields -> coerce_to_enum -2. list[Enum] fields -> coerce_to_enum_list +2. list[Enum] fields -> coerce_to_enum_list (or order-preserving unique coercion) 3. ContextObject fields -> coerce_to_model(ContextObject) 4. ExtensionObject fields -> coerce_to_model(ExtensionObject) 5. list[BaseModel] fields -> coerce_subclass_list (for subclass variance) @@ -30,6 +30,7 @@ # These are the main request types users construct REQUEST_TYPES_TO_ANALYZE = [ "ListCreativeFormatsRequest", + "ListCreativeFormatsRequestCreativeAgent", "ListCreativesRequest", "PackageRequest", "CreateMediaBuyRequest", @@ -156,10 +157,17 @@ def analyze_model(model_class) -> list[dict]: annotation, lambda t: isinstance(t, type) and issubclass(t, Enum) ) if is_enum_list: + coercion_type = ( + "unique_enum_list" + if model_class.__name__ + in {"ListCreativeFormatsRequest", "ListCreativeFormatsRequestCreativeAgent"} + and field_name in {"disclosure_positions", "disclosure_persistence"} + else "enum_list" + ) coercions.append( { "field": field_name, - "type": "enum_list", + "type": coercion_type, "target_class": enum_type, } ) @@ -230,6 +238,9 @@ def generate_code() -> str: # Import all the types we need to analyze from pydantic import BaseModel as _PydBaseModel + from adcp.types.generated_poc.creative.list_creative_formats_request import ( + ListCreativeFormatsRequestCreativeAgent, + ) from adcp.types.generated_poc.creative.list_creatives_request import ( ListCreativesRequest, Sort, @@ -281,6 +292,7 @@ def _find_media_buy_success_variant(module: Any) -> type[_PydBaseModel] | None: # Map names to classes request_classes = { "ListCreativeFormatsRequest": ListCreativeFormatsRequest, + "ListCreativeFormatsRequestCreativeAgent": ListCreativeFormatsRequestCreativeAgent, "ListCreativesRequest": ListCreativesRequest, "PackageRequest": PackageRequest, "CreateMediaBuyRequest": CreateMediaBuyRequest, @@ -384,12 +396,13 @@ def _find_media_buy_success_variant(module: Any) -> type[_PydBaseModel] | None: "from collections.abc import Sequence", "from typing import Annotated, Any", "", - "from pydantic import BeforeValidator", + "from pydantic import BeforeValidator, WrapValidator", "", "from adcp.types.coercion import (", " coerce_subclass_list,", " coerce_to_enum,", " coerce_to_enum_list,", + " coerce_to_unique_enum_list,", " coerce_to_model,", ")", "", @@ -414,6 +427,9 @@ def _find_media_buy_success_variant(module: Any) -> type[_PydBaseModel] | None: lines.append("from adcp.types.generated_poc.media_buy.list_creative_formats_request import (") lines.append(" ListCreativeFormatsRequest,") lines.append(")") + lines.append("from adcp.types.generated_poc.creative.list_creative_formats_request import (") + lines.append(" ListCreativeFormatsRequestCreativeAgent,") + lines.append(")") lines.append("from adcp.types.generated_poc.creative.list_creatives_request import (") lines.append(" Field1 as ListCreativesField,") lines.append(" ListCreativesRequest,") @@ -499,6 +515,7 @@ def _find_media_buy_success_variant(module: Any) -> type[_PydBaseModel] | None: type_order = [ # Request types "ListCreativeFormatsRequest", + "ListCreativeFormatsRequestCreativeAgent", "ListCreativesRequest", "Sort", "GetProductsRequest", @@ -526,7 +543,7 @@ def _find_media_buy_success_variant(module: Any) -> type[_PydBaseModel] | None: if c["type"] == "enum": target = get_symbol_name(c["target_class"]) field_comments.append(f'{c["field"]}: {target} | str | None') - elif c["type"] == "enum_list": + elif c["type"] in {"enum_list", "unique_enum_list"}: target = get_symbol_name(c["target_class"]) field_comments.append(f'{c["field"]}: list[{target} | str] | None') elif c["type"] == "context": @@ -563,6 +580,13 @@ def _find_media_buy_success_variant(module: Any) -> type[_PydBaseModel] | None: f"BeforeValidator(coerce_to_enum({target}))]," ) lines.append(" )") + elif c["type"] == "unique_enum_list": + target = get_symbol_name(c["target_class"]) + lines.append(" _patch_unique_enum_list(") + lines.append(f" {type_name},") + lines.append(f' "{field}",') + lines.append(f" {target},") + lines.append(" )") elif c["type"] == "enum_list": target = get_symbol_name(c["target_class"]) lines.append(" _patch_field_annotation(") @@ -620,7 +644,18 @@ def _find_media_buy_success_variant(module: Any) -> type[_PydBaseModel] | None: # PackageUpdate is now a single class (flattened from validation-only oneOf) - # Add helper function + # Add helper functions + lines.append("") + lines.append("def _patch_unique_enum_list(") + lines.append(" model: Any,") + lines.append(" field_name: str,") + lines.append(" enum_class: type,") + lines.append(") -> None:") + lines.append(' """Add an order-preserving unique-items enum validator."""') + lines.append(" model.model_fields[field_name].metadata.append(") + lines.append(" WrapValidator(coerce_to_unique_enum_list(enum_class))") + lines.append(" )") + lines.append("") lines.append("") lines.append("def _patch_field_annotation(") lines.append(" model: type,") diff --git a/src/adcp/canonical_formats/references.py b/src/adcp/canonical_formats/references.py index 9f15643d9..065499475 100644 --- a/src/adcp/canonical_formats/references.py +++ b/src/adcp/canonical_formats/references.py @@ -42,6 +42,7 @@ DEFAULT_REFERENCE_TIMEOUT_SECONDS = 5.0 DEFAULT_REFERENCE_BODY_LIMIT_BYTES = 1024 * 1024 DEFAULT_MAX_SCHEMA_REFS = 256 +DEFAULT_MAX_SCHEMA_IDS = 256 DEFAULT_MAX_REF_DEPTH = 8 DEFAULT_MAX_SCHEMA_KEYWORDS = 10_000 DEFAULT_MAX_SCHEMA_DEPTH = 128 @@ -157,6 +158,7 @@ def __init__( timeout: float = DEFAULT_REFERENCE_TIMEOUT_SECONDS, max_body_bytes: int = DEFAULT_REFERENCE_BODY_LIMIT_BYTES, max_schema_refs: int = DEFAULT_MAX_SCHEMA_REFS, + max_schema_ids: int = DEFAULT_MAX_SCHEMA_IDS, max_ref_depth: int = DEFAULT_MAX_REF_DEPTH, max_schema_keywords: int = DEFAULT_MAX_SCHEMA_KEYWORDS, max_schema_depth: int = DEFAULT_MAX_SCHEMA_DEPTH, @@ -165,6 +167,7 @@ def __init__( self._timeout = timeout self._max_body_bytes = max_body_bytes self._max_schema_refs = max_schema_refs + self._max_schema_ids = max_schema_ids self._max_ref_depth = max_ref_depth self._max_schema_keywords = max_schema_keywords self._max_schema_depth = max_schema_depth @@ -318,6 +321,7 @@ def _validate_schema( document, base_uri=reference.uri, max_refs=self._max_schema_refs, + max_ids=self._max_schema_ids, max_ref_depth=self._max_ref_depth, max_keywords=self._max_schema_keywords, max_depth=self._max_schema_depth, @@ -534,11 +538,12 @@ def _validate_schema_refs( *, base_uri: str, max_refs: int, + max_ids: int, max_ref_depth: int, max_keywords: int, max_depth: int, ) -> str | None: - state = _SchemaRefState(max_refs=max_refs, max_keywords=max_keywords) + state = _SchemaRefState(max_refs=max_refs, max_ids=max_ids, max_keywords=max_keywords) return _walk_schema_refs( document, base_uri=base_uri, @@ -552,8 +557,10 @@ def _validate_schema_refs( @dataclass class _SchemaRefState: max_refs: int + max_ids: int max_keywords: int refs: int = 0 + ids: int = 0 keywords: int = 0 @@ -577,6 +584,9 @@ def _walk_schema_refs( if raw_id is not None: if not isinstance(raw_id, str): return "$id must be a string" + state.ids += 1 + if state.ids > state.max_ids: + return "format_schema exceeds $id count bound" id_error, resolved_id = _validate_schema_id_value(raw_id, base_uri=base_uri) if id_error is not None: return id_error @@ -709,6 +719,7 @@ def _trusted_aao_catalog_origin(parts: SplitResult) -> bool: "CanonicalReferenceResolver", "CanonicalReferenceResult", "CanonicalReferenceStatus", + "DEFAULT_MAX_SCHEMA_IDS", "DEFAULT_REFERENCE_BODY_LIMIT_BYTES", "DEFAULT_REFERENCE_TIMEOUT_SECONDS", "parse_canonical_reference", diff --git a/src/adcp/client.py b/src/adcp/client.py index 3fd4f8210..5cc84cf35 100644 --- a/src/adcp/client.py +++ b/src/adcp/client.py @@ -626,6 +626,7 @@ def __init__( self.adapter.idempotency_capability_check = self._ensure_idempotency_capability if signing is not None: self.adapter.signing_request_hook = self._sign_outgoing_request + self.adapter.signing_capability_check = self._prepare_signing_capabilities # Apply schema validation modes (default: requests=warn, responses=strict # in dev/test, warn in production — see ``ValidationHookConfig`` docs). self.adapter.configure_validation(validation) @@ -1008,6 +1009,25 @@ async def _ensure_idempotency_capability(self) -> None: self._idempotency_capability_verified = False raise + async def _prepare_signing_capabilities(self) -> None: + """Populate signing policy before a transport writer sends a request.""" + await self.fetch_capabilities() + + @staticmethod + def _mcp_operation_from_request(request: httpx.Request) -> str | None: + """Extract one MCP ``tools/call`` name from its JSON-RPC body.""" + try: + payload = json.loads(request.content) + except (json.JSONDecodeError, TypeError, UnicodeDecodeError): + return None + if not isinstance(payload, dict) or payload.get("method") != "tools/call": + return None + params = payload.get("params") + if not isinstance(params, dict): + return None + name = params.get("name") + return name if isinstance(name, str) and name else None + async def _sign_outgoing_request(self, request: httpx.Request) -> None: """httpx request event hook that attaches RFC 9421 signature headers. @@ -1022,7 +1042,8 @@ async def _sign_outgoing_request(self, request: httpx.Request) -> None: """ if self.signing is None: return - operation = _signing_current_operation.get() + mcp_operation = self._mcp_operation_from_request(request) + operation = mcp_operation or _signing_current_operation.get() # Unset ContextVar → out-of-band call (agent-card fetch, session # initialize, etc). Skip without fetching capabilities. # @@ -1036,7 +1057,21 @@ async def _sign_outgoing_request(self, request: httpx.Request) -> None: if operation is None or operation == "get_adcp_capabilities": return - caps = await self.fetch_capabilities() + if mcp_operation is not None: + # MCP's httpx hook runs in a writer task whose ContextVar snapshot + # was captured when the session connected. The adapter prefetches + # capabilities in the caller task before enqueueing tools/call; + # fetching here would deadlock the same writer stream. + caps = self._capabilities + if caps is None: + raise RuntimeError( + "MCP request signing policy was not prefetched before tools/call" + ) + else: + # A2A's event hook runs in the caller task. Retain this fallback + # for direct/custom transports, while the bundled adapter also + # prefetches so normal hooks stay network-free. + caps = self._capabilities or await self.fetch_capabilities() req_signing = getattr(caps, "request_signing", None) # Detect and surface a malformed seller config: supported=False is diff --git a/src/adcp/decisioning/dispatch.py b/src/adcp/decisioning/dispatch.py index c86778d47..27deae525 100644 --- a/src/adcp/decisioning/dispatch.py +++ b/src/adcp/decisioning/dispatch.py @@ -59,6 +59,12 @@ TaskHandoffContext, TaskRegistry, ) +from adcp.decisioning.time_budget import ( + RoutedSyncExecution, + SyncExecutorAdmission, + _bind_routed_sync_execution, + submit_supervised, +) from adcp.decisioning.types import ( AdcpError, TaskHandoff, @@ -89,6 +95,11 @@ logger = logging.getLogger(__name__) +# Strong references for synchronous adopter lifecycles that outlive a +# cancelled request. A Python thread cannot be cancelled; its completion hooks +# must still settle durable proposal/idempotency state. +_SUPERVISED_SYNC_LIFECYCLES: set[asyncio.Task[Any]] = set() + # --------------------------------------------------------------------------- # Specialism enum — spec slugs known to the framework # --------------------------------------------------------------------------- @@ -587,6 +598,11 @@ def _internal_error_message(method_name: str, exc: BaseException) -> str: return f"Platform method {method_name!r} raised {cls_name}; see details for cause" +def _exception_cause_details(exc: BaseException) -> dict[str, Any]: + """Return the shared sanitized exception-type breadcrumb.""" + return {"caused_by": {"type": type(exc).__name__}} + + def _internal_error_details(exc: BaseException) -> dict[str, Any]: """Build the wire-side ``details`` payload for an INTERNAL_ERROR wrap. @@ -628,11 +644,7 @@ def _internal_error_details(exc: BaseException) -> dict[str, Any]: where a structured field list is meaningful, so we don't generalize this to other exception types. """ - details: dict[str, Any] = { - "caused_by": { - "type": type(exc).__name__, - } - } + details = _exception_cause_details(exc) # Try to import lazily so a future refactor that splits the # validation tooling can't ripple through the dispatch layer. try: @@ -651,8 +663,7 @@ def _internal_error_details(exc: BaseException) -> dict[str, Any]: details["validation_errors"] = list(narrow_union_errors(errors_list)) except Exception: # Defensive — never let a narrowing bug 500 the wire. - # The caused_by.message already carries the truncated - # repr; adopters can still triage via server logs. + # The exception type still lets adopters triage via server logs. pass return details @@ -1317,6 +1328,7 @@ async def _invoke_platform_method( webhook_target: WebhookDeliveryTarget | None = None, webhook_auto_emit: bool = True, pre_handoff_reject: Callable[[], None] | None = None, + sync_admission: SyncExecutorAdmission | None = None, ) -> Any: """Invoke a platform method, projecting hybrid returns. @@ -1388,12 +1400,17 @@ async def _invoke_platform_method( off on a ``wholesale`` request is rejected cleanly instead of leaking a task the buyer was told was rejected. Runs only on the ``TaskHandoff`` arm; sync / workflow-handoff returns ignore it. + :param sync_admission: Optional bounded admission controller for a sync + method. Its permit remains held until the underlying thread future + actually completes, including after caller cancellation. """ # pydantic is a required dep; import here (not at module level) to mirror # the lazy-import discipline used throughout this module. from pydantic import ValidationError as _ValidationError # noqa: PLC0415 method = getattr(platform, method_name) + sync_lifecycle_continues = False + routed_sync_execution: RoutedSyncExecution | None = None # Re-validate through the platform method's own annotation when it's a # stricter subclass of the shim's already-deserialized type. Skipped # when arg_projector is set — that path replaces positional args entirely. @@ -1412,31 +1429,50 @@ async def _invoke_platform_method( try: if asyncio.iscoroutinefunction(method): - if arg_projector is not None: - result = await method(**arg_projector, ctx=ctx) - elif extra_kwargs: - result = await method(params, ctx, **extra_kwargs) - else: - result = await method(params, ctx) + # Async router delegates may resolve to synchronous tenant + # children only after account routing. Propagate the same bounded + # admission controller and configured executor through ContextVars + # so that path cannot bypass the timed-sync limit. + with _bind_routed_sync_execution(sync_admission, executor) as routed_sync_execution: + if arg_projector is not None: + result = await method(**arg_projector, ctx=ctx) + elif extra_kwargs: + result = await method(params, ctx, **extra_kwargs) + else: + result = await method(params, ctx) else: - ctx_snapshot = contextvars.copy_context() - loop = asyncio.get_running_loop() if arg_projector is not None: projected_kwargs = {**arg_projector, "ctx": ctx} - result = await loop.run_in_executor( - executor, - functools.partial(ctx_snapshot.run, method, **projected_kwargs), - ) + worker_call = functools.partial(method, **projected_kwargs) elif extra_kwargs: - result = await loop.run_in_executor( - executor, - functools.partial(ctx_snapshot.run, method, params, ctx, **extra_kwargs), - ) + worker_call = functools.partial(method, params, ctx, **extra_kwargs) else: - result = await loop.run_in_executor( - executor, - functools.partial(ctx_snapshot.run, method, params, ctx), - ) + worker_call = functools.partial(method, params, ctx) + + worker_async_future = await submit_supervised( + executor, + sync_admission, + worker_call, + ) + try: + result = await asyncio.shield(worker_async_future) + except asyncio.CancelledError: + if on_complete is not None or on_failure is not None: + sync_lifecycle_continues = True + _supervise_sync_lifecycle( + worker_async_future, + ctx=ctx, + method_name=method_name, + registry=registry, + executor=executor, + on_complete=on_complete, + on_failure=on_failure, + pre_handoff_reject=pre_handoff_reject, + request_params=params, + webhook_target=webhook_target, + webhook_auto_emit=webhook_auto_emit, + ) + raise except AdcpError as exc: # Adopter raised structured error — propagate verbatim. The # outer middleware projects to the wire envelope. Fire @@ -1531,8 +1567,8 @@ async def _invoke_platform_method( # The ``details.caused_by`` shape (Emma AudioStack P2) gives # adopters a breadcrumb on the wire — without it, "An internal # error occurred" is a dead end and adopters have to grep - # server logs. We expose only the exception class name + str - # (not the traceback) so a misconfigured platform that throws + # server logs. We expose only the exception class name (not the + # message or traceback) so a misconfigured platform that throws # on secret material doesn't leak the secret value through # the wire response. logger.exception( @@ -1548,7 +1584,73 @@ async def _invoke_platform_method( if on_failure is not None: await _safe_on_failure_call(on_failure, wrapped, method_name) raise wrapped from exc + except BaseException as exc: + # ``asyncio.CancelledError`` (and shutdown BaseExceptions) bypass the + # wire-error wrapping above, but must still release framework state + # reserved before adapter dispatch. Preserve the exact exception. + nested_sync_future = ( + routed_sync_execution.worker if routed_sync_execution is not None else None + ) + if isinstance(nested_sync_future, asyncio.Future) and ( + on_complete is not None or on_failure is not None + ): + sync_lifecycle_continues = True + _supervise_sync_lifecycle( + nested_sync_future, + ctx=ctx, + method_name=method_name, + registry=registry, + executor=executor, + on_complete=on_complete, + on_failure=on_failure, + pre_handoff_reject=pre_handoff_reject, + request_params=params, + webhook_target=webhook_target, + webhook_auto_emit=webhook_auto_emit, + ) + # A cancelled async mutation may already have crossed an external + # side-effect boundary. Keep its reservation fail-closed for later + # reconciliation instead of making an immediate retry eligible to + # double-book. Synchronous work is settled from its real worker + # outcome above; ordinary BaseException failures still run the hook. + if ( + on_failure is not None + and not sync_lifecycle_continues + and not isinstance(exc, asyncio.CancelledError) + ): + await _safe_on_failure_call(on_failure, exc, method_name) + raise + + return await _project_invocation_result( + result, + ctx=ctx, + method_name=method_name, + registry=registry, + executor=executor, + on_complete=on_complete, + on_failure=on_failure, + pre_handoff_reject=pre_handoff_reject, + request_params=params, + webhook_target=webhook_target, + webhook_auto_emit=webhook_auto_emit, + ) + +async def _project_invocation_result( + result: Any, + *, + ctx: RequestContext[Any], + method_name: str, + registry: TaskRegistry, + executor: ThreadPoolExecutor, + on_complete: Callable[[Any], Awaitable[None]] | None, + on_failure: Callable[[BaseException], Awaitable[None]] | None, + pre_handoff_reject: Callable[[], None] | None, + request_params: BaseModel, + webhook_target: WebhookDeliveryTarget | None, + webhook_auto_emit: bool, +) -> Any: + """Project a raw adopter result and settle its framework lifecycle hooks.""" if is_task_handoff(result): # Reject before any side effect (registry row, background task, # completion webhook) is created. The wholesale discovery guard @@ -1565,7 +1667,7 @@ async def _invoke_platform_method( executor=executor, on_complete=on_complete, on_failure=on_failure, - request_params=params, + request_params=request_params, webhook_target=webhook_target, webhook_auto_emit=webhook_auto_emit, ) @@ -1576,7 +1678,7 @@ async def _invoke_platform_method( method_name=method_name, registry=registry, executor=executor, - request_params=params, + request_params=request_params, ) # Sync return path. Fire on_complete with the typed result before @@ -1601,6 +1703,99 @@ async def _invoke_platform_method( return strip_credentials_from_wire_result(method_name, result) +async def _settle_cancelled_sync_lifecycle( + worker_future: asyncio.Future[Any], + *, + ctx: RequestContext[Any], + method_name: str, + registry: TaskRegistry, + executor: ThreadPoolExecutor, + on_complete: Callable[[Any], Awaitable[None]] | None, + on_failure: Callable[[BaseException], Awaitable[None]] | None, + pre_handoff_reject: Callable[[], None] | None, + request_params: BaseModel, + webhook_target: WebhookDeliveryTarget | None, + webhook_auto_emit: bool, +) -> None: + """Settle a sync worker after its request task has been cancelled.""" + try: + result = await asyncio.shield(worker_future) + except asyncio.CancelledError: + # Cancelling this supervisor must not cancel or roll back the + # non-cancellable thread it observes. Its reservation remains held. + raise + except Exception as exc: + if on_failure is not None: + await _safe_on_failure_call(on_failure, exc, method_name) + return + if is_task_handoff(result) or is_workflow_handoff(result): + # The cancelled caller never received a task id. Do not promote an + # unreachable handoff; returning a handoff has not executed its work. + if on_failure is not None: + await _safe_on_failure_call(on_failure, asyncio.CancelledError(), method_name) + logger.warning( + "Discarded %s handoff returned after request cancellation; no task id was issued", + method_name, + ) + return + try: + await _project_invocation_result( + result, + ctx=ctx, + method_name=method_name, + registry=registry, + executor=executor, + on_complete=on_complete, + on_failure=on_failure, + pre_handoff_reject=pre_handoff_reject, + request_params=request_params, + webhook_target=webhook_target, + webhook_auto_emit=webhook_auto_emit, + ) + except Exception: + # Lifecycle hooks already apply their own rollback semantics. There is + # no request waiter left to receive this exception, so retain it in + # server logs rather than producing an unhandled-task warning. + logger.exception( + "Cancelled request's synchronous %s lifecycle failed while settling", + method_name, + ) + + +def _supervise_sync_lifecycle( + worker_future: asyncio.Future[Any], + *, + ctx: RequestContext[Any], + method_name: str, + registry: TaskRegistry, + executor: ThreadPoolExecutor, + on_complete: Callable[[Any], Awaitable[None]] | None, + on_failure: Callable[[BaseException], Awaitable[None]] | None, + pre_handoff_reject: Callable[[], None] | None, + request_params: BaseModel, + webhook_target: WebhookDeliveryTarget | None, + webhook_auto_emit: bool, +) -> None: + """Own a cancelled request's worker until its lifecycle settles.""" + lifecycle = asyncio.create_task( + _settle_cancelled_sync_lifecycle( + worker_future, + ctx=ctx, + method_name=method_name, + registry=registry, + executor=executor, + on_complete=on_complete, + on_failure=on_failure, + pre_handoff_reject=pre_handoff_reject, + request_params=request_params, + webhook_target=webhook_target, + webhook_auto_emit=webhook_auto_emit, + ) + ) + _SUPERVISED_SYNC_LIFECYCLES.add(lifecycle) + lifecycle.add_done_callback(_SUPERVISED_SYNC_LIFECYCLES.discard) + + async def _safe_on_failure_call( on_failure: Callable[[BaseException], Awaitable[None]], exc: BaseException, @@ -1838,6 +2033,11 @@ async def _run() -> None: ) await _fail(wrapped) return + except BaseException: + # Cancellation does not prove adopter work stopped. Leave any + # reservation fail-closed for expiry/reconciliation rather than + # release it while side effects may still be outstanding. + raise # Framework completion hook (e.g., proposal_store.commit for # finalize, mark_proposal_consumed for create_media_buy). Runs diff --git a/src/adcp/decisioning/handler.py b/src/adcp/decisioning/handler.py index e4ae2ef02..7cfd1fde4 100644 --- a/src/adcp/decisioning/handler.py +++ b/src/adcp/decisioning/handler.py @@ -55,6 +55,9 @@ ) from adcp.decisioning.dispatch import ( _build_request_context, + _exception_cause_details, + _internal_error_details, + _internal_error_message, _invoke_platform_method, ) from adcp.decisioning.implementation_config import ProductConfigStore @@ -79,7 +82,11 @@ has_refine_support, project_refine_response, ) -from adcp.decisioning.time_budget import project_incomplete_response, resolve_time_budget +from adcp.decisioning.time_budget import ( + SyncExecutorAdmission, + project_incomplete_response, + resolve_time_budget, +) from adcp.decisioning.types import ( Account as _DecisioningAccount, ) @@ -1297,6 +1304,7 @@ def __init__( property_list_fetcher: PropertyListFetcher | None = None, media_buy_store: MediaBuyStore | None = None, advertise_all: bool = False, + timed_sync_get_products_limit: int | None = None, ) -> None: super().__init__() self._platform = platform @@ -1323,6 +1331,13 @@ def __init__( self.canonical_format_legacy_resolver = getattr( platform, "canonical_format_legacy_resolver", None ) + # Direct PlatformHandler construction has no public way to inspect a + # BYO executor's capacity. The adopter-facing composition root passes + # an explicit resolved value; direct construction defaults to one. + admission_limit = ( + timed_sync_get_products_limit if timed_sync_get_products_limit is not None else 1 + ) + self._timed_sync_get_products_admission = SyncExecutorAdmission(admission_limit) # Cache whether the platform's create_media_buy accepts 'configs' # so we only pay the inspect.signature cost at construction time. @@ -1657,17 +1672,9 @@ async def get_adcp_capabilities( ) raise AdcpError( "INTERNAL_ERROR", - message=( - "Unhandled exception in platform.get_adcp_capabilities_for_request: " - f"{type(exc).__name__}: {exc}" - ), + message=_internal_error_message("get_adcp_capabilities_for_request", exc), recovery="terminal", - details={ - "caused_by": { - "type": type(exc).__name__, - "message": str(exc), - } - }, + details=_internal_error_details(exc), ) from exc has_scoped_caps = scoped_caps is not None if scoped_caps is not None: @@ -2003,6 +2010,9 @@ async def _persist_draft_hook(get_products_result: Any) -> None: registry=self._registry, on_complete=_persist_draft_hook, pre_handoff_reject=pre_handoff_reject, + sync_admission=( + self._timed_sync_get_products_admission if deadline is not None else None + ), **self._handoff_webhook_kwargs(), ) try: @@ -2011,9 +2021,9 @@ async def _persist_draft_hook(get_products_result: Any) -> None: ) except asyncio.TimeoutError: # Deadline expired. The platform coroutine is cancelled; for - # sync adopters the underlying thread runs to completion but the - # asyncio side has moved on (thread-pool slot leak documented in - # adcp.decisioning.time_budget module header). + # sync adopters an admitted underlying thread runs to completion, + # retaining its bounded admission permit. Saturated calls that + # never acquired a permit were not submitted to the executor. tb = params.time_budget interval = tb.interval if tb is not None else 0 unit_raw = tb.unit if tb is not None else None @@ -2026,7 +2036,9 @@ async def _persist_draft_hook(get_products_result: Any) -> None: "[adcp.decisioning] get_products timed out after %ds " "(time_budget=%d %s); returning incomplete response. " "To avoid timeout cancellations, optimise get_products " - "latency or reduce the platform's search scope.", + "latency or reduce the platform's search scope. Saturated " + "sync calls wait behind timed_sync_get_products_limit and " + "are not submitted after their budget expires.", deadline, interval, unit, @@ -2128,7 +2140,7 @@ async def create_media_buy( # type: ignore[override] "if the problem persists contact the seller." ), recovery="transient", - details={"caused_by": {"type": type(exc).__name__}}, + details=_exception_cause_details(exc), ) from exc # v1.5: when params.proposal_id is set AND a tenant store is diff --git a/src/adcp/decisioning/platform_router.py b/src/adcp/decisioning/platform_router.py index 5619e68b5..6d60bdadf 100644 --- a/src/adcp/decisioning/platform_router.py +++ b/src/adcp/decisioning/platform_router.py @@ -119,6 +119,7 @@ SalesPlatform, SignalsPlatform, ) +from adcp.decisioning.time_budget import _routed_sync_execution, submit_supervised from adcp.decisioning.types import AdcpError if TYPE_CHECKING: @@ -128,6 +129,26 @@ from adcp.decisioning.proposal_store import ProposalStore +async def _run_sync_delegate(method: Any, *args: Any, **kwargs: Any) -> Any: + """Run a sync child and expose its live future across request cancellation.""" + execution = _routed_sync_execution() + if execution is None: + worker: asyncio.Future[Any] = asyncio.create_task( + asyncio.to_thread(method, *args, **kwargs) + ) + else: + worker = await submit_supervised( + execution.executor, + execution.admission, + lambda: method(*args, **kwargs), + ) + execution.worker = worker + try: + return await asyncio.shield(worker) + except asyncio.CancelledError: + raise + + # Every specialism Protocol the framework knows about. New Protocol # classes added to ``adcp.decisioning.specialisms`` get picked up by # adding them here. Walking ``__protocol_attrs__`` (set by the runtime @@ -503,7 +524,7 @@ async def get_products(self, *args: Any, **kwargs: Any) -> Any: method = getattr(manager, method_name) if inspect.iscoroutinefunction(method): return await method(*args, **kwargs) - return await asyncio.to_thread(method, *args, **kwargs) + return await _run_sync_delegate(method, *args, **kwargs) # No proposal_manager for this tenant — fall through to the # platform. Reuses the same lookup helper as the synthesized @@ -512,7 +533,7 @@ async def get_products(self, *args: Any, **kwargs: Any) -> Any: method = getattr(platform, "get_products") if inspect.iscoroutinefunction(method): return await method(*args, **kwargs) - return await asyncio.to_thread(method, *args, **kwargs) + return await _run_sync_delegate(method, *args, **kwargs) def _make_delegate(self, method_name: str) -> Any: """Create a delegating callable for ``method_name``. @@ -549,7 +570,7 @@ async def _delegate(*args: Any, **kwargs: Any) -> Any: # contextvars snapshot; ``asyncio.to_thread`` does the same # using the running loop's default executor with copied # context. - return await asyncio.to_thread(method, *args, **kwargs) + return await _run_sync_delegate(method, *args, **kwargs) _delegate.__name__ = method_name _delegate.__qualname__ = f"PlatformRouter.{method_name}" @@ -1023,7 +1044,7 @@ async def _delegate(*args: Any, **kwargs: Any) -> Any: method = getattr(platform, method_name) if inspect.iscoroutinefunction(method): return await method(*args, **kwargs) - return await asyncio.to_thread(method, *args, **kwargs) + return await _run_sync_delegate(method, *args, **kwargs) _delegate.__name__ = method_name _delegate.__qualname__ = f"LazyPlatformRouter.{method_name}" @@ -1057,13 +1078,13 @@ async def get_products(self, *args: Any, **kwargs: Any) -> Any: method = getattr(manager, method_name) if inspect.iscoroutinefunction(method): return await method(*args, **kwargs) - return await asyncio.to_thread(method, *args, **kwargs) + return await _run_sync_delegate(method, *args, **kwargs) platform = await self._platform_for_method(ctx, "get_products") method = getattr(platform, "get_products") if inspect.iscoroutinefunction(method): return await method(*args, **kwargs) - return await asyncio.to_thread(method, *args, **kwargs) + return await _run_sync_delegate(method, *args, **kwargs) def proposal_manager_for_tenant(self, tenant_id: str) -> ProposalManager | None: """Return the :class:`ProposalManager` for ``tenant_id``, or ``None``.""" @@ -1245,7 +1266,7 @@ async def _delegate(*args: Any, **kwargs: Any) -> Any: method = getattr(platform, method_name) if inspect.iscoroutinefunction(method): return await method(*args, **kwargs) - return await asyncio.to_thread(method, *args, **kwargs) + return await _run_sync_delegate(method, *args, **kwargs) _delegate.__name__ = method_name _delegate.__qualname__ = f"_RegistryPlatformAdapter.{method_name}" @@ -1272,7 +1293,7 @@ async def get_products(self, *args: Any, **kwargs: Any) -> Any: method = getattr(platform, "get_products") if inspect.iscoroutinefunction(method): return await method(*args, **kwargs) - return await asyncio.to_thread(method, *args, **kwargs) + return await _run_sync_delegate(method, *args, **kwargs) def _make_registry_platform_adapter( diff --git a/src/adcp/decisioning/proposal_dispatch.py b/src/adcp/decisioning/proposal_dispatch.py index e989cc7db..b7e34a5a8 100644 --- a/src/adcp/decisioning/proposal_dispatch.py +++ b/src/adcp/decisioning/proposal_dispatch.py @@ -48,7 +48,6 @@ from __future__ import annotations import asyncio -import contextvars import functools import logging from datetime import datetime, timedelta, timezone @@ -73,6 +72,7 @@ _await_maybe, ) from adcp.decisioning.recipe import Recipe +from adcp.decisioning.time_budget import submit_supervised from adcp.decisioning.types import AdcpError, is_task_handoff if TYPE_CHECKING: @@ -84,6 +84,57 @@ from adcp.decisioning.task_registry import TaskRegistry logger = logging.getLogger("adcp.decisioning.proposal_dispatch") +_SUPERVISED_FINALIZATIONS: set[asyncio.Task[None]] = set() + + +async def _settle_cancelled_finalize( + worker: asyncio.Future[Any], + *, + store: Any, + proposal_id: str, + account_id: str, +) -> None: + """Commit a sync finalize result after its request task is cancelled.""" + try: + result = await asyncio.shield(worker) + except asyncio.CancelledError: + # Process shutdown may cancel this observer; never cancel the thread. + raise + except Exception: + logger.exception( + "Cancelled finalize_proposal worker failed for proposal %s; draft retained", + proposal_id, + ) + return + + if not isinstance(result, FinalizeProposalSuccess): + logger.error( + "Cancelled finalize_proposal returned %s for proposal %s; draft retained", + type(result).__name__, + proposal_id, + ) + return + try: + await _await_maybe( + store.commit( + proposal_id, + expires_at=result.expires_at, + proposal_payload=dict(result.proposal), + expected_account_id=account_id, + ) + ) + except Exception: + logger.exception( + "Cancelled finalize_proposal succeeded but proposal %s commit failed", + proposal_id, + ) + return + finalize_succeeded_log( + proposal_id=proposal_id, + account_id=account_id, + expires_at=result.expires_at, + path="inline-after-cancellation", + ) # --------------------------------------------------------------------------- @@ -239,12 +290,25 @@ async def maybe_intercept_finalize( if asyncio.iscoroutinefunction(method): result = await method(finalize_req, ctx) else: - ctx_snapshot = contextvars.copy_context() - loop = asyncio.get_running_loop() - result = await loop.run_in_executor( + worker = await submit_supervised( executor, - functools.partial(ctx_snapshot.run, method, finalize_req, ctx), + None, + functools.partial(method, finalize_req, ctx), ) + try: + result = await asyncio.shield(worker) + except asyncio.CancelledError: + supervisor = asyncio.create_task( + _settle_cancelled_finalize( + worker, + store=store, + proposal_id=proposal_id, + account_id=account_id, + ) + ) + _SUPERVISED_FINALIZATIONS.add(supervisor) + supervisor.add_done_callback(_SUPERVISED_FINALIZATIONS.discard) + raise if is_task_handoff(result): # HITL slow path. Per § D2 + § D3: framework projects Submitted @@ -745,12 +809,8 @@ async def maybe_hydrate_recipes_for_create_media_buy( field_path_prefix="packages", ) except Exception: - # Narrow to Exception (not BaseException): CancelledError / - # SystemExit / KeyboardInterrupt skip the release path — under - # cancellation the next worker will read the stale reservation - # and eviction handles it; under shutdown we want fast exit. - # release_consumption is idempotent on already-COMMITTED so a - # release that races with a concurrent worker is harmless. + # Ordinary derivation/validation failures release the reservation. + # Process-control BaseExceptions propagate without being intercepted. try: await _await_maybe( store.release_consumption(proposal_id, expected_account_id=ctx.account.id) diff --git a/src/adcp/decisioning/serve.py b/src/adcp/decisioning/serve.py index 4bac11713..bc560a542 100644 --- a/src/adcp/decisioning/serve.py +++ b/src/adcp/decisioning/serve.py @@ -82,6 +82,7 @@ def create_adcp_server_from_platform( *, executor: ThreadPoolExecutor | None = None, thread_pool_size: int | None = None, + timed_sync_get_products_limit: int | None = None, registry: TaskRegistry | None = None, state_reader: StateReader | None = None, resource_resolver: ResourceResolver | None = None, @@ -123,10 +124,19 @@ def create_adcp_server_from_platform( for operators with audit-instrumented thread pools or wrappers around stdlib's executor. Mutually exclusive with ``thread_pool_size``. Operator owns lifecycle (caller's - ``shutdown(wait=True)`` responsibility). + ``shutdown(wait=True)`` responsibility). Requires an explicit + ``timed_sync_get_products_limit`` because executor wrappers expose no + public capacity contract. :param thread_pool_size: Size the default framework-allocated executor. Mutually exclusive with ``executor``. Default is :func:`_default_thread_pool_size`. + :param timed_sync_get_products_limit: Maximum synchronous + ``get_products`` calls with SDK-managed deadlines admitted to the + executor at once. Saturated calls wait within their own time budget + and return ``incomplete`` without being submitted if it expires. + For framework-allocated pools, defaults to half the configured worker + count (minimum one), reserving capacity for other tools. Required with + ``executor=``. :param registry: Bring-your-own :class:`TaskRegistry` — typically a v6.1 durable backing store. Default is :class:`InMemoryTaskRegistry`, which the production-mode @@ -261,13 +271,28 @@ def create_adcp_server_from_platform( "vetted threadpool." ) - # Allocate executor. + # Allocate executor and resolve admission sizing while the public worker + # count is still available. Executor wrappers expose no stable capacity + # attribute, so BYO pools must provide the explicit admission limit. if executor is None: size = thread_pool_size if thread_pool_size is not None else _default_thread_pool_size() executor = ThreadPoolExecutor( max_workers=size, thread_name_prefix="adcp-decisioning-", ) + resolved_timed_sync_limit = ( + timed_sync_get_products_limit + if timed_sync_get_products_limit is not None + else max(1, size // 2) + ) + else: + if timed_sync_get_products_limit is None: + raise ValueError( + "executor= requires timed_sync_get_products_limit= because executor " + "wrappers expose no public worker-count contract. Pass an explicit " + "positive admission limit or use thread_pool_size=." + ) + resolved_timed_sync_limit = timed_sync_get_products_limit # Allocate registry, with production-mode gate (Emma #8). # Gate reads the registry's is_durable class-level marker rather @@ -377,6 +402,7 @@ def create_adcp_server_from_platform( property_list_fetcher=property_list_fetcher, media_buy_store=media_buy_store, advertise_all=advertise_all, + timed_sync_get_products_limit=resolved_timed_sync_limit, ) # Boot-time fail-fast: property_list_filtering declared but no fetcher wired. @@ -461,6 +487,7 @@ def serve( name: str | None = None, executor: ThreadPoolExecutor | None = None, thread_pool_size: int | None = None, + timed_sync_get_products_limit: int | None = None, registry: TaskRegistry | None = None, state_reader: StateReader | None = None, resource_resolver: ResourceResolver | None = None, @@ -491,8 +518,12 @@ def serve( :param name: Server name advertised on AdCP capabilities. Defaults to the platform class's ``__name__``. :param executor: BYO :class:`ThreadPoolExecutor` per - :func:`create_adcp_server_from_platform` D5 contract. + :func:`create_adcp_server_from_platform` D5 contract. Requires + ``timed_sync_get_products_limit``. :param thread_pool_size: Default-executor size override. + :param timed_sync_get_products_limit: Bounded admission limit for + deadline-managed synchronous ``get_products`` calls. See + :func:`create_adcp_server_from_platform`. :param registry: BYO :class:`TaskRegistry`. Default is :class:`InMemoryTaskRegistry` (gated for production). :param state_reader: Custom :class:`StateReader` impl (D15). @@ -587,6 +618,7 @@ def serve( platform, executor=executor, thread_pool_size=thread_pool_size, + timed_sync_get_products_limit=timed_sync_get_products_limit, registry=registry, state_reader=state_reader, resource_resolver=resource_resolver, diff --git a/src/adcp/decisioning/time_budget.py b/src/adcp/decisioning/time_budget.py index 65fd40d44..9fe4a1d07 100644 --- a/src/adcp/decisioning/time_budget.py +++ b/src/adcp/decisioning/time_budget.py @@ -17,15 +17,17 @@ past the ``except Exception`` in ``_invoke_platform_method`` cleanly. This invariant MUST be preserved if ``get_products`` ever gains registry work. -* **Thread-pool warning for sync adopters.** When a sync adopter runs via +* **Bounded sync-adopter admission.** When a sync adopter runs via ``loop.run_in_executor`` and ``asyncio.wait_for`` fires, the asyncio side moves on but the underlying thread continues until its blocking call - returns. No Python mechanism can interrupt a running thread. The pool - slot is occupied for the full duration; on a short-budget burst against a - slow sync adopter this can exhaust the pool. Async adopters are - unaffected. Adopters who need to co-operate with deadline cancellation - should implement the ``IncrementalGetProducts`` protocol or migrate to an - async ``get_products``. + returns. No Python mechanism can interrupt a running thread. The framework + therefore admits only a bounded number of deadline-managed synchronous + calls and holds each permit until the worker really exits, even after the + response timed out. Saturated calls spend their budget waiting for a permit + and return ``incomplete[]`` without entering the executor. By default the + limit is half the executor workers (minimum one), preserving capacity for + other tools; operators can tune it at server construction. Async adopters + are unaffected. * **``campaign`` unit → no SDK-managed deadline.** ``unit='campaign'`` means "the seller has the full campaign flight to respond" — this is a @@ -60,16 +62,117 @@ from __future__ import annotations +import asyncio +import contextvars import logging -from collections.abc import AsyncIterator +from collections.abc import AsyncIterator, Callable +from concurrent.futures import Executor +from contextlib import contextmanager +from contextvars import ContextVar +from dataclasses import dataclass from typing import TYPE_CHECKING, Any, Protocol, runtime_checkable if TYPE_CHECKING: + from collections.abc import Iterator + from adcp.decisioning.context import RequestContext from adcp.types import GetProductsRequest logger = logging.getLogger(__name__) + +class SyncExecutorAdmission: + """Bound outstanding deadline-managed synchronous executor work. + + A permit represents a worker submission, not a waiting HTTP request. It + is released only by the underlying ``concurrent.futures.Future`` done + callback, because cancelling its asyncio wrapper cannot stop a running + Python thread. + """ + + def __init__(self, limit: int) -> None: + if not isinstance(limit, int) or isinstance(limit, bool) or limit < 1: + raise ValueError("sync executor admission limit must be a positive integer") + self.limit = limit + self._semaphore = asyncio.BoundedSemaphore(limit) + + async def acquire(self) -> None: + """Wait until a bounded worker slot is available.""" + await self._semaphore.acquire() + + def release(self) -> None: + """Return a worker slot after its real thread future completes.""" + self._semaphore.release() + + +async def submit_supervised( + executor: Executor, + admission: SyncExecutorAdmission | None, + call: Callable[[], Any], +) -> asyncio.Future[Any]: + """Submit sync work and bind admission to the real worker lifetime. + + The returned asyncio future may be shielded or observed by another task; + its cancellation cannot stop the underlying thread. A permit is released + exactly once by the concurrent future's completion callback. + """ + if admission is not None: + await admission.acquire() + loop = asyncio.get_running_loop() + try: + snapshot = contextvars.copy_context() + concurrent_worker = executor.submit(snapshot.run, call) + except Exception: + if admission is not None: + admission.release() + raise + + if admission is not None: + + def _release_admission(_future: object) -> None: + try: + loop.call_soon_threadsafe(admission.release) + except RuntimeError: + # Event loop already closed during process teardown. + pass + + concurrent_worker.add_done_callback(_release_admission) + return asyncio.wrap_future(concurrent_worker, loop=loop) + + +@dataclass +class RoutedSyncExecution: + """Typed request scope shared by dispatch and a routed sync delegate.""" + + admission: SyncExecutorAdmission | None + executor: Executor + worker: asyncio.Future[Any] | None = None + + +_ROUTED_SYNC_EXECUTION: ContextVar[RoutedSyncExecution | None] = ContextVar( + "adcp_routed_sync_execution", default=None +) + + +@contextmanager +def _bind_routed_sync_execution( + admission: SyncExecutorAdmission | None, + executor: Executor, +) -> Iterator[RoutedSyncExecution]: + """Expose deadline admission to an async router's eventual sync child.""" + execution = RoutedSyncExecution(admission=admission, executor=executor) + token = _ROUTED_SYNC_EXECUTION.set(execution) + try: + yield execution + finally: + _ROUTED_SYNC_EXECUTION.reset(token) + + +def _routed_sync_execution() -> RoutedSyncExecution | None: + """Return the admission/executor inherited by a router delegate.""" + return _ROUTED_SYNC_EXECUTION.get() + + # ---- Unit conversion ---- _UNIT_TO_SECONDS: dict[str, float] = { @@ -258,6 +361,7 @@ async def get_products_incremental( __all__ = [ "IncrementalGetProducts", "ProductsCheckpoint", + "SyncExecutorAdmission", "project_incomplete_response", "resolve_time_budget", ] diff --git a/src/adcp/protocols/a2a.py b/src/adcp/protocols/a2a.py index 39d2f9caf..c307fd181 100644 --- a/src/adcp/protocols/a2a.py +++ b/src/adcp/protocols/a2a.py @@ -422,6 +422,9 @@ async def _call_a2a_tool( idempotency_key=idempotency_key, ) + if tool_name != "get_adcp_capabilities" and self.signing_capability_check: + await self.signing_capability_check() + a2a_client = await self._get_a2a_client() # Build A2A message diff --git a/src/adcp/protocols/base.py b/src/adcp/protocols/base.py index 07d6ffdcb..539360c26 100644 --- a/src/adcp/protocols/base.py +++ b/src/adcp/protocols/base.py @@ -45,6 +45,11 @@ def __init__(self, agent_config: AgentConfig): # via its httpx client's event_hooks; MCP consumes it via a custom # httpx_client_factory passed to streamablehttp_client. self.signing_request_hook: Callable[[httpx.Request], Awaitable[None]] | None = None + # Optional preflight paired with ``signing_request_hook``. Transport + # adapters invoke it before handing a request to an httpx writer task + # so the event hook never needs to fetch capabilities recursively on + # the same MCP session. + self.signing_capability_check: Callable[[], Awaitable[None]] | None = None # Schema validation modes — resolved by the owning ADCPClient via # ``configure_validation``. Class defaults match the TS port: warn # on requests (don't block partial payloads in error-path tests), diff --git a/src/adcp/protocols/mcp.py b/src/adcp/protocols/mcp.py index d9ff6bba1..f6b70df8b 100644 --- a/src/adcp/protocols/mcp.py +++ b/src/adcp/protocols/mcp.py @@ -669,6 +669,13 @@ async def _call_mcp_tool(self, tool_name: str, params: dict[str, Any]) -> TaskRe idempotency_key=idempotency_key, ) + # Streamable HTTP sends from a long-lived writer task whose + # ContextVar snapshot predates this call. Fetch signing policy in + # the caller task before enqueueing the message; the request hook + # then reads the cache without recursively using this session. + if tool_name != "get_adcp_capabilities" and self.signing_capability_check: + await self.signing_capability_check() + session = await self._get_session() if self.agent_config.debug: diff --git a/src/adcp/server/a2a_server.py b/src/adcp/server/a2a_server.py index 8972de398..49a1f0195 100644 --- a/src/adcp/server/a2a_server.py +++ b/src/adcp/server/a2a_server.py @@ -21,6 +21,8 @@ import json import logging import os +import warnings +from contextvars import ContextVar from typing import TYPE_CHECKING, Any from uuid import uuid4 @@ -34,6 +36,7 @@ from google.protobuf.json_format import MessageToDict, ParseDict from google.protobuf.struct_pb2 import Value from starlette.applications import Starlette +from starlette.requests import Request from adcp.exceptions import ADCPError from adcp.server._hooks import PreValidationHooks @@ -61,6 +64,7 @@ from a2a.server.tasks.push_notification_config_store import ( PushNotificationConfigStore, ) + from a2a.server.tasks.push_notification_sender import PushNotificationSender from a2a.server.tasks.task_store import TaskStore from adcp.server.auth import BearerTokenAuth @@ -133,6 +137,25 @@ async def agent_card_url(request: Request) -> str: from adcp.server.test_controller import TestControllerStore, _handle_test_controller logger = logging.getLogger(__name__) +_A2A_REQUEST_CONTEXT: ContextVar[Any | None] = ContextVar("adcp_a2a_request_context", default=None) + + +class _A2ARequestContextMiddleware: + """Make the originating HTTP request available during A2A dispatch.""" + + def __init__(self, app: Any) -> None: + self.app = app + + async def __call__(self, scope: Any, receive: Any, send: Any) -> None: + if scope["type"] != "http": + await self.app(scope, receive, send) + return + + token = _A2A_REQUEST_CONTEXT.set(Request(scope, receive=receive)) + try: + await self.app(scope, receive, send) + finally: + _A2A_REQUEST_CONTEXT.reset(token) def _part_data_dict(part: pb.Part) -> dict[str, Any] | None: @@ -382,6 +405,7 @@ def _build_tool_context(self, skill_name: str, request: RequestContext) -> ToolC tool_name=skill_name, transport="a2a", request_id=request.task_id, + request_context=_A2A_REQUEST_CONTEXT.get(), ) ctx = self._context_factory(meta) if not isinstance(ctx, ToolContext): @@ -964,6 +988,7 @@ def create_a2a_server( context_factory: ContextFactory | None = None, task_store: TaskStore | None = None, push_config_store: PushNotificationConfigStore | None = None, + push_sender: PushNotificationSender | None = None, middleware: Sequence[SkillMiddleware] | None = None, message_parser: MessageParser | None = None, advertise_all: bool = False, @@ -1025,6 +1050,12 @@ def create_a2a_server( in that context. A ``ContextVar`` is only needed as a fallback for direct or background sender calls that lack a context; the reference implementation demonstrates both paths. + push_sender: Optional a2a-sdk + :class:`~a2a.server.tasks.push_notification_sender.PushNotificationSender` + that delivers task updates to registered subscriptions. Pair + this with ``push_config_store`` to enable built-in delivery; + a store without a sender accepts subscriptions but cannot send + notifications and emits a startup warning. middleware: Optional sequence of :data:`~adcp.server.SkillMiddleware` callables wrapping every A2A skill dispatch. Composes outermost-first (first entry sees the call before later @@ -1133,6 +1164,13 @@ def agent_card_url(request: Request) -> str: if task_store is None: task_store = InMemoryTaskStore() + if push_config_store is not None and push_sender is None: + warnings.warn( + "push_config_store is configured without push_sender; A2A clients " + "can register push subscriptions, but task updates will not be delivered.", + UserWarning, + stacklevel=2, + ) # ``enable_v0_3_compat=True`` is load-bearing: it makes the server # dual-serve 0.3 and 1.0 wire formats on the same endpoint so existing @@ -1183,6 +1221,7 @@ def agent_card_url(request: Request) -> str: task_store=task_store, agent_card=fallback_card, push_config_store=push_config_store, + push_sender=push_sender, ) jsonrpc_kwargs["request_handler"] = request_handler routes = list(create_jsonrpc_routes(**jsonrpc_kwargs)) @@ -1228,6 +1267,7 @@ def agent_card_url(request: Request) -> str: task_store=task_store, agent_card=agent_card, push_config_store=push_config_store, + push_sender=push_sender, ) jsonrpc_kwargs["request_handler"] = request_handler routes = ( @@ -1241,6 +1281,12 @@ def agent_card_url(request: Request) -> str: ) app = Starlette(routes=routes) + # Keep the originating Starlette Request available to context factories + # during executor dispatch. This is installed for direct + # ``create_a2a_server`` adopters as well as the unified ``serve`` path, + # independent of whether bearer-auth middleware is configured. + app.add_middleware(_A2ARequestContextMiddleware) + # Startup log lives on the create_a2a_server path (symmetric with # MCP's _register_handler_tools). Moved out of # ADCPAgentExecutor.__init__ so per-test executor constructions diff --git a/src/adcp/server/idempotency/backends.py b/src/adcp/server/idempotency/backends.py index c74e07d84..ec51e7e81 100644 --- a/src/adcp/server/idempotency/backends.py +++ b/src/adcp/server/idempotency/backends.py @@ -4,12 +4,11 @@ 1. Retrieve a cached response by ``(principal_id, idempotency_key)``, honoring the seller's replay TTL. -2. Atomically commit ``(payload_hash, response)`` on a fresh key. Atomicity - with the handler's business writes is the backend's choice — - :class:`MemoryBackend` makes no such guarantee; :class:`PgBackend` shares - a connection pool so adopters with the same Postgres can compose their - handler's transaction with the cache write (v1 commits in a separate - pool connection — co-tx wiring is a v1.1 affordance). +2. Hold an execution lock across lookup, handler execution, and cache commit. +3. Atomically insert webhook-dedup markers with first-writer-wins semantics. + +The lock prevents concurrent duplicate execution. Atomicity with unrelated +business writes remains the adopter's responsibility. Backends expose async methods. The in-process :class:`MemoryBackend` is synchronous under the hood but wrapped in ``async`` signatures so the store @@ -21,9 +20,13 @@ import asyncio import json import re +import threading import time +import weakref from abc import ABC, abstractmethod -from collections.abc import Callable +from collections.abc import AsyncIterator, Callable +from contextlib import asynccontextmanager +from contextvars import ContextVar from dataclasses import dataclass from datetime import datetime, timezone from typing import Any @@ -50,6 +53,31 @@ DEFAULT_IDEMPOTENCY_TABLE = "adcp_idempotency" +@dataclass +class _LegacyBackendLockState: + guard: asyncio.Lock + locks: weakref.WeakValueDictionary[tuple[str, str], asyncio.Lock] + + +_LEGACY_LOCK_STATES: weakref.WeakKeyDictionary[object, _LegacyBackendLockState] = ( + weakref.WeakKeyDictionary() +) +_LEGACY_LOCK_STATES_GUARD = threading.Lock() + + +def _legacy_backend_lock_state(backend: object) -> _LegacyBackendLockState: + """Share compatibility locks across coordinators using one backend.""" + with _LEGACY_LOCK_STATES_GUARD: + state = _LEGACY_LOCK_STATES.get(backend) + if state is None: + state = _LegacyBackendLockState( + guard=asyncio.Lock(), + locks=weakref.WeakValueDictionary(), + ) + _LEGACY_LOCK_STATES[backend] = state + return state + + def _safe_identifier(name: str) -> str: if not _SAFE_IDENTIFIER_RE.fullmatch(name): raise ValueError( @@ -83,8 +111,8 @@ class IdempotencyBackend(ABC): """Abstract storage backend contract. All methods are async. Implementations MUST be safe to call concurrently - from multiple asyncio tasks — :class:`IdempotencyStore` does not serialize - access on the caller's behalf. + from multiple asyncio tasks. ``hold`` must coordinate all processes that + share the backend namespace. """ @abstractmethod @@ -109,6 +137,45 @@ async def put( or expired, so an overwrite in that window is a legitimate retry of the write itself.""" + def hold(self, scope_key: str, key: str) -> Any: + """Return an async context manager holding the key's execution lock. + + Custom backends must implement this operation to be usable by + :class:`IdempotencyStore`. It is concrete only to keep existing + backend subclasses importable while adopters migrate. + """ + raise NotImplementedError( + f"{type(self).__name__} must implement atomic hold(scope_key, key)" + ) + + async def supports_atomic_hold(self) -> bool: + """Return whether ``hold`` provides the backend's atomic guarantee. + + This explicit capability is delegable by wrappers such as + :class:`LazyBackend`; method-identity inspection is not. + """ + return type(self).hold is not IdempotencyBackend.hold + + async def supports_atomic_put_if_absent(self) -> bool: + """Return whether ``put_if_absent`` is implemented atomically.""" + return ( + type(self).put_if_absent is not IdempotencyBackend.put_if_absent + or await self.supports_atomic_hold() + ) + + async def put_if_absent(self, scope_key: str, key: str, entry: CachedResponse) -> bool: + """Atomically insert a fresh/expired slot; return whether it won. + + The default composes ``hold`` with the legacy ``get``/``put`` API, + allowing custom backends to implement one locking primitive. Backends + with a native conditional insert should override this method. + """ + async with self.hold(scope_key, key): + if await self.get(scope_key, key) is not None: + return False + await self.put(scope_key, key, entry) + return True + @abstractmethod async def delete_expired(self, now_epoch: float | None = None) -> int: """Best-effort sweep of expired entries. Returns the count removed. @@ -139,6 +206,9 @@ class MemoryBackend(IdempotencyBackend): def __init__(self, *, clock: Callable[[], float] = time.time) -> None: self._store: dict[tuple[str, str], CachedResponse] = {} self._lock = asyncio.Lock() + self._key_locks: weakref.WeakValueDictionary[tuple[str, str], asyncio.Lock] = ( + weakref.WeakValueDictionary() + ) self._clock = clock async def get(self, scope_key: str, key: str) -> CachedResponse | None: @@ -162,6 +232,28 @@ async def put( async with self._lock: self._store[(scope_key, key)] = entry + @asynccontextmanager + async def hold(self, scope_key: str, key: str) -> AsyncIterator[None]: + """Serialize one idempotent handler execution in this process.""" + slot = (scope_key, key) + async with self._lock: + key_lock = self._key_locks.get(slot) + if key_lock is None: + key_lock = asyncio.Lock() + self._key_locks[slot] = key_lock + async with key_lock: + yield + + async def put_if_absent(self, scope_key: str, key: str, entry: CachedResponse) -> bool: + """Atomically claim a missing or expired slot.""" + slot = (scope_key, key) + async with self._lock: + existing = self._store.get(slot) + if existing is not None and existing.expires_at_epoch > self._clock(): + return False + self._store[slot] = entry + return True + async def delete_expired(self, now_epoch: float | None = None) -> int: cutoff = now_epoch if now_epoch is not None else self._clock() async with self._lock: @@ -198,15 +290,17 @@ class PgBackend(IdempotencyBackend): from adcp.server.idempotency import IdempotencyStore, PgBackend pool = AsyncConnectionPool("postgresql://...", min_size=2, max_size=10) - backend = PgBackend(pool=pool) + lock_pool = AsyncConnectionPool("postgresql://...", min_size=2, max_size=10) + backend = PgBackend(pool=pool, lock_pool=lock_pool) await backend.create_schema() # idempotent; safe to call on every boot store = IdempotencyStore(backend=backend, ttl_seconds=86400) - **Atomicity caveat (v1).** ``put`` commits on a fresh pool connection — - the cache write is NOT in the same transaction as the handler's - business writes. A crash between handler success and cache commit - leaves the slot empty; the next retry re-executes the handler. + **Atomicity caveat.** The backend holds a Postgres advisory lock and writes + the cache in its transaction, but the cache write is NOT automatically in + the same transaction as the handler's unrelated business writes. A crash + after an external side effect but before cache commit can still leave the + slot empty; the next retry re-executes the handler. Idempotent handlers absorb this without harm. **Handlers with non-idempotent side effects** (e.g., ``INSERT INTO media_buys`` without a unique constraint on the buyer's idempotency_key) need @@ -261,6 +355,10 @@ class PgBackend(IdempotencyBackend): :param pool: ``psycopg_pool.AsyncConnectionPool`` owned by the caller. Each operation acquires a short-lived connection. We don't open, own, or close the pool. + :param lock_pool: A distinct caller-owned pool reserved for advisory-lock + transactions. It MUST NOT be the business/cache ``pool``: ``hold`` + keeps one connection checked out while adopter code runs, and sharing + that pool with handler SQL can deadlock under saturation. :param table_name: Override the default table name. Useful for multi-tenant schema scoping. Default ``adcp_idempotency``. @@ -274,12 +372,19 @@ def __init__( self, *, pool: Any, # psycopg_pool.AsyncConnectionPool — Any avoids runtime psycopg import + lock_pool: Any, table_name: str = DEFAULT_IDEMPOTENCY_TABLE, ) -> None: if not _PG_AVAILABLE: raise ImportError(_PG_INSTALL_HINT) + if lock_pool is pool: + raise ValueError("lock_pool must be distinct from pool to prevent handler deadlocks") self._pool = pool + self._lock_pool = lock_pool self._table = _safe_identifier(table_name) + self._active_connection: ContextVar[tuple[Any, asyncio.Task[Any] | None] | None] = ( + ContextVar(f"adcp_idempotency_connection_{id(self)}", default=None) + ) # Pre-format SQL once. Validated identifier so f-string interpolation # is byte-safe; values always go through %s parameterization. Same @@ -309,6 +414,16 @@ def __init__( f"WHERE {t}.expires_at <= now()" ) self._sql_delete_expired = f"DELETE FROM {t} WHERE expires_at <= %s" # noqa: S608 + self._sql_lock = "SELECT pg_advisory_xact_lock(hashtextextended(%s, 6217))" + self._sql_put_if_absent = ( + f"INSERT INTO {t} " # noqa: S608 + f"(scope_key, key, payload_hash, response, expires_at) " + f"VALUES (%s, %s, %s, %s::jsonb, %s) " + f"ON CONFLICT (scope_key, key) DO UPDATE SET " + f" payload_hash = EXCLUDED.payload_hash, response = EXCLUDED.response, " + f" expires_at = EXCLUDED.expires_at " + f"WHERE {t}.expires_at <= now() RETURNING 1" + ) async def create_schema(self) -> None: """Bootstrap the table + index. Idempotent. @@ -341,17 +456,25 @@ async def get(self, scope_key: str, key: str) -> CachedResponse | None: sweeps them. ``get`` self-filters via ``expires_at > now()`` so a stale row never replays. """ + active = self._active_connection.get() + if active is not None and active[1] is asyncio.current_task(): + return await self._get_on_connection(active[0], scope_key, key) async with self._pool.connection() as conn: - cur = await conn.execute(self._sql_get, (scope_key, key)) - row = await cur.fetchone() - if row is None: - return None - payload_hash, response, expires_at = row - return CachedResponse( - payload_hash=payload_hash, - response=response if isinstance(response, dict) else json.loads(response), - expires_at_epoch=_to_epoch(expires_at), - ) + return await self._get_on_connection(conn, scope_key, key) + + async def _get_on_connection( + self, conn: Any, scope_key: str, key: str + ) -> CachedResponse | None: + cur = await conn.execute(self._sql_get, (scope_key, key)) + row = await cur.fetchone() + if row is None: + return None + payload_hash, response, expires_at = row + return CachedResponse( + payload_hash=payload_hash, + response=response if isinstance(response, dict) else json.loads(response), + expires_at_epoch=_to_epoch(expires_at), + ) async def put( self, @@ -366,9 +489,42 @@ async def put( that window is a legitimate retry of the write itself. """ expires_at_dt = datetime.fromtimestamp(entry.expires_at_epoch, tz=timezone.utc) + params = ( + scope_key, + key, + entry.payload_hash, + json.dumps(entry.response), + expires_at_dt, + ) + active = self._active_connection.get() + if active is not None and active[1] is asyncio.current_task(): + await active[0].execute(self._sql_put, params) + return + async with self._pool.connection() as conn: + await conn.execute(self._sql_put, params) + + @asynccontextmanager + async def hold(self, scope_key: str, key: str) -> AsyncIterator[None]: + """Hold a cross-process transaction advisory lock for this key. + + The same pooled connection remains checked out while the handler runs; + nested ``get``/``put`` calls reuse it via a context-local binding. + """ + lock_identity = json.dumps([scope_key, key], separators=(",", ":")) + async with self._lock_pool.connection() as conn, conn.transaction(): + await conn.execute(self._sql_lock, (lock_identity,)) + token = self._active_connection.set((conn, asyncio.current_task())) + try: + yield + finally: + self._active_connection.reset(token) + + async def put_if_absent(self, scope_key: str, key: str, entry: CachedResponse) -> bool: + """Atomically insert a webhook dedup marker, including stale replace.""" + expires_at_dt = datetime.fromtimestamp(entry.expires_at_epoch, tz=timezone.utc) async with self._pool.connection() as conn: - await conn.execute( - self._sql_put, + cur = await conn.execute( + self._sql_put_if_absent, ( scope_key, key, @@ -377,6 +533,7 @@ async def put( expires_at_dt, ), ) + return await cur.fetchone() is not None async def delete_expired(self, now_epoch: float | None = None) -> int: """Best-effort sweep of expired entries. Returns rows removed.""" diff --git a/src/adcp/server/idempotency/lazy.py b/src/adcp/server/idempotency/lazy.py index 5aaa0e758..95864418f 100644 --- a/src/adcp/server/idempotency/lazy.py +++ b/src/adcp/server/idempotency/lazy.py @@ -13,7 +13,8 @@ async def _resolve() -> IdempotencyBackend: pool = await app.get_pg_pool() - backend = PgBackend(pool=pool) + lock_pool = await app.get_idempotency_lock_pool() + backend = PgBackend(pool=pool, lock_pool=lock_pool) await backend.create_schema() return backend @@ -33,7 +34,8 @@ async def _resolve() -> IdempotencyBackend: from __future__ import annotations import asyncio -from collections.abc import Awaitable, Callable +from collections.abc import AsyncIterator, Awaitable, Callable +from contextlib import asynccontextmanager from adcp.server.idempotency.backends import CachedResponse, IdempotencyBackend @@ -108,6 +110,20 @@ async def get(self, scope_key: str, key: str) -> CachedResponse | None: async def put(self, scope_key: str, key: str, entry: CachedResponse) -> None: await (await self._resolve()).put(scope_key, key, entry) + @asynccontextmanager + async def hold(self, scope_key: str, key: str) -> AsyncIterator[None]: + async with (await self._resolve()).hold(scope_key, key): + yield + + async def put_if_absent(self, scope_key: str, key: str, entry: CachedResponse) -> bool: + return await (await self._resolve()).put_if_absent(scope_key, key, entry) + + async def supports_atomic_hold(self) -> bool: + return await (await self._resolve()).supports_atomic_hold() + + async def supports_atomic_put_if_absent(self) -> bool: + return await (await self._resolve()).supports_atomic_put_if_absent() + async def delete_expired(self, now_epoch: float | None = None) -> int: return await (await self._resolve()).delete_expired(now_epoch) diff --git a/src/adcp/server/idempotency/store.py b/src/adcp/server/idempotency/store.py index fbd558835..bb7bd2843 100644 --- a/src/adcp/server/idempotency/store.py +++ b/src/adcp/server/idempotency/store.py @@ -28,20 +28,26 @@ from __future__ import annotations +import asyncio import copy import hashlib import logging import time import warnings import weakref -from collections.abc import Awaitable, Callable +from collections.abc import AsyncIterator, Awaitable, Callable +from contextlib import asynccontextmanager from functools import wraps from typing import Any from pydantic import BaseModel from adcp.exceptions import IdempotencyConflictError, IdempotencyScopeError -from adcp.server.idempotency.backends import CachedResponse, IdempotencyBackend +from adcp.server.idempotency.backends import ( + CachedResponse, + IdempotencyBackend, + _legacy_backend_lock_state, +) from adcp.server.idempotency.canonicalize import canonical_json_sha256 logger = logging.getLogger(__name__) @@ -59,6 +65,20 @@ # only granted by IdempotencyStore.wrap itself. _WRAPPED_FUNCTIONS: weakref.WeakSet[Callable[..., Any]] = weakref.WeakSet() +# Keyed idempotency operations continue after the request task is cancelled: +# cancellation cannot stop a synchronous adopter thread, and dropping the lock +# early would allow a retry to execute concurrently. Keep strong references +# until these detached operations settle. +_SUPERVISED_OPERATIONS: set[asyncio.Task[Any]] = set() + + +def _finish_supervised_operation(task: asyncio.Task[Any]) -> None: + """Drop a supervised operation and consume its terminal exception.""" + _SUPERVISED_OPERATIONS.discard(task) + if task.cancelled(): + return + task.exception() + def is_wrapped(fn: Any) -> bool: """Return True if ``fn`` was produced by :meth:`IdempotencyStore.wrap`. @@ -111,6 +131,34 @@ def __init__( self.ttl_seconds = ttl_seconds self._hash_fn = hash_fn self._clock = clock + self._warned_fallback_hold = False + + @asynccontextmanager + async def _hold(self, scope_key: str, key: str) -> AsyncIterator[None]: + """Use native backend locking or a deprecated process-local fallback.""" + if await self.backend.supports_atomic_hold(): + async with self.backend.hold(scope_key, key): + yield + return + + if not self._warned_fallback_hold: + warnings.warn( + f"{type(self.backend).__name__} does not implement hold(); using " + "process-local idempotency locking. Implement hold() for " + "cross-process atomicity; this compatibility fallback is deprecated.", + DeprecationWarning, + stacklevel=3, + ) + self._warned_fallback_hold = True + slot = (scope_key, key) + state = _legacy_backend_lock_state(self.backend) + async with state.guard: + lock = state.locks.get(slot) + if lock is None: + lock = asyncio.Lock() + state.locks[slot] = lock + async with lock: + yield def capability(self) -> dict[str, Any]: """Return the capabilities fragment declaring this store's replay window. @@ -178,28 +226,16 @@ async def _wrapped(*args: Any, **kwargs: Any) -> Any: payload_hash = self._hash_fn(params_dict) - cached = await self.backend.get(scope_key, idempotency_key) - if cached is not None: + def _replay_cached(cached: CachedResponse) -> Any: if cached.payload_hash == payload_hash: logger.debug( "idempotency replay: scope=%s key_prefix=%s", _scope_log_id(scope_key), idempotency_key[:8], ) - # AdCP L1/security idempotency rule 4: the replay - # envelope MUST carry ``replayed: true`` so buyer - # agents can suppress side effects (notifications, - # webhook dispatch, memory writes) on retry. The - # store owns this — sellers can't inject at the - # right point (cache lookup happens here, wire - # serialization happens later). The injection - # lands on the cloned dict, not ``cached.response``, - # so multiple replays of the same key all carry - # exactly one ``replayed: true`` without compounding. replay = _clone_response(cached.response) replay["replayed"] = True return replay - # Same key, different payload — spec-defined conflict. raise IdempotencyConflictError( operation=operation, errors=[ @@ -213,40 +249,68 @@ async def _wrapped(*args: Any, **kwargs: Any) -> Any: ], ) - response = await handler(*args, **kwargs) - # Deep-copy when caching so post-return mutation of the caller's - # copy can't poison future replays. `_clone_response` also deep- - # copies on the hit path, giving independent objects per replay. - response_dict = copy.deepcopy(_to_dict(response)) - entry = CachedResponse( - payload_hash=payload_hash, - response=response_dict, - expires_at_epoch=self._clock() + self.ttl_seconds, - ) - # Commit cache AFTER handler returns. Atomicity with the handler's - # side effects depends on the backend: MemoryBackend is best-effort - # (no transactional relationship to external resources); PgBackend - # (follow-up) will commit in the same transaction when the handler - # uses the same engine. On put failure we log loudly and return - # the handler's response — swallowing the exception would be wrong - # (operators need the signal that caching is broken), and raising - # would look to the caller like the handler failed, triggering a - # retry that re-executes side effects. Best compromise: warn - # operators, return the result, and accept that the next retry - # with this key will re-execute. + # A pure replay does not need an execution lock. Read once before + # acquiring the backend hold, then re-check inside the hold on a + # miss to preserve first-writer-wins under concurrent requests. + cached = await self.backend.get(scope_key, idempotency_key) + if cached is not None: + return _replay_cached(cached) + + async def _execute_locked() -> Any: + # The backend lock spans lookup, handler execution, and commit. + # This is the critical invariant: a concurrent request for the + # same scoped key waits, then observes the winner's cached result. + async with self._hold(scope_key, idempotency_key): + cached = await self.backend.get(scope_key, idempotency_key) + if cached is not None: + return _replay_cached(cached) + + response = await handler(*args, **kwargs) + # Deep-copy when caching so post-return mutation of the caller's + # copy can't poison future replays. `_clone_response` also deep- + # copies on the hit path, giving independent objects per replay. + response_dict = copy.deepcopy(_to_dict(response)) + entry = CachedResponse( + payload_hash=payload_hash, + response=response_dict, + expires_at_epoch=self._clock() + self.ttl_seconds, + ) + # Commit while the execution lock is still held. This does not + # make unrelated business writes transactional with the cache, + # but it prevents a concurrent duplicate handler execution. + try: + await self.backend.put(scope_key, idempotency_key, entry) + except Exception: + logger.warning( + "Idempotency cache put failed for scope=%s key_prefix=%s — " + "handler completed but a subsequent retry with this key will " + "re-execute rather than replay. This indicates an operational " + "issue with the idempotency backend.", + _scope_log_id(scope_key), + idempotency_key[:8], + exc_info=True, + ) + return response + + execution_task = asyncio.create_task(_execute_locked()) + _SUPERVISED_OPERATIONS.add(execution_task) + execution_task.add_done_callback(_finish_supervised_operation) try: - await self.backend.put(scope_key, idempotency_key, entry) - except Exception: - logger.warning( - "Idempotency cache put failed for scope=%s key_prefix=%s — " - "handler completed but a subsequent retry with this key will " - "re-execute rather than replay. This indicates an operational " - "issue with the idempotency backend.", - _scope_log_id(scope_key), - idempotency_key[:8], - exc_info=True, - ) - return response + return await asyncio.shield(execution_task) + except asyncio.CancelledError: + + def _log_late_failure(task: asyncio.Task[Any]) -> None: + if task.cancelled(): + return + exc = task.exception() + if exc is not None: + logger.error( + "Idempotency operation failed after its request was cancelled", + exc_info=(type(exc), exc, exc.__traceback__), + ) + + execution_task.add_done_callback(_log_late_failure) + raise # Register the wrapper for the boot-time validator at # adcp.decisioning.validate_idempotency. WeakSet membership — diff --git a/src/adcp/server/idempotency/webhook_dedup.py b/src/adcp/server/idempotency/webhook_dedup.py index 5d3eef01d..6e4900701 100644 --- a/src/adcp/server/idempotency/webhook_dedup.py +++ b/src/adcp/server/idempotency/webhook_dedup.py @@ -25,11 +25,17 @@ from __future__ import annotations +import asyncio import logging import time +import warnings from collections.abc import Callable -from adcp.server.idempotency.backends import CachedResponse, IdempotencyBackend +from adcp.server.idempotency.backends import ( + CachedResponse, + IdempotencyBackend, + _legacy_backend_lock_state, +) logger = logging.getLogger(__name__) @@ -80,6 +86,41 @@ def __init__( self.ttl_seconds = ttl_seconds self.namespace = namespace self._clock = clock + self._warned_legacy_backend = False + + async def _put_if_absent( + self, + scope_key: str, + key: str, + entry: CachedResponse, + ) -> bool: + """Use backend atomicity or a warned process-local legacy fallback.""" + if await self.backend.supports_atomic_put_if_absent(): + return await self.backend.put_if_absent(scope_key, key, entry) + + if not self._warned_legacy_backend: + warnings.warn( + f"{type(self.backend).__name__} implements neither put_if_absent() " + "nor hold(); using process-local webhook dedup locking. Implement " + "an atomic operation for cross-process safety; this compatibility " + "fallback is deprecated.", + DeprecationWarning, + stacklevel=3, + ) + self._warned_legacy_backend = True + + slot = (scope_key, key) + state = _legacy_backend_lock_state(self.backend) + async with state.guard: + lock = state.locks.get(slot) + if lock is None: + lock = asyncio.Lock() + state.locks[slot] = lock + async with lock: + if await self.backend.get(scope_key, key) is not None: + return False + await self.backend.put(scope_key, key, entry) + return True async def check_and_record(self, sender_id: str, idempotency_key: str) -> bool: """Atomically check for first-seen and record if new. @@ -88,18 +129,8 @@ async def check_and_record(self, sender_id: str, idempotency_key: str) -> bool: processed), ``False`` on duplicate (caller MUST still return 2xx to the sender — the event was delivered successfully, it's just a retry). - Race note: the check-then-put pattern is not atomic across concurrent - callers unless the backend provides its own atomicity. MemoryBackend - serializes individual ``get`` and ``put`` under an ``asyncio.Lock`` but - does NOT bracket them together — two concurrent retries of the same - event CAN both observe "first-seen" and both process the event. That's - a tolerable failure mode: the ultimate guarantee is "at most once per - replay window in the common case"; a concurrent retry arriving in the - same few milliseconds is rare and, if it happens, produces the same - "duplicated side effect" outcome the at-least-once contract already - warns callers to tolerate. PgBackend implementations SHOULD use - ``INSERT ... ON CONFLICT DO NOTHING`` returning ``rowcount`` for - lock-free atomicity. + The backend performs a single atomic insert-or-reject operation, so + concurrent deliveries cannot both be reported as first-seen. """ if not sender_id: raise ValueError("sender_id must be a non-empty string") @@ -107,22 +138,13 @@ async def check_and_record(self, sender_id: str, idempotency_key: str) -> bool: raise ValueError("idempotency_key must be a non-empty string") scoped_sender = f"{self.namespace}:{sender_id}" - existing = await self.backend.get(scoped_sender, idempotency_key) - if existing is not None: - logger.debug( - "webhook dedup: duplicate sender=%s key_prefix=%s", - sender_id, - idempotency_key[:8], - ) - return False - entry = CachedResponse( payload_hash=_SENTINEL_HASH, response={}, expires_at_epoch=self._clock() + self.ttl_seconds, ) try: - await self.backend.put(scoped_sender, idempotency_key, entry) + inserted = await self._put_if_absent(scoped_sender, idempotency_key, entry) except Exception: # Same fail-open reasoning as the request-side store: log and # process. Swallowing the put failure means this event MIGHT @@ -135,7 +157,14 @@ async def check_and_record(self, sender_id: str, idempotency_key: str) -> bool: idempotency_key[:8], exc_info=True, ) - return True + return True + if not inserted: + logger.debug( + "webhook dedup: duplicate sender=%s key_prefix=%s", + sender_id, + idempotency_key[:8], + ) + return inserted __all__ = ["WebhookDedupStore"] diff --git a/src/adcp/server/serve.py b/src/adcp/server/serve.py index b4676fa5b..261e77bee 100644 --- a/src/adcp/server/serve.py +++ b/src/adcp/server/serve.py @@ -58,6 +58,7 @@ async def get_adcp_capabilities(self, params, context=None): from a2a.server.tasks.push_notification_config_store import ( PushNotificationConfigStore, ) + from a2a.server.tasks.push_notification_sender import PushNotificationSender from a2a.server.tasks.task_store import TaskStore from adcp.server.a2a_server import MessageParser, PublicUrlResolver @@ -160,6 +161,7 @@ class ServeConfig: # --- A2A / both --- task_store: TaskStore | None = None push_config_store: PushNotificationConfigStore | None = None + push_sender: PushNotificationSender | None = None message_parser: MessageParser | None = None public_url: str | PublicUrlResolver | None = None @@ -191,7 +193,13 @@ class ServeConfig: debug_public: bool = False def __post_init__(self) -> None: - _a2a_only = ("task_store", "push_config_store", "message_parser", "public_url") + _a2a_only = ( + "task_store", + "push_config_store", + "push_sender", + "message_parser", + "public_url", + ) # ``session_idle_timeout`` (default 1800.0) is excluded from # the warning list: the ``not in (None, False)`` heuristic # treats any non-falsy default as "set" and would fire @@ -602,6 +610,7 @@ def serve( context_factory: ContextFactory | None = None, task_store: TaskStore | None = None, push_config_store: PushNotificationConfigStore | None = None, + push_sender: PushNotificationSender | None = None, middleware: Sequence[SkillMiddleware] | None = None, asgi_middleware: Sequence[ASGIMiddlewareEntry] | None = None, message_parser: MessageParser | None = None, @@ -668,6 +677,9 @@ def serve( ``UnsupportedOperationError`` — clients cannot register subscriptions at all. See ``examples/a2a_db_tasks.py`` for a durable reference implementation. + push_sender: Optional a2a-sdk ``PushNotificationSender`` for + delivering task updates to subscriptions in ``push_config_store`` + (A2A transport only). Configure both to enable built-in delivery. middleware: Optional sequence of :data:`SkillMiddleware` callables wrapping every skill dispatch on both the MCP and A2A transports. Use for audit logging, activity-feed hooks, @@ -941,6 +953,7 @@ async def force_account_status(self, account_id, status): context_factory = config.context_factory task_store = config.task_store push_config_store = config.push_config_store + push_sender = config.push_sender middleware = config.middleware asgi_middleware = config.asgi_middleware message_parser = config.message_parser @@ -1014,6 +1027,7 @@ async def force_account_status(self, account_id, status): context_factory=context_factory, task_store=task_store, push_config_store=push_config_store, + push_sender=push_sender, middleware=middleware, asgi_middleware=asgi_middleware, message_parser=message_parser, @@ -1070,6 +1084,7 @@ async def force_account_status(self, account_id, status): context_factory=context_factory, task_store=task_store, push_config_store=push_config_store, + push_sender=push_sender, middleware=middleware, asgi_middleware=asgi_middleware, message_parser=message_parser, @@ -1663,6 +1678,7 @@ def _serve_a2a( context_factory: ContextFactory | None = None, task_store: TaskStore | None = None, push_config_store: PushNotificationConfigStore | None = None, + push_sender: PushNotificationSender | None = None, middleware: Sequence[SkillMiddleware] | None = None, asgi_middleware: Sequence[ASGIMiddlewareEntry] | None = None, message_parser: MessageParser | None = None, @@ -1695,6 +1711,7 @@ def _serve_a2a( context_factory=context_factory, task_store=task_store, push_config_store=push_config_store, + push_sender=push_sender, middleware=middleware, message_parser=message_parser, advertise_all=advertise_all, @@ -1749,6 +1766,7 @@ def _build_mcp_and_a2a_app( context_factory: ContextFactory | None = None, task_store: TaskStore | None = None, push_config_store: PushNotificationConfigStore | None = None, + push_sender: PushNotificationSender | None = None, middleware: Sequence[SkillMiddleware] | None = None, message_parser: MessageParser | None = None, advertise_all: bool = False, @@ -1858,6 +1876,7 @@ def _build_mcp_and_a2a_app( context_factory=context_factory, task_store=task_store, push_config_store=push_config_store, + push_sender=push_sender, middleware=middleware, message_parser=message_parser, advertise_all=advertise_all, @@ -2004,6 +2023,7 @@ def _serve_mcp_and_a2a( context_factory: ContextFactory | None = None, task_store: TaskStore | None = None, push_config_store: PushNotificationConfigStore | None = None, + push_sender: PushNotificationSender | None = None, middleware: Sequence[SkillMiddleware] | None = None, asgi_middleware: Sequence[ASGIMiddlewareEntry] | None = None, message_parser: MessageParser | None = None, @@ -2059,6 +2079,7 @@ def _serve_mcp_and_a2a( context_factory=context_factory, task_store=task_store, push_config_store=push_config_store, + push_sender=push_sender, middleware=middleware, message_parser=message_parser, advertise_all=advertise_all, diff --git a/src/adcp/server/test_controller.py b/src/adcp/server/test_controller.py index bd15133f3..e862a2871 100644 --- a/src/adcp/server/test_controller.py +++ b/src/adcp/server/test_controller.py @@ -689,11 +689,18 @@ async def _apply_sandbox_gate( allowed = account_is_sandbox or ref_sandbox or context_sandbox or env_sandbox + if resolved_account is not None and not account_is_sandbox: + return _controller_error( + "FORBIDDEN", + "comply_test_controller requires a sandbox or mock account; " + "resolved account is in live mode.", + ) + if not allowed: return _controller_error( "PERMISSION_DENIED", "comply_test_controller requires a sandbox or mock account; " - "resolved account is in live mode (or no account resolved).", + "no account resolved and no sandbox signal present.", ) return None diff --git a/src/adcp/signing/__init__.py b/src/adcp/signing/__init__.py index e9ea83287..d35268095 100644 --- a/src/adcp/signing/__init__.py +++ b/src/adcp/signing/__init__.py @@ -241,7 +241,13 @@ SigningAlgorithm, SigningProvider, ) -from adcp.signing.replay import InMemoryReplayStore, ReplayStore +from adcp.signing.replay import ( + AtomicReplayStore, + InMemoryReplayStore, + ReplayClaimResult, + ReplayStore, + supports_atomic_claim, +) from adcp.signing.revocation import RevocationChecker, RevocationList from adcp.signing.revocation_fetcher import ( DEFAULT_GRACE_MULTIPLIER, @@ -315,6 +321,7 @@ def __init__(self, *args: object, **kwargs: object) -> None: "AsyncJwksFetcher", "AsyncJwksResolver", "AsyncRevocationListFetcher", + "AtomicReplayStore", "BrandAgentType", "BrandAuthorizationReason", "BrandAuthorizationResolver", @@ -377,6 +384,8 @@ def __init__(self, *args: object, **kwargs: object) -> None: "REQUEST_SIGNATURE_WINDOW_INVALID", "REVOCATION_LIST_TYP", "ReplayStore", + "ReplayClaimResult", + "supports_atomic_claim", "RevocationChecker", "RevocationList", "RevocationListFetchError", diff --git a/src/adcp/signing/_bounded_http.py b/src/adcp/signing/_bounded_http.py new file mode 100644 index 000000000..3c7ce6ef9 --- /dev/null +++ b/src/adcp/signing/_bounded_http.py @@ -0,0 +1,67 @@ +"""Small streaming body readers shared by signing discovery fetchers.""" + +from __future__ import annotations + +import httpx + + +class ResponseTooLargeError(ValueError): + """A decoded HTTP response exceeded its configured byte budget.""" + + def __init__(self, *, limit: int, received: int) -> None: + super().__init__(f"response exceeds {limit} bytes (received at least {received})") + self.limit = limit + self.received = received + + +def _reject_encoded_response(response: httpx.Response) -> None: + """Reject content codings that would be expanded before our byte limit.""" + content_encoding = response.headers.get("content-encoding", "identity") + codings = {coding.strip().lower() for coding in content_encoding.split(",")} + if codings - {"", "identity"}: + raise ValueError("encoded HTTP responses are not accepted") + + +def read_limited_bytes(response: httpx.Response, *, limit: int) -> bytes: + """Stream at most ``limit`` decoded bytes from ``response``.""" + _reject_encoded_response(response) + content_length = response.headers.get("content-length") + if content_length is not None: + try: + declared = int(content_length) + except ValueError: + declared = 0 + if declared > limit: + raise ResponseTooLargeError(limit=limit, received=declared) + + body = bytearray() + chunk_size = max(1, min(64 * 1024, limit + 1)) + for chunk in response.iter_bytes(chunk_size=chunk_size): + body.extend(chunk) + if len(body) > limit: + raise ResponseTooLargeError(limit=limit, received=len(body)) + return bytes(body) + + +async def async_read_limited_bytes(response: httpx.Response, *, limit: int) -> bytes: + """Async counterpart to :func:`read_limited_bytes`.""" + _reject_encoded_response(response) + content_length = response.headers.get("content-length") + if content_length is not None: + try: + declared = int(content_length) + except ValueError: + declared = 0 + if declared > limit: + raise ResponseTooLargeError(limit=limit, received=declared) + + body = bytearray() + chunk_size = max(1, min(64 * 1024, limit + 1)) + async for chunk in response.aiter_bytes(chunk_size=chunk_size): + body.extend(chunk) + if len(body) > limit: + raise ResponseTooLargeError(limit=limit, received=len(body)) + return bytes(body) + + +__all__ = ["ResponseTooLargeError", "async_read_limited_bytes", "read_limited_bytes"] diff --git a/src/adcp/signing/agent_resolver.py b/src/adcp/signing/agent_resolver.py index 41edf0053..28549b964 100644 --- a/src/adcp/signing/agent_resolver.py +++ b/src/adcp/signing/agent_resolver.py @@ -49,6 +49,8 @@ import httpx from pydantic import BaseModel, ConfigDict, Field +from adcp.signing._bounded_http import ResponseTooLargeError, async_read_limited_bytes +from adcp.signing._idna_canonicalize import canonicalize_host from adcp.signing.brand_jwks import ( BrandAgentType, BrandJsonJwksResolver, @@ -60,6 +62,12 @@ StaticJwksResolver, async_default_jwks_fetcher, ) +from adcp.signing.replay import ( + InMemoryReplayStore, + ReplayClaimResult, + ReplayStore, + supports_atomic_claim, +) #: Maximum capabilities response body in bytes. Capabilities documents #: are larger than brand.json (operator/agent declarations, supported @@ -230,7 +238,58 @@ async def _fetch_capabilities( try: async with client_cm as client: try: - response = await client.get(url, headers={"accept": "application/json"}) + request_cm = client.stream( + "GET", + url, + headers={"accept": "application/json", "accept-encoding": "identity"}, + ) + async with request_cm as response: + if 300 <= response.status_code < 400 and "location" in response.headers: + if hop == max_redirects: + raise AgentResolverError( + "capabilities_unreachable", + f"capabilities fetch hit redirect limit ({max_redirects})", + ) + url = str(httpx.URL(url).join(response.headers["location"])) + try: + transport = build_async_ip_pinned_transport( + url, allow_private=allow_private + ) + except SSRFValidationError as exc: + raise AgentResolverError( + "capabilities_unreachable", + f"redirect target failed SSRF check: {exc}", + ) from exc + client_cm = httpx.AsyncClient( + transport=transport, + timeout=timeout_seconds, + follow_redirects=False, + trust_env=False, + ) + continue + + if response.status_code != 200: + raise AgentResolverError( + "capabilities_unreachable", + f"capabilities fetch returned HTTP {response.status_code}", + ) + + try: + body_bytes = await async_read_limited_bytes( + response, limit=max_body_bytes + ) + except ResponseTooLargeError as exc: + raise AgentResolverError( + "capabilities_invalid", f"capabilities {exc}" + ) from exc + + try: + parsed = json.loads(body_bytes) + except (ValueError, UnicodeDecodeError) as exc: + raise AgentResolverError( + "capabilities_invalid", + "capabilities response is not valid JSON", + ) from exc except SSRFValidationError as exc: raise AgentResolverError( "capabilities_unreachable", @@ -242,53 +301,6 @@ async def _fetch_capabilities( f"capabilities fetch failed: {exc}", ) from exc - if 300 <= response.status_code < 400 and "location" in response.headers: - if hop == max_redirects: - raise AgentResolverError( - "capabilities_unreachable", - f"capabilities fetch hit redirect limit ({max_redirects})", - ) - url = str(httpx.URL(url).join(response.headers["location"])) - # New host → new transport. Rebuild client_cm. - try: - transport = build_async_ip_pinned_transport( - url, allow_private=allow_private - ) - except SSRFValidationError as exc: - raise AgentResolverError( - "capabilities_unreachable", - f"redirect target failed SSRF check: {exc}", - ) from exc - client_cm = httpx.AsyncClient( - transport=transport, - timeout=timeout_seconds, - follow_redirects=False, - trust_env=False, - ) - continue - - if response.status_code != 200: - raise AgentResolverError( - "capabilities_unreachable", - f"capabilities fetch returned HTTP {response.status_code}", - ) - - body_bytes = response.content - if len(body_bytes) > max_body_bytes: - raise AgentResolverError( - "capabilities_invalid", - f"capabilities response exceeds {max_body_bytes} bytes " - f"(got {len(body_bytes)})", - ) - - try: - parsed = response.json() - except (ValueError, httpx.DecodingError, json.JSONDecodeError) as exc: - raise AgentResolverError( - "capabilities_invalid", - "capabilities response is not valid JSON", - ) from exc - if not isinstance(parsed, dict): raise AgentResolverError( "capabilities_invalid", @@ -578,6 +590,53 @@ def resolve_agent( # ---- verify factory ---- +_REPLAY_STORE_UNSET = object() + + +class _NamespacedReplayStore: + """Partition one bounded replay store by canonical counterparty origin.""" + + def __init__(self, backend: ReplayStore, namespace: str) -> None: + self._backend = backend + self._namespace = namespace + + def _keyid(self, keyid: str) -> str: + return f"{len(self._namespace)}:{self._namespace}{keyid}" + + def seen(self, keyid: str, nonce: str) -> bool: + return self._backend.seen(self._keyid(keyid), nonce) + + def remember(self, keyid: str, nonce: str, ttl_seconds: float) -> bool | None: + return self._backend.remember(self._keyid(keyid), nonce, ttl_seconds) + + def at_capacity(self, keyid: str) -> bool: + return self._backend.at_capacity(self._keyid(keyid)) + + def supports_atomic_claim(self) -> bool: + return supports_atomic_claim(self._backend) + + def claim(self, keyid: str, nonce: str, ttl_seconds: float) -> ReplayClaimResult: + if not supports_atomic_claim(self._backend): + raise AttributeError("underlying replay store does not implement claim") + return self._backend.claim(self._keyid(keyid), nonce, ttl_seconds) + + +_DEFAULT_REPLAY_STORE = InMemoryReplayStore() + + +def _default_replay_store_for_origin(origin: str) -> ReplayStore: + """Return an origin partition backed by one process-wide bounded store.""" + return _NamespacedReplayStore(_DEFAULT_REPLAY_STORE, origin) + + +def _canonical_agent_origin(url: str) -> str: + parsed = httpx.URL(url) + host = canonicalize_host(parsed.host) + default_port = 443 if parsed.scheme == "https" else 80 + port = parsed.port or default_port + return f"{parsed.scheme}://{host}:{port}" + + class _BrandJsonStaticJwksResolver(StaticJwksResolver): """A :class:`StaticJwksResolver` carrying the ``"brand_json"`` source discriminant AND the resolved ``jwks_uri``. @@ -622,7 +681,7 @@ async def verify_from_agent_url( brand_id: str | None = None, capability: Any = None, now: float | None = None, - replay_store: Any = None, + replay_store: Any = _REPLAY_STORE_UNSET, revocation_checker: Any = None, revocation_list: Any = None, allow_private_destinations: bool = False, @@ -659,6 +718,12 @@ async def verify_from_agent_url( :class:`AgentResolverError.code` directly — both exception hierarchies are preserved. + When ``replay_store`` is omitted, replay protection uses a bounded, + process-wide in-memory store namespaced by resolved agent URL so separate + counterparties may safely reuse a ``kid``. Pass ``None`` explicitly only + when replay protection is intentionally disabled, or provide a shared + store for multi-process deployments. + Returns ------- VerifiedSigner @@ -713,18 +778,21 @@ async def verify_from_agent_url( # marker the verifier would treat a bare ``StaticJwksResolver`` as a # publisher-pin-equivalent and skip the check — defeating the # production helper's defense against the shared-tenancy spoof. + if replay_store is _REPLAY_STORE_UNSET: + resolved_agent_url = str(resolution.agent_entry.get("url") or resolution.agent_url) + replay_store = _default_replay_store_for_origin(_canonical_agent_origin(resolved_agent_url)) options = VerifyOptions( now=now if now is not None else _time.time(), capability=capability if capability is not None else VerifierCapability(supported=True), operation=operation, jwks_resolver=_BrandJsonStaticJwksResolver(resolution.jwks, jwks_uri=resolution.jwks_uri), - replay_store=replay_store, revocation_checker=revocation_checker, revocation_list=revocation_list, agent_url=resolution.agent_entry.get("url"), - expected_key_origins=resolution.key_origins, + expected_key_origins=resolution.key_origins or {}, signing_purpose=signing_purpose, posture=posture, + replay_store=replay_store, ) return await verify_starlette_request(request, options=options) diff --git a/src/adcp/signing/brand_authz.py b/src/adcp/signing/brand_authz.py index 04ec96a92..13ae6297a 100644 --- a/src/adcp/signing/brand_authz.py +++ b/src/adcp/signing/brand_authz.py @@ -44,6 +44,7 @@ DEFAULT_MAX_AGE_SECONDS, DEFAULT_MAX_BRAND_JSON_BYTES, DEFAULT_MAX_REDIRECTS, + DEFAULT_MAX_STALE_SECONDS, DEFAULT_MIN_COOLDOWN_SECONDS, BrandAgentType, BrandJsonJwksResolver, @@ -144,6 +145,7 @@ def __init__( *, min_cooldown_seconds: float = DEFAULT_MIN_COOLDOWN_SECONDS, max_age_seconds: float = DEFAULT_MAX_AGE_SECONDS, + max_stale_seconds: float = DEFAULT_MAX_STALE_SECONDS, max_redirects: int = DEFAULT_MAX_REDIRECTS, max_body_bytes: int = DEFAULT_MAX_BRAND_JSON_BYTES, allow_private_destinations: bool = False, @@ -158,6 +160,7 @@ def __init__( brand_json_url, min_cooldown_seconds=min_cooldown_seconds, max_age_seconds=max_age_seconds, + max_stale_seconds=max_stale_seconds, max_redirects=max_redirects, max_body_bytes=max_body_bytes, allow_private_destinations=allow_private_destinations, @@ -294,10 +297,14 @@ async def _snapshot(self) -> _BrandJsonSnapshot | BrandJsonResolverError: if self._fetcher.is_stale(snap) and self._fetcher.can_refresh(snap): try: return await self._fetcher.refresh() - except BrandJsonResolverError: - # Stale-on-error: serve the prior snapshot. Matches - # the JWKS resolver's posture exactly. - return snap + except BrandJsonResolverError as exc: + if self._fetcher.can_serve_stale(snap): + return snap + return exc + if self._fetcher.is_stale(snap) and not self._fetcher.can_serve_stale(snap): + return self._fetcher.last_error or BrandJsonResolverError( + "fetch_failed", "expired brand.json authorization snapshot" + ) return snap @@ -312,6 +319,7 @@ def build_brand_json_resolvers( brand_id: str | None = None, min_cooldown_seconds: float = DEFAULT_MIN_COOLDOWN_SECONDS, max_age_seconds: float = DEFAULT_MAX_AGE_SECONDS, + max_stale_seconds: float = DEFAULT_MAX_STALE_SECONDS, max_redirects: int = DEFAULT_MAX_REDIRECTS, max_body_bytes: int = DEFAULT_MAX_BRAND_JSON_BYTES, allow_private_destinations: bool = False, @@ -333,6 +341,7 @@ def build_brand_json_resolvers( brand_json_url, min_cooldown_seconds=min_cooldown_seconds, max_age_seconds=max_age_seconds, + max_stale_seconds=max_stale_seconds, max_redirects=max_redirects, max_body_bytes=max_body_bytes, allow_private_destinations=allow_private_destinations, @@ -346,6 +355,7 @@ def build_brand_json_resolvers( brand_id=brand_id, min_cooldown_seconds=min_cooldown_seconds, max_age_seconds=max_age_seconds, + max_stale_seconds=max_stale_seconds, max_redirects=max_redirects, max_body_bytes=max_body_bytes, allow_private_destinations=allow_private_destinations, @@ -357,6 +367,7 @@ def build_brand_json_resolvers( brand_json_url, min_cooldown_seconds=min_cooldown_seconds, max_age_seconds=max_age_seconds, + max_stale_seconds=max_stale_seconds, max_redirects=max_redirects, max_body_bytes=max_body_bytes, allow_private_destinations=allow_private_destinations, diff --git a/src/adcp/signing/brand_jwks.py b/src/adcp/signing/brand_jwks.py index 3e82d4415..114411e6b 100644 --- a/src/adcp/signing/brand_jwks.py +++ b/src/adcp/signing/brand_jwks.py @@ -36,6 +36,7 @@ from __future__ import annotations import asyncio +import json import re import time from collections.abc import Callable @@ -47,6 +48,7 @@ import httpx import idna +from adcp.signing._bounded_http import ResponseTooLargeError, async_read_limited_bytes from adcp.signing._idna_canonicalize import canonicalize_host from adcp.signing.jwks import ( AsyncCachingJwksResolver, @@ -94,7 +96,11 @@ ] DEFAULT_MIN_COOLDOWN_SECONDS = 30.0 -DEFAULT_MAX_AGE_SECONDS = 3600.0 +# security.mdx:1103 @ AdCP 3.1.8: successful freshness plus bounded +# stale-on-error service must not mask key rotation beyond the 30-minute +# revocation polling ceiling. Split that total budget evenly by default. +DEFAULT_MAX_AGE_SECONDS = 900.0 +DEFAULT_MAX_STALE_SECONDS = 900.0 DEFAULT_MAX_REDIRECTS = 3 DEFAULT_BRAND_JSON_TIMEOUT_SECONDS = 10.0 @@ -187,6 +193,7 @@ def __init__( *, min_cooldown_seconds: float = DEFAULT_MIN_COOLDOWN_SECONDS, max_age_seconds: float = DEFAULT_MAX_AGE_SECONDS, + max_stale_seconds: float = DEFAULT_MAX_STALE_SECONDS, max_redirects: int = DEFAULT_MAX_REDIRECTS, max_body_bytes: int = DEFAULT_MAX_BRAND_JSON_BYTES, allow_private_destinations: bool = False, @@ -197,6 +204,7 @@ def __init__( self._url = brand_json_url self._min_cooldown = min_cooldown_seconds self._max_age = max_age_seconds + self._max_stale = max_stale_seconds self._max_redirects = max_redirects self._max_body_bytes = max_body_bytes self._allow_private = allow_private_destinations @@ -205,6 +213,8 @@ def __init__( self._client_factory = _client_factory self._snapshot: _BrandJsonSnapshot | None = None + self._last_attempt_at: float | None = None + self._last_error: BrandJsonResolverError | None = None # In-flight refresh future for single-flighting concurrent # callers — N tasks hitting a cold cache do ONE fetch, not N. # ``asyncio.Lock`` would also work but SERIALIZES (waiter N+1 @@ -242,7 +252,26 @@ def can_refresh(self, snapshot: _BrandJsonSnapshot | None = None) -> bool: snap = snapshot if snapshot is not None else self._snapshot if snap is None: return True - return self._clock() - snap.fetched_at >= self._min_cooldown + reference = max(snap.fetched_at, self._last_attempt_at or snap.fetched_at) + return self._clock() - reference >= self._min_cooldown + + def can_serve_stale(self, snapshot: _BrandJsonSnapshot | None = None) -> bool: + """Return whether an expired authorization snapshot is within grace.""" + snap = snapshot if snapshot is not None else self._snapshot + if snap is None: + return False + stale_deadline = min( + snap.expires_at + self._max_stale, + # The default trust ceiling is over total snapshot age. Shorter + # explicit cache lifetimes may still use bounded stale-on-error + # grace, but no configuration extends trust past 30 minutes. + snap.fetched_at + DEFAULT_MAX_AGE_SECONDS + DEFAULT_MAX_STALE_SECONDS, + ) + return self._clock() <= stale_deadline + + @property + def last_error(self) -> BrandJsonResolverError | None: + return self._last_error def clear(self) -> None: """Drop the cached snapshot. Next refresh will be unconditional.""" @@ -284,15 +313,21 @@ async def refresh(self) -> _BrandJsonSnapshot: self._refresh_in_flight = None async def _do_refresh(self) -> _BrandJsonSnapshot: - fetched = await _fetch_brand_json( - start_url=self._url, - current_etag=self._snapshot.etag if self._snapshot is not None else None, - max_redirects=self._max_redirects, - allow_private=self._allow_private, - timeout_seconds=self._timeout, - max_body_bytes=self._max_body_bytes, - client_factory=self._client_factory, - ) + self._last_attempt_at = self._clock() + try: + fetched = await _fetch_brand_json( + start_url=self._url, + current_etag=self._snapshot.etag if self._snapshot is not None else None, + max_redirects=self._max_redirects, + allow_private=self._allow_private, + timeout_seconds=self._timeout, + max_body_bytes=self._max_body_bytes, + client_factory=self._client_factory, + ) + except BrandJsonResolverError as exc: + self._last_error = exc + raise + self._last_error = None now = self._clock() if fetched.status == "not_modified" and self._snapshot is not None: @@ -360,6 +395,7 @@ def __init__( brand_id: str | None = None, min_cooldown_seconds: float = DEFAULT_MIN_COOLDOWN_SECONDS, max_age_seconds: float = DEFAULT_MAX_AGE_SECONDS, + max_stale_seconds: float = DEFAULT_MAX_STALE_SECONDS, max_redirects: int = DEFAULT_MAX_REDIRECTS, max_body_bytes: int = DEFAULT_MAX_BRAND_JSON_BYTES, allow_private_destinations: bool = False, @@ -385,6 +421,7 @@ def __init__( brand_json_url, min_cooldown_seconds=min_cooldown_seconds, max_age_seconds=max_age_seconds, + max_stale_seconds=max_stale_seconds, max_redirects=max_redirects, max_body_bytes=max_body_bytes, allow_private_destinations=allow_private_destinations, @@ -393,9 +430,8 @@ def __init__( _client_factory=_client_factory, ) - # Derived selector state. Recomputed whenever the fetcher's - # snapshot identity changes (final_url or etag); cheap to redo, - # so we don't bother diffing the body itself. + # Derived selector state. Recomputed for every successful body + # refresh; ETags are optional and may be reused incorrectly. self._selected: _SelectedAgent | None = None self._selected_for: tuple[str, str | None] | None = None self._inner: AsyncCachingJwksResolver | None = None @@ -419,8 +455,14 @@ async def resolve(self, kid: str) -> dict[str, Any] | None: try: await self._refresh() except BrandJsonResolverError: - # Keep stale on transient failure — same posture as JS. - pass + if not self._fetcher.can_serve_stale(snap): + self._selected = None + self._inner = None + return None + elif self._fetcher.is_stale(snap) and not self._fetcher.can_serve_stale(snap): + self._selected = None + self._inner = None + return None if self._inner is None: return None @@ -477,18 +519,21 @@ async def _refresh(self) -> None: self._sync_selector(snap) def _sync_selector(self, snap: _BrandJsonSnapshot) -> None: - """Reselect the agent if the brand.json snapshot identity changed.""" + """Reselect the agent from the current body, independent of validators.""" identity = (snap.final_url, snap.etag) - if self._selected is not None and self._selected_for == identity: - return - - agent = _select_agent( - snap.data, - snap.final_url, - agent_type=self._agent_type, - agent_id=self._agent_id, - brand_id=self._brand_id, - ) + try: + agent = _select_agent( + snap.data, + snap.final_url, + agent_type=self._agent_type, + agent_id=self._agent_id, + brand_id=self._brand_id, + ) + except BrandJsonResolverError: + self._selected = None + self._selected_for = identity + self._inner = None + raise if self._inner is None or ( self._selected is not None and self._selected.jwks_uri != agent.jwks_uri @@ -537,8 +582,7 @@ async def _fetch_brand_json( Body cap: each response is bounded to ``max_body_bytes`` (default 256 KiB). brand.json is small by design; an adversarial - multi-megabyte body would otherwise be buffered into memory by - ``response.json()``. + multi-megabyte body is stopped during streaming, before JSON parsing. ``client_factory`` is the test seam — production callers pass ``None`` to use the IP-pinned client; tests inject a factory that @@ -589,7 +633,40 @@ async def _fetch_brand_json( try: async with client_cm as client: try: - response = await client.get(url, headers=headers) + request_cm = client.stream( + "GET", url, headers={**headers, "Accept-Encoding": "identity"} + ) + async with request_cm as response: + if hop == 0 and response.status_code == 304: + return _FetchedBrandJson( + status="not_modified", + final_url=url, + data=None, + etag=response.headers.get("etag"), + cache_control=response.headers.get("cache-control"), + ) + if response.status_code != 200: + raise BrandJsonResolverError( + "fetch_failed", + f"brand.json fetch returned HTTP {response.status_code}", + ) + + try: + body = await async_read_limited_bytes(response, limit=max_body_bytes) + except ResponseTooLargeError as exc: + raise BrandJsonResolverError( + "invalid_body", f"brand.json {exc}" + ) from exc + + try: + parsed = json.loads(body) + except (ValueError, UnicodeDecodeError) as exc: + raise BrandJsonResolverError( + "invalid_body", "brand.json response is not valid JSON" + ) from exc + + etag = response.headers.get("etag") + cache_control = response.headers.get("cache-control") except SSRFValidationError as exc: raise BrandJsonResolverError( "fetch_failed", f"brand.json URL failed SSRF check: {exc}" @@ -599,48 +676,12 @@ async def _fetch_brand_json( "fetch_failed", f"brand.json fetch failed: {exc}" ) from exc - if hop == 0 and response.status_code == 304: - return _FetchedBrandJson( - status="not_modified", - final_url=url, - data=None, - etag=response.headers.get("etag"), - cache_control=response.headers.get("cache-control"), - ) - if response.status_code != 200: - raise BrandJsonResolverError( - "fetch_failed", - f"brand.json fetch returned HTTP {response.status_code}", - ) - - # Body-size cap. ``response.content`` is already buffered - # by httpx (we're not streaming); reject if it exceeds - # the cap before paying the JSON-parse cost. - body = response.content - if len(body) > max_body_bytes: - raise BrandJsonResolverError( - "invalid_body", - f"brand.json response exceeds {max_body_bytes} bytes " f"(got {len(body)})", - ) - - try: - parsed = response.json() - except (ValueError, httpx.DecodingError) as exc: - raise BrandJsonResolverError( - "invalid_body", "brand.json response is not valid JSON" - ) from exc except BrandJsonResolverError: raise if not isinstance(parsed, dict): raise BrandJsonResolverError("invalid_body", "brand.json response is not an object") - # Capture response headers once before the client closes — - # used for both the `ok` return below and any 304 returns above - # (already returned by this point). - etag = response.headers.get("etag") - cache_control = response.headers.get("cache-control") - authoritative = parsed.get("authoritative_location") house = parsed.get("house") @@ -1029,5 +1070,6 @@ def _compute_lifetime(cache_control: str | None, max_age: float) -> float: "DEFAULT_BRAND_JSON_TIMEOUT_SECONDS", "DEFAULT_MAX_AGE_SECONDS", "DEFAULT_MAX_REDIRECTS", + "DEFAULT_MAX_STALE_SECONDS", "DEFAULT_MIN_COOLDOWN_SECONDS", ] diff --git a/src/adcp/signing/client.py b/src/adcp/signing/client.py index fcfa43fe3..b2dd9314b 100644 --- a/src/adcp/signing/client.py +++ b/src/adcp/signing/client.py @@ -53,7 +53,9 @@ from collections.abc import Awaitable, Callable, Iterator from contextlib import contextmanager from typing import TYPE_CHECKING, Any +from urllib.parse import urlsplit +from adcp.signing._idna_canonicalize import canonicalize_host from adcp.signing.autosign import ( SigningConfig, current_operation, @@ -81,12 +83,26 @@ """ +def _origin(url: Any) -> tuple[str, str, int] | None: + parsed = urlsplit(str(url)) + if not parsed.scheme or not parsed.hostname: + return None + try: + port = parsed.port + except ValueError as exc: + raise ValueError(f"invalid signing origin: {url!s}") from exc + if port is None: + port = 443 if parsed.scheme.lower() == "https" else 80 + return parsed.scheme.lower(), canonicalize_host(parsed.hostname), port + + def install_signing_event_hook( client: httpx.AsyncClient, *, signing: SigningConfig, seller_capability: RequestSigning | None = None, capability_provider: CapabilityProvider | None = None, + expected_origin: str | None = None, ) -> None: """Install an RFC 9421 request-signing event hook on ``client``. @@ -115,6 +131,12 @@ def install_signing_event_hook( capability needs lazy / re-resolved lookup. Sync and async are both supported. Returning ``None`` means "seller doesn't sign; skip every operation." + expected_origin: + Seller origin that signed requests are allowed to target. If omitted, + the client's ``base_url`` origin is used when available; otherwise the + first request inside :func:`signing_operation` binds the hook to its + origin. Cross-origin redirects and later cross-origin requests fail + before signing. Notes ----- @@ -128,7 +150,14 @@ def install_signing_event_hook( "`seller_capability` or `capability_provider`." ) + bound_origin = ( + _origin(expected_origin) if expected_origin is not None else _origin(client.base_url) + ) + if expected_origin is not None and bound_origin is None: + raise ValueError("expected_origin must be an absolute URL origin") + async def _hook(request: httpx.Request) -> None: + nonlocal bound_origin operation = current_operation.get() # Unset ContextVar → out-of-band call (health check, manual # probe). Skip without consulting capability. @@ -138,6 +167,19 @@ async def _hook(request: httpx.Request) -> None: if operation is None or operation == "get_adcp_capabilities": return + request_origin = _origin(request.url) + if request_origin is None: # pragma: no cover - httpx requests are absolute here + raise ValueError("cannot sign a request without an absolute origin") + if bound_origin is None: + # Bind before awaiting a dynamic provider so concurrent first-use + # requests cannot each establish a different signing origin. + bound_origin = request_origin + elif request_origin != bound_origin: + raise ValueError( + "refusing to sign a cross-origin request; redirects must be " + "handled and re-authorized outside signing_operation" + ) + capability: RequestSigning | None if seller_capability is not None: capability = seller_capability diff --git a/src/adcp/signing/jwks.py b/src/adcp/signing/jwks.py index 554cfd206..4f42f4a66 100644 --- a/src/adcp/signing/jwks.py +++ b/src/adcp/signing/jwks.py @@ -26,7 +26,9 @@ import asyncio import ipaddress +import json import socket +import threading import time from collections.abc import Callable from typing import Any, ClassVar, Literal, Protocol, runtime_checkable @@ -35,6 +37,7 @@ import httpx import idna +from adcp.signing._bounded_http import async_read_limited_bytes, read_limited_bytes from adcp.signing._idna_canonicalize import canonicalize_host from adcp.signing.errors import ( REQUEST_SIGNATURE_JWKS_UNAVAILABLE, @@ -43,7 +46,11 @@ ) DEFAULT_JWKS_COOLDOWN_SECONDS = 30.0 +# security.mdx:1457 @ AdCP 3.1.8 caps JWKS freshness at the revocation +# polling ceiling (30 minutes). +DEFAULT_JWKS_MAX_AGE_SECONDS = 1800.0 DEFAULT_JWKS_TIMEOUT_SECONDS = 10.0 +DEFAULT_MAX_JWKS_BYTES = 1024 * 1024 # Cloud metadata endpoints that MUST be blocked even if somehow marked non-private BLOCKED_METADATA_IPS: frozenset[str] = frozenset( @@ -337,7 +344,12 @@ def resolve_and_validate_host( return host, accepted_ip, port -def default_jwks_fetcher(uri: str, *, allow_private: bool = False) -> dict[str, Any]: +def default_jwks_fetcher( + uri: str, + *, + allow_private: bool = False, + max_body_bytes: int = DEFAULT_MAX_JWKS_BYTES, +) -> dict[str, Any]: """Validate + resolve the URI once, then GET the JWKS over an IP-pinned transport. @@ -362,14 +374,40 @@ def default_jwks_fetcher(uri: str, *, allow_private: bool = False) -> dict[str, follow_redirects=False, trust_env=False, ) as client: - response = client.get(uri, headers={"Accept": "application/json"}) - response.raise_for_status() - body = response.json() + with client.stream( + "GET", uri, headers={"Accept": "application/json", "Accept-Encoding": "identity"} + ) as response: + response.raise_for_status() + body = json.loads(read_limited_bytes(response, limit=max_body_bytes)) if not isinstance(body, dict) or "keys" not in body: raise ValueError(f"JWKS document at {uri!r} has no 'keys' array") return body +def _index_jwks_keys(jwks: dict[str, Any], *, uri: str) -> dict[str, dict[str, Any]]: + """Validate the JWKS container and index object-shaped keys by ``kid``. + + Keys without a ``kid`` remain ignorable, matching the resolver's existing + behavior. Entries that cannot be JWK objects, and present ``kid`` values + that cannot be resolver identifiers, make the document malformed. + """ + keys = jwks.get("keys") + if not isinstance(keys, list): + raise ValueError(f"JWKS document at {uri!r} has no 'keys' array") + + indexed: dict[str, dict[str, Any]] = {} + for index, jwk in enumerate(keys): + if not isinstance(jwk, dict): + raise ValueError(f"JWKS key at index {index} in {uri!r} is not an object") + kid = jwk.get("kid") + if kid is None: + continue + if not isinstance(kid, str): + raise ValueError(f"JWKS key at index {index} in {uri!r} has a non-string 'kid'") + indexed[kid] = jwk + return indexed + + class CachingJwksResolver: """JWKS resolver with per-URI cache and refetch cooldown. @@ -389,46 +427,85 @@ def __init__( *, fetcher: JwksFetcher | None = None, cooldown_seconds: float = DEFAULT_JWKS_COOLDOWN_SECONDS, + max_age_seconds: float = DEFAULT_JWKS_MAX_AGE_SECONDS, allow_private: bool = False, clock: Callable[[], float] = time.monotonic, ) -> None: self._jwks_uri = jwks_uri self._fetcher = fetcher or default_jwks_fetcher self._cooldown = cooldown_seconds + self._max_age = max_age_seconds self._allow_private = allow_private self._clock = clock self._cache: dict[str, dict[str, Any]] = {} self._last_attempt: float | None = None + self._last_successful_refresh: float | None = None + self._last_failure: SignatureVerificationError | None = None self._primed = False + self._refresh_lock = threading.Lock() def __call__(self, keyid: str) -> dict[str, Any] | None: - if keyid in self._cache: - return self._cache[keyid] now = self._clock() - if not self._primed or ( + cache_expired = ( + self._primed + and self._last_successful_refresh is not None + and now - self._last_successful_refresh >= self._max_age + ) + miss_can_refresh = keyid not in self._cache and ( self._last_attempt is not None and now - self._last_attempt >= self._cooldown - ): - self._refresh(now) + ) + if not self._primed or cache_expired or miss_can_refresh: + with self._refresh_lock: + # Re-check after waiting: another thread may have refreshed a + # cold/expired cache or populated this kid while we blocked. + now = self._clock() + cache_expired = ( + self._primed + and self._last_successful_refresh is not None + and now - self._last_successful_refresh >= self._max_age + ) + miss_can_refresh = keyid not in self._cache and ( + self._last_attempt is not None and now - self._last_attempt >= self._cooldown + ) + if ( + cache_expired + and self._last_attempt is not None + and (now - self._last_attempt < self._cooldown) + ): + raise SignatureVerificationError( + REQUEST_SIGNATURE_JWKS_UNAVAILABLE, + step=7, + message="cached JWKS is expired and refresh cooldown has not elapsed", + ) + if not self._primed or cache_expired or miss_can_refresh: + self._refresh(now) return self._cache.get(keyid) def _refresh(self, now: float) -> None: self._last_attempt = now try: jwks = self._fetcher(self._jwks_uri, allow_private=self._allow_private) + cache = _index_jwks_keys(jwks, uri=self._jwks_uri) except SSRFValidationError as exc: - raise SignatureVerificationError( + error = SignatureVerificationError( REQUEST_SIGNATURE_JWKS_UNTRUSTED, step=7, message=f"JWKS URI failed SSRF check: {exc}", - ) from exc + ) + self._last_failure = error + raise error from exc except (httpx.HTTPError, ValueError, OSError) as exc: - raise SignatureVerificationError( + error = SignatureVerificationError( REQUEST_SIGNATURE_JWKS_UNAVAILABLE, step=7, message=f"JWKS fetch failed: {exc}", - ) from exc + ) + self._last_failure = error + raise error from exc self._primed = True - self._cache = {jwk["kid"]: jwk for jwk in jwks.get("keys", []) if "kid" in jwk} + self._cache = cache + self._last_successful_refresh = now + self._last_failure = None class StaticJwksResolver: @@ -446,7 +523,12 @@ def __call__(self, keyid: str) -> dict[str, Any] | None: # --------------------------------------------------------------------------- -async def async_default_jwks_fetcher(uri: str, *, allow_private: bool = False) -> dict[str, Any]: +async def async_default_jwks_fetcher( + uri: str, + *, + allow_private: bool = False, + max_body_bytes: int = DEFAULT_MAX_JWKS_BYTES, +) -> dict[str, Any]: """Async counterpart to :func:`default_jwks_fetcher`. Uses :class:`httpx.AsyncClient` with an IP-pinned transport so @@ -464,9 +546,11 @@ async def async_default_jwks_fetcher(uri: str, *, allow_private: bool = False) - follow_redirects=False, trust_env=False, ) as client: - response = await client.get(uri, headers={"Accept": "application/json"}) - response.raise_for_status() - body = response.json() + async with client.stream( + "GET", uri, headers={"Accept": "application/json", "Accept-Encoding": "identity"} + ) as response: + response.raise_for_status() + body = json.loads(await async_read_limited_bytes(response, limit=max_body_bytes)) if not isinstance(body, dict) or "keys" not in body: raise ValueError(f"JWKS document at {uri!r} has no 'keys' array") return body @@ -492,16 +576,20 @@ def __init__( *, fetcher: AsyncJwksFetcher | None = None, cooldown_seconds: float = DEFAULT_JWKS_COOLDOWN_SECONDS, + max_age_seconds: float = DEFAULT_JWKS_MAX_AGE_SECONDS, allow_private: bool = False, clock: Callable[[], float] = time.monotonic, ) -> None: self._jwks_uri = jwks_uri self._fetcher = fetcher or async_default_jwks_fetcher self._cooldown = cooldown_seconds + self._max_age = max_age_seconds self._allow_private = allow_private self._clock = clock self._cache: dict[str, dict[str, Any]] = {} self._last_attempt: float | None = None + self._last_successful_refresh: float | None = None + self._last_failure: SignatureVerificationError | None = None self._primed = False # Construct the lock eagerly. Lazy init was racy: two tasks both # seeing ``self._lock is None`` would each construct a separate @@ -513,20 +601,38 @@ def __init__( self._lock: asyncio.Lock = asyncio.Lock() async def __call__(self, keyid: str) -> dict[str, Any] | None: - if keyid in self._cache: - return self._cache[keyid] now = self._clock() - if not self._primed or ( + cache_expired = ( + self._primed + and self._last_successful_refresh is not None + and now - self._last_successful_refresh >= self._max_age + ) + miss_can_refresh = keyid not in self._cache and ( self._last_attempt is not None and now - self._last_attempt >= self._cooldown - ): + ) + if not self._primed or cache_expired or miss_can_refresh: async with self._lock: # Re-check after acquiring: another task may have refreshed. - if keyid in self._cache: - return self._cache[keyid] now = self._clock() - if not self._primed or ( + cache_expired = ( + self._primed + and self._last_successful_refresh is not None + and now - self._last_successful_refresh >= self._max_age + ) + miss_can_refresh = keyid not in self._cache and ( self._last_attempt is not None and now - self._last_attempt >= self._cooldown + ) + if ( + cache_expired + and self._last_attempt is not None + and (now - self._last_attempt < self._cooldown) ): + raise SignatureVerificationError( + REQUEST_SIGNATURE_JWKS_UNAVAILABLE, + step=7, + message="cached JWKS is expired and refresh cooldown has not elapsed", + ) + if not self._primed or cache_expired or miss_can_refresh: await self._refresh(now) return self._cache.get(keyid) @@ -534,20 +640,27 @@ async def _refresh(self, now: float) -> None: self._last_attempt = now try: jwks = await self._fetcher(self._jwks_uri, allow_private=self._allow_private) + cache = _index_jwks_keys(jwks, uri=self._jwks_uri) except SSRFValidationError as exc: - raise SignatureVerificationError( + error = SignatureVerificationError( REQUEST_SIGNATURE_JWKS_UNTRUSTED, step=7, message=f"JWKS URI failed SSRF check: {exc}", - ) from exc + ) + self._last_failure = error + raise error from exc except (httpx.HTTPError, ValueError, OSError) as exc: - raise SignatureVerificationError( + error = SignatureVerificationError( REQUEST_SIGNATURE_JWKS_UNAVAILABLE, step=7, message=f"JWKS fetch failed: {exc}", - ) from exc + ) + self._last_failure = error + raise error from exc self._primed = True - self._cache = {jwk["kid"]: jwk for jwk in jwks.get("keys", []) if "kid" in jwk} + self._cache = cache + self._last_successful_refresh = now + self._last_failure = None def as_async_resolver(resolver: JwksResolver) -> AsyncJwksResolver: @@ -573,7 +686,9 @@ async def resolve(keyid: str) -> dict[str, Any] | None: "CachingJwksResolver", "DEFAULT_ALLOWED_PORTS", "DEFAULT_JWKS_COOLDOWN_SECONDS", + "DEFAULT_JWKS_MAX_AGE_SECONDS", "DEFAULT_JWKS_TIMEOUT_SECONDS", + "DEFAULT_MAX_JWKS_BYTES", "JwksFetcher", "JwksResolver", "SSRFValidationError", diff --git a/src/adcp/signing/pg/replay_store.py b/src/adcp/signing/pg/replay_store.py index f8b46077d..de2989eda 100644 --- a/src/adcp/signing/pg/replay_store.py +++ b/src/adcp/signing/pg/replay_store.py @@ -79,6 +79,8 @@ async def sweep_forever(store: PgReplayStore, interval: float = 60.0) -> None: import re from typing import TYPE_CHECKING +from adcp.signing.replay import ReplayClaimResult + if TYPE_CHECKING: from psycopg_pool import ConnectionPool @@ -115,9 +117,8 @@ class PgReplayStore: ---------- pool: A :class:`psycopg_pool.ConnectionPool` owned by the caller. Each - operation acquires a short-lived connection, runs a single - statement, and returns the connection. No long-lived - transactions, no cross-operation state. + operation acquires a short-lived connection and returns it promptly. + ``claim`` runs its capacity check and insert in one short transaction. per_keyid_cap: Maximum number of live (non-expired) nonces per ``keyid``. Mirrors :class:`InMemoryReplayStore`; spec-recommended 1M. @@ -151,7 +152,7 @@ def __init__( raise ImportError(_INSTALL_HINT) if not _is_safe_identifier(table_name): raise ValueError( - f"table_name must match [a-z_][a-z0-9_]* (ASCII only), " f"got {table_name!r}" + f"table_name must match [a-z_][a-z0-9_]* (ASCII only), got {table_name!r}" ) self._pool = pool self._per_keyid_cap = per_keyid_cap @@ -180,6 +181,14 @@ def __init__( f"WHERE keyid = %s AND expires_at > now()" ) self._sql_sweep = f"DELETE FROM {self._table} WHERE expires_at <= now()" # noqa: S608 + self._sql_claim_lock = "SELECT pg_advisory_xact_lock(hashtextextended(%s, 9173))" + self._sql_claim = ( + f"INSERT INTO {self._table} (keyid, nonce, expires_at) " # noqa: S608 + f"VALUES (%s, %s, now() + make_interval(secs => %s)) " + f"ON CONFLICT (keyid, nonce) DO UPDATE " + f"SET expires_at = EXCLUDED.expires_at " + f"WHERE {self._table}.expires_at <= now() RETURNING 1" + ) # -- schema bootstrap -------------------------------------------- @@ -218,7 +227,7 @@ def seen(self, keyid: str, nonce: str) -> bool: cur.execute(self._sql_seen, (keyid, nonce)) return cur.fetchone() is not None - def remember(self, keyid: str, nonce: str, ttl_seconds: float) -> None: + def remember(self, keyid: str, nonce: str, ttl_seconds: float) -> bool: """Record ``(keyid, nonce)`` with a TTL. ``ON CONFLICT ... DO UPDATE`` refreshes the expiry on a @@ -227,6 +236,7 @@ def remember(self, keyid: str, nonce: str, ttl_seconds: float) -> None: """ with self._pool.connection() as conn, conn.cursor() as cur: cur.execute(self._sql_remember, (keyid, nonce, ttl_seconds)) + return True def at_capacity(self, keyid: str) -> bool: """Return True iff the live row count for ``keyid`` meets the cap. @@ -252,6 +262,25 @@ def at_capacity(self, keyid: str) -> bool: row = cur.fetchone() return bool(row[0]) if row is not None else False + def claim(self, keyid: str, nonce: str, ttl_seconds: float) -> ReplayClaimResult: + """Atomically enforce the per-key cap and reserve a fresh nonce. + + A transaction-scoped advisory lock serializes claims for one key id + across all verifier processes. The primary key then provides the + exact nonce winner selection. + """ + with self._pool.connection() as conn, conn.transaction(), conn.cursor() as cur: + cur.execute(self._sql_claim_lock, (keyid,)) + cur.execute(self._sql_seen, (keyid, nonce)) + if cur.fetchone() is not None: + return "replayed" + cur.execute(self._sql_at_capacity, (self._per_keyid_cap, keyid)) + row = cur.fetchone() + if row is not None and bool(row[0]): + return "capacity" + cur.execute(self._sql_claim, (keyid, nonce, ttl_seconds)) + return "claimed" if cur.fetchone() is not None else "replayed" + # -- admin / cron ------------------------------------------------ def sweep_expired(self) -> int: diff --git a/src/adcp/signing/replay.py b/src/adcp/signing/replay.py index 2b2758557..185fb4547 100644 --- a/src/adcp/signing/replay.py +++ b/src/adcp/signing/replay.py @@ -1,8 +1,8 @@ """Replay dedup store for the AdCP request-signing profile. Stores `(keyid, nonce)` pairs that have already been accepted, with a TTL that -mirrors the signature's `expires` parameter plus skew. A per-keyid cap prevents -unbounded growth — when the cap is hit, new signatures for that keyid are +mirrors the signature's `expires` parameter plus skew. Per-keyid and global +caps prevent unbounded growth — when either cap is hit, new signatures are rejected with `request_signature_rate_abuse` rather than silently evicting older entries (which would create a replay window under attack). @@ -15,17 +15,50 @@ import threading import time from collections.abc import Callable -from typing import Protocol +from typing import Literal, Protocol, TypeGuard, runtime_checkable + +ReplayClaimResult = Literal["claimed", "replayed", "capacity"] class ReplayStore(Protocol): - """Minimum interface a replay backend must expose.""" + """Legacy-compatible interface a replay backend must expose. + + New backends should additionally implement :class:`AtomicReplayStore`. + Keeping the atomic operation on a separate Protocol lets applications + written against the pre-6.6 replay API continue to type-check while the + verifier provides a visible, race-prone compatibility fallback. + """ - def seen(self, keyid: str, nonce: str) -> bool: ... + def seen(self, keyid: str, nonce: str) -> bool: + raise NotImplementedError + + def remember(self, keyid: str, nonce: str, ttl_seconds: float) -> bool | None: + raise NotImplementedError - def remember(self, keyid: str, nonce: str, ttl_seconds: float) -> None: ... + def at_capacity(self, keyid: str) -> bool: + raise NotImplementedError - def at_capacity(self, keyid: str) -> bool: ... + +@runtime_checkable +class AtomicReplayStore(ReplayStore, Protocol): + """Replay backend that can reserve a nonce without a check/write race.""" + + def claim(self, keyid: str, nonce: str, ttl_seconds: float) -> ReplayClaimResult: + """Atomically reserve a nonce, or report why it cannot be reserved.""" + raise NotImplementedError + + +def supports_atomic_claim(store: ReplayStore) -> TypeGuard[AtomicReplayStore]: + """Return whether ``store`` exposes a trustworthy atomic claim operation. + + Delegating wrappers can declare their resolved backend's capability through + ``supports_atomic_claim()``. Bare stores are checked structurally against + the runtime-checkable :class:`AtomicReplayStore` contract. + """ + declared = getattr(store, "supports_atomic_claim", None) + if callable(declared): + return bool(declared()) + return isinstance(store, AtomicReplayStore) # Cap on the number of expired entries swept per mutating call. Bounded so that @@ -37,17 +70,29 @@ def at_capacity(self, keyid: str) -> bool: ... class InMemoryReplayStore: """Process-local replay store. Uses a monotonic clock for TTL bookkeeping so wall-clock jumps (NTP adjustments, VM suspend/resume) don't race eviction. + + ``global_cap`` bounds attacker-controlled key rotation as well as nonce + volume. An indexed min-heap expires entries incrementally without copying + or scanning the nonce table on accepted requests. """ def __init__( self, *, per_keyid_cap: int = 1_000_000, + global_cap: int = 1_000_000, clock: Callable[[], float] = time.monotonic, ) -> None: + if per_keyid_cap <= 0: + raise ValueError("per_keyid_cap must be greater than zero") + if global_cap <= 0: + raise ValueError("global_cap must be greater than zero") self._per_keyid_cap = per_keyid_cap + self._global_cap = global_cap self._clock = clock self._entries: dict[tuple[str, str], float] = {} + self._expiry_heap: list[tuple[float, tuple[str, str]]] = [] + self._heap_positions: dict[tuple[str, str], int] = {} self._counts: dict[str, int] = {} self._cap_hit: set[str] = set() self._lock = threading.RLock() @@ -57,19 +102,54 @@ def seen(self, keyid: str, nonce: str) -> bool: self._expire_one(keyid, nonce) return (keyid, nonce) in self._entries - def remember(self, keyid: str, nonce: str, ttl_seconds: float) -> None: + def remember(self, keyid: str, nonce: str, ttl_seconds: float) -> bool: + """Record a nonce, returning ``False`` when capacity refuses it.""" with self._lock: - self._sweep_for_keyid(keyid) + now = self._clock() + self._purge_expired(now) key = (keyid, nonce) if key not in self._entries: + if ( + len(self._entries) >= self._global_cap + or self._counts.get(keyid, 0) >= self._per_keyid_cap + ): + return False self._counts[keyid] = self._counts.get(keyid, 0) + 1 - self._entries[key] = self._clock() + ttl_seconds + expiry = now + ttl_seconds + self._entries[key] = expiry + self._push_expiry(key, expiry) + return True def at_capacity(self, keyid: str) -> bool: with self._lock: + self._purge_expired(self._clock()) if keyid in self._cap_hit: return True - return self._counts.get(keyid, 0) >= self._per_keyid_cap + return ( + len(self._entries) >= self._global_cap + or self._counts.get(keyid, 0) >= self._per_keyid_cap + ) + + def claim(self, keyid: str, nonce: str, ttl_seconds: float) -> ReplayClaimResult: + """Atomically check capacity/replay state and reserve ``nonce``.""" + with self._lock: + now = self._clock() + self._expire_one(keyid, nonce) + if (keyid, nonce) in self._entries: + return "replayed" + self._purge_expired(now) + if ( + keyid in self._cap_hit + or len(self._entries) >= self._global_cap + or self._counts.get(keyid, 0) >= self._per_keyid_cap + ): + return "capacity" + key = (keyid, nonce) + expiry = now + ttl_seconds + self._entries[key] = expiry + self._counts[keyid] = self._counts.get(keyid, 0) + 1 + self._push_expiry(key, expiry) + return "claimed" def mark_cap_hit(self, keyid: str) -> None: """Test-harness hook — simulate the cap being reached for this keyid.""" @@ -81,22 +161,80 @@ def _expire_one(self, keyid: str, nonce: str) -> None: expiry = self._entries.get(key) if expiry is not None and expiry < self._clock(): del self._entries[key] + self._remove_expiry(key) self._counts[keyid] = self._counts.get(keyid, 1) - 1 if self._counts[keyid] <= 0: self._counts.pop(keyid, None) - def _sweep_for_keyid(self, keyid: str) -> None: - now = self._clock() - removed = 0 - # Scan only entries for this keyid to bound per-call work under load. - for key, expiry in list(self._entries.items()): - if key[0] != keyid: - continue - if expiry < now: - del self._entries[key] - self._counts[keyid] = self._counts.get(keyid, 1) - 1 - if self._counts[keyid] <= 0: - self._counts.pop(keyid, None) - removed += 1 - if removed >= _SWEEP_BATCH: - return + def _push_expiry(self, key: tuple[str, str], expiry: float) -> None: + position = self._heap_positions.get(key) + if position is None: + position = len(self._expiry_heap) + self._expiry_heap.append((expiry, key)) + self._heap_positions[key] = position + self._sift_up(position) + return + old_expiry = self._expiry_heap[position][0] + self._expiry_heap[position] = (expiry, key) + if expiry < old_expiry: + self._sift_up(position) + else: + self._sift_down(position) + + def _remove_expiry(self, key: tuple[str, str]) -> None: + position = self._heap_positions.pop(key, None) + if position is None: + return + last = self._expiry_heap.pop() + if position == len(self._expiry_heap): + return + self._expiry_heap[position] = last + self._heap_positions[last[1]] = position + if position > 0 and self._expiry_heap[position] < self._expiry_heap[(position - 1) // 2]: + self._sift_up(position) + else: + self._sift_down(position) + + def _sift_up(self, position: int) -> None: + while position > 0: + parent = (position - 1) // 2 + if self._expiry_heap[parent] <= self._expiry_heap[position]: + return + self._swap_heap(parent, position) + position = parent + + def _sift_down(self, position: int) -> None: + size = len(self._expiry_heap) + while (left := position * 2 + 1) < size: + right = left + 1 + child = ( + right + if right < size and self._expiry_heap[right] < self._expiry_heap[left] + else left + ) + if self._expiry_heap[position] <= self._expiry_heap[child]: + return + self._swap_heap(position, child) + position = child + + def _swap_heap(self, left: int, right: int) -> None: + self._expiry_heap[left], self._expiry_heap[right] = ( + self._expiry_heap[right], + self._expiry_heap[left], + ) + self._heap_positions[self._expiry_heap[left][1]] = left + self._heap_positions[self._expiry_heap[right][1]] = right + + def _purge_expired(self, now: float) -> None: + examined = 0 + while self._expiry_heap and examined < _SWEEP_BATCH: + expiry, key = self._expiry_heap[0] + if expiry >= now: + return + self._remove_expiry(key) + examined += 1 + del self._entries[key] + keyid = key[0] + self._counts[keyid] = self._counts.get(keyid, 1) - 1 + if self._counts[keyid] <= 0: + self._counts.pop(keyid, None) diff --git a/src/adcp/signing/revocation_fetcher.py b/src/adcp/signing/revocation_fetcher.py index 1112ecd14..f66e8325c 100644 --- a/src/adcp/signing/revocation_fetcher.py +++ b/src/adcp/signing/revocation_fetcher.py @@ -38,12 +38,17 @@ import time from collections.abc import Callable from dataclasses import dataclass -from datetime import datetime, timedelta, timezone +from datetime import datetime, timezone from typing import Any, Protocol import httpx import idna +from adcp.signing._bounded_http import ( + ResponseTooLargeError, + async_read_limited_bytes, + read_limited_bytes, +) from adcp.signing._idna_canonicalize import canonicalize_host from adcp.signing.jwks import ( DEFAULT_JWKS_TIMEOUT_SECONDS, @@ -69,6 +74,7 @@ # within `next_update + grace * last_interval`, all subsequent is_revoked # calls fail closed. Spec recommends 2×. DEFAULT_GRACE_MULTIPLIER = 2.0 +DEFAULT_MAX_REVOCATION_LIST_BYTES = 1024 * 1024 # Shape-validate `Last-Modified` before persisting it for the next # `If-Modified-Since` request. The value comes from a (potentially @@ -246,6 +252,7 @@ def default_revocation_list_fetcher( if_modified_since: str | None = None, allow_private: bool = False, timeout: float = DEFAULT_JWKS_TIMEOUT_SECONDS, + max_body_bytes: int = DEFAULT_MAX_REVOCATION_LIST_BYTES, ) -> FetchResult: """HTTPS GET the revocation list, honoring SSRF rules and conditional requests. @@ -271,19 +278,27 @@ def default_revocation_list_fetcher( follow_redirects=False, trust_env=False, ) as client: - response = client.get(uri, headers=headers) - except httpx.HTTPError as exc: + with client.stream( + "GET", uri, headers={**headers, "Accept-Encoding": "identity"} + ) as response: + response_text = "" + if response.status_code == 200: + response_text = read_limited_bytes(response, limit=max_body_bytes).decode( + "utf-8" + ) + return _fetch_result_from_response( + uri, + response.status_code, + response_text, + response.headers, + if_none_match=if_none_match, + if_modified_since=if_modified_since, + ) + except ResponseTooLargeError as exc: + raise RevocationListFetchError(f"revocation list {uri!r} {exc}") from exc + except (httpx.HTTPError, UnicodeDecodeError) as exc: raise RevocationListFetchError(f"revocation list GET {uri!r} failed: {exc}") from exc - return _fetch_result_from_response( - uri, - response.status_code, - response.text, - response.headers, - if_none_match=if_none_match, - if_modified_since=if_modified_since, - ) - async def async_default_revocation_list_fetcher( uri: str, @@ -292,6 +307,7 @@ async def async_default_revocation_list_fetcher( if_modified_since: str | None = None, allow_private: bool = False, timeout: float = DEFAULT_JWKS_TIMEOUT_SECONDS, + max_body_bytes: int = DEFAULT_MAX_REVOCATION_LIST_BYTES, ) -> FetchResult: """Async counterpart to :func:`default_revocation_list_fetcher`. @@ -310,19 +326,27 @@ async def async_default_revocation_list_fetcher( follow_redirects=False, trust_env=False, ) as client: - response = await client.get(uri, headers=headers) - except httpx.HTTPError as exc: + async with client.stream( + "GET", uri, headers={**headers, "Accept-Encoding": "identity"} + ) as response: + response_text = "" + if response.status_code == 200: + response_text = ( + await async_read_limited_bytes(response, limit=max_body_bytes) + ).decode("utf-8") + return _fetch_result_from_response( + uri, + response.status_code, + response_text, + response.headers, + if_none_match=if_none_match, + if_modified_since=if_modified_since, + ) + except ResponseTooLargeError as exc: + raise RevocationListFetchError(f"revocation list {uri!r} {exc}") from exc + except (httpx.HTTPError, UnicodeDecodeError) as exc: raise RevocationListFetchError(f"revocation list GET {uri!r} failed: {exc}") from exc - return _fetch_result_from_response( - uri, - response.status_code, - response.text, - response.headers, - if_none_match=if_none_match, - if_modified_since=if_modified_since, - ) - def _sanitize_last_modified(raw: str | None) -> str | None: """Validate a ``Last-Modified`` header value before persisting it. @@ -396,23 +420,6 @@ def _normalize_issuer(issuer: str) -> str: return urlunsplit((scheme, netloc, "", "", "")) -def _slide_next_update(current: RevocationList, polling_interval_seconds: float) -> RevocationList: - """Return ``current`` with ``next_update`` advanced by one polling interval. - - Used on a 304 response so the cached list's freshness window slides - forward without needing a fresh JWS. Preserves every other field. - """ - prior = _parse_iso8601(current.next_update) - new_next_update = prior + timedelta(seconds=polling_interval_seconds) - return RevocationList( - issuer=current.issuer, - updated=current.updated, - next_update=new_next_update.isoformat().replace("+00:00", "Z"), - revoked_kids=current.revoked_kids, - revoked_jtis=current.revoked_jtis, - ) - - def _post_jws_validation( payload: dict[str, Any], *, @@ -438,15 +445,13 @@ def _post_jws_validation( delta = (updated - now_wall).total_seconds() if delta > 60: # 60s clock skew tolerance, mirrors JWS exp/iat rules raise RevocationListParseError( - f"revocation list updated={revocation_list.updated!r} is " - f"{delta:.0f}s in the future" + f"revocation list updated={revocation_list.updated!r} is {delta:.0f}s in the future" ) if next_update <= updated: raise RevocationListParseError( f"revocation list next_update {revocation_list.next_update!r} is not " f"after updated {revocation_list.updated!r}" ) - # Reject a freshly-fetched list whose `updated` is older than the # one we already have cached. Defense against CDN replay or # compromised operator serving an older list with revocations @@ -502,19 +507,12 @@ def _init_state(self) -> None: self._last_refresh_attempt = None def _handle_not_modified(self, *, now_mono: float) -> None: - """Slide ``next_update`` forward on a 304 response. + """Record a successful conditional request without changing signed freshness. - Without this, subsequent calls past the original ``next_update`` - would re-enter the refresh branch on every verification (gated - only by the 60s cooldown). Advancing the cached - ``next_update`` by one polling interval lets the hot path - short-circuit cleanly. + A 304 authenticates no new JWS payload, so it cannot extend the + signed ``next_update`` authorization boundary. """ self._last_successful_refresh = now_mono - if self._current_list is not None and self._last_polling_interval_seconds: - self._current_list = _slide_next_update( - self._current_list, self._last_polling_interval_seconds - ) def _commit( self, @@ -794,12 +792,21 @@ def _ensure_fresh(self) -> None: else float("inf") ) if since_last_attempt >= MIN_POLLING_INTERVAL_SECONDS: + last_exc: Exception = RevocationListFetchError( + "another refresh completed without extending signed freshness" + ) try: - self._refresh(conditional=True, now_wall=now_wall, now_mono=now_mono) - return + installed_signed_payload = self._refresh( + conditional=True, now_wall=now_wall, now_mono=now_mono + ) + if installed_signed_payload: + return + last_exc = RevocationListFetchError( + "304 response did not extend signed next_update" + ) except (RevocationListFetchError, RevocationListParseError) as exc: # Fall through to the grace-window check below. - last_exc: Exception = exc + last_exc = exc else: last_exc = RevocationListFetchError( f"refresh cooldown not elapsed ({since_last_attempt:.0f}s < " @@ -815,7 +822,7 @@ def _ensure_fresh(self) -> None: ) from last_exc # Still within grace — serve the cached list. - def _refresh(self, *, conditional: bool, now_wall: datetime, now_mono: float) -> None: + def _refresh(self, *, conditional: bool, now_wall: datetime, now_mono: float) -> bool: self._last_refresh_attempt = now_mono if_none_match = self._current_etag if conditional else None if_modified_since = self._current_last_modified if conditional else None @@ -826,7 +833,7 @@ def _refresh(self, *, conditional: bool, now_wall: datetime, now_mono: float) -> ) if result.not_modified: self._handle_not_modified(now_mono=now_mono) - return + return False try: payload = verify_jws_document( @@ -846,6 +853,7 @@ def _refresh(self, *, conditional: bool, now_wall: datetime, now_mono: float) -> current_list=self._current_list, ) self._commit(result=result, revocation_list=revocation_list, now_mono=now_mono) + return True class AsyncCachingRevocationChecker(_CheckerState): @@ -981,6 +989,9 @@ async def _ensure_fresh(self) -> None: else float("inf") ) if since_last_attempt >= MIN_POLLING_INTERVAL_SECONDS: + last_exc: Exception = RevocationListFetchError( + "another refresh completed without extending signed freshness" + ) try: async with self._lock: # Re-check under the lock with fresh clock reads. @@ -989,14 +1000,18 @@ async def _ensure_fresh(self) -> None: now_mono_inside - self._last_refresh_attempt >= MIN_POLLING_INTERVAL_SECONDS ): now_wall_inside = self._wall_clock() - await self._refresh( + installed_signed_payload = await self._refresh( conditional=True, now_wall=now_wall_inside, now_mono=now_mono_inside, ) - return + if installed_signed_payload: + return + last_exc = RevocationListFetchError( + "304 response did not extend signed next_update" + ) except (RevocationListFetchError, RevocationListParseError) as exc: - last_exc: Exception = exc + last_exc = exc else: last_exc = RevocationListFetchError( f"refresh cooldown not elapsed ({since_last_attempt:.0f}s < " @@ -1011,7 +1026,7 @@ async def _ensure_fresh(self) -> None: f"last refresh error: {last_exc}" ) from last_exc - async def _refresh(self, *, conditional: bool, now_wall: datetime, now_mono: float) -> None: + async def _refresh(self, *, conditional: bool, now_wall: datetime, now_mono: float) -> bool: # Stamp the attempt BEFORE the awaitable. On CancelledError the # finally block rolls it back so a cancelled task doesn't burn # the 60s cooldown for the next caller — non-cancellation @@ -1036,7 +1051,7 @@ async def _refresh(self, *, conditional: bool, now_wall: datetime, now_mono: flo raise if result.not_modified: self._handle_not_modified(now_mono=now_mono) - return + return False try: payload = await averify_jws_document( @@ -1056,6 +1071,7 @@ async def _refresh(self, *, conditional: bool, now_wall: datetime, now_mono: flo current_list=self._current_list, ) self._commit(result=result, revocation_list=revocation_list, now_mono=now_mono) + return True __all__ = [ @@ -1063,6 +1079,7 @@ async def _refresh(self, *, conditional: bool, now_wall: datetime, now_mono: flo "AsyncRevocationListFetcher", "CachingRevocationChecker", "DEFAULT_GRACE_MULTIPLIER", + "DEFAULT_MAX_REVOCATION_LIST_BYTES", "FetchResult", "REVOCATION_LIST_TYP", "RevocationListFetchError", diff --git a/src/adcp/signing/verifier.py b/src/adcp/signing/verifier.py index 2acc6b5f8..db7d3a976 100644 --- a/src/adcp/signing/verifier.py +++ b/src/adcp/signing/verifier.py @@ -8,6 +8,7 @@ from __future__ import annotations +import threading import warnings from collections.abc import Mapping, Sequence from dataclasses import dataclass, field @@ -60,7 +61,12 @@ ) from adcp.signing.jwks import JwksResolver from adcp.signing.key_origins import check_key_origin_consistency -from adcp.signing.replay import InMemoryReplayStore, ReplayStore +from adcp.signing.replay import ( + InMemoryReplayStore, + ReplayClaimResult, + ReplayStore, + supports_atomic_claim, +) from adcp.signing.revocation import RevocationChecker, RevocationList CoversDigestPolicy = Literal["required", "forbidden", "either"] @@ -74,6 +80,9 @@ # limit; 256 bytes is plenty for any real-world kid/nonce. _MAX_PARAM_LEN = 256 +_WARNED_LEGACY_REPLAY_STORE_TYPES: set[type[object]] = set() +_WARNED_LEGACY_REPLAY_STORE_TYPES_LOCK = threading.Lock() + @dataclass(frozen=True) class VerifiedSigner: @@ -90,18 +99,8 @@ class VerifiedSigner: class VerifierCapability: """The `request_signing` block a verifier advertises on get_adcp_capabilities. - Defaults to ``covers_content_digest="either"`` per the AdCP 3.0 schema - (`get-adcp-capabilities-response.json` declares this as the default - explicitly). The schema rationale recommends `"required"` for - spend-committing operations in production, and AdCP 4.0 recommends - `"required"` more broadly. - - Operators who want body-integrity authentication end-to-end on - every request — closing the MITM-inside-TLS-termination case where - a reverse proxy or service mesh can swap bodies on unsigned-digest - requests — opt INTO ``covers_content_digest="required"`` explicitly, - or use ``required_for=frozenset({"create_media_buy", ...})`` to - promote spend-committing operations selectively. + Defaults to the AdCP wire-schema value ``covers_content_digest="either"``. + Receivers that require signed body digests must opt into ``"required"``. The webhook-signing profile (``adcp.signing.webhook_verifier``) hard- codes ``"required"`` regardless of this default — webhook bodies @@ -118,15 +117,14 @@ class VerifierCapability: class VerifyOptions: """Options bag passed to ``verify_request_signature``. - ``replay_store`` defaults to a fresh :class:`InMemoryReplayStore` so the - verifier always enforces nonce uniqueness on every request — defaulting - to ``None`` would silently disable replay protection for callers who - forget to wire a store, the exact security regression the AdCP profile's - step 12 exists to prevent. Wire an explicit shared store (Redis, Postgres, - etc.) for multi-replica deployments where replay state must be - coordinated across processes; pass ``replay_store=None`` if you genuinely - need to bypass the check (uncommon — typically only short-lived - integration tests). + ``replay_store`` defaults to a fresh :class:`InMemoryReplayStore` so one + options instance enforces nonce uniqueness across every verification that + reuses it. Constructing a new :class:`VerifyOptions` per request also + creates a new store and therefore resets replay history; long-lived + verifiers must reuse the options instance or provide an explicit store. + Wire a shared store (Redis, Postgres, etc.) for multi-replica deployments + where replay state must be coordinated across processes; pass + ``replay_store=None`` only when you genuinely need to bypass the check. ``revocation_checker`` and ``revocation_list`` remain optional — most agents don't track key revocations at runtime, and the verifier @@ -146,6 +144,10 @@ class VerifyOptions: label: str = SIG_LABEL_DEFAULT expected_tag: str = DEFAULT_TAG expected_adcp_use: str = ADCP_USE_REQUEST + #: Optional compatibility accept-set for profiles whose verifier must + #: recognize more than one JWK purpose. Empty preserves the historical + #: single-value ``expected_adcp_use`` behavior. + accepted_adcp_uses: frozenset[str] = frozenset() allowed_algs: frozenset[str] = ALLOWED_ALGS agent_url: str | None = None #: ADCP #3690 step 7 — the signing peer's declared @@ -154,9 +156,9 @@ class VerifyOptions: #: (``request_signing``, ``webhook_signing``, ...). When provided #: AND the JWKS resolver reports ``jwks_source == "brand_json"``, #: the verifier checks that the resolved ``jwks_uri`` host - #: matches the declared origin for ``signing_purpose``. ``None`` - #: (default) skips the check — adopters who don't yet plumb - #: capabilities through to the verifier see no behavior change. + #: matches the declared origin for ``signing_purpose``. For a + #: brand-sourced resolver, ``None`` is treated as a missing declaration + #: and fails closed; other resolver sources skip the check. expected_key_origins: Mapping[str, str] | None = None #: Purpose key used to look up ``expected_key_origins`` and to #: render error messages. Default ``"request_signing"`` matches @@ -287,7 +289,12 @@ def verify_request_signature( ) alg = str(parsed.params["alg"]) - _check_key_purpose(jwk, alg, expected_adcp_use=options.expected_adcp_use) + _check_key_purpose( + jwk, + alg, + expected_adcp_use=options.expected_adcp_use, + accepted_adcp_uses=options.accepted_adcp_uses, + ) # ADCP #3690 step 7: ``identity.key_origins`` consistency check. # Mandatory ONLY when the JWKS source for this (agent, purpose, @@ -323,9 +330,8 @@ def verify_request_signature( message=f"key {keyid!r} is revoked", ) - # Step 9a (per spec, after adcp#2342): per-keyid cap runs between JWKS - # resolution and crypto verify. A compromised or misconfigured signer - # hitting the cap must be rejected cheaply, not after Ed25519/ECDSA verify. + # Cheap early rejection; ``claim`` repeats the capacity check atomically + # after crypto verification so concurrent claims cannot exceed the cap. if options.replay_store is not None and options.replay_store.at_capacity(keyid): raise SignatureVerificationError( REQUEST_SIGNATURE_RATE_ABUSE, @@ -381,17 +387,25 @@ def verify_request_signature( ) if options.replay_store is not None: - if options.replay_store.seen(keyid, nonce): + ttl = max( + float(parsed.params["expires"]) - options.now + options.max_skew_seconds, + 0.0, + ) + claim = _claim_replay_nonce(options.replay_store, keyid, nonce, ttl) + if claim == "replayed": raise SignatureVerificationError( REQUEST_SIGNATURE_REPLAYED, step=12, message=f"nonce {nonce!r} already seen for keyid {keyid!r}", ) - ttl = max( - float(parsed.params["expires"]) - options.now + options.max_skew_seconds, - 0.0, - ) - options.replay_store.remember(keyid, nonce, ttl) + if claim == "capacity": + raise SignatureVerificationError( + REQUEST_SIGNATURE_RATE_ABUSE, + # security.mdx:1324 @ AdCP 3.1.8: the atomic insert is + # authoritative replay-cap enforcement at step 13. + step=13, + message=f"replay cache at capacity for keyid {keyid!r}", + ) return VerifiedSigner( key_id=keyid, @@ -402,6 +416,55 @@ def verify_request_signature( ) +def _claim_replay_nonce( + store: ReplayStore, + keyid: str, + nonce: str, + ttl_seconds: float, +) -> ReplayClaimResult: + """Reserve a nonce, retaining compatibility with pre-atomic stores. + + The legacy ``seen`` then ``remember`` sequence is necessarily racy. It is + retained only so upgrading the SDK does not turn a previously valid custom + backend into an ``AttributeError`` after signature verification. + """ + if supports_atomic_claim(store): + result = store.claim(keyid, nonce, ttl_seconds) + if result not in {"claimed", "replayed", "capacity"}: + raise SignatureVerificationError( + REQUEST_SIGNATURE_RATE_ABUSE, + step=13, + message="replay store returned an invalid claim result", + ) + return result + + store_type = type(store) + with _WARNED_LEGACY_REPLAY_STORE_TYPES_LOCK: + should_warn = store_type not in _WARNED_LEGACY_REPLAY_STORE_TYPES + _WARNED_LEGACY_REPLAY_STORE_TYPES.add(store_type) + if should_warn: + warnings.warn( + "ReplayStore does not implement atomic claim(); falling back to the " + "legacy seen()/remember() sequence, which cannot prevent concurrent " + "replays. Implement claim() before this compatibility path is removed.", + DeprecationWarning, + stacklevel=2, + ) + if store.seen(keyid, nonce): + return "replayed" + if store.at_capacity(keyid): + return "capacity" + retained = store.remember(keyid, nonce, ttl_seconds) + if retained is False: + return "capacity" + # Legacy implementations cannot report a refused write. Verify that the + # nonce was retained so a silent cap drop fails closed rather than opening + # a replay window. + if not store.seen(keyid, nonce): + return "capacity" + return "claimed" + + def _precheck_presence( *, sig_input_raw: str | None, @@ -566,15 +629,12 @@ def _maybe_check_key_origin( we fail closed via the mismatch path (``actual_origin`` becomes ``None``). - Skip + warn cases (both fire :func:`warnings.warn` so the - one-time message in the operator's log surfaces the misconfig): + Missing declarations fail closed: - * ``jwks_source == "brand_json"`` + ``expected_key_origins is None``: - the resolver IS brand-json-sourced but the caller didn't surface - the operator's declared ``identity.key_origins`` map, so the - spec-mandated check silently no-ops. ``UserWarning`` — the - adopter needs to thread ``expected_key_origins`` through - ``VerifyOptions``. + * ``expected_key_origins is None`` means no capabilities declaration was + supplied and retains the schema's shared-origin compatibility posture. + An explicit empty map means capabilities were observed without the + required purpose and fails closed. * ``expected_key_origins`` set + resolver has no ``jwks_source``: adopter upgraded the SDK but their custom resolver predates the discriminant. ``DeprecationWarning`` — set @@ -584,19 +644,6 @@ def _maybe_check_key_origin( """ source = getattr(resolver, "jwks_source", None) if expected_key_origins is None: - if source == "brand_json": - warnings.warn( - "Resolver advertises jwks_source='brand_json' but VerifyOptions " - "did not supply expected_key_origins — the spec-mandated " - "identity.key_origins consistency check (ADCP #3690 step 7) " - "is silently skipped. Thread the operator's " - "identity.key_origins map through VerifyOptions(expected_key_origins=...) " - "to engage the check; pass an empty dict if the operator " - "advertises no map and you want the missing-declaration " - "rejection (request_signature_key_origin_missing) to fire.", - UserWarning, - stacklevel=2, - ) return if source != "brand_json": if source is None: @@ -634,13 +681,19 @@ def _maybe_check_key_origin( ) check_key_origin_consistency( jwks_uri=jwks_uri, - key_origins=expected_key_origins, + key_origins=expected_key_origins or {}, purpose=signing_purpose, posture=posture, ) -def _check_key_purpose(jwk: Mapping[str, Any], alg: str, *, expected_adcp_use: str) -> None: +def _check_key_purpose( + jwk: Mapping[str, Any], + alg: str, + *, + expected_adcp_use: str, + accepted_adcp_uses: frozenset[str] = frozenset(), +) -> None: if jwk.get("use") != "sig": raise SignatureVerificationError( REQUEST_SIGNATURE_KEY_PURPOSE_INVALID, @@ -654,11 +707,14 @@ def _check_key_purpose(jwk: Mapping[str, Any], alg: str, *, expected_adcp_use: s step=8, message=f"JWK.key_ops {key_ops!r} missing 'verify'", ) - if jwk.get("adcp_use") != expected_adcp_use: + allowed_adcp_uses = accepted_adcp_uses or frozenset({expected_adcp_use}) + if jwk.get("adcp_use") not in allowed_adcp_uses: raise SignatureVerificationError( REQUEST_SIGNATURE_KEY_PURPOSE_INVALID, step=8, - message=f"JWK.adcp_use {jwk.get('adcp_use')!r} != {expected_adcp_use!r}", + message=( + f"JWK.adcp_use {jwk.get('adcp_use')!r} not in " f"{sorted(allowed_adcp_uses)!r}" + ), ) try: jwk_alg = alg_for_jwk(dict(jwk)) diff --git a/src/adcp/signing/webhook_verifier.py b/src/adcp/signing/webhook_verifier.py index 75fcc833a..be018f00d 100644 --- a/src/adcp/signing/webhook_verifier.py +++ b/src/adcp/signing/webhook_verifier.py @@ -5,8 +5,8 @@ * ``tag`` — ``adcp/webhook-signing/v1`` (distinct from request signing so a signature from one profile can never be replayed as the other). -* JWK ``adcp_use`` — ``webhook-signing`` (cross-purpose key reuse is locally - enforceable here). +* JWK ``adcp_use`` — ``request-signing`` for current senders, while the + deprecated ``webhook-signing`` value remains accepted for compatibility. * ``content-digest`` — REQUIRED. No ``covers_content_digest: "forbidden"`` escape hatch; webhooks are delivery of an *event*, and a signature that doesn't cover the body is not protecting the attack surface. @@ -22,10 +22,11 @@ import logging import time from collections.abc import Callable, Mapping -from dataclasses import dataclass +from dataclasses import dataclass, field from adcp.signing.canonical import _lookup, parse_signature_input_header from adcp.signing.constants import ( + ADCP_USE_REQUEST, ADCP_USE_WEBHOOK, DEFAULT_SKEW_SECONDS, MAX_WINDOW_SECONDS, @@ -42,7 +43,7 @@ logger = logging.getLogger(__name__) from adcp.signing.jwks import JwksResolver -from adcp.signing.replay import ReplayStore +from adcp.signing.replay import InMemoryReplayStore, ReplayStore from adcp.signing.revocation import RevocationChecker, RevocationList from adcp.signing.verifier import ( VerifiedSigner, @@ -72,10 +73,15 @@ class WebhookVerifyOptions: verifier stamps time-of-check itself, so the same :class:`WebhookVerifyOptions` instance can live for the lifetime of your receiver without a factory closure around it. Override via ``clock=`` for deterministic tests. + + ``replay_store`` defaults to a per-options in-memory store so captured + signatures are rejected without extra configuration. Multi-process + receivers should supply a shared store. Passing ``None`` explicitly is + the opt-out for specialized tests or externally enforced replay policy. """ jwks_resolver: JwksResolver - replay_store: ReplayStore | None = None + replay_store: ReplayStore | None = field(default_factory=InMemoryReplayStore) revocation_checker: RevocationChecker | None = None revocation_list: RevocationList | None = None max_skew_seconds: int = DEFAULT_SKEW_SECONDS @@ -83,6 +89,8 @@ class WebhookVerifyOptions: label: str = SIG_LABEL_DEFAULT allowed_algs: frozenset[str] = ALLOWED_ALGS sender_url: str | None = None + expected_key_origins: Mapping[str, str] | None = None + posture: str | None = None clock: Callable[[], float] = time.time @@ -149,9 +157,12 @@ def verify_webhook_signature( max_window_seconds=options.max_window_seconds, label=options.label, expected_tag=WEBHOOK_TAG, - expected_adcp_use=ADCP_USE_WEBHOOK, + accepted_adcp_uses=frozenset({ADCP_USE_REQUEST, ADCP_USE_WEBHOOK}), allowed_algs=options.allowed_algs, agent_url=options.sender_url, + expected_key_origins=options.expected_key_origins, + signing_purpose="webhook_signing", + posture=options.posture, ) try: diff --git a/src/adcp/types/_ergonomic.py b/src/adcp/types/_ergonomic.py index c0e8c391a..aa259a674 100644 --- a/src/adcp/types/_ergonomic.py +++ b/src/adcp/types/_ergonomic.py @@ -36,12 +36,13 @@ from collections.abc import Sequence from typing import Annotated, Any -from pydantic import BeforeValidator +from pydantic import BeforeValidator, WrapValidator from adcp.types.coercion import ( coerce_subclass_list, coerce_to_enum, coerce_to_enum_list, + coerce_to_unique_enum_list, coerce_to_model, ) @@ -76,6 +77,9 @@ from adcp.types.generated_poc.media_buy.list_creative_formats_request import ( ListCreativeFormatsRequest, ) +from adcp.types.generated_poc.creative.list_creative_formats_request import ( + ListCreativeFormatsRequestCreativeAgent, +) from adcp.types.generated_poc.creative.list_creatives_request import ( Field1 as ListCreativesField, ListCreativesRequest, @@ -105,6 +109,7 @@ from adcp.types.generated_poc.media_buy.get_products_request import BuyingMode from adcp.types.generated_poc.media_buy.get_products_response import CacheScope from adcp.types.generated_poc.media_buy.list_creative_formats_response import Source +from adcp.types.generated_poc.creative.list_creative_formats_request import Type def _apply_coercion() -> None: @@ -133,21 +138,15 @@ def _apply_coercion() -> None: "wcag_level", Annotated[WcagLevel | None, BeforeValidator(coerce_to_enum(WcagLevel))], ) - _patch_field_annotation( + _patch_unique_enum_list( ListCreativeFormatsRequest, "disclosure_positions", - Annotated[ - list[DisclosurePosition] | None, - BeforeValidator(coerce_to_enum_list(DisclosurePosition)), - ], + DisclosurePosition, ) - _patch_field_annotation( + _patch_unique_enum_list( ListCreativeFormatsRequest, "disclosure_persistence", - Annotated[ - list[DisclosurePersistence] | None, - BeforeValidator(coerce_to_enum_list(DisclosurePersistence)), - ], + DisclosurePersistence, ) _patch_field_annotation( ListCreativeFormatsRequest, @@ -161,6 +160,54 @@ def _apply_coercion() -> None: ) ListCreativeFormatsRequest.model_rebuild(force=True) + # Apply coercion to ListCreativeFormatsRequestCreativeAgent + # - type: Type | str | None + # - asset_types: list[AssetContentType | str] | None + # - wcag_level: WcagLevel | str | None + # - disclosure_positions: list[DisclosurePosition | str] | None + # - disclosure_persistence: list[DisclosurePersistence | str] | None + # - context: ContextObject | dict | None + # - ext: ExtensionObject | dict | None + _patch_field_annotation( + ListCreativeFormatsRequestCreativeAgent, + "type", + Annotated[Type | None, BeforeValidator(coerce_to_enum(Type))], + ) + _patch_field_annotation( + ListCreativeFormatsRequestCreativeAgent, + "asset_types", + Annotated[ + list[AssetContentType] | None, + BeforeValidator(coerce_to_enum_list(AssetContentType)), + ], + ) + _patch_field_annotation( + ListCreativeFormatsRequestCreativeAgent, + "wcag_level", + Annotated[WcagLevel | None, BeforeValidator(coerce_to_enum(WcagLevel))], + ) + _patch_unique_enum_list( + ListCreativeFormatsRequestCreativeAgent, + "disclosure_positions", + DisclosurePosition, + ) + _patch_unique_enum_list( + ListCreativeFormatsRequestCreativeAgent, + "disclosure_persistence", + DisclosurePersistence, + ) + _patch_field_annotation( + ListCreativeFormatsRequestCreativeAgent, + "context", + Annotated[ContextObject | None, BeforeValidator(coerce_to_model(ContextObject))], + ) + _patch_field_annotation( + ListCreativeFormatsRequestCreativeAgent, + "ext", + Annotated[ExtensionObject | None, BeforeValidator(coerce_to_model(ExtensionObject))], + ) + ListCreativeFormatsRequestCreativeAgent.model_rebuild(force=True) + # Apply coercion to ListCreativesRequest # - fields: list[ListCreativesField | str] | None # - context: ContextObject | dict | None @@ -606,6 +653,17 @@ def _apply_coercion() -> None: GetMediaBuyDeliveryResponse.model_rebuild(force=True) +def _patch_unique_enum_list( + model: Any, + field_name: str, + enum_class: type, +) -> None: + """Add an order-preserving unique-items enum validator.""" + model.model_fields[field_name].metadata.append( + WrapValidator(coerce_to_unique_enum_list(enum_class)) + ) + + def _patch_field_annotation( model: type, field_name: str, diff --git a/src/adcp/types/coercion.py b/src/adcp/types/coercion.py index cc756a923..97eac35b1 100644 --- a/src/adcp/types/coercion.py +++ b/src/adcp/types/coercion.py @@ -17,7 +17,7 @@ from collections.abc import Callable from enum import Enum -from typing import TYPE_CHECKING, Any, TypeVar +from typing import TYPE_CHECKING, Any, TypeVar, cast if TYPE_CHECKING: from pydantic import BaseModel @@ -99,6 +99,30 @@ def validator(value: Any) -> list[T] | None: return validator +def coerce_to_unique_enum_list( + enum_class: type[T], +) -> Callable[[Any, Callable[[Any], Any]], list[T] | None]: + """Create a wrap validator that coerces and deduplicates enum lists.""" + enum_list_validator = coerce_to_enum_list(enum_class) + + def validator(value: Any, handler: Callable[[Any], Any]) -> list[T] | None: + # Bypass collection constraints for the optional branch. Pydantic's + # rebuilt FieldInfo otherwise applies min_length to None. + if value is None: + return None + coerced = enum_list_validator(value) + if isinstance(coerced, list): + try: + coerced = list(dict.fromkeys(coerced)) + except TypeError: + # Preserve Pydantic's normal validation error for unhashable, + # invalid members instead of leaking a validator TypeError. + pass + return cast("list[T] | None", handler(coerced)) + + return validator + + def coerce_to_model(model_class: type[M]) -> Callable[[Any], M | None]: """Create a validator that coerces dicts to Pydantic model instances. diff --git a/tests/conformance/decisioning/test_pg_idempotency_backend.py b/tests/conformance/decisioning/test_pg_idempotency_backend.py index d66196837..b5ad1e9e0 100644 --- a/tests/conformance/decisioning/test_pg_idempotency_backend.py +++ b/tests/conformance/decisioning/test_pg_idempotency_backend.py @@ -42,8 +42,11 @@ async def isolated_backend() -> AsyncIterator[PgBackend]: """Fresh async pool + isolated table per test. Drops on teardown.""" table = f"test_adcp_idem_{secrets.token_hex(6)}" - async with psycopg_pool.AsyncConnectionPool(TEST_URL, min_size=2, max_size=8) as pool: - backend = PgBackend(pool=pool, table_name=table) + async with ( + psycopg_pool.AsyncConnectionPool(TEST_URL, min_size=2, max_size=8) as pool, + psycopg_pool.AsyncConnectionPool(TEST_URL, min_size=2, max_size=8) as lock_pool, + ): + backend = PgBackend(pool=pool, lock_pool=lock_pool, table_name=table) await backend.create_schema() try: yield backend @@ -235,3 +238,37 @@ class Ctx: # AdCP L1/security rule 4 (#714): replay envelope carries ``replayed: true``. assert r2.get("replayed") is True assert {k: v for k, v in r2.items() if k != "replayed"} == r1 + + +@pytest.mark.asyncio +async def test_concurrent_wrapped_calls_execute_once(isolated_backend: PgBackend) -> None: + import asyncio + + store = IdempotencyStore(backend=isolated_backend, ttl_seconds=3600) + entered = asyncio.Event() + release = asyncio.Event() + calls = 0 + + @store.wrap + async def handler(self, params, context=None): + nonlocal calls + calls += 1 + entered.set() + await release.wait() + return {"task_id": "only", "status": "ok"} + + class Ctx: + caller_identity = "buyer-acme" + tenant_id = "tenant-1" + + params = {"idempotency_key": "shared-key", "x": 42} + first = asyncio.create_task(handler(None, params, Ctx())) + await entered.wait() + second = asyncio.create_task(handler(None, params, Ctx())) + await asyncio.sleep(0.05) + assert calls == 1 + release.set() + + first_result, second_result = await asyncio.gather(first, second) + assert first_result.get("replayed") is not True + assert second_result["replayed"] is True diff --git a/tests/conformance/signing/test_autosign_hook.py b/tests/conformance/signing/test_autosign_hook.py index 8823dce89..5643b2580 100644 --- a/tests/conformance/signing/test_autosign_hook.py +++ b/tests/conformance/signing/test_autosign_hook.py @@ -171,6 +171,54 @@ async def test_hook_skips_when_context_var_unset(signing_config: SigningConfig) assert dict(request.headers) == before +async def test_hook_reads_mcp_operation_from_jsonrpc_body( + signing_config: SigningConfig, +) -> None: + client = _make_client(signing=signing_config) + client._capabilities = _make_caps(required=["create_media_buy"]) + body = json.dumps( + { + "jsonrpc": "2.0", + "id": 1, + "method": "tools/call", + "params": {"name": "create_media_buy", "arguments": {"plan_id": "p1"}}, + }, + separators=(",", ":"), + ).encode() + request = _build_request(url="https://seller.example.com/mcp", body=body) + + # No ContextVar is set: this matches the MCP writer task that actually + # invokes the httpx hook. + await client._sign_outgoing_request(request) + + assert "Signature" in request.headers + assert "Signature-Input" in request.headers + _verify( + request, + body, + operation="create_media_buy", + required_for=frozenset({"create_media_buy"}), + ) + + +async def test_mcp_hook_fails_closed_without_prefetched_policy( + signing_config: SigningConfig, +) -> None: + client = _make_client(signing=signing_config) + body = json.dumps( + { + "jsonrpc": "2.0", + "id": 1, + "method": "tools/call", + "params": {"name": "create_media_buy", "arguments": {}}, + } + ).encode() + request = _build_request(url="https://seller.example.com/mcp", body=body) + + with pytest.raises(RuntimeError, match="was not prefetched"): + await client._sign_outgoing_request(request) + + async def test_hook_skips_for_get_adcp_capabilities(signing_config: SigningConfig) -> None: client = _make_client(signing=signing_config) request = _build_request() diff --git a/tests/conformance/signing/test_autosign_mcp.py b/tests/conformance/signing/test_autosign_mcp.py index a7d99e145..36baebef6 100644 --- a/tests/conformance/signing/test_autosign_mcp.py +++ b/tests/conformance/signing/test_autosign_mcp.py @@ -3,8 +3,9 @@ The hook behavior itself (``ADCPClient._sign_outgoing_request``) is covered by ``test_autosign_hook.py`` — both adapters share it. These tests focus on MCP-specific plumbing: the custom ``httpx_client_factory`` that -``streamablehttp_client`` receives, the SSE-transport warning path, and -the ``current_operation`` ContextVar scope around ``session.call_tool``. +``streamablehttp_client`` receives, the SSE-transport warning path, signing +policy prefetch, and the legacy ``current_operation`` scope around +``session.call_tool``. """ from __future__ import annotations @@ -205,6 +206,31 @@ async def _capture(*_args: Any, **_kwargs: Any) -> Any: assert observed == ["create_media_buy"] +async def test_signing_capabilities_prefetched_before_call_tool() -> None: + order: list[str] = [] + + async def _prefetch() -> None: + order.append("prefetch") + + async def _capture(*_args: Any, **_kwargs: Any) -> Any: + order.append("call_tool") + result = MagicMock() + result.isError = False + result.content = [] + result.structuredContent = None + return result + + adapter = _make_mcp_adapter("streamable_http") + adapter.signing_capability_check = _prefetch + fake_session = MagicMock() + fake_session.call_tool = _capture + adapter._get_session = AsyncMock(return_value=fake_session) # type: ignore[method-assign] + + await adapter._call_mcp_tool("create_media_buy", {}) + + assert order == ["prefetch", "call_tool"] + + async def test_context_var_reset_on_exception() -> None: """If call_tool raises, the ContextVar still resets.""" diff --git a/tests/conformance/signing/test_bounded_fetches.py b/tests/conformance/signing/test_bounded_fetches.py new file mode 100644 index 000000000..627253887 --- /dev/null +++ b/tests/conformance/signing/test_bounded_fetches.py @@ -0,0 +1,89 @@ +"""Remote signing documents are bounded while streaming, not after buffering.""" + +from __future__ import annotations + +import httpx +import pytest + +from adcp.signing.jwks import async_default_jwks_fetcher, default_jwks_fetcher +from adcp.signing.revocation_fetcher import ( + RevocationListFetchError, + async_default_revocation_list_fetcher, + default_revocation_list_fetcher, +) + + +class _SyncChunks(httpx.SyncByteStream): + def __init__(self) -> None: + self.read = 0 + + def __iter__(self): # type: ignore[no-untyped-def] + for chunk in (b"xxxx", b"yyyy", b"zzzz"): + self.read += 1 + yield chunk + + +class _AsyncChunks(httpx.AsyncByteStream): + def __init__(self) -> None: + self.read = 0 + + async def __aiter__(self): # type: ignore[no-untyped-def] + for chunk in (b"xxxx", b"yyyy", b"zzzz"): + self.read += 1 + yield chunk + + +def test_sync_jwks_stops_reading_chunked_oversize(monkeypatch: pytest.MonkeyPatch) -> None: + stream = _SyncChunks() + transport = httpx.MockTransport(lambda request: httpx.Response(200, stream=stream)) + monkeypatch.setattr( + "adcp.signing.ip_pinned_transport.build_ip_pinned_transport", + lambda uri, **kwargs: transport, + ) + with pytest.raises(ValueError, match="exceeds 5 bytes"): + default_jwks_fetcher("https://keys.example/jwks", max_body_bytes=5) + assert stream.read == 2 + + +@pytest.mark.asyncio +async def test_async_jwks_stops_reading_chunked_oversize( + monkeypatch: pytest.MonkeyPatch, +) -> None: + stream = _AsyncChunks() + transport = httpx.MockTransport(lambda request: httpx.Response(200, stream=stream)) + monkeypatch.setattr( + "adcp.signing.ip_pinned_transport.build_async_ip_pinned_transport", + lambda uri, **kwargs: transport, + ) + with pytest.raises(ValueError, match="exceeds 5 bytes"): + await async_default_jwks_fetcher("https://keys.example/jwks", max_body_bytes=5) + assert stream.read == 2 + + +def test_sync_revocation_stops_reading_chunked_oversize( + monkeypatch: pytest.MonkeyPatch, +) -> None: + stream = _SyncChunks() + transport = httpx.MockTransport(lambda request: httpx.Response(200, stream=stream)) + monkeypatch.setattr( + "adcp.signing.ip_pinned_transport.build_ip_pinned_transport", + lambda uri, **kwargs: transport, + ) + with pytest.raises(RevocationListFetchError, match="exceeds 5 bytes"): + default_revocation_list_fetcher("https://gov.example/list", max_body_bytes=5) + assert stream.read == 2 + + +@pytest.mark.asyncio +async def test_async_revocation_stops_reading_chunked_oversize( + monkeypatch: pytest.MonkeyPatch, +) -> None: + stream = _AsyncChunks() + transport = httpx.MockTransport(lambda request: httpx.Response(200, stream=stream)) + monkeypatch.setattr( + "adcp.signing.ip_pinned_transport.build_async_ip_pinned_transport", + lambda uri, **kwargs: transport, + ) + with pytest.raises(RevocationListFetchError, match="exceeds 5 bytes"): + await async_default_revocation_list_fetcher("https://gov.example/list", max_body_bytes=5) + assert stream.read == 2 diff --git a/tests/conformance/signing/test_install_signing_event_hook.py b/tests/conformance/signing/test_install_signing_event_hook.py index e7893d942..0379ef173 100644 --- a/tests/conformance/signing/test_install_signing_event_hook.py +++ b/tests/conformance/signing/test_install_signing_event_hook.py @@ -289,7 +289,7 @@ def provider() -> RequestSigning | None: @pytest.mark.asyncio async def test_forbidden_covers_content_digest_omits_digest_coverage() -> None: - """Capability with covers_content_digest='forbidden' ⇒ signature must NOT cover content-digest.""" + """A forbidden digest policy must not cover content-digest.""" body = b'{"plan_id":"p1"}' request = httpx.Request( method="POST", @@ -386,3 +386,52 @@ async def existing_hook(_request: httpx.Request) -> None: assert pre_existing_called == [True] assert "Signature" in request.headers + + +@pytest.mark.asyncio +async def test_cross_origin_redirect_is_rejected_before_second_request() -> None: + seen: list[str] = [] + + def handler(request: httpx.Request) -> httpx.Response: + seen.append(str(request.url)) + if request.url.host == "seller.example.com": + return httpx.Response( + 307, + headers={"Location": "https://attacker.example.net/capture"}, + ) + return httpx.Response(200, json={"captured": True}) + + client = httpx.AsyncClient( + follow_redirects=True, + transport=httpx.MockTransport(handler), + ) + install_signing_event_hook( + client, + signing=_config(), + seller_capability=_capability(required=["create_media_buy"]), + expected_origin="https://seller.example.com", + ) + + with signing_operation("create_media_buy"), pytest.raises(ValueError, match="cross-origin"): + await client.post("https://seller.example.com/mcp", content=b"{}") + + assert seen == ["https://seller.example.com/mcp"] + await client.aclose() + + +@pytest.mark.asyncio +async def test_first_scoped_request_binds_origin_when_not_configured() -> None: + capability = _capability(required=["create_media_buy"]) + client = httpx.AsyncClient() + install_signing_event_hook(client, signing=_config(), seller_capability=capability) + [hook] = client.event_hooks["request"] + + seller_request = httpx.Request("POST", "https://seller.example.com/mcp", content=b"{}") + attacker_request = httpx.Request("POST", "https://attacker.example.net/x", content=b"{}") + with signing_operation("create_media_buy"): + await hook(seller_request) + with pytest.raises(ValueError, match="cross-origin"): + await hook(attacker_request) + + assert "Signature" in seller_request.headers + assert "Signature" not in attacker_request.headers diff --git a/tests/conformance/signing/test_jwks.py b/tests/conformance/signing/test_jwks.py index 9aae2ef7f..fa994ff67 100644 --- a/tests/conformance/signing/test_jwks.py +++ b/tests/conformance/signing/test_jwks.py @@ -2,7 +2,9 @@ from __future__ import annotations +import asyncio import socket +import threading from typing import Any from unittest.mock import patch @@ -10,6 +12,7 @@ from adcp.signing import ( DEFAULT_ALLOWED_PORTS, + AsyncCachingJwksResolver, CachingJwksResolver, SignatureVerificationError, SSRFValidationError, @@ -388,6 +391,159 @@ def fetcher(uri: str, *, allow_private: bool = False) -> dict[str, Any]: assert calls == 2 +def test_caching_resolver_revalidates_known_kid_after_max_age() -> None: + calls = 0 + + def fetcher(uri: str, *, allow_private: bool = False) -> dict[str, Any]: + nonlocal calls + calls += 1 + return _make_jwks("k1") if calls == 1 else _make_jwks("replacement") + + clock = {"t": 0.0} + resolver = CachingJwksResolver( + "https://example.com/jwks.json", + fetcher=fetcher, + max_age_seconds=60.0, + clock=lambda: clock["t"], + ) + assert resolver("k1") is not None + clock["t"] = 61.0 + assert resolver("k1") is None + assert calls == 2 + + +def test_sync_caching_resolver_single_flights_concurrent_expiry_refresh() -> None: + calls = 0 + refresh_started = threading.Event() + release_refresh = threading.Event() + + def fetcher(uri: str, *, allow_private: bool = False) -> dict[str, Any]: + nonlocal calls + del uri, allow_private + calls += 1 + if calls == 1: + return _make_jwks("k1") + refresh_started.set() + assert release_refresh.wait(timeout=5) + return _make_jwks("replacement") + + clock = {"t": 0.0} + resolver = CachingJwksResolver( + "https://example.com/jwks.json", + fetcher=fetcher, + max_age_seconds=60.0, + clock=lambda: clock["t"], + ) + assert resolver("k1") is not None + + class _ObservedLock: + def __init__(self) -> None: + self._lock = threading.Lock() + self._guard = threading.Lock() + self._attempts = 0 + self.second_attempted = threading.Event() + + def __enter__(self) -> None: + with self._guard: + self._attempts += 1 + if self._attempts == 2: + self.second_attempted.set() + self._lock.acquire() + + def __exit__(self, *args: object) -> None: + self._lock.release() + + observed_lock = _ObservedLock() + resolver._refresh_lock = observed_lock + clock["t"] = 61.0 + start = threading.Barrier(3) + results: list[dict[str, Any] | None] = [] + errors: list[Exception] = [] + + def resolve_expired() -> None: + try: + start.wait() + results.append(resolver("replacement")) + except Exception as exc: # pragma: no cover - asserted below + errors.append(exc) + + threads = [threading.Thread(target=resolve_expired) for _ in range(2)] + for thread in threads: + thread.start() + start.wait() + assert refresh_started.wait(timeout=5) + assert observed_lock.second_attempted.wait(timeout=5) + release_refresh.set() + for thread in threads: + thread.join(timeout=5) + + assert errors == [] + assert all(not thread.is_alive() for thread in threads) + assert len(results) == 2 + assert all(result is not None for result in results) + assert calls == 2 + + +async def test_async_caching_resolver_revalidates_known_kid_after_max_age() -> None: + calls = 0 + + async def fetcher(uri: str, *, allow_private: bool = False) -> dict[str, Any]: + nonlocal calls + calls += 1 + return _make_jwks("k1") if calls == 1 else _make_jwks("replacement") + + clock = {"t": 0.0} + resolver = AsyncCachingJwksResolver( + "https://example.com/jwks.json", + fetcher=fetcher, + max_age_seconds=60.0, + clock=lambda: clock["t"], + ) + assert await resolver("k1") is not None + clock["t"] = 61.0 + assert await resolver("k1") is None + assert calls == 2 + + +async def test_async_caching_resolver_single_flights_concurrent_expiry_refresh() -> None: + calls = 0 + refresh_started = asyncio.Event() + release_refresh = asyncio.Event() + + async def fetcher(uri: str, *, allow_private: bool = False) -> dict[str, Any]: + nonlocal calls + del uri, allow_private + calls += 1 + if calls == 1: + return _make_jwks("k1") + refresh_started.set() + await release_refresh.wait() + return _make_jwks("replacement") + + clock = {"t": 0.0} + resolver = AsyncCachingJwksResolver( + "https://example.com/jwks.json", + fetcher=fetcher, + max_age_seconds=60.0, + clock=lambda: clock["t"], + ) + assert await resolver("k1") is not None + + clock["t"] = 61.0 + first = asyncio.create_task(resolver("replacement")) + await refresh_started.wait() + second = asyncio.create_task(resolver("replacement")) + await asyncio.sleep(0) + + assert not second.done() + release_refresh.set() + assert await asyncio.gather(first, second) == [ + _make_jwks("replacement")["keys"][0], + _make_jwks("replacement")["keys"][0], + ] + assert calls == 2 + + def test_caching_resolver_wraps_ssrf_as_untrusted() -> None: def fetcher(uri: str, *, allow_private: bool = False) -> dict[str, Any]: raise SSRFValidationError("blocked") @@ -410,6 +566,28 @@ def fetcher(uri: str, *, allow_private: bool = False) -> dict[str, Any]: assert exc.value.code == "request_signature_jwks_unavailable" +def test_caching_resolver_wraps_malformed_key_as_unavailable() -> None: + def fetcher(uri: str, *, allow_private: bool = False) -> dict[str, Any]: + return {"keys": [1]} + + resolver = CachingJwksResolver("https://example.com/jwks.json", fetcher=fetcher) + with pytest.raises(SignatureVerificationError) as exc: + resolver("k1") + assert exc.value.code == "request_signature_jwks_unavailable" + assert isinstance(exc.value.__cause__, ValueError) + + +async def test_async_caching_resolver_wraps_malformed_key_as_unavailable() -> None: + async def fetcher(uri: str, *, allow_private: bool = False) -> dict[str, Any]: + return {"keys": [1]} + + resolver = AsyncCachingJwksResolver("https://example.com/jwks.json", fetcher=fetcher) + with pytest.raises(SignatureVerificationError) as exc: + await resolver("k1") + assert exc.value.code == "request_signature_jwks_unavailable" + assert isinstance(exc.value.__cause__, ValueError) + + # ---- StaticJwksResolver ---- diff --git a/tests/conformance/signing/test_pg_replay_store.py b/tests/conformance/signing/test_pg_replay_store.py index eb0bf5ea6..cf88d4556 100644 --- a/tests/conformance/signing/test_pg_replay_store.py +++ b/tests/conformance/signing/test_pg_replay_store.py @@ -33,6 +33,7 @@ allow_module_level=True, ) +from adcp.signing import AtomicReplayStore # noqa: E402 from adcp.signing.pg import PgReplayStore # noqa: E402 @@ -70,6 +71,10 @@ def _store(fixture, **overrides) -> PgReplayStore: return PgReplayStore(pool=pool, table_name=table, **overrides) +def test_satisfies_atomic_replay_store_protocol(isolated_pool) -> None: + assert isinstance(_store(isolated_pool), AtomicReplayStore) + + # -- Protocol happy path ---------------------------------------------- @@ -174,19 +179,17 @@ def test_sweep_expired_returns_zero_when_clean(isolated_pool) -> None: # -- concurrency ----------------------------------------------------- -def test_concurrent_remember_same_nonce_is_idempotent(isolated_pool) -> None: - """Two workers racing on the same (keyid, nonce) MUST NOT error. - - ``ON CONFLICT ... DO UPDATE`` makes the second insert a no-op - (with refreshed TTL). Without it, the second worker would hit a - PK violation and blow up. - """ +def test_concurrent_claim_same_nonce_has_one_winner(isolated_pool) -> None: + """Only one verifier process may claim a nonce.""" store = _store(isolated_pool) errors: list[Exception] = [] + results: list[str] = [] + barrier = threading.Barrier(10) def worker() -> None: try: - store.remember("k", "shared", ttl_seconds=60) + barrier.wait() + results.append(store.claim("k", "shared", ttl_seconds=60)) except Exception as exc: # noqa: BLE001 errors.append(exc) @@ -197,6 +200,8 @@ def worker() -> None: t.join() assert errors == [] + assert results.count("claimed") == 1 + assert results.count("replayed") == 9 assert store.seen("k", "shared") is True assert store.live_count("k") == 1 diff --git a/tests/conformance/signing/test_replay.py b/tests/conformance/signing/test_replay.py index 7ff97d941..2bdc00b92 100644 --- a/tests/conformance/signing/test_replay.py +++ b/tests/conformance/signing/test_replay.py @@ -5,7 +5,7 @@ import threading from dataclasses import dataclass -from adcp.signing import InMemoryReplayStore +from adcp.signing import AtomicReplayStore, InMemoryReplayStore @dataclass @@ -38,6 +38,25 @@ def worker(tid: int) -> None: assert store._counts["kid"] == thread_count * nonces_per_thread +def test_concurrent_claim_has_exactly_one_winner() -> None: + store = InMemoryReplayStore() + barrier = threading.Barrier(16) + results: list[str] = [] + + def worker() -> None: + barrier.wait() + results.append(store.claim("kid", "shared", ttl_seconds=60.0)) + + threads = [threading.Thread(target=worker) for _ in range(16)] + for thread in threads: + thread.start() + for thread in threads: + thread.join() + + assert results.count("claimed") == 1 + assert results.count("replayed") == 15 + + def test_monotonic_clock_ttl_expires_and_decrements_count() -> None: clock = _FakeClock(now=0.0) store = InMemoryReplayStore(per_keyid_cap=5, clock=clock) @@ -68,6 +87,18 @@ def test_at_capacity_flips_when_entries_expire() -> None: assert store.at_capacity("kid") is False +def test_remember_reports_capacity_refusal() -> None: + store = InMemoryReplayStore(per_keyid_cap=2, global_cap=10) + assert store.remember("kid", "n1", ttl_seconds=60.0) is True + assert store.remember("kid", "n2", ttl_seconds=60.0) is True + assert store.remember("kid", "n3", ttl_seconds=60.0) is False + assert store.seen("kid", "n3") is False + + +def test_in_memory_store_satisfies_atomic_protocol() -> None: + assert isinstance(InMemoryReplayStore(), AtomicReplayStore) + + def test_at_capacity_is_o1_after_sweep() -> None: # Smoke test that at_capacity doesn't do an O(n) scan — we check the count # dict directly (construction and check). @@ -83,3 +114,36 @@ def test_mark_cap_hit_still_works() -> None: store = InMemoryReplayStore() store.mark_cap_hit("kid") assert store.at_capacity("kid") is True + + +def test_global_cap_bounds_rotating_keyids_and_recovers_after_expiry() -> None: + clock = _FakeClock(now=0.0) + store = InMemoryReplayStore(per_keyid_cap=10, global_cap=3, clock=clock) + assert store.claim("kid-1", "n", 10.0) == "claimed" + assert store.claim("kid-2", "n", 10.0) == "claimed" + assert store.claim("kid-3", "n", 10.0) == "claimed" + assert store.claim("kid-4", "n", 10.0) == "capacity" + assert len(store._entries) == 3 + + clock.now = 11.0 + assert store.claim("kid-4", "n", 10.0) == "claimed" + assert len(store._entries) == 1 + + +def test_claim_does_not_copy_the_entry_table() -> None: + class _NoItemsDict(dict[tuple[str, str], float]): + def items(self): # type: ignore[no-untyped-def] + raise AssertionError("accepted claim copied/scanned the full entry table") + + store = InMemoryReplayStore(global_cap=100) + store._entries = _NoItemsDict() + for index in range(50): + assert store.claim(f"kid-{index}", "nonce", 60.0) == "claimed" + + +def test_renewal_heap_remains_bounded() -> None: + store = InMemoryReplayStore(global_cap=3) + for index in range(50): + store.remember("kid", "nonce", float(index + 1)) + assert len(store._entries) == 1 + assert len(store._expiry_heap) <= 6 diff --git a/tests/conformance/signing/test_revocation_fetcher.py b/tests/conformance/signing/test_revocation_fetcher.py index aaf512716..e3ca4698c 100644 --- a/tests/conformance/signing/test_revocation_fetcher.py +++ b/tests/conformance/signing/test_revocation_fetcher.py @@ -336,9 +336,7 @@ def test_accepts_future_version_with_forward_compat() -> None: # version=2 should NOT hard-reject: additive schema changes shouldn't # force every old SDK into fail-closed across their entire traffic. private, _, jwks_resolver = _key_and_jwks() - token = _sign_jws_compact( - _make_payload(version=2, revoked_kids=["rev"]), private=private - ) + token = _sign_jws_compact(_make_payload(version=2, revoked_kids=["rev"]), private=private) fetcher = _ScriptedFetcher() fetcher.enqueue(FetchResult(body=token, etag=None, not_modified=False)) @@ -546,12 +544,12 @@ def test_replay_older_list_rejected() -> None: revoked_kids=[], # attacker un-revokes the kid ) fetcher = _ScriptedFetcher() - fetcher.enqueue(FetchResult( - body=_sign_jws_compact(newer, private=private), etag='"v2"', not_modified=False - )) - fetcher.enqueue(FetchResult( - body=_sign_jws_compact(older, private=private), etag='"v1"', not_modified=False - )) + fetcher.enqueue( + FetchResult(body=_sign_jws_compact(newer, private=private), etag='"v2"', not_modified=False) + ) + fetcher.enqueue( + FetchResult(body=_sign_jws_compact(older, private=private), etag='"v1"', not_modified=False) + ) wall_clock, mono_clock, advance = _controllable_clock( datetime(2026, 4, 18, 14, 15, tzinfo=timezone.utc) @@ -693,12 +691,14 @@ def test_if_modified_since_threaded_to_fetcher() -> None: private, _, jwks_resolver = _key_and_jwks() token = _sign_jws_compact(_make_payload(), private=private) fetcher = _ScriptedFetcher() - fetcher.enqueue(FetchResult( - body=token, - etag='"v1"', - last_modified="Sat, 18 Apr 2026 14:00:00 GMT", - not_modified=False, - )) + fetcher.enqueue( + FetchResult( + body=token, + etag='"v1"', + last_modified="Sat, 18 Apr 2026 14:00:00 GMT", + not_modified=False, + ) + ) fetcher.enqueue(FetchResult(body="", etag='"v1"', not_modified=True)) wall_clock, mono_clock, advance = _controllable_clock( @@ -819,19 +819,14 @@ def test_last_modified_header_injection_rejected() -> None: assert _sanitize_last_modified(None) is None -def test_304_slides_next_update_forward() -> None: - """Round-2: successive 304s advance the cached next_update so the - checker doesn't hit the refresh-cooldown path on every call past the - original next_update.""" +def test_304_does_not_extend_signed_next_update() -> None: + """Transport validators cannot extend a signed authorization window.""" private, _, jwks_resolver = _key_and_jwks() token = _sign_jws_compact(_make_payload(), private=private) fetcher = _ScriptedFetcher() fetcher.enqueue(FetchResult(body=token, etag='"v1"', not_modified=False)) fetcher.enqueue(FetchResult(body="", etag='"v1"', not_modified=True)) - # If next_update wasn't advanced on 304, the next two calls past 14:30 - # would each try to refetch (subject to the 60s cooldown). We only - # queue ONE more fetcher response, so if the invariant breaks, one of - # the later calls raises AssertionError from the scripted fetcher. + fetcher.enqueue(FetchResult(body="", etag='"v1"', not_modified=True)) wall_clock, mono_clock, advance = _controllable_clock( datetime(2026, 4, 18, 14, 1, tzinfo=timezone.utc) @@ -846,16 +841,33 @@ def test_304_slides_next_update_forward() -> None: ) checker("k") # 1 fetch → initial advance(15 * 60 + 30) # 14:16:30, past original next_update (14:15) - checker("k") # 2 fetches → 304, should slide next_update to 14:30 + checker("k") # within grace, but the signed deadline remains unchanged - # Now at 14:16:30. Cached next_update was 14:15, now should be 14:30. assert checker._current_list is not None - assert checker._current_list.next_update.startswith("2026-04-18T14:30:00") + assert checker._current_list.next_update == "2026-04-18T14:15:00Z" - # Additional calls WITHIN the new window should NOT refetch. - advance(60) # 14:17:30 - checker("k") # still no fetch — we're before the new 14:30 next_update - assert len(fetcher.calls) == 2 + advance(29 * 60) # beyond signed next_update + 2x interval grace + with pytest.raises(RevocationListFreshnessError): + checker("k") + + +def test_cold_checker_accepts_list_within_next_update_grace() -> None: + private, _, jwks_resolver = _key_and_jwks() + token = _sign_jws_compact(_make_payload(), private=private) + fetcher = _ScriptedFetcher() + fetcher.enqueue(FetchResult(body=token, etag='"v1"', not_modified=False)) + wall_clock, mono_clock, _ = _controllable_clock( + datetime(2026, 4, 18, 14, 16, tzinfo=timezone.utc) + ) + checker = CachingRevocationChecker( + revocation_uri=REVOCATION_URI, + issuer=ISSUER, + jwks_resolver=jwks_resolver, + fetcher=fetcher, + wall_clock=wall_clock, + clock=mono_clock, + ) + assert checker("k") is False def test_clock_footgun_rejects_time_time() -> None: diff --git a/tests/conformance/signing/test_verifier_behaviors.py b/tests/conformance/signing/test_verifier_behaviors.py index 6432079c0..b75e19e5e 100644 --- a/tests/conformance/signing/test_verifier_behaviors.py +++ b/tests/conformance/signing/test_verifier_behaviors.py @@ -9,6 +9,7 @@ import dataclasses import json +import warnings from pathlib import Path from typing import Any @@ -346,11 +347,144 @@ def test_verify_options_rejects_positional() -> None: # ---- 6a: VerifierCapability default ---- -def test_verifier_capability_defaults_to_either_digest() -> None: - """The AdCP 3.0 schema declares ``covers_content_digest`` default as - ``"either"`` (``get-adcp-capabilities-response.json``); ``"required"`` - is opt-in for spend-committing operations. AdCP 4.0 is expected to - recommend ``"required"`` more broadly. Operators promote operations - selectively via ``required_for=frozenset({"create_media_buy", ...})``.""" +def test_verifier_capability_defaults_to_wire_digest_policy() -> None: cap = VerifierCapability() assert cap.covers_content_digest == "either" + + +def test_default_capability_accepts_spec_legal_signature_without_body_binding() -> None: + headers, body = _sign_basic() + options = VerifyOptions( + now=1776520800.0, + capability=VerifierCapability(), + operation="create_media_buy", + jwks_resolver=StaticJwksResolver({"keys": [ED25519_KEY]}), + ) + + signer = verify_request_signature( + method="POST", + url="https://seller.example.com/adcp/create_media_buy", + headers=headers, + body=body, + options=options, + ) + assert signer.key_id + + +def test_legacy_replay_store_warns_and_remains_compatible() -> None: + class LegacyReplayStore: + def __init__(self) -> None: + self.entries: set[tuple[str, str]] = set() + + def seen(self, keyid: str, nonce: str) -> bool: + return (keyid, nonce) in self.entries + + def remember(self, keyid: str, nonce: str, ttl_seconds: float) -> None: + del ttl_seconds + self.entries.add((keyid, nonce)) + + def at_capacity(self, keyid: str) -> bool: + del keyid + return False + + headers, body = _sign_basic() + store = LegacyReplayStore() + options = VerifyOptions( + now=1776520800.0, + capability=VerifierCapability(covers_content_digest="either"), + operation="create_media_buy", + jwks_resolver=StaticJwksResolver({"keys": [ED25519_KEY]}), + replay_store=store, + ) + + with pytest.warns(DeprecationWarning, match=r"does not implement atomic claim\(\)"): + verify_request_signature( + method="POST", + url="https://seller.example.com/adcp/create_media_buy", + headers=headers, + body=body, + options=options, + ) + + with warnings.catch_warnings(record=True) as emitted: + with pytest.raises(SignatureVerificationError) as exc: + verify_request_signature( + method="POST", + url="https://seller.example.com/adcp/create_media_buy", + headers=headers, + body=body, + options=options, + ) + assert emitted == [] + assert exc.value.code == "request_signature_replayed" + + +def test_atomic_replay_store_invalid_result_fails_closed() -> None: + class InvalidReplayStore: + def seen(self, keyid: str, nonce: str) -> bool: + return False + + def remember(self, keyid: str, nonce: str, ttl_seconds: float) -> None: + pass + + def at_capacity(self, keyid: str) -> bool: + return False + + def claim(self, keyid: str, nonce: str, ttl_seconds: float) -> bool: + return True + + headers, body = _sign_basic() + options = VerifyOptions( + now=1776520800.0, + capability=VerifierCapability(covers_content_digest="either"), + operation="create_media_buy", + jwks_resolver=StaticJwksResolver({"keys": [ED25519_KEY]}), + replay_store=InvalidReplayStore(), # type: ignore[arg-type] + ) + + with pytest.raises(SignatureVerificationError) as exc: + verify_request_signature( + method="POST", + url="https://seller.example.com/adcp/create_media_buy", + headers=headers, + body=body, + options=options, + ) + assert exc.value.code == "request_signature_rate_abuse" + assert exc.value.step == 13 + + +def test_atomic_replay_capacity_rejection_is_step_13() -> None: + class CapacityReplayStore: + def seen(self, keyid: str, nonce: str) -> bool: + return False + + def remember(self, keyid: str, nonce: str, ttl_seconds: float) -> bool: + return False + + def at_capacity(self, keyid: str) -> bool: + return False + + def claim(self, keyid: str, nonce: str, ttl_seconds: float) -> str: + return "capacity" + + headers, body = _sign_basic() + options = VerifyOptions( + now=1776520800.0, + capability=VerifierCapability(covers_content_digest="either"), + operation="create_media_buy", + jwks_resolver=StaticJwksResolver({"keys": [ED25519_KEY]}), + replay_store=CapacityReplayStore(), # type: ignore[arg-type] + ) + + with pytest.raises(SignatureVerificationError) as exc: + verify_request_signature( + method="POST", + url="https://seller.example.com/adcp/create_media_buy", + headers=headers, + body=body, + options=options, + ) + + assert exc.value.code == "request_signature_rate_abuse" + assert exc.value.step == 13 diff --git a/tests/conformance/signing/test_verifier_key_origins.py b/tests/conformance/signing/test_verifier_key_origins.py index 2a00eebd0..298bb6c88 100644 --- a/tests/conformance/signing/test_verifier_key_origins.py +++ b/tests/conformance/signing/test_verifier_key_origins.py @@ -291,11 +291,7 @@ def test_resolver_without_source_attribute_skips_check() -> None: # ----- check does not fire without expected_key_origins ----- -def test_brand_json_source_skips_check_when_no_expected_origins() -> None: - """``expected_key_origins=None`` (default) → check skips even on a - brand-json-sourced resolver. Adopters who haven't yet plumbed - capabilities through the verifier see no behavior change. - """ +def test_brand_json_source_skips_when_no_capabilities_map_supplied() -> None: headers, body = _sign_basic() resolver = _BrandJsonStaticResolver( # Even with a mismatched jwks_uri, the check skips when the @@ -304,13 +300,14 @@ def test_brand_json_source_skips_check_when_no_expected_origins() -> None: jwks_uri="https://different.example/.well-known/jwks.json", ) options = _options_with(resolver, expected_key_origins=None) - verify_request_signature( + signer = verify_request_signature( method="POST", url="https://seller.example.com/adcp/create_media_buy", headers=headers, body=body, options=options, ) + assert signer.key_id # ----- earlier failure codes still surface ----- diff --git a/tests/conformance/signing/test_webhook_dedup.py b/tests/conformance/signing/test_webhook_dedup.py index b35255c95..2c4396155 100644 --- a/tests/conformance/signing/test_webhook_dedup.py +++ b/tests/conformance/signing/test_webhook_dedup.py @@ -2,9 +2,17 @@ from __future__ import annotations +import asyncio + import pytest -from adcp.server.idempotency import MemoryBackend, WebhookDedupStore +from adcp.server.idempotency import ( + CachedResponse, + IdempotencyBackend, + LazyBackend, + MemoryBackend, + WebhookDedupStore, +) @pytest.fixture @@ -23,6 +31,92 @@ async def test_repeat_returns_false(store: WebhookDedupStore) -> None: assert await store.check_and_record("sender-1", "whk_abc") is False +@pytest.mark.asyncio +async def test_legacy_backend_warns_and_preserves_repeat_dedup() -> None: + class LegacyBackend(IdempotencyBackend): + def __init__(self) -> None: + self.entries: dict[tuple[str, str], CachedResponse] = {} + + async def get(self, scope_key: str, key: str) -> CachedResponse | None: + return self.entries.get((scope_key, key)) + + async def put(self, scope_key: str, key: str, entry: CachedResponse) -> None: + self.entries[(scope_key, key)] = entry + + async def delete_expired(self, now_epoch: float | None = None) -> int: + return 0 + + store = WebhookDedupStore(LegacyBackend()) + with pytest.warns(DeprecationWarning, match="process-local webhook dedup locking"): + assert await store.check_and_record("sender-1", "whk_legacy") is True + assert await store.check_and_record("sender-1", "whk_legacy") is False + + +@pytest.mark.asyncio +async def test_lazy_wrapped_legacy_backend_preserves_fallback_dedup() -> None: + class LegacyBackend(IdempotencyBackend): + def __init__(self) -> None: + self.entries: dict[tuple[str, str], CachedResponse] = {} + + async def get(self, scope_key: str, key: str) -> CachedResponse | None: + return self.entries.get((scope_key, key)) + + async def put(self, scope_key: str, key: str, entry: CachedResponse) -> None: + self.entries[(scope_key, key)] = entry + + async def delete_expired(self, now_epoch: float | None = None) -> int: + return 0 + + store = WebhookDedupStore(LazyBackend(LegacyBackend)) + with pytest.warns(DeprecationWarning, match="process-local webhook dedup locking"): + assert await store.check_and_record("sender-1", "whk_lazy_legacy") is True + assert await store.check_and_record("sender-1", "whk_lazy_legacy") is False + + +@pytest.mark.asyncio +async def test_legacy_backend_lock_is_shared_across_store_instances() -> None: + class LegacyBackend(IdempotencyBackend): + def __init__(self) -> None: + self.entries: dict[tuple[str, str], CachedResponse] = {} + + async def get(self, scope_key: str, key: str) -> CachedResponse | None: + await asyncio.sleep(0) + return self.entries.get((scope_key, key)) + + async def put(self, scope_key: str, key: str, entry: CachedResponse) -> None: + await asyncio.sleep(0) + self.entries[(scope_key, key)] = entry + + async def delete_expired(self, now_epoch: float | None = None) -> int: + return 0 + + backend = LegacyBackend() + stores = [WebhookDedupStore(backend), WebhookDedupStore(backend)] + with pytest.warns(DeprecationWarning): + results = await asyncio.gather( + *(store.check_and_record("sender-1", "whk_shared") for store in stores) + ) + assert sorted(results) == [False, True] + + +@pytest.mark.asyncio +async def test_concurrent_deliveries_have_exactly_one_first_seen( + store: WebhookDedupStore, +) -> None: + gate = asyncio.Event() + + async def deliver() -> bool: + await gate.wait() + return await store.check_and_record("sender-1", "whk_shared") + + tasks = [asyncio.create_task(deliver()) for _ in range(20)] + gate.set() + results = await asyncio.gather(*tasks) + + assert results.count(True) == 1 + assert results.count(False) == 19 + + @pytest.mark.asyncio async def test_different_senders_independent(store: WebhookDedupStore) -> None: """Per-sender scoping: the same key from a different sender is fresh.""" diff --git a/tests/conformance/signing/test_webhook_receiver.py b/tests/conformance/signing/test_webhook_receiver.py index 661f8d219..4de12645b 100644 --- a/tests/conformance/signing/test_webhook_receiver.py +++ b/tests/conformance/signing/test_webhook_receiver.py @@ -107,7 +107,10 @@ async def test_duplicate_detected() -> None: receiver = _build_receiver() first = await receiver.receive(method="POST", url=URL, headers=headers, body=body) - second = await receiver.receive(method="POST", url=URL, headers=headers, body=body) + # A retry is freshly signed (new signature nonce) while retaining the + # payload idempotency key. Reusing the captured signature itself is now + # rejected by the verifier before payload dedup. + second = await receiver.receive(method="POST", url=URL, headers=_sign_webhook(body), body=body) assert first.duplicate is False assert second.duplicate is True diff --git a/tests/conformance/signing/test_webhook_signer.py b/tests/conformance/signing/test_webhook_signer.py index c48b5cce9..d322bc59d 100644 --- a/tests/conformance/signing/test_webhook_signer.py +++ b/tests/conformance/signing/test_webhook_signer.py @@ -21,6 +21,7 @@ ) from adcp.signing.errors import ( WEBHOOK_SIGNATURE_KEY_PURPOSE_INVALID, + WEBHOOK_SIGNATURE_REPLAYED, WEBHOOK_SIGNATURE_REQUIRED, WEBHOOK_SIGNATURE_TAG_INVALID, SignatureVerificationError, @@ -86,11 +87,51 @@ def test_sign_then_verify_roundtrip() -> None: assert result.alg == "ed25519" -def test_rejects_request_signing_key() -> None: - """adcp_use='request-signing' MUST NOT verify as a webhook.""" +def test_default_replay_store_rejects_captured_signature() -> None: + body = b'{"idempotency_key":"whk_replay","task_id":"t1"}' + headers = _sign_and_headers(body) + options = _webhook_verify_options([WEBHOOK_ED25519]) + + verify_webhook_signature( + method="POST", + url="https://buyer.example.com/webhooks/adcp", + headers=headers, + body=body, + options=options, + ) + with pytest.raises(SignatureVerificationError) as exc_info: + verify_webhook_signature( + method="POST", + url="https://buyer.example.com/webhooks/adcp", + headers=headers, + body=body, + options=options, + ) + assert exc_info.value.code == WEBHOOK_SIGNATURE_REPLAYED + + +def test_explicit_none_opts_out_of_signature_replay_check() -> None: + body = b'{"idempotency_key":"whk_external_dedup","task_id":"t1"}' + headers = _sign_and_headers(body) + options = WebhookVerifyOptions( + jwks_resolver=StaticJwksResolver({"keys": [WEBHOOK_ED25519]}), + replay_store=None, + ) + + for _ in range(2): + verify_webhook_signature( + method="POST", + url="https://buyer.example.com/webhooks/adcp", + headers=headers, + body=body, + options=options, + ) + + +def test_accepts_request_signing_key_for_webhook_profile() -> None: + """Current request-signing JWKs MUST verify with the webhook tag.""" body = b'{"idempotency_key":"whk_abc","task_id":"t1"}' private_key = private_key_from_jwk(REQUEST_ED25519, d_field="_private_d_for_test_only") - # Sign with webhook tag but present a request-signing JWK to the verifier. signed = sign_request( method="POST", url="https://buyer.example.com/webhooks/adcp", @@ -100,17 +141,32 @@ def test_rejects_request_signing_key() -> None: key_id=REQUEST_ED25519["kid"], alg="ed25519", cover_content_digest=True, - tag="adcp/webhook-signing/v1", # Malicious sender picks webhook tag + tag="adcp/webhook-signing/v1", ) headers = {"Content-Type": "application/json", **signed.as_dict()} + verified = verify_webhook_signature( + method="POST", + url="https://buyer.example.com/webhooks/adcp", + headers=headers, + body=body, + options=_webhook_verify_options([REQUEST_ED25519]), + ) + assert verified.key_id == REQUEST_ED25519["kid"] + + +def test_rejects_unknown_webhook_key_purpose() -> None: + body = b'{"idempotency_key":"whk_abc","task_id":"t1"}' + unknown_purpose_key = {**WEBHOOK_ED25519, "adcp_use": "governance-signing"} + headers = _sign_and_headers(body, key=unknown_purpose_key) + with pytest.raises(SignatureVerificationError) as exc_info: verify_webhook_signature( method="POST", url="https://buyer.example.com/webhooks/adcp", headers=headers, body=body, - options=_webhook_verify_options([REQUEST_ED25519]), + options=_webhook_verify_options([unknown_purpose_key]), ) assert exc_info.value.code == WEBHOOK_SIGNATURE_KEY_PURPOSE_INVALID diff --git a/tests/test_a2a_server.py b/tests/test_a2a_server.py index a7c5052aa..8510acfd5 100644 --- a/tests/test_a2a_server.py +++ b/tests/test_a2a_server.py @@ -23,6 +23,7 @@ ADCPAgentExecutor as _ADCPAgentExecutor, ) from adcp.server.a2a_server import ( + _A2ARequestContextMiddleware, _build_agent_card, _part_data_dict, create_a2a_server, @@ -642,6 +643,41 @@ def test_create_a2a_server_creates_starlette_app(): assert ( "/.well-known/agent.json" in route_paths ), "0.3 alias /.well-known/agent.json route missing from create_a2a_server" + assert any( + middleware.cls is _A2ARequestContextMiddleware for middleware in app.user_middleware + ), "A2A request-context middleware is not installed" + + +async def test_a2a_context_factory_receives_originating_http_request(): + """A2A matches MCP by exposing request headers/state to factories.""" + import httpx + + observed: list[Any] = [] + + def context_factory(meta: Any) -> ToolContext: + observed.append(meta) + return ToolContext() + + executor = ADCPAgentExecutor(_TestHandler(), context_factory=context_factory) + + async def dispatch(scope: Any, receive: Any, send: Any) -> None: + request = RequestContext( + request=MessageSendParams(message=_make_datapart_msg("get_products")) + ) + executor._build_tool_context("get_products", request) + await send({"type": "http.response.start", "status": 204, "headers": []}) + await send({"type": "http.response.body", "body": b""}) + + app = _A2ARequestContextMiddleware(dispatch) + transport = httpx.ASGITransport(app=app, raise_app_exceptions=True) + async with httpx.AsyncClient(transport=transport, base_url="http://testserver") as client: + response = await client.post("/", headers={"x-tenant-id": "tenant-a"}) + + assert response.status_code == 204 + assert len(observed) == 1 + request_context = observed[0].request_context + assert request_context is not None + assert request_context.headers["x-tenant-id"] == "tenant-a" # --------------------------------------------------------------------------- @@ -1088,6 +1124,32 @@ def test_create_a2a_server_accepts_custom_push_config_store(): ) +@pytest.mark.parametrize( + "public_url", + ["https://agent.example", lambda _request: "https://agent.example"], + ids=["static-card", "per-request-card"], +) +def test_create_a2a_server_accepts_push_sender_on_both_card_paths(public_url: Any): + """The delivery sender reaches both request-handler construction paths.""" + store = _RecordingPushConfigStore() + sender: Any = object() + app = create_a2a_server( + _TestHandler(), + name="test-agent", + push_config_store=store, + push_sender=sender, + public_url=public_url, + ) + handler = _extract_default_request_handler(app) + assert handler._push_sender is sender + + +def test_create_a2a_server_warns_when_push_store_has_no_sender(): + store = _RecordingPushConfigStore() + with pytest.warns(UserWarning, match="will not be delivered"): + create_a2a_server(_TestHandler(), push_config_store=store) + + async def test_sqlite_push_config_store_isolates_scopes_by_contextvar(): """Reference ``SqlitePushNotificationConfigStore`` scopes reads and writes by the ContextVar the seller's auth middleware populates. diff --git a/tests/test_account_mode_gate.py b/tests/test_account_mode_gate.py index b6e7314cc..690ef877c 100644 --- a/tests/test_account_mode_gate.py +++ b/tests/test_account_mode_gate.py @@ -96,7 +96,7 @@ def resolve(_ref: dict[str, Any] | None) -> Account: account_resolver=resolve, ) assert result["success"] is False - assert result["error"] == "PERMISSION_DENIED" + assert result["error"] == "FORBIDDEN" @pytest.mark.asyncio @@ -174,7 +174,7 @@ def resolve(_ref: dict[str, Any] | None) -> Account: account_resolver=resolve, ) assert result["success"] is False - assert result["error"] == "PERMISSION_DENIED" + assert result["error"] == "FORBIDDEN" # --------------------------------------------------------------------------- @@ -246,7 +246,7 @@ def resolve(_ref: dict[str, Any] | None) -> Account: account_resolver=resolve, ) assert result["success"] is False - assert result["error"] == "PERMISSION_DENIED" + assert result["error"] == "FORBIDDEN" # --------------------------------------------------------------------------- diff --git a/tests/test_agent_resolver.py b/tests/test_agent_resolver.py index 10279c6da..764c7a06c 100644 --- a/tests/test_agent_resolver.py +++ b/tests/test_agent_resolver.py @@ -46,6 +46,12 @@ async def handle_async_request(self, request: httpx.Request) -> httpx.Response: if url not in self.responses: return httpx.Response(404, content=b"") spec = self.responses[url] + if "stream" in spec: + return httpx.Response( + spec.get("status", 200), + stream=spec["stream"], + headers=spec.get("headers", {}), + ) return httpx.Response( spec.get("status", 200), content=spec.get("body", b""), @@ -53,6 +59,17 @@ async def handle_async_request(self, request: httpx.Request) -> httpx.Response: ) +class _ChunkedStream(httpx.AsyncByteStream): + def __init__(self, chunks: list[bytes]) -> None: + self.chunks = chunks + self.read = 0 + + async def __aiter__(self): # type: ignore[no-untyped-def] + for chunk in self.chunks: + self.read += 1 + yield chunk + + @pytest.fixture def patch_resolver(monkeypatch: pytest.MonkeyPatch): """Wire a single ``_MockTransport`` into every hop of the resolver. @@ -355,6 +372,28 @@ async def test_resolve_rejects_oversize_capabilities_body(patch_resolver) -> Non assert "exceeds" in exc.value.message +@pytest.mark.asyncio +async def test_resolve_stops_streaming_oversize_capabilities_body(patch_resolver) -> None: + stream = _ChunkedStream([b"xxxx", b"yyyy", b"zzzz"]) + _, factory = patch_resolver( + { + "https://buyer.example.com/mcp": { + "stream": stream, + "headers": {"content-type": "application/json"}, + } + } + ) + with pytest.raises(AgentResolverError) as exc: + await async_resolve_agent( + "https://buyer.example.com/mcp", + agent_type="sales", + max_capabilities_bytes=5, + _capabilities_client_factory=factory, + ) + assert exc.value.code == "capabilities_invalid" + assert stream.read == 2 + + # ---- Sync wrapper ---- diff --git a/tests/test_brand_authz.py b/tests/test_brand_authz.py index 530f8ab84..76b6b90fd 100644 --- a/tests/test_brand_authz.py +++ b/tests/test_brand_authz.py @@ -117,6 +117,36 @@ async def test_authz_etld1_match_authorizes_same_origin_agent() -> None: assert result.matched_agent_type == "signals" +@pytest.mark.asyncio +async def test_authz_stale_on_error_is_bounded() -> None: + url = "https://brand.com/.well-known/brand.json" + body = _brand_json({"agents": [{"type": "signals", "url": "https://ads.brand.com/signals"}]}) + transport = _MockTransport({url: {"body": body}}) + clock = {"t": 0.0} + resolver = BrandJsonAuthorizationResolver( + url, + max_age_seconds=10.0, + max_stale_seconds=20.0, + min_cooldown_seconds=0.0, + clock=lambda: clock["t"], + _client_factory=_factory(transport), + ) + kwargs = { + "agent_url": "https://ads.brand.com/signals", + "brand_domain": "brand.com", + } + assert (await resolver.check(**kwargs)).authorized + + transport.responses[url] = {"status": 503} + clock["t"] = 11.0 + assert (await resolver.check(**kwargs)).authorized + + clock["t"] = 31.0 + result = await resolver.check(**kwargs) + assert result.authorized is False + assert result.reason == "brand_json_unavailable" + + @pytest.mark.asyncio async def test_authz_etld1_match_with_subdomain_brand_url() -> None: body = _brand_json( diff --git a/tests/test_brand_jwks.py b/tests/test_brand_jwks.py index ed8747b70..ccdd9de1c 100644 --- a/tests/test_brand_jwks.py +++ b/tests/test_brand_jwks.py @@ -17,22 +17,50 @@ from __future__ import annotations import asyncio +import gzip import httpx import pytest +from adcp.signing._bounded_http import async_read_limited_bytes from adcp.signing.brand_jwks import ( + DEFAULT_MAX_AGE_SECONDS, + DEFAULT_MAX_STALE_SECONDS, BrandJsonJwksResolver, BrandJsonResolverError, _assert_brand_json_shape, + _BrandJsonFetcher, + _BrandJsonSnapshot, _canonicalize_url, _compute_lifetime, _select_agent, ) +from adcp.signing.jwks import DEFAULT_JWKS_MAX_AGE_SECONDS # ----- Fake HTTP transport — mock httpx.AsyncClient ----- +def test_default_fresh_and_stale_budget_does_not_exceed_revocation_ceiling() -> None: + assert DEFAULT_MAX_AGE_SECONDS + DEFAULT_MAX_STALE_SECONDS <= DEFAULT_JWKS_MAX_AGE_SECONDS + + +def test_default_stale_grace_uses_remaining_revocation_budget() -> None: + clock = {"t": DEFAULT_MAX_AGE_SECONDS + DEFAULT_MAX_STALE_SECONDS - 1} + fetcher = _BrandJsonFetcher( + "https://example.com/.well-known/brand.json", clock=lambda: clock["t"] + ) + snapshot = _BrandJsonSnapshot( + data={}, + final_url="https://example.com/.well-known/brand.json", + fetched_at=0.0, + expires_at=DEFAULT_MAX_AGE_SECONDS, + ) + + assert fetcher.can_serve_stale(snapshot) + clock["t"] += 2 + assert not fetcher.can_serve_stale(snapshot) + + class _MockTransport(httpx.AsyncBaseTransport): """Minimal async transport that returns canned responses keyed on URL. Each call records the request for assertion.""" @@ -47,6 +75,12 @@ async def handle_async_request(self, request: httpx.Request) -> httpx.Response: if url not in self.responses: return httpx.Response(404, content=b"") spec = self.responses[url] + if "stream" in spec: + return httpx.Response( + spec.get("status", 200), + stream=spec["stream"], + headers=spec.get("headers", {}), + ) # Return 304 when the request's If-None-Match matches the spec. if spec.get("etag") is not None and request.headers.get("if-none-match") == spec["etag"]: return httpx.Response( @@ -60,6 +94,17 @@ async def handle_async_request(self, request: httpx.Request) -> httpx.Response: ) +class _ChunkedStream(httpx.AsyncByteStream): + def __init__(self, chunks: list[bytes]) -> None: + self.chunks = chunks + self.read = 0 + + async def __aiter__(self): # type: ignore[no-untyped-def] + for chunk in self.chunks: + self.read += 1 + yield chunk + + @pytest.fixture def patch_httpx(monkeypatch): """Inject a fake-transport ``client_factory`` into every @@ -453,6 +498,49 @@ async def test_resolver_fetches_brand_json_and_inner_jwks(patch_httpx) -> None: assert resolver.agent_url == "https://x.example/" +@pytest.mark.asyncio +async def test_resolver_reselects_when_body_changes_without_etag(patch_httpx) -> None: + url = "https://example.com/.well-known/brand.json" + responses = { + url: { + "body": _brand_json("https://x.example/", "https://x.example/old-jwks"), + "headers": {"content-type": "application/json"}, + } + } + transport = patch_httpx(responses) + clock = {"t": 0.0} + resolver = BrandJsonJwksResolver( + url, + agent_type="brand", + max_age_seconds=10.0, + min_cooldown_seconds=0.0, + clock=lambda: clock["t"], + jwks_fetcher=_jwks_fetcher_for( + { + "https://x.example/old-jwks": { + "kty": "OKP", + "crv": "Ed25519", + "x": "old", + "kid": "k1", + }, + "https://x.example/new-jwks": { + "kty": "OKP", + "crv": "Ed25519", + "x": "new", + "kid": "k1", + }, + } + ), + ) + assert (await resolver("k1"))["x"] == "old" # type: ignore[index] + + transport.responses[url]["body"] = _brand_json( + "https://x.example/", "https://x.example/new-jwks" + ) + clock["t"] = 11.0 + assert (await resolver("k1"))["x"] == "new" # type: ignore[index] + + @pytest.mark.asyncio async def test_resolver_returns_none_for_unknown_kid(patch_httpx) -> None: patch_httpx( @@ -698,6 +786,17 @@ async def test_resolver_satisfies_jwks_resolver_protocol() -> None: # ----- Security regressions from expert review ----- +@pytest.mark.asyncio +async def test_bounded_reader_rejects_compressed_response_before_decompression() -> None: + response = httpx.Response( + 200, + headers={"content-encoding": "gzip"}, + content=gzip.compress(b"A" * 100_000), + ) + with pytest.raises(ValueError, match="encoded HTTP responses"): + await async_read_limited_bytes(response, limit=1024) + + @pytest.mark.asyncio async def test_resolver_rejects_oversized_brand_json(patch_httpx) -> None: """Body cap regression — counterparty serving a large brand.json @@ -727,6 +826,27 @@ async def test_resolver_rejects_oversized_brand_json(patch_httpx) -> None: assert "exceeds" in str(exc.value) +@pytest.mark.asyncio +async def test_resolver_stops_streaming_oversized_brand_json(patch_httpx) -> None: + stream = _ChunkedStream([b"xxxx", b"yyyy", b"zzzz"]) + patch_httpx( + { + "https://example.com/.well-known/brand.json": { + "stream": stream, + "headers": {"content-type": "application/json"}, + } + } + ) + resolver = BrandJsonJwksResolver( + "https://example.com/.well-known/brand.json", + agent_type="brand", + max_body_bytes=5, + ) + with pytest.raises(BrandJsonResolverError, match="exceeds 5 bytes"): + await resolver("k1") + assert stream.read == 2 + + @pytest.mark.asyncio async def test_resolver_loop_detection_handles_case_aliasing(patch_httpx) -> None: """Review finding #2 — without host lowercase + port-strip, diff --git a/tests/test_canonical_reference_resolver.py b/tests/test_canonical_reference_resolver.py index de4514a86..7c9e761ed 100644 --- a/tests/test_canonical_reference_resolver.py +++ b/tests/test_canonical_reference_resolver.py @@ -443,6 +443,39 @@ def test_excessive_ref_count_is_rejected(monkeypatch: pytest.MonkeyPatch) -> Non assert result.message == "format_schema exceeds $ref count bound" +def test_excessive_schema_ids_stop_before_dns_amplification( + monkeypatch: pytest.MonkeyPatch, +) -> None: + dns_calls = 0 + + def counted_getaddrinfo(host: str, port: int, *args: Any, **kwargs: Any) -> list[Any]: + nonlocal dns_calls + del host, args, kwargs + dns_calls += 1 + return [(2, 1, 6, "", ("93.184.216.34", port))] + + monkeypatch.setattr("socket.getaddrinfo", counted_getaddrinfo) + document = { + "$schema": "https://json-schema.org/draft/2020-12/schema", + "allOf": [{"$id": f"child-{index}"} for index in range(100)], + } + body = json.dumps(document).encode() + resolver = CanonicalReferenceResolver( + max_schema_ids=2, + transport_factory=lambda _host, _ip: httpx.MockTransport( + lambda _request: httpx.Response(200, content=body) + ), + ) + + result = resolver.resolve_format_schema(_reference(body)) + + assert result.status is CanonicalReferenceStatus.INVALID_SCHEMA + assert result.message == "format_schema exceeds $id count bound" + # One lookup validates the fetched document URL, then at most the two + # configured $id values are resolved before the walker stops. + assert dns_calls <= 3 + + def test_deep_schema_nesting_returns_structured_invalid_schema( monkeypatch: pytest.MonkeyPatch, ) -> None: diff --git a/tests/test_capabilities_response_shape_validation.py b/tests/test_capabilities_response_shape_validation.py index a971ad7b7..cefb5bba4 100644 --- a/tests/test_capabilities_response_shape_validation.py +++ b/tests/test_capabilities_response_shape_validation.py @@ -396,6 +396,7 @@ async def test_create_adcp_server_validate_at_init_false_works_in_async_context( handler, _executor, _registry = create_adcp_server_from_platform( _ConformantSalesPlatform(), executor=pool, + timed_sync_get_products_limit=1, registry=InMemoryTaskRegistry(), auto_emit_completion_webhooks=False, validate_at_init=False, @@ -427,6 +428,7 @@ async def test_create_adcp_server_default_init_blows_up_in_async_context() -> No create_adcp_server_from_platform( _ConformantSalesPlatform(), executor=pool, + timed_sync_get_products_limit=1, registry=InMemoryTaskRegistry(), auto_emit_completion_webhooks=False, # default validate_at_init=True — the boom case. @@ -441,6 +443,7 @@ def test_create_adcp_server_validate_at_init_true_still_validates_conformant() - handler, _executor, _registry = create_adcp_server_from_platform( _ConformantSalesPlatform(), executor=pool, + timed_sync_get_products_limit=1, registry=InMemoryTaskRegistry(), auto_emit_completion_webhooks=False, # validate_at_init=True is the default @@ -458,6 +461,7 @@ def test_create_adcp_server_validate_at_init_true_rejects_bad_platform() -> None create_adcp_server_from_platform( _MediaBuyMissingBillingPlatform(), executor=pool, + timed_sync_get_products_limit=1, registry=InMemoryTaskRegistry(), auto_emit_completion_webhooks=False, ) diff --git a/tests/test_decisioning_capabilities_projection.py b/tests/test_decisioning_capabilities_projection.py index 391dc5e6b..f2fecd166 100644 --- a/tests/test_decisioning_capabilities_projection.py +++ b/tests/test_decisioning_capabilities_projection.py @@ -657,6 +657,9 @@ def get_adcp_capabilities_for_request(self, params=None, context=None): assert exc_info.value.code == "INTERNAL_ERROR" assert exc_info.value.details["caused_by"]["type"] == "RuntimeError" + assert exc_info.value.details["caused_by"] == {"type": "RuntimeError"} + assert "tenant lookup failed" not in str(exc_info.value) + assert "tenant lookup failed" not in str(exc_info.value.details) def test_request_scoped_capabilities_hook_may_be_async( diff --git a/tests/test_decisioning_dispatch.py b/tests/test_decisioning_dispatch.py index 558419877..b94c09a6b 100644 --- a/tests/test_decisioning_dispatch.py +++ b/tests/test_decisioning_dispatch.py @@ -9,6 +9,7 @@ from __future__ import annotations import asyncio +import threading import warnings from concurrent.futures import ThreadPoolExecutor from contextvars import ContextVar @@ -32,6 +33,8 @@ _coerce_params_to_platform_type, _invoke_platform_method, _project_handoff, + _safe_on_failure_call, + _settle_cancelled_sync_lifecycle, compose_caller_identity, validate_platform, ) @@ -1642,6 +1645,187 @@ async def get_products(self, req: _StrictSubRequest, ctx): assert on_failure_calls[0] is exc_info.value +@pytest.mark.asyncio +async def test_async_cancellation_preserves_failure_hook_and_propagates_unchanged( + executor: ThreadPoolExecutor, +) -> None: + entered = asyncio.Event() + on_failure_calls: list[BaseException] = [] + + class _WaitingPlatform(DecisioningPlatform): + capabilities = DecisioningCapabilities() + accounts = SingletonAccounts(account_id="x") + + async def get_products(self, req: _BaseRequest, ctx): + entered.set() + await asyncio.Event().wait() + + async def _on_failure(exc: BaseException) -> None: + on_failure_calls.append(exc) + + ctx = _build_request_context(ToolContext(), Account(id="x"), None) + task = asyncio.create_task( + _invoke_platform_method( + _WaitingPlatform(), + "get_products", + _BaseRequest(known_field="wait"), + ctx, + executor=executor, + registry=InMemoryTaskRegistry(), + on_failure=_on_failure, + ) + ) + await entered.wait() + task.cancel("client disconnected") + + # Python 3.10 does not preserve Task.cancel(msg) text through shield(). + with pytest.raises(asyncio.CancelledError) as exc_info: + await asyncio.gather(task) + assert exc_info.type is asyncio.CancelledError + # Cancellation does not prove that an async mutation has not crossed an + # external side-effect boundary. Keep framework state fail-closed rather + # than releasing it through the failure hook. + assert on_failure_calls == [] + + +@pytest.mark.asyncio +async def test_sync_cancellation_settles_success_before_on_complete( + executor: ThreadPoolExecutor, +) -> None: + entered = threading.Event() + release = threading.Event() + settled = asyncio.Event() + completed: list[Any] = [] + + class _SyncPlatform(DecisioningPlatform): + capabilities = DecisioningCapabilities() + accounts = SingletonAccounts(account_id="x") + + def get_products(self, req: _BaseRequest, ctx): + entered.set() + release.wait(timeout=2) + return {"products": []} + + async def _on_complete(result: Any) -> None: + completed.append(result) + settled.set() + + ctx = _build_request_context(ToolContext(), Account(id="x"), None) + task = asyncio.create_task( + _invoke_platform_method( + _SyncPlatform(), + "get_products", + _BaseRequest(known_field="wait"), + ctx, + executor=executor, + registry=InMemoryTaskRegistry(), + on_complete=_on_complete, + ) + ) + assert await asyncio.to_thread(entered.wait, 1) + task.cancel("client disconnected") + with pytest.raises(asyncio.CancelledError): + _ = await task + assert completed == [] + + release.set() + await asyncio.wait_for(settled.wait(), 1) + assert completed == [{"products": []}] + + +@pytest.mark.asyncio +async def test_sync_cancellation_settles_real_failure_before_on_failure( + executor: ThreadPoolExecutor, +) -> None: + entered = threading.Event() + release = threading.Event() + settled = asyncio.Event() + failures: list[BaseException] = [] + + class _FailingSyncPlatform(DecisioningPlatform): + capabilities = DecisioningCapabilities() + accounts = SingletonAccounts(account_id="x") + + def get_products(self, req: _BaseRequest, ctx): + entered.set() + release.wait(timeout=2) + raise RuntimeError("worker failed") + + async def _on_failure(exc: BaseException) -> None: + failures.append(exc) + settled.set() + + ctx = _build_request_context(ToolContext(), Account(id="x"), None) + task = asyncio.create_task( + _invoke_platform_method( + _FailingSyncPlatform(), + "get_products", + _BaseRequest(known_field="wait"), + ctx, + executor=executor, + registry=InMemoryTaskRegistry(), + on_failure=_on_failure, + ) + ) + assert await asyncio.to_thread(entered.wait, 1) + task.cancel() + with pytest.raises(asyncio.CancelledError) as exc_info: + await asyncio.gather(task) + assert exc_info.type is asyncio.CancelledError + assert failures == [] + + release.set() + await asyncio.wait_for(settled.wait(), 1) + assert len(failures) == 1 + assert isinstance(failures[0], RuntimeError) + assert str(failures[0]) == "worker failed" + + +@pytest.mark.asyncio +async def test_cancelling_sync_supervisor_does_not_cancel_worker_or_release( + executor: ThreadPoolExecutor, +) -> None: + worker: asyncio.Future[Any] = asyncio.get_running_loop().create_future() + failures: list[BaseException] = [] + + async def _on_failure(exc: BaseException) -> None: + failures.append(exc) + + ctx = _build_request_context(ToolContext(), Account(id="x"), None) + supervisor = asyncio.create_task( + _settle_cancelled_sync_lifecycle( + worker, + ctx=ctx, + method_name="create_media_buy", + registry=InMemoryTaskRegistry(), + executor=executor, + on_complete=None, + on_failure=_on_failure, + pre_handoff_reject=None, + request_params=_BaseRequest(known_field="wait"), + webhook_target=None, + webhook_auto_emit=False, + ) + ) + await asyncio.sleep(0) + supervisor.cancel() + with pytest.raises(asyncio.CancelledError): + _ = await supervisor + + assert worker.cancelled() is False + assert failures == [] + worker.cancel() + + +@pytest.mark.asyncio +async def test_on_failure_hook_cancellation_propagates() -> None: + async def _cancelled_hook(_exc: BaseException) -> None: + raise asyncio.CancelledError + + with pytest.raises(asyncio.CancelledError): + await _safe_on_failure_call(_cancelled_hook, RuntimeError("original"), "get_products") + + def test_coerce_varargs_annotation_is_noop() -> None: """Annotated *args should not trigger coercion — VAR_POSITIONAL guard fires.""" diff --git a/tests/test_decisioning_serve.py b/tests/test_decisioning_serve.py index dfd8a4c1b..d49ae81ff 100644 --- a/tests/test_decisioning_serve.py +++ b/tests/test_decisioning_serve.py @@ -17,10 +17,11 @@ from __future__ import annotations +import importlib import os from concurrent.futures import ThreadPoolExecutor from inspect import signature -from unittest.mock import patch +from unittest.mock import MagicMock, patch import pytest @@ -37,6 +38,9 @@ create_adcp_server_from_platform, serve, ) +from adcp.decisioning.serve import ( + serve as serve_platform, +) class _BarePlatform(DecisioningPlatform): @@ -172,12 +176,71 @@ def test_create_uses_byo_executor_unchanged() -> None: platform = _BarePlatform() custom = ThreadPoolExecutor(max_workers=2, thread_name_prefix="byo-") try: - _, executor, _ = create_adcp_server_from_platform(platform, executor=custom) + _, executor, _ = create_adcp_server_from_platform( + platform, + executor=custom, + timed_sync_get_products_limit=1, + ) assert executor is custom finally: custom.shutdown(wait=True) +def test_create_requires_admission_limit_for_byo_executor() -> None: + custom = ThreadPoolExecutor(max_workers=64) + try: + with pytest.raises(ValueError, match="timed_sync_get_products_limit"): + create_adcp_server_from_platform(_BarePlatform(), executor=custom) + finally: + custom.shutdown(wait=True) + + +def test_create_projects_resolved_admission_limit_to_handler() -> None: + handler, executor, _ = create_adcp_server_from_platform( + _BarePlatform(), + thread_pool_size=4, + ) + try: + assert handler._timed_sync_get_products_admission.limit == 2 # noqa: SLF001 + finally: + executor.shutdown(wait=True) + + +def test_create_projects_explicit_admission_limit_to_handler() -> None: + handler, executor, _ = create_adcp_server_from_platform( + _BarePlatform(), + timed_sync_get_products_limit=1, + ) + try: + assert handler._timed_sync_get_products_admission.limit == 1 # noqa: SLF001 + finally: + executor.shutdown(wait=True) + + +def test_serve_forwards_timed_sync_admission_limit() -> None: + handler = MagicMock() + executor = MagicMock() + registry = MagicMock() + decisioning_serve_module = importlib.import_module("adcp.decisioning.serve") + server_serve_module = importlib.import_module("adcp.server.serve") + with ( + patch.object( + decisioning_serve_module, + "create_adcp_server_from_platform", + return_value=(handler, executor, registry), + ) as create, + patch.object(server_serve_module, "serve") as server_serve, + ): + serve_platform( + _BarePlatform(), + timed_sync_get_products_limit=3, + validate_at_init=False, + ) + + assert create.call_args.kwargs["timed_sync_get_products_limit"] == 3 + server_serve.assert_called_once() + + def test_create_thread_pool_size_overrides_default() -> None: """``thread_pool_size=`` sizes the framework-allocated default executor.""" diff --git a/tests/test_pg_idempotency_backend.py b/tests/test_pg_idempotency_backend.py index 453c6f2e0..8c6089489 100644 --- a/tests/test_pg_idempotency_backend.py +++ b/tests/test_pg_idempotency_backend.py @@ -16,6 +16,8 @@ ``expires_at > now()``). * ``put`` upserts with ``ON CONFLICT DO UPDATE``; serializes response via json.dumps; converts epoch to tz-aware datetime. +* ``hold`` uses a distinct advisory-lock pool so handler SQL cannot deadlock + against the ordinary cache/business pool. * ``delete_expired`` returns the rowcount. """ @@ -72,6 +74,11 @@ async def _connection(): return pool +def _pg_backend(pool: Any, **kwargs: Any) -> PgBackend: + """Build with a distinct lock pool for tests that do not exercise hold().""" + return PgBackend(pool=pool, lock_pool=MagicMock(), **kwargs) + + # --------------------------------------------------------------------------- # Construction # --------------------------------------------------------------------------- @@ -80,27 +87,32 @@ async def _connection(): class TestConstruction: def test_default_table_name(self) -> None: pool = MagicMock() - backend = PgBackend(pool=pool) + backend = _pg_backend(pool) assert backend._table == DEFAULT_IDEMPOTENCY_TABLE def test_custom_table_name_accepted(self) -> None: pool = MagicMock() - backend = PgBackend(pool=pool, table_name="my_idem_cache") + backend = _pg_backend(pool, table_name="my_idem_cache") assert backend._table == "my_idem_cache" def test_invalid_identifier_rejected(self) -> None: pool = MagicMock() with pytest.raises(ValueError, match="Table name must match"): - PgBackend(pool=pool, table_name="bad-name") + _pg_backend(pool, table_name="bad-name") def test_uppercase_identifier_rejected(self) -> None: with pytest.raises(ValueError, match="Table name must match"): - PgBackend(pool=MagicMock(), table_name="MyTable") + _pg_backend(MagicMock(), table_name="MyTable") def test_satisfies_idempotency_backend_protocol(self) -> None: - backend = PgBackend(pool=MagicMock()) + backend = _pg_backend(MagicMock()) assert isinstance(backend, IdempotencyBackend) + def test_rejects_shared_lock_pool(self) -> None: + pool = MagicMock() + with pytest.raises(ValueError, match="lock_pool must be distinct"): + PgBackend(pool=pool, lock_pool=pool) + # --------------------------------------------------------------------------- # create_schema @@ -113,7 +125,7 @@ async def test_create_schema_executes_create_table_and_index() -> None: separate ``execute()`` call (psycopg does not split on ``;``).""" conn = _make_conn(_cursor(), _cursor()) pool = _make_pool(conn) - backend = PgBackend(pool=pool, table_name="adcp_idempotency") + backend = _pg_backend(pool, table_name="adcp_idempotency") await backend.create_schema() @@ -130,7 +142,7 @@ async def test_create_schema_executes_create_table_and_index() -> None: async def test_create_schema_uses_custom_table_name() -> None: conn = _make_conn(_cursor(), _cursor()) pool = _make_pool(conn) - backend = PgBackend(pool=pool, table_name="alt_idem") + backend = _pg_backend(pool, table_name="alt_idem") await backend.create_schema() assert "CREATE TABLE IF NOT EXISTS alt_idem" in conn.execute.call_args_list[0].args[0] @@ -148,7 +160,7 @@ async def test_create_schema_uses_custom_table_name() -> None: async def test_get_returns_none_on_miss() -> None: conn = _make_conn(_cursor(fetchone_value=None)) pool = _make_pool(conn) - backend = PgBackend(pool=pool) + backend = _pg_backend(pool) assert await backend.get("scope-x", "key-y") is None sql = conn.execute.call_args.args[0] @@ -161,7 +173,7 @@ async def test_get_parses_dict_response() -> None: expires = datetime(2030, 1, 1, tzinfo=timezone.utc) conn = _make_conn(_cursor(fetchone_value=("hash-1", {"k": "v"}, expires))) pool = _make_pool(conn) - backend = PgBackend(pool=pool) + backend = _pg_backend(pool) cached = await backend.get("scope-a", "key-1") assert cached is not None @@ -176,7 +188,7 @@ async def test_get_parses_json_string_response() -> None: expires = datetime(2030, 1, 1, tzinfo=timezone.utc) conn = _make_conn(_cursor(fetchone_value=("hash-1", '{"k":"v"}', expires))) pool = _make_pool(conn) - backend = PgBackend(pool=pool) + backend = _pg_backend(pool) cached = await backend.get("scope-a", "key-1") assert cached is not None @@ -191,7 +203,7 @@ async def test_get_raises_on_naive_timestamp() -> None: naive = datetime(2030, 1, 1) conn = _make_conn(_cursor(fetchone_value=("hash", {}, naive))) pool = _make_pool(conn) - backend = PgBackend(pool=pool) + backend = _pg_backend(pool) with pytest.raises(ValueError, match="naive datetime"): await backend.get("scope", "key") @@ -206,7 +218,7 @@ async def test_get_raises_on_naive_timestamp() -> None: async def test_put_upserts_with_on_conflict() -> None: conn = _make_conn(_cursor()) pool = _make_pool(conn) - backend = PgBackend(pool=pool) + backend = _pg_backend(pool) entry = CachedResponse( payload_hash="hash-1", @@ -231,6 +243,43 @@ async def test_put_upserts_with_on_conflict() -> None: assert expires_at.tzinfo is not None # tz-aware +@pytest.mark.asyncio +async def test_put_if_absent_reports_atomic_insert_result() -> None: + conn = _make_conn(_cursor(fetchone_value=(1,))) + backend = _pg_backend(_make_pool(conn)) + entry = CachedResponse("hash", {}, time.time() + 3600) + + assert await backend.put_if_absent("scope", "key", entry) is True + sql = conn.execute.call_args.args[0] + assert "ON CONFLICT (scope_key, key) DO UPDATE" in sql + assert "RETURNING 1" in sql + + +@pytest.mark.asyncio +async def test_hold_reuses_locked_connection_for_get_and_put() -> None: + expires = datetime(2030, 1, 1, tzinfo=timezone.utc) + conn = _make_conn( + _cursor(), # advisory lock + _cursor(fetchone_value=None), # get + _cursor(), # put + ) + + @asynccontextmanager + async def transaction(): + yield + + conn.transaction = transaction + lock_pool = _make_pool(conn) + backend = PgBackend(pool=MagicMock(), lock_pool=lock_pool) + + async with backend.hold("scope", "key"): + assert await backend.get("scope", "key") is None + await backend.put("scope", "key", CachedResponse("hash", {}, expires.timestamp())) + + assert conn.execute.call_count == 3 + assert "pg_advisory_xact_lock" in conn.execute.call_args_list[0].args[0] + + # --------------------------------------------------------------------------- # delete_expired # --------------------------------------------------------------------------- @@ -240,7 +289,7 @@ async def test_put_upserts_with_on_conflict() -> None: async def test_delete_expired_uses_supplied_cutoff() -> None: conn = _make_conn(_cursor(rowcount=7)) pool = _make_pool(conn) - backend = PgBackend(pool=pool) + backend = _pg_backend(pool) cutoff_epoch = 1_000_000_000.0 deleted = await backend.delete_expired(cutoff_epoch) @@ -258,7 +307,7 @@ async def test_delete_expired_uses_supplied_cutoff() -> None: async def test_delete_expired_defaults_to_wall_clock() -> None: conn = _make_conn(_cursor(rowcount=0)) pool = _make_pool(conn) - backend = PgBackend(pool=pool) + backend = _pg_backend(pool) before = time.time() deleted = await backend.delete_expired() @@ -276,6 +325,6 @@ async def test_delete_expired_returns_zero_on_no_rowcount() -> None: backend coerces to 0.""" conn = _make_conn(_cursor(rowcount=None)) # type: ignore[arg-type] pool = _make_pool(conn) - backend = PgBackend(pool=pool) + backend = _pg_backend(pool) assert await backend.delete_expired() == 0 diff --git a/tests/test_proposal_lifecycle_e2e.py b/tests/test_proposal_lifecycle_e2e.py index 70f89fad6..7a50e92e6 100644 --- a/tests/test_proposal_lifecycle_e2e.py +++ b/tests/test_proposal_lifecycle_e2e.py @@ -31,6 +31,7 @@ import asyncio import sys +import threading from concurrent.futures import ThreadPoolExecutor from datetime import datetime, timedelta, timezone from pathlib import Path @@ -291,6 +292,69 @@ async def test_finalize_commits_proposal( assert record.expires_at is not None +@pytest.mark.asyncio +async def test_cancelled_sync_finalize_commits_after_worker_finishes( + router: Any, + store: InMemoryProposalStore, + executor: ThreadPoolExecutor, +) -> None: + """Cancellation cannot leave a completed sync finalize as a draft.""" + from adcp.types import GetProductsRequest + + handler = PlatformHandler( + router, + executor=executor, + registry=InMemoryTaskRegistry(), + ) + await handler.get_products( + GetProductsRequest(buying_mode="brief", brief="initial"), + ToolContext(), + ) + manager = router.proposal_manager_for_tenant("default") + original = manager.finalize_proposal + entered = threading.Event() + release = threading.Event() + + def _blocking_finalize(req: Any, ctx: Any) -> Any: + entered.set() + release.wait(timeout=2) + return asyncio.run(original(req, ctx)) + + manager.finalize_proposal = _blocking_finalize # type: ignore[method-assign] + finalize_req = GetProductsRequest.model_validate( + { + "buying_mode": "refine", + "refine": [ + { + "scope": "proposal", + "proposal_id": PROPOSAL_ID, + "action": "finalize", + } + ], + } + ) + try: + task = asyncio.create_task(handler.get_products(finalize_req, ToolContext())) + assert await asyncio.to_thread(entered.wait, 1) + task.cancel("buyer disconnected") + with pytest.raises(asyncio.CancelledError): + _ = await task + + draft = await store.get(PROPOSAL_ID, expected_account_id="acct_demo") + assert draft is not None and draft.state == ProposalState.DRAFT + release.set() + for _ in range(100): + committed = await store.get(PROPOSAL_ID, expected_account_id="acct_demo") + if committed is not None and committed.state == ProposalState.COMMITTED: + break + await asyncio.sleep(0.01) + else: + pytest.fail("cancelled sync finalize did not commit after worker completion") + finally: + release.set() + manager.finalize_proposal = original # type: ignore[method-assign] + + @pytest.mark.asyncio async def test_finalize_unknown_proposal_is_correctable( handler: PlatformHandler, @@ -1398,6 +1462,98 @@ async def _seed_committed_proposal(handler: PlatformHandler) -> None: ) +@pytest.mark.asyncio +async def test_create_media_buy_cancellation_keeps_reservation_fail_closed( + executor: ThreadPoolExecutor, + registry: InMemoryTaskRegistry, +) -> None: + from examples.sales_proposal_mode_seller.src.app import build_router + + router = build_router() + store = router.proposal_store_for_tenant("default") + handler = _build_handler(router, executor, registry) + await _seed_committed_proposal(handler) + + entered = asyncio.Event() + target_platform = router._platforms["default"] # noqa: SLF001 + original = target_platform.create_media_buy + + async def _waiting_create(req: Any, ctx: Any) -> Any: + del req, ctx + entered.set() + await asyncio.Event().wait() + + target_platform.create_media_buy = _waiting_create # type: ignore[method-assign] + try: + task = asyncio.create_task( + handler.create_media_buy(_build_create_media_buy_request("cancel"), ToolContext()) + ) + await entered.wait() + + reserved = await store.get(PROPOSAL_ID, expected_account_id="acct_demo") + assert reserved is not None and reserved.state == ProposalState.CONSUMING + + task.cancel("buyer disconnected") + with pytest.raises(asyncio.CancelledError) as exc_info: + await asyncio.gather(task) + assert exc_info.type is asyncio.CancelledError + + retained = await store.get(PROPOSAL_ID, expected_account_id="acct_demo") + assert retained is not None and retained.state == ProposalState.CONSUMING + finally: + target_platform.create_media_buy = original # type: ignore[method-assign] + + +@pytest.mark.asyncio +async def test_sync_create_media_buy_cancellation_waits_for_worker_success( + executor: ThreadPoolExecutor, + registry: InMemoryTaskRegistry, +) -> None: + """A cancelled request cannot release a reservation while its thread runs.""" + router = build_router() + store = router.proposal_store_for_tenant("default") + handler = _build_handler(router, executor, registry) + await _seed_committed_proposal(handler) + + entered = threading.Event() + release = threading.Event() + target_platform = router._platforms["default"] # noqa: SLF001 + original = target_platform.create_media_buy + + def _blocking_create(req: Any, ctx: Any) -> Any: + del req, ctx + entered.set() + release.wait(timeout=2) + return {"media_buy_id": "mb_sync_cancel", "status": "active"} + + target_platform.create_media_buy = _blocking_create # type: ignore[method-assign] + try: + task = asyncio.create_task( + handler.create_media_buy(_build_create_media_buy_request("sync-cancel"), ToolContext()) + ) + assert await asyncio.to_thread(entered.wait, 1) + task.cancel("buyer disconnected") + with pytest.raises(asyncio.CancelledError) as exc_info: + await asyncio.gather(task) + assert exc_info.type is asyncio.CancelledError + + reserved = await store.get(PROPOSAL_ID, expected_account_id="acct_demo") + assert reserved is not None and reserved.state == ProposalState.CONSUMING + + release.set() + for _ in range(100): + consumed = await store.get(PROPOSAL_ID, expected_account_id="acct_demo") + if consumed is not None and consumed.state == ProposalState.CONSUMED: + break + await asyncio.sleep(0.01) + else: + pytest.fail("sync worker success did not finalize proposal reservation") + assert consumed.media_buy_id == "mb_sync_cancel" + finally: + release.set() + target_platform.create_media_buy = original # type: ignore[method-assign] + + @pytest.mark.asyncio async def test_create_media_buy_handoff_finalizes_consumption_on_completion( executor: ThreadPoolExecutor, diff --git a/tests/test_release_configuration.py b/tests/test_release_configuration.py index 3898462a0..f503f82ff 100644 --- a/tests/test_release_configuration.py +++ b/tests/test_release_configuration.py @@ -1,4 +1,4 @@ -"""Release automation is the single source of truth for the v7 RC version.""" +"""Release automation is the single source of truth for package versions.""" from __future__ import annotations @@ -23,9 +23,9 @@ def test_worktree_version_matches_normalized_release_manifest() -> None: assert project_section.group(1) == pep440_prerelease(manifest["."]) -def test_release_please_targets_v7_rc_from_breaking_commit() -> None: +def test_release_please_targets_stable_versions() -> None: config = json.loads((ROOT / "release-please-config.json").read_text()) package = config["packages"]["."] - assert package["versioning"] == "prerelease" - assert package["prerelease-type"] == "rc" - assert package["prerelease"] is True + assert "versioning" not in package + assert "prerelease-type" not in package + assert "prerelease" not in package diff --git a/tests/test_serve_config.py b/tests/test_serve_config.py index 8710dbb69..13ab9e24f 100644 --- a/tests/test_serve_config.py +++ b/tests/test_serve_config.py @@ -191,6 +191,19 @@ def test_serve_config_max_active_sessions_propagates_to_both_transport() -> None assert kwargs.get("max_active_sessions") == 10 +def test_serve_config_push_sender_propagates_to_a2a_transport() -> None: + handler = _StubHandler() + sender = MagicMock() + cfg = ServeConfig(transport="a2a", push_sender=sender) + + with patch.object(_serve_mod, "_serve_a2a") as mock_a2a: + _serve_mod.serve(handler, config=cfg) + + mock_a2a.assert_called_once() + _, kwargs = mock_a2a.call_args + assert kwargs.get("push_sender") is sender + + def test_serve_config_session_count_source_wires_debug_middleware() -> None: handler = _StubHandler() source = lambda: {"active_sessions": 0} # noqa: E731 diff --git a/tests/test_server_idempotency.py b/tests/test_server_idempotency.py index f288cea14..b7791f712 100644 --- a/tests/test_server_idempotency.py +++ b/tests/test_server_idempotency.py @@ -5,6 +5,8 @@ import asyncio import time import uuid +from collections.abc import AsyncIterator +from contextlib import asynccontextmanager from typing import Any import pytest @@ -407,7 +409,7 @@ def test_construction_without_pg_extra_raises_import_error(self) -> None: with patch("adcp.server.idempotency.backends._PG_AVAILABLE", False): with pytest.raises(ImportError, match="adcp\\[pg\\]"): - PgBackend(pool=MagicMock()) + PgBackend(pool=MagicMock(), lock_pool=MagicMock()) class TestScopeKeySeparatorValidation: @@ -484,6 +486,32 @@ class TestIdempotencyStoreWrap: def _make_store(self, ttl_seconds: int = 86400) -> IdempotencyStore: return IdempotencyStore(backend=MemoryBackend(), ttl_seconds=ttl_seconds) + @pytest.mark.asyncio + async def test_cache_hit_does_not_acquire_execution_hold(self) -> None: + class TrackingBackend(MemoryBackend): + def __init__(self) -> None: + super().__init__() + self.holds = 0 + + @asynccontextmanager + async def hold(self, scope_key: str, key: str) -> AsyncIterator[None]: + self.holds += 1 + async with super().hold(scope_key, key): + yield + + backend = TrackingBackend() + store = IdempotencyStore(backend) + handler = _FakeHandler() + wrapped = store.wrap(_FakeHandler.create_media_buy) + params = {"idempotency_key": str(uuid.uuid4()), "brand": "A"} + ctx = ToolContext(caller_identity="principal-a") + + await wrapped(handler, params, ctx) + replay = await wrapped(handler, params, ctx) + + assert replay["replayed"] is True + assert backend.holds == 1 + @pytest.mark.asyncio async def test_cache_miss_runs_handler_and_caches(self) -> None: store = self._make_store() @@ -518,6 +546,124 @@ async def test_cache_hit_replays_without_handler_call(self) -> None: # Everything else about the response is identical. assert {k: v for k, v in r2.items() if k != "replayed"} == r1 + @pytest.mark.asyncio + async def test_concurrent_same_key_executes_handler_once(self) -> None: + store = self._make_store() + entered = asyncio.Event() + release = asyncio.Event() + calls = 0 + + async def handler( + _self: object, params: dict[str, Any], context: ToolContext | None = None + ) -> dict[str, Any]: + nonlocal calls + calls += 1 + entered.set() + await release.wait() + return {"media_buy_id": "mb_only", "status": "completed"} + + wrapped = store.wrap(handler) + params = {"idempotency_key": str(uuid.uuid4()), "brand": "A"} + ctx = ToolContext(caller_identity="principal-a") + first = asyncio.create_task(wrapped(object(), params, ctx)) + await entered.wait() + second = asyncio.create_task(wrapped(object(), params, ctx)) + await asyncio.sleep(0) + assert calls == 1 + + release.set() + first_result, second_result = await asyncio.gather(first, second) + assert first_result.get("replayed") is not True + assert second_result["replayed"] is True + + @pytest.mark.asyncio + async def test_cancelled_request_keeps_lock_until_sync_work_is_cached(self) -> None: + store = self._make_store() + entered = asyncio.Event() + release = asyncio.Event() + calls = 0 + + async def handler( + _self: object, params: dict[str, Any], context: ToolContext | None = None + ) -> dict[str, Any]: + nonlocal calls + calls += 1 + entered.set() + await release.wait() + return {"media_buy_id": "mb_only", "status": "completed"} + + wrapped = store.wrap(handler) + params = {"idempotency_key": str(uuid.uuid4()), "brand": "A"} + ctx = ToolContext(caller_identity="principal-a") + first = asyncio.create_task(wrapped(object(), params, ctx)) + await entered.wait() + first.cancel("client disconnected") + # Python 3.10 does not preserve Task.cancel(msg) text through shield(). + with pytest.raises(asyncio.CancelledError): + _ = await first + + retry = asyncio.create_task(wrapped(object(), params, ctx)) + await asyncio.sleep(0) + assert calls == 1 + release.set() + retry_result = await retry + assert retry_result["replayed"] is True + + @pytest.mark.asyncio + async def test_legacy_backend_uses_deprecated_process_local_hold(self) -> None: + class LegacyBackend(IdempotencyBackend): + def __init__(self) -> None: + self.entry: CachedResponse | None = None + + async def get(self, scope_key: str, key: str) -> CachedResponse | None: + return self.entry + + async def put(self, scope_key: str, key: str, entry: CachedResponse) -> None: + self.entry = entry + + async def delete_expired(self, now_epoch: float | None = None) -> int: + return 0 + + store = IdempotencyStore(LegacyBackend()) + handler = _FakeHandler() + wrapped = store.wrap(_FakeHandler.create_media_buy) + params = {"idempotency_key": str(uuid.uuid4()), "brand": "A"} + ctx = ToolContext(caller_identity="principal-a") + + with pytest.warns(DeprecationWarning, match="process-local idempotency locking"): + await wrapped(handler, params, ctx) + replay = await wrapped(handler, params, ctx) + assert replay["replayed"] is True + assert handler.call_count == 1 + + @pytest.mark.asyncio + async def test_lazy_wrapped_legacy_backend_uses_fallback_hold(self) -> None: + class LegacyBackend(IdempotencyBackend): + def __init__(self) -> None: + self.entry: CachedResponse | None = None + + async def get(self, scope_key: str, key: str) -> CachedResponse | None: + return self.entry + + async def put(self, scope_key: str, key: str, entry: CachedResponse) -> None: + self.entry = entry + + async def delete_expired(self, now_epoch: float | None = None) -> int: + return 0 + + store = IdempotencyStore(LazyBackend(LegacyBackend)) + handler = _FakeHandler() + wrapped = store.wrap(_FakeHandler.create_media_buy) + params = {"idempotency_key": str(uuid.uuid4()), "brand": "A"} + ctx = ToolContext(caller_identity="principal-a") + + with pytest.warns(DeprecationWarning, match="process-local idempotency locking"): + await wrapped(handler, params, ctx) + replay = await wrapped(handler, params, ctx) + + assert replay["replayed"] is True + assert handler.call_count == 1 + @pytest.mark.asyncio async def test_replay_flag_does_not_poison_cached_entry(self) -> None: """The cached ``CachedResponse.response`` MUST stay clean — the @@ -585,8 +731,9 @@ async def test_cache_hit_different_payload_raises_conflict(self) -> None: key = str(uuid.uuid4()) ctx = ToolContext(caller_identity="principal-a") await wrapped(handler, {"idempotency_key": key, "brand": "A"}, ctx) - with pytest.raises(IdempotencyConflictError): + with pytest.raises(IdempotencyConflictError) as exc: await wrapped(handler, {"idempotency_key": key, "brand": "B"}, ctx) + assert exc.value.operation == "create_media_buy" assert handler.call_count == 1 # conflict path does NOT run handler again @pytest.mark.asyncio @@ -1133,6 +1280,29 @@ async def create_media_buy(self, params: Any, context: Any = None) -> Any: class TestBackendPutFailure: + @pytest.mark.asyncio + async def test_normal_handler_failure_does_not_log_cancelled_request_message( + self, caplog: Any + ) -> None: + import logging as _logging + + async def handler( + _self: object, params: dict[str, Any], context: ToolContext | None = None + ) -> dict[str, Any]: + raise RuntimeError("expected handler failure") + + wrapped = IdempotencyStore(MemoryBackend()).wrap(handler) + ctx = ToolContext(caller_identity="principal-a") + with caplog.at_level(_logging.ERROR, logger="adcp.server.idempotency.store"): + with pytest.raises(RuntimeError, match="expected handler failure"): + await wrapped( + object(), + {"idempotency_key": str(uuid.uuid4()), "brand": "A"}, + ctx, + ) + + assert not any("request was cancelled" in record.message for record in caplog.records) + @pytest.mark.asyncio async def test_put_failure_logs_warning_and_returns_handler_result(self, caplog: Any) -> None: import logging as _logging diff --git a/tests/test_time_budget.py b/tests/test_time_budget.py index d8b08ebd4..7c695eb38 100644 --- a/tests/test_time_budget.py +++ b/tests/test_time_budget.py @@ -14,8 +14,8 @@ from __future__ import annotations import asyncio +import threading from concurrent.futures import ThreadPoolExecutor -from unittest.mock import AsyncMock import pytest @@ -24,18 +24,20 @@ DecisioningPlatform, IncrementalGetProducts, InMemoryTaskRegistry, + LazyPlatformRouter, + PlatformRouter, ProductsCheckpoint, SingletonAccounts, ) from adcp.decisioning.handler import PlatformHandler from adcp.decisioning.time_budget import ( + SyncExecutorAdmission, project_incomplete_response, resolve_time_budget, ) from adcp.server.base import ToolContext from adcp.types import GetProductsRequest - # --------------------------------------------------------------------------- # resolve_time_budget # --------------------------------------------------------------------------- @@ -113,6 +115,18 @@ def test_project_incomplete_response_contains_budget_info(): assert "minutes" in description +@pytest.mark.parametrize("limit", [0, -1, True]) +def test_sync_executor_admission_rejects_invalid_limits(limit) -> None: + with pytest.raises(ValueError, match="positive integer"): + SyncExecutorAdmission(limit) + + +def test_sync_executor_admission_release_is_bounded() -> None: + admission = SyncExecutorAdmission(1) + with pytest.raises(ValueError, match="released too many times"): + admission.release() + + # --------------------------------------------------------------------------- # Fixtures # --------------------------------------------------------------------------- @@ -204,12 +218,20 @@ async def get_products(self, req, ctx): ) result = await handler.get_products(req, context=ToolContext()) - products = result.get("products") if isinstance(result, dict) else list(getattr(result, "products", [])) # type: ignore[union-attr] + products = ( + result.get("products") + if isinstance(result, dict) + else list(getattr(result, "products", [])) + ) # type: ignore[union-attr] assert len(products) == 1 pid = products[0].get("product_id") if isinstance(products[0], dict) else products[0].product_id # type: ignore[union-attr] assert pid == "p1" # No incomplete key / field when fully resolved - incomplete = result.get("incomplete") if isinstance(result, dict) else getattr(result, "incomplete", None) + incomplete = ( + result.get("incomplete") + if isinstance(result, dict) + else getattr(result, "incomplete", None) + ) assert not incomplete @@ -232,7 +254,11 @@ async def get_products(self, req, ctx): ) req = GetProductsRequest.model_construct(account=None, time_budget=None) result = await handler.get_products(req, context=ToolContext()) - products = result.get("products") if isinstance(result, dict) else list(getattr(result, "products", [])) # type: ignore[union-attr] + products = ( + result.get("products") + if isinstance(result, dict) + else list(getattr(result, "products", [])) + ) # type: ignore[union-attr] assert len(products) == 1 @@ -258,10 +284,237 @@ async def get_products(self, req, ctx): time_budget=_make_time_budget(interval=1, unit="campaign"), ) result = await handler.get_products(req, context=ToolContext()) - products = result.get("products") if isinstance(result, dict) else list(getattr(result, "products", [])) # type: ignore[union-attr] + products = ( + result.get("products") + if isinstance(result, dict) + else list(getattr(result, "products", [])) + ) # type: ignore[union-attr] assert len(products) == 1 +@pytest.mark.asyncio +async def test_sync_timeout_admission_saturates_without_executor_queue_growth( + executor: ThreadPoolExecutor, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Timed-out threads retain permits; later short-budget calls are not submitted.""" + release = threading.Event() + two_started = threading.Event() + calls = 0 + calls_lock = threading.Lock() + deadline = [0.05] + + class _BlockingSyncSeller(DecisioningPlatform): + capabilities = DecisioningCapabilities(specialisms=["sales-non-guaranteed"]) + accounts = SingletonAccounts(account_id="test") + + def get_products(self, req, ctx): + nonlocal calls + with calls_lock: + calls += 1 + if calls == 2: + two_started.set() + release.wait(timeout=2.0) + return {"products": [{"product_id": f"p{calls}", "name": "Recovered"}]} + + monkeypatch.setattr("adcp.decisioning.handler.resolve_time_budget", lambda _value: deadline[0]) + handler = PlatformHandler( + _BlockingSyncSeller(), + executor=executor, + registry=InMemoryTaskRegistry(), + timed_sync_get_products_limit=2, + ) + req = GetProductsRequest.model_construct( + account=None, + time_budget=_make_time_budget(interval=1, unit="seconds"), + ) + + first_two = [ + asyncio.create_task(handler.get_products(req, context=ToolContext())) for _ in range(2) + ] + assert await asyncio.to_thread(two_started.wait, 1.0) + timed_out = await asyncio.gather(*first_two) + assert all(getattr(result, "incomplete", None) for result in timed_out) + + # Both permits remain attached to the still-running worker threads. This + # request exhausts its budget waiting and never reaches executor.submit. + saturated = await handler.get_products(req, context=ToolContext()) + assert getattr(saturated, "incomplete", None) + assert calls == 2 + + # Once real worker completion callbacks return the permits, admission + # recovers and a later call executes normally. + release.set() + deadline[0] = 0.5 + recovered = await handler.get_products(req, context=ToolContext()) + products = ( + recovered.get("products", []) + if isinstance(recovered, dict) + else list(getattr(recovered, "products", [])) + ) + assert len(products) == 1 + assert calls == 3 + + +@pytest.mark.asyncio +@pytest.mark.parametrize("router_kind", ["eager", "lazy"]) +async def test_router_sync_timeout_uses_bounded_admission( + router_kind: str, + executor: ThreadPoolExecutor, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Eager and lazy async routers must not bypass sync-child admission.""" + release = threading.Event() + started = threading.Event() + calls = 0 + deadline = [0.05] + + class _BlockingChild(DecisioningPlatform): + capabilities = DecisioningCapabilities(specialisms=["sales-non-guaranteed"]) + accounts = SingletonAccounts(account_id="child") + + def get_products(self, req, ctx): + nonlocal calls + calls += 1 + started.set() + release.wait(timeout=2.0) + return {"products": [{"product_id": f"p{calls}", "name": "Recovered"}]} + + accounts = SingletonAccounts( + account_id="router", + metadata_factory=lambda: {"tenant_id": "tenant-a"}, + ) + capabilities = DecisioningCapabilities(specialisms=["sales-non-guaranteed"]) + if router_kind == "eager": + platform: DecisioningPlatform = PlatformRouter( + accounts=accounts, + platforms={"tenant-a": _BlockingChild()}, + capabilities=capabilities, + ) + else: + platform = LazyPlatformRouter( + accounts=accounts, + factory=lambda _tenant_id: _BlockingChild(), + capabilities=capabilities, + ) + + monkeypatch.setattr("adcp.decisioning.handler.resolve_time_budget", lambda _value: deadline[0]) + handler = PlatformHandler( + platform, + executor=executor, + registry=InMemoryTaskRegistry(), + timed_sync_get_products_limit=1, + ) + req = GetProductsRequest.model_construct( + account=None, + time_budget=_make_time_budget(interval=1, unit="seconds"), + ) + + first = asyncio.create_task(handler.get_products(req, context=ToolContext())) + assert await asyncio.to_thread(started.wait, 1.0) + assert getattr(await first, "incomplete", None) + + # The first timed-out child still owns the sole permit, so this request + # times out waiting for admission and is never submitted. + assert getattr(await handler.get_products(req, context=ToolContext()), "incomplete", None) + assert calls == 1 + + release.set() + deadline[0] = 0.5 + recovered = await handler.get_products(req, context=ToolContext()) + products = ( + recovered.get("products", []) + if isinstance(recovered, dict) + else list(getattr(recovered, "products", [])) + ) + assert len(products) == 1 + assert calls == 2 + + +@pytest.mark.asyncio +async def test_router_sync_without_deadline_uses_configured_executor() -> None: + observed_threads: list[str] = [] + + class _SyncChild(DecisioningPlatform): + capabilities = DecisioningCapabilities(specialisms=["sales-non-guaranteed"]) + accounts = SingletonAccounts(account_id="child") + + def get_products(self, req, ctx): + observed_threads.append(threading.current_thread().name) + return {"products": []} + + router = PlatformRouter( + accounts=SingletonAccounts( + account_id="router", + metadata_factory=lambda: {"tenant_id": "tenant-a"}, + ), + platforms={"tenant-a": _SyncChild()}, + capabilities=DecisioningCapabilities(specialisms=["sales-non-guaranteed"]), + ) + with ThreadPoolExecutor(max_workers=2, thread_name_prefix="framework-router-") as pool: + handler = PlatformHandler( + router, + executor=pool, + registry=InMemoryTaskRegistry(), + ) + req = GetProductsRequest.model_construct(account=None, time_budget=None) + await handler.get_products(req, context=ToolContext()) + + assert len(observed_threads) == 1 + assert observed_threads[0].startswith("framework-router-") + + +@pytest.mark.asyncio +async def test_sync_campaign_requests_bypass_deadline_admission() -> None: + """Campaign-unit semantics remain unlimited by the deadline-only gate.""" + release = threading.Event() + two_started = threading.Event() + calls = 0 + calls_lock = threading.Lock() + + class _CampaignSyncSeller(DecisioningPlatform): + capabilities = DecisioningCapabilities(specialisms=["sales-non-guaranteed"]) + accounts = SingletonAccounts(account_id="test") + + def get_products(self, req, ctx): + nonlocal calls + with calls_lock: + calls += 1 + if calls == 2: + two_started.set() + release.wait(timeout=2.0) + return {"products": [{"product_id": "campaign", "name": "Campaign"}]} + + with ThreadPoolExecutor(max_workers=2) as campaign_executor: + handler = PlatformHandler( + _CampaignSyncSeller(), + executor=campaign_executor, + registry=InMemoryTaskRegistry(), + timed_sync_get_products_limit=1, + ) + req = GetProductsRequest.model_construct( + account=None, + time_budget=_make_time_budget(interval=1, unit="campaign"), + ) + tasks = [ + asyncio.create_task(handler.get_products(req, context=ToolContext())) for _ in range(2) + ] + assert await asyncio.to_thread(two_started.wait, 1.0) + release.set() + results = await asyncio.gather(*tasks) + + assert calls == 2 + assert all( + len( + result.get("products", []) + if isinstance(result, dict) + else list(getattr(result, "products", [])) + ) + == 1 + for result in results + ) + + @pytest.mark.asyncio async def test_get_products_timeout_logs_warning(executor, caplog): """A timeout emits a WARNING with budget info.""" @@ -287,15 +540,15 @@ async def test_get_products_timeout_logs_warning(executor, caplog): def test_incremental_get_products_importable_from_decisioning(): - from adcp.decisioning import IncrementalGetProducts as IGP # noqa: F401 + from adcp.decisioning import IncrementalGetProducts as ImportedIncrementalGetProducts - assert IGP is IncrementalGetProducts + assert ImportedIncrementalGetProducts is IncrementalGetProducts def test_products_checkpoint_importable_from_decisioning(): - from adcp.decisioning import ProductsCheckpoint as PC # noqa: F401 + from adcp.decisioning import ProductsCheckpoint as ImportedProductsCheckpoint - assert PC is ProductsCheckpoint + assert ImportedProductsCheckpoint is ProductsCheckpoint def test_products_checkpoint_accumulates_batches(): diff --git a/tests/test_type_coercion.py b/tests/test_type_coercion.py index d591aa03f..8ce4644d9 100644 --- a/tests/test_type_coercion.py +++ b/tests/test_type_coercion.py @@ -8,6 +8,8 @@ from __future__ import annotations +import pytest + from adcp.types import ( AssetContentType, GetProductsRequest, @@ -16,6 +18,9 @@ ) from adcp.types.generated_poc.core.context import ContextObject from adcp.types.generated_poc.core.ext import ExtensionObject +from adcp.types.generated_poc.creative.list_creative_formats_request import ( + ListCreativeFormatsRequestCreativeAgent, +) from adcp.types.generated_poc.creative.list_creatives_request import Field1 as FieldModel from adcp.types.generated_poc.creative.list_creatives_request import Sort from adcp.types.generated_poc.enums.creative_sort_field import CreativeSortField @@ -57,6 +62,28 @@ def test_asset_types_accepts_none(self): req = ListCreativeFormatsRequest(asset_types=None) assert req.asset_types is None + @pytest.mark.parametrize( + "request_type", + [ListCreativeFormatsRequest, ListCreativeFormatsRequestCreativeAgent], + ) + @pytest.mark.parametrize( + ("field", "values", "expected"), + [ + ("disclosure_positions", ["footer", "prominent", "footer"], ["footer", "prominent"]), + ( + "disclosure_persistence", + ["initial", "continuous", "initial"], + ["initial", "continuous"], + ), + ], + ) + def test_unique_disclosure_filters_are_deduplicated( + self, request_type, field, values, expected + ): + """Both role-specific models preserve first-seen order for uniqueItems.""" + req = request_type(**{field: values}) + assert [item.value for item in getattr(req, field)] == expected + class TestDictToModelCoercion: """Test that model fields accept dict values.""" diff --git a/tests/test_verify_from_agent_url.py b/tests/test_verify_from_agent_url.py index 3d5674230..f1b8bcb2b 100644 --- a/tests/test_verify_from_agent_url.py +++ b/tests/test_verify_from_agent_url.py @@ -25,6 +25,7 @@ REQUEST_SIGNATURE_JWKS_UNTRUSTED, SignatureVerificationError, ) +from adcp.signing.replay import InMemoryReplayStore # ---- Test seams ---- @@ -290,6 +291,100 @@ async def fake_verify_starlette(request, *, options): # type: ignore[no-untyped assert seen["options"].signing_purpose == "request_signing" +@pytest.mark.asyncio +async def test_factory_uses_secure_replay_store_default_when_omitted( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The convenience factory reuses one per-origin bounded cache.""" + seen: list[Any] = [] + monkeypatch.setattr(agent_resolver, "_DEFAULT_REPLAY_STORE", InMemoryReplayStore()) + + async def fake_resolve(*args, **kwargs): + return _resolved_with_origins(None) + + async def fake_verify_starlette(request, *, options): # type: ignore[no-untyped-def] + seen.append(options.replay_store.claim("shared-kid", "shared-nonce", 60.0)) + return "ok" + + monkeypatch.setattr(agent_resolver, "async_resolve_agent", fake_resolve) + monkeypatch.setattr("adcp.signing.middleware.verify_starlette_request", fake_verify_starlette) + + await verify_from_agent_url( + _FakeStarletteRequest(), + "https://buyer.example.com/mcp", + agent_type="sales", + operation="get_products", + ) + await verify_from_agent_url( + _FakeStarletteRequest(), + "https://buyer.example.com/mcp", + agent_type="sales", + operation="get_products", + ) + + assert seen == ["claimed", "replayed"] + + +def test_default_replay_state_survives_many_counterparty_origins( + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.setattr( + agent_resolver, + "_DEFAULT_REPLAY_STORE", + InMemoryReplayStore(per_keyid_cap=10, global_cap=2_000), + ) + original = agent_resolver._default_replay_store_for_origin("https://a.example:443") + assert original.claim("kid", "nonce", 60.0) == "claimed" # type: ignore[attr-defined] + + for index in range(1_025): + partition = agent_resolver._default_replay_store_for_origin( + f"https://origin-{index}.example:443" + ) + assert partition.claim("kid", f"nonce-{index}", 60.0) == "claimed" # type: ignore[attr-defined] + + same_origin = agent_resolver._default_replay_store_for_origin("https://a.example:443") + assert same_origin.claim("kid", "nonce", 60.0) == "replayed" # type: ignore[attr-defined] + + +def test_default_replay_store_namespaces_identical_key_and_nonce_by_origin( + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.setattr(agent_resolver, "_DEFAULT_REPLAY_STORE", InMemoryReplayStore()) + first = agent_resolver._default_replay_store_for_origin("https://a.example:443") + second = agent_resolver._default_replay_store_for_origin("https://b.example:443") + + assert first.claim("kid", "nonce", 60.0) == "claimed" # type: ignore[attr-defined] + assert second.claim("kid", "nonce", 60.0) == "claimed" # type: ignore[attr-defined] + + +@pytest.mark.asyncio +async def test_factory_preserves_explicit_replay_store_none( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Callers can still explicitly opt out for compatibility or tests.""" + seen: dict[str, Any] = {} + + async def fake_resolve(*args, **kwargs): + return _resolved_with_origins(None) + + async def fake_verify_starlette(request, *, options): # type: ignore[no-untyped-def] + seen["options"] = options + return "ok" + + monkeypatch.setattr(agent_resolver, "async_resolve_agent", fake_resolve) + monkeypatch.setattr("adcp.signing.middleware.verify_starlette_request", fake_verify_starlette) + + await verify_from_agent_url( + _FakeStarletteRequest(), + "https://buyer.example.com/mcp", + agent_type="sales", + operation="get_products", + replay_store=None, + ) + + assert seen["options"].replay_store is None + + @pytest.mark.asyncio async def test_factory_passes_signing_purpose_through( monkeypatch: pytest.MonkeyPatch, @@ -385,7 +480,7 @@ async def fake_verify_starlette(request, *, options): # type: ignore[no-untyped agent_type="sales", operation="get_products", ) - assert seen["options"].expected_key_origins is None + assert seen["options"].expected_key_origins == {} # ---- Integration: production resolver drives the real verifier path ---- @@ -536,15 +631,7 @@ def test_static_jwks_resolver_does_not_satisfy_brand_sourced_protocol() -> None: # ---- Misconfig warnings (Argus first-pass follow-ups) ---- -def test_brand_json_source_without_expected_origins_emits_user_warning() -> None: - """A resolver advertising ``jwks_source='brand_json'`` paired with - ``expected_key_origins=None`` is an observable misconfig — the - spec's identity.key_origins consistency check (ADCP #3690 step 7) - silently no-ops. Surface as a :class:`UserWarning` so adopters - catch it in operator logs and thread the origins map through - ``VerifyOptions``.""" - import warnings as _w - +def test_brand_json_source_without_capabilities_map_uses_shared_origin_posture() -> None: from adcp.signing.agent_resolver import _BrandJsonStaticJwksResolver from adcp.signing.verifier import _maybe_check_key_origin @@ -552,17 +639,12 @@ def test_brand_json_source_without_expected_origins_emits_user_warning() -> None {"keys": []}, jwks_uri="https://keys.brand.example/jwks.json", ) - with _w.catch_warnings(record=True) as caught: - _w.simplefilter("always") - _maybe_check_key_origin( - resolver=resolver, - expected_key_origins=None, - signing_purpose="request_signing", - posture=None, - ) - user_warnings = [w for w in caught if issubclass(w.category, UserWarning)] - assert len(user_warnings) == 1 - assert "jwks_source='brand_json'" in str(user_warnings[0].message) + _maybe_check_key_origin( + resolver=resolver, + expected_key_origins=None, + signing_purpose="request_signing", + posture=None, + ) def test_legacy_resolver_with_expected_origins_emits_deprecation_warning() -> None: