Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .env.example
Original file line number Diff line number Diff line change
Expand Up @@ -318,6 +318,11 @@ ANALYZER_ASSIGNMENT_SUGGESTIONS_MAX_SNAPSHOT_AGE_SECONDS=86400
# Acceptance window in days (a proposal expires this long after creation unless accepted);
# the per-reviewer notification_settings override is clamped to >= 7.
ANALYZER_ASSIGNMENT_PROPOSAL_WINDOW_DAYS=7
# Rolling window (days) the per-reviewer new-assignment rate limit is measured over
# (ReviewerPreference.max_new_assignments_per_week, design doc 054). This defines what "per week"
# means for every reviewer's number, so changing it re-interprets all of them. No enable flag:
# the limit is unset (unlimited) by default, so opting in is the switch.
ANALYZER_ASSIGNMENT_RATE_WINDOW_DAYS=7
# On-queue-exit policy for a pending proposal: invalidate (default) | retain.
ANALYZER_ASSIGNMENT_PROPOSAL_ON_QUEUE_EXIT=invalidate
# Propose task schedule (daily; default 00:45 UTC, superseding the legacy apply slot).
Expand Down
795 changes: 795 additions & 0 deletions docs/design-decisions/054-assignment-rate-limit.md

Large diffs are not rendered by default.

18 changes: 15 additions & 3 deletions qb_site/analyzer/AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,27 @@
review-load (weighted, matching the assignment engine's capacity gate, **incl. pending assignment proposals**
per design doc 050) as of the latest cached queue snapshot, plus `format_load_line`. Single authority shared by
the `assigned-prs` command, the daily reviewer-attention digest, and the reviewer console; read-only (never
builds a snapshot), returns `{}`/`None` when no snapshot exists.
builds a snapshot), returns `{}`/`None` when no snapshot exists. `ReviewerLoad` also carries the *flow* gate
(`weekly_count` / `weekly_limit` / `at_weekly_limit`, design doc 054), read off the same `ReviewerProfile` the
engine gate uses, and `format_load_line` appends `· last 7 days: 4 / 5` for a reviewer who has opted into a
limit (nothing at all for one who has not).
- `assignment_rate_limit.py` — `recent_assignment_counts(repository, logins, *, window_days, now)`: distinct
**newly assigned** PRs per reviewer in the trailing `ANALYZER_ASSIGNMENT_RATE_WINDOW_DAYS`, from the
`ReviewerAssignmentApplication` history (design doc 054). The *flow* half of reviewer capacity, next to
`maximum_capacity`'s *stock*. One grouped query, `lower()` on both sides — the history column stores login
casing verbatim, so a case-sensitive count silently reads **zero** for a capitalized reviewer and their limit
never fires. Consumed via `build_reviewer_catalog`, which puts the count and the reviewer's
`max_new_assignments_per_week` on every profile so the engine gate and the load line cannot disagree.
Also exports `assignment_rate_window_days()` — read the window through it, never hardcode 7.
- `assignment_suggestions.py` — `suggest_prs_for_reviewer(repository, login, *, labels, limit)`:
on-demand "what should I review?" (design doc 053). Single authority for which open PRs a
reviewer could take right now and why not the rest; the Zulip `suggest-prs` command and the
console suggestions page both render its output and never re-derive eligibility. Shares the
nightly builder's candidate pool via `reviewer_assignment.prepare_assignment_inputs` (new pool
exclusions belong there, not at call sites), overrides only the requester's push throttles
(`away_until`, `auto_assign`, `maximum_capacity`) via a profile substitution — the engine is
unmodified and correctness rules (authorship, conflicts, opt-outs, cooldowns) stay in force —
(`away_until`, `auto_assign`, `maximum_capacity`, and the 054 rate limit) via a profile
substitution — the engine is unmodified and correctness rules (authorship, conflicts, opt-outs,
cooldowns) stay in force —
and reads only the trace's `available`/`potential` membership, never the random `picked`, so
results are deterministic per snapshot. Read-only: never builds a snapshot, persists nothing.
Refuses a snapshot older than `ANALYZER_ASSIGNMENT_SUGGESTIONS_MAX_SNAPSHOT_AGE_SECONDS`
Expand Down
112 changes: 112 additions & 0 deletions qb_site/analyzer/services/assignment_rate_limit.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
"""Rolling-window intake counts — the *flow* half of reviewer capacity (design doc 054).

``ReviewerPreference.maximum_capacity`` bounds the **stock** a reviewer holds at once; it does not
bound **flow**. A reviewer who clears PRs quickly frees the slot and the next nightly run refills
them, so the concurrent cap only ever bit reviewers who *didn't* act (measured: reviewers with
``maximum_capacity=10`` taking 22–30 new PRs in 30 days). ``max_new_assignments_per_week`` bounds
the flow instead, and this module is the single place that answers the question it needs:

> how many **distinct PRs** has this reviewer been newly assigned in this repository within the
> trailing ``ANALYZER_ASSIGNMENT_RATE_WINDOW_DAYS``?

Two consumers share it so they cannot disagree: ``build_reviewer_catalog`` (which puts the count on
every ``ReviewerProfile``, where the engine gate reads it) and, through the same profiles, the
reviewer-facing load line. A reviewer whose push went quiet sees the same number that silenced it.

Counting notes, each load-bearing:

- **Source is** ``analyzer.ReviewerAssignmentApplication`` **(design doc 046)** — an append-only,
indefinitely-retained row per *system-mediated* assignment: the nightly auto direct-assign,
confirm-mode accepts, and console pull-claims all route through ``assign_reviewer_and_record``.
A raw Zulip ``assign`` self-assign writes no row and so does not count (design doc 053's known
audit asymmetry, inherited rather than introduced here — see 054 Subtlety 4).
- **Distinct PRs, not rows.** A PR that is auto-unassigned by the attention sweep and later
re-assigned writes several ``applied`` rows; the limit counts "new PRs", so it counts once. On
production this currently saves nothing (zero re-assignment churn in 67 days) — it is insurance
against the sweep producing a repeat, where row-counting would then be wrong.
- **Case-insensitive on both sides.** ``ReviewerAssignmentApplication.reviewer_login`` is stored
**verbatim**, not normalized: the nightly paths pass engine logins while the console accept and
the 053 claim pass ``User.github_login``, which keeps GitHub's original casing (``core_user``
enforces only case-*insensitive* uniqueness). Measured, 11 of 41 production reviewers are stored
capitalized, and this failure mode is not a partial undercount but a **zero** — a case-sensitive
filter would silently exempt a quarter of the population from limits they had opted into. The
``Lower()`` on the column and the normalization of the requested logins are both required.
- **Only** ``status='applied'`` **counts**, and ``applied_at`` is the clock. Proposed-but-unaccepted
work is bounded by the concurrent cap instead (054 Subtlety 5).
"""

