fix(controller): don't fail the losing racer of a concurrent wakeup_model() call - #409
Open
AmirF194 wants to merge 3 commits into
Open
fix(controller): don't fail the losing racer of a concurrent wakeup_model() call#409AmirF194 wants to merge 3 commits into
AmirF194 wants to merge 3 commits into
Conversation
…odel() call del self.sleeping_models[model_name] raises KeyError when two wakeup_model() calls for the same model race: both pass the sleeping-membership check, both await their own real upstream wake call, and only the first del succeeds. The second caller's KeyError is caught by the surrounding except Exception and turned into a spurious False, even though its own upstream wake call succeeded. Use self.sleeping_models.pop(model_name, None) instead, so the losing racer no longer raises and both callers report the true outcome of their own upstream call. Fixes ovg-project#408
Contributor
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Contributor
Author
|
No rush, just a check-in after a week. Pre-commit CI is still awaiting approval to run on this fork PR, nothing has executed yet. |
This was referenced Aug 6, 2026
Contributor
Author
|
Closing to keep the queue clean; happy to reopen if there's interest. |
Contributor
Author
|
Reopening. My previous close was a mistake on my end, this is still open work and I'd like to see it through. |
Contributor
Author
|
Closing to keep the queue clean, no maintainer signal in a month. Happy to reopen if there is interest, the branch stays up. |
Contributor
Author
|
Sorry, reopened. I closed this by mistake, it is still on the roadmap in #419 and nothing here needed closing. |
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.
Fixes #408.
wakeup_model()checksmodel_name in self.sleeping_models, awaits thereal upstream wake call, then does
del self.sleeping_models[model_name].That dict has exactly two writers in the package (
put_model_to_sleepsetsit,
wakeup_modeldeleted it): no lock guards the check-then-mutate acrossthe
await. Two concurrentwakeup_model()calls for the same model bothpass the membership check, both call their own real upstream wake API, and
both reach the
del. Only the first succeeds; the second raisesKeyError, caught by the surroundingexcept Exception, so that callerreports
Falseeven though its own upstream wake call succeeded.Fix:
self.sleeping_models.pop(model_name, None)instead ofdel. Thelosing racer no longer raises, so both callers reach
return True, whichis correct since both of their own upstream calls succeeded.
Verified in a clean
python:3.11-slimcontainer at HEAD (b55096c):tests/test_wakeup_race.pyfires two concurrentwakeup_model()callswith a mocked (always-succeeding) upstream wake API. Before the fix:
[True, False]. After:[True, True]. The mock only replaces theupstream HTTP call, not
sleeping_modelsor either of its two writers.Also ran the repo's own gates on the changed files:
pre-commit run(ruff, codespell, isort, SPDX header check, trailing-whitespace, local
mypy) and the CI-only
mypy-3.12manual hook, all passing.Not verified: behavior against the real vLLM/SGLang wake APIs (only the
mocked upstream call), and whether two concurrent upstream wake calls to
the same backend are themselves safe to fire together, that's existing
behavior this change does not touch.