fix: ensure push notifications order by guglielmo-san · Pull Request #141 · a2aproject/a2a-js

guglielmo-san · 2025-10-24T12:25:20Z

Description

This PR introduces a promise chain (this.notificationChain) to serialize the batches of notifications. This ensures that only one batch at a time is processed, in a predictable First-In, First-Out (FIFO) order. The test was already ensuring it and it caused some flakiness before, examples:

https://github.com/a2aproject/a2a-js/actions/runs/18750636883 (see top right corner for first two failed attempts)
https://github.com/a2aproject/a2a-js/actions/runs/17686476400

The push_notification_integration.spec.ts test file has been modified to simulate a more deterministic delay (100ms) on the completion of the first received notification. This guarantee a 100% test failure when the notification serialization is not implemented.

Thank you for opening a Pull Request!
Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

Follow the CONTRIBUTING Guide.
Make your Pull Request title in the https://www.conventionalcommits.org/ specification.
- Important Prefixes for release-please:
  - fix: which represents bug fixes, and correlates to a SemVer patch.
  - feat: represents a new feature, and correlates to a SemVer minor.
  - feat!:, or fix!:, refactor!:, etc., which represent a breaking change (indicated by the !) and will result in a SemVer major.
Ensure the tests and linter pass
Appropriate docs were updated (if necessary)

Fixes #140 🦕

…of notifications

gemini-code-assist · 2025-10-24T12:25:41Z

Summary of Changes

Hello @guglielmo-san, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request resolves a race condition within the push notification sending logic by introducing a promise-based serialization mechanism. This ensures that notification batches are processed sequentially in a predictable FIFO order, preventing concurrency issues. The effectiveness of this solution is confirmed by an enhanced integration test that now deterministically reproduces the original race condition.

Highlights

Race Condition Fix: Implemented a fix for a race condition in the SendPushNotification process to ensure reliable notification delivery.
Notification Serialization: Introduced a notificationChain (a promise chain) within DefaultPushNotificationSender to serialize batches of push notifications, ensuring they are processed one at a time.
FIFO Ordering: The new serialization mechanism guarantees First-In, First-Out (FIFO) processing order for notification batches, preventing out-of-order delivery issues.
Improved Test Coverage: Updated push_notification_integration.spec.ts to include a deterministic 100ms delay for 'submitted' status notifications, making the race condition reliably reproducible and validating the effectiveness of the fix.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

gemini-code-assist

Code Review

This pull request effectively addresses the race condition in SendPushNotification by serializing notification batches using a promise chain. The changes to the integration test, which introduce a delay to deterministically trigger the race condition, are a great way to validate the fix. My review includes a suggestion to make the promise chain more resilient to potential errors, preventing the entire notification mechanism from halting. I've also recommended an improvement to the test assertions to make them clearer and more explicit about verifying the correct order of notifications.

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

ishymko

Let's reference failed test runs from Actions as an example in the PR description.

Co-authored-by: Ivan Shymko <vana.shimko@gmail.com>

🤖 I have created a release *beep* *boop* --- ## [0.3.5](v0.3.4...v0.3.5) (2025-11-05) ### Bug Fixes * cancel/task now returns the expected 'cancelled' task status ([#158](#158)) ([3eec0bd](3eec0bd)) * correct the default value in the agentCardPath comment ([#130](#130)) ([3279c7b](3279c7b)) * enable follow up task issue on non blocking request ([#160](#160)) ([352dc05](352dc05)) * ensure push notifications order ([#141](#141)) ([22360e6](22360e6)) * ensure user messages are added to existing tasks ([#138](#138)) ([00fb916](00fb916)) * validate JSON-RPC params ([#155](#155)) ([5b39f4d](5b39f4d)) * validate JSON-RPC request id according to the spec ([#152](#152)) ([9558c36](9558c36)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

guglielmo-san added 7 commits October 23, 2025 22:01

Add await on dispatchNotification to guarantee correct sending order …

cea3600

…of notifications

allow manual run of run unit test action workflow

41ee22e

Implement code to avoid race condition on subsequent notitifications

977e998

add for loop in test execution

952d647

Merge branch 'main' into fix_flacky_tests

