diff --git a/packages/plugins/scm-github/src/graphql-batch.ts b/packages/plugins/scm-github/src/graphql-batch.ts index cf92add294..3653d8f806 100644 --- a/packages/plugins/scm-github/src/graphql-batch.ts +++ b/packages/plugins/scm-github/src/graphql-batch.ts @@ -89,6 +89,25 @@ const etagCache: ETagCache = { reviewComments: new LRUCache(MAX_REVIEW_COMMENTS_ETAGS), }; +const TRANSIENT_NETWORK_PATTERNS = [ + "error connecting to api.github.com", + "check your internet connection", + "could not resolve host", + "failed to connect to api.github.com", + "network is unreachable", + "connection timed out", + "connection reset", + "ssl", + "tls", + "certificate", + "dial tcp", +] as const; + +const MAX_TRANSIENT_WARNING_KEYS = + (MAX_PR_LIST_ETAGS + MAX_COMMIT_STATUS_ETAGS + MAX_REVIEW_COMMENTS_ETAGS) * 2; + +const transientNetworkWarningKeys = new Set(); + /** * Result of checking if PR data has changed via ETag guards. */ @@ -114,6 +133,7 @@ export function clearETagCache(): void { etagCache.prList.clear(); etagCache.commitStatus.clear(); etagCache.reviewComments.clear(); + transientNetworkWarningKeys.clear(); } /** @@ -411,6 +431,58 @@ function extractErrorOutput(err: unknown): string | null { return combined.length > 0 ? combined : null; } +function isTransientNetworkError(message: string): boolean { + const normalized = message.toLowerCase(); + return TRANSIENT_NETWORK_PATTERNS.some((pattern) => normalized.includes(pattern)); +} + +function pruneTransientNetworkWarningKeys(): void { + if (transientNetworkWarningKeys.size < MAX_TRANSIENT_WARNING_KEYS) { + return; + } + + const activeWarningKeys = new Set(); + for (const repoKey of etagCache.prList.keys()) activeWarningKeys.add(`ETag Guard 1:${repoKey}`); + for (const commitKey of etagCache.commitStatus.keys()) + activeWarningKeys.add(`ETag Guard 2:${commitKey}`); + for (const reviewKey of etagCache.reviewComments.keys()) + activeWarningKeys.add(`ETag Guard 3:${reviewKey}`); + + for (const key of transientNetworkWarningKeys) { + if (!activeWarningKeys.has(key)) { + transientNetworkWarningKeys.delete(key); + } + } + + while (transientNetworkWarningKeys.size >= MAX_TRANSIENT_WARNING_KEYS) { + const oldestKey = transientNetworkWarningKeys.values().next().value; + if (!oldestKey) break; + transientNetworkWarningKeys.delete(oldestKey); + } +} + +function warnETagGuardFailure( + observer: BatchObserver | undefined, + guard: string, + key: string, + message: string, +): void { + const warningKey = `${guard}:${key}`; + if (isTransientNetworkError(message)) { + pruneTransientNetworkWarningKeys(); + if (transientNetworkWarningKeys.has(warningKey)) { + return; + } + transientNetworkWarningKeys.add(warningKey); + } + + observer?.log("warn", `[${guard}] ${message}`); +} + +function markETagGuardSuccess(guard: string, key: string): void { + transientNetworkWarningKeys.delete(`${guard}:${key}`); +} + /** * Extract ETag from HTTP response output. * Used on both 200 and 304 paths — RFC 7232 allows servers to rotate @@ -457,6 +529,7 @@ async function checkPRListETag( // Re-read ETag on 304 — RFC 7232 allows rotated validators const rotatedETag = extractETag(output); if (rotatedETag) setPRListETag(owner, repo, rotatedETag); + markETagGuardSuccess("ETag Guard 1", repoKey); return false; } @@ -466,6 +539,7 @@ async function checkPRListETag( setPRListETag(owner, repo, newETag); } + markETagGuardSuccess("ETag Guard 1", repoKey); // PR list changed - cost: 1 REST point return true; } catch (err) { @@ -474,6 +548,7 @@ async function checkPRListETag( if (output && is304(output)) { const rotatedETag = extractETag(output); if (rotatedETag) setPRListETag(owner, repo, rotatedETag); + markETagGuardSuccess("ETag Guard 1", repoKey); return false; } @@ -482,9 +557,15 @@ async function checkPRListETag( // that don't populate stdout on non-zero exit). Use is304() anchored to the HTTP status // line to avoid false positives from URL paths like "pulls/304/comments". if (is304(errorMsg)) { + markETagGuardSuccess("ETag Guard 1", repoKey); return false; } - observer?.log("warn", `[ETag Guard 1] PR list check failed for ${repoKey}: ${errorMsg}`); + warnETagGuardFailure( + observer, + "ETag Guard 1", + repoKey, + `PR list check failed for ${repoKey}: ${errorMsg}`, + ); return true; // Assume changed to be safe } } @@ -528,6 +609,7 @@ async function checkCommitStatusETag( if (is304(output)) { const rotatedETag = extractETag(output); if (rotatedETag) setCommitStatusETag(owner, repo, sha, rotatedETag); + markETagGuardSuccess("ETag Guard 2", commitKey); return false; } @@ -537,6 +619,7 @@ async function checkCommitStatusETag( setCommitStatusETag(owner, repo, sha, newETag); } + markETagGuardSuccess("ETag Guard 2", commitKey); // CI status changed - cost: 1 REST point return true; } catch (err) { @@ -545,16 +628,20 @@ async function checkCommitStatusETag( if (output && is304(output)) { const rotatedETag = extractETag(output); if (rotatedETag) setCommitStatusETag(owner, repo, sha, rotatedETag); + markETagGuardSuccess("ETag Guard 2", commitKey); return false; } const errorMsg = err instanceof Error ? err.message : String(err); if (is304(errorMsg)) { + markETagGuardSuccess("ETag Guard 2", commitKey); return false; } - observer?.log( - "warn", - `[ETag Guard 2] Commit status check failed for ${commitKey}: ${errorMsg}`, + warnETagGuardFailure( + observer, + "ETag Guard 2", + commitKey, + `Commit status check failed for ${commitKey}: ${errorMsg}`, ); return true; // Assume changed to be safe } @@ -597,6 +684,7 @@ export async function checkReviewCommentsETag( if (is304(output)) { const rotatedETag = extractETag(output); if (rotatedETag) etagCache.reviewComments.set(cacheKey, rotatedETag); + markETagGuardSuccess("ETag Guard 3", cacheKey); return false; } @@ -605,22 +693,27 @@ export async function checkReviewCommentsETag( etagCache.reviewComments.set(cacheKey, newETag); } + markETagGuardSuccess("ETag Guard 3", cacheKey); return true; } catch (err) { const output = extractErrorOutput(err); if (output && is304(output)) { const rotatedETag = extractETag(output); if (rotatedETag) etagCache.reviewComments.set(cacheKey, rotatedETag); + markETagGuardSuccess("ETag Guard 3", cacheKey); return false; } const errorMsg = err instanceof Error ? err.message : String(err); if (is304(errorMsg)) { + markETagGuardSuccess("ETag Guard 3", cacheKey); return false; } - observer?.log( - "warn", - `[ETag Guard 3] Review comments check failed for ${cacheKey}: ${errorMsg}`, + warnETagGuardFailure( + observer, + "ETag Guard 3", + cacheKey, + `Review comments check failed for ${cacheKey}: ${errorMsg}`, ); return true; // Assume changed to be safe } diff --git a/packages/plugins/scm-github/test/graphql-batch.test.ts b/packages/plugins/scm-github/test/graphql-batch.test.ts index 2c987ea71c..3f94ec060a 100644 --- a/packages/plugins/scm-github/test/graphql-batch.test.ts +++ b/packages/plugins/scm-github/test/graphql-batch.test.ts @@ -963,6 +963,122 @@ describe("shouldRefreshPREnrichment - ETag Guard Strategy", () => { expect(result.shouldRefresh).toBe(true); // Not a cache hit — real error expect(mockObserver.log).toHaveBeenCalledWith("warn", expect.stringContaining("[ETag Guard 1]")); }); + + it("should suppress repeated transient network warnings until connectivity returns", async () => { + const prs = [ + { + owner: "owner", + repo: "repo", + number: 123, + url: "https://github.com/owner/repo/pull/123", + title: "Test PR", + branch: "feature", + baseBranch: "main", + isDraft: false, + }, + ]; + + const mockObserver = { + log: vi.fn(), + recordSuccess: vi.fn(), + recordError: vi.fn(), + }; + const networkError = new Error( + "Command failed: gh api\nerror connecting to api.github.com\ncheck your internet connection", + ); + + mockExecFileImpl.mockRejectedValueOnce(networkError); + await expect(shouldRefreshPREnrichment(prs, [], mockObserver)).resolves.toMatchObject({ + shouldRefresh: true, + }); + + mockExecFileImpl.mockRejectedValueOnce(networkError); + await expect(shouldRefreshPREnrichment(prs, [], mockObserver)).resolves.toMatchObject({ + shouldRefresh: true, + }); + + expect(mockObserver.log).toHaveBeenCalledTimes(1); + + mockExecFileImpl.mockResolvedValueOnce({ + stdout: 'HTTP/2 200\neTag: "abc123"', + stderr: "", + }); + await shouldRefreshPREnrichment(prs, [], mockObserver); + + mockExecFileImpl.mockRejectedValueOnce(networkError); + await shouldRefreshPREnrichment(prs, [], mockObserver); + + expect(mockObserver.log).toHaveBeenCalledTimes(2); + }); + + it("should clear Guard 1 suppression after catch-path 304 recovery", async () => { + const prs = [ + { + owner: "owner", + repo: "repo", + number: 123, + url: "https://github.com/owner/repo/pull/123", + title: "Test PR", + branch: "feature", + baseBranch: "main", + isDraft: false, + }, + ]; + + const mockObserver = { + recordSuccess: vi.fn(), + recordFailure: vi.fn(), + log: vi.fn(), + }; + const networkError = new Error("error connecting to api.github.com"); + + mockExecFileImpl.mockRejectedValueOnce(networkError); + await shouldRefreshPREnrichment(prs, [], mockObserver); + mockExecFileImpl.mockRejectedValueOnce(networkError); + await shouldRefreshPREnrichment(prs, [], mockObserver); + expect(mockObserver.log).toHaveBeenCalledTimes(1); + + const notModifiedError = new Error("gh exits with code 1 on 304"); + (notModifiedError as Error & { stdout?: string }).stdout = 'HTTP/2 304\netag: "rotated"'; + mockExecFileImpl.mockRejectedValueOnce(notModifiedError); + await expect(shouldRefreshPREnrichment(prs, [], mockObserver)).resolves.toMatchObject({ + shouldRefresh: false, + }); + + mockExecFileImpl.mockRejectedValueOnce(networkError); + await shouldRefreshPREnrichment(prs, [], mockObserver); + + expect(mockObserver.log).toHaveBeenCalledTimes(2); + }); + + it("does not suppress repeated non-transient Guard 1 warnings", async () => { + const prs = [ + { + owner: "owner", + repo: "repo", + number: 123, + url: "https://github.com/owner/repo/pull/123", + title: "Test PR", + branch: "feature", + baseBranch: "main", + isDraft: false, + }, + ]; + + const mockObserver = { + recordSuccess: vi.fn(), + recordFailure: vi.fn(), + log: vi.fn(), + }; + const nonTransientError = new Error("GraphQL query parse failed"); + + mockExecFileImpl.mockRejectedValueOnce(nonTransientError); + await shouldRefreshPREnrichment(prs, [], mockObserver); + mockExecFileImpl.mockRejectedValueOnce(nonTransientError); + await shouldRefreshPREnrichment(prs, [], mockObserver); + + expect(mockObserver.log).toHaveBeenCalledTimes(2); + }); }); describe("Guard 2: Commit Status ETag - Pending CI PRs", () => { @@ -1158,6 +1274,58 @@ describe("shouldRefreshPREnrichment - ETag Guard Strategy", () => { expect(result.shouldRefresh).toBe(false); }); + + it("should clear Guard 2 suppression after catch-path 304 recovery", async () => { + setPRMetadata("owner/repo#123", { headSha: "abc123", ciStatus: "pending" }); + setPRListETag("owner", "repo", "etag-pr-list"); + + const prs = [ + { + owner: "owner", + repo: "repo", + number: 123, + url: "https://github.com/owner/repo/pull/123", + title: "Test PR", + branch: "feature", + baseBranch: "main", + isDraft: false, + }, + ]; + + const mockObserver = { + recordSuccess: vi.fn(), + recordFailure: vi.fn(), + log: vi.fn(), + }; + const networkError = new Error("failed to connect to api.github.com"); + + mockExecFileImpl + .mockResolvedValueOnce({ stdout: 'HTTP/2 304', stderr: "" }) + .mockRejectedValueOnce(networkError); + await shouldRefreshPREnrichment(prs, [], mockObserver); + + mockExecFileImpl + .mockResolvedValueOnce({ stdout: 'HTTP/2 304', stderr: "" }) + .mockRejectedValueOnce(networkError); + await shouldRefreshPREnrichment(prs, [], mockObserver); + expect(mockObserver.log).toHaveBeenCalledTimes(1); + + const notModifiedError = new Error("gh exits with code 1 on 304"); + (notModifiedError as Error & { stdout?: string }).stdout = 'HTTP/2 304\netag: "rotated-2"'; + mockExecFileImpl + .mockResolvedValueOnce({ stdout: 'HTTP/2 304', stderr: "" }) + .mockRejectedValueOnce(notModifiedError); + await expect(shouldRefreshPREnrichment(prs, [], mockObserver)).resolves.toMatchObject({ + shouldRefresh: false, + }); + + mockExecFileImpl + .mockResolvedValueOnce({ stdout: 'HTTP/2 304', stderr: "" }) + .mockRejectedValueOnce(networkError); + await shouldRefreshPREnrichment(prs, [], mockObserver); + + expect(mockObserver.log).toHaveBeenCalledTimes(2); + }); }); describe("Multiple Repositories", () => { @@ -1344,6 +1512,62 @@ describe("shouldRefreshPREnrichment - ETag Guard Strategy", () => { }); }); +describe("checkReviewCommentsETag - Guard 3 throttling", () => { + beforeEach(() => { + clearETagCache(); + mockExecFileImpl.mockClear(); + }); + + it("suppresses repeated transient Guard 3 warnings and resets on success", async () => { + const mockObserver = { + recordSuccess: vi.fn(), + recordFailure: vi.fn(), + log: vi.fn(), + }; + const networkError = new Error("dial tcp 140.82.112.6:443: connect: network is unreachable"); + + mockExecFileImpl.mockRejectedValueOnce(networkError); + await expect(checkReviewCommentsETag("owner", "repo", 12, mockObserver)).resolves.toBe(true); + mockExecFileImpl.mockRejectedValueOnce(networkError); + await expect(checkReviewCommentsETag("owner", "repo", 12, mockObserver)).resolves.toBe(true); + expect(mockObserver.log).toHaveBeenCalledTimes(1); + + mockExecFileImpl.mockResolvedValueOnce({ + stdout: 'HTTP/2 200\netag: "review-etag"', + stderr: "", + }); + await expect(checkReviewCommentsETag("owner", "repo", 12, mockObserver)).resolves.toBe(true); + + mockExecFileImpl.mockRejectedValueOnce(networkError); + await expect(checkReviewCommentsETag("owner", "repo", 12, mockObserver)).resolves.toBe(true); + expect(mockObserver.log).toHaveBeenCalledTimes(2); + }); + + it("clears Guard 3 suppression after catch-path 304 recovery", async () => { + const mockObserver = { + recordSuccess: vi.fn(), + recordFailure: vi.fn(), + log: vi.fn(), + }; + const networkError = new Error("tls handshake timeout"); + + mockExecFileImpl.mockRejectedValueOnce(networkError); + await checkReviewCommentsETag("owner", "repo", 34, mockObserver); + mockExecFileImpl.mockRejectedValueOnce(networkError); + await checkReviewCommentsETag("owner", "repo", 34, mockObserver); + expect(mockObserver.log).toHaveBeenCalledTimes(1); + + const notModifiedError = new Error("gh exits with code 1 on 304"); + (notModifiedError as Error & { stdout?: string }).stdout = 'HTTP/2 304\netag: "review-rotated"'; + mockExecFileImpl.mockRejectedValueOnce(notModifiedError); + await expect(checkReviewCommentsETag("owner", "repo", 34, mockObserver)).resolves.toBe(false); + + mockExecFileImpl.mockRejectedValueOnce(networkError); + await checkReviewCommentsETag("owner", "repo", 34, mockObserver); + expect(mockObserver.log).toHaveBeenCalledTimes(2); + }); +}); + describe("extractPREnrichment ciChecks", () => { it("parses CheckRun contexts into ciChecks", () => { const pullRequest = {