fix(session): pool health sweep heals an under-filled warm pool

[javenciu]

fix(session): pool health sweep heals an under-filled warm pool

Problem / Motivation

_pool_health_loop (src/kiro_crew/session_pool.py) documents its job as
"Periodically discard dead/expired providers and refill the pool." The refill
half of that contract only executed when the sweep itself removed an in-queue
entry, so population lost outside the sweep's own accounting was never
healed:

• An empty pool was an absorbing state. _sweep_warm_pool_once
  early-returned at if not qsize: return before any accounting ran, so a
  pool at zero was never swept and never refilled. A pool can reach zero with
  no compensating replenish signal: the fill loop breaks on a spawn failure
  (provider binary briefly unavailable, auth hiccup), and the claim path's
  exception arm hard-kills a claimed provider and re-raises before the
  success-path _schedule_replenish() is reached. Once at zero, every
  subsequent session cold-starts — the warm-pool feature silently turns off
  until gateway restart. The miss_empty decision path records the miss but
  never schedules a refill.

• A short-but-healthy pool never replenished from the sweep. The sweep
  computed removed = qsize - len(healthy) and only scheduled a replenish
  inside if removed:. A deficit created by out-of-queue kills is invisible
  to that arithmetic (removed == 0), so an idle pool sat under target
  indefinitely — the component whose docstring owns the healing job could not
  perform it.

Under ongoing claim traffic a deficit heals as a side effect of the next
successful claim (_schedule_replenish() on the success path, and
_fill_warm_pool loops while qsize < pool_size, healing the full deficit).
The genuinely stuck cases are exactly the ones the health loop exists for:
a deterministic claim-path exception draining the pool to empty (no claim
ever succeeds again to fire the success-path heal), an idle pool with no
further claims, and pool_size=1 where a single exception empties the pool
immediately.

Why it matters

The warm pool exists to hide provider spawn latency. When it silently decays
to empty, the degradation is invisible — nothing errors, sessions just get
slower — and it is sticky until a restart. A health loop that cannot heal the
empty state inverts its purpose: the worst decay state is the one state it
refuses to touch.

What changed (motivation → approach → change)

Replace removal-triggered refill with target-triggered refill in
_sweep_warm_pool_once:

• Drop the if not qsize: return fast path (the if not self._pool_size: return guard stays — a disabled pool remains a no-op; the sweep-loop body
  is skipped naturally when the queue is empty).
• After the sweep, compute the deficit against the target
  (self._pool_size - len(healthy)) and call _schedule_replenish()
  whenever the pool is under target — covering removed > 0, pre-existing
  shortfall with zero removals, and the empty pool.
• Logging: the removal info line is unchanged; a shortfall with no removals
  gets a debug line; the all-healthy-at-target case keeps its debug line.

_fill_warm_pool is already idempotent, lock-guarded, and target-capped
(while ... qsize < pool_size), so redundant replenish signals are safe by
construction; the fix adds a trigger, not a new fill mechanism. The deny
direction is preserved: pool_size=0 schedules nothing.

Tests

All in test/test_session_pool.py (TestPoolHealthLoop):

Test	Shape	Before fix	After
test_empty_pool_schedules_replenish	pool_size set, queue empty → sweep	FAILS (early return, replenish never called)	passes
test_under_target_pool_schedules_replenish	pool_size=3, 1 healthy in queue → sweep	FAILS (removed == 0 arm skipped replenish)	passes
test_at_target_pool_does_not_replenish	pool_size=2, 2 healthy → sweep	passes (regression pin: no deficit → no churn)	passes
test_disabled_pool_sweep_is_noop	pool_size=0 → sweep	passes (regression pin: disabled pool stays no-op)	passes
test_keeps_healthy_provider (updated)	healthy provider at target survives sweep	passes	passes (now also pins no-replenish at target)

The dead-provider removal path (removed > 0 → replenish) keeps its existing
coverage in the suite. Full file: 82 passed, 0 failed.

Manual verification

Reproduced the absorbing state deterministically via the failing tests
against the unchanged tree: with the fix stashed, the empty-pool and
under-target sweeps complete without ever calling _schedule_replenish
(2 failed, 3 passed); with the fix restored, the full module suite is green
(82 passed). Gates run locally: targeted + neighboring pytest (effort/pool
suites, 310 passed), mypy on the changed source file (clean), flake8,
isort --check-only, the repo black gate (scripts/check_black_formatting.py,
passes — the test file is baseline-listed, no unrelated reformat churn
included), and scripts/docs_lint.py (all checks passed).

Related Issues

No open issue tracks this decay; filing as a direct fix. Related population
accounting: #8835 capped unbounded pool growth on the eager-spawn path —
this PR is the complementary direction, healing unreplenished shrinkage.
(#2264 asks for per-agent standby counts — a feature on a different seam,
not covered or affected here.)

Pattern harvest

The pattern is a maintenance loop that triggers its remediation off its own
bookkeeping instead of the invariant it owns. The sweep measured "how much
did I remove this pass" when the docstring's promise is "the pool is at
target" — so every decay path that bypassed the loop's ledger (fill-time
spawn failure, claim-path exception kill) produced drift the loop could not
see, and the deepest decay state (empty) was excluded by a fast path before
measurement even began. Harvested across this seam: the claim path already
heals opportunistically on success, which masks the loop's gap under
traffic and makes the defect present only in idle/poisoned/size-1 regimes —
the regimes where a background loop is the only actor left.