from __future__ import annotations

from datetime import datetime, timedelta
from typing import Sequence

from django.conf import settings
from django.db.models import Count
from django.db.models.functions import Lower

from analyzer.models import ReviewerAssignmentApplication
from core.models import Repository


def normalize_login(login: str | None) -> str:
"""Lowercase/strip a login into the key space this module counts in."""
return (login or "").strip().lower()


def assignment_rate_window_days() -> int:
"""The configured rolling window, in days (``ANALYZER_ASSIGNMENT_RATE_WINDOW_DAYS``).

Read through one helper so every call site — catalog build, surfacing, UI copy — measures and
*describes* the same period. The setting defines what "per week" means for every reviewer's
stored number, so it is deliberately global rather than per-reviewer.
"""
return int(settings.ANALYZER_ASSIGNMENT_RATE_WINDOW_DAYS)


def recent_assignment_counts(
repository: Repository,
logins: Sequence[str],
*,
window_days: int,
now: datetime,
) -> dict[str, int]:
"""Distinct newly-assigned PRs per reviewer in the trailing window, keyed by normalized login.

Counts ``applied`` ``ReviewerAssignmentApplication`` rows for ``repository`` whose
``applied_at`` is at or after ``now - window_days``, deduplicated by ``pr_number`` per reviewer.
Every requested login appears in the result — a reviewer with no intake maps to ``0`` — so
callers can index without a default and never confuse "no rows" with "not asked about".

``window_days <= 0`` disables the count (all zeros) rather than degenerating into "everything
since the epoch", which would silently block every limited reviewer.
"""
normalized = sorted({key for key in (normalize_login(login) for login in logins) if key})
counts: dict[str, int] = {login: 0 for login in normalized}
if not normalized or int(window_days) <= 0:
return counts

since = now - timedelta(days=int(window_days))
rows = (
ReviewerAssignmentApplication.objects.filter(
repository=repository,
status=ReviewerAssignmentApplication.STATUS_APPLIED,
applied_at__gte=since,
)
.annotate(login_lower=Lower("reviewer_login"))
.filter(login_lower__in=normalized)
# The model has a Meta.ordering; without clearing it Django folds those columns into the
# GROUP BY and the aggregate below fragments into one row per (login, run_date, pr, id).
.order_by()
.values("login_lower")
.annotate(pr_count=Count("pr_number", distinct=True))
)
for row in rows:
counts[str(row["login_lower"])] = int(row["pr_count"])
return counts


__all__ = [
"assignment_rate_window_days",
"normalize_login",
"recent_assignment_counts",
]
31 changes: 22 additions & 9 deletions qb_site/analyzer/services/assignment_suggestions.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,12 @@
random ``picked`` — and the ranking's sort key is a total order, so identical requests against
one snapshot generation return identical ordered results (and a smaller ``limit`` returns a
strict prefix of a larger one).
4. Push-throttle preferences (``away_until``, ``auto_assign``, ``maximum_capacity``) are
overridden by the explicit request; correctness rules (authorship, conflict-of-interest,
opt-outs, cooldowns, assignment-forbidden labels, active assignees/proposals) never are.
4. Push-throttle preferences (``away_until``, ``auto_assign``, ``maximum_capacity``, and the
rolling-window ``max_new_assignments_per_week`` of design doc 054) are overridden by the
explicit request; correctness rules (authorship, conflict-of-interest, opt-outs, cooldowns,
assignment-forbidden labels, active assignees/proposals) never are. The rate limit in
particular *depends* on this: it is a deliberately un-saveable weekly trickle, and this pull
path is where a reviewer with spare capacity right now catches up.
5. One candidate pool: the assignable set comes from ``prepare_assignment_inputs``, shared with
the nightly builder, so a suggestion can never offer what the scheduled run would refuse.
7. Capacity is reported, never enforced: ``load`` comes from the reviewer's *real*
Expand Down Expand Up @@ -61,8 +64,9 @@
STATUS_NONE_ELIGIBLE = "none_eligible"

# Skip-tally reasons, in the engine's own evaluation order (each pool PR is counted once against
# the first rule that excluded the requester). Deliberately no `at_capacity`: the requester's
# capacity is always overridden, so it can never be the reason *they* were skipped (Invariant 7).
# the first rule that excluded the requester). Deliberately no `at_capacity` and, for the same
# reason, no `at_rate_limit`: both of the requester's capacity gates are always overridden, so
# neither can ever be why *they* were skipped (Invariant 7).
SKIP_ALREADY_ASSIGNED = "already_assigned"
SKIP_NO_TOPIC_LABEL = "no_topic_label"
SKIP_AUTHORED = "authored"
Expand Down Expand Up @@ -215,8 +219,9 @@ def suggest_prs_for_reviewer(
Read-only (Invariant 1): reads the cached queue snapshot for the repo's default rule set —
never builds one — and persists nothing. ``labels`` *replaces* the reviewer's stored
``preferred_labels`` for this request; the request also overrides ``away_until``,
``auto_assign`` and ``maximum_capacity`` (push throttles, Invariant 4), while authorship,
conflict-of-interest, opt-outs, cooldowns and the pool filters stay in force.
``auto_assign``, ``maximum_capacity`` and ``max_new_assignments_per_week`` (push throttles,
Invariant 4), while authorship, conflict-of-interest, opt-outs, cooldowns and the pool filters
stay in force.
"""
current_time = now or datetime.now(timezone.utc)
effective_limit = int(settings.ANALYZER_ASSIGNMENT_SUGGESTIONS_LIMIT) if limit is None else int(limit)
Expand Down Expand Up @@ -268,8 +273,16 @@ def _result(**overrides) -> SuggestionResult:

# The request profile: an explicit request overrides the push throttles (Invariant 4). The
# capacity override is unconditional (Invariant 7) — the load line below carries the honest
# capacity signal instead.
override_kwargs: dict = {"auto_assign": True, "temporary_break": False, "maximum_capacity": sys.maxsize}
# capacity signal instead. `weekly_limit=None` retires the 054 rate limit for this request on
# the same footing: a reviewer *asking* for work is not a statement about how much the
# scheduled pipeline should send them, and this is the catch-up path a weekly window
# deliberately gives up on the push side.
override_kwargs: dict = {
"auto_assign": True,
"temporary_break": False,
"maximum_capacity": sys.maxsize,
"weekly_limit": None,
}
if label_override:
override_kwargs["preferred_labels"] = list(known_labels)
override_kwargs["preferred_labels_lower"] = set(known_labels)
Expand Down
25 changes: 23 additions & 2 deletions qb_site/analyzer/services/reviewer_assignment.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
ReviewerAssignmentSnapshot,
ReviewerOptOut,
)
from analyzer.services.assignment_rate_limit import assignment_rate_window_days, recent_assignment_counts
from analyzer.services.queue_rules import default_rule_set_for_repo, rules_for_rule_set
from analyzer.services.queueboard_snapshot import QueueboardSnapshotBuilder
from analyzer.services.reviewer_assignment_engine import (
Expand Down Expand Up @@ -237,17 +238,32 @@ class AssignmentStatistics:


def build_reviewer_catalog(repository: Repository, *, now: datetime | None = None) -> list[ReviewerProfile]:
"""Hydrate reviewer profiles from ReviewerPreference rows."""
"""Hydrate reviewer profiles from ReviewerPreference rows.

Carries both capacity gates: the concurrent ``maximum_capacity`` (stock) and the rolling-window
``max_new_assignments_per_week`` with its trailing intake count (flow, design doc 054). The
count is fetched here, in one grouped query for the whole catalog, so that *every* consumer of a
catalog — the nightly builder, the diagnostic trace, on-demand suggestions, and the reviewer-
facing load line — sees the same figure. A reviewer whose push goes quiet then reads the number
that silenced it, and the gate and the surfacing cannot drift apart.
"""
current_time = now or datetime.now(timezone.utc)
prefs = list(ReviewerPreference.objects.filter(repository=repository).select_related("user").order_by("user__github_login"))
recent_counts = recent_assignment_counts(
repository,
[getattr(pref.user, "github_login", "") or "" for pref in prefs],
window_days=assignment_rate_window_days(),
now=current_time,
)
profiles: list[ReviewerProfile] = []
prefs = ReviewerPreference.objects.filter(repository=repository).select_related("user").order_by("user__github_login")
for pref in prefs:
login = getattr(pref.user, "github_login", None)
if not login:
continue
temporary_break = bool(pref.away_until and pref.away_until > current_time)
preferred_labels = list(pref.preferred_labels or [])
conflicts = list(pref.conflict_of_interest or [])
weekly_limit = pref.max_new_assignments_per_week
profile = ReviewerProfile(
github_login=login,
maximum_capacity=pref.maximum_capacity,
Expand All @@ -258,6 +274,11 @@ def build_reviewer_catalog(repository: Repository, *, now: datetime | None = Non
free_form=pref.free_form or "",
conflict_of_interest=conflicts,
conflict_of_interest_lower={c.lower() for c in conflicts},
weekly_limit=None if weekly_limit is None else int(weekly_limit),
# Keyed by normalized login: the history column stores whatever casing the writing
# caller used, so an unnormalized lookup here would read 0 for every reviewer whose
# GitHub login is capitalized and quietly disable their limit (design doc 054).
recent_assignment_count=recent_counts.get(_normalize_login(login), 0),
)
profiles.append(profile)
return profiles
Expand Down
Loading