Skip to content
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

fix(agents-api): Retry 524 http status code #1154

Merged
merged 1 commit into from
Feb 13, 2025

Conversation

HamadaSalhab
Copy link
Contributor

@HamadaSalhab HamadaSalhab commented Feb 13, 2025

PR Type

Bug fix


Description

  • Added HTTP status code 524 to retryable codes.

  • Ensures the system retries on Cloudflare-specific timeouts.


Changes walkthrough 📝

Relevant files
Bug fix
tasks.py
Include 524 in retryable HTTP status codes                             

agents-api/agents_api/common/exceptions/tasks.py

  • Added HTTP status code 524 to the RETRYABLE_HTTP_STATUS_CODES tuple.
  • Updated comments to reflect the addition.
  • +1/-0     

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.

  • Important

    Add HTTP status code 524 to retryable status codes in tasks.py.

    • Behavior:
      • Add HTTP status code 524 to RETRYABLE_HTTP_STATUS_CODES in tasks.py, allowing retries for this status code.

    This description was created by Ellipsis for e935566. It will automatically update as commits are pushed.

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Duplicate Code

    Both status codes 504 and 524 have the same comment "Gateway Timeout (the internet took a detour)". Consider updating the comment for 524 to be more specific about it being a Cloudflare timeout.

    504,  # Gateway Timeout (the internet took a detour)
    524,  # Gateway Timeout (the internet took a detour)

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Clarify duplicate status code comments

    Remove duplicate Gateway Timeout status code 524 or update its comment to
    differentiate it from 504, as both are currently described identically.

    agents-api/agents_api/common/exceptions/tasks.py [145-151]

     RETRYABLE_HTTP_STATUS_CODES = (
         408,  # Request Timeout (server needs a coffee break)
         429,  # Too Many Requests (slow down, speedster!)
         503,  # Service Unavailable (server is having a moment)
    -    504,  # Gateway Timeout (the internet took a detour)
    -    524,  # Gateway Timeout (the internet took a detour)
    +    504,  # Gateway Timeout (standard gateway timeout)
    +    524,  # Gateway Timeout (Cloudflare specific timeout)
     )
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    __

    Why: The suggestion improves code clarity by differentiating between two similar Gateway Timeout status codes (504 and 524), making it clear that 524 is Cloudflare-specific. This helps developers better understand the different timeout scenarios.

    Medium

    Copy link
    Contributor

    @ellipsis-dev ellipsis-dev bot left a comment

    Choose a reason for hiding this comment

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

    ❌ Changes requested. Reviewed everything up to e935566 in 6 minutes and 1 seconds

    More details
    • Looked at 12 lines of code in 1 files
    • Skipped 0 files when reviewing.
    • Skipped posting 1 drafted comments based on config settings.
    1. agents-api/agents_api/common/exceptions/tasks.py:150
    • Draft comment:
      Consider clarifying that 524 is Cloudflare-specific (if applicable) rather than using the same generic 'Gateway Timeout' comment as 504.
    • Reason this comment was not posted:
      Confidence changes required: 33% <= threshold 50%
      None

    Workflow ID: wflow_Pj5o3iUU9M4dOVRi


    Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

    @whiterabbit1983 whiterabbit1983 merged commit 233d683 into dev Feb 13, 2025
    15 checks passed
    @whiterabbit1983 whiterabbit1983 deleted the x/retryable-status-codes branch February 13, 2025 10:10
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants