feat(buzz-acp): idle re-sleep for woken lazy pools - #5682
Conversation
A lazy harness that gets woken by an @mention eagerly spawns all `--agents` worker subprocesses and, until now, kept every one alive forever — there was no path back from `pool_ready` to the empty-slot state. With per-agent parallelism in the tens across a warm fleet, that ratchets into hundreds of standing idle workers. Add an opt-in idle re-sleep: after a configurable quiet window (`--idle-pool-sleep` / `BUZZ_ACP_IDLE_POOL_SLEEP`, 0 = off, requires `--lazy-pool`) with no dispatched turn or heartbeat in flight, no in-flight prompt tasks, an empty queue, and no wake/respawn task running, the harness tears the pool down through the normal `shutdown_agent_pool` path and returns to the exact pre-wake lazy state (empty slots, `Listening` lifecycle). The next accepted event re-wakes it through the existing lazy machinery — no second pool lifecycle. Teardown is race-safe with enqueue/wake by construction: the decision and event ingress are arms of the same single-task `select!`, and the gate requires an empty queue, so an event landing at the boundary is either dispatched that iteration or re-woken the next — a queued batch is never stranded. The sleep transition emits the existing `listening` lifecycle frame (a label Desktop already accepts), so the paired UI returns to its listening state and re-shows waking→ready on re-wake with zero Desktop enum changes. Desktop wires `BUZZ_ACP_IDLE_POOL_SLEEP` (reserved key, desktop-owned lifetime policy) to a 900s default, gated to lazy spawns, matching the harness's own per-turn idle window. Decision logic is extracted to a pure `idle_pool_sleep_due` helper (mirroring `inactivity_expired`) and covered by a full gate matrix: active-turn protection, in-flight prompt task, queued-work-at-boundary, wake/respawn-in-flight, not-ready, zero-bound, and recent-activity. Co-authored-by: Mongo <5c25403eab7271f9f94ddd4f2b270e8cac2c92e2c830c51877cca6ec974ffb3f@buzz.block.builderlab.xyz> Signed-off-by: Wes <wesbillman@users.noreply.github.com>
|
Blocking review finding, reviewing on Wes's behalf. GitHub cannot accept a formal “Request changes” review from this shared account because it is also the PR author, so this comment is the blocking record. The teardown gate does not currently require an empty queue. It passes
Please distinguish any queued/retry-held work exists from work is flushable now. Re-sleep must require the former to be false. Add a regression with a retry-throttled queued batch proving re-sleep is denied (not merely an immediately flushable batch). If sleeping with delayed retries is desired later, it needs an explicit retry-deadline wake arm; this focused PR should take the safer empty-queue rule. The pure predicate matrix cannot catch this because the bug is at the call-site mapping. The test needs to exercise the real |
The idle-pool-sleep teardown gate mapped the queue's `work_queued` signal to `has_flushable_work()`, which returns false for a batch held back only by a `retry_after` backoff throttle. A failed turn requeued with a future backoff deadline is still real queued work. Failure path: a turn fails, requeues with backoff, and completes; the idle bound then expires while the batch is not-yet-flushable, so the pool sleeps. But lazy re-wake is itself gated on `has_flushable_work()` and the maintenance timer (which re-flushes expired-retry batches) only runs while `pool_ready`, so once asleep nothing re-wakes when the backoff deadline passes — the batch is stranded until unrelated traffic arrives. Add `EventQueue::has_undispatched_work()`: like `has_flushable_work()` but without the `retry_after` filter, covering all three tables where undispatched non-in-flight work can live (queues, cancelled batches, withheld native-steer). Gate the sleep decision on it instead. The pool now never sleeps while any throttled batch is pending, so no throttled work can exist across the sleep boundary. Regression: a real failure -> requeue-with-backoff -> mark_complete cycle proves the throttled batch is undispatched (blocks sleep) while still not flushable, plus an empty/in-flight negative case. Co-authored-by: Mongo <5c25403eab7271f9f94ddd4f2b270e8cac2c92e2c830c51877cca6ec974ffb3f@buzz.block.builderlab.xyz> Signed-off-by: Wes <wesbillman@users.noreply.github.com>
|
Re-reviewed on Wes's behalf at
This is a review finding only, not an approval; Wes did not request that I approve this PR. |
The idle-pool-sleep feature pushed `runtime.rs` to 1017 lines, past the Desktop file-size ratchet's 1000-line cap (`check:file-sizes`), failing the Desktop Core lint/format CI step. Extract the warm-worker lifetime constant and its lazy-gated value selection into `agent_env::idle_pool_sleep_env(lazy)`, collapsing the 14-line inline env block at the `spawn_agent_child` call site to a single call. Behavior is identical: lazy harnesses get "900", eager harnesses get "0" (disabled); the reserved-key registration and its test are unchanged. `runtime.rs` returns to 996 lines, under the cap. Co-authored-by: Mongo <5c25403eab7271f9f94ddd4f2b270e8cac2c92e2c830c51877cca6ec974ffb3f@buzz.block.builderlab.xyz> Signed-off-by: Wes <wesbillman@users.noreply.github.com>
|
Reviewing on Wes's behalf.
Please reap completed respawn handles (including successful completions) before using the set as a busy signal, or track actual respawns-in-flight separately, and add a regression that completes a respawn then proves idle re-sleep becomes eligible. |
The idle-pool-sleep teardown gate used `!respawn_tasks.is_empty()` as a respawn busy bit, but completed respawn tasks are never joined: their payloads arrive out-of-band via `respawn_rx` and the only JoinSet op is `.shutdown()` at teardown. Tokio retains finished tasks in a JoinSet until `join_next`, so after the first slot refill or crash recovery `!respawn_tasks.is_empty()` stays true forever — `idle_pool_sleep_due` could then never pass and a woken lazy pool would never re-sleep, defeating the feature. Fix uses the authoritative signal that already exists: `any_respawn_in_flight(&crash_history)`, backed by each slot's `respawn_in_flight` flag (set at all three spawn sites, cleared when the `respawn_rx` payload is received, and guaranteed to clear even on panic via RespawnGuard). The gate call site now reads that instead of JoinSet occupancy, keeping `!wake_tasks.is_empty()` (which self-drains). Also add a non-blocking JoinSet reaper (`respawn_tasks.join_next().now_or_never()`) after the `respawn_rx` drain, so completed handles are released and the set does not grow without bound — same pattern already used for `pool.join_set`. Regression: `respawn_in_flight_signal_gates_then_clears_for_sleep` proves the busy->eligible transition through the real predicate, and `completed_respawn_tasks_are_reaped_from_the_joinset` proves the reaper drains finished tasks. Co-authored-by: Mongo <5c25403eab7271f9f94ddd4f2b270e8cac2c92e2c830c51877cca6ec974ffb3f@buzz.block.builderlab.xyz> Signed-off-by: Wes <wesbillman@users.noreply.github.com>
|
Re-reviewed on Wes's behalf at The sleep gate now uses the authoritative per-slot I also re-read the current PR comments and the accumulated diff. The retry-held queue blocker remains fixed, the Desktop extraction preserves lazy This is a review finding only, not an approval; Wes did not request that I approve this PR. |
wpfleger96
left a comment
wpfleger96
left a comment
There was a problem hiding this comment.
🤖 Combined review from two independent agent passes (Paul + Thufir), both at head 62486fd. No blockers.
Verified at source: the BUZZ_ACP_IDLE_POOL_SLEEP contract is coherent on both sides (clap env= in config.rs ↔ runtime.rs spawn env, with the reserved-key gate blocking user override and eager spawns pinned to "0"); the enqueue/teardown race-safety claim holds (an unprocessed PromptResult keeps its channel in in_flight_channels until mark_complete, so has_in_flight() blocks the sleep gate — no agent can be torn down with a result in transit); and both earlier review blockers (retry-throttled work via has_undispatched_work, respawn JoinSet reaping vs any_respawn_in_flight) are fixed with regression tests that drive real state. One pass additionally ran the full buzz-acp + desktop-tauri suites at the exact head plus a mutation probe (disabling the sleep predicate turns both all-clear assertions red, so the tests observe the feature seam).
Two non-blocking nits inline. The remaining red checks (Relay E2E, Backend Integration, Desktop E2E Integration) all fail in the activate-hermit setup step with curl: (56) Connection died — infra, and main's latest CI run is red on the same gate.
| // keeps `work_queued` true for a retry-throttled batch too, | ||
| // so a failed turn awaiting backoff is never stranded — the | ||
| // next iteration dispatches or re-wakes it. | ||
| if idle_pool_sleep_due( |
There was a problem hiding this comment.
🤖 Nit: heartbeat activity never refreshes last_activity (only dispatch_pending does), so a CLI config combining --heartbeat-interval + --lazy-pool + --idle-pool-sleep will re-sleep after the quiet window and then stop heartbeating permanently — a sleeping pool skips heartbeat ticks, and heartbeats can't wake a lazy pool (wake is gated on flushable queue work). Desktop never sets heartbeats and heartbeat+lazy was already an awkward pairing before this PR, so a doc note on --idle-pool-sleep (or a startup warn on the combination) seems enough.
| // path. Only meaningful under `lazy_pool`; the tick arm additionally gates | ||
| // on `pool_ready`, so a still-sleeping pool never re-sleeps. Reuses the | ||
| // `last_activity` clock the dispatch path already maintains. | ||
| let idle_pool_sleep_bound = if config.lazy_pool { |
There was a problem hiding this comment.
🤖 Nit: --idle-pool-sleep without --lazy-pool is silently zeroed here. The help text documents it, but a one-line tracing::warn! when idle_pool_sleep_secs > 0 && !lazy_pool would save a CLI user a confusing debugging session.
…1-merge * origin/main: perf(desktop): coalesce thread-activity localStorage writes (#5693) Batch observer-store publications per relay envelope (#5680) feat(buzz-acp): idle re-sleep for woken lazy pools (#5682) Signed-off-by: Larry <8cf5a83f590ec0955b11647d1c88f796a98e088c30a492c58e0e46c3026ae7a4@buzz.block.builderlab.xyz>
* origin/main: perf(desktop): coalesce thread-activity localStorage writes (#5693) Batch observer-store publications per relay envelope (#5680) feat(buzz-acp): idle re-sleep for woken lazy pools (#5682) Signed-off-by: Larry <8cf5a83f590ec0955b11647d1c88f796a98e088c30a492c58e0e46c3026ae7a4@buzz.block.builderlab.xyz>
2 participants
What
Adds an opt-in idle re-sleep for woken lazy ACP pools. A lazy harness woken by an @mention eagerly spawns all
--agentsworker subprocesses and, before this, kept every one alive forever — there is no path back frompool_readyto the empty-slot state. Across a warm fleet with parallelism in the tens, that ratchets into hundreds of standing idle workers (observed: 9 woken harnesses × 24 = 216 workers that never shrink).After a configurable quiet window with no dispatched turn/heartbeat in flight, no in-flight prompt tasks, an empty queue, and no wake/respawn task running, the harness tears the pool down via the normal
shutdown_agent_poolpath and returns to the exact pre-wake lazy state (empty slots,Listeninglifecycle). The next accepted event re-wakes it through the existing lazy machinery. No second pool lifecycle.Why it's safe
tokio::select!. The gate requires an empty queue, so an event landing at the boundary is either dispatched that iteration or re-woken the next — a queued batch is never stranded.listeninglifecycle frame (a label Desktop already accepts and round-trips), so the paired UI returns to its listening state and re-shows waking→ready on re-wake with zero Desktop enum changes.idle_pool_sleep_duehelper (mirrors the siblinginactivity_expired) with a full gate matrix test.Config / policy
--idle-pool-sleep/BUZZ_ACP_IDLE_POOL_SLEEP— 0 = disabled (default), requires--lazy-pool.Tests
idle_pool_sleep_duegate matrix: active-turn, in-flight prompt task, queued-work-at-boundary, wake/respawn-in-flight, not-ready, zero-bound, recent-activity, all-clear.--idle-pool-sleep), reserved-key membership.cargo test -p buzz-acp→ 761 passed, 0 failed at base63f961c7e. Desktopenv_varstests pass;cargo check --testsclean on the desktop crate.Scope
Idle re-sleep only. Parallelism defaults/caps and
start_on_app_launchpolicy are deliberately separate, separately-reviewable changes per the runtime-lane plan.