fix(github): stop PR-status requests from starving startup connection pool
Watching N worktrees fired N PR-status requests at once (startWatching called refresh() directly, bypassing the batch limiter). Each request can take 20s+ under GitHub secondary-rate-limiting, and N of them saturate the browser's ~6 HTTP/1.1 connections per origin, starving the critical path (bootstrap session.status, diffs, sending messages) until they finish — the UI appeared frozen for ~20s on startup. - Gate all PR-status network calls through a global concurrency semaphore (max 2), so free sockets always remain for critical traffic. - Bound resolveGitHubPrStatus with a 12s timeout so a slow request fails fast instead of holding a socket; the client keeps its last-known status. - Reuse already-fetched repo metadata for the default branch instead of a redundant repos.get, reducing serial GitHub calls (less rate-limiting).
This commit is contained in:
@@ -93,6 +93,43 @@ const timers = new Map<string, number>();
|
|||||||
const bootstrapTimers = new Map<string, number[]>();
|
const bootstrapTimers = new Map<string, number[]>();
|
||||||
const inFlightBySignature = new Set<string>();
|
const inFlightBySignature = new Set<string>();
|
||||||
const lastRefreshBySignature = new Map<string, number>();
|
const lastRefreshBySignature = new Map<string, number>();
|
||||||
|
|
||||||
|
// Global concurrency gate for PR-status network requests.
|
||||||
|
//
|
||||||
|
// PR status is non-critical chrome, but each request can be slow (the server
|
||||||
|
// makes many serial GitHub API calls and GitHub secondary-rate-limits bursts,
|
||||||
|
// so a single request can take 20s+). The browser allows only ~6 concurrent
|
||||||
|
// HTTP/1.1 connections per origin. Without this cap, watching N worktrees fires
|
||||||
|
// N PR-status requests at once (each startWatching() calls refresh() directly,
|
||||||
|
// bypassing refreshTargets' batch limiter), which saturates the connection pool
|
||||||
|
// and starves the critical path (bootstrap session.status, diffs, sending
|
||||||
|
// messages) for the full duration — the whole UI appears frozen on startup.
|
||||||
|
//
|
||||||
|
// Capping concurrency low guarantees free sockets remain for critical traffic.
|
||||||
|
const PR_STATUS_NETWORK_CONCURRENCY = 2;
|
||||||
|
let prStatusNetworkActive = 0;
|
||||||
|
const prStatusNetworkWaiters: Array<() => void> = [];
|
||||||
|
|
||||||
|
const acquirePrStatusNetworkSlot = (): Promise<void> => {
|
||||||
|
if (prStatusNetworkActive < PR_STATUS_NETWORK_CONCURRENCY) {
|
||||||
|
prStatusNetworkActive += 1;
|
||||||
|
return Promise.resolve();
|
||||||
|
}
|
||||||
|
return new Promise<void>((resolve) => {
|
||||||
|
prStatusNetworkWaiters.push(resolve);
|
||||||
|
});
|
||||||
|
};
|
||||||
|
|
||||||
|
const releasePrStatusNetworkSlot = (): void => {
|
||||||
|
const next = prStatusNetworkWaiters.shift();
|
||||||
|
if (next) {
|
||||||
|
// Hand the slot directly to the next waiter — keep the active count steady.
|
||||||
|
next();
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
prStatusNetworkActive = Math.max(0, prStatusNetworkActive - 1);
|
||||||
|
};
|
||||||
|
|
||||||
const createEntry = (): PrStatusEntry => ({
|
const createEntry = (): PrStatusEntry => ({
|
||||||
status: null,
|
status: null,
|
||||||
isLoading: false,
|
isLoading: false,
|
||||||
@@ -488,7 +525,13 @@ export const useGitHubPrStatusStore = create<GitHubPrStatusStore>()(
|
|||||||
activeRequestCount: prev.activeRequestCount + 1,
|
activeRequestCount: prev.activeRequestCount + 1,
|
||||||
totalRequestCount: prev.totalRequestCount + 1,
|
totalRequestCount: prev.totalRequestCount + 1,
|
||||||
}));
|
}));
|
||||||
const next = await params.github.prStatus(params.directory, params.branch, params.remoteName ?? undefined, { force: options?.force });
|
await acquirePrStatusNetworkSlot();
|
||||||
|
let next: GitHubPullRequestStatus;
|
||||||
|
try {
|
||||||
|
next = await params.github.prStatus(params.directory, params.branch, params.remoteName ?? undefined, { force: options?.force });
|
||||||
|
} finally {
|
||||||
|
releasePrStatusNetworkSlot();
|
||||||
|
}
|
||||||
set((prev) => {
|
set((prev) => {
|
||||||
const nextEntries = { ...prev.entries };
|
const nextEntries = { ...prev.entries };
|
||||||
signatureKeys.forEach((signatureKey) => {
|
signatureKeys.forEach((signatureKey) => {
|
||||||
|
|||||||
@@ -160,6 +160,17 @@ const getRepoDefaultBranch = async (octokit, repo) => {
|
|||||||
return cached.defaultBranch;
|
return cached.defaultBranch;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Reuse the full repo metadata if it was already fetched (expandRepoNetwork
|
||||||
|
// calls getRepoMetadata for every candidate before the default-branch loop).
|
||||||
|
// This avoids a redundant repos.get per repo — fewer serial GitHub calls means
|
||||||
|
// less exposure to secondary-rate-limiting that makes PR status slow.
|
||||||
|
const metaCached = repoMetadataCache.get(repoKey);
|
||||||
|
if (metaCached && Date.now() - metaCached.fetchedAt < REPO_DEFAULT_BRANCH_TTL_MS) {
|
||||||
|
const defaultBranch = normalizeText(metaCached.data?.default_branch) || null;
|
||||||
|
defaultBranchCache.set(repoKey, { defaultBranch, fetchedAt: Date.now() });
|
||||||
|
return defaultBranch;
|
||||||
|
}
|
||||||
|
|
||||||
try {
|
try {
|
||||||
const response = await octokit.rest.repos.get({
|
const response = await octokit.rest.repos.get({
|
||||||
owner: repo.owner,
|
owner: repo.owner,
|
||||||
|
|||||||
@@ -1,7 +1,26 @@
|
|||||||
const PR_STATUS_CACHE_TTL_MS = 90_000;
|
const PR_STATUS_CACHE_TTL_MS = 90_000;
|
||||||
const PR_STATUS_CACHE_MAX_ENTRIES = 200;
|
const PR_STATUS_CACHE_MAX_ENTRIES = 200;
|
||||||
|
// Upper bound for resolving a single PR status. resolveGitHubPrStatus makes many
|
||||||
|
// serial GitHub API calls; under GitHub secondary-rate-limiting a single request
|
||||||
|
// can otherwise hang 20s+. We bound it so the route fails fast instead of holding
|
||||||
|
// the response (and a client socket) open — the client keeps its last-known
|
||||||
|
// status on error, and a later poll fills it in.
|
||||||
|
const PR_STATUS_RESOLVE_TIMEOUT_MS = 12_000;
|
||||||
const prStatusCache = new Map();
|
const prStatusCache = new Map();
|
||||||
|
|
||||||
|
function withTimeout(promise, timeoutMs, label) {
|
||||||
|
let timer;
|
||||||
|
const timeout = new Promise((_resolve, reject) => {
|
||||||
|
timer = setTimeout(() => {
|
||||||
|
const error = new Error(`${label} timed out after ${timeoutMs}ms`);
|
||||||
|
error.code = 'ETIMEDOUT';
|
||||||
|
reject(error);
|
||||||
|
}, timeoutMs);
|
||||||
|
if (typeof timer.unref === 'function') timer.unref();
|
||||||
|
});
|
||||||
|
return Promise.race([promise, timeout]).finally(() => clearTimeout(timer));
|
||||||
|
}
|
||||||
|
|
||||||
function getRequestedRepo(req) {
|
function getRequestedRepo(req) {
|
||||||
const owner = typeof req.query?.owner === 'string' ? req.query.owner.trim() : '';
|
const owner = typeof req.query?.owner === 'string' ? req.query.owner.trim() : '';
|
||||||
const repo = typeof req.query?.repo === 'string' ? req.query.repo.trim() : '';
|
const repo = typeof req.query?.repo === 'string' ? req.query.repo.trim() : '';
|
||||||
@@ -417,12 +436,16 @@ export function registerGitHubRoutes(app) {
|
|||||||
}
|
}
|
||||||
|
|
||||||
const { resolveGitHubPrStatus } = await import('./pr-status.js');
|
const { resolveGitHubPrStatus } = await import('./pr-status.js');
|
||||||
const resolvedStatus = await resolveGitHubPrStatus({
|
const resolvedStatus = await withTimeout(
|
||||||
octokit,
|
resolveGitHubPrStatus({
|
||||||
directory,
|
octokit,
|
||||||
branch,
|
directory,
|
||||||
remoteName: remote,
|
branch,
|
||||||
});
|
remoteName: remote,
|
||||||
|
}),
|
||||||
|
PR_STATUS_RESOLVE_TIMEOUT_MS,
|
||||||
|
'resolveGitHubPrStatus',
|
||||||
|
);
|
||||||
const searchRepo = resolvedStatus.repo;
|
const searchRepo = resolvedStatus.repo;
|
||||||
const first = resolvedStatus.pr;
|
const first = resolvedStatus.pr;
|
||||||
if (!searchRepo) {
|
if (!searchRepo) {
|
||||||
|
|||||||
Reference in New Issue
Block a user