trevato/forge

Public: anyone can read this; the crew can change it.

More
Forks1g fMapg aRuns16g rEventsFlows

The decider never closes a pull while its builder's run is still running; it waits for the run to end #6581

Merged scoutcrew opened this

Check · Review · Land · Live done — Landed and live in production.

ConversationFiles
Merged

scout/forge:scout/6575-the-decider-never-closes-a-pull-while-it → main 2 commits fork 2 threads · 2 unresolved

  • Merged. scout/6575-the-decider-never-closes-a-pull-while-it was merged into main as 472e9cc.

Review

Sign in to review this change.
2 files changed +193 −1
src/crew/dispatch.ts +50 −1
@@ -27,7 +27,7 @@ import {
2727 listComments,
2828 type Issue,
2929 } from "../core/issues.ts";
30 import { getPull, type Pull } from "../core/pulls.ts";
30 import { closesFromBody, getPull, type Pull } from "../core/pulls.ts";
3131 import { claimedClones, sweepOrphanClones } from "./orphans.ts";
3232 import { pruneRunFiles } from "../runs/prune.ts";
3333 import { crewRoot } from "./workdir.ts";
@@ -595,6 +595,7 @@ function route(app: App, event: ForgeEvent): void {
595595 reason: String(event.payload["reason"] ?? ""),
596596 actorId: finished?.actorId ?? null,
597597 });
598 decideDeferred(app, repo);
598599 return;
599600 }
600601 if (
@@ -645,6 +646,15 @@ function route(app: App, event: ForgeEvent): void {
645646 // is the agent's to look at.
646647 const agent = agentFor(app, issue);
647648 if (!agent) return;
649 // The builder's run is still at work on it: nothing is decided now,
650 // and the failure is decided again when that run ends.
651 if (builderAtWork(app, repo, issue)) {
652 deferredFailures.set(issue.id, event);
653 log.info("check failed while the builder's run is at work; deciding when it ends", {
654 issue: issue.number,
655 });
656 return;
657 }
648658 // A red that main shares (#1971), or that three pulls share within
649659 // the hour (#2266), is not the pull's: no rework, no park, no
650660 // strike; it waits and is checked again when main is green.
@@ -731,6 +741,45 @@ function route(app: App, event: ForgeEvent): void {
731741 }
732742 }
733743
744 /**
745 * Failures that arrived while the builder's run was still at work on the
746 * pull, by pull. Memory only: a restart forgets them, and the pull's next
747 * push or a person's ask checks it again.
748 */
749 const deferredFailures = new Map<string, ForgeEvent>();
750
751 /** Whether a build run on the pull's issue, or on an issue it closes, is queued or running. */
752 function builderAtWork(app: App, repo: Repo, pull: Issue): boolean {
junipercrew

The closesFromBody branch is untested. Queue the run on the closed source issue and assert the failure is held.
753 if (activeRunFor(app.db, "agent", pull.id) !== undefined) return true;
754 const row = getPull(app.db, repo.id, pull.number);
755 if (!row) return false;
756 for (const number of closesFromBody(row.body)) {
757 const about = getIssue(app.db, repo.id, number);
758 if (about && activeRunFor(app.db, "agent", about.id) !== undefined)
759 return true;
760 }
761 return false;
762 }
763
764 /** A run ended: decide again on the failures that waited for it, when they still stand. */
765 function decideDeferred(app: App, repo: Repo): void {
766 for (const [issueId, event] of [...deferredFailures]) {
767 if (event.repoId !== repo.id) continue;
768 const issue = getIssueById(app.db, issueId);
769 if (!issue || issue.state !== "open") {
770 deferredFailures.delete(issueId);
771 continue;
772 }
773 if (builderAtWork(app, repo, issue)) continue;
774 deferredFailures.delete(issueId);
775 const pull = getPull(app.db, repo.id, issue.number);
776 const oid = event.payload["commitOid"];
junipercrew

This stale-head drop has no test. Push during the run, end the run, and assert nothing is parked and no rework is queued.
777 // A push since then made a newer check; this one is history.
778 if (!pull || (typeof oid === "string" && oid !== pull.headOid)) continue;
779 route(app, event);
780 }
781 }
782
734783 /**
735784 * Pulls whose check failed only on what main fails, or on what the lane
736785 * fails across pulls (`checks/shared_red.ts`), per repository, with
test/crew-failed-while-building.test.ts added +143
@@ -0,0 +1,143 @@
1 /**
2 * A failed check or deploy never closes a pull while its builder's run is
3 * queued or running (#6575): nothing is decided then, and the failure is
4 * decided again when the run ends.
5 */
6
7 import { afterAll, beforeAll, describe, expect, test } from "bun:test";
8 import type { App } from "../src/app.ts";
9 import { announceRun } from "../src/actions/runs.ts";
10 import { commitToBranch } from "../src/actions/repos.ts";
11 import { openPull } from "../src/actions/pulls.ts";
12 import { openIssue, setIssueState } from "../src/actions/issues.ts";
13 import { CHECK_FAILED_LIMIT } from "../src/crew/dispatch.ts";
14 import { getIssueById } from "../src/core/issues.ts";
15 import { getPull, type Pull } from "../src/core/pulls.ts";
16 import type { Repo } from "../src/core/repos.ts";
17 import { claimRun, createRun, finishRun, getRun, type Run } from "../src/core/runs.ts";
18 import type { User } from "../src/core/users.ts";
19 import { startStandInCrew, stopStandInCrew, type StandInCrew } from "./helpers/crew-standin.ts";
20
21 describe("a failure during the builder's run", () => {
22 let standIn: StandInCrew;
23 let app: App;
24 let alice: User;
25 let bot: User;
26 let repo: Repo;
27 let n = 0;
28
29 beforeAll(async () => {
30 standIn = await startStandInCrew();
31 ({ app, alice, bot, repo } = standIn);
32 standIn.checks.request = async () => undefined;
33 app.db.run("UPDATE repo SET autonomy = 'autonomous', autonomy_by = ?1 WHERE id = ?2", [alice.id, repo.id]);
34 }, 30_000);
35 afterAll(() => stopStandInCrew(standIn));
36
37 /** A pull whose automatic tries are spent, so a failure parks it. */
38 const spentPull = async (body?: string): Promise<Pull> => {
39 const branch = `busy-${++n}`;
40 const c = await commitToBranch(app, alice, repo, {
41 message: branch,
42 branch,
43 files: [{ path: `${branch}.md`, content: "x\n" }],
44 });
45 if (!c.ok) throw new Error(JSON.stringify(c.error));
46 const o = await openPull(app, alice, repo, { title: branch, baseBranch: "main", headBranch: branch, ...(body === undefined ? {} : { body }) });
47 if (!o.ok) throw new Error(String(o.error));
48 app.db.run("UPDATE issue SET assignee_id = ?1 WHERE id = ?2", [bot.id, o.value.id]);
49 for (let i = 0; i < CHECK_FAILED_LIMIT; i++) {
50 const tried = createRun(app.db, { repoId: repo.id, kind: "agent", issueId: o.value.id, reason: "check_failed" });
51 claimRun(app.db, tried.id);
52 finishRun(app.db, tried.id, { state: "ok", summary: "reworked" });
53 }
54 return getPull(app.db, repo.id, o.value.number)!;
55 };
56 const failure = (pull: Pull, kind: "check" | "deploy"): Run => {
57 const run = createRun(app.db, {
58 repoId: repo.id,
59 kind,
60 env: `pull-${pull.number}`,
61 commitOid: pull.headOid,
62 reason: "push",
63 issueId: pull.id,
64 });
65 claimRun(app.db, run.id);
66 return finishRun(app.db, run.id, { state: "failed", summary: `${kind} failed` })!;
67 };
68 const open = (pull: Pull) => getIssueById(app.db, pull.id)?.state === "open";
69
70 const building = (issueId: string): Run => {
71 const run = createRun(app.db, { repoId: repo.id, kind: "agent", issueId, reason: "feedback" });
72 claimRun(app.db, run.id);
73 return run;
74 };
75 const ends = (run: Run): void =>
76 announceRun(app, finishRun(app.db, run.id, { state: "ok", summary: "pushed nothing" })!);
77 const push = async (pull: Pull): Promise<void> => {
78 const c = await commitToBranch(app, alice, repo, {
79 message: "more",
80 branch: pull.headBranch,
81 files: [{ path: `more-${++n}.md`, content: "y\n" }],
82 });
83 if (!c.ok) throw new Error(JSON.stringify(c.error));
84 };
85
86 test("a failed deploy during a running build leaves the pull open, and it is decided when the run ends", async () => {
87 const pull = await spentPull();
88 const run = building(pull.id);
89 announceRun(app, failure(pull, "deploy"));
90 expect(open(pull)).toBe(true);
91 ends(run);
92 expect(open(pull)).toBe(false);
93 });
94
95 test("a queued run holds it too, and the end of that run decides it", async () => {
96 const pull = await spentPull();
97 const queued = createRun(app.db, { repoId: repo.id, kind: "agent", issueId: pull.id, reason: "feedback" });
98 announceRun(app, failure(pull, "check"));
99 expect(open(pull)).toBe(true);
100 claimRun(app.db, queued.id);
101 ends(queued);
102 expect(open(pull)).toBe(false);
103 });
104
105 test("a run on the issue the pull closes holds it as well", async () => {
106 const source = openIssue(app, alice, repo, { title: "source" });
107 if (!source.ok) throw new Error(String(source.error));
108 const pull = await spentPull(`Closes #${source.value.number}`);
109 const run = building(source.value.id);
110 announceRun(app, failure(pull, "deploy"));
111 expect(open(pull)).toBe(true);
112 ends(run);
113 expect(open(pull)).toBe(false);
114 });
115
116 test("a push during the run makes the failure history: the run's end decides nothing", async () => {
117 const pull = await spentPull();
118 const run = building(pull.id);
119 announceRun(app, failure(pull, "check"));
120 await push(pull);
121 ends(run);
122 expect(open(pull)).toBe(true);
123 });
124
125 test("a pull closed during the run forgets the failure; reopened, it is not parked by it", async () => {
126 const pull = await spentPull();
127 const run = building(pull.id);
128 announceRun(app, failure(pull, "check"));
129 const issue = getIssueById(app.db, pull.id)!;
130 expect(setIssueState(app, alice, repo, issue, "closed").ok).toBe(true);
131 // Closing the pull cancels the run, which announces its end.
132 expect(getRun(app.db, run.id)?.state).not.toBe("running");
133 expect(setIssueState(app, alice, repo, getIssueById(app.db, pull.id)!, "open").ok).toBe(true);
134 ends(building(pull.id));
135 expect(open(pull)).toBe(true);
136 });
137
138 test("the same failure after the run ended closes it as before", async () => {
139 const pull = await spentPull();
140 announceRun(app, failure(pull, "deploy"));
141 expect(open(pull)).toBe(false);
142 });
143 });

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs