Skip to content

fix(server): deliver push notifications across all owners#1016

Merged
sokoliva merged 4 commits intoa2aproject:mainfrom
sokoliva:bug-fix
Apr 24, 2026
Merged

fix(server): deliver push notifications across all owners#1016
sokoliva merged 4 commits intoa2aproject:mainfrom
sokoliva:bug-fix

Conversation

@sokoliva
Copy link
Copy Markdown
Member

@sokoliva sokoliva commented Apr 24, 2026

Description

Fix a silent multi-tenant bug where push notifications were dropped for any deployment using authenticated users, and split the PushNotificationConfigStore read API into a user-callable owner-scoped read (get_info) and an internal cross-owner read for dispatch (get_info_for_dispatch).

The bug

BasePushNotificationSender accepted a ServerCallContext at construction time and called config_store.get_info(task_id, self._call_context) at dispatch. Because the sender is a process-wide singleton, callers passed a dummy ServerCallContext() (e.g. itk/main.py). The default OwnerResolverthen resolved the dummy to the empty-string owner, which never matched any real registrar's partition. Result: get_info returned [] and every notification was silently dropped in any deployment with real authentication.

The fix

  • New non-abstract method PushNotificationConfigStore.get_info_for_dispatch(task_id) returns every config for the task across all owners. Implemented in the in-memory and database stores. Custom 1.0 subclasses inherit a default implementation that forwards to get_info(task_id, ServerCallContext()) preserving their 1.0 behavior exactly and emits a DeprecationWarning
  • BasePushNotificationSender no longer takes context in __init__ and now calls get_info_for_dispatch. Identity is no longer held on the sender.
  • get_info(task_id, context) is unchanged and remains owner-scoped. Used by the user-callable read endpoints.

The split encodes the asymmetry in the type system: the user-callable method requires a context, the dispatch-only method does not. Authorization (check if the user can create a config for a specific task) happens at registration (set_info), not at dispatch.

Fixes #1015 🦕

@sokoliva sokoliva requested a review from a team as a code owner April 24, 2026 09:46
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request decouples the push notification dispatch path from the caller's identity by introducing a new get_info_for_dispatch method in the PushNotificationConfigStore. This change ensures that all registered webhooks for a task are triggered during events, regardless of which user initiated the action. Consequently, BasePushNotificationSender no longer requires a ServerCallContext at initialization. The PR also includes comprehensive E2E and unit tests to verify that user-facing read endpoints remain strictly owner-scoped while the internal dispatch path correctly handles cross-owner fan-out. Feedback includes a suggestion to use a nested list comprehension in the in-memory store implementation for a more Pythonic approach.

Comment thread src/a2a/server/tasks/inmemory_push_notification_config_store.py
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 24, 2026

🧪 Code Coverage (vs main)

⬇️ Download Full Report

Base PR Delta
src/a2a/server/tasks/base_push_notification_sender.py 100.00% 94.44% 🔴 -5.56%
src/a2a/server/tasks/database_push_notification_config_store.py 94.81% 94.96% 🟢 +0.15%
src/a2a/server/tasks/push_notification_config_store.py 100.00% 60.00% 🔴 -40.00%
Total 93.04% 93.00% 🔴 -0.03%

Generated by coverage-comment.yml

Co-authored-by: Copilot <copilot@github.com>
@sokoliva sokoliva changed the title fix(server)!: deliver push notifications to all registrars across owners fix(server): deliver push notifications across all owners Apr 24, 2026
@sokoliva sokoliva requested a review from ishymko April 24, 2026 12:28
Comment thread src/a2a/server/tasks/database_push_notification_config_store.py
Comment thread src/a2a/server/tasks/base_push_notification_sender.py Outdated
@sokoliva sokoliva requested a review from ishymko April 24, 2026 13:16
Copy link
Copy Markdown
Member

@ishymko ishymko left a comment

Choose a reason for hiding this comment

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

🚀

@sokoliva sokoliva merged commit c24ae05 into a2aproject:main Apr 24, 2026
35 checks passed
ishymko pushed a commit that referenced this pull request Apr 24, 2026
🤖 I have created a release *beep* *boop*
---


##
[1.0.2](v1.0.1...v1.0.2)
(2026-04-24)


### Features

* **helpers:** add non-text Part, Message, and Artifact helpers
([#1004](#1004))
([cfdbe4c](cfdbe4c))


### Bug Fixes

* **proto:** use field.label instead of is_repeated for protobuf
compatibility
([#1010](#1010))
([7d197db](7d197db))
* **server:** deliver push notifications across all owners
([#1016](#1016))
([c24ae05](c24ae05))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
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.

[Bug]: BasePushNotificationSender requires request-scoped ServerCallContext in constructor

2 participants