-
Notifications
You must be signed in to change notification settings - Fork 0
Fix role broadcast loop in Stripe webhook #171
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Fix role broadcast loop in Stripe webhook #171
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary of Changes
Hello @Cloudhabil, I'm Gemini Code Assist1! 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 primarily addresses a critical bug in the Stripe webhook handler that prevented proper role broadcast, ensuring that all relevant internal systems are notified of Stripe events. Additionally, it significantly improves the robustness and type safety of the bus_client by refining its retry mechanism and tightening type hints for better code maintainability and error prevention.
Highlights
- Stripe Webhook Fix: Corrected a logic error in the Stripe webhook handler that caused a premature return, preventing the broadcast of role updates and potentially leading to a broadcast loop.
- Type Hinting Improvements: Enhanced type safety in core/bus_client.py by tightening the typing for _arequest's keyword arguments and making the _with_retry function generic and more robust.
- Health Endpoint Indentation: Fixed an indentation issue in the /health endpoint within server/stripe_server.py for proper formatting.
- Retry Logic Refinement: Simplified the _with_retry utility in core/bus_client.py to consistently await the provided function, removing the need for inspect.isawaitable checks and explicit casting.
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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| 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 issue 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.
Footnotes
-
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. ↩
There was a problem hiding this 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 fixes a bug in the Stripe webhook handler where a broadcast to different roles was prevented by an early return. It also includes several cleanups, such as improving type hints in the bus client to satisfy mypy and fixing formatting. The fix in the webhook handler is correct. However, my review identifies a significant performance issue: the handler now makes synchronous network calls within an async function. I've provided a suggestion to use asynchronous, concurrent requests to prevent blocking the event loop and improve response time.
| for role in ["CFO", "COO", "CEO", "CMO", "CPO"]: | ||
| requests.post( | ||
| f"{BUS_URL}/publish", json={"topic": role, "data": {"stripe_event": event.get("type")}} | ||
| f"{BUS_URL}/publish", | ||
| json={"topic": role, "data": {"stripe_event": event.get("type")}}, | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While the fix correctly enables the broadcast to different roles, using synchronous requests.post in an async function blocks the event loop. This can severely degrade server performance, especially under load. It's highly recommended to use an asynchronous HTTP client like httpx to perform these requests without blocking.
Furthermore, the requests to each role's topic are sent sequentially. These can be sent concurrently using asyncio.gather to reduce the total time taken for the webhook to respond.
As a minor improvement for maintainability, the list of roles could be defined as a module-level constant.
To apply these changes, you would need to add import httpx and import asyncio at the top of the file, and remove import requests.
| for role in ["CFO", "COO", "CEO", "CMO", "CPO"]: | |
| requests.post( | |
| f"{BUS_URL}/publish", json={"topic": role, "data": {"stripe_event": event.get("type")}} | |
| f"{BUS_URL}/publish", | |
| json={"topic": role, "data": {"stripe_event": event.get("type")}}, | |
| ) | |
| async with httpx.AsyncClient() as client: | |
| tasks = [ | |
| client.post( | |
| f"{BUS_URL}/publish", | |
| json={"topic": role, "data": {"stripe_event": event.get("type")}}, | |
| ) | |
| for role in ["CFO", "COO", "CEO", "CMO", "CPO"] | |
| ] | |
| await asyncio.gather(*tasks) |
|
Codex Review: Here are some suggestions. Lines 33 to 38 in 333366c
[P1] Broadcast Stripe events without blocking the async server The webhook now executes a synchronous Reply with About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback". |
Summary
_arequest_with_retrygeneric and propagate return value to satisfy mypyTesting
ruff check core/bus_client.py core/kb.py server/stripe_server.pymypy core/bus_client.py core/kb.py server/stripe_server.pypytesthttps://chatgpt.com/codex/tasks/task_e_68bcb47871c083228dcbb2dd2386e160