fix(release): harden version 7 for stable release - #1025
Conversation
Require PostgreSQL idempotency adopters to provide a distinct lock_pool so advisory-lock waits cannot deadlock the handler query pool. BREAKING CHANGE: PgBackend and lazy PostgreSQL idempotency wiring now require a distinct lock_pool. Missing or shared lock pools fail at construction.
Keep synchronous admission permits and proposal/idempotency lifecycle ownership until the underlying worker really settles. Async cancellation remains fail-closed because external mutation may already have occurred. BREAKING CHANGE: adopters that pass executor= to create_adcp_server_from_platform or serve must also set timed_sync_get_products_limit=.
Webhook verification now accepts both the spec-mandated request-signing purpose and the deprecated webhook-signing value while retaining tag-based cross-profile replay protection. Closes #1018
Derive the operation from the MCP JSON-RPC body and prefetch signing capabilities before enqueueing tools/call, avoiding both ContextVar loss and recursive writer-session deadlock. Closes #1017
Keep unresolved and misconfigured account paths on PERMISSION_DENIED while matching the mode-gate storyboard for an explicitly resolved live account. Closes #1011
There was a problem hiding this comment.
Request changes on one Major bug. Everything else here is clean, fail-closed hardening — the async JWKS resolver just needs to mirror the sync resolver it diverged from.
Must fix (blocking)
- Async
CachingJwksResolver.__call__rejects valid signed requests during every refresh window.src/adcp/signing/jwks.py— the newrequest_signature_jwks_unavailableraise ("cached JWKS is expired and refresh cooldown has not elapsed") lives in the unlocked pre-check and is never re-evaluated insideasync with self._lock._refreshsetsself._last_attempt = nowbefore the ~10s awaited fetch, so at eachmax_ageboundary the first request enters refresh and every concurrent verification arriving during the fetch window computescache_expired and (now - _last_attempt < cooldown)and rejects — legitimate signed inbound requests getrequest_signature_jwks_unavailablefor the duration of the fetch, recurring every 30 minutes under load. The syncCachingJwksResolverhandles this correctly: it raises insideself._refresh_lockafter re-reading state, so waiters block and observe the fresh cache. Move the async raise inside the lock and re-evaluate after re-reading_last_successful_refresh/_last_attempt, mirroring the sync path. Fails closed (security-reviewer: no replay window opened), but it's a verification-path availability regression this PR introduces on the default async resolver.code-reviewer: Major.
The sync resolver already does exactly this three lines up; the async one didn't get the memo.
Things I checked
- Semver signal is correct. The two breaking hardening changes —
PgBackend/lazy PG wiring now requiring a distinctlock_pool, andexecutor=now requiringtimed_sync_get_products_limit=— land underfix(signing)!/fix(decisioning)!withBREAKING CHANGE:footers and a Compatibility section in the body. Both fail at construction, not at runtime.push_senderis additive (feat(a2a)); new signing exports (AtomicReplayStore,ReplayClaimResult,supports_atomic_claim) are additive. - Sync-executor admission has no permit leak.
submit_supervised(time_budget.py) acquires theBoundedSemaphoreonce and releases exactly once — immediately onexecutor.submitfailure, otherwise via theconcurrent.futuresdone callback throughloop.call_soon_threadsafe. Supervised lifecycle tasks are strong-referenced in_SUPERVISED_SYNC_LIFECYCLES/_SUPERVISED_FINALIZATIONS/_SUPERVISED_OPERATIONSso they can't be GC'd mid-flight on the deadline path; the routed-sync ContextVar path acquires its permit inside_run_sync_delegate. - Idempotency lock contract holds.
dispatch.py(262 net lines) read in full: thehold()/put_if_absent()backend surface is concrete-with-NotImplementedErrorso existing subclasses stay importable,PgBackend._active_connectionreuse is keyed onasyncio.current_task()and only reused in the task that set it, and the legacy process-local fallback warns + deprecates rather than silently degrading.lock_pool is poolfails closed at construction. - Replay store is atomic and never evicts a live nonce.
claim()is check-and-set under oneRLock(in-memory) /pg_advisory_xact_lock+ conditional insert in one txn (PG); the indexed min-heap_purge_expiredpops only entries withexpiry < nowand rejects at capacity instead of evicting; caps validated> 0. - Bounded fetches close SSRF + decompression-bomb vectors. Redirect handling inside the new
client.stream()blocks still rebuilds the IP-pinned transport withfollow_redirects=False;_bounded_httpcounts actual streamed bytes (immune to a missing/lyingContent-Length) and rejects non-identitycontent-encodingbefore reading. - Freshness changes are strict tightenings and match AdCP 3.1.8. JWKS
max_age=1800, brand.json split, and revocation dropping_slide_next_updateon 304 (a 304 authenticates no new signednext_update) all fail closed.verify_from_agent_urlpassingresolution.key_origins or {}turns a missing brand-json declaration into a fail-closed reject rather than warn-and-skip.ad-tech-protocol-expert: sound. - Unique disclosure filters are the right shape. Both request schemas declare
disclosure_positions/disclosure_persistenceasuniqueItems: true, minItems: 1; order-preservingdict.fromkeysdedup viaWrapValidator(needed to run after enum coercion and short-circuitNonepast the rebuiltmin_length) is lossless, non-breaking, and keeps the SDK from emitting a schema-invalid array. Generator (scripts/generate_ergonomic_coercion.py) and its output (_ergonomic.py) updated in lockstep; nogenerated_poc/**hand-edits. - Webhook accepting
request-signingadcp_use is spec-mandated (webhooks reuse the request-signing key; isolation is thetag, not the purpose —expected_tag=WEBHOOK_TAGretained). Closes #1018. - Test-controller FORBIDDEN closes a real live-account bypass (
env_sandboxcould previously flipallowed=Truefor a resolved live account). No new bypass. - Credential exposure tightened — wire
details.caused_bynow carries only the exception class name, notstr(exc);ctx_metadatafail-close untouched.
Follow-ups (non-blocking — file as issues)
- brand.json stale-on-error window is dead in the default config.
brand_jwks.py:can_serve_staleclampsstale_deadline = min(expires_at + max_stale, fetched_at + DEFAULT_MAX_AGE_SECONDS); withDEFAULT_MAX_AGE_SECONDS=900and the default no-Cache-Controlpath whereexpires_at = fetched_at + 900, the second term collapses the deadline back toexpires_at→ zero grace. The code comment ("no configuration extends trust past 30 minutes") contradicts the 15-minute value it enforces. Fails closed, so not a block, but the 900+900 split the PR advertises never fires — the ceiling constant should be1800(orDEFAULT_MAX_AGE_SECONDS + DEFAULT_MAX_STALE_SECONDS).ad-tech-protocol-expert. Cheap to fix while you're already in the async-JWKS change. - Shared default replay store caps all counterparties together.
agent_resolver._DEFAULT_REPLAY_STOREis one 1M-entryInMemoryReplayStorepartitioned only by keyid namespace, so a single valid signer can exhaust the sharedglobal_capand fail-closed-reject other counterparties' legitimate requests. Net-new protection (old default wasreplay_store=None), so it's an improvement — but decoupleglobal_capfromper_keyid_capor expose it for sizing.security-reviewer: Low. _execute_lockeddetached task isolates the handler's contextvars. Running the idempotent handler inasyncio.create_taskmeans ContextVars it mutates are no longer visible after the wrapper returns (previously inline). Worth a one-line note in the PR body / changelog.code-reviewer.
Minor nits (non-blocking)
platform_router._run_sync_delegateexecution-is-None branch.worker = asyncio.create_task(asyncio.to_thread(...))thenawait asyncio.shield(worker)— under caller cancellation the only strong ref is dropped and the task can be GC'd ("Task was destroyed but it is pending"). Not reachable on the deadline-managedget_productspath (which always binds an execution), hence a nit.backends.pyper-instance dynamic ContextVar.ContextVar(f"adcp_idempotency_connection_{id(self)}", ...)is the documented dynamic-ContextVar anti-pattern; harmless for a long-lived backend, but prefer one module-level var keyed by backend identity.
Fix the async JWKS resolver and I'll approve. security-reviewer clean, ad-tech-protocol-expert sound-with-caveats, code-reviewer one Major.
|
Addressed Argus’s blocking finding in dd9634e:
Verification: |
|
The automated review encountered an issue (possibly reached max turns, timed out, or failed to post the final This is an automated message from the Argus AI review workflow. |
|
Maintainer merge note: the blocking Argus finding was fixed in dd9634e and covered by a concurrent-expiry regression. Full |
Summary
Tighten the version 7 release candidate and switch Release Please back to stable versioning so the next release PR promotes
7.0.0instead of producing another RC.This consolidates the releasable work from PRs #999 and #1000, including their requested review fixes, plus the bounded v7 issues found during the open issue/PR audit.
What changed
request-signingJWK purpose for webhook verification while retaining legacy compatibilitypush_senderthroughcreate_a2a_server,serve, unified transport, andServeConfigFORBIDDENfor test-controller attempts against resolved live accountsCompatibility
Two intentional breaking hardening changes are called out in the commits:
PgBackendand lazy PostgreSQL idempotency wiring require a distinctlock_poolexecutor=must also settimed_sync_get_products_limit=Both changes fail at construction rather than allowing unsafe runtime behavior.
Validation
make pre-pushCloses #1018
Closes #1017
Closes #1011
Closes #1009
Closes #1008
Closes #971
Supersedes #999 and #1000.