feat(gateway): session model pool — concurrency-aware auto-assignment with auxiliary slot tracking - #37519
Conversation
… with auxiliary slot tracking Add SessionModelPool module that automatically assigns models to new sessions from a configured pool, respecting per-model concurrency limits and reserving slots for auxiliary tasks (vision, compression, curator, etc.). New config section: session_model_pool - enabled (bool, default false) - strategy: round-robin | least-loaded | priority - inactive_timeout: seconds before idle slot release - pool[]: list of model entries with max_concurrent, reserved_for_auxiliary, context_length, priority Gateway integration (run.py): - _resolve_session_agent_runtime: pool assigns model when no manual /model override exists, stores in _pool_assigned_models dict - _handle_message_with_agent: releases pool slot on session reset - _handle_model_command: releases pool slot on manual /model override Auxiliary integration (auxiliary_client.py): - call_llm: acquires auxiliary slot before API call, releases in finally block Tests: 35 unit tests covering pool entry logic, all 3 strategies, session/auxiliary slot management, thread safety, and edge cases. Closes NousResearch#37511
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
- Remove read_raw_config() from auxiliary_client hot path (H4/R2) Singleton caches config; disk I/O on every call was wasteful. - Verify auxiliary slot release in finally block (H1/R2) Original code already had correct finally block; subagents reviewed committed diff (pre-fix) and flagged false positive. - _pool_assigned_models_lock protects all 5 access sites (H3/R1) - threading.Condition replaces polling for aux slots (H5/R1) - Per-session timestamps in _evict_inactive_sessions (B2/R1) - Stoic quotes fully removed from run.py (B1/R1) - Singleton restart documented in module docstring (H2/R1)
- B4: Gate auxiliary LLM call when acquire_auxiliary_slot returns False (previously the call proceeded unconditionally, defeating throttling) - H1/H6: Propagate pool-assigned provider to runtime_kwargs so the session uses the correct provider endpoint, not just the model name - H3: Integrate mark_manual_override in /model handler so the pool won't reassign manually-overridden sessions on the next turn
This comment was marked as resolved.
This comment was marked as resolved.
- H5: Extract _release_pool_slot() helper, replace 3 duplicated release blocks - M1: Warn on duplicate pool_key in from_config() - M4: Replace silent except Exception:pass with logger.debug in all pool paths - Fix inactive_timeout validation to accept floats (not just int) - Fix misleading test comment (round-robin, not priority) - Add 9 new tests: eviction (4), duplicate pool_key (2), aux slot blocking (3) 44/44 tests passing.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
… type)
- Finding 1: Extract _get_session_model_pool() helper in auxiliary_client.py
with module-level cache, replacing 2 inline lazy imports
- Finding 2: Change aux blocked return from {"error": ...} dict to None —
callers expect OpenAI response object, not a plain dict
- Finding 5: Fix test session_slots type from List[str] to Dict[str, float]
in 3 TestPoolModelEntry tests (was masked by len() working on both)
44/44 tests passing.
This comment was marked as resolved.
This comment was marked as resolved.
Critical fixes (Claude + Qwen consensus):
- Sentinel (_UNSET) in _get_session_model_pool() prevents permanent None
caching when pool is not yet initialized on first auxiliary call
- Always call acquire_session_slot() instead of local cache check —
refreshes session timestamps (prevents premature eviction) and
eliminates TOCTOU race condition in _resolve_session_agent_runtime
Medium fixes (all 3 reviewers):
- _release_pool_slot passes {} to singleton instead of _load_gateway_config()
(avoids disk I/O on every session reset / model override)
- Priority clamped to 1-10 range in from_config() validation
Minor (Grok):
- Finally block reuses _pool ref instead of second _get_session_model_pool()
- Docstring corrected: '4 places' → '3 places'
New test: priority clamping (0→1, 99→10)
45/45 tests passing.
This comment was marked as resolved.
This comment was marked as resolved.
Critical fix: - C2 (MinMax): Both paths of _on_model_selected now call mark_manual_override. Path 1 was missing the call, causing the pool to silently reassign models after manual /model override. Logging improvements (Kimi + MinMax consensus): - mark_manual_override: except Exception:pass → logger.debug (both paths) - Finally block in call_llm: except Exception:pass → logger.error for resource leak detection if aux slot release fails - acquire_session_slot: logger.info → logger.debug to prevent log flood in high-traffic gateways Cleanup: - Removed reconstruct_from_active_sessions() — no callers, param unused - Removed corresponding test - Added TODO on async_call_llm documenting known limitation 44/44 tests passing.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
DRY: Extract _mark_pool_override(session_key) to eliminate duplicate mark_manual_override blocks in _on_model_selected (both paths now call _release_pool_slot + _mark_pool_override). Tests: Add priority tie-break by least-loaded and least-loaded tie-break by priority tests to cover the tie-breaking logic in _pick_candidate. 46/46 tests passing.
This comment was marked as resolved.
This comment was marked as resolved.
…de not cleared CRITICAL fixes: - IndentationError in _mark_pool_override and _release_pool_slot: method bodies were at class level (4sp) instead of method level (8sp). This prevented the gateway module from importing entirely. - Sentinel leak in _get_session_model_pool(): except block now sets _aux_pool_cache = None instead of leaving it as _UNSET (truthy), which would cause AttributeError on .enabled access. MEDIUM fix: - Session reset (/new) now clears manual_override in the pool so the pool can reassign a model on the next turn. Previously, /model followed by /new left the session permanently marked as overridden. 46/46 tests passing. gateway/run.py parses cleanly.
This comment was marked as resolved.
This comment was marked as resolved.
Warning previously said 'Only the last entry will be used' but the code retains both entries and acquire uses the first match. Updated to accurately reflect the actual behavior. 46/46 tests passing.
This comment was marked as resolved.
This comment was marked as resolved.
…view)
H2+H3: Deduplicate pool_key in from_config() — keeps last entry, discards
earlier duplicates with warning. Prevents auxiliary_count going negative.
M1: Added _entries_by_key dict for O(1) pool_key lookups. Converted 6
lookup sites (acquire/release session, acquire/release auxiliary,
_aux_count, _reserved_count) from O(n) linear scan to dict.get().
M4: _mark_pool_override and clear_manual_override now use {} (singleton
ignore) instead of _load_gateway_config(), consistent with _release_pool_slot.
M3: Documented return None in call_llm docstring.
H1: Created issue NousResearch#37744 for async_call_llm integration, linked in TODO.
46/46 tests passing.
This comment has been minimized.
This comment has been minimized.
rafaumeu
left a comment
There was a problem hiding this comment.
Code Review — PR #37519: Session Model Pool
Veredito: APPROVE com 1 HIGH + 4 MEDIUM + 3 LOW
HIGH
H1 — async_call_llm sem integracao com pool
gateway/auxiliary_client.py tem TODO referenciando issue #37744, mas esse issue nao existe. O path async bypassa todo o pool (round-robin, aux count, override, health check). Se alguem chamar async antes do pool ser integrado, vai direto num provider aleatorio.
- Acao: Criar o issue #37744 e linkar no TODO, ou remover a referencia.
MEDIUM
M1 — Config e runtime state na mesma dataclass
PoolModelEntry mistura campos de config (provider_config, pool_key) com runtime state (_failure_count, _last_failure). Qualquer teste que instancia PoolModelEntry precisa mockar ambos os mundos. Separar em PoolModelConfig (frozen) + runtime state facilita testes e evita mutabilidade acidental.
- Acao: Considerar
PoolModelConfigfrozen dataclass + runtime wrapper.
M2 — call_llm retorna None quando slot auxiliar bloqueado
Quando _max_auxiliary_count e atingido, call_llm retorna None. Todos os callers esperam Response. Isso pode causar AttributeError downstream se o caller nao verificar.
- Acao: Documentar o contrato (return type
Response | None) ou levantar exception dedicada.
M3 — Inconsistencia de config loading
_mark_pool_override le config do disco (self._load_config()), enquanto _resolve_session_agent_runtime usa um singleton {} como fallback. Dois caminhos diferentes pro mesmo dado.
- Acao: Unificar em um unico config loader.
M4 — _manual_overrides: set sem type hint generico
Deveria ser set[str] para consistencia com o resto do codebase tipado.
- Acao:
_manual_overrides: set[str] = set().
LOW
L1 — time.sleep(0.5) em tests
Pode causar flakiness em CI lento. Considerar mockar ou usar timeout maior.
- Acao: Avaliar se
time.sleeppode ser substituido porasyncio.sleep+ mock.
L2 — Round-robin e semanticamente LRU
O nome round_robin confunde porque a implementacao e mais proxima de LRU (reordena no acesso). Isso pode enganar quem le o codigo.
- Acao: Renomear ou documentar a semantica.
L3 — _aux_pool_cache nunca resetado
O cache de pool auxiliar cresce indefinidamente. Em sessoes longas, pode consumir memoria.
- Acao: Adicionar TTL ou limite de tamanho.
Positivos
- Thread safety robusta (locks bem colocados, sentinel pattern elegante)
- Config validation completa com erros claros
- 46 testes solidos cobrindo casos de borda
- Integration points bem isolados (pool nao vaza pro gateway)
- Commit history transparente e incremental
Review por @rafaumeu via Hermes Agent
ricardo-camilo-programador-frontend-web
left a comment
There was a problem hiding this comment.
All issues from previous CHANGES_REQUESTED reviews have been addressed in subsequent commits. All 9 inline comments resolved.
…, async gap - _get_session_model_pool(): only cache non-None results; retry on None (disabled) and import failures instead of permanently caching - call_llm(): raise RuntimeError instead of returning None when pool blocks auxiliary call — prevents AttributeError in callers (vision, session_search, curator) - async_call_llm(): add SessionModelPool auxiliary slot tracking (acquire before call, release in finally), removing the TODO gap where async calls bypassed pool throttling entirely
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the detailed pool implementation and the follow-up fixes. The feature premise is still present on current main, but this needs rework before it can be safely salvaged.
Problems
agent/auxiliary_client.py:571returns the_UNSETsentinel when no pool is configured. The sync and async callers then access_pool.enabledoutside the guarded acquisition block (agent/auxiliary_client.py:5017and5495on the PR head). Sincesession_model_pool.enableddefaults to false, this breaks normal auxiliary calls rather than becoming a no-op.- The PR resolver predates current routing precedence. Current main applies
channel_overridesingateway/run.py:3811-3844, whereas the PR applies its pool provider atgateway/run.py:2658. Salvage must not let a pool overwrite an explicit channel provider or bypass persisted/modelrestoration.
Suggested changes
- Return
Nonerather than_UNSETfrom the auxiliary helper when no enabled pool is available; add sync and async disabled-pool regressions. - Integrate allocation into current main's resolver after preserving
/modeland channel-override precedence, with focused gateway routing tests.
Automated hermes-sweeper review.
| except Exception: | ||
| # Import failure is NOT cached — next call retries. | ||
| pass | ||
| return _aux_pool_cache |
There was a problem hiding this comment.
When no pool is configured, _aux_pool_cache remains _UNSET, so this returns a truthy sentinel rather than the documented None. The callers then evaluate _pool.enabled outside their acquisition try blocks (lines 5017 and 5495), breaking auxiliary calls under the default-disabled configuration. Return None here when the cache is still _UNSET, and add disabled-pool sync/async coverage.
| # pool wants to assign a different model. Pool assignments are | ||
| # weaker than manual /model overrides and are released when the | ||
| # session ends or when a manual override takes effect. | ||
| if not override and resolved_session_key: |
There was a problem hiding this comment.
Current main resolves channel_overrides after the global runtime and documents /model → channel_overrides → global precedence at gateway/run.py:3750-3844. When salvaging this allocation, apply it only below both explicit override layers; otherwise the later pool provider write can pair a channel-selected model with the pool provider.
|
Complementary approach to #62467. This PR handles session-level model+pool assignment, while #62467 handles the credential resolution layer underneath. What this PR does: auto-assigns models to sessions with concurrency-aware pool slot tracking. This is orthogonal to credential resolution — it decides WHICH pool entry a session uses, not HOW that entry's base_url is resolved. How they stack:
Both are needed. This PR's session-level assignment would consume the No conflict — these two PRs are complementary layers. |
Problem or Use Case
Users running multiple concurrent sessions (Discord threads, Telegram topics, WhatsApp chats, etc.) must manually run
/modeland/yoloin every new session. Without intentional distribution, multiple sessions hit the same model, exhausting provider rate limits.Additionally, auxiliary tasks (vision, compression, curator, session_search, approval, etc.) make independent API calls that are invisible to session management — meaning even a perfect session distribution can cause rate-limit collisions when an auxiliary call saturates a model already used by active sessions.
Concrete scenario: With the Z.AI Coding Plan, 7+ GLM models are available with separate concurrency pools totaling 28+ slots. Today only 1 session uses capacity before collisions start, and auxiliary calls further erode available slots unpredictably.
Proposed Solution
Add a
session_model_poolsection toconfig.yamlthat:New files
gateway/session_model_pool.py— Thread-safeSessionModelPoolclass withPoolModelEntrydataclass, 3 allocation strategies, session + auxiliary slot tracking, inactive timeout, and module-level singleton (get_session_model_pool())tests/gateway/test_session_model_pool.py— 35 unit tests covering pool entry logic, all 3 strategies, session/auxiliary slot management, thread safety, edge casesModified files
gateway/run.py— 3 integration points:_resolve_session_agent_runtime: pool assigns model when no manual/modeloverride exists (stores in_pool_assigned_modelsdict)_handle_message_with_agent: releases pool slot on session reset_handle_model_command: releases pool slot when user makes manual/modeloverride (override always prevails)agent/auxiliary_client.py—call_llm()acquires auxiliary slot before API call, releases infinallyblockhermes_cli/config.py—session_model_poolsection added toDEFAULT_CONFIGProposed config schema
Field definitions
max_concurrent(required) — Hard limit on simultaneous uses (sessions + auxiliary)reserved_for_auxiliary(required) — Guaranteed minimum slots for auxiliary tool calls; sessions can only usemax_concurrent - reserved_for_auxiliaryslotscontext_length(optional) — If set, gateway may warn on context overflowpriority(optional, 1–10, default 5) — Higher = preferred when multiple models have free slotsBehavior
session_model_pool.disabled→ current behavior (globalmodel.default), no overhead/modeloverride always prevails — pool slot is released and session is marked as "manually overridden"model.defaultTest results
Closes #37511