Repository navigation
✨ review: reviewer lane scales concurrency with backlog - #14695
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Open PR overlap checkBase branch: Conflicts are checked with every PR merged onto the current tip of
|
|
@clubanderson this PR is held at Failing checks:
Next: the hive does not push to your branch. Push a new head (or reply here); CI and the review swarm re-run automatically and the PR returns to the normal path. |
|
@clubanderson is auto-promoted for contributor-lane merging on this PR: the last 5 merged PRs by this author in this hive's repositories have no revert or sentinel-alert evidence. CI, reviewer verdict, diff fences, soak, holds and branch protection still apply. |
4c1e01f
ℹ️ Sentinel notice — informational; author is trusted (owner), merge is not blockedHive flagged this PR (author @clubanderson, head
Hive did not add the |
|
@clubanderson this PR is held at Why:
Next: the hive does not push to your branch. Push a new head (or reply here); CI and the review swarm re-run automatically and the PR returns to the normal path. |
Add review.reviewers.autoscale: a controller that each governor cycle reads the PRs the dispatch planner left waiting for a free reviewer or a parallel slot and raises or lowers the reviewer lane's materialised replica count between min and max. Scale-up fires after scale_up_cycles cycles behind target_backlog_per_reviewer (or a wait older than the review-pending stage dwell) and is refused by named guards: at-max, cooldown, saturated kicks (CLI did not reach input prompt), provider-budget, provider-headroom, max-parallel-reviews, and cgroup cpu/memory. Scale-down drains the top replica after scale_down_cycles idle cycles and retires it only when its turn ends. Dynamic replicas go through ExpandAgentReplicas, so reviewer-3..N share role, prompt, token, pane and cadence with reviewer-2; the entrypoint pre-creates users up to max. Decisions log as 'reviewer autoscale: 2→3 reason=backlog=7 oldest=41m', are exposed on /api/status.reviewerAutoscale, and show as an auto badge on the lane's agent cards. Settings → Review Gate → Reviewers exposes the knobs. Fixes #14685 Signed-off-by: Andy Anderson <andy@clubanderson.com>
- settings parity: cite each review.reviewers.autoscale.* path literally - style ratchet: drop the inline display style on the autoscale wrapper - timezone: format the last decision time through formatDashboardTime - api-reference.md: resync server.go citations shifted by StatusPayload - dispatch_waiting_test: enable FanOut, which gates review dispatch Signed-off-by: Andy Anderson <andy@clubanderson.com>
…ositive Signed-off-by: Andy Anderson <andy@clubanderson.com>
Signed-off-by: Andy Anderson <andy@clubanderson.com>
0ed194d to
112c27f
Compare
|
@clubanderson this PR is held at Failing checks:
Next: the hive does not push to your branch. Push a new head (or reply here); CI and the review swarm re-run automatically and the PR returns to the normal path. |
112c27f
ℹ️ Sentinel notice — informational; author is trusted (owner), merge is not blockedHive flagged this PR (author @clubanderson, head
Hive did not add the |
|
merged by Hive trusted-author auto-merge: author @clubanderson holds role owner and GitHub permission admin on repo; CI green at 112c27f. |
|
Thank you for your contribution! Your PR has been merged. We'd love to hear how your experience was: share feedback |
There was a problem hiding this comment.
Decision needed: @clubanderson should decide which of #14695 and #14702 lands for #14685 — both add review.reviewers.autoscale and both edit pkg/config/config.go, validate.go, pkg/review/dispatch.go and cmd/hive/main.go, so they conflict and cannot both merge.
Conclusion: the controller and the config/replica wiring read correct to me; one medium defect in the retire path, and this PR conflicts with #14702 (same feature, same issue, same config key, different package layout), so a maintainer has to pick one.
Conflict, not duplicate
#14702 (@Karibhou) implements review.reviewers.autoscale for the same issue. I compared both file sets: they share config.go, validate.go, review/dispatch.go, cmd/hive/main.go, hive.yaml.example, docs/review-swarm.md but diverge everywhere else (pkg/review/autoscale/ package + drain/retire + cgroup prober + dashboard settings here vs. a single pkg/review/autoscale.go + ReplicaLimits there). Different content, same intent — whichever lands second will not apply. Not recommending either closure; that is the decision above.
Finding (medium) — a drained replica that was never kicked, or whose turn-end signal was lost, is never retired
src/cmd/hive/reviewer_autoscale.go:145setsBusy = proc.State == crux.StateRunning && !proc.KickOutcome.Settled(proc.LastKick).src/crux/kick.go:72—Settledreturns false whenkickAt == nil. So a running replica that has not yet received any kick (scale-up started it, demand dropped before the planner handed it a PR; cadence kicks are withheld once draining) isBusy == trueindefinitely.src/pkg/review/autoscale/controller.go:426—retirableskips anyBusydraining reviewer, andcontroller.go:307refuses a further scale-down whilelen(c.draining) != 0.
Consequence: the lane gets stuck one above where it should be, with an idle process still holding its CPU/memory reservation, until demand returns and un-drains it. The same hold applies to a wedged turn (lost turn-end), which is exactly the case src/cmd/hive/self_upgrade_drain.go:190-201 already handles for the upgrade drain: it skips proc.LastKick == nil outright and treats an unsettled kick older than inFlightKickCeiling as wedged rather than in flight. Reusing that predicate (or agentKicksInFlight itself) for rv.Busy would close both gaps. Note the controller tests cannot see this because Busy is an input; reviewer_autoscale.go's Run has no test of its own.
What I verified and found sound
KicksUndeliverableis a consecutive counter reset on delivery/restart (src/crux/agent/manager.go:214,busy_visibility.go:281), soSaturated = KicksUndeliverable > 0means what the body says.ExpandAgentReplicasstrips and re-materialises derived agents, so re-expanding onSetReviewerAutoscaleActiveand after reload is idempotent;main.go:3961copiesReviewerAutoscaleActiveontonewCfgbefore the reload'sExpandAgentReplicasatmain.go:4057, so the live count does survive a reload as claimed.Drainingexcludes a reviewer fromreviewCapableAgents(dispatch.go:1736), so a PR left waiting while a replica drains triggers the un-drain path rather than a fresh replica — matches the design.- The settings PUT validates the band before mutating and keeps pointer-field "absent means unchanged" semantics;
entrypoint.shcaps pre-created users atmin(max, MAX_REPLICAS).
Body matches the diff; nothing claimed that is not implemented.
— hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88
Fixes #14685
Why
Live evidence 2026-10-27 ~19:20Z on hive's own pod (8 CPU / 40Gi): 23–25 open PRs, merge-skip reasons in 15 min
review-gate:no-review-bundle×41 andcontributor-lane:no-verdict×17,stage labels: PR waits on the hive past the stage dwell — reviewer lane needs attention. Onlyreviewerandreviewer-2exist; the pod had headroom. Review throughput is the chokepoint and a fixed reviewer count cannot follow it.What
review.reviewers.autoscalesizes the reviewer lane to its backlog. Off by default.Controller (
pkg/review/autoscale)DispatchPlan.Waiting: PRs with perspectives still to review on their current head that it could not dispatch because every eligible reviewer was busy (no-free-reviewer) ormax_parallel_reviewswas spent (no-parallel-slot). Drafts/holds/already-dispatched are excluded by construction. Waits are aged across cycles keyed onrepo#number@head.stage/050-review-pendingdwell. Sustainedscale_up_cycles→ +1 replica. Guards, in order, each logged by name:at-max,cooldown,saturated(≥ half of active reviewers' last kicks wereCLI did not reach input prompt— saturation, not demand),provider-budget(inference budget latch),provider-headroom(< 10 % on any provider when rotation reports),max-parallel-reviews, cgroupcpu/memory(v2cpu.max,cpu.stat,memory.max,memory.current; unknown cgroup never blocks). A refused scale-up keeps its streak.scale_down_cycles(and > min, cooldown passed) → highest replica marked draining: planner and cadence kick skip it; it is retired (tmux session ended, config entry dropped) only when its turn has ended and it has no pending review. Never killed mid-turn. Demand during drain un-drains instead of adding.pkg/config/reviewer_autoscale.go/pkg/review/autoscale/controller.go.Materialisation
The controller only sets
Config.ReviewerAutoscaleActive;ExpandAgentReplicasuses it as the lane's effective count, soreviewer-3…Nare ordinary replicas — same role, prompt, beads dir, scoped token, tmux/pluk wiring and cadence key as a configuredreviewer-2. Planner, kick governor, dashboard agent list and token cache cannot tell them apart.ReconcileAgentsadds/removes the processes. The live count survives a config reload and is never persisted (MarshalYAMLalready strips replicas).entrypoint.shpre-creates per-agent users/token caches up tomax.Observability
reviewer autoscale: 2→3 reason=backlog=7 oldest=41m0s,… 3→2 drain reviewer-3 reason=idle for 5 cycles,… retire reviewer-3,… blocked by cpu./api/status.reviewerAutoscale: enabled/agent/min/max/current/target/draining/backlog/oldestWait/streaks/lastDecision.N reviewers · autobadge (replicas· auto/· draining) with last decision in the tooltip.PUT /api/config/reviewreviewer_autoscale).Docs / changelog
docs/review-swarm.md(new section, help-mark anchor),docs/operator-reference.md,docs/agent-configuration.md,hive.yaml.example,changelog.d/added-reviewer-autoscale.md.Tests
Controller decision table (sustained backlog up, single cycle holds, at-max, cpu/memory guards, unknown resources don't block, saturated, provider budget/headroom, max-parallel, dwell-age trigger, cooldown, drain → busy never retired → retire when idle, never below min, undrain on demand, wait ageing, cgroup prober fixture), config tests (defaults, validate, agent resolution, expand/clamp/scale-down), planner
Waiting/Drainingtests, entrypoint shell tests. Notime.Sleep; injectednow.Validated locally with
go build ./...,gofmt -l,go vet, dashboard inline-JS lint, entrypoint shell tests.go testleft to CI.