diff --git a/apps/server/src/orchestration-v2/Orchestrator.ts b/apps/server/src/orchestration-v2/Orchestrator.ts index 37c4a75f663b6..17978c64e8e0f 100644 --- a/apps/server/src/orchestration-v2/Orchestrator.ts +++ b/apps/server/src/orchestration-v2/Orchestrator.ts @@ -4161,6 +4161,14 @@ const makeOrchestrator = Effect.fn("orchestrationV2.Orchestrator.layer")(functio ...(existing.endedWatch?.checksAwaitedSince !== undefined && { checksAwaitedSince: existing.endedWatch.checksAwaitedSince, }), + // A user re-watch is not new PR progress. Restore the one-shot + // stall watermark so re-watching cannot manufacture a retry wake. + ...(existing.endedWatch?.lastProgressAt !== undefined && { + lastProgressAt: existing.endedWatch.lastProgressAt, + }), + ...(existing.endedWatch?.releaseStallNotifiedAt !== undefined && { + releaseStallNotifiedAt: existing.endedWatch.releaseStallNotifiedAt, + }), remarksThrough: startedAt, remarkIds: [], wakes: 0, diff --git a/apps/server/src/orchestration-v2/PullRequestWatchReactor.ts b/apps/server/src/orchestration-v2/PullRequestWatchReactor.ts index 5050ac3272894..0d8e2b8e99b10 100644 --- a/apps/server/src/orchestration-v2/PullRequestWatchReactor.ts +++ b/apps/server/src/orchestration-v2/PullRequestWatchReactor.ts @@ -38,6 +38,7 @@ import * as ProjectionStore from "./ProjectionStore.ts"; import { evaluatePullRequestWatch, PULL_REQUEST_WATCH_NO_CHECKS_MS, + PULL_REQUEST_WATCH_RELEASE_STALL_MS, pullRequestWatchMessage, } from "./pullRequestWatch.ts"; @@ -152,20 +153,28 @@ function planRead( watch.checksAwaitedSince != null && now - Date.parse(watch.checksAwaitedSince) >= PULL_REQUEST_WATCH_NO_CHECKS_MS, ); + const stalledDue = group.targets.some( + ({ watch }) => + watch.passed && + watch.releaseStallNotifiedAt === undefined && + now - Date.parse(watch.lastProgressAt ?? watch.startedAt) >= + PULL_REQUEST_WATCH_RELEASE_STALL_MS, + ); if (fingerprint === null) { const read = last.inFlight || !last.remarksComplete || last.fingerprint !== group.fingerprint || now - last.at >= QUIET_REREAD_MS; - return { detail: read || checksDue, activity: read }; + return { detail: read || checksDue || stalledDue, activity: read }; } const activity = last.remarks !== fingerprint.remarks || !last.remarksComplete || now - last.at >= FINGERPRINT_REREAD_MS; return { - detail: activity || checksDue || last.status !== fingerprint.status || last.inFlight, + detail: + activity || checksDue || stalledDue || last.status !== fingerprint.status || last.inFlight, activity, }; } @@ -182,7 +191,9 @@ function watchesEqual(left: ThreadPullRequestWatch, right: ThreadPullRequestWatc left.remarkIds.join("\n") === right.remarkIds.join("\n") && left.conflicting === right.conflicting && left.wakes === right.wakes && - left.checksAwaitedSince === right.checksAwaitedSince + left.checksAwaitedSince === right.checksAwaitedSince && + left.lastProgressAt === right.lastProgressAt && + left.releaseStallNotifiedAt === right.releaseStallNotifiedAt ); } diff --git a/apps/server/src/orchestration-v2/pullRequestWatch.test.ts b/apps/server/src/orchestration-v2/pullRequestWatch.test.ts index 6ab0191649490..f90093865b36c 100644 --- a/apps/server/src/orchestration-v2/pullRequestWatch.test.ts +++ b/apps/server/src/orchestration-v2/pullRequestWatch.test.ts @@ -389,6 +389,93 @@ describe("evaluatePullRequestWatch", () => { ); }); + it("wakes only once when a green PR has no release progress for an hour", () => { + const green = detail({ checks: [check("test", "success")] }); + const first = evaluate(watch(), green, noRemarks, Date.parse(STARTED)); + assert.deepEqual(first.changes, [{ kind: "checks-passed", count: 1, required: false }]); + const before = evaluate(first.next, green, noRemarks, Date.parse(STARTED) + 59 * 60_000); + assert.deepEqual(before.changes, []); + const stalled = evaluate(before.next, green, noRemarks, Date.parse(STARTED) + 61 * 60_000); + assert.deepEqual(stalled.changes, [{ kind: "release-stalled", minutes: 61 }]); + const again = evaluate(stalled.next, green, noRemarks, Date.parse(STARTED) + 24 * 60 * 60_000); + assert.deepEqual(again.changes, []); + const { text } = pullRequestWatchMessage({ + number: 1, + url: "https://github.com/o/r/pull/1", + baseBranch: "main", + headSha: green.headSha ?? null, + report: stalled, + }); + assert.include(text, "merge-group CI"); + assert.include(text, "Do not just re-watch"); + }); + + it("does not stall pending checks or immediately after new material progress", () => { + const green = detail({ checks: [check("test", "success")] }); + const start = Date.parse(STARTED); + const initial = watch({ + headSha: "aaaaaaaaaa", + passed: true, + passedChecks: ["test"], + lastProgressAt: STARTED, + }); + const pending = evaluate(initial, detail(), noRemarks, start + 61 * 60_000); + assert.deepEqual(pending.changes, []); + const changed = evaluate( + initial, + detail({ ...green, headSha: "bbbbbbbbbb" }), + noRemarks, + start + 61 * 60_000, + ); + assert.deepEqual(changed.changes, [{ kind: "checks-passed", count: 1, required: false }]); + assert.equal(changed.next.releaseStallNotifiedAt, undefined); + assert.deepEqual( + evaluate( + changed.next, + detail({ ...green, headSha: "bbbbbbbbbb" }), + noRemarks, + start + 62 * 60_000, + ).changes, + [], + ); + const later = evaluate( + changed.next, + detail({ ...green, headSha: "bbbbbbbbbb" }), + noRemarks, + start + 122 * 60_000, + ); + assert.deepEqual(later.changes, [{ kind: "release-stalled", minutes: 61 }]); + }); + + it("clears the stalled-release clock when a same-head conflict is resolved", () => { + const green = detail({ checks: [check("test", "success")] }); + const t0 = Date.parse(STARTED); + const before = evaluate( + watch({ + headSha: "aaaaaaaaaa", + passed: true, + passedChecks: ["test"], + conflicting: true, + lastProgressAt: STARTED, + releaseStallNotifiedAt: STARTED, + }), + green, + noRemarks, + t0 + 3 * 60 * 60_000, + ); + assert.deepEqual(before.changes, []); + assert.equal(before.next.conflicting, false); + assert.equal(before.next.releaseStallNotifiedAt, undefined); + assert.deepEqual( + evaluate(before.next, green, noRemarks, t0 + 3 * 60 * 60_000 + 60_000).changes, + [], + ); + assert.deepEqual( + evaluate(before.next, green, noRemarks, t0 + 4 * 60 * 60_000 + 60_000).changes, + [{ kind: "release-stalled", minutes: 61 }], + ); + }); + it("does not spend the comment wake limit on check results", () => { const tired = watch({ headSha: "aaaaaaaaaa", wakes: PULL_REQUEST_WATCH_WAKE_LIMIT - 1 }); const result = evaluatePullRequestWatch( diff --git a/apps/server/src/orchestration-v2/pullRequestWatch.ts b/apps/server/src/orchestration-v2/pullRequestWatch.ts index f4c0000847236..1a1e453e79860 100644 --- a/apps/server/src/orchestration-v2/pullRequestWatch.ts +++ b/apps/server/src/orchestration-v2/pullRequestWatch.ts @@ -11,6 +11,8 @@ import * as DateTime from "effect/DateTime"; export const PULL_REQUEST_WATCH_WAKE_LIMIT = 10; /** How long a head may go without any check before the agent is told none reported. */ export const PULL_REQUEST_WATCH_NO_CHECKS_MS = 15 * 60_000; +/** Give one actionable wake when a green head makes no release progress for an hour. */ +export const PULL_REQUEST_WATCH_RELEASE_STALL_MS = 60 * 60_000; const LISTED_ITEMS = 10; const SNIPPET_LENGTH = 200; @@ -19,7 +21,8 @@ export type PullRequestWatchChange = | { readonly kind: "checks-passed"; readonly count: number; readonly required: boolean } | { readonly kind: "remarks"; readonly remarks: ReadonlyArray } | { readonly kind: "no-checks"; readonly minutes: number } - | { readonly kind: "conflicting" }; + | { readonly kind: "conflicting" } + | { readonly kind: "release-stalled"; readonly minutes: number }; export interface PullRequestWatchReport { readonly changes: ReadonlyArray; @@ -136,6 +139,29 @@ export function evaluatePullRequestWatch( const conflicting = detail.mergeability === "unknown" ? toldConflict : detail.mergeability === "conflicting"; + // The PR head can be green while a repository-specific merge controller refuses + // release. Don't turn a quiet watch into an indefinite no-action state. + const materialProgress = + headMoved || + conflicting !== watch.conflicting || + changes.some((change) => change.kind !== "remarks"); + const lastProgressAt = materialProgress + ? DateTime.formatIso(DateTime.makeUnsafe(now)) + : (watch.lastProgressAt ?? watch.startedAt); + let releaseStallNotifiedAt = materialProgress ? undefined : watch.releaseStallNotifiedAt; + if ( + !materialProgress && + passed && + detail.checks.length > 0 && + releaseStallNotifiedAt === undefined && + now - Date.parse(lastProgressAt) >= PULL_REQUEST_WATCH_RELEASE_STALL_MS + ) { + changes.push({ + kind: "release-stalled", + minutes: Math.floor((now - Date.parse(lastProgressAt)) / 60_000), + }); + releaseStallNotifiedAt = DateTime.formatIso(DateTime.makeUnsafe(now)); + } const commentsOnly = changes.length > 0 && changes.every((change) => change.kind === "remarks"); const progress = headMoved || (changes.length > 0 && !commentsOnly); const wakes = (progress ? 0 : watch.wakes) + (commentsOnly ? 1 : 0); @@ -153,6 +179,8 @@ export function evaluatePullRequestWatch( conflicting, wakes, checksAwaitedSince, + lastProgressAt, + ...(releaseStallNotifiedAt === undefined ? {} : { releaseStallNotifiedAt }), }, exhausted: commentsOnly && wakes >= PULL_REQUEST_WATCH_WAKE_LIMIT, }; @@ -210,6 +238,10 @@ function changeLines( ]; case "conflicting": return [`- The branch now conflicts with ${context.baseBranch}.`]; + case "release-stalled": + return [ + `- This open PR has shown no material release progress for ${change.minutes} minutes despite passing head checks. Inspect the actual host merge queue, merge-group CI, or repository-specific merge controller, and repair its blocking verdict. Do not just re-watch or blindly requeue.`, + ]; } } @@ -219,6 +251,7 @@ const SUMMARY: Record = { remarks: "new comments", "no-checks": "no checks reported", conflicting: "merge conflict", + "release-stalled": "release progress stalled", }; export function pullRequestWatchMessage(input: { diff --git a/apps/server/src/orchestration-v2/runtimeLayer.test.ts b/apps/server/src/orchestration-v2/runtimeLayer.test.ts index b9f5cd049eec9..0d05f49357bd9 100644 --- a/apps/server/src/orchestration-v2/runtimeLayer.test.ts +++ b/apps/server/src/orchestration-v2/runtimeLayer.test.ts @@ -3632,6 +3632,27 @@ it.layer(layerTest)("OrchestrationV2LayerLive lifecycle", (it) => { (yield* orchestrator.getThreadShell(threadId))?.pullRequests?.[0]?.watch?.owedChecks, [], ); + // An already-delivered release-stall alert survives an explicit stop/re-watch. + yield* TestClock.adjust("61 minutes"); + yield* restarted.sweep; + yield* worker.drain(); + assert.deepEqual(yield* summaries, [ + "#12: checks passed", + "#12: checks passed", + "#12: release progress stalled", + ]); + const beforeRewatch = (yield* orchestrator.getThreadShell(threadId))?.pullRequests?.[0] + ?.watch; + assert.isDefined(beforeRewatch?.releaseStallNotifiedAt); + assert.isDefined(beforeRewatch?.lastProgressAt); + yield* watch(false, "end-first"); + yield* watch(true, "end-rewatch"); + const afterRewatch = (yield* orchestrator.getThreadShell(threadId))?.pullRequests?.[0]?.watch; + assert.equal(afterRewatch?.releaseStallNotifiedAt, beforeRewatch?.releaseStallNotifiedAt); + assert.equal(afterRewatch?.lastProgressAt, beforeRewatch?.lastProgressAt); + yield* restarted.sweep; + yield* worker.drain(); + assert.equal((yield* summaries).length, 3); yield* watch(false, "end"); }), ); @@ -3963,7 +3984,8 @@ it.layer(layerTest)("OrchestrationV2LayerLive lifecycle", (it) => { yield* TestClock.adjust("1 hour"); yield* sweep; const settled = yield* wakes; - assert.lengthOf(settled, 2); + assert.lengthOf(settled, 3); + assert.equal(settled[2]!.notification?.summary, "#14: release progress stalled"); assert.lengthOf( settled.filter((message) => message.text.includes("No checks have reported")), 1, diff --git a/packages/contracts/src/threadPullRequest.ts b/packages/contracts/src/threadPullRequest.ts index 3daee9ce197e2..be06c3af2edcc 100644 --- a/packages/contracts/src/threadPullRequest.ts +++ b/packages/contracts/src/threadPullRequest.ts @@ -100,6 +100,10 @@ export const ThreadPullRequestWatch = Schema.Struct({ * none did. Null once either happened; absent on watches saved before it existed. */ checksAwaitedSince: Schema.optional(Schema.NullOr(IsoDateTime)), + /** Last substantive progress for this watched head; absent in older watches. */ + lastProgressAt: Schema.optional(IsoDateTime), + /** Last one-shot stalled-release notification; resets on substantive progress. */ + releaseStallNotifiedAt: Schema.optional(IsoDateTime), }); export type ThreadPullRequestWatch = typeof ThreadPullRequestWatch.Type;