Make plugin hook interfaces async; drop sync hooks and deprecated params#511
Merged
Conversation
0aeb5c0 to
6b83d67
Compare
somethingnew2-0
added a commit
that referenced
this pull request
Jul 8, 2026
…Task The multi-Okta-call operations (ModifyGroupUsers, ModifyRoleGroups, DeleteGroup, and their composers ApproveAccessRequest / CreateApp / ApproveRoleRequest) committed local DB state and then awaited a fan-out of Okta API calls plus notification dispatch before returning — that tail dominates request latency. It now runs in a FastAPI BackgroundTask after the response, so the response returns as soon as the local DB state commits. Operations hand their fan-out batch to a request-scoped collector via `defer_or_drain_fan_out`; the `defer_fan_out` dependency on the six mutating routers drains it post-response (and drains inline on the endpoint-error path, where FastAPI's error response won't carry the background task). Outside an HTTP request (CLI, syncer, MCP, direct `execute()`) there is no collector and the batch drains inline exactly as before — so a route missing the dependency degrades to correct-but-not-deferred, never incorrect. Notification payloads are expunged from the session before deferral: the drain runs after the router's `db.expire_all()` and session teardown, and the async notification hooks read ORM attributes synchronously, so without detaching they would raise MissingGreenlet/DetachedInstanceError on the dead session and send_notification would silently swallow the drop. Expunge only runs when the fan-out is actually being deferred, so the inline path keeps its objects session-attached for callers that reuse them. Deferred-drain failures log at WARNING rather than ERROR: they are on-demand Okta stragglers the syncer CronJob reconciles on its next run, so they should not surface as Sentry ERROR noise. Depends on the async plugin hooks (#511), which make notification dispatch awaitable — the prerequisite for moving it off the request path — and on the fan-out error-logging drain (#481). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
b1c3d04 to
89cfcee
Compare
somethingnew2-0
added a commit
that referenced
this pull request
Jul 8, 2026
…Task The multi-Okta-call operations (ModifyGroupUsers, ModifyRoleGroups, DeleteGroup, and their composers ApproveAccessRequest / CreateApp / ApproveRoleRequest) committed local DB state and then awaited a fan-out of Okta API calls plus notification dispatch before returning — that tail dominates request latency. It now runs in a FastAPI BackgroundTask after the response, so the response returns as soon as the local DB state commits. Operations hand their fan-out batch to a request-scoped collector via `defer_or_drain_fan_out`; the `defer_fan_out` dependency on the six mutating routers drains it post-response (and drains inline on the endpoint-error path, where FastAPI's error response won't carry the background task). Outside an HTTP request (CLI, syncer, MCP, direct `execute()`) there is no collector and the batch drains inline exactly as before — so a route missing the dependency degrades to correct-but-not-deferred, never incorrect. Notification payloads are expunged from the session before deferral: the drain runs after the router's `db.expire_all()` and session teardown, and the async notification hooks read ORM attributes synchronously, so without detaching they would raise MissingGreenlet/DetachedInstanceError on the dead session and send_notification would silently swallow the drop. Expunge only runs when the fan-out is actually being deferred, so the inline path keeps its objects session-attached for callers that reuse them. Deferred-drain failures log at WARNING rather than ERROR: they are on-demand Okta stragglers the syncer CronJob reconciles on its next run, so they should not surface as Sentry ERROR noise. Depends on the async plugin hooks (#511), which make notification dispatch awaitable — the prerequisite for moving it off the request path — and on the fan-out error-logging drain (#481). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
somethingnew2-0
added a commit
that referenced
this pull request
Jul 8, 2026
…Task The multi-Okta-call operations (ModifyGroupUsers, ModifyRoleGroups, DeleteGroup, and their composers ApproveAccessRequest / CreateApp / ApproveRoleRequest) committed local DB state and then awaited a fan-out of Okta API calls plus notification dispatch before returning — that tail dominates request latency. It now runs in a FastAPI BackgroundTask after the response, so the response returns as soon as the local DB state commits. Operations hand their fan-out batch to a request-scoped collector via `defer_or_drain_fan_out`; the `defer_fan_out` dependency on the six mutating routers drains it post-response (and drains inline on the endpoint-error path, where FastAPI's error response won't carry the background task). Outside an HTTP request (CLI, syncer, MCP, direct `execute()`) there is no collector and the batch drains inline exactly as before — so a route missing the dependency degrades to correct-but-not-deferred, never incorrect. Notification payloads are expunged from the session before deferral: the drain runs after the router's `db.expire_all()` and session teardown, and the async notification hooks read ORM attributes synchronously, so without detaching they would raise MissingGreenlet/DetachedInstanceError on the dead session and send_notification would silently swallow the drop. Expunge only runs when the fan-out is actually being deferred, so the inline path keeps its objects session-attached for callers that reuse them. Deferred-drain failures log at WARNING rather than ERROR: they are on-demand Okta stragglers the syncer CronJob reconciles on its next run, so they should not surface as Sentry ERROR noise. Depends on the async plugin hooks (#511), which make notification dispatch awaitable — the prerequisite for moving it off the request path — and on the fan-out error-logging drain (#481). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
cf451ff to
56139e5
Compare
somethingnew2-0
commented
Jul 8, 2026
somethingnew2-0
commented
Jul 8, 2026
somethingnew2-0
commented
Jul 8, 2026
somethingnew2-0
commented
Jul 8, 2026
somethingnew2-0
added a commit
that referenced
this pull request
Jul 8, 2026
Addresses review on #511: - Replace the arbitrary ``hook_name: str`` parameter on the plugin dispatch wrappers with per-plugin StrEnums — NotificationHook, ConditionalAccessHook, and AppGroupLifecycleHook — and update every call site. Each StrEnum value is the pluggy hook attribute name, so members still pass straight to ``getattr(hook_relay, member)`` and key the metrics map. - Remove the dead commented-out notification/membership_id block in ApproveAccessRequest (that work now lives in ModifyGroupUsers). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
somethingnew2-0
added a commit
that referenced
this pull request
Jul 9, 2026
Per review discussion on #511: Access should not quietly swallow errors thrown by plugins. run_hooks_to_completion now logs each failed hook at ERROR (still non-propagating, so one plugin can't break the committed operation). This covers every plugin type — notifications included, undoing the notification-specific warn from #481. Plugins that expect noisy failures (e.g. connection timeouts) should catch those themselves rather than rely on the app downgrading them. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
somethingnew2-0
added a commit
that referenced
this pull request
Jul 11, 2026
Addresses review on #511: - Replace the arbitrary ``hook_name: str`` parameter on the plugin dispatch wrappers with per-plugin StrEnums — NotificationHook, ConditionalAccessHook, and AppGroupLifecycleHook — and update every call site. Each StrEnum value is the pluggy hook attribute name, so members still pass straight to ``getattr(hook_relay, member)`` and key the metrics map. - Remove the dead commented-out notification/membership_id block in ApproveAccessRequest (that work now lives in ModifyGroupUsers). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
somethingnew2-0
added a commit
that referenced
this pull request
Jul 11, 2026
Per review discussion on #511: Access should not quietly swallow errors thrown by plugins. run_hooks_to_completion now logs each failed hook at ERROR (still non-propagating, so one plugin can't break the committed operation). This covers every plugin type — notifications included, undoing the notification-specific warn from #481. Plugins that expect noisy failures (e.g. connection timeouts) should catch those themselves rather than rely on the app downgrading them. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
somethingnew2-0
added a commit
that referenced
this pull request
Jul 11, 2026
…Task The multi-Okta-call operations (ModifyGroupUsers, ModifyRoleGroups, DeleteGroup, and their composers ApproveAccessRequest / CreateApp / ApproveRoleRequest) committed local DB state and then awaited a fan-out of Okta API calls plus notification dispatch before returning — that tail dominates request latency. It now runs in a FastAPI BackgroundTask after the response, so the response returns as soon as the local DB state commits. Operations hand their fan-out batch to a request-scoped collector via `defer_or_drain_fan_out`; the `defer_fan_out` dependency on the six mutating routers drains it post-response (and drains inline on the endpoint-error path, where FastAPI's error response won't carry the background task). Outside an HTTP request (CLI, syncer, MCP, direct `execute()`) there is no collector and the batch drains inline exactly as before — so a route missing the dependency degrades to correct-but-not-deferred, never incorrect. Notification payloads are expunged from the session before deferral: the drain runs after the router's `db.expire_all()` and session teardown, and the async notification hooks read ORM attributes synchronously, so without detaching they would raise MissingGreenlet/DetachedInstanceError on the dead session and send_notification would silently swallow the drop. Expunge only runs when the fan-out is actually being deferred, so the inline path keeps its objects session-attached for callers that reuse them. Fan-out failures (Okta stragglers, notification-hook errors) log at ERROR, not WARNING: Access surfaces the errors its plugins and Okta calls raise rather than swallowing them as warnings — matching the async plugin-hook dispatch (#511). The failure still never propagates (the DB change is committed and the syncer reconciles Okta drift), and a component that expects noisy failures should catch them itself rather than rely on the drain downgrading them. Depends on the async plugin hooks (#511), which make notification dispatch awaitable — the prerequisite for moving it off the request path — and on the fan-out error-logging drain (#481). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
POST_MIGRATION_TODO items 18 and 19. The application became fully async during the SQLAlchemy migration, but the four pluggy plugin surfaces (notifications, conditional_access, app_group_lifecycle, metrics_reporter) were still synchronous, so every call site bridged the sync hook differently — AsyncSession.run_sync for the session-bearing lifecycle and syncer notification hooks, and inline-on-the-loop for request-path notification/conditional-access hooks (which blocks the request when a plugin does network I/O). Since 2.0 permits breaking plugin-API changes, the hooks are converted to native `async def` and the sync variants are removed outright (no deprecation window). This collapses all three bridges into a single `await`: lifecycle hooks receive the AsyncSession directly, and notification/conditional-access hooks are gathered on the event loop. metrics_reporter is async too so a reporter can push over an async client (batch_metrics is now an async context manager); the pure metadata/config/status/validation lifecycle hooks stay synchronous. pluggy does not await hooks, so the old @hookimpl(wrapper=True) wrappers (which recorded the "sent" metric and swallowed plugin errors) could not survive — that cross-cutting logic moves into two dispatch helpers, send_notification and evaluate_conditional_access. A load-time check (verify_async_impls) fails fast with a clear error if a plugin registers a sync impl for an async hook. Also completes item 19: the deprecated groups/roles/users notification parameters and their backward-compat shims are removed at this 2.0 boundary. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The async plugin interface (item 18) and the deprecated-parameter removal (item 19) are implemented in this PR, so drop their now-stale entries. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…Client The notifications_slack example was calling the synchronous slack_sdk WebClient inside asyncio.to_thread. The SDK ships a native-async client (AsyncWebClient), so await it directly on the event loop instead — matching the "prefer an async client over thread-wrapping a sync one" guidance. The retry helper is now async (asyncio.sleep), and aiohttp (the AsyncWebClient backend) is added to the example's requirements. README's plugin guidance is reworded to lead with async clients. (No example used requests or sync httpx; the Slack SDK was the only sync HTTP client, and httpx doesn't apply to an SDK-based integration.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Bumps the pluggy pin across the app dependency (pyproject.toml + uv.lock), the access-plugins package (api/plugins/setup.py), and the example plugins' requirements. Plugin hooks (now async) verified against 1.6.0. Stacked on #507 (uv/ty toolchain), where the pin lives in pyproject.toml/uv.lock. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Per the review on #481 (the fan-out drain this stack coordinates with): gather propagates a cancellation of the awaiting request to its children, tearing down in-flight hook coroutines, and abandons siblings when one fails. These hooks fire after the authoritative DB change has committed and only do network I/O (notifications, metrics), so an in-flight send must be allowed to finish, and one plugin failing must not cancel the others. Adds a shared run_hooks_to_completion(coros) helper (asyncio.wait + per-task exception collection, mirroring api/operations/_fan_out.py) and routes the notification, conditional-access, app-group-lifecycle, metrics, and CLI hook fan-outs through it. Failure semantics preserved: notifications record "sent" only when every impl succeeded; lifecycle rolls back on any hook error; the conditional-access gate is unchanged for the common <=1-plugin case. Includes a regression test that cancels the runner mid-flight and asserts the in-flight hook still completes — it fails if swapped back to gather. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Items 18 & 19 were removed from POST_MIGRATION_TODO.md in this PR, so the "(TODO 18)" tags in code comments and docstrings no longer point anywhere. Comment-only change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Addresses review on #511: - Replace the arbitrary ``hook_name: str`` parameter on the plugin dispatch wrappers with per-plugin StrEnums — NotificationHook, ConditionalAccessHook, and AppGroupLifecycleHook — and update every call site. Each StrEnum value is the pluggy hook attribute name, so members still pass straight to ``getattr(hook_relay, member)`` and key the metrics map. - Remove the dead commented-out notification/membership_id block in ApproveAccessRequest (that work now lives in ModifyGroupUsers). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The StrEnum members pushed a few invoke_app_group_lifecycle_hook calls over the line-length limit; reflow them so `ruff format --check` (CI lint) passes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Per review discussion on #511: Access should not quietly swallow errors thrown by plugins. run_hooks_to_completion now logs each failed hook at ERROR (still non-propagating, so one plugin can't break the committed operation). This covers every plugin type — notifications included, undoing the notification-specific warn from #481. Plugins that expect noisy failures (e.g. connection timeouts) should catch those themselves rather than rely on the app downgrading them. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
8d2f24d to
e8b6299
Compare
eguerrant
reviewed
Jul 14, 2026
eguerrant
previously approved these changes
Jul 14, 2026
eguerrant
left a comment
Contributor
There was a problem hiding this comment.
Just the one broken example comment, looks good otherwise
The development context files merged in #528 describe the pre-2.0 plugin behavior; this PR makes the interface async, so bring the two plugin-facing sections up to date. Plugin hooks are now awaited via run_hooks_to_completion (asyncio.wait, not gather) and a failing hook logs at ERROR without breaking the committed operation. The Plugin system section now states that hook impls must be async def across all four surfaces (with the sync metadata/config/status/ validation exceptions), how to do blocking I/O off the event loop, and that app-group-lifecycle mutating hooks receive an AsyncSession. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ample TODO 19 removed the deprecated `groups` param from access_expiring_user and `groups`/`roles`/`users` from access_expiring_owner, but the notifications_slack example still declared them. Since pluggy passes hook arguments by name, an impl declaring params the spec no longer supplies raises at call time — so the example was broken. Drop them to match the current NotificationPluginSpec. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
eguerrant
approved these changes
Jul 14, 2026
somethingnew2-0
added a commit
that referenced
this pull request
Jul 14, 2026
…Task The multi-Okta-call operations (ModifyGroupUsers, ModifyRoleGroups, DeleteGroup, and their composers ApproveAccessRequest / CreateApp / ApproveRoleRequest) committed local DB state and then awaited a fan-out of Okta API calls plus notification dispatch before returning — that tail dominates request latency. It now runs in a FastAPI BackgroundTask after the response, so the response returns as soon as the local DB state commits. Operations hand their fan-out batch to a request-scoped collector via `defer_or_drain_fan_out`; the `defer_fan_out` dependency on the six mutating routers drains it post-response (and drains inline on the endpoint-error path, where FastAPI's error response won't carry the background task). Outside an HTTP request (CLI, syncer, MCP, direct `execute()`) there is no collector and the batch drains inline exactly as before — so a route missing the dependency degrades to correct-but-not-deferred, never incorrect. Notification payloads are expunged from the session before deferral: the drain runs after the router's `db.expire_all()` and session teardown, and the async notification hooks read ORM attributes synchronously, so without detaching they would raise MissingGreenlet/DetachedInstanceError on the dead session and send_notification would silently swallow the drop. Expunge only runs when the fan-out is actually being deferred, so the inline path keeps its objects session-attached for callers that reuse them. Fan-out failures (Okta stragglers, notification-hook errors) log at ERROR, not WARNING: Access surfaces the errors its plugins and Okta calls raise rather than swallowing them as warnings — matching the async plugin-hook dispatch. The failure still never propagates (the DB change is committed and the syncer reconciles Okta drift), and a component that expects noisy failures should catch them itself rather than rely on the drain downgrading them. Builds on the async plugin hooks (#511, now merged), which make notification dispatch awaitable — the prerequisite for moving it off the request path — and on the fan-out error-logging drain (#481, now merged). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
somethingnew2-0
added a commit
that referenced
this pull request
Jul 14, 2026
…Task The multi-Okta-call operations (ModifyGroupUsers, ModifyRoleGroups, DeleteGroup, and their composers ApproveAccessRequest / CreateApp / ApproveRoleRequest) committed local DB state and then awaited a fan-out of Okta API calls plus notification dispatch before returning — that tail dominates request latency. It now runs in a FastAPI BackgroundTask after the response, so the response returns as soon as the local DB state commits. Operations hand their fan-out batch to a request-scoped collector via `defer_or_drain_fan_out`; the `defer_fan_out` dependency on the six mutating routers drains it post-response (and drains inline on the endpoint-error path, where FastAPI's error response won't carry the background task). Outside an HTTP request (CLI, syncer, MCP, direct `execute()`) there is no collector and the batch drains inline exactly as before — so a route missing the dependency degrades to correct-but-not-deferred, never incorrect. Notification payloads are expunged from the session before deferral: the drain runs after the router's `db.expire_all()` and session teardown, and the async notification hooks read ORM attributes synchronously, so without detaching they would raise MissingGreenlet/DetachedInstanceError on the dead session and send_notification would silently swallow the drop. Expunge only runs when the fan-out is actually being deferred, so the inline path keeps its objects session-attached for callers that reuse them. Fan-out failures (Okta stragglers, notification-hook errors) log at ERROR, not WARNING: Access surfaces the errors its plugins and Okta calls raise rather than swallowing them as warnings — matching the async plugin-hook dispatch. The failure still never propagates (the DB change is committed and the syncer reconciles Okta drift), and a component that expects noisy failures should catch them itself rather than rely on the drain downgrading them. Builds on the async plugin hooks (#511, now merged), which make notification dispatch awaitable — the prerequisite for moving it off the request path — and on the fan-out error-logging drain (#481, now merged). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
somethingnew2-0
added a commit
that referenced
this pull request
Jul 14, 2026
…Task The multi-Okta-call operations (ModifyGroupUsers, ModifyRoleGroups, DeleteGroup, and their composers ApproveAccessRequest / CreateApp / ApproveRoleRequest) committed local DB state and then awaited a fan-out of Okta API calls plus notification dispatch before returning — that tail dominates request latency. It now runs in a FastAPI BackgroundTask after the response, so the response returns as soon as the local DB state commits. Operations hand their fan-out batch to a request-scoped collector via `defer_or_drain_fan_out`; the `defer_fan_out` dependency on the six mutating routers drains it post-response (and drains inline on the endpoint-error path, where FastAPI's error response won't carry the background task). Outside an HTTP request (CLI, syncer, MCP, direct `execute()`) there is no collector and the batch drains inline exactly as before — so a route missing the dependency degrades to correct-but-not-deferred, never incorrect. Notification payloads are expunged from the session before deferral: the drain runs after the router's `db.expire_all()` and session teardown, and the async notification hooks read ORM attributes synchronously, so without detaching they would raise MissingGreenlet/DetachedInstanceError on the dead session and send_notification would silently swallow the drop. Expunge only runs when the fan-out is actually being deferred, so the inline path keeps its objects session-attached for callers that reuse them. Fan-out failures (Okta stragglers, notification-hook errors) log at ERROR, not WARNING: Access surfaces the errors its plugins and Okta calls raise rather than swallowing them as warnings — matching the async plugin-hook dispatch. The failure still never propagates (the DB change is committed and the syncer reconciles Okta drift), and a component that expects noisy failures should catch them itself rather than rely on the drain downgrading them. Builds on the async plugin hooks (#511, now merged), which make notification dispatch awaitable — the prerequisite for moving it off the request path — and on the fan-out error-logging drain (#481, now merged). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This was referenced Jul 15, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
POST_MIGRATION_TODO.mditems 18 (async plugin interfaces) and 19 (drop deprecated notification params).The app became fully async during the SQLAlchemy migration, but the four
pluggyplugin surfaces stayed synchronous, so every call site bridged the sync hook differently:AsyncSession.run_syncfor the session-bearing app-group-lifecycle hooks and the syncer's expiring-access notifications, and inline on the event loop for request-path notification/conditional-access hooks — which blocks the request whenever a plugin does network I/O.Since 2.0 allows breaking plugin-API changes, the hooks become native
async defand the sync variants are removed outright (no deprecation window). All three bridges collapse into a singleawait:AsyncSessiondirectly (norun_sync).batch_metricsis now an async context manager. The pure app-group-lifecycle metadata/config/status/validation hooks intentionally stay synchronous.pluggynever awaits hooks, so the old@hookimpl(wrapper=True)wrappers (which recorded the "sent" metric and swallowed plugin errors) can't survive — that cross-cutting logic moved intosend_notification/evaluate_conditional_access, both routed through a sharedrun_hooks_to_completionhelper. That helper usesasyncio.wait, notgather(following the #481 fan-out-drain review): a cancelled request doesn't tear down an in-flight hook, and one plugin failing doesn't cancel its siblings. A load-timeverify_async_implscheck fails fast if a plugin registers a sync impl for an async hook.Also: completes item 19 (removes the deprecated
groups/roles/usersnotification params), updates the example plugins to the async interface (thenotifications_slackexample uses Slack's native-asyncAsyncWebClient), documents the contract in the README, and bumpspluggy1.5.0 → 1.6.0 (pyproject.toml+uv.lock).Rebased on
main(no longer stacked): the uv/ty toolchain (#507) and the fan-out-drain fix (#481) it builds on have both merged. The request-path notifications run inside thecreate_taskfan-out that #481'sdrain_fan_out_tasksawaits.🤖 Generated with Claude Code