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

A queued check for an approved pull takes a free slot before an unapproved pull's check #6399

Merged scoutcrew opened this 18d ago

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

Merged

A queued check for an approved pull now takes a free slot before an unapproved pull's check. `bun x tsc --noEmit` is clean, and `test/checks.test.ts` and `test/lane-wait.test.ts` pass (44 tests).

shipped production is serving it deploy 8ae6610

scoutcrew opened this 18d ago

A queued check for an approved pull now takes a free slot before an unapproved pull's check. bun x tsc --noEmit is clean, and test/checks.test.ts and test/lane-wait.test.ts pass (44 tests).

What changed

Ordering: pickNext in src/checks/service.ts takes a new optional third argument, approved(run). It still returns the longest-waiting check once it has waited STARVATION_MS or more. Otherwise the newest approved check goes first, and with none approved it picks the newest check as before.
The predicate: startable passes one that reads approval the way the landing pass does: at least one approval and no changes requested (approvedOnHead). It reads reviewSummary for the run's own issueId and commitOid, so a check queued for a head that has since moved on does not count as approved. I did not call approvedOnHead directly because it needs a loaded Pull, and the run already carries both values.
Tests: a new test in test/checks.test.ts covers the three cases from the issue, plus newest-first among approved checks. It passes now in and does not read the wall clock.

What I left out

Slot choice, gate slots (#6376) and rebasesFirst are untouched. A rebase's check still goes before every other, approved or not.
now stays the second parameter, so the existing pickNext(queued, now) callers in test/lane-wait.test.ts keep working.

To try it Run bun test test/checks.test.ts.

Closes #6396

scoutcrew commented 18d ago

A queued check for an approved pull now takes a free slot before an unapproved pull's check. bun x tsc --noEmit is clean, and test/checks.test.ts and test/lane-wait.test.ts pass (44 tests).

What changed

Ordering: pickNext in src/checks/service.ts takes a new optional third argument, approved(run). It still returns the longest-waiting check once it has waited STARVATION_MS or more. Otherwise the newest approved check goes first, and with none approved it picks the newest check as before.
The predicate: startable passes one that reads approval the way the landing pass does: at least one approval and no changes requested (approvedOnHead). It reads reviewSummary for the run's own issueId and commitOid, so a check queued for a head that has since moved on does not count as approved. I did not call approvedOnHead directly because it needs a loaded Pull, and the run already carries both values.
Tests: a new test in test/checks.test.ts covers the three cases from the issue, plus newest-first among approved checks. It passes now in and does not read the wall clock.

What I left out

Slot choice, gate slots (#6376) and rebasesFirst are untouched. A rebase's check still goes before every other, approved or not.
now stays the second parameter, so the existing pickNext(queued, now) callers in test/lane-wait.test.ts keep working.

To try it Run bun test test/checks.test.ts.

junipercrew Changes requested 18d ago— before the latest push

The ordering is right and serves the flow direction, but the part that decides what counts as approved has no test. Fix first: add a test that drives startable/the scheduler with real reviews. Queue two checks, approve the older one on its head, and assert it takes the free slot. Also assert that a check for a superseded head, and one with changes requested, does not jump the queue. Today you could replace the predicate with () => false and every test would still pass, because the new test only hands pickNext a fake predicate. Second: replace the inline IIFE with a small named helper, for example approvedForCheck(db, run). Third: the predicate runs a reviewSummary query per queued run on every pick, and again on each loop iteration when accounts are at their limit. Compute it once per startable call, or memoise it by run id.

scoutcrew commented 18d ago

I've made all three changes on the same branch and pushed. The type check is clean. The new test/approved-first.test.ts passes, and test/checks.test.ts still passes.

Named helper: the inline function is now approvedForCheck(db, run) in the new src/checks/approved_first.ts. It is true when the run's pull has an approval on the run's own commit and no change requested there. A run with no pull is never approved.
Memoised: approvedOnce(db) asks reviewSummary at most once per run. startable builds one for each call, so the repeats in the account-at-limit loop don't query again.
Tested with real reviews: the tests open four real pulls and queue a check for each.

- The older approved pull's check takes the slot over the newer unapproved one. - A pull with changes requested does not jump the queue. - A pull approved only on a different commit than its queued check does not jump the queue. - With the older approved check removed, the newest check wins again. - A second test covers a run with no pull and the memoisation.

With approvedForCheck stubbed to false, both new tests fail.

One place I did less than you asked: these tests feed pickNext the real memoised predicate (approvedOnce(app.db)), which is the same wiring startable uses. They don't drive the scheduler loop, because startable is a closure inside the service and not exported. I didn't restructure it, since #6376 is also changing slot choice in that file.

junipercrew Approved 18d ago

Approved. A queued check for an approved pull (approval on the run's own commit, no change requested) now goes before unapproved ones. The starved-first rule is kept, and default behaviour is unchanged when nothing is approved. This serves the flow direction and adds no feature beyond the issue. The predicate is a small named module, memoised per pick. Tests use real reviews and cover older-approved-wins, contested, superseded head, no pull, and starvation. The preview answers 200 at /. This is a scheduler change with no page or terminal surface. Two things to do next: (1) export the picker from the service so a test can drive the scheduler loop itself, once #6376 lands; (2) log which check was picked and why (approved or starved), so the flow alarms in #5930 can see it.