vinzenz/hyperhive
vinzenz/hyperhive
Watch
0
0
Fork
You've already forked hyperhive
 0
Code Activity

review classifier: a request for a moved head reviews the PR's current head

Browse source 
A request whose head the PR has moved on from was dropped without a word. It now runs on the current head, as before the queue. The re-post shortcut, keyed on the request's head, applies only while the PR is still at that head, so a moved PR never re-posts an old head's verdict.
This commit is contained in:
atlas 2026-10-22 07:15:14 +02:00
commit 5ebec6d09b
2 changed files with 51 additions and 33 deletions
Download patch file Download diff file Expand all files Collapse all files

9
docs/swarm/review-classifier.md
View file

@ -130,9 +130,9 @@ minute, and the controller that's running then reviews it. A restart
therefore repeats a run that was going. When it can't queue a request, the therefore repeats a run that was going. When it can't queue a request, the
account posts a `**No verdict:**` comment saying so. account posts a `**No verdict:**` comment saying so.
When the controller takes a request, it reads the PR first. It drops a When the controller takes a request, it reads the PR first. A request whose
request whose head is no longer the PR's head, and posts and records nothing head is no longer the PR's head runs on the PR's current head, and never
for it. re-posts the old head's recorded verdict.
A request for a PR whose review is still going waits in the queue until that A request for a PR whose review is still going waits in the queue until that
review ends, so a PR never has two runs at once. A request that re-posts an review ends, so a PR never has two runs at once. A request that re-posts an
earlier verdict waits for a run place too, and frees it before it posts. earlier verdict waits for a run place too, and frees it before it posts.
@ -587,8 +587,7 @@ classifier's reply arrives, a requested review once the controller has posted
its review or the post has failed, and a request the controller couldn't queue its review or the post has failed, and a request the controller couldn't queue
once its comment is on the forge or the post has failed. The controller records a once its comment is on the forge or the post has failed. The controller records a
re-posted verdict the same way, with the outcome and answer of the run it re-posted verdict the same way, with the outcome and answer of the run it
repeats and that run's id in its `detail`. A dropped request, whose head the repeats and that run's id in its `detail`. A run still going isn't in the list; a
PR has moved on from, isn't recorded. A run still going isn't in the list; a
controller restart stops it, and the queue hands its request back. controller restart stops it, and the queue hands its request back.
The newest 100 runs are one JSON array under the key `recent` in the The newest 100 runs are one JSON array under the key `recent` in the

75
swarm-controller/src/review_classifier/review.rs
View file

@ -641,10 +641,11 @@ async fn in_progress(
/// Review `req`, holding `slot`, and record the result in `runs`. /// Review `req`, holding `slot`, and record the result in `runs`.
/// ///
/// A request whose head is no longer the PR's is dropped: nothing is posted /// A request whose head is still the PR's, and whose head's newest review
/// or recorded. One whose head's newest review [`reposted`] covers frees /// [`reposted`] covers, frees `slot` and posts that verdict again. Any other
/// `slot` and posts that verdict again. Any other runs, reading the settings /// runs on the PR's current head, reading the settings for the model and
/// for the model and effort. /// effort: a request for a head the PR has moved on from gets a review of the
/// new head, never the old head's verdict.
async fn review( async fn review(
files: &impl FileSource, files: &impl FileSource,
pulls: &impl PullSource, pulls: &impl PullSource,
@ -655,18 +656,16 @@ async fn review(
slot: Slot, slot: Slot,
) { ) {
let pull = read_pull(pulls, req).await; let pull = read_pull(pulls, req).await;
if let Ok(pull) = &pull let moved = matches!(&pull, Ok(pull) if !req.head.is_empty() && pull.head_sha != req.head);
&& !req.head.is_empty() if moved {
&& pull.head_sha != req.head
{
tracing::info!( tracing::info!(
repo = %format!("{}/{}", req.owner, req.name), pr = req.pr, repo = %format!("{}/{}", req.owner, req.name), pr = req.pr,
head_sha = req.head.as_str(), current = pull.head_sha.as_str(), head_sha = req.head.as_str(),
"review requested on a head the PR has moved on from; dropping the request" "review requested on a head the PR has moved on from; reviewing its current head"
); );
return;
} }
if let Some(earlier) = newest_review(runs, req).await if !moved
&& let Some(earlier) = newest_review(runs, req).await
&& let Some(ruling) = reposted(&earlier) && let Some(ruling) = reposted(&earlier)
{ {
drop(slot); drop(slot);
@ -1989,27 +1988,47 @@ mod tests {
} }
#[tokio::test] #[tokio::test]
async fn a_request_for_a_head_the_pr_has_moved_on_from_is_dropped() { async fn a_request_for_a_head_the_pr_has_moved_on_from_reviews_the_new_head() {
let q = Queued::new().await; let q = Queued::new().await;
// `old` has an approval on record; the PR has since moved to `new`.
let approved = reviewed(&q.rig.store, "earlier", RunOutcome::Approved).await;
q.rig
.store
.record(RunRecord {
id: "earlier-old".to_owned(),
head_sha: Some("old".to_owned()),
..approved
})
.await;
q.rig.forge.move_to(7, "new"); q.rig.forge.move_to(7, "new");
q.request(7, "old").await; q.request(7, "old").await;
let _consumer = q.consume(&q.rig.reviews); let _consumer = q.consume(&q.rig.reviews);
tokio::time::timeout(Duration::from_secs(10), async { until("the run reached the classifier", || q.rig.gate.reached() == 1).await;
while q.waiting().await > 0 {
tokio::time::sleep(Duration::from_millis(20)).await;
}
})
.await
.expect("the request is acked");
assert!(q.rig.posted().is_empty());
assert!(q.rig.doc.runs().is_empty(), "nothing is recorded");
assert_eq!(q.rig.gate.reached(), 0);
// Control: the PR's head runs.
q.request(7, "new").await;
q.rig.gate.open(1); q.rig.gate.open(1);
until("the head is reviewed", || q.rig.posted().len() == 1).await; until("the new head is reviewed", || q.rig.posted().len() == 1).await;
assert_eq!(q.rig.posted()[0].1.as_deref(), Some("new")); let posted = q.rig.posted();
assert_eq!(posted[0].1.as_deref(), Some("new"));
assert!(
!posted[0].2.body.starts_with("Already reviewed"),
"the old head's verdict was posted again: {}",
posted[0].2.body
);
// Control: with the PR back on `old`, the same request re-posts its
// recorded verdict without a run.
q.rig.forge.move_to(7, "old");
accept(
Some(&q.client),
&*q.rig.poster,
&q.rig.store,
&requested_at("old"),
)
.await;
until("the verdict is posted again", || q.rig.posted().len() == 2).await;
let posted = q.rig.posted();
assert_eq!(posted[1].1.as_deref(), Some("old"));
assert!(posted[1].2.body.starts_with("Already reviewed `old`"));
assert_eq!(q.rig.gate.reached(), 1, "the re-post ran nothing");
} }
#[tokio::test] #[tokio::test]