diff --git a/migrations/0095_pull_request_detail_sync_pr_state.sql b/migrations/0095_pull_request_detail_sync_pr_state.sql new file mode 100644 index 0000000000..342aefe6e4 --- /dev/null +++ b/migrations/0095_pull_request_detail_sync_pr_state.sql @@ -0,0 +1,8 @@ +ALTER TABLE pull_request_detail_sync_state + ADD COLUMN pr_mergeable_state TEXT; + +ALTER TABLE pull_request_detail_sync_state + ADD COLUMN pr_state TEXT; + +ALTER TABLE pull_request_detail_sync_state + ADD COLUMN pr_state_fetched_at TEXT; diff --git a/src/db/repositories.ts b/src/db/repositories.ts index b67deb789e..e928883cb2 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -1138,8 +1138,10 @@ export async function getRepoQueueTrendSnapshot(env: Env, repoFullName: string): // drizzle's `onConflictDoUpdate` strips `undefined` entries from the generated SQL `SET` clause rather than // writing NULL. Every "running" pre-fetch stamp (backfill.ts) relies on this to touch only `status` without // clearing the PREVIOUS `headSha`/`*SyncedAt` row — including the repo+PR+headSha file cache -// (#audit-rate-headroom), which would silently stop hitting if a future edit here coalesced an omitted field to -// `null` (e.g. `headSha: state.headSha ?? null`). Pass `null` explicitly to actually clear a column. +// (#audit-rate-headroom) and the durable bare-PR-state cache (#2537), which would silently stop hitting if a +// future edit here coalesced an omitted field to `null` (e.g. `headSha: state.headSha ?? null`). Pass `null` +// explicitly to actually clear a column (this is exactly how webhook invalidation clears prMergeableState/prState +// below). export async function upsertPullRequestDetailSyncState(env: Env, state: PullRequestDetailSyncStateRecord): Promise { const db = getDb(env.DB); await db @@ -1156,6 +1158,9 @@ export async function upsertPullRequestDetailSyncState(env: Env, state: PullRequ checksSyncedAt: state.checksSyncedAt, lastSyncedAt: state.lastSyncedAt, errorSummary: state.errorSummary, + prMergeableState: state.prMergeableState, + prState: state.prState, + prStateFetchedAt: state.prStateFetchedAt, updatedAt: nowIso(), }) .onConflictDoUpdate({ @@ -1169,6 +1174,9 @@ export async function upsertPullRequestDetailSyncState(env: Env, state: PullRequ checksSyncedAt: state.checksSyncedAt, lastSyncedAt: state.lastSyncedAt, errorSummary: state.errorSummary, + prMergeableState: state.prMergeableState, + prState: state.prState, + prStateFetchedAt: state.prStateFetchedAt, updatedAt: nowIso(), }, }); @@ -4295,6 +4303,9 @@ function toPullRequestDetailSyncStateRecord(row: typeof pullRequestDetailSyncSta checksSyncedAt: row.checksSyncedAt, lastSyncedAt: row.lastSyncedAt, errorSummary: row.errorSummary, + prMergeableState: row.prMergeableState, + prState: row.prState, + prStateFetchedAt: row.prStateFetchedAt, updatedAt: row.updatedAt, }; } diff --git a/src/db/schema.ts b/src/db/schema.ts index 31f9f9d372..796e95ccd7 100644 --- a/src/db/schema.ts +++ b/src/db/schema.ts @@ -240,6 +240,14 @@ export const pullRequestDetailSyncState = sqliteTable( checksSyncedAt: text("checks_synced_at"), lastSyncedAt: text("last_synced_at"), errorSummary: text("error_summary"), + // Durable bare-PR-state cache (#2537): mirrors GET /pulls/{n}'s mutable state/mergeable_state, refreshed on + // synchronize/closed/reopened webhooks and read by the freshness-guard/readiness/dup-winner call sites that + // don't need a live-recompute guarantee. NEVER read by the act-boundary merge/close decision + // (planAgentMaintenanceActions / the unified-comment mirror) or by resolveOverrideHeadSha (gate-override) -- + // both intentionally force a live read immediately before acting. + prMergeableState: text("pr_mergeable_state"), + prState: text("pr_state"), + prStateFetchedAt: text("pr_state_fetched_at"), updatedAt: text("updated_at").notNull().$defaultFn(() => nowIso()), }, (table) => ({ diff --git a/src/github/backfill.ts b/src/github/backfill.ts index bcb20c62fa..2e7fe1ec4a 100644 --- a/src/github/backfill.ts +++ b/src/github/backfill.ts @@ -326,6 +326,12 @@ const PR_DETAIL_BATCH_SIZE: Record = { light: 12, full: 40 const MERGED_PR_FILE_HYDRATION_BATCH_SIZE: Record = { light: 10, full: 20, resume: 20 }; const PULL_REQUEST_FILES_FETCH_METRIC = "gittensory_github_pull_request_files_fetch_total"; type PullRequestFilesFetchCaller = "backfill_open_pr_details" | "backfill_merged_history" | "live_review"; +// #2537: durable-cache counter for the bare PR-state read, mirroring PULL_REQUEST_FILES_FETCH_METRIC's bounded- +// label style (no per-PR-number labels — cardinality-safe). +const PR_STATE_CACHE_METRIC = "gittensory_pr_state_cache_total"; +// Safety-net max age for a webhook-invalidated PR-state cache row (a dropped/missed webhook must not pin a stale +// value forever). Short enough that a missed synchronize/closed/reopened event self-heals within one sweep tick. +const PR_STATE_CACHE_MAX_AGE_MS = 5 * 60 * 1000; const CURRENT_OPEN_SCAN_MARKER = "gittensory-current-open-scan-v1"; const FRESH_TOTALS_SNAPSHOT_MS = 10 * 60 * 1000; const TOTALS_SNAPSHOT_LOOKBACK = 8; @@ -2761,6 +2767,183 @@ export async function fetchLivePullRequest( return result?.data ?? undefined; } +// #2537: durable, webhook-invalidated cache for the bare PR-state read (GET /pulls/{n}). Unlike the request-local +// LiveGithubFacts memo (queue/processors.ts), this survives ACROSS webhook deliveries / sweep ticks, cutting +// repeat /pulls/{n} calls for an unchanged PR at the freshness-guard/readiness/dup-winner call sites. NEVER used +// by the act-boundary merge/close decision (planAgentMaintenanceActions's liveMergeState read, or its +// unified-comment mirror) or by resolveOverrideHeadSha (gate-override, queue/processors.ts) -- those force- +// refetch by design (the #4220 fix; the gate-override race respectively) and must keep doing so; this cache +// exists purely for the OTHER, non-authoritative reads. +function isPrStateCacheFresh(fetchedAt: string | null | undefined): boolean { + if (!fetchedAt) return false; + const fetchedAtMs = Date.parse(fetchedAt); + if (!Number.isFinite(fetchedAtMs)) return false; + return Date.now() - fetchedAtMs < PR_STATE_CACHE_MAX_AGE_MS; +} + +/** Best-effort write-through for the PR-state cache fields. Always stamps prStateFetchedAt = now on a successful + * live read (even when the live value itself is undefined/null — a confirmed-empty read is still a fresh read, + * distinct from "never fetched"), so a run of undefined reads doesn't force every caller back to GitHub. Preserves + * the row's own `status` (defaulting to "never_synced" only when no row exists yet) — this write must NEVER force + * `status: "complete"`, since `status` is shared with the FILES-cache staleness machinery + * (backfillOpenPullRequestDetails / refreshPullRequestDetails treat `status !== "complete"` as "needs a files + * resync"); a PR-state-only write claiming `complete` would falsely mark a files sync that never happened. A + * write failure is swallowed (#2537 fail-open: the cache is an optimization, never a correctness dependency — + * every caller already tolerates a live-fetch fallback). */ +async function writeThroughPrStateCache( + env: Env, + repoFullName: string, + prNumber: number, + previousStatus: PullRequestDetailSyncStateRecord["status"] | undefined, + fields: { prMergeableState?: string | null; prState?: string | null; headSha?: string | null }, +): Promise { + incr(PR_STATE_CACHE_METRIC, { field: "write", result: "set" }); + await upsertPullRequestDetailSyncState(env, { + repoFullName, + pullNumber: prNumber, + status: previousStatus ?? "never_synced", + prStateFetchedAt: nowIso(), + ...fields, + }).catch(() => undefined); +} + +/** + * Shared live-fetch for the three cached PR-state readers below (#2537 review fix). A SINGLE `GET /pulls/{n}` + * already returns `mergeable_state`, `state`, AND `head.sha` together, so a cache miss on any ONE field now + * fetches and write-throughs ALL THREE at once under the one shared `prStateFetchedAt` stamp they share -- + * instead of writing only the field the caller happened to ask for. Without this, a fresh write for field A + * would make an UN-fetched field B look "fresh" to the NEXT reader (they share one timestamp), so that reader + * would silently return `undefined` for a field that was simply never populated, mistaking it for a + * confirmed-empty GitHub value. Reusing the full-payload fetch costs nothing extra: all three narrow fetchers + * (`fetchLivePullRequestMergeState` / `fetchLivePullRequestState` / `fetchLivePullRequestHeadSha`) already hit + * this exact same endpoint, just extracting one field each -- this only changes what the CACHED wrappers fetch + * internally; those narrow fetchers stay untouched for their other, uncached, act-boundary callers. + * Returns the full payload, or `undefined` on a failed fetch -- in which case the cache is left untouched + * entirely (a failed live read must not poison it with a false "confirmed fresh" stamp). + */ +async function fetchAndCachePrStateFields( + env: Env, + repoFullName: string, + prNumber: number, + token: string | undefined, + admissionKey: GitHubRateLimitAdmissionKey | undefined, + previousStatus: PullRequestDetailSyncStateRecord["status"] | undefined, +): Promise { + const live = await fetchLivePullRequest(env, repoFullName, prNumber, token, admissionKey); + if (!live) return undefined; + const liveHeadSha = live.head?.sha; + await writeThroughPrStateCache(env, repoFullName, prNumber, previousStatus, { + prMergeableState: live.mergeable_state ?? null, + prState: live.state ?? null, + // Omit (not null) when the live payload carries no head SHA -- mirrors primeDurablePrStateCache's own + // PARTIAL-UPDATE CONTRACT guard below: a PR-state write must never CLEAR the headSha the files cache + // (#audit-rate-headroom) relies on. + ...(liveHeadSha ? { headSha: liveHeadSha } : {}), + }); + return live; +} + +/** Prime the durable PR-state cache (#2537) from an ALREADY-FETCHED live payload (e.g. the sweep-resync's + * `fetchLivePullRequest` read), so OTHER readers (readiness, dup-winner) benefit from this already-paid-for + * fetch instead of re-fetching moments later. Best-effort, mirrors writeThroughPrStateCache's own "preserve + * prior status" contract. */ +export async function primeDurablePrStateCache( + env: Env, + repoFullName: string, + prNumber: number, + live: { mergeable_state?: string | null; state?: string | null; head?: { sha?: string | null } | null } | undefined, +): Promise { + if (!live) return; + const existing = await getPullRequestDetailSyncState(env, repoFullName, prNumber).catch(() => null); + const liveHeadSha = live.head?.sha; + await writeThroughPrStateCache(env, repoFullName, prNumber, existing?.status, { + prMergeableState: live.mergeable_state ?? null, + prState: live.state ?? null, + // Omit (not null) when the live payload carries no head SHA — a PR-state-only write must never CLEAR the + // headSha the files cache (#audit-rate-headroom) relies on (PARTIAL-UPDATE CONTRACT: omitted = unchanged). + ...(liveHeadSha ? { headSha: liveHeadSha } : {}), + }); +} + +/** Cached read of the PR's live mergeable_state, backed by pull_request_detail_sync_state (#2537). A fresh cache + * row (webhook-invalidated, capped at PR_STATE_CACHE_MAX_AGE_MS) is served without a GitHub call; otherwise + * fetches live via fetchAndCachePrStateFields (which write-throughs ALL THREE cached fields together, not just + * this one, since they share one fetchedAt stamp) and returns this field from that shared response. + * Fail-open throughout: any cache read/write hiccup falls back to / degrades to a live fetch, never blocks it. */ +export async function cachedFetchLivePullRequestMergeState( + env: Env, + repoFullName: string, + prNumber: number, + token: string | undefined, + admissionKey?: GitHubRateLimitAdmissionKey, +): Promise { + const cached = await getPullRequestDetailSyncState(env, repoFullName, prNumber).catch(() => null); + if (cached && isPrStateCacheFresh(cached.prStateFetchedAt)) { + incr(PR_STATE_CACHE_METRIC, { field: "mergeable_state", result: "hit" }); + return cached.prMergeableState ?? undefined; + } + incr(PR_STATE_CACHE_METRIC, { field: "mergeable_state", result: "miss" }); + const live = await fetchAndCachePrStateFields(env, repoFullName, prNumber, token, admissionKey, cached?.status); + return live?.mergeable_state ?? undefined; +} + +/** Cached read of the PR's live state (open/closed), backed by pull_request_detail_sync_state (#2537). Same + * freshness/fail-open/shared-fetch contract as cachedFetchLivePullRequestMergeState. */ +export async function cachedFetchLivePullRequestState( + env: Env, + repoFullName: string, + prNumber: number, + token: string | undefined, + admissionKey?: GitHubRateLimitAdmissionKey, +): Promise { + const cached = await getPullRequestDetailSyncState(env, repoFullName, prNumber).catch(() => null); + if (cached && isPrStateCacheFresh(cached.prStateFetchedAt)) { + incr(PR_STATE_CACHE_METRIC, { field: "state", result: "hit" }); + return cached.prState ?? undefined; + } + incr(PR_STATE_CACHE_METRIC, { field: "state", result: "miss" }); + const live = await fetchAndCachePrStateFields(env, repoFullName, prNumber, token, admissionKey, cached?.status); + return live?.state ?? undefined; +} + +/** Cached read of the PR's live head SHA, backed by pull_request_detail_sync_state (#2537). Reuses the EXISTING + * headSha column (written by the files-cache path too) as the cached value; a cache hit still respects the same + * PR_STATE_CACHE_MAX_AGE_MS freshness window as the other two fields (headSha alone predates this issue and + * carries no fetchedAt guarantee, so gate it on prStateFetchedAt like its siblings). NOT used by + * resolveOverrideHeadSha (gate-override) -- that call site is security-sensitive and intentionally stays on + * the raw live fetchLivePullRequestHeadSha instead (see queue/processors.ts). */ +export async function cachedFetchLivePullRequestHeadSha( + env: Env, + repoFullName: string, + prNumber: number, + token: string | undefined, + admissionKey?: GitHubRateLimitAdmissionKey, +): Promise { + const cached = await getPullRequestDetailSyncState(env, repoFullName, prNumber).catch(() => null); + if (cached && isPrStateCacheFresh(cached.prStateFetchedAt) && cached.headSha) { + incr(PR_STATE_CACHE_METRIC, { field: "head_sha", result: "hit" }); + return cached.headSha; + } + incr(PR_STATE_CACHE_METRIC, { field: "head_sha", result: "miss" }); + const live = await fetchAndCachePrStateFields(env, repoFullName, prNumber, token, admissionKey, cached?.status); + return live?.head?.sha ?? undefined; +} + +/** Invalidate the durable PR-state cache fields (#2537) — called on synchronize/closed/reopened. Explicit null + * (not omitted) so the PARTIAL-UPDATE CONTRACT actually clears the stale value rather than leaving it. Best- + * effort by design at the call site (never blocks webhook processing on a cache-invalidation write). */ +export async function invalidatePrStateCache(env: Env, repoFullName: string, pullNumber: number): Promise { + const existing = await getPullRequestDetailSyncState(env, repoFullName, pullNumber).catch(() => null); + await upsertPullRequestDetailSyncState(env, { + repoFullName, + pullNumber, + status: existing?.status ?? "never_synced", + prMergeableState: null, + prState: null, + prStateFetchedAt: null, + }); +} + /** Resolve the OPEN PRs associated with a commit SHA via the REST `GET /repos/{owner}/{repo}/commits/{sha}/pulls` * endpoint. This is the only PR↔commit resolution that works for FORK (cross-repo) PRs, whose CI-completion * webhooks (`check_suite`/`check_run`) carry an EMPTY `pull_requests[]`. Returns the de-duplicated open PR numbers. diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 93a0a88e5d..a3270c15f7 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -76,6 +76,7 @@ import { backfillOpenPullRequestDetails, backfillRegisteredRepositories, backfillRepositorySegment, + cachedFetchLivePullRequestMergeState, enqueueRepositoryOpenDataBackfill, fetchAndStorePullRequestFilesForReview, fetchLinkedIssueFacts, @@ -91,6 +92,8 @@ import { fetchLivePullRequestState, fetchOpenPullRequestNumbersForCommit, fetchRequiredStatusContexts, + invalidatePrStateCache, + primeDurablePrStateCache, refreshContributorActivity, refreshInstallationHealth, refreshPullRequestDetails, @@ -615,15 +618,24 @@ function cachedLiveMergeState( const key = liveFactKey(repoFullName, prNumber, liveFactTokenPart(token)); const cached = facts.mergeStates.get(key); if (cached) return cached; + // #2537: on a request-local miss, check the DURABLE cross-webhook cache before hitting GitHub — this is the + // readiness/freshness-guard path, not the act-boundary disposition (that's refreshLiveMergeState below, which + // NEVER routes through the durable cache). A durable hit is itself memoized request-locally for the rest of + // this pass via facts.mergeStates, same as a live fetch would be. const next = evictLiveFactOnReject( facts.mergeStates, key, - fetchLivePullRequestMergeState(env, repoFullName, prNumber, token, admissionKey), + cachedFetchLivePullRequestMergeState(env, repoFullName, prNumber, token, admissionKey), ); facts.mergeStates.set(key, next); return next; } +// #4220 contradiction: the stored pr.mergeableState lags GitHub's async recompute, so a base-conflicting PR could +// read clean here (safe to merge) while the disposition reads the live dirty and auto-CLOSES it. This ALWAYS +// force-refetches live from GitHub and MUST NEVER be routed through the durable pull_request_detail_sync_state +// cache added by #2537 — both act-boundary-adjacent callers (runAgentMaintenancePlanAndExecute's disposition +// input, and the unified-comment mirror) depend on this staying live and uncached. function refreshLiveMergeState( env: Env, repoFullName: string, @@ -2189,6 +2201,10 @@ async function reReviewStoredPullRequest( resyncAdmissionKey, ); primeLiveMergeState(liveFacts, repoFullName, prNumber, resyncToken, live?.mergeable_state); + // #2537: this resync ALREADY paid for a bare GET /pulls/{n} — persist it to the durable cross-webhook cache so + // the readiness/dup-winner readers below (and future webhook deliveries) don't re-fetch it. Best-effort, never + // blocks the sweep on a write hiccup. + await primeDurablePrStateCache(env, repoFullName, prNumber, live).catch(() => undefined); // Terminal early-exit (#1942): the PR is CLOSED/merged on GitHub even though the stored row still reads open — a // dropped `closed` webhook (relay down). Reconcile the stored row from the live payload and RETURN before the // expensive resync (files) + readiness + re-review reads. A stale sweep must never spend GitHub budget — or post @@ -4249,6 +4265,14 @@ async function processGitHubWebhook( repoFullName, payload.pull_request, ); + // #2537: the durable PR-state cache (mergeable_state/state) goes stale exactly when GitHub recomputes them — + // synchronize (new head → new mergeable_state recompute), closed (state flips), reopened (state flips back). + // Clear explicitly (null, not omitted — PARTIAL-UPDATE CONTRACT) so the next cached read is a forced live + // miss; other pull_request actions (labeled, edited, etc.) don't change these fields and are left untouched + // to avoid spurious cache churn / extra writes on high-frequency low-signal actions. + if (eventName === "pull_request" && (payload.action === "synchronize" || payload.action === "closed" || payload.action === "reopened")) { + await invalidatePrStateCache(env, repoFullName, pr.number).catch(() => undefined); + } // Reopen-prevention (#one-shot-reopen): a CONTRIBUTOR may not reopen a PR that gittensory or a maintainer // closed — closes are one-shot (resubmit, don't reopen). If a non-maintainer reopened a PR whose last close // was by the bot / repo owner / admin, re-close it and skip the re-review. Self-closes (the contributor @@ -5640,6 +5664,12 @@ export async function reconcileLiveDuplicateSiblings( const staleClosed = new Set(); await Promise.all( lowerOverlapping.map(async (sibling) => { + // #2537: deliberately NOT durable-cached (flagged by the gate's own review) -- despite recomputing every + // delivery, this reconcile feeds duplicate-winner selection, which can auto-CLOSE the CURRENT PR when + // duplicateWinnerEnabled. A cached "open" read up to PR_STATE_CACHE_MAX_AGE_MS stale after a missed + // `closed` webhook would keep an already-closed sibling eligible as the winner, wrongly closing this PR + // as the loser. That is the same class of irreversible-actuation risk the merge/close decision and + // gate-override guard against, so this stays on the raw live fetch like they do. const liveState = await fetchLivePullRequestState( env, repoFullName, @@ -7415,6 +7445,11 @@ async function recordGithubProductUsage( * THAT commit (the neutral check-run is per-commit by design). FAIL-OPEN: an unreadable live fetch returns the * cached head, so a transient GitHub hiccup never strands the override — it just targets the stored SHA as before. * Mirrors the rebase path's live re-fetch (prReadyForReview) and the dup-winner live reconcile. + * #2537: deliberately NOT routed through the durable head-SHA cache (cachedFetchLivePullRequestHeadSha, + * backfill.ts) -- this is the same class of security-sensitive, human-triggered re-check as the act-boundary + * merge/close decision, wanting the literal current commit rather than a value that can be up to + * PR_STATE_CACHE_MAX_AGE_MS stale. A commit landing inside that freshness window right after the override + * comment is exactly the race this function exists to close; a cache hit would silently reintroduce it. */ export async function resolveOverrideHeadSha( env: Env, diff --git a/src/types.ts b/src/types.ts index aa0fb8870c..3c830da2e8 100644 --- a/src/types.ts +++ b/src/types.ts @@ -874,6 +874,10 @@ export type PullRequestDetailSyncStateRecord = { checksSyncedAt?: string | null | undefined; lastSyncedAt?: string | null | undefined; errorSummary?: string | null | undefined; + // #2537: durable bare-PR-state cache fields (mergeable_state/state from GET /pulls/{n}). + prMergeableState?: string | null | undefined; + prState?: string | null | undefined; + prStateFetchedAt?: string | null | undefined; updatedAt?: string | null | undefined; }; diff --git a/test/unit/pr-detail-durable-cache.test.ts b/test/unit/pr-detail-durable-cache.test.ts new file mode 100644 index 0000000000..db475e773b --- /dev/null +++ b/test/unit/pr-detail-durable-cache.test.ts @@ -0,0 +1,472 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import { + getPullRequestDetailSyncState, + upsertPullRequestDetailSyncState, +} from "../../src/db/repositories"; +import { + cachedFetchLivePullRequestHeadSha, + cachedFetchLivePullRequestMergeState, + cachedFetchLivePullRequestState, + invalidatePrStateCache, + primeDurablePrStateCache, +} from "../../src/github/backfill"; +import { clearGitHubResponseCacheForTest } from "../../src/github/client"; +import { renderMetrics, resetMetrics } from "../../src/selfhost/metrics"; +import { createTestEnv } from "../helpers/d1"; + +// Durable, webhook-invalidated cache for the bare PR-state read (#2537). Mirrors +// backfill-file-hydration-scoping.test.ts's helpers/structure for the sibling files cache. +describe("durable PR-state cache (#2537)", () => { + afterEach(() => { + clearGitHubResponseCacheForTest(); + resetMetrics(); + vi.unstubAllGlobals(); + }); + + function stubFetchTracking(handler: (url: string, init?: RequestInit) => Response | Promise): string[] { + const urls: string[] = []; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + urls.push(url); + return handler(url, init); + }); + return urls; + } + + // Simulates a D1 write hiccup ONLY for pull_request_detail_sync_state upserts, so the cache's fail-open + // write-through can be exercised without a full DB outage. + function withPrStateWriteFailure(env: Env): Env { + const db = env.DB as unknown as { prepare(sql: string): unknown; batch(statements: unknown[]): Promise }; + return { + ...env, + DB: { + prepare(sql: string) { + if (sql.includes("pull_request_detail_sync_state") && sql.trim().toUpperCase().startsWith("INSERT")) { + throw new Error("pull_request_detail_sync_state write failed"); + } + return db.prepare.call(db, sql); + }, + batch(statements: unknown[]) { + return db.batch.call(db, statements); + }, + } as unknown as D1Database, + }; + } + + // REGRESSION (#2595 review defect): the three cached readers below share ONE prStateFetchedAt column as their + // freshness stamp. Before this fix, each reader wrote through ONLY the one field it cared about, so a write + // from reader A would make reader B's UN-fetched field look "fresh" to a subsequent call -- silently returning + // undefined for a field that was simply never populated, not confirmed empty on GitHub. The fix fetches the + // full PR payload (all three narrow fetchers already hit the exact same endpoint) and writes all three fields + // through together on every live fetch, so this cross-field false-freshness can no longer happen. + it("REGRESSION (#2595): a live fetch from ONE cached reader also warms the OTHER TWO, since they share one fetchedAt stamp", async () => { + const env = createTestEnv(); + let fetchCount = 0; + stubFetchTracking((url) => { + if (url.includes("/pulls/40")) { + fetchCount += 1; + return Response.json({ number: 40, mergeable_state: "clean", state: "open", head: { sha: "shared-sha" } }); + } + return new Response("not found", { status: 404 }); + }); + + // Only the mergeable_state reader is called... + const mergeableState = await cachedFetchLivePullRequestMergeState(env, "owner/repo", 40, "tok"); + expect(mergeableState).toBe("clean"); + expect(fetchCount).toBe(1); + + // ...yet the OTHER two fields are now cache HITS too, without a second GitHub call, and return the REAL + // fetched values -- not a false "confirmed fresh, but never actually fetched" undefined. + const state = await cachedFetchLivePullRequestState(env, "owner/repo", 40, "tok"); + const headSha = await cachedFetchLivePullRequestHeadSha(env, "owner/repo", 40, "tok"); + expect(state).toBe("open"); + expect(headSha).toBe("shared-sha"); + expect(fetchCount).toBe(1); // no additional GitHub calls were needed + }); + + describe("cachedFetchLivePullRequestMergeState", () => { + it("cache miss on first read — fetches live and writes the row through", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + const urls = stubFetchTracking((url) => (url.includes("/pulls/10") ? Response.json({ number: 10, mergeable_state: "clean" }) : new Response("not found", { status: 404 }))); + + const result = await cachedFetchLivePullRequestMergeState(env, "owner/repo", 10, "tok"); + + expect(result).toBe("clean"); + expect(urls.some((url) => url.includes("/pulls/10"))).toBe(true); + expect(await getPullRequestDetailSyncState(env, "owner/repo", 10)).toMatchObject({ prMergeableState: "clean" }); + }); + + it("fail-open: a write-through hiccup still returns the live value (the cache is an optimization, not a dependency)", async () => { + const env = withPrStateWriteFailure(createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" })); + stubFetchTracking((url) => (url.includes("/pulls/14") ? Response.json({ number: 14, mergeable_state: "clean" }) : new Response("not found", { status: 404 }))); + + const result = await cachedFetchLivePullRequestMergeState(env, "owner/repo", 14, "tok"); + + expect(result).toBe("clean"); + }); + + it("cache hit on unchanged state — never calls GitHub", async () => { + const env = createTestEnv(); + await upsertPullRequestDetailSyncState(env, { + repoFullName: "owner/repo", + pullNumber: 11, + status: "complete", + prMergeableState: "blocked", + prStateFetchedAt: new Date().toISOString(), + }); + const urls = stubFetchTracking(() => new Response("must not be called", { status: 500 })); + + const result = await cachedFetchLivePullRequestMergeState(env, "owner/repo", 11, "tok"); + + expect(result).toBe("blocked"); + expect(urls).toHaveLength(0); + }); + + it("cache expiry — a stale row past the TTL is treated as a miss (fetch IS called)", async () => { + const env = createTestEnv(); + await upsertPullRequestDetailSyncState(env, { + repoFullName: "owner/repo", + pullNumber: 12, + status: "complete", + prMergeableState: "blocked", + prStateFetchedAt: "2020-01-01T00:00:00.000Z", + }); + const urls = stubFetchTracking((url) => (url.includes("/pulls/12") ? Response.json({ number: 12, mergeable_state: "clean" }) : new Response("not found", { status: 404 }))); + + const result = await cachedFetchLivePullRequestMergeState(env, "owner/repo", 12, "tok"); + + expect(result).toBe("clean"); + expect(urls.some((url) => url.includes("/pulls/12"))).toBe(true); + }); + + it("an undefined live read still stamps prStateFetchedAt, so the next read within the TTL is a cache hit returning undefined, not a fetch", async () => { + const env = createTestEnv(); + let fetchCount = 0; + stubFetchTracking((url) => { + if (url.includes("/pulls/13")) { + fetchCount += 1; + return Response.json({ number: 13 }); // no mergeable_state field + } + return new Response("not found", { status: 404 }); + }); + + const first = await cachedFetchLivePullRequestMergeState(env, "owner/repo", 13, "tok"); + expect(first).toBeUndefined(); + expect(fetchCount).toBe(1); + expect(await getPullRequestDetailSyncState(env, "owner/repo", 13)).toMatchObject({ prMergeableState: null }); + + const second = await cachedFetchLivePullRequestMergeState(env, "owner/repo", 13, "tok"); + expect(second).toBeUndefined(); + expect(fetchCount).toBe(1); // still 1 — served from cache, not re-fetched + }); + }); + + describe("cachedFetchLivePullRequestState", () => { + it("cache miss then cache hit", async () => { + const env = createTestEnv(); + let fetchCount = 0; + stubFetchTracking((url) => { + if (url.includes("/pulls/20")) { + fetchCount += 1; + return Response.json({ number: 20, state: "open" }); + } + return new Response("not found", { status: 404 }); + }); + + const first = await cachedFetchLivePullRequestState(env, "owner/repo", 20, "tok"); + const second = await cachedFetchLivePullRequestState(env, "owner/repo", 20, "tok"); + + expect(first).toBe("open"); + expect(second).toBe("open"); + expect(fetchCount).toBe(1); + }); + + it("a cache hit whose stored prState is null (nullish live read) returns undefined, not null", async () => { + const env = createTestEnv(); + await upsertPullRequestDetailSyncState(env, { + repoFullName: "owner/repo", + pullNumber: 21, + status: "complete", + prState: null, + prStateFetchedAt: new Date().toISOString(), + }); + const urls = stubFetchTracking(() => new Response("must not be called", { status: 500 })); + + const result = await cachedFetchLivePullRequestState(env, "owner/repo", 21, "tok"); + + expect(result).toBeUndefined(); + expect(urls).toHaveLength(0); + }); + + it("a cache-miss live fetch whose payload omits state returns undefined (nullish fallback on a fresh fetch, not a cache hit)", async () => { + const env = createTestEnv(); + let fetchCount = 0; + stubFetchTracking((url) => { + if (url.includes("/pulls/22")) { + fetchCount += 1; + return Response.json({ number: 22 }); // no state field + } + return new Response("not found", { status: 404 }); + }); + + const result = await cachedFetchLivePullRequestState(env, "owner/repo", 22, "tok"); + + expect(result).toBeUndefined(); + expect(fetchCount).toBe(1); + expect(await getPullRequestDetailSyncState(env, "owner/repo", 22)).toMatchObject({ prState: null }); + }); + }); + + describe("cachedFetchLivePullRequestHeadSha", () => { + it("cache miss then cache hit, and does not serve a cached row missing headSha", async () => { + const env = createTestEnv(); + // A row that is fresh (prStateFetchedAt set) but has no headSha yet must still be treated as a miss. + await upsertPullRequestDetailSyncState(env, { + repoFullName: "owner/repo", + pullNumber: 30, + status: "complete", + prStateFetchedAt: new Date().toISOString(), + }); + let fetchCount = 0; + stubFetchTracking((url) => { + if (url.includes("/pulls/30")) { + fetchCount += 1; + return Response.json({ number: 30, head: { sha: "live-sha" } }); + } + return new Response("not found", { status: 404 }); + }); + + const first = await cachedFetchLivePullRequestHeadSha(env, "owner/repo", 30, "tok"); + expect(first).toBe("live-sha"); + expect(fetchCount).toBe(1); + + const second = await cachedFetchLivePullRequestHeadSha(env, "owner/repo", 30, "tok"); + expect(second).toBe("live-sha"); + expect(fetchCount).toBe(1); + }); + + // REGRESSION (#2595 review defect): the three cached readers share ONE prStateFetchedAt stamp, so a live + // fetch triggered by ANY of them must write through ALL THREE fields together -- otherwise a field this + // reader doesn't care about (mergeable_state/state) would look "fresh" to a later, different reader despite + // never having been fetched. A fresh full-payload fetch that carries no head.sha still writes the OTHER two + // fields through (and still never CLEARS a prior headSha -- the PARTIAL-UPDATE CONTRACT is preserved). + it("writes mergeable_state/state through even when the live head SHA is undefined, and never clears a prior headSha", async () => { + const env = createTestEnv(); + await upsertPullRequestDetailSyncState(env, { + repoFullName: "owner/repo", + pullNumber: 31, + status: "never_synced", + headSha: "prior-sha", + prStateFetchedAt: "2020-01-01T00:00:00.000Z", // stale -- forces a live re-fetch + }); + stubFetchTracking(() => Response.json({ number: 31, mergeable_state: "clean", state: "open" })); // no head.sha + + const result = await cachedFetchLivePullRequestHeadSha(env, "owner/repo", 31, "tok"); + + expect(result).toBeUndefined(); // no head.sha in the live payload + const row = await getPullRequestDetailSyncState(env, "owner/repo", 31); + expect(row?.headSha).toBe("prior-sha"); // NOT cleared -- omitted, not written as null + expect(row?.prMergeableState).toBe("clean"); // written through together with this call's own fetch + expect(row?.prState).toBe("open"); + expect(row?.prStateFetchedAt).not.toBe("2020-01-01T00:00:00.000Z"); // the shared stamp advanced + }); + + it("still creates a row (with the other fields null) on a fresh PR whose live payload carries no head.sha at all", async () => { + const env = createTestEnv(); + stubFetchTracking(() => Response.json({ number: 32 })); // no head.sha, no mergeable_state, no state + + const result = await cachedFetchLivePullRequestHeadSha(env, "owner/repo", 32, "tok"); + + expect(result).toBeUndefined(); + const row = await getPullRequestDetailSyncState(env, "owner/repo", 32); + expect(row?.headSha).toBeNull(); // never had one to preserve + expect(row?.prMergeableState).toBeNull(); + expect(row?.prState).toBeNull(); + expect(row?.prStateFetchedAt).not.toBeNull(); // the fetch DID succeed (confirmed-empty, not "never fetched") + }); + }); + + describe("primeDurablePrStateCache", () => { + it("does nothing when the live payload is undefined (upstream fetchLivePullRequest failed)", async () => { + const env = createTestEnv(); + + await primeDurablePrStateCache(env, "owner/repo", 60, undefined); + + expect(await getPullRequestDetailSyncState(env, "owner/repo", 60)).toBeNull(); + }); + + it("writes mergeable_state/state/headSha through from an already-fetched live payload", async () => { + const env = createTestEnv(); + + await primeDurablePrStateCache(env, "owner/repo", 61, { mergeable_state: "dirty", state: "open", head: { sha: "primed-sha" } }); + + expect(await getPullRequestDetailSyncState(env, "owner/repo", 61)).toMatchObject({ + prMergeableState: "dirty", + prState: "open", + headSha: "primed-sha", + prStateFetchedAt: expect.any(String), + }); + }); + + it("stores a nullish mergeable_state/state from the live payload as null, not undefined", async () => { + const env = createTestEnv(); + + await primeDurablePrStateCache(env, "owner/repo", 62, { head: { sha: "primed-sha-2" } }); + + expect(await getPullRequestDetailSyncState(env, "owner/repo", 62)).toMatchObject({ + prMergeableState: null, + prState: null, + headSha: "primed-sha-2", + }); + }); + + it("PARTIAL-UPDATE CONTRACT: omits headSha (does not clear a prior one) when the live payload carries no head.sha", async () => { + const env = createTestEnv(); + await upsertPullRequestDetailSyncState(env, { + repoFullName: "owner/repo", + pullNumber: 63, + status: "complete", + headSha: "files-cache-sha", + filesSyncedAt: "2026-05-20T00:00:00.000Z", + }); + + await primeDurablePrStateCache(env, "owner/repo", 63, { mergeable_state: "clean", state: "open" }); + + expect(await getPullRequestDetailSyncState(env, "owner/repo", 63)).toMatchObject({ + prMergeableState: "clean", + prState: "open", + headSha: "files-cache-sha", + filesSyncedAt: "2026-05-20T00:00:00.000Z", + }); + }); + + it("primes the cache so a subsequent cachedFetchLivePullRequestMergeState reads the primed value without a network call", async () => { + const env = createTestEnv(); + let fetchCount = 0; + stubFetchTracking(() => { + fetchCount += 1; + return Response.json({ number: 64, mergeable_state: "should-not-be-fetched" }); + }); + + await primeDurablePrStateCache(env, "owner/repo", 64, { mergeable_state: "primed-dirty", head: { sha: "sha-64" } }); + const result = await cachedFetchLivePullRequestMergeState(env, "owner/repo", 64, "tok"); + + expect(result).toBe("primed-dirty"); + expect(fetchCount).toBe(0); + }); + }); + + describe("invalidatePrStateCache", () => { + it("clears prMergeableState/prState/prStateFetchedAt and forces the next read to miss", async () => { + const env = createTestEnv(); + await upsertPullRequestDetailSyncState(env, { + repoFullName: "owner/repo", + pullNumber: 40, + status: "complete", + prMergeableState: "clean", + prState: "open", + prStateFetchedAt: new Date().toISOString(), + }); + + await invalidatePrStateCache(env, "owner/repo", 40); + + expect(await getPullRequestDetailSyncState(env, "owner/repo", 40)).toMatchObject({ + prMergeableState: null, + prState: null, + prStateFetchedAt: null, + }); + + const urls = stubFetchTracking((url) => (url.includes("/pulls/40") ? Response.json({ number: 40, mergeable_state: "dirty" }) : new Response("not found", { status: 404 }))); + const result = await cachedFetchLivePullRequestMergeState(env, "owner/repo", 40, "tok"); + expect(result).toBe("dirty"); + expect(urls.some((url) => url.includes("/pulls/40"))).toBe(true); + }); + + it("defaults status to never_synced when no prior row exists", async () => { + const env = createTestEnv(); + await invalidatePrStateCache(env, "owner/repo", 41); + expect(await getPullRequestDetailSyncState(env, "owner/repo", 41)).toMatchObject({ status: "never_synced" }); + }); + }); + + describe("write-through preserves status (regression: must not force status: complete)", () => { + it("a cachedFetchLivePullRequest* write does not clear an in-progress files sync's status/filesSyncedAt", async () => { + const env = createTestEnv(); + await upsertPullRequestDetailSyncState(env, { + repoFullName: "owner/repo", + pullNumber: 50, + status: "partial", + headSha: "sha-a", + filesSyncedAt: "2026-05-20T00:00:00.000Z", + }); + stubFetchTracking((url) => (url.includes("/pulls/50") ? Response.json({ number: 50, mergeable_state: "clean" }) : new Response("not found", { status: 404 }))); + + await cachedFetchLivePullRequestMergeState(env, "owner/repo", 50, "tok"); + + expect(await getPullRequestDetailSyncState(env, "owner/repo", 50)).toMatchObject({ + status: "partial", + headSha: "sha-a", + filesSyncedAt: "2026-05-20T00:00:00.000Z", + prMergeableState: "clean", + }); + }); + }); + + describe("isPrStateCacheFresh branch coverage (via cachedFetchLivePullRequestMergeState)", () => { + it("null fetchedAt ⇒ treated as stale (miss)", async () => { + const env = createTestEnv(); + await upsertPullRequestDetailSyncState(env, { repoFullName: "owner/repo", pullNumber: 60, status: "complete", prMergeableState: "clean", prStateFetchedAt: null }); + const urls = stubFetchTracking((url) => (url.includes("/pulls/60") ? Response.json({ number: 60, mergeable_state: "dirty" }) : new Response("not found", { status: 404 }))); + + const result = await cachedFetchLivePullRequestMergeState(env, "owner/repo", 60, "tok"); + expect(result).toBe("dirty"); + expect(urls.some((url) => url.includes("/pulls/60"))).toBe(true); + }); + + it("unparseable fetchedAt string ⇒ treated as stale (NaN branch, miss)", async () => { + const env = createTestEnv(); + await upsertPullRequestDetailSyncState(env, { repoFullName: "owner/repo", pullNumber: 61, status: "complete", prMergeableState: "clean", prStateFetchedAt: "not-a-date" }); + const urls = stubFetchTracking((url) => (url.includes("/pulls/61") ? Response.json({ number: 61, mergeable_state: "dirty" }) : new Response("not found", { status: 404 }))); + + const result = await cachedFetchLivePullRequestMergeState(env, "owner/repo", 61, "tok"); + expect(result).toBe("dirty"); + expect(urls.some((url) => url.includes("/pulls/61"))).toBe(true); + }); + + it("fresh fetchedAt ⇒ hit (no fetch)", async () => { + const env = createTestEnv(); + await upsertPullRequestDetailSyncState(env, { repoFullName: "owner/repo", pullNumber: 62, status: "complete", prMergeableState: "clean", prStateFetchedAt: new Date().toISOString() }); + const urls = stubFetchTracking(() => new Response("must not be called", { status: 500 })); + + const result = await cachedFetchLivePullRequestMergeState(env, "owner/repo", 62, "tok"); + expect(result).toBe("clean"); + expect(urls).toHaveLength(0); + }); + + it("expired fetchedAt ⇒ miss (fetch called)", async () => { + const env = createTestEnv(); + await upsertPullRequestDetailSyncState(env, { repoFullName: "owner/repo", pullNumber: 63, status: "complete", prMergeableState: "clean", prStateFetchedAt: "2020-01-01T00:00:00.000Z" }); + const urls = stubFetchTracking((url) => (url.includes("/pulls/63") ? Response.json({ number: 63, mergeable_state: "dirty" }) : new Response("not found", { status: 404 }))); + + const result = await cachedFetchLivePullRequestMergeState(env, "owner/repo", 63, "tok"); + expect(result).toBe("dirty"); + expect(urls.some((url) => url.includes("/pulls/63"))).toBe(true); + }); + }); + + describe("metrics", () => { + it("records miss then hit for mergeable_state across a cold then warm read", async () => { + resetMetrics(); + const env = createTestEnv(); + stubFetchTracking((url) => (url.includes("/pulls/70") ? Response.json({ number: 70, mergeable_state: "clean" }) : new Response("not found", { status: 404 }))); + + await cachedFetchLivePullRequestMergeState(env, "owner/repo", 70, "tok"); + await cachedFetchLivePullRequestMergeState(env, "owner/repo", 70, "tok"); + + const metrics = await renderMetrics(); + expect(metrics).toContain('gittensory_pr_state_cache_total{field="mergeable_state",result="miss"} 1'); + expect(metrics).toContain('gittensory_pr_state_cache_total{field="mergeable_state",result="hit"} 1'); + expect(metrics).toContain('gittensory_pr_state_cache_total{field="write",result="set"} 1'); + }); + }); +}); diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index 6b29f55f83..8cdc1a077a 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -19,6 +19,7 @@ import { getLatestUpstreamRulesetSnapshot, getPullRequest, getPullRequestDetailSyncState, + upsertPullRequestDetailSyncState, getRepository, listUpstreamDriftReports, listInstallationHealth, @@ -46,7 +47,7 @@ import { upsertRepositoryFromGitHub, putCachedAiReview, } from "../../src/db/repositories"; -import { agentMaintenanceHeadMatchesGate, changedPathsForGuardrail, claimAiReviewLock, claimPrActuationLock, contributorEvidenceBatchSize, processJob, releaseAiReviewLock, releasePrActuationLock } from "../../src/queue/processors"; +import { agentMaintenanceHeadMatchesGate, changedPathsForGuardrail, claimAiReviewLock, claimPrActuationLock, contributorEvidenceBatchSize, processJob, reconcileLiveDuplicateSiblings, releaseAiReviewLock, releasePrActuationLock } from "../../src/queue/processors"; import { aiReviewCacheInputFingerprint } from "../../src/review/ai-review-cache-input"; import { upsertRepoFocusManifest } from "../../src/signals/focus-manifest-loader"; import { normalizeRegistryPayload } from "../../src/registry/normalize"; @@ -14993,4 +14994,134 @@ describe("installation app_id capture + dual-app webhook filter (#selfhost-app-i const evt = await env.DB.prepare("select payload_hash from webhook_events where delivery_id = ?").bind("own-app-pr").first<{ payload_hash: string }>(); expect(evt?.payload_hash).not.toBe("foreign_app"); }); + + // #2537: durable PR-state cache — webhook invalidation + the act-boundary regression. + describe("durable PR-state cache (#2537)", () => { + function seedWarmPrStateCache(env: Env, repoFullName: string, pullNumber: number): Promise { + return upsertPullRequestDetailSyncState(env, { + repoFullName, + pullNumber, + status: "complete", + prMergeableState: "clean", + prState: "open", + prStateFetchedAt: new Date().toISOString(), + }); + } + + it.each(["synchronize", "closed", "reopened"] as const)( + "pull_request %s action invalidates the durable PR-state cache", + async (action) => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await upsertInstallation(env, { action: "created", installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" }, target_type: "User", repository_selection: "all", permissions: {}, events: [] } }); + await upsertRepositoryFromGitHub(env, { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, 123); + await upsertRepositorySettings(env, { repoFullName: "JSONbored/gittensory", gateCheckMode: "off", checkRunMode: "off", commentMode: "off", publicSurface: "off" }); + await seedWarmPrStateCache(env, "JSONbored/gittensory", 200); + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + const url = input.toString(); + if (url.includes("/access_tokens")) return Response.json({ token: "tok" }); + return Response.json({}); + }); + + await processJob(env, { + type: "github-webhook", + deliveryId: `invalidate-pr-state-${action}`, + eventName: "pull_request", + payload: { + action, + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + pull_request: { number: 200, title: "PR", state: action === "closed" ? "closed" : "open", user: { login: "contributor" }, head: { sha: "a200" }, labels: [], body: "" }, + }, + }); + + expect(await getPullRequestDetailSyncState(env, "JSONbored/gittensory", 200)).toMatchObject({ + prMergeableState: null, + prState: null, + prStateFetchedAt: null, + }); + }, + ); + + it("a non-invalidating pull_request action (labeled) leaves the durable PR-state cache UNCHANGED", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await upsertInstallation(env, { action: "created", installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" }, target_type: "User", repository_selection: "all", permissions: {}, events: [] } }); + await upsertRepositoryFromGitHub(env, { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, 123); + await upsertRepositorySettings(env, { repoFullName: "JSONbored/gittensory", gateCheckMode: "off", checkRunMode: "off", commentMode: "off", publicSurface: "off" }); + await seedWarmPrStateCache(env, "JSONbored/gittensory", 201); + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + const url = input.toString(); + if (url.includes("/access_tokens")) return Response.json({ token: "tok" }); + return Response.json({}); + }); + + await processJob(env, { + type: "github-webhook", + deliveryId: "invalidate-pr-state-labeled", + eventName: "pull_request", + payload: { + action: "labeled", + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + pull_request: { number: 201, title: "PR", state: "open", user: { login: "contributor" }, head: { sha: "a201" }, labels: [], body: "" }, + }, + }); + + expect(await getPullRequestDetailSyncState(env, "JSONbored/gittensory", 201)).toMatchObject({ + prMergeableState: "clean", + prState: "open", + }); + }); + + it("REGRESSION (#2537, gate-flagged): reconcileLiveDuplicateSiblings must NOT serve a warm durable PR-state cache row — a cached 'open' read up to PR_STATE_CACHE_MAX_AGE_MS stale after a missed closed webhook would keep an already-closed sibling eligible as the duplicate-cluster winner, wrongly closing the CURRENT PR as the loser", async () => { + const env = createTestEnv({ GITTENSORY_DUPLICATE_WINNER: "true" }); + // Seed a WARM cache row claiming the sibling is still open, but the live GitHub state below says CLOSED — + // proving the cache is never consulted: only a genuine live read can discover this and correctly reconcile it. + await seedWarmPrStateCache(env, "owner/repo", 5); + let liveStateFetches = 0; + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + const url = input.toString(); + if (url.includes("/access_tokens")) return Response.json({ token: "tok" }); + if (/\/pulls\/5(?:\?|$)/.test(url)) { + liveStateFetches += 1; + return Response.json({ number: 5, state: "closed" }); + } + return Response.json({}); + }); + + const winner: Parameters[3] = { repoFullName: "owner/repo", number: 10, title: "Winner", state: "open", labels: [], linkedIssues: [1] }; + const sibling: Parameters[3] = { repoFullName: "owner/repo", number: 5, title: "Sibling", state: "open", labels: [], linkedIssues: [1] }; + const result = await reconcileLiveDuplicateSiblings(env, null, "owner/repo", winner, [sibling]); + + // The sibling is correctly dropped as stale-closed, proving a genuine live fetch happened rather than + // trusting the warm-but-wrong cached "open" value. + expect(result).toEqual([]); + expect(liveStateFetches).toBe(1); + }); + + it("REGRESSION (#2537): the per-PR sweep unit's live resync primes the durable PR-state cache for later readers", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + 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" }, 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", gateCheckMode: "off", checkRunMode: "off", commentMode: "off", publicSurface: "off" }); + await upsertPullRequestFromGitHub(env, "owner/agent-repo", { number: 6, title: "Sweep target", state: "open", user: { login: "contributor" }, head: { sha: "a6" }, base: { ref: "main" }, labels: [], body: "" }); + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + const url = input.toString(); + if (url.includes("/access_tokens")) return Response.json({ token: "tok" }); + if (/\/pulls\/6(?:\?|$)/.test(url)) return Response.json({ number: 6, state: "open", mergeable_state: "clean", head: { sha: "a6" } }); + if (url.includes("/pulls/6/files")) return Response.json([]); + if (url.includes("/pulls/6/reviews")) return Response.json([]); + if (url.includes("/commits/a6/check-runs")) return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes("/commits/a6/status")) return Response.json({ state: "success", statuses: [] }); + if (url.includes("/branches/")) return Response.json({ protected: false, protection: { required_status_checks: { contexts: [] } } }); + return Response.json({}); + }); + + await processJob(env, { type: "agent-regate-pr", deliveryId: "prime-pr-state-cache", repoFullName: "owner/agent-repo", prNumber: 6, installationId: 9001 }); + + expect(await getPullRequestDetailSyncState(env, "owner/agent-repo", 6)).toMatchObject({ + prMergeableState: "clean", + prState: "open", + }); + }); + }); }); diff --git a/test/unit/resolve-override-head-sha.test.ts b/test/unit/resolve-override-head-sha.test.ts index 6ed575aecc..9fc69bc23b 100644 --- a/test/unit/resolve-override-head-sha.test.ts +++ b/test/unit/resolve-override-head-sha.test.ts @@ -1,6 +1,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { resolveOverrideHeadSha } from "../../src/queue/processors"; import { createInstallationToken } from "../../src/github/app"; +import { upsertPullRequestDetailSyncState } from "../../src/db/repositories"; import type { PullRequestRecord } from "../../src/types"; import { createTestEnv } from "../helpers/d1"; @@ -59,4 +60,18 @@ describe("resolveOverrideHeadSha (#16 / gate-override stale head)", () => { stubLiveHead(null); expect(await resolveOverrideHeadSha(env, 123, "owner/repo", makePr("stale-sha"))).toBe("stale-sha"); }); + + it("REGRESSION (#2537, gate-flagged): a FRESH durable PR-state cache row for this PR must NOT short-circuit the live fetch — a commit landing inside the cache's freshness window right after the override comment is exactly the race this function exists to close, so it must always hit GitHub directly rather than trust a recent-but-possibly-already-stale cached headSha", async () => { + const env = createTestEnv(); + mockedToken.mockResolvedValue("inst-tok"); + await upsertPullRequestDetailSyncState(env, { + repoFullName: "owner/repo", + pullNumber: 90, + status: "complete", + headSha: "cached-sha", + prStateFetchedAt: new Date().toISOString(), + }); + stubLiveHead("brand-new-live-sha"); + expect(await resolveOverrideHeadSha(env, 123, "owner/repo", makePr("stale-sha"))).toBe("brand-new-live-sha"); + }); });