server: take the state lock around model unload

[CryptVenture]

handle_unload_models and handle_unload_all_models read the loaded-model state without holding models_mutex_, and unload-all waited on the busy guard with no timeout. This takes the lock and applies the configured timeout. Access control is unchanged: both routes stay reachable exactly as they are today.

The race

models_ is a std::vector<std::unique_ptr<LoadedModel>> and model_index_ maps id to index. /v1/models/load appends to models_ under models_mutex_; that append can reallocate the vector.

• handle_unload_models looked the id up in model_index_ and dereferenced models_[it->second] with no lock. A concurrent load can reallocate between the lookup and the dereference.
• handle_unload_all_models iterated models_ directly, so a reallocation mid-iteration invalidates the iterator.
• live_ingest_limits read both members unlocked on the request path.

The fix resolves each id under the lock and keeps only the LoadedModel *. That pointer stays valid across a reallocation because the vector holds unique_ptr — the pointee does not move. Unload-all snapshots the resident pointers under the lock and then works from the snapshot. live_ingest_limits takes the lock for its two reads.

The unbounded wait

Unload-all called model->busy.acquire(0, model->config.id), which waits forever. An operator calling this route is usually trying to clear a wedged run, so waiting on that run indefinitely is the wrong behaviour. It now calls acquire_model_run(*model, std::nullopt), which applies the configured busy timeout — the per-id path already did this.

Verification

Built and run on macOS with the Metal backend, started without --ui-management:

POST /v1/tasks/unload_all_models  -> 200
POST /v1/tasks/unload_models      -> 200
POST /v1/models/load              -> 403   (pre-existing gate, untouched)

Both unload routes behave exactly as they do on main for every caller. ctest passes 40/40.

Note

An earlier revision of this PR also gated the two unload routes behind --ui-management. That was wrong for this project — the server does not implement access control by design, and deployments are expected to keep these endpoints away from untrusted clients. The gate is gone; only the concurrency fix remains.

[0xShug0]

The endpoint was introduced in PR #197 for server-side model memory management. I don’t think we should gate it behind --ui-management. The boundary feels awkward for non-UI pipelines. More importantly, the server is intentionally lightweight and does not provide a full security model, such as built-in access control. We expect access to the server to be protected by the deployment environment. That means users should make sure these endpoints are not reachable by untrusted clients. Changing it now would break existing direct API users of /v1/tasks/unload_models and /v1/tasks/unload_all_models.

[CryptVenture]

That is fair, and I have dropped the gate. Thanks for the context on #197 — I had read the asymmetry with /v1/models/load as an oversight rather than a deliberate boundary.

I have force-pushed a version that keeps only the concurrency fix, and retitled it accordingly. /v1/tasks/unload_models and /v1/tasks/unload_all_models stay reachable exactly as before; no existing caller changes behaviour.

What remains is the data race:

• handle_unload_models read models_ and model_index_ without holding models_mutex_. A concurrent /v1/models/load appends to models_, and that reallocation invalidates a pointer another thread is walking. Each id is now resolved under the lock, keeping only the LoadedModel * — stable because the vector holds unique_ptr.
• handle_unload_all_models iterated models_ directly for the same reason; it now snapshots the resident pointers under the lock first.
• live_ingest_limits read the same two members unlocked and now takes the lock.
• Unload-all waited on busy.acquire(0, id), an unbounded wait against exactly the wedged run an operator calls this route to clear. It now uses acquire_model_run, so the configured timeout applies — the per-id path already did this.

Happy to split the acquire_model_run change out if you would rather review the locking on its own.