fix: harden completion model migration tenant safety and error handling#240
Open
fix: harden completion model migration tenant safety and error handling#240
Conversation
…andling
- Fix critical tenant filter bug: singular entity names ("app", "assistant")
never matched plural keys from ENTITY_TABLE_MAP, causing unscoped updates
- Fix transaction conflict in migrate-all-tenants: remove nested begin() that
conflicted with already-begun session
- Add per-tenant model resolution for all-tenants migration using (name, family)
canonical matching instead of passing same UUID to all tenants
- Fix exception mapping: preserve HTTPException status codes instead of wrapping
all errors as 500
- Allow disabled/deprecated source model in migration (only target must be enabled)
- Make kwargs reset warning informational instead of always forcing confirm_migration
- Reset completion_model_kwargs for all entity types (apps, services, templates),
not just assistants
- Scope template migration to tenant (templates have tenant_id column)
- Add deterministic ordering to per-tenant model resolution
Contributor
Author
|
Follow-up: Test coverage needed There are currently no tests targeting the migration endpoints, particularly:
We should add unit/integration tests to confirm the fixes in this PR:
|
Contributor
Author
|
Fixes #241 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
"app","assistant") butENTITY_TABLE_MAPuses plural keys ("apps","assistants"), causing the filter to fall throughto an unscoped
return True— meaning no tenant filtering was applied on any migrationmigrate-all-tenantsendpoint: removedsession.begin()that conflictedwith the already-begun request-scoped session, causing "A transaction is already begun" for all tenants
migrate-all-tenants: resolves source/target models by(name, family)per tenant instead of passing the same UUID to all tenants (which fails becausemodels are tenant-scoped with different UUIDs)
HTTPExceptionandValidationExceptionare no longer swallowed by catch-all and re-raised as 500
confirm_migration=trueon every migrationcompletion_model_kwargsto{}for all entity types that store them (apps, services,assistant_templates, app_templates), not just assistants
tenant_idinstead of unscoped (return True)is_enabled DESC, updated_at DESC) to per-tenant model resolutionWhy
The model migration endpoints had several bugs discovered during a production migration
from Claude Sonnet 3.7 to Claude Haiku 4.5:
migrate-all-tenantsendpoint failed for all tenants with "A transaction is already begun on this Session"confirm_migration=truebecause an informational kwargs warning was treated as a blocking issuecompletion_model_kwargsreset — apps, services, and templates kept stale params