Skip to content

⚡ Optimize GitHub Activity Fetching with Fail-Fast Concurrency #364

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
+9 −1
Open

⚡ Optimize GitHub Activity Fetching with Fail-Fast Concurrency #364

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/lib/github.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -682,12 +682,20 @@ export const fetchActivity = cache(async function fetchActivity(
)
);

const criticalErrorPromise = new Promise<never>((_, reject) => {
promises.forEach((p) => p.catch((e) => {
if (e instanceof UserNotFoundError || e instanceof RateLimitError) {
reject(e);
}
}));
});
Comment on lines +685 to +691

@gemini-code-assist gemini-code-assist Bot Jun 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

If criticalErrorPromise rejects after the loop has already broken early (e.g., because page 1 had fewer than 100 events), the rejection of criticalErrorPromise will occur in the background. To prevent potential unhandled promise rejection warnings in environments with strict rejection tracking, attach a .catch() handler directly to criticalErrorPromise immediately after its creation. Ensure you log the error instead of silently ignoring it to facilitate debugging.

Suggested change
const criticalErrorPromise = new Promise<never>((_, reject) => {
promises.forEach((p) => p.catch((e) => {
if (e instanceof UserNotFoundError || e instanceof RateLimitError) {
reject(e);
}
}));
});
const criticalErrorPromise = new Promise<never>((_, reject) => {
promises.forEach((p) => p.catch((e) => {
if (e instanceof UserNotFoundError || e instanceof RateLimitError) {
reject(e);
}
}));
});
criticalErrorPromise.catch((err) => console.error(err));
References
  1. When suppressing unhandled promise rejections, log the error instead of silently ignoring it to facilitate debugging.


// 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)));
Comment on lines 693 to 694

@greptile-apps greptile-apps Bot Jun 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 UserNotFoundError / RateLimitError が不要にエラーログへ出力される問題です。criticalErrorPromise 内の .catch と、後続の logging forEach の .catch は同じ Promise に対して独立して両方とも発火します。そのため、UserNotFoundError や RateLimitError が発生すると、caller へ正しく再スローされながらも logger.error("Event fetch promise rejected:", ...) にもエラーとして記録され、「想定外のエラー」のように見える誤解を招くノイズになります。ロギング側でクリティカルエラーを除外するか、コメントの意図通り「後続ページの未処理リジェクション抑制」に絞った条件付きログにすることが望ましいです。

Suggested change
// 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)));
// Suppress unhandled promise rejections for subsequent pages if we break early or throw
// クリティカルエラーは呼び出し元へ再スローされるため、ログ出力から除外する
promises.forEach((p) =>
p.catch((e) => {
if (!(e instanceof UserNotFoundError || e instanceof RateLimitError)) {
logger.error("Event fetch promise rejected:", e);
}
})
);
Prompt To Fix With AI 
This is a comment left during a code review.
Path: src/lib/github.ts
Line: 693-694

Comment:
`UserNotFoundError` / `RateLimitError` が不要にエラーログへ出力される問題です。`criticalErrorPromise` 内の `.catch` と、後続の logging `forEach` の `.catch` は**同じ Promise に対して独立して両方とも発火**します。そのため、`UserNotFoundError` や `RateLimitError` が発生すると、caller へ正しく再スローされながらも `logger.error("Event fetch promise rejected:", ...)` にもエラーとして記録され、「想定外のエラー」のように見える誤解を招くノイズになります。ロギング側でクリティカルエラーを除外するか、コメントの意図通り「後続ページの未処理リジェクション抑制」に絞った条件付きログにすることが望ましいです。

```suggestion
  // Suppress unhandled promise rejections for subsequent pages if we break early or throw
  // クリティカルエラーは呼び出し元へ再スローされるため、ログ出力から除外する
  promises.forEach((p) =>
    p.catch((e) => {
      if (!(e instanceof UserNotFoundError || e instanceof RateLimitError)) {
        logger.error("Event fetch promise rejected:", e);
      }
    })
  );
```

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


for (const p of promises) {
try {
const events = await p;
const events = await Promise.race([p, criticalErrorPromise]);
allEvents.push(...events);
if (events.length < 100) break;
} catch (error) {
Expand Down
Loading