Repository navigation
✨ agents: recycle idle CLI sessions over an RSS or age budget - #14848
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 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. |
|
@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. |
eb5d405 to
2c03ba3
Compare
|
@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. |
2c03ba3 to
f06345b
Compare
A CLI session's V8 heap grows with every turn and never shrinks on idle; on the hosted hive 14–17 copilot CLI children totalled 5–6.5 GB and the pod OOM'd daily while the hive process itself sat flat at ~1 GB. The health tick (CheckAndRestartCrashedAgents) now samples each running agent's CLI process-tree RSS (tmux pane pid, /proc/*/stat ppid walk, /proc/<pid>/status VmRSS) and, when a session is over agents.session_recycle.max_rss_mb (1536) or max_age (6h) AND idle at the input prompt with no kick in flight for min_idle (2m), ends it through restartWithStormBreaker — the same graceful path a stall recovery uses — so claims and queues are preserved. 10-minute per-agent backoff after a recycle; 5-minute startup grace. The fleet block rides the existing agents:-map YAML hook and is stamped onto agents without their own override in EnabledAgents(); per-agent agents.<name>.session_recycle overrides it. The dashboard shows Session RSS, session age and recycle count per agent. Fixes #14842 Signed-off-by: Andy Anderson <andy@clubanderson.com>
… tests Signed-off-by: Andy Anderson <andy@clubanderson.com>
f06345b to
c1eadb4
Compare
There was a problem hiding this comment.
Read the diff against the v5 tree plus the callers; this looks correct to me, and it matches #14842 point for point (defaults 1536 MiB / 6h / 2m, 10-min backoff, idle-only, dashboard surfacing).
What I verified:
- Lock discipline.
CheckAndRestartCrashedAgentsdropsm.muatmanager_restart.go:179beforesampleAndRecycleSessionsruns at:225, which re-takesRLock/Lockper candidate — no self-deadlock. The RSS sample (tmux exec +/procscan) happens unlocked. - Age recycle can't loop. Every launch path sets
agent.StartedAt = &now(manager_launch.go:242,340,manager.go:1005), so after a recycle the age clock restarts and the 5-min startup grace insessionRecycleDecisionapplies. - Scope. Sandboxed and on-demand agents are
continued atmanager_restart.go:88-93before the recycle branch; the stall/busy-no-activity branches take precedence (:153-162), so a mid-turn pane is never a candidate. - Storm breaker. Recycles go through
restartWithStormBreakerand so count towardrestartStormPauseAfter = 10/h; with the 10-min backoff a single agent tops out at ~6/h, so a misconfiguredmax_rss_mbbelow the CLI baseline degrades to exponential backoff, not an auto-pause. - YAML hook. The rewritten splice loop in
pkg/config/config.gono longerbreaks aftergithub_api_hourly_capand correctly doesn't advanceiafter removing a pair; the round-trip test covers both keys.
One low-severity observation, not a blocker: session_recycle.go:280-282 bumps SessionRecycles and lastSessionRecycle before restartWithStormBreaker returns, so a recycle the breaker backs off (didRestart == false) still counts on the dashboard and still arms the 10-min backoff. Arming the backoff is probably what you want; the counter overstating is cosmetic.
go build ./... passes at c1eadb4.
— hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88
There was a problem hiding this comment.
Read the policy end to end at c1eadb4 (session_recycle.go, the restart path it calls, and the config plumbing), plus the dashboard handlers that feed Manager.UpdateConfig. The core is sound: restartWithStormBreaker → launchInTmux resets StartedAt (crux/agent/manager_launch.go:340), so an age recycle does not re-fire after the 10 min backoff; procPPID anchors on the last ); the decision is only reached from CheckAndRestartCrashedAgents for non-stalled running agents and re-checks State/Paused under the lock. Defaults match #14842 (1536 MiB / 6h / 2m).
One gap worth fixing before merge (medium): the fleet agents.session_recycle block is dropped on every dashboard config write.
pkg/config/config.go:6689-6692stamps the fleet block onto agents only insideEnabledAgents();Config.Agents[name]itself staysSessionRecycle == nil.- The dashboard syncs per-agent config into the manager with the raw map entry, not the stamped one:
pkg/dashboard/api_agent_config.go:765agentCfg = s.deps.Config.Agents[name]→:823UpdateConfig(name, agentCfg)(this runs after theReconcileAgents(EnabledAgents())at:766, so it overwrites the stamp the same handler just applied)pkg/dashboard/api_agent_control.go:197ac, ok := s.deps.Config.Agents[name]→:210/:246UpdateConfig(name, ac)- same shape at
api_agent_config.go:1021,api_agent_control.go:419
Manager.UpdateConfigassigns it verbatim (crux/agent/manager.go:1375), and the policy readsagent.Config.SessionRecycle.IsEnabled()/.Effective()where a nil receiver means enabled with defaults (crux/config/session_recycle.go:48-50,67-71).
Consequence: an operator who sets agents: { session_recycle: { enabled: false } } (or a lower max_rss_mb) and then changes any agent's model/backend/display name in the dashboard silently gets the default 1536 MiB / 6h policy back on that agent until the next full ReconcileAgents. The existing test (pkg/config/session_recycle_test.go:12) covers EnabledAgents() only, so it would not catch this.
Smallest fix I can see: have Manager.UpdateConfig (or a small Config.ResolvedAgent(name) helper used by those four call sites) apply the same nil → fleet stamp that EnabledAgents() does, with a test that round-trips enabled: false through UpdateConfig.
Everything else I checked looks correct to me; not posting nits.
— hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88
There was a problem hiding this comment.
Correctness review (read at head c1eadb4; go build of the touched packages passes, new _test.go files type-check; go test is blocked on this host so results are from reading + CI).
1. Per-agent session_recycle override does not merge with the fleet block as documented — src/docs/operator-reference.md:1160 says "absent fields inherit the fleet value", but src/pkg/config/config.go:6684 only stamps the fleet block when agent.SessionRecycle == nil. A partial override (scanner.session_recycle: {max_rss_mb: 2048}) drops every other fleet field to the hardcoded defaults in crux/config/session_recycle.go Effective(). Concrete consequence: fleet enabled: false + any per-agent field re-enables recycling for that agent (per-agent Enabled is nil → Effective() sets it true). TestSessionRecycleFleetBlockDecodesAndStampsAgents only covers nil-inherit and a full override, so this isn't caught. Suggested fix in EnabledAgents():
- if agent.SessionRecycle == nil && !c.AgentsSessionRecycle.IsZero() {
+ if !c.AgentsSessionRecycle.IsZero() {
fleet := c.AgentsSessionRecycle
- agent.SessionRecycle = &fleet
+ if agent.SessionRecycle == nil {
+ agent.SessionRecycle = &fleet
+ } else {
+ merged := *agent.SessionRecycle
+ if merged.Enabled == nil {
+ merged.Enabled = fleet.Enabled
+ }
+ if merged.MaxRSSMB == 0 {
+ merged.MaxRSSMB = fleet.MaxRSSMB
+ }
+ if merged.MaxAge == 0 {
+ merged.MaxAge = fleet.MaxAge
+ }
+ if merged.MinIdle == 0 {
+ merged.MinIdle = fleet.MinIdle
+ }
+ agent.SessionRecycle = &merged
+ }
}(plus a test case with a fleet max_age: 4h and per-agent max_rss_mb only, asserting MaxAge == 4h). Alternatively fix the doc to say the per-agent block replaces the fleet block — but the merge is the safer behavior given the enabled: false case.
2. Recycle decision uses the last stored RSS sample, not the fresh one — src/crux/agent/session_recycle.go:256-261: when procTreeRSSMB returns 0 (tmux display-message failed, pane pid unreadable) the previous value is kept for the dashboard, which is fine, but it is also fed to sessionRecycleDecision as RSSMB. Nothing resets SessionRSSMB on restart (manager_launch.go:242,340 reset StartedAt only), so after a recycle the stale over-budget value survives the 5m grace + 10m backoff, and one failed sample then triggers another recycle of a fresh, small session. Low likelihood, but it is exactly the "recycle loop" the backoff is meant to prevent. Suggested fix — decide on the fresh sample only:
RSSMB: rssMB,
(TestSampleAndRecycleSessionsPublishesRSSAndRestarts still passes: the pid-unknown case uses a busy pane.)
— hive: agent=adjudicator backend=copilot model=claude-fable-5.1 copilot=1.0.88
|
@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. |
|
merged by Hive trusted-author auto-merge: author @clubanderson holds role owner and GitHub permission admin on repo; CI green at c1eadb4. |
|
Thank you for your contribution! Your PR has been merged. We'd love to hear how your experience was: share feedback |
Fixes #14842