acp: heartbeat wakes a sleeping lazy pool - #6800
Conversation
With --lazy-pool, a heartbeat tick that landed while the pool was asleep was skipped at DEBUG level and nothing ever woke the pool for it — on a quiet agent the configured heartbeat interval silently never ran. In an eight-agent deployment, seven had not heartbeated for days before this surfaced; the only healthy one was simply the chattiest. The tick now sets heartbeat_wake_pending, the lazy-wake gate treats that flag as pending work, and the Wake(Ok) handler dispatches the deferred heartbeat once the pool is ready (unless real queued work claimed the slots — the pool is awake then, so the next tick fires normally). Observed after deploy: all seven sleeping agents logged heartbeat_wake_requested -> heartbeat_fired on their first tick.
Chessing234
left a comment
There was a problem hiding this comment.
the bug is real — a heartbeat tick that lands while a lazy pool is asleep currently logs heartbeat_skipped_pool_not_ready and is gone, so on a quiet agent the configured interval never actually runs. plumbing it into the existing lazy_wake_work_pending gate is the right seam.
what i think needs deciding before it lands is the interaction with the idle reaper, because as written the two loops fight each other.
idle_pool_sleep_bound (lib.rs:2287) tears a woken lazy pool back down after idle_pool_sleep_secs of quiet, "releasing worker subprocesses". a dispatched heartbeat is activity, so it refreshes the last_activity clock the reaper reads. so on an agent with no traffic at all and heartbeat_interval_secs > 0:
- heartbeat tick →
heartbeat_wake_pending = true→ pool wakes (subprocess spawn) - heartbeat dispatches,
last_activityrefreshes idle_pool_sleep_secslater the reaper tears the pool down- next heartbeat tick wakes it again
that's a spawn/teardown cycle per heartbeat period, forever, on an agent nobody is talking to — which is the cost lazy_pool exists to avoid. whether that's acceptable depends on the ratio: if heartbeat_interval_secs is comfortably longer than idle_pool_sleep_secs you get a periodic churn; if it's shorter the pool effectively never sleeps and lazy_pool becomes a no-op for anyone who has also set a heartbeat. config.rs:2278 uses 30s for heartbeat_interval_secs in at least one path, which is on the short side of that.
worth saying explicitly in the pr which behaviour is intended, and if it's "heartbeats win", worth having the reaper skip a teardown whose only preceding activity was a heartbeat — otherwise the two features quietly cancel out and the operator has no way to see it.
one concrete thing in the diff:
the Err arm doesn't clear heartbeat_wake_pending. the flag is only reset inside the Ok branch of the wake handler. on a failed wake it stays set, so lazy_wake_work_pending at :2411 stays true on every subsequent loop iteration and start_wake_if_due keeps being asked to retry — for a pool that may be failing for a persistent reason. the comment right above that line is about not busy-spinning "whenever the queued work drained after a failed wake", which is the same hazard; a heartbeat request that outlives its wake attempt reintroduces it, with no queued work that can ever drain to end it. clearing the flag in the Err arm (the next tick will re-request) keeps the retry cadence at the heartbeat interval instead of the loop rate.
and the drop is silent. in the Ok arm, if queue.has_flushable_work() or !pool.any_idle(), the flag is cleared and no heartbeat is dispatched — the comment says the next tick fires normally, which is true, but that's a second skipped heartbeat with no log line, where the old code at least emitted heartbeat_skipped_pool_not_ready. a heartbeat_skipped_after_wake debug there would keep the "did my heartbeat run" question answerable from logs, which is the question that produced this pr.
With --lazy-pool, a heartbeat tick that landed while the pool was asleep
was skipped at DEBUG level and nothing ever woke the pool for it — on a
quiet agent the configured heartbeat interval silently never ran. In an
eight-agent deployment, seven had not heartbeated for days before this
surfaced; the only healthy one was simply the chattiest.
The tick now sets heartbeat_wake_pending, the lazy-wake gate treats that
flag as pending work, and the Wake(Ok) handler dispatches the deferred
heartbeat once the pool is ready (unless real queued work claimed the
slots — the pool is awake then, so the next tick fires normally).
Observed after deploy: all seven sleeping agents logged
heartbeat_wake_requested -> heartbeat_fired on their first tick.