Skip to content

Conversation

@Cloudhabil
Copy link
Owner

Summary

  • clean up stripe webhook handler and fix health endpoint
  • tighten typing for bus client _arequest
  • make _with_retry generic and propagate return value to satisfy mypy

Testing

  • ruff check core/bus_client.py core/kb.py server/stripe_server.py
  • mypy core/bus_client.py core/kb.py server/stripe_server.py
  • pytest

https://chatgpt.com/codex/tasks/task_e_68bcb47871c083228dcbb2dd2386e160

Copy link
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.

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

  1. 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.

Copy link
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 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.

Comment on lines 34 to 38
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")}},
)
Copy link
Contributor

Choose a reason for hiding this comment

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

high

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.

Suggested change
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)

@chatgpt-codex-connector
Copy link

Codex Review: Here are some suggestions.

add_entry(kind="stripe_event", type=event.get("type"))
for role in ["CFO", "COO", "CEO", "CMO", "CPO"]:
requests.post(
f"{BUS_URL}/publish",
json={"topic": role, "data": {"stripe_event": event.get("type")}},
)

[P1] Broadcast Stripe events without blocking the async server

The webhook now executes a synchronous requests.post call for each role, but this handler is an async def. Because the calls are performed directly in the event loop and no timeout is supplied, a slow or unavailable bus will block the entire FastAPI worker until each HTTP request finishes (and, with the default timeout of None, that can be indefinitely). That means a single failed bus publish can stall all incoming webhook and health requests. Consider using an async HTTP client (e.g. httpx.AsyncClient with await) or offloading the blocking work to a background task/thread with a reasonable timeout so the event loop stays responsive.


Reply with @codex fix comments to fix any unresolved comments.

About Codex in GitHub

Your 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".

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants