Add rate limiting and retry logic to source map uploads#10191
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces concurrency limiting and retry logic for Crashlytics source map uploads, along with improved error handling for 409 conflict responses. Feedback was provided to extract the retry logic into a helper function to ensure consistency between single-file and directory upload paths while reducing code duplication.
| let success = await uploadMap(request, 1); | ||
| if (!success) { | ||
| // Wait 5s and retry | ||
| await new Promise((res) => setTimeout(res, (options.retryDelay as number) || 5000)); | ||
| success = await uploadMap(request); | ||
| } | ||
| return success; |
There was a problem hiding this comment.
This retry logic is a great addition for handling transient errors. However, it's currently only applied when uploading source maps from a directory. For consistency, the same logic should be applied when uploading a single source map file (in the if (fstat.isFile()) block around line 86).
To avoid code duplication and improve maintainability, you could extract this retry logic into a helper function. This aligns with the repository's best practice of using helper functions to encapsulate branching logic.
For example:
async function uploadMapWithRetry(request: UploadRequest, options: CommandOptions): Promise<boolean> {
let success = await uploadMap(request, 1);
if (!success) {
// Wait and retry
await new Promise((res) => setTimeout(res, (options.retryDelay as number) || 5000));
success = await uploadMap(request);
}
return success;
}You could then use this helper in both the single file and directory upload scenarios.
References
- The style guide recommends using helper functions to encapsulate branching logic and reduce nesting. (link)
Description
Limits in-flight requests to upload/register source maps, and adds a retry with backoff
Scenarios Tested
Manual, unit tests