a18fb47

Fix race conditions on notification send

08c023a

remove workflow_dispatch modification

6e95e1d

revert spacing modification

c96305a

gemini-code-assist Bot reviewed Oct 24, 2025

View reviewed changes

Comment thread src/server/push_notification/default_push_notification_sender.ts Outdated

Comment thread test/server/push_notification_integration.spec.ts Outdated

Update test/server/push_notification_integration.spec.ts

aae0f84

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

guglielmo-san changed the title ~~Fix: Race condition on SendPushNotification~~ fix: Race condition on SendPushNotification Oct 24, 2025

ishymko reviewed Oct 24, 2025

View reviewed changes

guglielmo-san added 2 commits October 24, 2025 13:11

move the catch from the _dispatchNotification function to the send

94fdb1a

move the delay case to the /notify/:scenario endpoint

0e50542

ishymko changed the title ~~fix: Race condition on SendPushNotification~~ fix: ensure push notifications order Oct 24, 2025

ishymko approved these changes Oct 24, 2025

View reviewed changes

Comment thread test/server/push_notification_integration.spec.ts Outdated

Comment thread test/server/push_notification_integration.spec.ts

Comment thread test/server/push_notification_integration.spec.ts Outdated

Comment thread test/server/push_notification_integration.spec.ts Outdated

fix spacing and formatting

7e7880f

ishymko requested a review from swapydapy October 24, 2025 14:32

swapydapy requested changes Oct 24, 2025

View reviewed changes

Comment thread src/server/push_notification/default_push_notification_sender.ts Outdated

Comment thread test/server/push_notification_integration.spec.ts Outdated

guglielmo-san added 2 commits October 28, 2025 10:58

make a per task notification chain

cf08a80

remove duplicate import

98deaab

ishymko reviewed Oct 28, 2025

View reviewed changes

Comment thread src/server/push_notification/default_push_notification_sender.ts Outdated

Update src/server/push_notification/default_push_notification_sender.ts

22eeea6

Co-authored-by: Ivan Shymko <vana.shimko@gmail.com>

guglielmo-san requested a review from swapydapy October 28, 2025 14:37

guglielmo-san enabled auto-merge (squash) October 28, 2025 16:13

guglielmo-san disabled auto-merge October 28, 2025 16:13

swapydapy approved these changes Oct 28, 2025

View reviewed changes

Comment thread src/server/push_notification/default_push_notification_sender.ts

guglielmo-san added 2 commits October 29, 2025 08:09

Add comment to explain garbacee collector behavior

7371468

Merge branch 'main' into fix_flacky_tests

4a4154b

guglielmo-san merged commit 22360e6 into a2aproject:main Oct 29, 2025
3 checks passed

guglielmo-san deleted the fix_flacky_tests branch October 29, 2025 08:12

a2a-bot mentioned this pull request Oct 28, 2025

chore(main): release 0.3.5 #131

Merged

bartek-gralewicz mentioned this pull request Apr 30, 2026

chore(bgralewicz/update_release_please_for_alpha_versions): release sdk 1.0.0-alpha.0 #438

Closed

a2a-bot mentioned this pull request May 4, 2026

chore(epic/1.0_breaking_changes): release sdk 1.0.0-alpha.0 #442

Merged

a2a-bot mentioned this pull request May 12, 2026

chore(epic/1.0_breaking_changes): release 0.3.9 #463

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: ensure push notifications order#141

fix: ensure push notifications order#141
guglielmo-san merged 17 commits into
a2aproject:mainfrom
guglielmo-san:fix_flacky_tests

guglielmo-san commented Oct 24, 2025 •

edited by ishymko

Loading

Uh oh!

gemini-code-assist Bot commented Oct 24, 2025

Uh oh!

gemini-code-assist Bot left a comment

Uh oh!

Uh oh!

Uh oh!

ishymko left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

guglielmo-san commented Oct 24, 2025 • edited by ishymko Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

gemini-code-assist Bot commented Oct 24, 2025

Summary of Changes

Highlights

Footnotes

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

ishymko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

guglielmo-san commented Oct 24, 2025 •

edited by ishymko

Loading