Mark/vllm_mm

Fork 0

[CRITICAL] Model switch holds _model_switch_lock across 10-30s subprocess shutdown, serializing the entire inference queue (proxy.py:163-220) #41

New issue

Closed

opened 2026-09-14 17:18:59 +00:00 by Mark · 1 comment

Mark commented

2026-09-14 17:18:59 +00:00

Owner

Summary

_model_switch_lock is held across process_manager.stop_model and start_model, which serialize the entire inference queue behind a single 10-30s model swap.

Location

backend/app/routers/proxy.py lines 60-128 (InferenceQueue worker loop)
backend/app/routers/proxy.py lines 163-220 (switch path inside _forward_request)
backend/app/services/process_manager.py lines 412-421 (stop_model graceful shutdown)

Bug

InferenceQueue._worker_loop (line 96) launches max_workers (default 4) worker tasks that all funnel through async with self._model_switch_lock: (line 163) whenever a request names a model that isn't currently active. Inside that critical section:

await process_manager.stop_model(active.process_id) (line 179)
await session.commit() (line 187, 227)
await process_manager.start_model(...) (line 213)

stop_model blocks for up to 30 seconds on graceful termination (process_manager.py:412-421) before SIGKILL — and during those 30 seconds the other three workers in the inference queue are all blocked waiting on the same asyncio.Lock. They cannot pick up requests, cannot forward to the still-warming model, and the queue fills to max_queue_size (default 100) before requests start returning 503.

For a vLLM 7B model on a single GPU this manifests as: a single /v1/chat/completions call requesting a model switch stalls the entire proxy for 15-45 seconds. Streaming requests through the same path are particularly bad because the client sees zero bytes until the lock is released.

Reproduction

# 1. Start model A.
curl -X POST -H "Authorization: Bearer $TOKEN" "$BASE/api/models/A/start"
sleep 30

# 2. In one terminal, fire a steady stream of requests to A.
while true; do
  curl -sS -X POST -H "Authorization: Bearer $TOKEN" -H "Content-Type: application/json" \
       "$BASE/v1/chat/completions" \
       -d '{"model":"A","messages":[{"role":"user","content":"hi"}],"max_tokens":4}' &
done

# 3. In another terminal, switch to model B.
time curl -sS -X POST -H "Authorization: Bearer $TOKEN" "$BASE/api/models/B/start"

# 4. Observe: requests to A during the swap either hang or return 503
#    queue-full once the worker pool is starved.

Suggested Fix

Reduce the lock scope. Hold _model_switch_lock only for the in-process active-model re-check and the DB flip (proxy.py:163-188), then release before calling stop_model / start_model. Use a separate is_starting: str | None field (or second lock) so that:

Non-switching requests (active.id == target_model.id) pass through with no lock contention.
Switching requests acquire the swap lock only briefly to update DB state, then release so other workers can serve the new model once it's ready.

Concretely:

# 1. Inside lock: pick the target_model, mark "starting" with target id.
async with self._model_switch_lock:
    # ... active re-check, decide if switching, write "starting" sentinel
# 2. Outside lock: do stop_model() + start_model() (potentially long).
# 3. Re-acquire lock: flip active to target_model.

Workers checking the active model see is_starting and either (a) wait on a per-model event, or (b) return 503 "model loading" with Retry-After.

Severity

Critical — defeats the purpose of the inference queue under realistic model-swap workloads.

Detected

Code review of HEAD e91deb0 (2026-09-14).

