-
Notifications
You must be signed in to change notification settings - Fork 52k
fix(gateway): offload /model context-length resolution off the event loop #74155
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,111 @@ | ||
| """``/model`` context-length resolution must not run on the gateway event loop. | ||
|
|
||
| ``resolve_display_context_length`` runs two blocking chains — the route | ||
| comparison in ``should_clear_context_pin`` and the provider probe ladder in | ||
| ``get_model_context_length`` (blocking ``requests`` calls to Anthropic | ||
| ``/v1/models``, Copilot, Nous, Codex, GMI, Ollama, models.dev and OpenRouter). | ||
|
|
||
| The gateway message path already offloads both (``get_model_context_length_async``, | ||
| ``should_clear_context_pin_async``); the ``/model`` slash-command handlers called | ||
| the sync helper directly, freezing the loop for every user on every platform for | ||
| the duration of the probe ladder. | ||
| """ | ||
|
|
||
| import asyncio | ||
| import threading | ||
| import time | ||
|
|
||
| import pytest | ||
|
|
||
| import agent.model_metadata as model_meta_mod | ||
| from hermes_cli import model_switch | ||
|
|
||
| PROBE_SECONDS = 0.4 | ||
|
|
||
| RESOLVE_ARGS = dict( | ||
| model="claude-opus-4", | ||
| provider="anthropic", | ||
| base_url="", | ||
| api_key="", | ||
| custom_providers=None, | ||
| config_context_length=None, | ||
| ) | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def slow_probe(monkeypatch): | ||
| """Stand in for one blocking provider probe inside the resolution chain.""" | ||
| calls = {} | ||
|
|
||
| def _probe(model, **kwargs): | ||
| calls["thread"] = threading.current_thread() | ||
| time.sleep(PROBE_SECONDS) | ||
| return 128000 | ||
|
|
||
| monkeypatch.setattr(model_meta_mod, "get_model_context_length", _probe) | ||
| return calls | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_async_variant_matches_sync(slow_probe): | ||
| """The async wrapper resolves the same value as the sync helper.""" | ||
| sync_value = model_switch.resolve_display_context_length(**RESOLVE_ARGS) | ||
| async_value = await model_switch.resolve_display_context_length_async( | ||
| **RESOLVE_ARGS | ||
| ) | ||
| assert async_value == sync_value == 128000 | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_resolution_runs_off_the_event_loop_thread(slow_probe): | ||
| """The blocking chain must execute on a worker thread, not the loop thread.""" | ||
| loop_thread = threading.current_thread() | ||
| await model_switch.resolve_display_context_length_async(**RESOLVE_ARGS) | ||
| assert slow_probe["thread"] is not loop_thread | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_event_loop_stays_responsive_during_resolution(slow_probe): | ||
| """A concurrent heartbeat keeps ticking while the probe ladder runs. | ||
|
|
||
| This is the regression: with the bare sync call the loop stalled for the | ||
| full probe duration, which is what times out Discord heartbeats and stalls | ||
| Telegram polling for every other chat. | ||
| """ | ||
| lags = [] | ||
| stop = asyncio.Event() | ||
|
|
||
| async def heartbeat(): | ||
| interval = 0.02 | ||
| while not stop.is_set(): | ||
| t0 = time.monotonic() | ||
| try: | ||
| await asyncio.wait_for(stop.wait(), timeout=interval) | ||
| except asyncio.TimeoutError: | ||
| pass | ||
| lags.append(time.monotonic() - t0 - interval) | ||
|
|
||
| hb = asyncio.create_task(heartbeat()) | ||
| await asyncio.sleep(0.05) # let the heartbeat settle | ||
|
|
||
| ctx = await model_switch.resolve_display_context_length_async(**RESOLVE_ARGS) | ||
|
|
||
| stop.set() | ||
| await hb | ||
|
|
||
| assert ctx == 128000 | ||
| # The loop was never blocked for anything close to the probe duration. | ||
| assert max(lags) < PROBE_SECONDS / 2, f"event loop stalled {max(lags):.3f}s" | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_gateway_model_handlers_await_the_async_variant(): | ||
| """The ``/model`` handlers must not reach the sync helper again.""" | ||
| import inspect | ||
|
|
||
| from gateway import slash_commands | ||
|
|
||
| source = inspect.getsource(slash_commands) | ||
| assert "resolve_display_context_length_async(" in source | ||
| # No bare sync call: every occurrence carries the _async suffix. | ||
| assert "resolve_display_context_length(" not in source | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inspect.getsource()is a source-shape assertion, whichAGENTS.mdprohibits. Replace this with a behavioral test that drives the actual handler paths and proves a deliberately blocking resolver runs off the event-loop thread.