Skip to content

⚡ Optimize GitHub events fetch to fail fast on rate limit #292

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

Open
is0692vs wants to merge 1 commit into
base: main
Choose a base branch
from
+13 −1
Open

⚡ Optimize GitHub events fetch to fail fast on rate limit #292

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
14 changes: 13 additions & 1 deletion src/lib/github.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -685,9 +685,21 @@ export const fetchActivity = cache(async function fetchActivity(
// Suppress unhandled promise rejections for subsequent pages if we break early or throw
promises.forEach((p) => p.catch((e) => logger.error("Event fetch promise rejected:", e)));

// A promise that rejects immediately if a critical error occurs
const failFast = Promise.all(
promises.map((p) =>
p.catch((e) => {
if (e instanceof UserNotFoundError || e instanceof RateLimitError) {
throw e;
}
return null;
})
)
).then(() => new Promise<never>(() => {})); // Never resolves to prevent Promise.race from resolving with nulls
Comment on lines +689 to +698
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot May 22, 2026

Choose a reason for hiding this comment

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

medium

The failFast promise implementation using Promise.all on the mapped promises array is clever, but it has a minor efficiency drawback: Promise.all will hold onto the full result arrays (GitHubEvent[]) of all successful pages until every page has settled. While manageable for 3 pages, a more memory-efficient approach for a 'fail-fast' monitor is to use Promise.race with a map that only returns rejections or never-resolving promises for successes. This ensures the monitor doesn't keep references to the data fetched by other concurrent requests. I have updated the suggestion to include explicit return types and to log suppressed errors for better maintainability and debugging.

const failFast = Promise.race(
  promises.map((p: Promise<GitHubEvent[]>): Promise<never> =>
    p.then(
      (): Promise<never> => new Promise<never>(() => {}),
      (e: unknown): Promise<never> => {
        if (e instanceof UserNotFoundError || e instanceof RateLimitError) {
          throw e;
        }
        console.error('Non-critical error suppressed in fail-fast monitor:', e);
        return new Promise<never>(() => {});
      }
    )
  )
);
References
  1. When suppressing unhandled promise rejections, log the error instead of silently ignoring it to facilitate debugging.
  2. In TypeScript, ensure functions and mock implementations have explicit return types to maintain type safety and readability.


for (const p of promises) {
try {
const events = await p;
const events = await Promise.race([p, failFast]);
Comment on lines +689 to +702
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot May 22, 2026

Choose a reason for hiding this comment

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

P1 failFast が「中断予定のイテレーション」でも早期エラーを発火させてしまう

failFast は全 3 ページのいずれかが RateLimitError / UserNotFoundError を投げた時点で即座に reject します。しかし Promise.race([p, failFast]) はループの 第 1 イテレーション（page 1 の待機中） でも使われるため、たとえば「page 1 が 50 件を返してくれれば break できる」状況であっても、page 3 が先に RateLimitError を返すと failFast が先に reject して throw に至ります。元のコードでは page 1 が解決した後に break が実行され成功していたケースです。PR の説明には「成功ケースのロジックや動作は変えない」と記載されていますが、このタイミング依存の競合状態により、元コードでデータを返せていたシナリオで例外をスローする可能性があります。

Prompt To Fix With AI 
This is a comment left during a code review.
Path: src/lib/github.ts
Line: 689-702

Comment:
**failFast が「中断予定のイテレーション」でも早期エラーを発火させてしまう**

`failFast` は全 3 ページのいずれかが `RateLimitError` / `UserNotFoundError` を投げた時点で即座に reject します。しかし `Promise.race([p, failFast])` はループの **第 1 イテレーション（page 1 の待機中）** でも使われるため、たとえば「page 1 が 50 件を返してくれれば `break` できる」状況であっても、page 3 が先に `RateLimitError` を返すと `failFast` が先に reject して `throw` に至ります。元のコードでは page 1 が解決した後に `break` が実行され成功していたケースです。PR の説明には「成功ケースのロジックや動作は変えない」と記載されていますが、このタイミング依存の競合状態により、元コードでデータを返せていたシナリオで例外をスローする可能性があります。

How can I resolve this? If you propose a fix, please make it concise.

allEvents.push(...events);
if (events.length < 100) break;
} catch (error) {
Expand Down
Loading