Skip to content

fix(decisioning): supervise timed and canceled work - #1000

Open
bokelley wants to merge 1 commit into
mainfrom
codex/security-runtime-supervision
Open

fix(decisioning): supervise timed and canceled work#1000
bokelley wants to merge 1 commit into
mainfrom
codex/security-runtime-supervision

Conversation

@bokelley

Copy link
Copy Markdown
Contributor

Summary

  • supervise synchronous work that outlives request cancellation or time budgets
  • bound timed synchronous admission, including router-backed platforms
  • preserve idempotency and proposal lifecycle ownership until underlying work completes

Why

Timed-out or cancelled requests could release reservations while worker threads were still mutating state. Under load this allowed duplicate work and unbounded executor queues.

Validation

  • focused decisioning, time-budget, proposal, and router saturation tests
  • independent concurrency review against current origin/main

Compatibility

Admission and executor context now propagate through eager, lazy, proposal-manager, and registry routers. Existing direct asynchronous platform behavior is unchanged.

task.cancel("client disconnected")

with pytest.raises(asyncio.CancelledError, match="client disconnected") as exc_info:
await task
assert await asyncio.to_thread(entered.wait, 1)
task.cancel("client disconnected")
with pytest.raises(asyncio.CancelledError, match="client disconnected"):
await task
assert await asyncio.to_thread(entered.wait, 1)
task.cancel()
with pytest.raises(asyncio.CancelledError):
await task

task.cancel("buyer disconnected")
with pytest.raises(asyncio.CancelledError, match="buyer disconnected"):
await task
assert await asyncio.to_thread(entered.wait, 1)
task.cancel("buyer disconnected")
with pytest.raises(asyncio.CancelledError, match="buyer disconnected"):
await task
"""Settle a sync worker after its request task has been cancelled."""
try:
result = await worker_future
except BaseException as exc:
webhook_target=webhook_target,
webhook_auto_emit=webhook_auto_emit,
)
except BaseException:
try:
await on_failure(exc)
except Exception:
except BaseException:
)
)
except Exception:
except BaseException:

@aao-ipr-bot aao-ipr-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. Right shape: a cancelled or timed-out request can no longer release a durable reservation while the worker thread is still mutating state, and the fix bounds the pre-existing thread-pool slot leak instead of papering over it.

The load-bearing move is asyncio.shield over the wrapped worker future plus a supervised settle-task (_settle_cancelled_sync_lifecycle) that fires on_complete/on_failure from the worker's real terminal outcome — so the CONSUMING→CONSUMED transition happens even after the buyer disconnects. Verified in test_sync_create_media_buy_cancellation_waits_for_worker_success.

Things I checked

  • Permit accounting is leak-free. SyncExecutorAdmission acquires before executor.submit, releases exactly once via the future's done-callback (fires on success and worker exception), and releases in the except on submit failure without adding the callback. Release is marshaled back with loop.call_soon_threadsafe — correct, since asyncio.Semaphore is not thread-safe (time_budget.py:95, dispatch.py:1445-1470).
  • No double settlement. Direct-sync cancel sets sync_lifecycle_continues=True in the inner except asyncio.CancelledError; the re-raised CancelledError then re-enters the outer except BaseException, which sees the flag and skips _safe_on_failure_call. Router-path cancel carries the _adcp_sync_worker_future marker and is settled once in the outer handler. security-reviewer and code-reviewer both traced this — single settlement each path.
  • _project_invocation_result extraction is faithful. paramsrequest_params rename threads through; the sync/handoff/workflow arms are unchanged; happy path is unchanged.
  • get_adcp_capabilities INTERNAL_ERROR alignment closes a wire leak. It was the last outlier hand-rolling caused_by={type, message}; it now routes through _internal_error_message/_internal_error_details, which emit {type} only. str(exc) is deliberately dropped so a platform that raises on secret material can't leak an OAuth secret on the wire. Confirmed by the new assertions at test_decisioning_capabilities_projection.py:640-642.
  • Not a wire-contract break. ad-tech-protocol-expert: sound — per schemas/cache/3.1/core/error.json, details is additionalProperties: true and caused_by is not a defined property; the normative recovery signal is error.recovery (still terminal). incomplete[]-when-saturated is invisible to the buyer and matches the existing timeout contract.
  • No tenant/auth regression. _run_sync_delegate carries only admission + executor through two ContextVars; the tenant-scoped ctx is still a positional arg, and _bind_routed_sync_execution resets both tokens in finally so nothing bleeds across sibling delegates.
  • proposal_dispatch.py except Exceptionexcept BaseException. Awaiting release_consumption inside a CancelledError handler is safe — the exception is already caught, the bare raise re-raises it, and the inner except BaseException absorbs a second cancel during release. This is the point: cancellation must not strand a CONSUMING reservation.

Follow-ups (non-blocking — file as issues)

  • Routed, no-deadline get_products cancel drops its on_complete. In _run_sync_delegate the sync_admission is None branch (platform_router.py:135-139) sets the marker and re-raises, but dispatch's outer handler only builds a settle-task when on_failure is not None. get_products wires on_complete=_persist_draft_hook with no on_failure, so on client-disconnect the worker is orphaned (stray "Task exception was never retrieved" on failure; draft-persist silently skipped on success). No reservation is held on this path, so it's not corruption — but it's the one gap in "supervise cancelled work." Supervise whenever on_complete is not None or on_failure is not None.
  • asyncio.BoundedSemaphore over Semaphore. Accounting is correct today, but a future stray release() would silently raise the ceiling above timed_sync_get_products_limit rather than fail-fast (time_budget.py:95).
  • Admission is per-handler, not per-tenant. One tenant bursting slow/cancelled timed get_products can hold all workers//2 permits and stall other tenants' timed calls. Strictly better than the pre-PR shared-pool exhaustion; key on ctx.account if per-tenant fairness matters.

Minor nits (non-blocking)

  1. Stale comment now contradicts the invariant. dispatch.py:1529-1532 still reads "We expose only the exception class name + str (not the traceback)." _internal_error_details no longer exposes str — that omission is the whole point of the sanitization this PR leans on. Pre-existing, but adjacent enough that a maintainer could "restore" the str to match the comment and reintroduce the leak. Drop "+ str."

Independent concurrency review claimed in the PR body; the test matrix (direct-sync, eager/lazy router, campaign-unit bypass, saturation-without-queue-growth) covers the new paths well. Safe to merge once CI is green.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant