-
-
Notifications
You must be signed in to change notification settings - Fork 6.7k
[Fix] Remove dangerous schema diff force-apply from Prisma migration startup #23464
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: litellm_internal_dev_03_12_2026
Are you sure you want to change the base?
Changes from all commits
d4e30a9
06b12ad
347090d
d41e5e2
c47bcec
3738ca5
b87dc64
27a10dc
be4ba42
fdd8a70
85e28c9
b4ee36c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| -- DropIndex | ||
| DROP INDEX "LiteLLM_MCPServerTable_approval_status_idx"; | ||
|
|
||
| -- AlterTable | ||
| ALTER TABLE "LiteLLM_MCPServerTable" DROP COLUMN "approval_status", | ||
| DROP COLUMN "review_notes", | ||
| DROP COLUMN "reviewed_at", | ||
| DROP COLUMN "source_url", | ||
| DROP COLUMN "submitted_at", | ||
| DROP COLUMN "submitted_by"; | ||
|
|
||
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,9 @@ | ||
| import os | ||
| import subprocess | ||
| import sys | ||
| from unittest.mock import MagicMock, patch | ||
|
|
||
| import pytest | ||
|
|
||
| sys.path.insert( | ||
| 0, | ||
|
|
@@ -82,10 +86,10 @@ def test_is_idempotent_error_column_already_exists(self): | |
| error_message = "column 'email' already exists" | ||
| assert ProxyExtrasDBManager._is_idempotent_error(error_message) is True | ||
|
|
||
| def test_is_idempotent_error_duplicate_key(self): | ||
| """Test detection of duplicate key violation error""" | ||
| def test_duplicate_key_violation_not_idempotent(self): | ||
| """Duplicate key violations (e.g., CREATE INDEX with duplicate data) are NOT idempotent""" | ||
| error_message = "duplicate key value violates unique constraint" | ||
| assert ProxyExtrasDBManager._is_idempotent_error(error_message) is True | ||
| assert ProxyExtrasDBManager._is_idempotent_error(error_message) is False | ||
|
|
||
| def test_is_idempotent_error_relation_already_exists(self): | ||
| """Test detection of 'relation already exists' error""" | ||
|
|
@@ -138,3 +142,203 @@ def test_unknown_error_classified_as_neither(self): | |
| error_message = "connection timeout" | ||
| assert ProxyExtrasDBManager._is_permission_error(error_message) is False | ||
| assert ProxyExtrasDBManager._is_idempotent_error(error_message) is False | ||
|
|
||
|
|
||
| class TestMarkAllMigrationsApplied: | ||
| """Test that _mark_all_migrations_applied only marks migrations without applying diffs""" | ||
|
|
||
| @patch("litellm_proxy_extras.utils.subprocess.run") | ||
| @patch.object( | ||
| ProxyExtrasDBManager, | ||
| "_get_migration_names", | ||
| return_value=["20250326162113_baseline", "20250329084805_new_cron_job_table"], | ||
| ) | ||
| def test_marks_each_migration_as_applied(self, mock_get_names, mock_run): | ||
| """Verify each migration is marked as applied via prisma migrate resolve""" | ||
| ProxyExtrasDBManager._mark_all_migrations_applied("/fake/migrations/dir") | ||
|
|
||
| assert mock_run.call_count == 2 | ||
| for call_args in mock_run.call_args_list: | ||
| cmd = call_args[0][0] | ||
| assert "migrate" in cmd | ||
| assert "resolve" in cmd | ||
| assert "--applied" in cmd | ||
|
|
||
| @patch("litellm_proxy_extras.utils.subprocess.run") | ||
| @patch.object( | ||
| ProxyExtrasDBManager, | ||
| "_get_migration_names", | ||
| return_value=["20250326162113_baseline"], | ||
| ) | ||
| def test_does_not_generate_or_apply_diffs(self, mock_get_names, mock_run): | ||
| """Verify no diff generation or db execute commands are run""" | ||
| ProxyExtrasDBManager._mark_all_migrations_applied("/fake/migrations/dir") | ||
|
|
||
| for call_args in mock_run.call_args_list: | ||
| cmd = call_args[0][0] | ||
| # Should never run diff or db execute | ||
| assert "diff" not in cmd | ||
| assert "execute" not in cmd | ||
| assert "push" not in cmd | ||
|
|
||
| @patch("litellm_proxy_extras.utils.subprocess.run") | ||
| @patch.object( | ||
| ProxyExtrasDBManager, | ||
| "_get_migration_names", | ||
| return_value=["20250326162113_baseline"], | ||
| ) | ||
| def test_skips_already_applied_migration(self, mock_get_names, mock_run): | ||
| """Verify already-applied migrations are silently skipped""" | ||
| mock_run.side_effect = subprocess.CalledProcessError( | ||
| 1, | ||
| "prisma", | ||
| stderr="Migration `20250326162113_baseline` is already recorded as applied in the database.", | ||
| ) | ||
|
Comment on lines
+188
to
+196
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tests assert exact Prisma command without mocking Multiple tests assert against a hardcoded assert cmd == ["prisma", "migrate", "deploy"]
The same pattern appears in @patch("litellm_proxy_extras.utils._get_prisma_command", return_value="prisma") |
||
| # Should not raise | ||
| ProxyExtrasDBManager._mark_all_migrations_applied("/fake/migrations/dir") | ||
|
|
||
| @patch("litellm_proxy_extras.utils.subprocess.run") | ||
| @patch.object( | ||
| ProxyExtrasDBManager, | ||
| "_get_migration_names", | ||
| return_value=["20250326162113_baseline"], | ||
| ) | ||
| def test_raises_on_unexpected_error(self, mock_get_names, mock_run): | ||
| """Verify non-'already applied' errors are propagated, not silently swallowed""" | ||
| mock_run.side_effect = subprocess.CalledProcessError( | ||
| 1, | ||
| "prisma", | ||
| stderr="connection refused", | ||
| ) | ||
| with pytest.raises(RuntimeError, match="Failed to mark migration"): | ||
| ProxyExtrasDBManager._mark_all_migrations_applied("/fake/migrations/dir") | ||
|
|
||
|
|
||
| class TestDeployWithIdempotentResolution: | ||
| """Test _deploy_with_idempotent_resolution loops through multiple idempotent failures""" | ||
|
|
||
| @patch("litellm_proxy_extras.utils.subprocess.run") | ||
| def test_resolves_multiple_idempotent_migrations_in_one_pass(self, mock_run): | ||
| """Multiple P3018 idempotent errors should all be resolved without consuming outer retries""" | ||
| p3018_error_1 = subprocess.CalledProcessError( | ||
| 1, | ||
| "prisma", | ||
| stderr="P3018\nMigration name: 20251113000000_add_project_table\nERROR: relation \"LiteLLM_ProjectTable\" already exists", | ||
| output="", | ||
| ) | ||
| p3018_error_2 = subprocess.CalledProcessError( | ||
| 1, | ||
| "prisma", | ||
| stderr="P3018\nMigration name: 20251113000001_add_project_fields\nERROR: column \"description\" already exists", | ||
| output="", | ||
| ) | ||
| # deploy fails, rollback, resolve, deploy fails again, rollback, resolve, deploy succeeds | ||
| mock_run.side_effect = [ | ||
| p3018_error_1, | ||
| MagicMock(returncode=0), # roll_back | ||
| MagicMock(returncode=0), # resolve | ||
| p3018_error_2, | ||
| MagicMock(returncode=0), # roll_back | ||
| MagicMock(returncode=0), # resolve | ||
| MagicMock(stdout="All migrations applied", returncode=0), # final deploy | ||
| ] | ||
|
|
||
| ProxyExtrasDBManager._deploy_with_idempotent_resolution() | ||
|
|
||
| assert mock_run.call_count == 7 | ||
| # First, fourth, and seventh calls should be deploy | ||
| for idx in [0, 3, 6]: | ||
| cmd = mock_run.call_args_list[idx][0][0] | ||
| assert cmd == ["prisma", "migrate", "deploy"] | ||
|
|
||
| @patch("litellm_proxy_extras.utils.subprocess.run") | ||
| def test_p3009_non_idempotent_raises_runtime_error(self, mock_run): | ||
| """P3009 with non-idempotent error should raise RuntimeError""" | ||
| deploy_error = subprocess.CalledProcessError( | ||
| 1, | ||
| "prisma", | ||
| stderr="P3009: migrate found failed migrations in the target database, `20250329084805_new_cron_job_table` migration. Error: syntax error at or near 'ALTR'", | ||
| output="", | ||
| ) | ||
| mock_run.side_effect = [deploy_error, MagicMock(returncode=0)] | ||
|
|
||
| with pytest.raises(RuntimeError, match="requires manual intervention"): | ||
| ProxyExtrasDBManager._deploy_with_idempotent_resolution() | ||
|
|
||
| # deploy + rollback | ||
| assert mock_run.call_count == 2 | ||
| rollback_cmd = mock_run.call_args_list[1][0][0] | ||
| assert "--rolled-back" in rollback_cmd | ||
|
|
||
| @patch("litellm_proxy_extras.utils.subprocess.run") | ||
| def test_p3009_unmatched_regex_raises_runtime_error(self, mock_run): | ||
| """P3009 with unparseable migration name should fail fast""" | ||
| error = subprocess.CalledProcessError( | ||
| 1, | ||
| "prisma", | ||
| stderr="P3009: migrate found failed migrations in the target database, unexpected format", | ||
| output="", | ||
| ) | ||
| mock_run.side_effect = error | ||
|
|
||
| with pytest.raises(RuntimeError, match="could not extract migration name"): | ||
| ProxyExtrasDBManager._deploy_with_idempotent_resolution() | ||
|
|
||
| assert mock_run.call_count == 1 | ||
|
|
||
| @patch("litellm_proxy_extras.utils.subprocess.run") | ||
| def test_p3009_idempotent_redeploys(self, mock_run): | ||
| """P3009 with idempotent error should resolve then re-deploy""" | ||
| deploy_error = subprocess.CalledProcessError( | ||
| 1, | ||
| "prisma", | ||
| stderr="P3009: migrate found failed migrations in the target database, `20250329084805_new_cron_job_table` migration. Error: column 'status' already exists", | ||
| output="", | ||
| ) | ||
| mock_run.side_effect = [ | ||
| deploy_error, | ||
| MagicMock(returncode=0), # roll_back | ||
| MagicMock(returncode=0), # resolve | ||
| MagicMock(stdout="All migrations applied", returncode=0), # re-deploy | ||
| ] | ||
|
|
||
| ProxyExtrasDBManager._deploy_with_idempotent_resolution() | ||
|
|
||
| assert mock_run.call_count == 4 | ||
| last_cmd = mock_run.call_args_list[3][0][0] | ||
| assert last_cmd == ["prisma", "migrate", "deploy"] | ||
|
|
||
| @patch("litellm_proxy_extras.utils.subprocess.run") | ||
| def test_p3018_permission_error_raises(self, mock_run): | ||
| """P3018 with permission error should raise RuntimeError""" | ||
| deploy_error = subprocess.CalledProcessError( | ||
| 1, | ||
| "prisma", | ||
| stderr="P3018\nMigration name: 20251113000000_add_project_table\nDatabase error code: 42501\npermission denied for table users", | ||
| output="", | ||
| ) | ||
| mock_run.side_effect = [deploy_error, MagicMock(returncode=0)] | ||
|
|
||
| with pytest.raises(RuntimeError, match="permission error"): | ||
| ProxyExtrasDBManager._deploy_with_idempotent_resolution() | ||
|
|
||
|
|
||
| class TestSetupDatabase: | ||
| """Test setup_database integration with deploy and resolution""" | ||
|
|
||
| @patch("litellm_proxy_extras.utils.os.chdir") | ||
| @patch("litellm_proxy_extras.utils.os.getcwd", return_value="/original") | ||
| @patch.object( | ||
| ProxyExtrasDBManager, "_get_prisma_dir", return_value="/fake/prisma/dir" | ||
| ) | ||
| @patch("litellm_proxy_extras.utils.subprocess.run") | ||
| def test_successful_deploy(self, mock_run, mock_dir, mock_getcwd, mock_chdir): | ||
| """After successful prisma migrate deploy, no diff/resolve should be called""" | ||
| mock_run.return_value = MagicMock(stdout="All migrations applied", returncode=0) | ||
|
|
||
| result = ProxyExtrasDBManager.setup_database(use_migrate=True) | ||
|
|
||
| assert result is True | ||
| assert mock_run.call_count == 1 | ||
| cmd = mock_run.call_args[0][0] | ||
| assert cmd == ["prisma", "migrate", "deploy"] | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Destructive column removal is not safe for rolling deployments
This migration permanently drops
approval_status,review_notes,reviewed_at,source_url,submitted_at, andsubmitted_byfromLiteLLM_MCPServerTable. During a rolling deployment, pods still running the previous version will attempt to read or write these columns and receive acolumn does not existdatabase error the moment the new pod applies this migration.The PR is specifically addressing rolling-deployment safety, so this bundled schema change works against that goal. The standard zero-downtime pattern is: (1) deploy new code that no longer references the columns, then (2) in a subsequent deploy, run the
DROP COLUMNmigration once all pods have moved to the new code. If these columns are already unreferenced by application code in the current branch, please confirm that before merging, and consider documenting that a brief read-path error window is expected.