[Feature] Proxy: opt-in v2 migration resolver - #26194
Conversation
Greptile SummaryThis PR adds an opt-in Confidence Score: 4/5Safe to merge after fixing the unguarded _create_baseline_migration call in the P3005 v2 branch. All prior review-thread issues (psycopg transaction state, db-push error swallowing, _warn_if_db_ahead_of_head DatabaseError, _resolve_specific_migration CalledProcessError escape) have been addressed in this revision. One new P1 remains: in the v2 P3005 recovery branch, _create_baseline_migration can raise subprocess.CalledProcessError, which propagates past the outer try/finally (no outer except in v2, unlike v1's loop-level handler) and crashes proxy startup with an unhandled exception instead of the clean sys.exit(2). litellm-proxy-extras/litellm_proxy_extras/utils.py — P3005 branch in _setup_database_v2 (around line 573–578)
|
| Filename | Overview |
|---|---|
| litellm-proxy-extras/litellm_proxy_extras/utils.py | Adds v2 migration resolver; _create_baseline_migration in the P3005 branch can escape as a raw CalledProcessError, bypassing proxy_cli's RuntimeError guard. |
| litellm-proxy-extras/tests/test_setup_database_fail_fast.py | New test file with 10 unit tests covering v2 paths; no test for P3005 baseline-creation failure leaking CalledProcessError. |
| litellm/proxy/proxy_cli.py | Adds --use_v2_migration_resolver flag and wraps setup_database in try/except RuntimeError + sys.exit(2); correctly backwards-compatible. |
| litellm/proxy/db/prisma_client.py | Threads use_v2_resolver through to ProxyExtrasDBManager.setup_database; minimal, correct change. |
| .circleci/config.yml | Adds separate CI job for v2 resolver with Postgres; excludes v2 test from existing Python version jobs to prevent DB collision; checksum-verified uv download. |
| tests/local_testing/test_basic_python_version.py | Refactors smoke test into a helper, adds v2 variant; clean, no regression risk. |
| tests/test_litellm/proxy/test_proxy_cli.py | Updates mock assertions to include use_v2_resolver=False; correctly pins default behavior. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[proxy_cli run_server] --> B{use_v2_migration_resolver?}
B -- No --> C[PrismaManager.setup_database\nuse_v2_resolver=False]
B -- Yes --> D[PrismaManager.setup_database\nuse_v2_resolver=True]
C --> E[ProxyExtrasDBManager.setup_database v1]
E --> F[prisma migrate deploy]
F -- success --> G[_resolve_all_migrations\npost-migration sanity check]
F -- P3005 --> H[_create_baseline_migration\nthen _resolve_all_migrations]
F -- P3009 idempotent --> I[_resolve_specific_migration\nthen _resolve_all_migrations]
D --> J[_setup_database_v2]
J --> K[_warn_if_db_ahead_of_head\ninfo only, never blocks]
K --> L[prisma migrate deploy loop x4]
L -- success --> M[return True]
L -- TimeoutExpired --> N[sleep + retry]
L -- P3005 --> O[_create_baseline_migration\n⚠️ CalledProcessError not wrapped!]
O --> P[continue loop]
L -- P3009 idempotent --> Q[_roll_back_migration\n_resolve_specific_migration\nwrapped as RuntimeError]
Q --> P
L -- P3018 permission --> R[raise RuntimeError]
L -- P3018 idempotent --> Q
L -- other --> R
D --> S{RuntimeError?}
S -- Yes --> T[print error + sys.exit 2]
S -- No --> U{setup_ok?}
U -- False + enforce check --> V[sys.exit 1]
U -- True --> W[startup continues]
Reviews (4): Last reviewed commit: "[Fix] v2 resolver: swallow non-connectio..." | Re-trigger Greptile
| except psycopg.errors.UndefinedTable: | ||
| return # fresh DB, table not yet created |
There was a problem hiding this comment.
return inside with psycopg.connect() leaves transaction in aborted state
In psycopg3 (unlike psycopg2), with conn: calls conn.commit() on a clean exit (__exit__ gets None for exc_type). After UndefinedTable is raised and caught, the server-side transaction is in an aborted state. When return fires, psycopg3's __exit__ sends COMMIT, which PostgreSQL rejects with ERROR: current transaction is aborted, raising psycopg.errors.InFailedSqlTransaction (DatabaseError). That is not caught by the outer except psycopg.OperationalError, so it propagates uncaught through setup_database and crashes fresh-DB startups with a confusing traceback rather than a clean first-run.
The fix is to roll back the aborted transaction before returning, or use autocommit=True:
except psycopg.errors.UndefinedTable:
conn.rollback() # clear aborted transaction before the context-manager commits
return # fresh DB, table not yet createdAlternatively, open the connection with autocommit=True:
with psycopg.connect(cleaned_url, connect_timeout=10, autocommit=True) as conn:| except psycopg.errors.UndefinedTable: | |
| return # fresh DB, table not yet created | |
| except psycopg.errors.UndefinedTable: | |
| conn.rollback() # clear aborted transaction before the context-manager commits | |
| return # fresh DB, table not yet created |
| raise RuntimeError( | ||
| "Database migration failed after 4 attempts (persistent timeouts). " | ||
| "Check database connectivity and load." | ||
| ) |
There was a problem hiding this comment.
Misleading "persistent timeouts" message after non-timeout loop exhaustion
The loop (for attempt in range(4)) is exhausted not only by TimeoutExpired but also by every P3005/P3009/P3018 idempotent recovery (continue). If four consecutive recovery attempts are made (e.g., P3005 baseline, then three successive P3009 idempotent retries), the function falls through to this message even though no timeout occurred, confusing operators trying to diagnose the failure.
| raise RuntimeError( | |
| "Database migration failed after 4 attempts (persistent timeouts). " | |
| "Check database connectivity and load." | |
| ) | |
| raise RuntimeError( | |
| "Database migration failed after 4 attempts (all attempts used). " | |
| "Check database connectivity and load." | |
| ) |
…olver) Default behavior (v1) is unchanged. Users who have seen schema thrashing during rolling deploys can opt into the v2 resolver with `--use_v2_migration_resolver`. Why v2 is safer: - Runs `prisma migrate deploy` only. - Recovers from P3005 (baseline) and idempotent P3009/P3018 errors, same as v1. - Never calls `_resolve_all_migrations`, which generates a schema diff between the live DB and the shipped schema.prisma and applies it via `prisma db execute`. That path bypassed every migration's SQL and was the root cause of thrashing when two LiteLLM versions contended for the same DB. - Logs a non-blocking warning when the DB has migrations applied that are newer than anything this build ships (ahead-of-HEAD). It does not refuse to start — many users have unusual ledger state from past thrashing, and blocking startup would be a breaking change. Also prints a message on startup when the default (v1) resolver is in use, pointing operators at the opt-in flag. Adds unit tests covering the v2 fail-fast paths, the stripping of Prisma-specific query params from DATABASE_URL (needed for psycopg), the timestamp helpers, and pins the default: v1 still invokes `_resolve_all_migrations`, v2 must not.
89e56a6 to
a16c00e
Compare
Adds end-to-end CI coverage for `--use_v2_migration_resolver` via a new job `installing_litellm_on_python_v2_migration_resolver`: - Clones the pytest smoke path from `installing_litellm_on_python` but uses a local Postgres sidecar instead of the shared DB to prevent collisions with the v1 variant. - Runs only the new `test_litellm_proxy_server_config_no_general_settings_v2_resolver` which spawns the proxy with `--use_v2_migration_resolver` and smoke-tests `/health/liveliness` and `/chat/completions`. Refactors `test_basic_python_version.py`: - Extracts the proxy spawn + smoke-test body into `_run_proxy_server_smoke_test` so the v1 and v2 tests share the same code path. - The existing `test_litellm_proxy_server_config_no_general_settings` is now a thin wrapper that passes no extra args (v1 default, unchanged). - Adds `..._v2_resolver` variant that passes `--use_v2_migration_resolver`. The existing `installing_litellm_on_python` / `installing_litellm_on_python_3_13` jobs filter out the v2 variant via `-k "not v2_resolver"` so they keep running only against their shared DB, unchanged behavior.
Existing tests pinned exact kwargs on `PrismaManager.setup_database`, but the opt-in v2 resolver added `use_v2_resolver=False` to every call. Update the three assertions to reflect the new signature. Fixes: - TestHealthAppFactory::test_use_prisma_db_push_flag_behavior - TestHealthAppFactory::test_startup_fails_when_db_setup_fails
|
@greptile re review please |
| if not use_migrate: | ||
| # Preserve `prisma db push` path unchanged. | ||
| original_dir = os.getcwd() | ||
| os.chdir(migrations_dir) | ||
| try: | ||
| subprocess.run( | ||
| [_get_prisma_command(), "db", "push", "--accept-data-loss"], | ||
| timeout=60, | ||
| check=True, | ||
| env=_get_prisma_env(), | ||
| ) | ||
| return True | ||
| finally: | ||
| os.chdir(original_dir) |
There was a problem hiding this comment.
db push path in v2 swallows CalledProcessError/TimeoutExpired silently
The db push branch in _setup_database_v2 wraps subprocess.run in a bare try/finally (directory restore only). If Prisma returns a non-zero exit code or the call times out, CalledProcessError / TimeoutExpired propagates straight through to proxy_cli.py, which only catches RuntimeError. The result is an unhandled traceback crash rather than a logged warning and a graceful False return.
The v1 path avoids this by catching both exceptions at the outer loop level (except subprocess.TimeoutExpired / except subprocess.CalledProcessError) and returning False after retries. The PR comment "Preserve prisma db push path unchanged" is therefore incorrect — the error-handling contract changed.
The fix is to either catch these exceptions in the db push branch and return False, or raise a RuntimeError (consistent with the rest of v2):
if not use_migrate:
original_dir = os.getcwd()
os.chdir(migrations_dir)
try:
subprocess.run(
[_get_prisma_command(), "db", "push", "--accept-data-loss"],
timeout=60,
check=True,
env=_get_prisma_env(),
)
return True
except (subprocess.CalledProcessError, subprocess.TimeoutExpired) as e:
raise RuntimeError(
f"prisma db push failed.\n\nDetail: {e}"
) from e
finally:
os.chdir(original_dir)
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
- Open the psycopg connection in `_warn_if_db_ahead_of_head` with autocommit=True. Without it, psycopg3's `with conn` calls COMMIT on clean exit, which fails after the `UndefinedTable` (fresh-DB) branch left the transaction in an aborted state — crashing first-run startups. - Wrap the v2 `prisma db push` path in try/except and raise RuntimeError on CalledProcessError/TimeoutExpired. Otherwise these propagate past proxy_cli.py's `except RuntimeError` as unhandled tracebacks. - Reword the loop-exhaustion error to cover the non-timeout exit path (repeated P3005/P3009/P3018 idempotent-recovery `continue`s), not just persistent timeouts. Adds a unit test for the db_push error wrapping.
| try: | ||
| # autocommit=True keeps the SELECT outside a transaction. Without | ||
| # it, psycopg3's `with conn` calls COMMIT on clean exit — which | ||
| # fails after `UndefinedTable` (fresh DB) leaves the transaction | ||
| # in an aborted state. | ||
| with psycopg.connect( | ||
| cleaned_url, connect_timeout=10, autocommit=True | ||
| ) as conn: | ||
| try: | ||
| rows = conn.execute( | ||
| "SELECT migration_name FROM _prisma_migrations " | ||
| "WHERE finished_at IS NOT NULL AND rolled_back_at IS NULL" | ||
| ).fetchall() | ||
| except psycopg.errors.UndefinedTable: | ||
| return | ||
| except psycopg.OperationalError: | ||
| return |
There was a problem hiding this comment.
_warn_if_db_ahead_of_head can crash startup on non-connection DB errors
The outer except psycopg.OperationalError only catches connection-level failures. Any psycopg.DatabaseError raised by conn.execute(...) that is not UndefinedTable (e.g., InsufficientPrivilege / error code 42501 if the runtime DB user lacks SELECT on _prisma_migrations) propagates uncaught, crashing proxy startup — directly contradicting the docstring's "Safe no-op if psycopg isn't installed or DB isn't reachable" guarantee.
Fix: widen the outer catch to psycopg.DatabaseError so all DB-layer errors are swallowed consistently with the intent:
except (psycopg.OperationalError, psycopg.DatabaseError):
return| try: | ||
| ProxyExtrasDBManager._roll_back_migration(name) | ||
| except ( | ||
| subprocess.CalledProcessError, | ||
| subprocess.TimeoutExpired, | ||
| ): | ||
| pass | ||
| ProxyExtrasDBManager._resolve_specific_migration(name) | ||
| continue |
There was a problem hiding this comment.
_resolve_specific_migration errors bypass proxy_cli.py's RuntimeError guard
In the P3009 recovery path (and the equivalent P3018 path at lines 624–626), _resolve_specific_migration(name) is called inside an active except subprocess.CalledProcessError as e: handler without its own try/except. If this call raises subprocess.CalledProcessError, Python does not re-enter the same except block — the new exception propagates as a raw CalledProcessError. proxy_cli.py only catches RuntimeError, so this crashes startup with an unhandled traceback instead of the intended graceful sys.exit(2).
Wrap the resolve call:
try:
ProxyExtrasDBManager._resolve_specific_migration(name)
except (subprocess.CalledProcessError, subprocess.TimeoutExpired) as resolve_err:
raise RuntimeError(
f"Failed to mark migration {name} as applied after idempotent recovery. "
f"Manual intervention may be required.\n\nDetail: {resolve_err}"
) from resolve_err
continue…lures Addresses two further Greptile findings: - `_warn_if_db_ahead_of_head` only caught `psycopg.OperationalError`. Non-connection DB errors (e.g. `InsufficientPrivilege` / 42501 if the runtime DB user lacks SELECT on `_prisma_migrations`) would propagate uncaught and crash startup — contradicting the docstring's "informational only, never blocks" guarantee. Widen the catch to `psycopg.DatabaseError` so all DB-layer errors are swallowed. - In the P3009 and P3018 idempotent-recovery paths, the call to `_resolve_specific_migration(name)` was not wrapped in its own try/except. Being inside an active `except CalledProcessError` handler, a new `CalledProcessError` from the resolve call would NOT re-enter the same handler — it would propagate out as `CalledProcessError`, past `proxy_cli.py`'s `except RuntimeError`, crashing startup with an unhandled traceback instead of the intended clean `sys.exit(2)`. Wrap both call sites to convert to RuntimeError. Adds unit tests for both behaviors.
| if "P3005" in stderr and "database schema is not empty" in stderr: | ||
| logger.info( | ||
| "Schema exists but no migrations ledger — creating baseline" | ||
| ) | ||
| ProxyExtrasDBManager._create_baseline_migration(schema_path) | ||
| continue |
There was a problem hiding this comment.
_create_baseline_migration CalledProcessError escapes RuntimeError guard
_create_baseline_migration (line 170 in the existing code) catches CalledProcessError, logs it, then re-raises it as a raw CalledProcessError. When that happens inside the inner except subprocess.CalledProcessError as e: handler here, Python propagates the new exception outward — past the outer try/finally (which only runs os.chdir), past setup_database, and up to proxy_cli.py. Because proxy_cli.py's handler only catches RuntimeError, the process crashes with an unhandled traceback instead of the clean sys.exit(2) the v2 resolver promises.
The v1 path avoids this because its structure has an outer except subprocess.CalledProcessError loop-level handler (line 932 of utils.py) that re-catches the escaping exception. v2 has no such outer handler — only the finally for os.chdir.
Fix: wrap the call (consistent with how _resolve_specific_migration is already wrapped):
if "P3005" in stderr and "database schema is not empty" in stderr:
logger.info(
"Schema exists but no migrations ledger — creating baseline"
)
try:
ProxyExtrasDBManager._create_baseline_migration(schema_path)
except (subprocess.CalledProcessError, subprocess.TimeoutExpired) as baseline_err:
raise RuntimeError(
"Failed to create baseline migration during P3005 recovery. "
f"Detail: {baseline_err}"
) from baseline_err
continue…hrashing [Feature] Proxy: opt-in v2 migration resolver
Relevant issues
Summary
Problem
ProxyExtrasDBManager._resolve_all_migrationsgenerates a Prisma schema diffbetween the live DB and the shipped
schema.prismaand applies itunconditionally via
prisma db execute. This bypasses every migration'scareful SQL (data backfills, multi-step transforms, safety checks). When two
LiteLLM versions contend for the same DB during a rolling deploy — or a
previous-version container restarts unexpectedly — each restart forces the
schema to its own version, producing a thrashing loop.
Fix
Add an opt-in v2 migration resolver. The existing (v1) behavior is
unchanged to avoid breaking users whose DBs have unusual ledger state from
prior thrashing.
Users who have seen thrashing during rolling deploys can opt in with
--use_v2_migration_resolver. The v2 resolver:prisma migrate deployonly.as v1.
_resolve_all_migrations(the diff-and-force recoverythat caused the thrash).
this build ships (ahead-of-HEAD). Does not refuse to start — users with
unusual ledger state from past thrashing can still upgrade.
Startup prints a message in default (v1) mode pointing at the opt-in flag
so operators who are affected know the option exists.
Testing
litellm-proxy-extras/tests/test_setup_database_fail_fast.pycovering the v2 fail-fast paths, URL query-param stripping (for psycopg),
the timestamp helpers, and pinning the default: v1 still invokes
_resolve_all_migrations, v2 must not.(
v1.79.3-stablethroughv1.83.7-stable) × 3 scenario types (cleanupgrade, thrashed
_prisma_migrationsstate, ahead-of-HEAD DBs). 840/840tests passed across a 10× stability gate against the fix.
Type
🆕 New Feature
✅ Test