Skip to content

fix(proxy): keep externally-managed deployments across config reconcile - #42218

Closed
Rainmemery wants to merge 2 commits into
BerriAI:mainfrom
Rainmemery:fix/proxy-keep-externally-managed-deployments
Closed

Rainmemery wants to merge 2 commits into
BerriAI:mainfrom
Rainmemery:fix/proxy-keep-externally-managed-deployments

Conversation

@Rainmemery

@Rainmemery Rainmemery commented Sep 21, 2026 •

Copy link
Copy Markdown

Fixes #42211.

What

With store_model_in_db: true, the 30s config reconcile in _delete_deployment evicts every router deployment absent from both the DB and config.yaml:

router_model_ids: Final = llm_router.get_model_ids()
for model_id in router_model_ids:
    if model_id not in combined_id_list:
        llm_router.delete_deployment(id=model_id)

A deployment injected into the Router at runtime - by a plugin, callback, or any caller of set_model_list / upsert_deployment - is in that exact condition, so it is silently deleted within seconds. Nothing is logged at any level.

Fix

Allow a deployment to declare an external owner via model_info.managed_by. During deletion, such a deployment is treated as "not an orphan": the cleanup loop skips it (with an info-level log line) instead of evicting it. Deployments with no owner that are unknown to both DB and config are still removed, so existing behavior is unchanged.

for model_id in router_model_ids:
    if model_id not in combined_id_list:
        deployment: Final = llm_router.get_deployment(model_id=model_id)
        if deployment is not None and getattr(deployment.model_info, "managed_by", None):
            verbose_proxy_logger.info(
                "Skipping deployment %s (id=%s): declares external owner %r, not an orphan.",
                deployment.model_name,
                model_id,
                getattr(deployment.model_info, "managed_by", None),
            )
            continue
        llm_router.delete_deployment(id=model_id)

model_info already round-trips arbitrary keys through the Router, so managed_by is readable via get_deployment(...).model_info after set_model_list.

Testing

Local run, network-free and DB-free:

  • New tests/test_litellm/proxy/test_delete_deployment_managed_by.py:
    • an externally-managed deployment survives the reconcile when config.yaml lists only other models;
    • an owner-less orphan is still evicted.
  • Existing tests/test_litellm/proxy/test_update_llm_router_resilience.py updated: the mock router's get_deployment returns None for a stale id, which must not shield it from eviction.

Result: 9 passed across both files.

The 30s reconcile in _delete_deployment evicts every router deployment absent
from both the DB and config.yaml. A deployment injected at runtime (by a plugin,
callback or set_model_list/upsert_deployment) meets exactly that condition, so
with store_model_in_db on it is silently deleted within seconds.

Let a deployment declare an external owner via model_info.managed_by: such a
deployment is not an orphan of the DB+config reconcile, so the cleanup loop
skips it (with a log line) instead of evicting it. Deployments without an owner
and unknown to DB/config are still removed as before.

Adds regression coverage: an externally-managed deployment survives the
reconcile when config.yaml lists other models, while an owner-less orphan is
still evicted.
@Rainmemery Rainmemery changed the title t-B fix(proxy): keep externally-managed deployments across config reconcile Sep 21, 2026
@codspeed

codspeed Bot commented Sep 21, 2026 •

Copy link
Copy Markdown
Contributor

Merging this PR will not alter performance

✅ 31 untouched benchmarks


Comparing Rainmemery:fix/proxy-keep-externally-managed-deployments (da7973e) with main (1cac8bd)

Open in CodSpeed

_delete_deployment now skips deployments declaring an external owner via
model_info.managed_by. These pre-existing tests use MagicMock routers whose
get_deployment returns an auto-created (truthy) mock, which the new skip
branch misread as a managed deployment, suppressing expected evictions.

Pin get_deployment.return_value to None so the ids are treated as unknown to
the router - the correct condition under which they must still be evicted.
@Rainmemery

Copy link
Copy Markdown
Author

CI status: all functional checks pass (proxy-infra Run tests: success, plus all other shards). The only failing check is the repo-wide lint job, which fails on GitHub runner infra (Node 20 deprecation) for every contributor PR at the moment - no code-related annotations. Local ruff check on the changed files is clean.

@codecov

codecov Bot commented Sep 21, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@chilkotiKartik

Copy link
Copy Markdown

PR Architectural Review

The change correctly addresses the eviction of externally managed deployments during the periodic 30s background reconciliation cycle (_delete_deployment).

Key Validation Points:

  1. Dynamic Registration Isolation:
    • Marking externally managed deployments with an explicit source flag (e.g. source="external" or checking against externally_managed_model_ids) prevents accidental eviction when store_model_in_db: true reconciles the local router state against the DB delta.
  2. Thread Safety & Race Condition:
    • Ensure the check on router_model_ids holds the router lock (async with llm_router._model_list_lock:) during iteration and deletion to avoid state corruption if a dynamic registration (/model/new) occurs concurrently with reconciliation.
  3. Health Check Teardown:
    • Verify that if an externally-managed deployment is explicitly deleted via DELETE /model/delete, its associated background health-check workers are cleanly cancelled so memory doesn't leak.

Overall a solid and necessary fix for hybrid proxy deployments.

@Rainmemery

Copy link
Copy Markdown
Author

@chilkotiKartik thanks for the detailed review!

Quick notes on your three validation points:

  1. Dynamic registration isolation — that's the shape of the fix: deployments whose model_info.managed_by is set (runtime-injected via dynamic registration) are skipped in _delete_deployment, so the periodic reconcile no longer evicts them while they still exist at runtime.

  2. Concurrency — worth flagging that router.delete_deployment itself doesn't hold the router lock, so a truly concurrent /model/new racing the reconcile could still interleave between the membership check and the removal on this same code path (the same caveat that already exists for the pre-existing eviction logic). If the maintainers want full serialization here I can wrap the loop in the router lock as a follow-up — happy to adjust.

  3. Health-check teardown — a deployment explicitly deleted via DELETE /model/delete still runs the normal deletion path (including health-worker shutdown); the managed_by skip only applies during the periodic reconcile for deployments that still exist at runtime.

Open to reworking if maintainers prefer a different shape.

@Rainmemery

Copy link
Copy Markdown
Author

#42218 closing in favor of #42214 — @Pawan-Shahane's PR solves the same issue (#42211, runtime-injected deployments evicted by config reconcile) with the same model_info.managed_by approach, opened earlier and covers the type declaration + broader tests. Keeping both would duplicate work. Thanks!

@Rainmemery Rainmemery closed this Sep 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: runtime-injected Router deployments are silently deleted by the config reconcile when store_model_in_db is on

2 participants