-
Notifications
You must be signed in to change notification settings - Fork 6
Fix streaming backoff logic flaws in retry module #147
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
Conversation
|
@heyitsaamir 👋 This repository doesn't have Copilot instructions. With Copilot instructions, I can understand the repository better, work faster and produce higher quality PRs. I can generate a .github/copilot-instructions.md file for you automatically. Click here to open a pre-filled issue and assign it to me. I'll write the instructions, and then tag you for review. |
Co-authored-by: heyitsaamir <[email protected]>
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.
Pull Request Overview
This PR fixes critical flaws in the streaming backoff retry logic that caused incorrect attempt counting, non-exponential delays, and nested logger names during recursive retry calls.
- Fixed attempt number calculation to properly increment from 1 instead of staying constant
- Corrected exponential backoff formula to use
attempt_number - 1for true exponential progression - Prevented logger name nesting by passing existing child logger through recursive calls
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/apps/src/microsoft/teams/apps/utils/retry.py | Added attempt_number and _internal_logger parameters, fixed exponential delay calculation and logger passing |
| packages/apps/tests/test_retry.py | Added comprehensive test suite covering attempt counting, exponential backoff, and logger behavior |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
MehakBindra
left a comment
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.
Looks good, thanks Aamir !
The retry logic in the streaming backoff implementation had three critical flaws that caused incorrect behavior during rate limiting scenarios:
Issues Fixed
1. Logger attempt count stuck at 1
The attempt number in log messages was calculated using
options.max_attempts - max_attempts + 1, but sinceoptions.max_attemptsalways referenced the original value whilemax_attemptsdecreased, the calculation remained constant at 1 for all retry attempts.2. Incorrect exponential backoff delays
The exponential delay calculation used
base_delay * (2 ** (options.max_attempts - max_attempts)), which resulted in non-exponential delays like 0.13s → 0.29s → 0.20s instead of the expected exponential progression.3. Logger names growing with recursive calls
Each recursive retry call created a new
RetryOptionswithlogger.getChild("@teams/retry"), causing nested logger names likeparent.@teams/retry.@teams/retry.@teams/retry.Solution
attempt_numberparameter toRetryOptionsto properly track the current attempt numberbase_delay * (2 ** (attempt_number - 1))for true exponential backoff (0.5s → 1.0s → 2.0s → 4.0s)_internal_loggerparameter to pass the existing child logger between recursive calls, preventing nested logger namesBefore/After Example
This ensures proper rate limit handling in streaming scenarios where Microsoft Teams API calls need reliable exponential backoff.
Fixes #145.
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.