From 51603b2b1add6909d866d45a46243d97d1617dc1 Mon Sep 17 00:00:00 2001 From: ChiragArora31 Date: Wed, 29 Apr 2026 07:02:16 +0530 Subject: [PATCH 1/3] fix(scm-github): throttle transient etag warnings --- .../plugins/scm-github/src/graphql-batch.ts | 55 ++++++++++++++++++- .../scm-github/test/graphql-batch.test.ts | 47 ++++++++++++++++ 2 files changed, 100 insertions(+), 2 deletions(-) diff --git a/packages/plugins/scm-github/src/graphql-batch.ts b/packages/plugins/scm-github/src/graphql-batch.ts index 108d471683..ac61a2acd5 100644 --- a/packages/plugins/scm-github/src/graphql-batch.ts +++ b/packages/plugins/scm-github/src/graphql-batch.ts @@ -88,6 +88,8 @@ const etagCache: ETagCache = { reviewComments: new LRUCache(MAX_REVIEW_COMMENTS_ETAGS), }; +const transientNetworkWarningKeys = new Set(); + /** * Result of checking if PR data has changed via ETag guards. */ @@ -113,6 +115,7 @@ export function clearETagCache(): void { etagCache.prList.clear(); etagCache.commitStatus.clear(); etagCache.reviewComments.clear(); + transientNetworkWarningKeys.clear(); } /** @@ -392,6 +395,40 @@ function extractErrorOutput(err: unknown): string | null { return combined.length > 0 ? combined : null; } +function isTransientNetworkError(message: string): boolean { + const normalized = message.toLowerCase(); + return [ + "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", + ].some((pattern) => normalized.includes(pattern)); +} + +function warnETagGuardFailure( + observer: BatchObserver | undefined, + guard: string, + key: string, + message: string, +): void { + const warningKey = `${guard}:${key}`; + if (isTransientNetworkError(message)) { + 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 @@ -438,6 +475,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; } @@ -447,6 +485,7 @@ async function checkPRListETag( setPRListETag(owner, repo, newETag); } + markETagGuardSuccess("ETag Guard 1", repoKey); // PR list changed - cost: 1 REST point return true; } catch (err) { @@ -465,7 +504,12 @@ async function checkPRListETag( if (is304(errorMsg)) { 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 } } @@ -509,6 +553,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; } @@ -518,6 +563,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) { @@ -533,7 +579,12 @@ async function checkCommitStatusETag( if (is304(errorMsg)) { 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 } } diff --git a/packages/plugins/scm-github/test/graphql-batch.test.ts b/packages/plugins/scm-github/test/graphql-batch.test.ts index 2c987ea71c..431aa7ac51 100644 --- a/packages/plugins/scm-github/test/graphql-batch.test.ts +++ b/packages/plugins/scm-github/test/graphql-batch.test.ts @@ -963,6 +963,53 @@ 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); + }); }); describe("Guard 2: Commit Status ETag - Pending CI PRs", () => { From a39c5c6ed6a95a3e08a0beaecf9675ec391aec0d Mon Sep 17 00:00:00 2001 From: ChiragArora31 Date: Fri, 1 May 2026 19:11:45 +0530 Subject: [PATCH 2/3] fix(scm-github): close all ETag guard suppression gaps Clear suppression keys on catch-path 304 recovery for all guards, migrate Guard 3 to shared throttling, and expand transient network detection/tests so repeated warnings are deduped only for transient failures. Made-with: Cursor --- .../plugins/scm-github/src/graphql-batch.ts | 65 ++++++- .../scm-github/test/graphql-batch.test.ts | 178 ++++++++++++++++++ 2 files changed, 233 insertions(+), 10 deletions(-) diff --git a/packages/plugins/scm-github/src/graphql-batch.ts b/packages/plugins/scm-github/src/graphql-batch.ts index ac61a2acd5..c27a75699f 100644 --- a/packages/plugins/scm-github/src/graphql-batch.ts +++ b/packages/plugins/scm-github/src/graphql-batch.ts @@ -88,6 +88,23 @@ 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(); /** @@ -397,15 +414,32 @@ function extractErrorOutput(err: unknown): string | null { function isTransientNetworkError(message: string): boolean { const normalized = message.toLowerCase(); - return [ - "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", - ].some((pattern) => normalized.includes(pattern)); + 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( @@ -416,6 +450,7 @@ function warnETagGuardFailure( ): void { const warningKey = `${guard}:${key}`; if (isTransientNetworkError(message)) { + pruneTransientNetworkWarningKeys(); if (transientNetworkWarningKeys.has(warningKey)) { return; } @@ -494,6 +529,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; } @@ -572,6 +608,7 @@ 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; } @@ -626,6 +663,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; } @@ -634,12 +672,14 @@ 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; } @@ -647,7 +687,12 @@ export async function checkReviewCommentsETag( if (is304(errorMsg)) { 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 431aa7ac51..b199933a80 100644 --- a/packages/plugins/scm-github/test/graphql-batch.test.ts +++ b/packages/plugins/scm-github/test/graphql-batch.test.ts @@ -28,6 +28,7 @@ import { shouldRefreshPREnrichment, checkReviewCommentsETag, setExecFileAsync, + checkReviewCommentsETag, } from "../src/graphql-batch.js"; // Mock execFile using the injection function @@ -1010,6 +1011,75 @@ describe("shouldRefreshPREnrichment - ETag Guard Strategy", () => { 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", () => { @@ -1205,6 +1275,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", () => { @@ -1391,6 +1513,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 = { From 59e456ebb5009bb7c3c0c4d64fe92a50560282a9 Mon Sep 17 00:00:00 2001 From: ChiragArora31 Date: Sun, 17 May 2026 05:16:16 +0530 Subject: [PATCH 3/3] fix(scm-github): clear etag warnings on message-only 304s Co-authored-by: Cursor --- packages/plugins/scm-github/src/graphql-batch.ts | 3 +++ packages/plugins/scm-github/test/graphql-batch.test.ts | 1 - 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/plugins/scm-github/src/graphql-batch.ts b/packages/plugins/scm-github/src/graphql-batch.ts index c27a75699f..57299f2f38 100644 --- a/packages/plugins/scm-github/src/graphql-batch.ts +++ b/packages/plugins/scm-github/src/graphql-batch.ts @@ -538,6 +538,7 @@ 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; } warnETagGuardFailure( @@ -614,6 +615,7 @@ async function checkCommitStatusETag( const errorMsg = err instanceof Error ? err.message : String(err); if (is304(errorMsg)) { + markETagGuardSuccess("ETag Guard 2", commitKey); return false; } warnETagGuardFailure( @@ -685,6 +687,7 @@ export async function checkReviewCommentsETag( const errorMsg = err instanceof Error ? err.message : String(err); if (is304(errorMsg)) { + markETagGuardSuccess("ETag Guard 3", cacheKey); return false; } warnETagGuardFailure( diff --git a/packages/plugins/scm-github/test/graphql-batch.test.ts b/packages/plugins/scm-github/test/graphql-batch.test.ts index b199933a80..3f94ec060a 100644 --- a/packages/plugins/scm-github/test/graphql-batch.test.ts +++ b/packages/plugins/scm-github/test/graphql-batch.test.ts @@ -28,7 +28,6 @@ import { shouldRefreshPREnrichment, checkReviewCommentsETag, setExecFileAsync, - checkReviewCommentsETag, } from "../src/graphql-batch.js"; // Mock execFile using the injection function