[CRITICAL] start_model memory check + status update nested inside 'stop other active models' loop (processes.py:90-140) #39

New issue

Closed

opened 2026-09-14 17:18:59 +00:00 by Mark · 3 comments

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

Owner

Copy link

Summary

Memory check and status="loading" update are incorrectly nested inside the "stop other active models" loop, producing redundant work, stale GPU state, and a missing status update when no other models are active.

Location

backend/app/routers/processes.py lines 88-140 (in start_model).

Bug

Lines 103-140 (# Run pre-flight memory check... through await repo.update_status(model_id, status="loading")) are indented under the for other in active_models: loop started at line 90, not after it.

Observed effects at HEAD e91deb0:

1. N-fold redundant memory check. With N other active models, MemoryChecker.check_before_start runs N times — each against progressively-changing GPU state as stop_model calls free VRAM. The check on iteration 2+ sees memory reclaimed by iteration 1's stop, so it may pass when it shouldn't (or vice versa).

2. Wrong GPU state on first check. The very first memory check runs before any stop_model call has freed memory from competing models. If two models are simultaneously active, the check is performed against occupied VRAM and may fail or succeed non-deterministically.

3. status="loading" set N times instead of once. Cosmetic but noisy; also means the DB row gets touched per-iteration.

4. Memory check skipped entirely when active_models is empty. The loop body still runs once for each item, but if the list is empty the body never executes — so the check and the status update both vanish. A fresh start of the only model skips the pre-flight check.

5. Dead variable. backend_impl = get_backend_from_string(backend) at line 144 is created and never used.

Reproduction

# Start two models concurrently active:
#   POST /api/models/<model_A>/start
#   POST /api/models/<model_B>/start  (with --proxies-or-not to allow concurrent)
# Then call:
curl -X POST -H "Authorization: Bearer $TOKEN" \
     "$BASE/api/models/<model_A>/start"
# Watch logs: "Memory check passed:" prints N+1 times instead of 1.

Expected: exactly one memory check, performed after all other models are stopped.

Suggested Fix

Dedent lines 103-140 to be siblings of the for loop (i.e., at the same indentation as line 142's try:). Sequence should be:

1. Stop other active models (loop at line 90).
2. Run memory check (once).
3. Set status="loading".
4. Reserve port and start subprocess.

Drop the unused backend_impl at line 144.

Severity

Critical — affects every multi-model start, can produce false-positive OOM errors or false-negative OOM kills under load.

Detected

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

## Summary Memory check and `status="loading"` update are incorrectly nested inside the "stop other active models" loop, producing redundant work, stale GPU state, and a missing status update when no other models are active. ## Location `backend/app/routers/processes.py` lines 88-140 (in `start_model`). ## Bug Lines 103-140 (`# Run pre-flight memory check...` through `await repo.update_status(model_id, status="loading")`) are indented under the `for other in active_models:` loop started at line 90, not after it. Observed effects at HEAD e91deb0: 1. **N-fold redundant memory check.** With N other active models, `MemoryChecker.check_before_start` runs N times — each against progressively-changing GPU state as `stop_model` calls free VRAM. The check on iteration 2+ sees memory reclaimed by iteration 1's stop, so it may pass when it shouldn't (or vice versa). 2. **Wrong GPU state on first check.** The very first memory check runs *before* any `stop_model` call has freed memory from competing models. If two models are simultaneously active, the check is performed against occupied VRAM and may fail or succeed non-deterministically. 3. **`status="loading"` set N times instead of once.** Cosmetic but noisy; also means the DB row gets touched per-iteration. 4. **Memory check skipped entirely when `active_models` is empty.** The loop body still runs once for each item, but if the list is empty the body never executes — so the check and the status update both vanish. A fresh start of the only model skips the pre-flight check. 5. **Dead variable.** `backend_impl = get_backend_from_string(backend)` at line 144 is created and never used. ## Reproduction ```bash # Start two models concurrently active: # POST /api/models/<model_A>/start # POST /api/models/<model_B>/start (with --proxies-or-not to allow concurrent) # Then call: curl -X POST -H "Authorization: Bearer $TOKEN" \ "$BASE/api/models/<model_A>/start" # Watch logs: "Memory check passed:" prints N+1 times instead of 1. ``` Expected: exactly one memory check, performed after all other models are stopped. ## Suggested Fix Dedent lines 103-140 to be siblings of the `for` loop (i.e., at the same indentation as line 142's `try:`). Sequence should be: 1. Stop other active models (loop at line 90). 2. Run memory check (once). 3. Set `status="loading"`. 4. Reserve port and start subprocess. Drop the unused `backend_impl` at line 144. ## Severity Critical — affects every multi-model start, can produce false-positive OOM errors or false-negative OOM kills under load. ## 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 commented 2026-09-18 15:22:35 +00:00

Author

Owner

Copy link

Fix applied

Drops the dead backend_impl = get_backend_from_string(backend) line in start_model plus its now-orphaned get_backend_from_string import.

Context

The indentation bug originally reported in this issue (memory check + status=loading nested under the for other in active_models loop) was already fixed by the #35 PR (commit 9aa91ec). However, the dead backend_impl assignment survived the dedent, and ruff F401 was flagging the now-orphaned import. This commit cleans up the remaining artifacts.

Diff

backend/app/routers/processes.py  | 2 --
backend/tests/test_processes.py   | 7 -------
2 files changed, 9 deletions(-)

Verification

• python3 -m py_compile backend/app/routers/processes.py — OK
• python3 -m ruff check backend/app/routers/processes.py backend/tests/test_processes.py — All checks passed
• python3 -m pytest backend/tests/test_processes.py — 11 passed
• python3 -m pytest backend/ — 234 passed, 2 skipped

Branch: fix/39-start-model-memory-check-dedent (commit 1ef3f9f) — sitting on top of origin/main (ea16ebf). Currently blocked from pushing due to a server-side sideband-disconnect issue unrelated to the diff. Local commit verified; ready for verifier to push and merge.

## Fix applied Drops the dead `backend_impl = get_backend_from_string(backend)` line in `start_model` plus its now-orphaned `get_backend_from_string` import. ### Context The indentation bug originally reported in this issue (memory check + status=loading nested under the `for other in active_models` loop) was already fixed by the #35 PR (commit 9aa91ec). However, the dead `backend_impl` assignment survived the dedent, and ruff F401 was flagging the now-orphaned import. This commit cleans up the remaining artifacts. ### Diff ``` backend/app/routers/processes.py | 2 -- backend/tests/test_processes.py | 7 ------- 2 files changed, 9 deletions(-) ``` ### Verification - `python3 -m py_compile backend/app/routers/processes.py` — OK - `python3 -m ruff check backend/app/routers/processes.py backend/tests/test_processes.py` — All checks passed - `python3 -m pytest backend/tests/test_processes.py` — 11 passed - `python3 -m pytest backend/` — 234 passed, 2 skipped Branch: `fix/39-start-model-memory-check-dedent` (commit `1ef3f9f`) — sitting on top of `origin/main` (`ea16ebf`). Currently blocked from pushing due to a server-side sideband-disconnect issue unrelated to the diff. Local commit verified; ready for verifier to push and merge.

Mark referenced this issue from a commit 2026-09-18 23:14:02 +00:00

Fix #39: drop dead backend_impl binding in start_model

Mark referenced this issue from a pull request that will close it, 2026-09-18 23:14:29 +00:00

Fix #39: dedent pre-flight memory check + status update out of 'stop other active models' loop #44

Mark closed this issue 2026-09-18 23:14:45 +00:00

Mark referenced this issue from a commit 2026-09-18 23:14:46 +00:00

Merge pull request 'Fix #39: dedent pre-flight memory check + status update out of 'stop other active models' loop' (#44) from fix/39-start-model-memory-check-dedent into main

Mark commented 2026-09-18 23:15:34 +00:00

Author

Owner

Copy link

PR #4 opened with the proper title + rule-to-guard mapping:

PR: Mark/kanban#4

Note: PR #2 was the merge PR (auto-generated title "Merge 137-kanban-guards-and-watcher-branches -> main"), already merged to main on 2026-09-12 16:51:50 UTC as commit 4b983dc. PR #4 captures the rule-to-guard mapping table that PR #2's auto-description lacked.

Rule-to-guard summary (full table in PR #4 description)

#	Rule	Code guard	Test
1	done is terminal; no transition out of done	server/transition_guards.ts::computeFlagUpdatesForTransition sets terminal: 1 on done entry; line 132 rejects done→X with kind: done_is_terminal. Wired into server/storage.ts::transitionTask (line 290, atomic flag update). 409 surface at server/routes.ts.	(a) four_guards_integration.test.ts:142 — 2 tests
2	blocked only returns to active via kanban_unblock	server/watcher.ts::branch_blocked (line 150, unsubscribe; re-add only on kanban_unblock event in deliverKanbanEvent line 287) + server/storage.ts::transitionTask (line 236, rejects blocked→active with typed error referencing kanbanUnblock) + server/storage.ts::kanbanUnblock (line 55, sole handler)	(b) four_guards_integration.test.ts:251 — 3 tests
3	running is owned exclusively by the watcher	server/transition_guards.ts::assertCallerAllowedForRunning (rejects non-watcher with kind: foreign_running_write, missing/null caller with kind: missing_caller_for_running) + server/watcher.ts::branch_running (line 170, tick-entry assertion card.owner !== WATCHER_PRINCIPAL throws WatcherError{kind: foreign_running_write}) + server/principal.ts::deriveCallerFromRequest (HTTP trust boundary, body-supplied caller ignored, server-derived from X-Internal-Actor header)	(c) four_guards_integration.test.ts:382 — 5 tests
4	Done cards never re-watched	server/watcher.ts::branch_done (line 131, drop_done action; _watchSet.add only at line 181 [legit running dispatch] and line 287 [kanban_unblock arm]) + KANBAN_EVENT_CATALOG exposes no event that re-adds a done card	(d) four_guards_integration.test.ts:552 — 3 tests

CI status

CI on the merge commit (4b983dc) is GREEN — run #6623, status=success, 93 seconds duration, triggered by push to main on 2026-09-12 17:55:51Z → 2026-09-12 17:57:24Z. URL: https://forgejo.tail018ac4.ts.net/Mark/kanban/actions/runs/2.

The PR #4 head (a0bc081) is an ancestor of the merge commit (4b983dc) and contains the same code that CI verified — 157/157 vitest pass locally on the branch HEAD.

Acceptance

• All four rules enforced in code with explicit guards (not just convention) ✓
• kanban_unblock is the only path out of blocked ✓
• Watcher drop-list for done verified by test (5 sweeps, no re-subscribe) ✓
• PR opened referencing #137 with rule-to-guard mapping table ✓
• CI green on the merged code ✓
• Four named integration tests exist and pass ✓

Refs: kanban t_d3f147f9 (root), t_e49638bb (implementer), t_002084ad/t_c657a9cc/t_383734ad/t_fc944a5a (children), t_7fd87e14 (verifier).

PR #4 opened with the proper title + rule-to-guard mapping: **PR**: https://forgejo.tail018ac4.ts.net/Mark/kanban/pulls/4 Note: PR #2 was the merge PR (auto-generated title "Merge 137-kanban-guards-and-watcher-branches -> main"), already merged to main on 2026-09-12 16:51:50 UTC as commit `4b983dc`. PR #4 captures the rule-to-guard mapping table that PR #2's auto-description lacked. ## Rule-to-guard summary (full table in PR #4 description) | # | Rule | Code guard | Test | |---|---|---|---| | 1 | `done` is terminal; no transition out of `done` | `server/transition_guards.ts::computeFlagUpdatesForTransition` sets `terminal: 1` on done entry; line 132 rejects done→X with `kind: done_is_terminal`. Wired into `server/storage.ts::transitionTask` (line 290, atomic flag update). 409 surface at `server/routes.ts`. | (a) `four_guards_integration.test.ts:142` — 2 tests | | 2 | `blocked` only returns to active via `kanban_unblock` | `server/watcher.ts::branch_blocked` (line 150, unsubscribe; re-add only on `kanban_unblock` event in `deliverKanbanEvent` line 287) + `server/storage.ts::transitionTask` (line 236, rejects blocked→active with typed error referencing `kanbanUnblock`) + `server/storage.ts::kanbanUnblock` (line 55, sole handler) | (b) `four_guards_integration.test.ts:251` — 3 tests | | 3 | `running` is owned exclusively by the watcher | `server/transition_guards.ts::assertCallerAllowedForRunning` (rejects non-watcher with `kind: foreign_running_write`, missing/null caller with `kind: missing_caller_for_running`) + `server/watcher.ts::branch_running` (line 170, tick-entry assertion `card.owner !== WATCHER_PRINCIPAL` throws `WatcherError{kind: foreign_running_write}`) + `server/principal.ts::deriveCallerFromRequest` (HTTP trust boundary, body-supplied caller ignored, server-derived from `X-Internal-Actor` header) | (c) `four_guards_integration.test.ts:382` — 5 tests | | 4 | Done cards never re-watched | `server/watcher.ts::branch_done` (line 131, `drop_done` action; `_watchSet.add` only at line 181 [legit running dispatch] and line 287 [`kanban_unblock` arm]) + `KANBAN_EVENT_CATALOG` exposes no event that re-adds a done card | (d) `four_guards_integration.test.ts:552` — 3 tests | ## CI status CI on the merge commit (`4b983dc`) is GREEN — run #6623, status=success, 93 seconds duration, triggered by push to main on 2026-09-12 17:55:51Z → 2026-09-12 17:57:24Z. URL: https://forgejo.tail018ac4.ts.net/Mark/kanban/actions/runs/2. The PR #4 head (`a0bc081`) is an ancestor of the merge commit (`4b983dc`) and contains the same code that CI verified — 157/157 vitest pass locally on the branch HEAD. ## Acceptance - All four rules enforced in code with explicit guards (not just convention) ✓ - `kanban_unblock` is the only path out of `blocked` ✓ - Watcher drop-list for `done` verified by test (5 sweeps, no re-subscribe) ✓ - PR opened referencing #137 with rule-to-guard mapping table ✓ - CI green on the merged code ✓ - Four named integration tests exist and pass ✓ Refs: kanban t_d3f147f9 (root), t_e49638bb (implementer), t_002084ad/t_c657a9cc/t_383734ad/t_fc944a5a (children), t_7fd87e14 (verifier).

Mark commented 2026-09-18 23:15:35 +00:00

Author

Owner

Copy link

Verified merged — fix landed on main at 2fe61099 via PR #44.

• Merge commit: 2fe61099d536a7c12f0be74cdb775ff913b92323 (main advanced from ea16ebf → 2fe61099)
• Issue auto-closed on merge: closed_at: 2026-09-18T23:14:45Z
• Branch fix/39-start-model-memory-check-dedent merged into main

Diff was 9 deletions across 2 files (backend/app/routers/processes.py, backend/tests/test_processes.py), dropping the dead backend_impl = get_backend_from_string(backend) binding, the orphaned get_backend_from_string import, and the matching @patch decorator + Mock setup from the #35 regression test. Ruff clean, 234/234 backend tests passed on the worktree.

**Verified merged** — fix landed on `main` at `2fe61099` via PR [#44](https://forgejo.tail018ac4.ts.net/Mark/vllm_mm/pulls/44). - Merge commit: `2fe61099d536a7c12f0be74cdb775ff913b92323` (main advanced from `ea16ebf` → `2fe61099`) - Issue auto-closed on merge: `closed_at: 2026-09-18T23:14:45Z` - Branch `fix/39-start-model-memory-check-dedent` merged into `main` Diff was 9 deletions across 2 files (`backend/app/routers/processes.py`, `backend/tests/test_processes.py`), dropping the dead `backend_impl = get_backend_from_string(backend)` binding, the orphaned `get_backend_from_string` import, and the matching `@patch` decorator + `Mock` setup from the #35 regression test. Ruff clean, 234/234 backend tests passed on the worktree.

Sign in to join this conversation.

No Branch/Tag specified

Branches Tags

main

No results found.

Labels

Clear labels   

blocked:lint-failed

  

blocked:needs-decision

  

blocked:no-changes

  

blocked:no-procedure

  

blocked:preflight

Blocked by the kilo-gsd preflight gate (lint dirty, missing tooling, etc.). Picker skips until the cause is cleared.

  

blocked:verify-failed

  

bug

  

feature-request

  

in-progress

Issue is currently being worked on by a quickfix runner

  

lint-cleanup

Repo-wide ruff lint cleanup required before kilo-gsd can dispatch feature work on this repo.

  

ops-task

Operations task — install, configure, deploy, API calls

  

quickfix

Pipe to opencode for direct fix

  

security/high

High severity security issue

  

security/medium

Medium severity security issue

  

test-coverage

Test coverage review / gap report

  

triage-approved

No labels

blocked:lint-failed

blocked:needs-decision

blocked:no-changes

blocked:no-procedure

blocked:preflight

blocked:verify-failed

bug

feature-request

in-progress

lint-cleanup

ops-task

quickfix

security/high

security/medium

test-coverage

triage-approved

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

Mark/vllm_mm#39

No description provided.