Skip to content

fix: count fresh eager-spawned sessions against the live-population cap - #8835

Merged
iamwhatever merged 1 commit into
kirodotdev:mainfrom
javenciu:fix/eager-spawn-live-population-cap
Sep 6, 2026
Merged

fix: count fresh eager-spawned sessions against the live-population cap#8835
iamwhatever merged 1 commit into
kirodotdev:mainfrom
javenciu:fix/eager-spawn-live-population-cap

Conversation

@javenciu

@javenciu javenciu commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Problem / Motivation

session.eager_spawn (on by default) speculatively creates a slot's session
ahead of its first message, from five trigger sites: slot create, agent reset,
relaunch, project set (chat_handlers.py), and slot focus (ws.py, the only
resume path).

The eager-spawn machinery bounds two resource dimensions but not the third:

  • Concurrent spawns are capped (_EAGER_SPAWN_MAX_CONCURRENT = 2) — its own
    comment says it bounds the burst, i.e. in-flight handshakes only.
  • Speculatively RESUMED sessions are population-capped
    (_RESUME_PREFETCH_MAX_LIVE = 3, evict-oldest via the _armed_prefetches
    registry) and TTL'd (600s).
  • FRESH eager sessions are neither: the tail of _eager_spawn registers
    population accounting only inside if allow_resume and resumed:, so a
    fresh session is reaped by nothing but the 30-minute idle sweep.

A user who creates or reconfigures K slots within that window accumulates K
live-but-unclaimed agent processes. The concurrency semaphore does not help:
sequential slot signals pass it trivially. And each unclaimed session is not
one process — each session spawns its own full set of M configured MCP
servers (#3259 measured 6), so K idle tabs cost roughly K x (1 + M)
processes doing nothing.

Why it matters

Baseline memory pressure is a live user complaint (#5033: 5+ GB around one
chat on a 16 GB machine; #3259: CPU/RAM saturation from per-session MCP
process fan-out). Unclaimed speculative sessions add to exactly that baseline
without the user ever sending a message. The repo already decided a bounded
unclaimed population is the right shape — this change closes the gap between
that decision and the fresh-spawn path.

What changed (motivation → approach → change)

Observed symptom: create several chat tabs (or re-point a few tabs' agent or
project) and idle — one full kiro-cli process per tab stays alive for up to
30 minutes, none of them claimed by any turn. The registry that exists to
bound exactly this population stays empty (reproducer output below).

Root cause: _cap_armed_prefetches is wired only into the resumed-prefetch
arm of _eager_spawn's tail, so fresh speculative sessions never enter the
live-population accounting.

Change: register EVERY successful speculative registration in the existing
registry, keeping the TTL resume-only (fresh sessions hold no native
per-session lock, so the idle sweep remains their backstop — the cap just
bounds how many can pile up):

if allow_resume and resumed:
    _schedule_prefetch_ttl(state, slot, session_key)
await _cap_armed_prefetches(sessions, session_key)

Eviction stays conditional (remove_if_unclaimed): a claimed session is
never touched and lazily falls out of the accounting — semantics already
proven by the shipped resume-prefetch cap and its tests. A lost same-key
race (is_new=False) still never registers; that session belongs to a real
creator and must not occupy unclaimed accounting.

Alternatives considered and rejected:

  • A separate cap/registry for fresh sessions: duplicates machinery and makes
    two constants drift independently; the existing registry's semantics
    (insertion-ordered, evict-oldest-unclaimed, conditional remove) already fit.
  • Extending the TTL to fresh sessions: a behavior change beyond the defect —
    fresh sessions hold no prior transcript's lock, which is what motivated the
    TTL; the idle sweep already reaps them.
  • Renaming _RESUME_PREFETCH_MAX_LIVE: the name is now slightly narrow, but
    it is pinned by existing tests in two files; the comment block documents
    the widened scope instead, keeping the diff minimal.

Tests

New TestFreshSpawnPopulationCap in test/test_eager_spawn.py (3 tests),
plus a module-level autouse fixture isolating the module-global registry
around every test so registrations from one test can never trigger a
spurious over-cap eviction in another.

Fails before (at origin/main, fix reverted, tests kept):

FAILED test/test_eager_spawn.py::TestFreshSpawnPopulationCap::test_fresh_spawn_registers_in_live_population - AssertionError: assert 'dashboard:t1' in {}
FAILED test/test_eager_spawn.py::TestFreshSpawnPopulationCap::test_fresh_spawns_beyond_cap_evict_the_oldest_unclaimed - AssertionError: Expected remove_if_unclaimed to have been awaited once. Awa...
=================== 2 failed, 1 passed, 33 warnings in 3.38s ===================

Passes after:

======================== 3 passed, 33 warnings in 3.05s ========================

Full file and neighbours:

test/test_eager_spawn.py ................................ 64 passed
test/test_chat_runner_coverage.py -k "prefetch or eager or armed or cap" 26 passed

Local gates: mypy clean on the changed source file, flake8 clean, isort
clean, scripts/check_black_formatting.py passed, scripts/docs-lint.sh
passed.

Manual verification

Not run against a live gateway; the mechanism is fully exercised by the
suite's existing mocked-session conventions (same approach as the shipped
resume-prefetch cap tests). The eviction path reuses remove_if_unclaimed,
whose live semantics are unchanged by this PR.

Related Issues

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 secrets, credentials, or internal references in the diff

Pattern harvest

