[squid-dev] Timeouts for abandoned negative reviews

[Alex Rousskov]

Hello,

    Squid GitHub pull requests have the following problem: A core
developer can stall PR progress by submitting a negative review and then
ignoring the PR (despite others reminding them that the reviewer action
is required). Such stalled PRs cannot be merged because our policies
strictly prohibit merging of vetoed PRs.

This problem has affected many PRs. Collection of meaningful stats is
prohibitively expensive, but there are ~50 PRs that were open for 100+
days and many of them are not abandoned/irrelevant. Here are some of the
worst examples (the stalled day counts below are approximate):

* PR 369: stalled for 310 days (and counting)
  https://github.com/squid-cache/squid/pull/369

* PR 59: 120-480 days, depending on when you think the PR was stalled
  https://github.com/squid-cache/squid/pull/59

* PR 443: stalled for 100+ days (and counting)
  https://github.com/squid-cache/squid/pull/443

Stalled PRs may significantly increase development overheads and badly
reflect on the Squid Project as a whole. I have heard many complains
from contributors frustrated with the lack of Project responsiveness and
accountability. Stalled PRs have also been used in attempts to block
features from being added to the next numbered release.

While 100-day stalling is unheard of in other open source projects I
know about, the problem with unresponsive reviewers is not unique to
Squid. The best (applicable to the Squid Project) solution that I know
of is a timeout:

If the reviewer remains unresponsive for N days, their negative review
can be dismissed. The counting starts from a well-defined event S, and
there are at least two reminder comments addressed at the reviewer (R1
days after S and R2 days before the dismissal).

Do you think timeouts can solve the problem with stalled PRs if Project
contributors do not attempt to game the system? Can you think of a
better solution?


We can discuss specific parameter values and minor technical details
separately, but I would post the following rough suggestions for
illustrative purposes:

* N (stalling days after which the veto is dismissed): 30. N should be
small enough to prevent long stalls (keeping in mind that an
irresponsible reviewer can stall the same PR many times) but large
enough to accommodate vacations, sickness, overload, and similar
temporary conditions.

* R1 (days between S and the first "you are stalling!" reminder): 20. R1
should be small enough to avoid rushing re-review but large enough to
indicate a likely problem and avoid noise.

* R2 (days between the second reminder and vote dismissal): 5. R2 should
be small enough to reduce "Oh, I will do it later!" failures to
re-review and to clearly indicate a stalling problem (the PR was stalled
for N-R2 days already) but large enough to be sufficient for a thorough
re-review.

* S (the event that resets the timeout counter): The last time the PR is
assigned to the reviewer. GitHub already logs assignments and generates
a notification email. I do not like to abuse the "PR Assignment" feature
to pass the "who is responsible for the next step" baton, but I cannot
think of a _simpler_ way to implement this. Assignment also makes it
easy to search for PRs that await your action. The reviewer would be
responsible for assigning the PR back to the author (if there are
unaddressed change requests) or to Anubis (if a commit is required).


Thank you,

Alex.
P.S. A similar timeout approach can be applied to PRs stalled by
authors, but those PRs represent a smaller problem and their incorrect
handling results in little harm. We can consider that problem and argue
about appropriate parameters for it later.