diff --git a/packages/loopover-miner/lib/submission-freshness-check.d.ts b/packages/loopover-miner/lib/submission-freshness-check.d.ts index e215c0c149..fbc8864aa5 100644 --- a/packages/loopover-miner/lib/submission-freshness-check.d.ts +++ b/packages/loopover-miner/lib/submission-freshness-check.d.ts @@ -29,4 +29,14 @@ export type SubmissionFreshnessDeps = { export type SubmissionFreshnessResult = { fresh: true } | { fresh: false; reason: FreshnessAbortReason }; -export function checkSubmissionFreshness(candidate: SubmissionFreshnessCandidate, deps: SubmissionFreshnessDeps): Promise; +export type SubmissionFreshnessRetryOptions = { + maxAttempts?: number; + sleepFn?: (ms: number) => Promise; + backoffMs?: (attempt: number) => number; +}; + +export function checkSubmissionFreshness( + candidate: SubmissionFreshnessCandidate, + deps: SubmissionFreshnessDeps, + options?: SubmissionFreshnessRetryOptions, +): Promise; diff --git a/packages/loopover-miner/lib/submission-freshness-check.js b/packages/loopover-miner/lib/submission-freshness-check.js index 2d90b0d787..22dfb106ee 100644 --- a/packages/loopover-miner/lib/submission-freshness-check.js +++ b/packages/loopover-miner/lib/submission-freshness-check.js @@ -15,14 +15,27 @@ // FAIL CLOSED: an unreachable/failed live-state fetch is treated as stale (aborts), never as "no evidence of // staleness, so proceed" -- mirrors this package's fail-closed convention elsewhere (harness-submission- // trigger.js's predicted_gate_unavailable/slop_assessment_unavailable, iterate-loop.ts's ambiguous-on-error). +// That fail-closed OUTCOME is unchanged; a single transient blip just gets a bounded retry-with-backoff to +// resolve itself FIRST (#7089) -- the same window claim-conflict-resolver.js's resolveClaimConflict (#6058) +// already gives its own call to this identical fetchLiveIssueSnapshot, reusing http-retry.js's shared backoff. +// Aborting here discards a fully-completed create/iterate loop's local work, so riding out a brief 5xx / +// GraphQL-index propagation lag before failing closed matters more here than in the post-submission case. // // NOT a rejection outcome: staleness is caught BEFORE any PR exists, so it is not the same lifecycle event as // rejection-state-machine.js's DISENGAGED_OUTCOME (which handles an EXISTING PR a maintainer closed). "No PR, // no noisy failure" here just means: return a quiet not-fresh result, same shape as any other blocked gate // decision in this package -- never throw, never surface anything to the target repo. +import { defaultRetryBackoffMs } from "./http-retry.js"; + export const SUBMISSION_FRESHNESS_ABORT_EVENT = "submission_freshness_abort"; +// Bounded retry for the pre-submission live-state fetch (#7089), mirroring claim-conflict-resolver.js's +// resolveClaimConflict (#6058): a few attempts with exponential backoff let a transient GitHub blur (a brief +// 5xx, or GraphQL-index propagation lag) resolve itself before we fail closed, without an unbounded loop. +const DEFAULT_SNAPSHOT_MAX_ATTEMPTS = 3; +const defaultSnapshotSleep = (delayMs) => new Promise((resolve) => setTimeout(resolve, delayMs)); + /** * Evaluate whether a submission candidate's live repo state is still fresh enough to proceed toward open_pr. * Checks the miner's own claim-ledger status first (local, free) before spending a network round-trip on the @@ -34,8 +47,14 @@ export const SUBMISSION_FRESHNESS_ABORT_EVENT = "submission_freshness_abort"; * fetchLiveIssueSnapshot: (repoFullName: string, issueNumber: number) => Promise<{ state: "open"|"closed", referencingPrs: Array<{ number: number, state: "open"|"closed"|"merged", authorLogin: string }> } | null>, * eventLedger: { appendEvent(event: { type: string, repoFullName?: string, payload: Record }): unknown }, * }} deps + * @param {{ maxAttempts?: number, sleepFn?: (ms: number) => Promise, backoffMs?: (attempt: number) => number }} [options] + * Bounded retry for the live-state snapshot fetch (#7089): up to `maxAttempts` (default 3) attempts with + * `backoffMs(attempt)` backoff between them, returning as soon as a real (non-null, well-formed) snapshot is + * obtained. Optional -- every existing caller works unchanged. Pure over the injected `sleepFn`/`backoffMs` + * -- no real timers in tests. Only the fetch itself is retried; a well-formed snapshot's own signals + * (issue_closed / already_addressed) are decided once, never retried. */ -export async function checkSubmissionFreshness(candidate, deps) { +export async function checkSubmissionFreshness(candidate, deps, options = {}) { if (!candidate || typeof candidate !== "object") throw new Error("invalid_freshness_candidate"); const repoFullName = typeof candidate.repoFullName === "string" ? candidate.repoFullName.trim() : ""; if (!repoFullName) throw new Error("invalid_repo_full_name"); @@ -49,18 +68,34 @@ export async function checkSubmissionFreshness(candidate, deps) { if (typeof fetchLiveIssueSnapshot !== "function") throw new Error("invalid_live_state_fetcher"); if (!eventLedger || typeof eventLedger.appendEvent !== "function") throw new Error("invalid_event_ledger"); + const maxAttempts = + Number.isFinite(options.maxAttempts) && options.maxAttempts >= 1 ? Math.floor(options.maxAttempts) : DEFAULT_SNAPSHOT_MAX_ATTEMPTS; + const sleepFn = typeof options.sleepFn === "function" ? options.sleepFn : defaultSnapshotSleep; + const backoffMs = typeof options.backoffMs === "function" ? options.backoffMs : defaultRetryBackoffMs; + const claim = claimLedger.listClaims({ repoFullName }).find((c) => c.issueNumber === candidate.issueNumber); if (!claim || claim.status !== "active") { return abort(eventLedger, repoFullName, candidate.issueNumber, "claim_superseded"); } - let snapshot; - try { - snapshot = await fetchLiveIssueSnapshot(repoFullName, candidate.issueNumber); - } catch { - snapshot = null; + let snapshot = null; + for (let attempt = 1; attempt <= maxAttempts; attempt += 1) { + let current; + try { + current = await fetchLiveIssueSnapshot(repoFullName, candidate.issueNumber); + } catch { + current = null; + } + if (current && typeof current === "object") { + // A real, well-formed snapshot resolves the transient window: stop retrying and decide on it now. + snapshot = current; + break; + } + // Back off before the next attempt (transient 5xx / index-propagation lag); never after the last one. + if (attempt < maxAttempts) await sleepFn(backoffMs(attempt)); } - if (!snapshot || typeof snapshot !== "object") { + if (!snapshot) { + // Retry budget exhausted with no usable snapshot -- fail closed exactly as before (#7089 only widens the window). return abort(eventLedger, repoFullName, candidate.issueNumber, "live_state_unavailable"); } diff --git a/test/unit/miner-submission-freshness-check.test.ts b/test/unit/miner-submission-freshness-check.test.ts index 8cccf4306b..ffc031dbaa 100644 --- a/test/unit/miner-submission-freshness-check.test.ts +++ b/test/unit/miner-submission-freshness-check.test.ts @@ -194,7 +194,7 @@ describe("checkSubmissionFreshness (#3007)", () => { expect(result).toEqual({ fresh: true }); }); - it("live-state-unavailable abort when the fetch returns null", async () => { + it("live-state-unavailable abort when the fetch returns null on every attempt", async () => { const { claimLedger } = stubClaimLedger([activeClaim]); const { eventLedger } = stubEventLedger(); const fetchLiveIssueSnapshot = vi.fn(async () => null); @@ -202,6 +202,7 @@ describe("checkSubmissionFreshness (#3007)", () => { const result = await checkSubmissionFreshness( { repoFullName: "acme/widgets", issueNumber: 42, minerLogin: "miner-bot" }, { claimLedger, fetchLiveIssueSnapshot, eventLedger }, + { sleepFn: async () => {}, backoffMs: () => 0 }, ); expect(result).toEqual({ fresh: false, reason: "live_state_unavailable" }); @@ -217,6 +218,7 @@ describe("checkSubmissionFreshness (#3007)", () => { const result = await checkSubmissionFreshness( { repoFullName: "acme/widgets", issueNumber: 42, minerLogin: "miner-bot" }, { claimLedger, fetchLiveIssueSnapshot, eventLedger }, + { sleepFn: async () => {}, backoffMs: () => 0 }, ); expect(result).toEqual({ fresh: false, reason: "live_state_unavailable" }); @@ -260,3 +262,152 @@ describe("checkSubmissionFreshness (#3007)", () => { await expect(checkSubmissionFreshness(candidate, { claimLedger, fetchLiveIssueSnapshot } as never)).rejects.toThrow("invalid_event_ledger"); }); }); + +describe("checkSubmissionFreshness live-state retry (#7089)", () => { + it("rides out a thrown transient failure and proceeds on a later attempt instead of aborting", async () => { + const { claimLedger } = stubClaimLedger([activeClaim]); + const { eventLedger, appendEvent } = stubEventLedger(); + // First call: transient GitHub blip (a 5xx surfaces as a thrown fetch); second call: real snapshot. + const fetchLiveIssueSnapshot = vi + .fn() + .mockRejectedValueOnce(new Error("transient 5xx")) + .mockResolvedValueOnce({ state: "open" as const, referencingPrs: [] }); + const sleeps: number[] = []; + + const result = await checkSubmissionFreshness( + { repoFullName: "acme/widgets", issueNumber: 42, minerLogin: "miner-bot" }, + { claimLedger, fetchLiveIssueSnapshot, eventLedger }, + { maxAttempts: 3, sleepFn: async (ms: number) => { sleeps.push(ms); }, backoffMs: (attempt: number) => attempt * 100 }, + ); + + expect(result).toEqual({ fresh: true }); + expect(fetchLiveIssueSnapshot).toHaveBeenCalledTimes(2); + expect(sleeps).toEqual([100]); // backed off once (after attempt 1) with backoffMs(1); no abort logged + expect(appendEvent).not.toHaveBeenCalled(); + }); + + it("rides out a null (non-object) snapshot and proceeds on a later attempt", async () => { + const { claimLedger } = stubClaimLedger([activeClaim]); + const { eventLedger, appendEvent } = stubEventLedger(); + const fetchLiveIssueSnapshot = vi + .fn() + .mockResolvedValueOnce(null) + .mockResolvedValueOnce({ state: "open" as const, referencingPrs: [] }); + + const result = await checkSubmissionFreshness( + { repoFullName: "acme/widgets", issueNumber: 42, minerLogin: "miner-bot" }, + { claimLedger, fetchLiveIssueSnapshot, eventLedger }, + { maxAttempts: 3, sleepFn: async () => {}, backoffMs: () => 0 }, + ); + + expect(result).toEqual({ fresh: true }); + expect(fetchLiveIssueSnapshot).toHaveBeenCalledTimes(2); + expect(appendEvent).not.toHaveBeenCalled(); + }); + + it("stops the instant a real snapshot is obtained: does not burn remaining attempts or sleep", async () => { + const { claimLedger } = stubClaimLedger([activeClaim]); + const { eventLedger } = stubEventLedger(); + const fetchLiveIssueSnapshot = vi.fn(async () => ({ state: "open" as const, referencingPrs: [] })); + const sleepFn = vi.fn(async () => {}); + + const result = await checkSubmissionFreshness( + { repoFullName: "acme/widgets", issueNumber: 42, minerLogin: "miner-bot" }, + { claimLedger, fetchLiveIssueSnapshot, eventLedger }, + { maxAttempts: 3, sleepFn, backoffMs: (attempt: number) => attempt * 100 }, + ); + + expect(result).toEqual({ fresh: true }); + expect(fetchLiveIssueSnapshot).toHaveBeenCalledTimes(1); + expect(sleepFn).not.toHaveBeenCalled(); + }); + + it("still fails closed after the bounded retries are exhausted, appending the abort event exactly once", async () => { + const { claimLedger } = stubClaimLedger([activeClaim]); + const { eventLedger, appendEvent } = stubEventLedger(); + const fetchLiveIssueSnapshot = vi.fn(async () => null); // never recovers + const sleeps: number[] = []; + + const result = await checkSubmissionFreshness( + { repoFullName: "acme/widgets", issueNumber: 42, minerLogin: "miner-bot" }, + { claimLedger, fetchLiveIssueSnapshot, eventLedger }, + { maxAttempts: 3, sleepFn: async (ms: number) => { sleeps.push(ms); }, backoffMs: (attempt: number) => attempt * 100 }, + ); + + expect(result).toEqual({ fresh: false, reason: "live_state_unavailable" }); + expect(fetchLiveIssueSnapshot).toHaveBeenCalledTimes(3); + expect(sleeps).toEqual([100, 200]); // backed off between attempts, never after the last + expect(appendEvent).toHaveBeenCalledTimes(1); // logged once for the whole check, not once per failed attempt + expect(appendEvent).toHaveBeenCalledWith({ + type: SUBMISSION_FRESHNESS_ABORT_EVENT, + repoFullName: "acme/widgets", + payload: { issueNumber: 42, reason: "live_state_unavailable" }, + }); + }); + + it("does not retry a real staleness signal: a closed issue aborts on the first snapshot", async () => { + const { claimLedger } = stubClaimLedger([activeClaim]); + const { eventLedger } = stubEventLedger(); + const fetchLiveIssueSnapshot = vi.fn(async () => ({ state: "closed" as const, referencingPrs: [] })); + const sleepFn = vi.fn(async () => {}); + + const result = await checkSubmissionFreshness( + { repoFullName: "acme/widgets", issueNumber: 42, minerLogin: "miner-bot" }, + { claimLedger, fetchLiveIssueSnapshot, eventLedger }, + { maxAttempts: 3, sleepFn, backoffMs: () => 0 }, + ); + + expect(result).toEqual({ fresh: false, reason: "issue_closed" }); + expect(fetchLiveIssueSnapshot).toHaveBeenCalledTimes(1); // a well-formed snapshot ends the retry loop + expect(sleepFn).not.toHaveBeenCalled(); + }); + + it("defaults maxAttempts/sleepFn/backoffMs when no options are passed (existing callers unchanged)", async () => { + const { claimLedger } = stubClaimLedger([activeClaim]); + const { eventLedger, appendEvent } = stubEventLedger(); + const fetchLiveIssueSnapshot = vi.fn(async () => ({ state: "open" as const, referencingPrs: [] })); + + const result = await checkSubmissionFreshness( + { repoFullName: "acme/widgets", issueNumber: 42, minerLogin: "miner-bot" }, + { claimLedger, fetchLiveIssueSnapshot, eventLedger }, + ); + + expect(result).toEqual({ fresh: true }); + expect(fetchLiveIssueSnapshot).toHaveBeenCalledTimes(1); // succeeds first attempt, so no default backoff/sleep runs + expect(appendEvent).not.toHaveBeenCalled(); + }); + + it("uses the default (real) sleep between retries when no sleepFn is injected", async () => { + const { claimLedger } = stubClaimLedger([activeClaim]); + const { eventLedger } = stubEventLedger(); + // No sleepFn → exercises the default setTimeout-based sleep; backoffMs 0 keeps it instant. + const fetchLiveIssueSnapshot = vi + .fn() + .mockResolvedValueOnce(null) + .mockResolvedValueOnce({ state: "open" as const, referencingPrs: [] }); + + const result = await checkSubmissionFreshness( + { repoFullName: "acme/widgets", issueNumber: 42, minerLogin: "miner-bot" }, + { claimLedger, fetchLiveIssueSnapshot, eventLedger }, + { maxAttempts: 2, backoffMs: () => 0 }, + ); + + expect(result).toEqual({ fresh: true }); + expect(fetchLiveIssueSnapshot).toHaveBeenCalledTimes(2); + }); + + it("clamps a maxAttempts below 1 back to the default rather than skipping every attempt", async () => { + const { claimLedger } = stubClaimLedger([activeClaim]); + const { eventLedger } = stubEventLedger(); + const fetchLiveIssueSnapshot = vi.fn(async () => null); // never recovers, so it runs the full default budget + + const result = await checkSubmissionFreshness( + { repoFullName: "acme/widgets", issueNumber: 42, minerLogin: "miner-bot" }, + { claimLedger, fetchLiveIssueSnapshot, eventLedger }, + { maxAttempts: 0, sleepFn: async () => {}, backoffMs: () => 0 }, + ); + + expect(result).toEqual({ fresh: false, reason: "live_state_unavailable" }); + expect(fetchLiveIssueSnapshot).toHaveBeenCalledTimes(3); // DEFAULT_SNAPSHOT_MAX_ATTEMPTS, not 0 + }); +});