Summary
SubmitMany (batch review submit) aborts the entire submission the moment any single run errors. If one run in the batch was superseded to failed by a concurrent new-commit trigger, submitting the original batch strands the valid sibling reviews — permanently, with no recovery path. This contradicts the function's own documented invariant.
Where
backend/internal/service/review/review.go, SubmitMany:
// Delivery is scoped to the runs in this submission, so a missing/stuck result
// for another PR in the same trigger cannot block feedback.
func (s *Service) SubmitMany(...) (...) {
...
for _, review := range reviews {
run, err := s.submitOne(ctx, workerID, review)
if err != nil {
return nil, err // aborts the whole batch
}
runs = append(runs, run)
}
...
delivered, err := s.deliverSubmitted(ctx, workerID, runs) // never reached
submitOne returns an error for a run that is no longer running (its default/terminal case: "review run %q is not running"). A run that SupersedeStaleRunningReviewRuns moved to failed hits exactly that branch.
Why it's wrong
The doc comment guarantees a stuck/stale result for one PR cannot block feedback for the others. But because submitOne mutates the store as it goes (a running run is written to complete) and SubmitMany aborts on the first error before delivery, a single superseded run blocks delivery of every valid sibling.
Repro
- A trigger creates a 2-PR batch:
run-A (PR-A) and run-B (PR-B), both running, same batch_id.
- The worker pushes a new commit to PR-B; a fresh trigger fires
SupersedeStaleRunningReviewRuns, marking run-B failed.
- The reviewer submits both results in one
ao review submit --reviews - call.
submitOne(run-A) succeeds (→ complete); submitOne(run-B) returns "review run ... is not running"; SubmitMany returns the error → HTTP 422.
run-A is left complete-but-undelivered — the worker is never told about PR-A's requested changes. Because run-B stays failed, every reviewer retry re-produces the 422, so PR-A's feedback is stranded permanently.
Suggested fix
Distinguish "run is no longer running (superseded/terminal)" from genuine input errors, and have SubmitMany skip the superseded run and keep delivering the valid siblings, instead of aborting. Genuine input errors (bad verdict, empty body, unknown run, wrong worker, verdict conflict) should still abort.
Summary
SubmitMany(batch review submit) aborts the entire submission the moment any single run errors. If one run in the batch was superseded tofailedby a concurrent new-commit trigger, submitting the original batch strands the valid sibling reviews — permanently, with no recovery path. This contradicts the function's own documented invariant.Where
backend/internal/service/review/review.go,SubmitMany:submitOnereturns an error for a run that is no longer running (itsdefault/terminal case:"review run %q is not running"). A run thatSupersedeStaleRunningReviewRunsmoved tofailedhits exactly that branch.Why it's wrong
The doc comment guarantees a stuck/stale result for one PR cannot block feedback for the others. But because
submitOnemutates the store as it goes (a running run is written tocomplete) andSubmitManyaborts on the first error before delivery, a single superseded run blocks delivery of every valid sibling.Repro
run-A(PR-A) andrun-B(PR-B), both running, samebatch_id.SupersedeStaleRunningReviewRuns, markingrun-Bfailed.ao review submit --reviews -call.submitOne(run-A)succeeds (→complete);submitOne(run-B)returns"review run ... is not running";SubmitManyreturns the error → HTTP 422.run-Ais left complete-but-undelivered — the worker is never told about PR-A's requested changes. Becauserun-Bstaysfailed, every reviewer retry re-produces the 422, so PR-A's feedback is stranded permanently.Suggested fix
Distinguish "run is no longer running (superseded/terminal)" from genuine input errors, and have
SubmitManyskip the superseded run and keep delivering the valid siblings, instead of aborting. Genuine input errors (bad verdict, empty body, unknown run, wrong worker, verdict conflict) should still abort.