diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 6192134d31..28e1ba599c 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -2462,6 +2462,14 @@ async function maybeRunAgentMaintenance( deliveryId: string; gate: ReturnType | undefined; liveFacts: LiveGithubFacts; + // A {@link claimPrActuationLock} claim the CALLER already acquired (#9013) before its own publish call, so + // ONE lock spans the publish-then-maintain pair instead of leaving the publish half unprotected. Mirrors + // preAcquiredAiReviewLock's exact contract (ai-review-orchestration.ts): when supplied and `.acquired`, this + // function trusts it, skips its OWN claim below entirely, and does NOT release it in its `finally` (release + // stays the caller's job, so the lock keeps covering the caller's own publish call too). Absent (any direct/ + // test caller that doesn't thread it) ⇒ this function claims + releases its own lock exactly as before — + // byte-identical to today. + preAcquiredActuationLock?: TransientLockClaim | undefined; }, ): Promise { const { @@ -2496,7 +2504,14 @@ async function maybeRunAgentMaintenance( // plan-and-execute critical section (extracted below so the try/finally doesn't force-reindent that whole // block); a pass that loses the race defers cleanly — the next webhook/sweep tick is the backstop. Prefers // the SubmissionLock Durable Object when bound; otherwise the transient-cache mutex in transient-locks.ts. - const actuationLock = await claimPrActuationLock(env, repoFullName, pr.number); + // #9013: prefer the caller's OWN claim (args.preAcquiredActuationLock) when it already did one — the caller + // now claims this SAME lock before its own public-surface publish call and threads it through here so ONE + // claim spans publish-then-maintain, rather than leaving the publish half unlocked. Claiming again here would + // be this function contending against its own caller's claim (always losing) rather than against a genuinely + // different pass. Absent (any direct/test caller) ⇒ claim it here exactly as before. + const selfClaimedActuationLock = args.preAcquiredActuationLock === undefined; + const actuationLock = + args.preAcquiredActuationLock ?? (await claimPrActuationLock(env, repoFullName, pr.number)); if (!actuationLock.acquired) { // #9025: this used to `return` silently -- the job completed "successfully", nothing re-queued the // disposition, and no audit row recorded that a planned action was abandoned. That silently amplified @@ -2534,7 +2549,10 @@ async function maybeRunAgentMaintenance( liveFacts: args.liveFacts, }); } finally { - await releasePrActuationLock(env, repoFullName, pr.number, actuationLock.ownerToken); + // #9013: only release a lock THIS call actually claimed -- a caller-supplied preAcquiredActuationLock must + // keep covering the caller's own post-return work, exactly like preAcquiredAiReviewLock's own finally. + if (selfClaimedActuationLock) + await releasePrActuationLock(env, repoFullName, pr.number, actuationLock.ownerToken); } } @@ -3937,86 +3955,117 @@ export async function reReviewStoredPullRequest( () => undefined, ); } - const gate = await withReviewPipelineSpan( - "selfhost.review.public_surface", - { - installationId, - repoFullName, - pullNumber: pr.number, - operation: "public_surface", - }, - () => - maybePublishPrPublicSurface( - env, + // #9013: ONE per-PR actuation-lock claim spans the publish pass AND the maintenance pass right after it. + // maybePublishPrPublicSurface used to run with no lock at all -- only the LATER maybeRunAgentMaintenance + // claimed one -- so two concurrent passes for the SAME PR (this sweep re-review racing a webhook delivery, + // or vice versa) could BOTH publish: duplicate gate check-runs (createOrUpdateNamedCheckRun has no dedup for + // check-runs, unlike panel comments' deleteDuplicateMarkerComments self-heal) and a lock-losing pass's + // placeholder verdict overwriting a real one, whichever PATCHes last. Claiming here, before the publish call, + // and threading the SAME claim into both maybePublishPrPublicSurface (preAcquiredActuationLock, which also + // covers its internal type-label section) and maybeRunAgentMaintenance (preAcquiredActuationLock) makes "does + // another pass already own this PR" one question with one answer for the whole publish-then-maintain unit, + // not two separately-timed ones. A losing pass defers the WHOLE unit (throws, uncaught below -- reaches the + // queue's retry path exactly like maybeRunAgentMaintenance's own pre-existing contention throw) instead of + // racing ahead on a stale/concurrent read. + const actuationLock = await claimPrActuationLock(env, repoFullName, pr.number); + if (!actuationLock.acquired) { + await recordAuditEvent(env, { + eventType: "github_app.pr_public_surface_lock_contended", + actor: null, + targetKey: `${repoFullName}#${pr.number}`, + outcome: "queued", + detail: "Another pass holds this PR's actuation lock; the publish-and-maintain pass retries instead of racing it.", + metadata: { deliveryId, repoFullName }, + }).catch(() => undefined); + throw new PrActuationLockContendedError(repoFullName, pr.number, "public-surface-publish"); + } + let gate: ReturnType | undefined; + try { + gate = await withReviewPipelineSpan( + "selfhost.review.public_surface", + { installationId, repoFullName, - pr, - repo, - settings, - advisory, - otherOpenPullRequests, - { + pullNumber: pr.number, + operation: "public_surface", + }, + () => + maybePublishPrPublicSurface( + env, + installationId, + repoFullName, + pr, + repo, + settings, + advisory, + otherOpenPullRequests, + { + deliveryId, + baseSha: live?.base?.sha ?? null, + liveFacts, + ...(previewPollAttempt !== undefined ? { previewPollAttempt } : {}), + ...(options.skipAiReview || autoreviewPaused ? { skipAiReview: true } : {}), + ...(options.force || pendingRetriggerForceReview ? { forceAiReview: true } : {}), + hasPendingRefreshSignal: otherRefreshReasons || reviewsCacheStale, + preAcquiredActuationLock: actuationLock, + }, + ), + ).catch((error) => { + /* v8 ignore next -- retryable/rate-limit propagation is exercised by queue retry tests; this catch only preserves that contract. */ + if (isGitHubRateLimitedError(error) || isRetryableJobError(error)) throw error; + console.error( + JSON.stringify({ + level: "error", + event: "pr_public_surface_failed", deliveryId, - baseSha: live?.base?.sha ?? null, - liveFacts, - ...(previewPollAttempt !== undefined ? { previewPollAttempt } : {}), - ...(options.skipAiReview || autoreviewPaused ? { skipAiReview: true } : {}), - ...(options.force || pendingRetriggerForceReview ? { forceAiReview: true } : {}), - hasPendingRefreshSignal: otherRefreshReasons || reviewsCacheStale, - }, - ), - ).catch((error) => { - /* v8 ignore next -- retryable/rate-limit propagation is exercised by queue retry tests; this catch only preserves that contract. */ - if (isGitHubRateLimitedError(error) || isRetryableJobError(error)) throw error; - console.error( - JSON.stringify({ - level: "error", - event: "pr_public_surface_failed", - deliveryId, - repository: repoFullName, - pullNumber: prNumber, - error: errorMessage(error), - }), - ); - return undefined; - }); - await withReviewPipelineSpan( - "selfhost.review.maintenance", - { - installationId, - repoFullName, - pullNumber: pr.number, - operation: "maintenance", - decisionOutcome: gate?.conclusion, - }, - () => - maybeRunAgentMaintenance(env, { + repository: repoFullName, + pullNumber: prNumber, + error: errorMessage(error), + }), + ); + return undefined; + }); + await withReviewPipelineSpan( + "selfhost.review.maintenance", + { installationId, repoFullName, - repo, - pr, - settings, - otherOpenPullRequests, - deliveryId, - gate, - liveFacts, - }), - ).catch((error) => { - // #9025: rate-limit / retryable errors (chiefly PrActuationLockContendedError from the maintenance - // lock claim) MUST reach the queue so the disposition retries instead of being logged-and-dropped -- - // the same propagation contract the review pipeline's own catch sites already follow. - if (isGitHubRateLimitedError(error) || isRetryableJobError(error)) throw error; - console.error( - JSON.stringify({ - level: "error", - event: "agent_maintenance_failed", - deliveryId, - repository: repoFullName, - pullNumber: prNumber, - error: errorMessage(error), - }), - ); - }); + pullNumber: pr.number, + operation: "maintenance", + decisionOutcome: gate?.conclusion, + }, + () => + maybeRunAgentMaintenance(env, { + installationId, + repoFullName, + repo, + pr, + settings, + otherOpenPullRequests, + deliveryId, + gate, + liveFacts, + preAcquiredActuationLock: actuationLock, + }), + ).catch((error) => { + // #9025: rate-limit / retryable errors (chiefly PrActuationLockContendedError from the maintenance + // lock claim) MUST reach the queue so the disposition retries instead of being logged-and-dropped -- + // the same propagation contract the review pipeline's own catch sites already follow. + if (isGitHubRateLimitedError(error) || isRetryableJobError(error)) throw error; + console.error( + JSON.stringify({ + level: "error", + event: "agent_maintenance_failed", + deliveryId, + repository: repoFullName, + pullNumber: prNumber, + error: errorMessage(error), + }), + ); + }); + } finally { + await releasePrActuationLock(env, repoFullName, pr.number, actuationLock.ownerToken); + } return true; } @@ -6906,89 +6955,117 @@ async function handlePullRequestWebhookEvent( pr.number, pr.headSha, ); - gate = await withReviewPipelineSpan( - "selfhost.review.public_surface", - { - installationId, - repoFullName, - pullNumber: pr.number, - operation: "public_surface", - }, - () => - maybePublishPrPublicSurface( - env, + // #9013: ONE per-PR actuation-lock claim spans the publish pass AND the maintenance pass right after + // it -- see reReviewStoredPullRequest's identical claim (the sweep/CI-completion sibling of this + // webhook path) for the full race this closes: maybePublishPrPublicSurface used to run with no lock + // at all, so a webhook delivery racing a sweep re-review for the SAME PR could BOTH publish (duplicate + // gate check-runs, a lock-losing pass's placeholder overwriting a real verdict). Threaded into both + // maybePublishPrPublicSurface (preAcquiredActuationLock, covering its internal type-label section) and + // maybeRunAgentMaintenance (preAcquiredActuationLock) so "does another pass already own this PR" is + // one question with one answer for the whole unit. A losing pass defers the WHOLE unit (throws, + // uncaught below -- reaches the queue's retry path exactly like maybeRunAgentMaintenance's own + // pre-existing contention throw). + const actuationLock = await claimPrActuationLock(env, repoFullName, pr.number); + if (!actuationLock.acquired) { + await recordAuditEvent(env, { + eventType: "github_app.pr_public_surface_lock_contended", + actor: null, + targetKey: `${repoFullName}#${pr.number}`, + outcome: "queued", + detail: "Another pass holds this PR's actuation lock; the publish-and-maintain pass retries instead of racing it.", + metadata: { deliveryId, repoFullName }, + }).catch(() => undefined); + throw new PrActuationLockContendedError(repoFullName, pr.number, "public-surface-publish"); + } + try { + gate = await withReviewPipelineSpan( + "selfhost.review.public_surface", + { installationId, repoFullName, - pr, - repo, - settings, - advisory, - otherOpenPullRequests, - { - deliveryId, - authorType: payloadPullRequest.user?.type, - action: payload.action, - eventName, - baseSha: payloadPullRequest.base?.sha ?? null, - liveFacts, - ...(pendingRetriggerForceReview ? { forceAiReview: true } : {}), - }, - ), - ).catch((error) => { - if (isGitHubRateLimitedError(error) || isRetryableJobError(error)) throw error; - console.error( - JSON.stringify({ - level: "error", - event: "pr_public_surface_failed", - deliveryId, - repository: payload.repository?.full_name, pullNumber: pr.number, - error: errorMessage(error), - }), - ); - return undefined; - }); - // #778 maintainer auto-maintain: act on the PR's state (label/review/merge/close) per the repo's - // autonomy config, after the gate has run. The function self-guards on agent config; best-effort here - // so it never blocks the gate or public surface. - await withReviewPipelineSpan( - "selfhost.review.maintenance", - { - installationId, - repoFullName, - pullNumber: pr.number, - operation: "maintenance", - decisionOutcome: gate?.conclusion, - }, - () => - maybeRunAgentMaintenance(env, { + operation: "public_surface", + }, + () => + maybePublishPrPublicSurface( + env, + installationId, + repoFullName, + pr, + repo, + settings, + advisory, + otherOpenPullRequests, + { + deliveryId, + authorType: payloadPullRequest.user?.type, + action: payload.action, + eventName, + baseSha: payloadPullRequest.base?.sha ?? null, + liveFacts, + ...(pendingRetriggerForceReview ? { forceAiReview: true } : {}), + preAcquiredActuationLock: actuationLock, + }, + ), + ).catch((error) => { + if (isGitHubRateLimitedError(error) || isRetryableJobError(error)) throw error; + console.error( + JSON.stringify({ + level: "error", + event: "pr_public_surface_failed", + deliveryId, + repository: payload.repository?.full_name, + pullNumber: pr.number, + error: errorMessage(error), + }), + ); + return undefined; + }); + // #778 maintainer auto-maintain: act on the PR's state (label/review/merge/close) per the repo's + // autonomy config, after the gate has run. The function self-guards on agent config; best-effort here + // so it never blocks the gate or public surface. + await withReviewPipelineSpan( + "selfhost.review.maintenance", + { installationId, repoFullName, - repo, - pr, - settings, - otherOpenPullRequests, - deliveryId, - gate, - liveFacts, - }), - ).catch((error) => { - // #9025: same propagation contract as the sibling maintenance catch above -- a retryable error - // (chiefly the maintenance lock's own PrActuationLockContendedError) must reach the queue's retry - // path instead of being logged-and-dropped, or the disposition is silently lost. - if (isGitHubRateLimitedError(error) || isRetryableJobError(error)) throw error; - /* v8 ignore next -- best-effort: auto-maintain failures are logged, never surfaced to the gate. */ - console.error( - JSON.stringify({ - level: "error", - event: "agent_maintenance_failed", - deliveryId, - repository: repoFullName, pullNumber: pr.number, - error: errorMessage(error), - }), - ); - }); + operation: "maintenance", + decisionOutcome: gate?.conclusion, + }, + () => + maybeRunAgentMaintenance(env, { + installationId, + repoFullName, + repo, + pr, + settings, + otherOpenPullRequests, + deliveryId, + gate, + liveFacts, + preAcquiredActuationLock: actuationLock, + }), + ).catch((error) => { + // #9025: same propagation contract as the sibling maintenance catch above -- a retryable error + // (chiefly the maintenance lock's own PrActuationLockContendedError) must reach the queue's retry + // path instead of being logged-and-dropped, or the disposition is silently lost. + if (isGitHubRateLimitedError(error) || isRetryableJobError(error)) throw error; + /* v8 ignore next -- best-effort: auto-maintain failures are logged, never surfaced to the gate. */ + console.error( + JSON.stringify({ + level: "error", + event: "agent_maintenance_failed", + deliveryId, + repository: repoFullName, + pullNumber: pr.number, + error: errorMessage(error), + }), + ); + }); + } finally { + await releasePrActuationLock(env, repoFullName, pr.number, actuationLock.ownerToken); + } } // Reputation (convergence, flag-gated by LOOPOVER_REVIEW_REPUTATION). After the gate decides, record this // submitter's terminal outcome (merged / closed / manual) so the INTERNAL reputation stays current. The @@ -9095,6 +9172,14 @@ async function maybePublishPrPublicSurface( // besides the head SHA could make the published output differ from what is already live. hasPendingRefreshSignal?: boolean | undefined; liveFacts: LiveGithubFacts; + // A {@link claimPrActuationLock} claim the CALLER already acquired (#9013) before this call, spanning this + // publish pass AND the maybeRunAgentMaintenance call that follows it — see that function's own + // preAcquiredActuationLock doc comment for the full contract. Threaded down to the type-label block below + // (the only section of this function that itself claims the SAME per-PR actuation lock): when supplied and + // `.acquired`, that block trusts it instead of re-claiming, which would contend against the caller's own + // claim and always lose. Absent (any direct/test caller that doesn't thread it) ⇒ the type-label block + // claims + releases its own lock exactly as before. + preAcquiredActuationLock?: TransientLockClaim | undefined; }, ): Promise | undefined> { const author = pr.authorLogin ?? null; @@ -9345,9 +9430,21 @@ async function maybePublishPrPublicSurface( // a correct propagation_exclusive decision, followed within 30-90s by a second concurrent pass computing // a DIFFERENT (wrong) verdict that then overwrote the first. A losing pass must defer to the next tick, // never compute-and-act on a stale/racing verdict for a PR another pass is actively deciding for. - const typeLabelLock = await claimPrActuationLock(env, repoFullName, pr.number); + // #9013: prefer the caller's OWN claim (webhook.preAcquiredActuationLock) exactly like + // maybeRunAgentMaintenance's identical preAcquiredActuationLock contract above -- re-claiming here when the + // caller already holds this SAME lock around the whole publish pass would only contend against that claim + // and always lose. Absent (any direct/test caller) ⇒ claim it here exactly as before. + const selfClaimedTypeLabelLock = webhook.preAcquiredActuationLock === undefined; + const typeLabelLock = + webhook.preAcquiredActuationLock ?? (await claimPrActuationLock(env, repoFullName, pr.number)); if (!typeLabelLock.acquired) { await logTypeLabelSkip(env, repoFullName, pr.number, "lock_contended"); + // #9013: promoted from "skip the label and keep publishing anyway" to deferring the WHOLE pass -- the + // prior behavior let a lock-losing pass publish a full surface (comment, check run, disposition) built + // from a stale/racing read while another pass was actively mutating this same PR, exactly the class of + // duplicate-publish bug this lock exists to prevent. Throwing here (retryable, same shape as the sibling + // agent-maintenance contention branch above) defers the entire publish to the queue's fast retry instead. + throw new PrActuationLockContendedError(repoFullName, pr.number, "type-label"); } else { try { // Same reasoning as `typeLabelsEnabled` above: `settings.typeLabels` is optional only for @@ -9461,7 +9558,10 @@ async function maybePublishPrPublicSurface( metadata: { labels: [], source: null }, }).catch(() => undefined); } finally { - await releasePrActuationLock(env, repoFullName, pr.number, typeLabelLock.ownerToken); + // #9013: only release a lock THIS block actually claimed -- a caller-supplied preAcquiredActuationLock + // must keep covering the rest of the caller's publish pass (and the maintenance call after it). + if (selfClaimedTypeLabelLock) + await releasePrActuationLock(env, repoFullName, pr.number, typeLabelLock.ownerToken); } } } else { diff --git a/test/unit/queue-2.test.ts b/test/unit/queue-2.test.ts index 0fed47bf4b..56e1e5ecf5 100644 --- a/test/unit/queue-2.test.ts +++ b/test/unit/queue-2.test.ts @@ -3335,7 +3335,7 @@ describe("queue processors", () => { expect(store.size).toBe(0); }); - it("INVARIANT (#2129 per-PR lock): a maintenance pass defers when another pass already holds the PR's lock", async () => { + it("INVARIANT (#2129/#9013 per-PR lock): a publish-and-maintain pass defers when another pass already holds the PR's lock", 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: {}, events: [] } }); await upsertRepositoryFromGitHub(env, { name: "agent-repo", full_name: "owner/agent-repo", private: false, owner: { login: "owner" } }, 9001); @@ -3382,6 +3382,10 @@ describe("queue processors", () => { // disposition a second time with no trace). Throwing the retryable error is the same contract // review-evasion.ts's withPrActuationLock already used for this exact condition; the queue honors its // retryAfterMs via consumingRetryDelayMs, so the disposition lands once the contending pass releases. + // #9013: ONE lock now spans publish-then-maintain, claimed BEFORE the publish call -- so this contention + // surfaces at the public-surface publish stage (reReviewStoredPullRequest's own claim), never even + // reaching maybeRunAgentMaintenance's own (now pre-satisfied) claim. Strictly earlier and stronger than + // the pre-#9013 behavior, where only the trailing maintenance call was locked and publish ran unprotected. await expect( processJob(env, { type: "agent-regate-pr", deliveryId: "race-sweep", repoFullName: "owner/agent-repo", prNumber: 7, installationId: 9001 }), ).rejects.toMatchObject({ name: "PrActuationLockContendedError", retryKind: "pr_actuation_lock_contended" }); @@ -3393,15 +3397,15 @@ describe("queue processors", () => { expect(actionAudits?.n).toBe(0); // ...and the contention itself is now visible in the ledger instead of leaving no trace at all. const contendedAudit = await env.DB.prepare("select outcome, detail from audit_events where event_type = ? and target_key = ?") - .bind("github_app.agent_maintenance_lock_contended", "owner/agent-repo#7") + .bind("github_app.pr_public_surface_lock_contended", "owner/agent-repo#7") .first<{ outcome: string; detail: string }>(); expect(contendedAudit?.outcome).toBe("queued"); - // #9025: the contention audit write is best-effort -- a failing write must never swallow or replace the - // retry itself. The disposition's durability cannot depend on the ledger being writable. + // #9025/#9013: the contention audit write is best-effort -- a failing write must never swallow or replace + // the retry itself. The disposition's durability cannot depend on the ledger being writable. const originalRecordAuditEvent = repositoriesModule.recordAuditEvent; const auditSpy = vi.spyOn(repositoriesModule, "recordAuditEvent").mockImplementation(async (auditEnv, event) => { - if (event.eventType === "github_app.agent_maintenance_lock_contended") throw new Error("audit DB down"); + if (event.eventType === "github_app.pr_public_surface_lock_contended") throw new Error("audit DB down"); await originalRecordAuditEvent(auditEnv, event); }); await expect( diff --git a/test/unit/queue-3.test.ts b/test/unit/queue-3.test.ts index aed167b889..c2907792f4 100644 --- a/test/unit/queue-3.test.ts +++ b/test/unit/queue-3.test.ts @@ -3110,10 +3110,12 @@ describe("queue processors", () => { return Response.json({}); }); - // #9025: the trailing maintenance pass hits the SAME still-held actuation lock and now throws the - // retryable contention error rather than returning silently, so the disposition retries instead of being - // dropped. The early-cap short-circuit under test here is a different call site and still defers cleanly - // (its own `return false`, unchanged) -- both assertions below are exactly as before. + // #9013: the public-surface publish call now claims this SAME still-held actuation lock BEFORE it (or the + // trailing maintenance pass) does anything, so the contention now surfaces there instead of at the + // trailing maintenance call's own (later) claim -- publish and maintain defer together, as ONE unit. The + // early-cap short-circuit under test here is a different call site and still defers cleanly (its own + // `return false`, unchanged); it's the REST of the pipeline that no longer "falls through" past a still- + // held lock the way it used to pre-#9013. await expect( processJob(env, { type: "github-webhook", @@ -3128,10 +3130,10 @@ describe("queue processors", () => { }), ).rejects.toMatchObject({ name: "PrActuationLockContendedError" }); - // The early close DEFERRED (no PATCH close fired from it) and the pipeline fell through, mirroring the - // author-lock contention semantics one namespace over. + // The early close DEFERRED (no PATCH close fired from it) and the REST of the pass deferred too, both + // parts of the SAME lock namespace now (#9013) -- unlike pre-#9013, the AI review never ran either. expect(seen.closed).toBe(false); - expect(aiCalls).toBeGreaterThan(0); + expect(aiCalls).toBe(0); }); it("early cap short-circuit (#7284-fix): close autonomy not granted (observe) plans nothing early -- no crash, falls through to the normal pipeline", async () => { diff --git a/test/unit/queue-4.test.ts b/test/unit/queue-4.test.ts index e5af4ad5a0..710c0a29a9 100644 --- a/test/unit/queue-4.test.ts +++ b/test/unit/queue-4.test.ts @@ -2117,6 +2117,96 @@ describe("queue processors", () => { expect(usageEvents).toEqual(expect.arrayContaining([expect.objectContaining({ surface: "github_app", eventName: "pr_panel_retriggered", outcome: "completed" })])); }); + it("#9013: a manual panel rerun's own type-label block defers the WHOLE publish pass when it self-claims a contended actuation lock", async () => { + // Unlike the two webhook/sweep call sites (reReviewStoredPullRequest, handlePullRequestWebhookEvent), the + // manual panel-retrigger call site does NOT thread a preAcquiredActuationLock into + // maybePublishPrPublicSurface -- so its type-label block still claims the lock itself, exactly as before + // #9013. This exercises that self-claimed path's OWN promoted contention behavior: throw and defer the + // whole pass, instead of the pre-#9013 "skip the label and keep publishing anyway". + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await upsertRepositoryFromGitHub(env, { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, 123); + await upsertRepositorySettings(env, { + repoFullName: "JSONbored/gittensory", + autoLabelEnabled: false, + commandAuthorization: { default: ["maintainer", "collaborator", "confirmed_miner"], commands: { "review-now": ["maintainer"] } }, + }); + await upsertRepoFocusManifest(env, "JSONbored/gittensory", { settings: { commentMode: "all_prs", publicAudienceMode: "oss_maintainer", publicSignalLevel: "standard", publicSurface: "comment_only", checkRunMode: "off", includeMaintainerAuthors: true } }); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { + number: 91, + title: "Refresh panel under contention", + state: "open", + user: { login: "contributor" }, + author_association: "CONTRIBUTOR", + head: { sha: "panel91" }, + labels: [], + body: "Validation: npm test", + }); + const checkedPanel = [ + "", + "", + "- [x] Re-run LoopOver review", + ].join("\n"); + let labelPosts = 0; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url === "https://api.gittensor.io/miners") { + return Response.json([ + { uid: 7, githubUsername: "contributor", githubId: "123", totalPrs: 4, totalMergedPrs: 3, totalOpenPrs: 1, totalClosedPrs: 0, totalOpenIssues: 0, totalClosedIssues: 0, totalSolvedIssues: 0, totalValidSolvedIssues: 0, isEligible: true, credibility: 1, eligibleRepoCount: 1 }, + ]); + } + if (url === "https://api.gittensor.io/miners/123") { + return Response.json({ repositories: [{ repositoryFullName: "JSONbored/gittensory", totalPrs: "4", totalMergedPrs: "3", totalOpenPrs: "1", totalClosedPrs: "0", totalOpenIssues: "0", totalClosedIssues: "0", isEligible: true, credibility: "1.000000" }] }); + } + if (url === "https://api.gittensor.io/miners/123/prs") return Response.json([]); + if (url === "https://mirror.gittensor.io/api/v1/miners/123/issues") return Response.json({ issues: [] }); + if (url.endsWith("/users/contributor")) return Response.json({ login: "contributor", public_repos: 2, followers: 1 }); + if (url.includes("/users/contributor/repos")) return Response.json([]); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.includes("/check-runs")) return Response.json({ id: 888 }); + if (url.includes("/collaborators/maintainer/permission")) return Response.json({ permission: "maintain" }); + if (url.includes("/issues/91/comments") && method === "GET") { + return Response.json([{ id: 777, body: checkedPanel, user: { login: "loopover-orb[bot]", type: "Bot" } }]); + } + if (url.includes("/issues/comments/777") && method === "PATCH") return Response.json({ id: 777 }); + if (url.includes("/issues/91/labels") && method === "POST") { + labelPosts += 1; + return Response.json([]); + } + return new Response("not found", { status: 404 }); + }); + + // Simulates a concurrent pass (a sibling webhook delivery, or the sweep) already holding this exact PR's + // actuation lock when the manual retrigger reaches the type-label block's own self-claim. + const held = await claimPrActuationLock(env, "JSONbored/gittensory", 91); + expect(held.acquired).toBe(true); + try { + await expect( + processJob(env, { + type: "github-webhook", + deliveryId: "panel-retrigger-lock-contended", + eventName: "issue_comment", + payload: { + action: "edited", + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + issue: { number: 91, title: "Refresh panel under contention", state: "open", user: { login: "contributor" }, pull_request: {} }, + comment: { id: 777, body: checkedPanel, user: { login: "loopover-orb[bot]", type: "Bot" } }, + sender: { login: "maintainer", type: "User" }, + }, + }), + ).rejects.toMatchObject({ name: "PrActuationLockContendedError" }); + } finally { + await releasePrActuationLock(env, "JSONbored/gittensory", 91, held.ownerToken); + } + + expect(labelPosts).toBe(0); // the label decision never ran -- the whole pass deferred instead + const typeLabelEvents = await env.DB.prepare( + "select outcome, detail from audit_events where event_type = 'github_app.type_label_decision' and target_key = 'JSONbored/gittensory#91'", + ).all<{ outcome: string; detail: string }>(); + expect(typeLabelEvents.results).toEqual([{ outcome: "denied", detail: "lock_contended" }]); + }); + it("defers a manual panel rerun while CI is still running", async () => { const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); await upsertRepositoryFromGitHub(env, { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, 123); diff --git a/test/unit/queue-5.test.ts b/test/unit/queue-5.test.ts index 3b37d87a3f..fb959a9b4e 100644 --- a/test/unit/queue-5.test.ts +++ b/test/unit/queue-5.test.ts @@ -6674,7 +6674,7 @@ describe("queue processors", () => { expect(seen.removed.sort()).toEqual(["gittensor:bug", "gittensor:priority"]); }); - it("REGRESSION (#regression-safe-propagation): a contended per-PR actuation lock skips the label decision entirely instead of racing the pass that already holds it", async () => { + it("REGRESSION (#regression-safe-propagation/#9013): a contended per-PR actuation lock defers the WHOLE publish-and-maintain pass, not just the label decision", async () => { const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); await upsertRepositoryFromGitHub(env, { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, 123); await upsertRepositorySettings(env, { @@ -6701,21 +6701,26 @@ describe("queue processors", () => { stubPropagationFetch(223, 1, seen, () => Response.json({ number: 1, state: "open", user: { login: "contributor" }, labels: ["gittensor:priority"] })); // Simulates a concurrent pass (a sibling webhook delivery, or the sweep) already holding this exact - // PR's actuation lock when this pass reaches the type-label block. + // PR's actuation lock. #9013: the public-surface publish call now claims this SAME lock BEFORE it does + // anything -- including before the type-label block ever runs -- so a contended pass defers the WHOLE + // publish-and-maintain unit instead of reaching (and self-recovering from) the type-label block's own + // (now unreachable in this scenario) inner claim. const held = await claimPrActuationLock(env, "JSONbored/gittensory", 223); expect(held.acquired).toBe(true); try { - await processJob(env, { - type: "github-webhook", - deliveryId: "priority-propagation-lock-contended", - eventName: "pull_request", - payload: { - action: "opened", - 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: 223, title: "fix: some bug", state: "open", user: { login: "contributor" }, author_association: "NONE", head: { sha: "sha223" }, labels: [], body: "Fixes #1" }, - }, - }); + await expect( + processJob(env, { + type: "github-webhook", + deliveryId: "priority-propagation-lock-contended", + eventName: "pull_request", + payload: { + action: "opened", + 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: 223, title: "fix: some bug", state: "open", user: { login: "contributor" }, author_association: "NONE", head: { sha: "sha223" }, labels: [], body: "Fixes #1" }, + }, + }), + ).rejects.toMatchObject({ name: "PrActuationLockContendedError" }); } finally { await releasePrActuationLock(env, "JSONbored/gittensory", 223, held.ownerToken); } @@ -6724,10 +6729,16 @@ describe("queue processors", () => { expect(seen.issueFetches).toBe(0); expect(seen.posted).toEqual([]); expect(seen.removed).toEqual([]); - const events = await env.DB.prepare( + // The type-label block's OWN "lock_contended" self-claim path never runs -- the outer publish-level + // claim (#9013) already deferred the whole pass before maybePublishPrPublicSurface was ever called. + const typeLabelEvents = await env.DB.prepare( `select outcome, detail from audit_events where event_type = 'github_app.type_label_decision' and target_key = 'JSONbored/gittensory#223'`, ).all(); - expect(events.results).toEqual([{ outcome: "denied", detail: "lock_contended" }]); + expect(typeLabelEvents.results).toEqual([]); + const publishLockEvents = await env.DB.prepare( + `select outcome from audit_events where event_type = 'github_app.pr_public_surface_lock_contended' and target_key = 'JSONbored/gittensory#223'`, + ).all<{ outcome: string }>(); + expect(publishLockEvents.results).toEqual([{ outcome: "queued" }]); }); describe("review-family events never touch the type label (#4818 follow-up)", () => { diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index fc9ca62808..bd3e2042d8 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -8512,16 +8512,28 @@ describe("queue processors", () => { // Two DIFFERENT delivery ids (matching the real incident's two distinct webhook deliveries) for the SAME // repo/PR/head, fired concurrently — neither awaits the other before both are in flight. - await Promise.all([ + // #9013: the per-PR actuation lock now ALSO wraps the whole public-surface publish call (not just the + // narrower ai-review-lock deep inside it), so the loser of this race defers the WHOLE pass and its + // processJob call REJECTS with PrActuationLockContendedError instead of completing with a placeholder -- + // Promise.allSettled (not Promise.all) so both outcomes are observed rather than the first rejection + // aborting the assertion before the winner's own call has necessarily settled. + const [outcomeA, outcomeB] = await Promise.allSettled([ processJob(env, { type: "agent-regate-pr", deliveryId: "delivery-a", repoFullName: "JSONbored/gittensory", prNumber: 91, installationId: 123 }), processJob(env, { type: "agent-regate-pr", deliveryId: "delivery-b", repoFullName: "JSONbored/gittensory", prNumber: 91, installationId: 123 }), ]); + const settled = [outcomeA, outcomeB]; + const fulfilled = settled.filter((outcome) => outcome.status === "fulfilled"); + const rejected = settled.filter((outcome) => outcome.status === "rejected"); + expect(fulfilled).toHaveLength(1); // the lock winner completes a real review normally + expect(rejected).toHaveLength(1); // the lock loser defers the whole pass instead of racing it + expect((rejected[0] as PromiseRejectedResult).reason).toMatchObject({ name: "PrActuationLockContendedError" }); // The DISCRIMINATING assertion (fails on unfixed code, verified by temporarily reverting the fix): exactly // ONE pass logged a genuine cache miss (read the cache, found nothing, ran fresh) — the loser never reached - // the cache-read at all, because the lock now wraps that read too, not just the LLM call. Without the fix - // this is 2: both passes independently reach the cache-read and both log a miss before either one's - // runAiReviewForAdvisory-internal lock (the historical, narrower placement) ever engages. + // the cache-read at all: pre-#9013 that was because the (narrower) ai-review-lock wrapped the cache-read + // deep inside runAiReviewForAdvisory; post-#9013 the loser never even reaches maybePublishPrPublicSurface, + // since the coarser per-PR actuation lock now defers the whole pass before that call. Without either fix + // this is 2: both passes independently reach the cache-read and both log a miss. const missAudit = await env.DB.prepare("select count(*) as n from audit_events where event_type = ? and target_key = ?") .bind("github_app.ai_review_cache_miss", "JSONbored/gittensory#91") .first<{ n: number }>();