From 40efbddb17dae8847cbaa099cec27c404a36d995 Mon Sep 17 00:00:00 2001 From: Clayton Date: Fri, 24 Jul 2026 07:55:11 -0500 Subject: [PATCH] fix(queue): observe unresolved required-contexts lookups (#8358) --- src/queue/ci-resolution.ts | 25 ++++++++++ src/queue/processors.ts | 4 ++ test/unit/ci-resolution.test.ts | 83 ++++++++++++++++++++++++++++++++- test/unit/queue.test.ts | 7 ++- 4 files changed, 116 insertions(+), 3 deletions(-) diff --git a/src/queue/ci-resolution.ts b/src/queue/ci-resolution.ts index 1376c2b777..a4ee9d7653 100644 --- a/src/queue/ci-resolution.ts +++ b/src/queue/ci-resolution.ts @@ -31,6 +31,9 @@ import type { GitHubRateLimitAdmissionKey } from "../github/client"; import { incr } from "../selfhost/metrics"; import type { LiveGithubFacts, RequiredStatusContextsLookup } from "./processors"; +/** Emitted when a live gate evaluation falls back to config-only required contexts because the branch-protection read failed. */ +export const REQUIRED_CONTEXTS_UNRESOLVED_METRIC = "loopover_required_contexts_unresolved_total"; + export function liveFactKey(...parts: Array): string { return JSON.stringify(parts.map((part) => [typeof part, part])); } @@ -105,6 +108,28 @@ export function cachedRequiredStatusContexts( return next; } +/** + * #8358: surface a previously unused `resolved: false` on RequiredStatusContextsLookup. Callers (the live + * gate path in processors.ts) must invoke this after cachedRequiredStatusContexts so a degraded + * branch-protection read is observable via metric + structured warn — gate disposition is unchanged. + */ +export function observeRequiredContextsLookup( + lookup: RequiredStatusContextsLookup, + meta: { repoFullName: string; pullNumber: number; baseRef: string | null | undefined }, +): void { + if (lookup.resolved) return; + incr(REQUIRED_CONTEXTS_UNRESOLVED_METRIC); + console.warn( + JSON.stringify({ + level: "warn", + event: "required_contexts_branch_protection_unresolved", + repoFullName: meta.repoFullName, + pullNumber: meta.pullNumber, + baseRef: meta.baseRef ?? null, + }), + ); +} + function evictLiveFactOnReject( cache: Map>, key: string, diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 2fc2f399a7..ef553fdf2b 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -412,6 +412,7 @@ import { cachedRequiredStatusContexts, liveFactKey, liveFactTokenPart, + observeRequiredContextsLookup, refreshLiveCiAggregate, refreshLiveMergeState, reuseOrRefreshLiveCiAggregate, @@ -2877,6 +2878,9 @@ async function runAgentMaintenancePlanAndExecute( // is not re-reviewed for the same state. fetchLivePullRequestReviewDecision(env, repoFullName, pr.number, token, admissionKey), ]); + // #8358: consume `.resolved` — a branch-protection read failure still yields a config-fallback set, but + // without this signal the degraded path was silent. Observability only; disposition is unchanged. + observeRequiredContextsLookup(requiredContextsLookup, { repoFullName, pullNumber: pr.number, baseRef }); const requiredContexts = requiredContextsLookup.requiredContexts; // Same reuse-this-pass-else-refresh-live rationale as reuseOrRefreshLiveMergeState above (#4498). const ciAggregate = await reuseOrRefreshLiveCiAggregate( diff --git a/test/unit/ci-resolution.test.ts b/test/unit/ci-resolution.test.ts index b71ac07964..089798cfd6 100644 --- a/test/unit/ci-resolution.test.ts +++ b/test/unit/ci-resolution.test.ts @@ -1,7 +1,13 @@ -import { afterEach, describe, expect, it, vi } from "vitest"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import * as backfillModule from "../../src/github/backfill"; -import { cachedLiveCiAggregate } from "../../src/queue/ci-resolution"; +import { + cachedLiveCiAggregate, + cachedRequiredStatusContexts, + observeRequiredContextsLookup, + REQUIRED_CONTEXTS_UNRESOLVED_METRIC, +} from "../../src/queue/ci-resolution"; import type { LiveGithubFacts } from "../../src/queue/processors"; +import { counterValue, resetMetrics } from "../../src/selfhost/metrics"; import { createTestEnv } from "../helpers/d1"; function emptyFacts(): LiveGithubFacts { @@ -79,3 +85,76 @@ describe("cachedLiveCiAggregate request-scoped memoization (#4498)", () => { expect(liveCiSpy).toHaveBeenCalledTimes(3); // +1 only, the reversed list reused the key }); }); + +describe("cachedRequiredStatusContexts resolved flag (#8358)", () => { + afterEach(() => { + vi.restoreAllMocks(); + }); + + it("sets resolved:false when the live branch-protection read fails (onFetchFailure fires) and keeps the expectedCiContexts config fallback", async () => { + const env = createTestEnv(); + vi.spyOn(backfillModule, "fetchRequiredStatusContexts").mockImplementation( + async (_env, _repo, _base, _token, _admission, onFetchFailure) => { + onFetchFailure(new Error("branch protection forbidden")); + return null; + }, + ); + const lookup = await cachedRequiredStatusContexts(env, "owner/repo", emptyFacts(), "main", "tok", ["lint"]); + expect(lookup.resolved).toBe(false); + expect(lookup.requiredContexts).toEqual(new Set(["lint"])); + }); + + it("sets resolved:true when the live branch-protection read succeeds", async () => { + const env = createTestEnv(); + vi.spyOn(backfillModule, "fetchRequiredStatusContexts").mockResolvedValue(new Set(["ci"])); + const lookup = await cachedRequiredStatusContexts(env, "owner/repo", emptyFacts(), "main", "tok", null); + expect(lookup.resolved).toBe(true); + expect(lookup.requiredContexts).toEqual(new Set(["ci"])); + }); +}); + +describe("observeRequiredContextsLookup (#8358)", () => { + beforeEach(() => { + resetMetrics(); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + it("emits a structured warn and increments the unresolved metric when resolved is false", () => { + const warn = vi.spyOn(console, "warn").mockImplementation(() => undefined); + observeRequiredContextsLookup( + { requiredContexts: new Set(["lint"]), resolved: false }, + { repoFullName: "owner/repo", pullNumber: 9, baseRef: "main" }, + ); + expect(counterValue(REQUIRED_CONTEXTS_UNRESOLVED_METRIC)).toBe(1); + expect(warn).toHaveBeenCalledTimes(1); + expect(JSON.parse(String(warn.mock.calls[0]?.[0]))).toEqual({ + level: "warn", + event: "required_contexts_branch_protection_unresolved", + repoFullName: "owner/repo", + pullNumber: 9, + baseRef: "main", + }); + }); + + it("coalesces a nullish baseRef to null in the warn payload", () => { + const warn = vi.spyOn(console, "warn").mockImplementation(() => undefined); + observeRequiredContextsLookup( + { requiredContexts: null, resolved: false }, + { repoFullName: "owner/repo", pullNumber: 3, baseRef: undefined }, + ); + expect(JSON.parse(String(warn.mock.calls[0]?.[0]))).toMatchObject({ baseRef: null }); + }); + + it("is a no-op when resolved is true (no metric, no warn)", () => { + const warn = vi.spyOn(console, "warn").mockImplementation(() => undefined); + observeRequiredContextsLookup( + { requiredContexts: new Set(["ci"]), resolved: true }, + { repoFullName: "owner/repo", pullNumber: 9, baseRef: "main" }, + ); + expect(counterValue(REQUIRED_CONTEXTS_UNRESOLVED_METRIC)).toBe(0); + expect(warn).not.toHaveBeenCalled(); + }); +}); diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index 7d823bf2f9..b690967c4d 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -10,7 +10,8 @@ import * as repositoriesModule from "../../src/db/repositories"; import * as reviewEffortModule from "../../src/review/review-effort"; import * as repositorySettingsModule from "../../src/settings/repository-settings"; import * as sentryModule from "../../src/selfhost/sentry"; -import { renderMetrics, resetMetrics } from "../../src/selfhost/metrics"; +import { counterValue, renderMetrics, resetMetrics } from "../../src/selfhost/metrics"; +import { REQUIRED_CONTEXTS_UNRESOLVED_METRIC } from "../../src/queue/ci-resolution"; import { jobCoalesceKey } from "../../src/selfhost/queue-common"; import { listCollisionEdges, @@ -3241,6 +3242,7 @@ describe("queue processors", () => { // existed (see the sibling "#audit-rate-headroom: the per-PR re-review refreshes..." dedup test above). it("REGRESSION (#selfhost-ci-verification): expectedCiContexts in the cache key does not defeat within-job required-contexts memoization", async () => { const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + resetMetrics(); await upsertInstallation(env, { action: "created", installation: { id: 9001, account: { login: "owner", id: 1, type: "Organization" }, target_type: "Organization", repository_selection: "selected", permissions: { pull_requests: "write", checks: "write" }, events: [] } }); await upsertRepositoryFromGitHub(env, { name: "agent-repo", full_name: "owner/agent-repo", private: false, owner: { login: "owner" } }, 9001); await upsertRepositorySettings(env, { repoFullName: "owner/agent-repo", autonomy: { merge: "auto", update_branch: "auto" }, autoMaintain: { requireApprovals: 0, mergeMethod: "squash" }, gatePack: "oss-anti-slop" }); @@ -3271,6 +3273,9 @@ describe("queue processors", () => { // One fetch for the whole job despite three internal call sites sharing the config-aware cache key. expect(branchProtectionGets).toBe(1); + // #8358: the 403 branch-protection read sets resolved:false; runAgentMaintenancePlanAndExecute must + // observe that (metric + warn) rather than silently consuming only `.requiredContexts`. + expect(counterValue(REQUIRED_CONTEXTS_UNRESOLVED_METRIC)).toBeGreaterThanOrEqual(1); const deferred = await env.DB.prepare("select count(*) as n from audit_events where event_type = ?") .bind("github_app.review_deferred_ci_pending") .first<{ n: number }>();