FIX: Keep the backend responsive while starting scenario runs - #2522
Open
varunj-msft wants to merge 1 commit into
Open
FIX: Keep the backend responsive while starting scenario runs#2522varunj-msft wants to merge 1 commit into
varunj-msft wants to merge 1 commit into
Conversation
| # Initialization loads the default datasets, which takes minutes, and is mostly | ||
| # synchronous work. Run it on a worker thread so the event loop stays free to | ||
| # answer health checks and status polls while a run is starting. | ||
| scenario = await asyncio.to_thread(self._prepare_run_blocking, request=request) |
Contributor
There was a problem hiding this comment.
we need to wait until the work thread actually finishes; right now if when cancel to_thread the working is not actually cancelled so we free capacity too early.
| Returns: | ||
| Scenario: The initialized scenario. | ||
| """ | ||
| return asyncio.run(self._prepare_run_async(request=request)) |
Contributor
There was a problem hiding this comment.
so this asyncio.run loop cancels tasks created during initialization when we close it so that could leave the returned scenario with dead async resources
Starting a run initializes everything eagerly so configuration errors reach the caller. That work is slow and mostly synchronous -- loading the default datasets alone takes minutes -- and it ran directly on the event loop, so the server answered nothing for its duration. Health probes timed out and the CLI reported the server as unavailable even though it was alive and busy. Move the eager initialization off the event loop, following the pattern initializer_service already uses for the same reason. The semaphore, the active-task registry and the create_task hand-off all stay on the server loop: asyncio.run cancels whatever is still pending when it closes its loop, so a background task created inside the worker would be destroyed as soon as initialization finished. That contract is now reported rather than merely documented -- initialization warns, naming the tasks, if it leaves any behind. Run the preparations on a single dedicated worker rather than the default executor. Initialization writes to CentralMemory, and the in-memory SQLite backend shares one DBAPI connection across every thread, so two preparations at once used that connection concurrently and lost writes. Three concurrent starts against an in-memory database landed 40 of 120 seeds and raised InterfaceError; serialized on one worker they land 120 of 120. The event loop is still free while they run, which is the point of the offload. Hold the concurrency permit until the worker thread actually stops. A cancelled await does not kill the thread, so releasing the permit as the frame unwound admitted the next run while the abandoned one was still loading datasets, and max_concurrent_runs stopped bounding the work that was really running. The prepare call is shielded and, when it is abandoned, ownership of the permit passes to a completion callback that returns it once the thread has finished. Also fix two paths that leaked a concurrency permit. CancelledError is a BaseException, so cancellation during initialization -- on shutdown, or whenever the task is cancelled -- was not caught by the existing except Exception, and the missing scenario_result_id check sat outside the try block entirely. Enough such failures exhaust the limit and wedge the server for the rest of the session. The permit is now released from a finally block until ownership transfers to the background task, and the response is built before the hand-off so a lookup failure cannot leave a run executing that the caller has no id for. test_start_run_exceeds_concurrent_limit now holds its background runs open. It previously relied on the event loop never yielding during start, so the mocked runs completed and returned their permits before the limit could be reached.
varunj-msft
force-pushed
the
varunj-msft/v1.1.0-Release-Scenario-Start-Responsiveness
branch
from
September 1, 2026 18:04
1b69719 to
6eae54c
Compare
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
POST /runsinitializes everything eagerly, on purpose, so that configuration errors reach the caller instead of disappearing into a background task. The problem is where that work ran.It ran directly on the event loop, and it is slow and almost entirely synchronous — loading the default datasets alone takes minutes. For that whole window the server answered nothing at all. Health probes timed out, and the CLI reported the server as unavailable even though it was alive and simply busy. That is the failure mode behind the current End to End Tests failures, where the client gives up before the server has any chance to reply.
The main fix moves the eager initialization onto a worker thread with
asyncio.to_thread, following the patterninitializer_servicealready uses for the same reason. The semaphore, the active-task registry and thecreate_taskhand-off all deliberately stay on the server loop:asyncio.runcancels whatever is still pending when it closes its loop, so a background task created inside the worker thread would be destroyed the instant initialization finished. That shape silently cancels every run, and it is specifically avoided here.Two concurrency-permit leaks are fixed along the way. Both are on paths the previous
except Exception: release; raisecould not reach:CancelledError, which is aBaseExceptionand so was never caught.scenario_result_idcheck sat outside thetryblock entirely.Either one leaked a permit, and three such failures exhausted the concurrency limit and wedged the server for the rest of the session. For the E2E suite that matters, because one session-scoped backend serves every scenario in the run. The permit is now released from a
finallyblock until ownership transfers to the background task, tracked with an explicitrelease_on_exitflag so it is released exactly once and never twice.The response is also built before the task hand-off, so a lookup failure can no longer leave a run executing that the caller never received an id for. The
active_tasksentry is unwound on that path too.Finally, the
start_scenario_runroute docstring said "Returns immediately", which was not true before this change and is still not true after it. It now describes what actually happens.Part of the v1.1.0 release wave with #2510, #2511 and #2512.
Tests and Documentation
Six new tests in
tests/unit/backend/test_scenario_run_service.py:test_start_run_keeps_event_loop_responsivecounts heartbeats on the loop during a slow start. A blocked loop yields zero.test_start_run_background_task_survives_handoffasserts the run actually executes. This is the test that catches the "silently cancels every run" shape.test_start_run_releases_semaphore_when_cancelled_during_initcoversCancelledErrorbeing aBaseException.test_start_run_releases_semaphore_when_result_id_missingcovers the check that used to sit outside thetry.test_start_run_cleans_up_when_response_lookup_failsasserts no stranded permit and no strandedactive_tasksentry.test_start_run_releases_semaphore_exactly_once_on_successguards against the obvious over-correction of double-releasing.The first two matter as a pair rather than individually: a responsiveness fix that cancels every run would pass the responsiveness test on its own, so the hand-off test is what makes the first one meaningful.
test_start_run_exceeds_concurrent_limitneeded a fix. It was passing for the wrong reason: it relied on the event loop never yielding during start, so the mocked runs completed and handed their permits straight back before the limit could ever be reached. Now that start yields, the test holds its background runs open, which is what a real run does.Ran
pytest tests/unit/backend/test_scenario_run_service.py: 70 passed.Documentation: the
start_scenario_runroute docstring is corrected in this PR. JupyText was not run and is not applicable: no notebooks or code samples are affected, and the public API is unchanged.