## Summary `_model_switch_lock` is held across `process_manager.stop_model` and `start_model`, which serialize the entire inference queue behind a single 10-30s model swap. ## Location - `backend/app/routers/proxy.py` lines 60-128 (`InferenceQueue` worker loop) - `backend/app/routers/proxy.py` lines 163-220 (switch path inside `_forward_request`) - `backend/app/services/process_manager.py` lines 412-421 (`stop_model` graceful shutdown) ## Bug `InferenceQueue._worker_loop` (line 96) launches `max_workers` (default 4) worker tasks that all funnel through `async with self._model_switch_lock:` (line 163) whenever a request names a model that isn't currently active. Inside that critical section: - `await process_manager.stop_model(active.process_id)` (line 179) - `await session.commit()` (line 187, 227) - `await process_manager.start_model(...)` (line 213) `stop_model` blocks for up to 30 seconds on graceful termination (process_manager.py:412-421) before SIGKILL — and during those 30 seconds the *other three* workers in the inference queue are all blocked waiting on the same `asyncio.Lock`. They cannot pick up requests, cannot forward to the still-warming model, and the queue fills to `max_queue_size` (default 100) before requests start returning 503. For a vLLM 7B model on a single GPU this manifests as: a single `/v1/chat/completions` call requesting a model switch stalls the entire proxy for 15-45 seconds. Streaming requests through the same path are particularly bad because the client sees zero bytes until the lock is released. ## Reproduction ```bash # 1. Start model A. curl -X POST -H "Authorization: Bearer $TOKEN" "$BASE/api/models/A/start" sleep 30 # 2. In one terminal, fire a steady stream of requests to A. while true; do curl -sS -X POST -H "Authorization: Bearer $TOKEN" -H "Content-Type: application/json" \ "$BASE/v1/chat/completions" \ -d '{"model":"A","messages":[{"role":"user","content":"hi"}],"max_tokens":4}' & done # 3. In another terminal, switch to model B. time curl -sS -X POST -H "Authorization: Bearer $TOKEN" "$BASE/api/models/B/start" # 4. Observe: requests to A during the swap either hang or return 503 # queue-full once the worker pool is starved. ``` ## Suggested Fix Reduce the lock scope. Hold `_model_switch_lock` only for the in-process active-model re-check and the DB flip (proxy.py:163-188), then release before calling `stop_model` / `start_model`. Use a separate `is_starting: str | None` field (or second lock) so that: - Non-switching requests (`active.id == target_model.id`) pass through with no lock contention. - Switching requests acquire the swap lock only briefly to update DB state, then release so other workers can serve the new model once it's ready. Concretely: ```python # 1. Inside lock: pick the target_model, mark "starting" with target id. async with self._model_switch_lock: # ... active re-check, decide if switching, write "starting" sentinel # 2. Outside lock: do stop_model() + start_model() (potentially long). # 3. Re-acquire lock: flip active to target_model. ``` Workers checking the active model see `is_starting` and either (a) wait on a per-model event, or (b) return 503 "model loading" with `Retry-After`. ## Severity Critical — defeats the purpose of the inference queue under realistic model-swap workloads. ## Detected Code review of HEAD `e91deb0` (2026-09-14).

Mark added the

triage-approved

bug

labels

2026-09-15 15:36:55 +00:00

Mark added the

in-progress

label

2026-09-18 14:26:48 +00:00

Mark referenced this issue from a commit

2026-09-18 14:48:35 +00:00

# Fix #41: narrow _swap_inflight lock scope — don't hold across stop/start

Mark commented

2026-09-18 14:48:46 +00:00

Author

Owner

Closed by fix landed on fix/41-model-switch-lock-scope (commit 374736d).

What changed:

_model_switch_lock (now _swap_inflight) was renamed and its scope narrowed: it now serializes only the short DB flip in two windows, not the entire stop/start subprocess shutdown. Between those windows, stop_model + start_model run with the lock released. Concurrent workers asking for the same target model share one load by waiting on a per-model Event in _loading_events.

Regression tests added under TestModelSwitchLockScope + existing TestSwapLockScope:

test_lock_released_during_stop_model — observes lock state during stop_model
test_concurrent_swap_waiters_share_one_load — exactly 1 start_model call for N concurrent requests
test_loading_slot_cleared_after_successful_swap — slot cleared on success
test_failed_start_clears_loading_slot — slot cleared on start failure (no hang)
test_fast_path_no_lock_for_matching_active — fast path acquires no lock
test_concurrent_different_targets_run_concurrently — different targets don't serialize

Full backend test suite: 243 passed, 2 skipped.

Open follow-up: streaming /v1/chat/completions bypasses the queue entirely (proxy.py:939-953), so a model swap during a streaming request can still drop the SSE connection. That path should also surface a Retry-After 503 when the active model is being torn down.

Closed by fix landed on `fix/41-model-switch-lock-scope` (commit 374736d). **What changed:** `_model_switch_lock` (now `_swap_inflight`) was renamed and its scope narrowed: it now serializes only the short DB flip in two windows, not the entire stop/start subprocess shutdown. Between those windows, `stop_model` + `start_model` run with the lock released. Concurrent workers asking for the same target model share one load by waiting on a per-model `Event` in `_loading_events`. **Regression tests added under `TestModelSwitchLockScope` + existing `TestSwapLockScope`:** - `test_lock_released_during_stop_model` — observes lock state during stop_model - `test_concurrent_swap_waiters_share_one_load` — exactly 1 start_model call for N concurrent requests - `test_loading_slot_cleared_after_successful_swap` — slot cleared on success - `test_failed_start_clears_loading_slot` — slot cleared on start failure (no hang) - `test_fast_path_no_lock_for_matching_active` — fast path acquires no lock - `test_concurrent_different_targets_run_concurrently` — different targets don't serialize **Full backend test suite: 243 passed, 2 skipped.** Open follow-up: streaming /v1/chat/completions bypasses the queue entirely (proxy.py:939-953), so a model swap during a streaming request can still drop the SSE connection. That path should also surface a `Retry-After` 503 when the active model is being torn down.

Mark closed this issue

2026-09-18 14:48:51 +00:00