The pattern is a speculative-resource population where only in-flight concurrency is
bounded, not the accumulated live set. Harvested across the eager-spawn seam: all five
spawn triggers (slot create, agent reset, relaunch, project set, slot focus) funnel
through _eager_spawn, so the fix lands once at the registration tail rather than
per-trigger. The resumed path already had population accounting
(_RESUME_PREFETCH_MAX_LIVE + _armed_prefetches); the fresh path was the one
uncovered instance of the class at this seam. One adjacent candidate of the same
class exists on the warm-cache path (chat_done-driven refreshes are not
population-capped); left out deliberately per one-topic-per-PR — happy to file it
separately if maintainers agree it is worth bounding.

Rule candidate: any speculative-resource spawn path must register in a bounded live-set
population cap (evict-oldest-unclaimed), not just an in-flight concurrency limit — audit
every trigger that funnels into the spawn seam, not only the one that surfaced the leak.

The armed-prefetch registry and its evict-oldest population cap
(_RESUME_PREFETCH_MAX_LIVE) only registered speculatively RESUMED
sessions, so fresh eager spawns - slot create, agent reset, relaunch,
project set - accumulated one live-but-unclaimed agent process per
slot, each with its own MCP servers, bounded only by the 30-minute
idle sweep. The spawn semaphore does not help: it gates concurrent
handshakes, not accumulated live processes, and sequential slot
signals pass it trivially.

Register every successful speculative registration in the existing
registry so arming beyond the cap evicts the oldest unclaimed session
via the conditional remove_if_unclaimed. The TTL stays resume-only:
fresh sessions hold no native per-session lock. A lost same-key race
(is_new=False) still never registers - that session belongs to a real
creator.
@javenciu
javenciu requested a review from a team as a code owner September 5, 2026 22:46
@javenciu
javenciu requested a review from pepmach September 5, 2026 22:46
@github-actions github-actions Bot added fork Pull request from a fork (external contributor) readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 5, 2026
@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review (fork) — ✅ no blocking findings

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

Review details

No findings.
[GPT-REVIEWED] ef303b8

@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5, fork) — ✅ PASS

Design-level review of ef303b82b3ed8f1d831dad79d2dedab9fb154e88 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

Wires fresh spawns into the already-proven registry instead of duplicating machinery; conditional evict keeps claimed sessions safe, and the invariant (unclaimed ≤ cap) holds.

I verified in the base chat_runner.py that unclaimed speculative sessions can only leave _armed_prefetches via eviction (torn down) or the resume TTL, so after this change the live-unclaimed population is genuinely bounded at _RESUME_PREFETCH_MAX_LIVE — stale claimed/swept entries can dilute the registry but never let an unclaimed session survive over the cap, since every unclaimed one is inside it. Keeping the TTL resume-only is the right cut (fresh sessions hold no native lock), the evict-oldest shape correctly favors the newest user intent, the spec is updated in the same commit, and the fail-before test evidence closes the loop.

[DESIGN-REVIEWED] ef303b8

@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

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

Premise-level review of ef303b82b3ed8f1d831dad79d2dedab9fb154e88 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 evidence verified against the base tree. Every speculative session flows through the single speculative=True call site inside _eager_spawn (chat_runner.py:4078), so the tail registration covers all five trigger sites with no sibling paths left unfixed, and the base code confirms registration was resume-arm-only.

First-Principles-Verdict: PASS

Closes the one un-wired arm of an already-shipped population cap by reusing the existing registry: no new surface, no new constants, cause-level.

What this change ships

Intent: stop idle-but-unclaimed pre-warmed chat sessions from stacking one agent process (plus its MCP servers) per tab. This is a FIX.

  1. Creating/reconfiguring several tabs no longer accumulates one idle agent process each; unclaimed speculative sessions cap at 3 — justified
  2. Fresh and resumed speculative sessions now share that one cap; either kind can evict the other — declared
  3. Fresh sessions still get no TTL, only the cap; idle sweep stays their reaper — declared non-change
  4. ws.py spec paragraph updated to document the widened cap — justified (AGENTS.md same-commit invariant)
  5. Comment blocks restated for the widened scope — declared
  6. Three new tests plus a module-wide registry-isolation fixture — declared

Verified mechanics: base chat_runner.py:4135-4137 registers into _armed_prefetches only under if allow_resume and resumed:, exactly as described; the fix sits at the cause (the registration gap), not a symptom guard. Duplicate check: grepped _cap_armed_prefetches|_armed_prefetches — one registry, one eviction helper, and the change reuses both instead of adding a second cap. Sibling check: grepped schedule_eager_spawn|speculative=True — all five trigger sites funnel into the one patched tail; zero unfixed siblings. New public surface: zero (no key, flag, or symbol added). The rejected alternatives (separate registry, TTL extension, constant rename) are each the larger shape; what shipped is the smallest honest version.

[FIRST-PRINCIPLES-REVIEWED] ef303b8

@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review (fork) — ✅ no blocking findings

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

Review details

No findings.

[OPUS-REVIEWED] ef303b8

@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: passed Eligible automated validation passed for the current revision readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running readiness: passed Eligible automated validation passed for the current revision labels Sep 5, 2026
@iamwhatever
iamwhatever enabled auto-merge (squash) September 6, 2026 01:03

@iamwhatever iamwhatever left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tier 1 auto-approve: fix (3 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: fresh eager-spawned sessions were never registered in the live-population cap registry, so sequential slot signals (create, agent/project set) stacked one unclaimed agent process per slot until the idle sweep; the fix lifts the _cap_armed_prefetches call out of the resume-only branch so fresh and resumed sessions are capped alike. Spec files changed as a ride-along (a minority of the diff on both file count and changed lines), not reviewed as a design decision: docs/system-specs/modules/learn-cron-dashboard.md. 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.

@iamwhatever
iamwhatever merged commit 5ac38ef into kirodotdev:main Sep 6, 2026
89 of 91 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fork Pull request from a fork (external contributor)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants