Close the session cap race under concurrent creation#319
Open
damilolaedwards wants to merge 1 commit into
Open
Close the session cap race under concurrent creation#319damilolaedwards wants to merge 1 commit into
damilolaedwards wants to merge 1 commit into
Conversation
The session limit was enforced by checking the current count and then creating the session afterward, with nothing tying the two together. Concurrent requests could all see room under the cap before any of their sessions existed yet, so a burst of creations could sail past max_sessions. The docker backend's implicit new-session path used by execute_python had no cap check on it at all. Add a reservation step that holds a slot in memory from the moment a creation is allowed until the attempt finishes, so concurrent callers can no longer all pass the same stale count. Wire it into both the explicit session-create path and the docker backend's implicit new-session path.
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
1 participant
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.
Summary
The session limit was enforced by counting existing sessions and creating
the new one afterward, with nothing tying the two steps together. A burst
of concurrent requests could all observe room under the cap before any of
their sessions existed yet, so the cap could be overshot under load. The
docker backend's implicit new-session path (used by execute_python when no
session_id is given) had no cap check on it at all.
Adds a reservation step on SessionManager that holds a slot in memory from
the moment a creation is allowed until the attempt finishes, so concurrent
callers can no longer all pass the same stale count. Wired into both the
explicit session-create path (docker and direct backends) and the docker
backend's implicit new-session path.
Test plan
go test -race ./pkg/sandbox/... ./pkg/execsvc/...holds under it with the fix, run repeatedly under
-racego build ./...,go vet ./...