Public: anyone can read this; the crew can change it.
A new head cancels its pull's running check, not only the queued one #1821
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/1793-a-new-head-cancels-its-pull-s-running-ch → main 1 commit fork 1 thread · 1 unresolved
2 files
changed +57
−2
src/checks/service.ts +10 −2
| @@ -225,17 +225,22 @@ export function startChecks(app: App, options: ChecksOptions): ChecksHandle { | ||
| 225 | 225 | // is still waiting, and the same commit is not checked twice. Looked for |
| 226 | 226 | // and written in one write transaction, so a second forge process over |
| 227 | 227 | // this database (a roll) cannot queue the same check beside it. |
| 228 | const superseded: Run[] = []; | |
| 228 | 229 | const { run, fresh } = transact(app.db, () => { |
| 229 | 230 | for (const active of listActive(app.db, "check")) { |
| 230 | 231 | if (active.repoId !== repo.id || active.env !== env) continue; |
| 231 | 232 | if (active.commitOid === commitOid) |
| 232 | 233 | return { run: active, fresh: false }; |
| 233 | if (active.state === "queued") | |
| 234 | cancelRun( | |
| 234 | // A pull's running check is for a head nobody will land either: | |
| 235 | // it is stopped too, below, once the row says so. | |
| 236 | if (active.state === "queued" || issueId !== undefined) { | |
| 237 | const gone = cancelRun( | |
| 235 | 238 | app.db, |
| 236 | 239 | active.id, |
| 237 | 240 | `superseded by ${commitOid.slice(0, 7)}`, |
| 238 | 241 | ); |
| 242 | if (gone && active.state === "running") superseded.push(gone); | |
| 243 | } | |
| 239 | 244 | } |
| 240 | 245 | const created = createRun(app.db, { |
| 241 | 246 | repoId: repo.id, |
| @@ -248,6 +253,9 @@ export function startChecks(app: App, options: ChecksOptions): ChecksHandle { | ||
| 248 | 253 | }); |
| 249 | 254 | return { run: created, fresh: true }; |
| 250 | 255 | }); |
| 256 | // The row is cancelled, so the check's own end finishes nothing and | |
| 257 | // announces nothing; the abort is what frees its slot and its checkout. | |
| 258 | for (const old of superseded) inFlight.get(old.id)?.abort.abort(); | |
| 251 | 259 | if (!fresh) return run; |
| 252 | 260 | announceRun(app, run, actorId); |
| 253 | 261 | schedule(); |
test/checks.test.ts +47
| @@ -537,6 +537,53 @@ describe("checks", () => { | ||
| 537 | 537 | expect(middle?.summary).toContain("superseded by"); |
| 538 | 538 | }, 60_000); |
| 539 | 539 | |
| 540 | test("a new head on a pull stops its running check for the old head, superseded not failed", async () => { | |
| 541 | const slow = await commitToBranch(app, alice, repo, { | |
| 542 | branch: "supersede", | |
| 543 | message: "A very slow check", | |
| 544 | files: [ | |
| 545 | { path: "forge.json", content: checkJson("sleep 30") }, | |
| 546 | { path: "S.md", content: "s\n" }, | |
| 547 | ], | |
| 548 | }); | |
| 549 | if (!slow.ok) throw new Error(JSON.stringify(slow.error)); | |
| 550 | const opened = await openPull(app, alice, repo, { | |
| 551 | title: "supersede", | |
| 552 | baseBranch: "main", | |
| 553 | headBranch: "supersede", | |
| 554 | }); | |
| 555 | if (!opened.ok) throw new Error(JSON.stringify(opened.error)); | |
| 556 | const env = `pull-${opened.value.number}`; | |
| 557 | const old = await checks.request( | |
| 558 | repo, | |
| 559 | env, | |
| 560 | slow.value.commitOid, | |
| 561 | "push", | |
| 562 | opened.value.id, | |
| 563 | ); | |
| 564 | if (!old) throw new Error("no run"); | |
| 565 | const began = Date.now(); | |
| 566 | while (getRun(app.db, old.id)?.state !== "running") { | |
| 567 | if (Date.now() - began > 20_000) throw new Error("never started"); | |
| 568 | await Bun.sleep(50); | |
| 569 | } | |
| 570 | const next = await commitToBranch(app, alice, repo, { | |
| 571 | branch: "supersede", | |
| 572 | message: "Fast", | |
| 573 | files: [ | |
| 574 | { path: "forge.json", content: checkJson("test -f S.md") }, | |
| 575 | ], | |
| 576 | }); | |
| 577 | if (!next.ok) throw new Error(JSON.stringify(next.error)); | |
| 578 | const fresh = await settled((r) => r.commitOid === next.value.commitOid); | |
| 579 | expect(fresh.state).toBe("ok"); | |
| 580 | const stopped = getRun(app.db, old.id); | |
| 581 | expect(stopped?.state).toBe("cancelled"); | |
| 582 | expect(stopped?.summary).toContain("superseded by"); | |
| 583 | // Well short of the 30 s the old check would have taken. | |
| 584 | expect(Date.now() - began).toBeLessThan(25_000); | |
| 585 | }, 60_000); | |
| 586 | ||
| 540 | 587 | for (const ending of ["merge", "close"] as const) |
| 541 | 588 | test(`${ending === "merge" ? "merging" : "closing"} a pull leaves no queued or running run for it`, async () => { |
| 542 | 589 | const blocker = await commitToBranch(app, alice, repo, { |
new above
offline
The abort fires after the transaction and the row is already cancelled, but nothing announces the cancelled run. Call announceRun(app, old) next to the abort, as pull.merged/closed does, so a web viewer sees it stop.