[CRITICAL] Port reservation leak when start_model fails between reserve and Popen (process_manager.py:127-142, 262-272) #40

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

If start_model fails between reserve_process_id and Popen, the reserved port is never released — stop_model early-returns because the process isn't registered. After ~1000 failed starts the port pool is exhausted.

Location

backend/app/services/process_manager.py lines 123-142 (reserve_process_id)
backend/app/services/process_manager.py lines 262-272 (start_model exception handler)
backend/app/services/process_manager.py lines 404-435 (stop_model)

Bug

reserve_process_id records self._ports[process_id] = port (line 133) but does not store a Popen. If start_model raises between the reserve call and line 220 (self._processes[process_id] = process), the exception handler at line 262-272 calls await self.stop_model(process_id). stop_model checks if process_id not in self._processes: return False at line 406-407 and bails without touching _ports.

Failure windows where this leaks:

_download_gguf_model raises (lines 188, 274-402 — RuntimeError, HTTP error, disk full, OOM during download).
os.makedirs(log_dir, exist_ok=True) fails (line 226).
open(log_file, "w") fails (line 231) — read-only filesystem, permission denied.
subprocess.Popen(...) itself raises (line 212-218) — for example, the executable is missing.

Each leak consumes one port from the _base_port.._max_port range (defaults 18000-19000 = 1000 ports).

Reproduction

# In a Python shell with the backend venv active:
import asyncio
from app.services.process_manager import process_manager

async def leak():
    # Make start fail by passing a backend that won't be installed
    for i in range(1001):
        try:
            await process_manager.start_model(
                model_repo="nonexistent/model",
                backend="llama.cpp",
                settings={"gguf_file": "missing.gguf"},
            )
        except Exception:
            pass
    print(f"Ports held: {len(process_manager._ports)}")
    print(f"Available: {process_manager._max_port - process_manager._base_port - len(process_manager._ports)}")

asyncio.run(leak())

Expected: after enough failures to consume the range, a subsequent call raises RuntimeError("No free ports available...").

Suggested Fix

In start_model's exception block (around line 262), before calling stop_model:

except Exception as e:
    # Release port reservation if process never started
    if process_id not in self._processes and process_id in self._ports:
        del self._ports[process_id]
    if process_id in self._progress:
        del self._progress[process_id]
    # ... existing notify() + cleanup

Alternatively, teach reserve_process_id to track a reserved flag and stop_model to honor it.

Severity

Critical — silent resource leak that hard-fails the backend after enough failed starts. Especially dangerous in CI / scripted multi-model workflows.

Detected

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

## Summary If `start_model` fails between `reserve_process_id` and `Popen`, the reserved port is never released — `stop_model` early-returns because the process isn't registered. After ~1000 failed starts the port pool is exhausted. ## Location - `backend/app/services/process_manager.py` lines 123-142 (`reserve_process_id`) - `backend/app/services/process_manager.py` lines 262-272 (`start_model` exception handler) - `backend/app/services/process_manager.py` lines 404-435 (`stop_model`) ## Bug `reserve_process_id` records `self._ports[process_id] = port` (line 133) but does not store a `Popen`. If `start_model` raises between the reserve call and line 220 (`self._processes[process_id] = process`), the exception handler at line 262-272 calls `await self.stop_model(process_id)`. `stop_model` checks `if process_id not in self._processes: return False` at line 406-407 and bails without touching `_ports`. Failure windows where this leaks: - `_download_gguf_model` raises (lines 188, 274-402 — `RuntimeError`, HTTP error, disk full, OOM during download). - `os.makedirs(log_dir, exist_ok=True)` fails (line 226). - `open(log_file, "w")` fails (line 231) — read-only filesystem, permission denied. - `subprocess.Popen(...)` itself raises (line 212-218) — for example, the executable is missing. Each leak consumes one port from the `_base_port`..`_max_port` range (defaults 18000-19000 = 1000 ports). ## Reproduction ```python # In a Python shell with the backend venv active: import asyncio from app.services.process_manager import process_manager async def leak(): # Make start fail by passing a backend that won't be installed for i in range(1001): try: await process_manager.start_model( model_repo="nonexistent/model", backend="llama.cpp", settings={"gguf_file": "missing.gguf"}, ) except Exception: pass print(f"Ports held: {len(process_manager._ports)}") print(f"Available: {process_manager._max_port - process_manager._base_port - len(process_manager._ports)}") asyncio.run(leak()) ``` Expected: after enough failures to consume the range, a subsequent call raises `RuntimeError("No free ports available...")`. ## Suggested Fix In `start_model`'s exception block (around line 262), before calling `stop_model`: ```python except Exception as e: # Release port reservation if process never started if process_id not in self._processes and process_id in self._ports: del self._ports[process_id] if process_id in self._progress: del self._progress[process_id] # ... existing notify() + cleanup ``` Alternatively, teach `reserve_process_id` to track a `reserved` flag and `stop_model` to honor it. ## Severity Critical — silent resource leak that hard-fails the backend after enough failed starts. Especially dangerous in CI / scripted multi-model workflows. ## 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 referenced this issue from a commit

2026-09-18 23:13:07 +00:00

Fix #40: release reserved port on failed start_model

Mark closed this issue

2026-09-18 23:13:07 +00:00

Mark commented

2026-09-18 23:13:27 +00:00

Author

Owner

Verification complete — fix verified on remote.

Commit on origin/main: 4d96d915be55d34b6e41f564aa90898ad196e718 ("Fix #40: release reserved port on failed start_model")
Push: git push origin main succeeded — ea16ebf..4d96d91 main -> main
Tests: 245 backend tests pass / 2 skipped, including 2 new regression tests in TestPortReservationLeak (FileNotFoundError-from-Popen and immediate-exit paths through reserve_process_id -> start_model)
Fix: Added an else branch to start_model exception handler in backend/app/services/process_manager.py that releases the reserved port when the subprocess never registered (otherwise stop_model early-returns on the missing process_id and _ports leaks until the 1000-entry pool is exhausted).
Issue: #40 is now closed (closed_at 2026-09-18T23:13:07Z) with labels bug, triage-approved, vllm_mm.

Closing the fixit board card.

Verification complete — fix verified on remote. - **Commit on origin/main:** `4d96d915be55d34b6e41f564aa90898ad196e718` ("Fix #40: release reserved port on failed start_model") - **Push:** `git push origin main` succeeded — `ea16ebf..4d96d91 main -> main` - **Tests:** 245 backend tests pass / 2 skipped, including 2 new regression tests in `TestPortReservationLeak` (FileNotFoundError-from-Popen and immediate-exit paths through `reserve_process_id -> start_model`) - **Fix:** Added an `else` branch to `start_model` exception handler in `backend/app/services/process_manager.py` that releases the reserved port when the subprocess never registered (otherwise `stop_model` early-returns on the missing process_id and `_ports` leaks until the 1000-entry pool is exhausted). - **Issue:** #40 is now `closed` (closed_at 2026-09-18T23:13:07Z) with labels `bug`, `triage-approved`, `vllm_mm`. Closing the fixit board card.