Rule candidate: a self-heal loop must compute its trigger from the owned
invariant (target minus observed state), never solely from its own
removal/mutation count — and fast-path exits must be proven not to skip the
states the loop exists to repair (an early return on "nothing to sweep" must
not also mean "nothing to refill").

Checklist

• At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
• Existing tests pass and new tests added for new functionality
• Self-review completed; code follows project style guidelines
• Documentation updated (if applicable — no doc file documents the sweep's refill trigger; the corrected behavior now matches the existing _pool_health_loop docstring)
• No secrets, credentials, or internal references in the diff

Contribution License Agreement

Per the template placeholder (CLA text pending): offered under the same terms as my prior merged contributions to this repository (#8835).

Why no screenshot: no visual delta — backend-only change to the session
pool's health sweep; no UI surface is touched.

[github-actions]

Design Review (Fable 5, fork) — ✅ PASS

Design-level review of 8cb795073f341d0edb43cc47b502672ec2df97e5 via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: PASS

Root-cause fix: the sweep now triggers off the invariant it owns (target deficit) instead of its own removal count; redundant refills are safe by the fill loop's existing lock and target cap.

Verified against the base tree: the if not qsize: return early exit (session_pool.py:498) and the removal-gated replenish (session_pool.py:561) match the described absorbing-state defect; _fill_warm_pool re-checks qsize < pool_size under _pool_fill_lock, so the new trigger cannot over-fill or stack fills. pool_size=0 stays a no-op via both the sweep guard and _schedule_replenish. Persistent spawn failure degrades to one bounded attempt per sweep interval with a warning — a reasonable failure story. Tests pin both new arms and both no-churn regressions; no spec documents the refill trigger, so the doc-in-same-commit rule is satisfied.

[DESIGN-REVIEWED] 8cb7950

[github-actions]

First Principles Review (Fable 5, fork) — ✅ PASS

Premise-level review of 8cb795073f341d0edb43cc47b502672ec2df97e5 via the fork AI-review pipeline — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All mechanism claims in the description verify against the base tree: the empty-queue early return at session_pool.py:498, the removal-only replenish trigger at session_pool.py:561-567, the fill loop's break on spawn failure at session_pool.py:277, the claim path's exception arm that hard-kills and re-raises before the success-path replenish (session_allocation.py:1384-1387), and the miss_empty path that records but never refills (session_allocation.py:1268). The change adds no new public surface, config key, or state — it re-derives the existing trigger from the target the loop's docstring already owns. Final review:

First-Principles-Verdict: PASS

A health loop that couldn't heal its worst state now triggers off the invariant it owns; every item is the fix or a pin on it.

What this change ships

Intent: make the warm-pool health sweep actually restore the pool to target, including from empty — a FIX.

1. An empty warm pool now refills on the next sweep instead of staying dead until restart — justified (early return at base session_pool.py:498 verified).
2. A short-but-healthy pool now refills on sweep, not only after a removal — justified (removal-only trigger at base session_pool.py:561-567 verified).
3. New debug line when refilling a shortfall with no removals — rides along; earns its place as the only trace of the new path.
4. Disabled pool (pool_size=0) stays a no-op, now pinned by test — justified regression pin.
5. Three tests added, one renamed to assert healing instead of skipping — justified.

No new config, flag, or public symbol; consumer-count and duplicate-mechanism lenses have nothing to bite. The fix sits at cause level: the trigger is now computed from the owned invariant (target − observed), not the sweep's own removal ledger, which is exactly the decision gap that produced all three decay paths.

Subtractions

• Shrink if removed or deficit > 0 to if deficit > 0 (session_pool.py): when removed > 0 with deficit <= 0 the pool is at/above target and _fill_warm_pool already no-ops (while ... qsize < pool_size, session_pool.py:257) — the removed term is a redundant trigger kept only for symmetry with the old code.

[FIRST-PRINCIPLES-REVIEWED] 8cb7950

[github-actions]

GPT 5.6 Review (fork) — ✅ no blocking findings

Reviewed 8cb795073f341d0edb43cc47b502672ec2df97e5 via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 8cb7950

[github-actions]

Opus 4.8 Review (fork) — ✅ no blocking findings

Reviewed 8cb795073f341d0edb43cc47b502672ec2df97e5 via the fork AI-review pipeline; updated in place on each push.

Review details

I've independently verified the change. The removed early return (if not qsize: return) now lets an empty pool fall through to deficit-based replenish scheduling; range(0) is a no-op so no crash, and _schedule_replenish/_fill_warm_pool are lock-guarded and idempotent (while qsize < pool_size), so redundant replenish tasks cannot over-fill. The if not self._pool_size: return guard still keeps a disabled pool a no-op. Deficit arithmetic (pool_size - len(healthy)) and the schedule condition are correct in the empty, under-target, at-target, and removed cases. No reachable crash, data loss, or boundary crossing on the changed lines.

No findings.

[OPUS-REVIEWED] 8cb7950

[iamwhatever]

Tier 1 auto-approve: fix (2 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean, security checklist all-NO, AI reviewers green. Category: the warm-pool health sweep returned early whenever the queue was empty, so an under-filled pool never scheduled a replenish; the sweep now computes a deficit against pool_size and schedules a refill on either removal or deficit, with pool_size=0 still a no-op. CodeQL is not applicable on this fork PR (default-setup emits no check-run); SAST coverage is Semgrep only, latest run success with 0 annotations.