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.

Sign up for GitHub

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

Jump to bottom

handle repeated 429s from Observable API with exponential back off #1761

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

handle repeated 429s from Observable API with exponential back off #1761

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion src/observableApiClient.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import type {ApiKey} from "./observableApiConfig.js";
import {faint, red} from "./tty.js";

const MIN_RATE_LIMIT_RETRY_AFTER = 1;
const MAX_RATE_LIMIT_ATTEMPTS = 3;

export function getObservableUiOrigin(env = process.env): URL {
const urlText = env["OBSERVABLE_ORIGIN"] ?? "https://observablehq.com";
Expand Down Expand Up @@ -69,11 +70,18 @@ export class ObservableApiClient {
throw error;
}

if (response.status === 429) {
let rateLimitCount = 0;
while (response.status === 429) {
if (rateLimitCount >= MAX_RATE_LIMIT_ATTEMPTS) {
Copy link
Member

Choose a reason for hiding this comment

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

It looks like rateLimitCount is only incremented once when this._rateLimit === null; am I reading it wrong? Normally I like to combine the increment and check into the same statement so that it’s more obvious that it can never infinite loop.

Suggested change
if (rateLimitCount >= MAX_RATE_LIMIT_ATTEMPTS) {
if (rateLimitCount++ >= MAX_RATE_LIMIT_ATTEMPTS) {

Copy link
Member Author

@mythmon mythmon Oct 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

You're right that the rateLimitCount is only incremented when this._rateLimit is null. However, the bug is something different. this._rateLimit is intended to be used for multiple concurrent callers of this function to wait collaboratively. That's not working with this new approach either though. I'll fix it up.

this._clack.log.error(`Rate limited ${rateLimitCount} times. Giving up.`);
break;
}
// rate limit
if (this._rateLimit === null) {
let retryAfter = +response.headers.get("Retry-After");
if (isNaN(retryAfter) || retryAfter < MIN_RATE_LIMIT_RETRY_AFTER) retryAfter = MIN_RATE_LIMIT_RETRY_AFTER;
retryAfter *= Math.pow(2, rateLimitCount);
rateLimitCount++;
this._clack.log.warn(`Hit server rate limit. Waiting for ${retryAfter} seconds.`);
this._rateLimit = new Promise((resolve) => setTimeout(resolve, retryAfter * 1000));
}
Expand Down