fix(registry): preset/model-endpoint mutation integrity (rename, default, delete) - #588
Conversation
…point mutations #5 Preset rename now migrates partition references atomically: PgPresetRepository.rename inserts the new-named row, repoints every partition that referenced the old name (partitions are a plain string ref with no FK), then drops the old row in one transaction. PresetService.update_preset rejects renaming onto an existing name. Previously a rename orphaned partitions on a preset name that no longer existed. #6 ModelEndpointService.update_model_endpoint evicts the 'default' alias client cache when the updated endpoint is the current default, so callers that resolved 'default' don't keep a stale client (mirrors set_default). #7 delete_model_endpoint promotes a surviving endpoint to default when the current default is deleted, so the 'default' alias keeps resolving instead of disappearing (load_all only adds it for an is_default row) and a later factory("default") raising. Adds unit tests for all three plus a real-Postgres check of the rename migration.
The 'default' preset is the system fallback: partitions.{indexation,retrieval}_preset
default to that literal name (schema server_default + create_partition), and
resolve_partition_row raises ConfigError if it is missing. Renaming it away or
deleting it would orphan that fallback for every new/unset partition (seed_defaults
only re-seeds when a type has zero rows, so it never comes back). Reject both with a
422; editing its config stays allowed. Sibling of the model-endpoint default guards.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughModel endpoint deletion now promotes a surviving default through the repository and keeps default cache invalidation aligned with reloads. Preset handling now reserves the ChangesModel endpoint default alias protection
Reserved default preset protection and atomic rename
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@codex review |
- Preset rename now uses a plain UPDATE instead of an upsert, so the (name, preset_type) unique constraint rejects a collision with an existing target name (no silent overwrite, even on a direct call or check-then-act race), and the row keeps its identity/created_at. - delete_model_endpoint deletes the endpoint and promotes a survivor to default in a single repository transaction, so a mid-operation failure can never leave a model type with no default endpoint.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openrag/services/orchestrators/model_endpoint_service.py`:
- Around line 271-281: The default-survivor choice in model_endpoint_service.py
is still made from a stale read before the repository transaction, so concurrent
deletes can leave the type with no default. Move the survivor selection logic
out of the caller’s existing branch around existing.is_default and into
_repo.delete_and_promote_default so the repo picks the replacement (or detects
stale state and fails) inside the same transaction that performs the delete. Use
the existing symbols delete_and_promote_default, existing.is_default, and
all_of_type/model_type to locate the flow and ensure the transaction owns both
the delete and promotion decision.
In `@openrag/services/persistence/preset_repo.py`:
- Around line 91-104: In preset_repo.py, the rename flow in the method that runs
the pipeline_presets UPDATE and then repoints partitions should stop immediately
when conn.fetchrow returns None. Add a guard after the UPDATE on
pipeline_presets and before the partitions UPDATE so _row_to_dict(rec) is never
reached for a missing row, causing the transaction to abort/rollback instead of
committing stale partition references. Refer to the rename logic around rec,
conn.fetchrow, and the subsequent conn.execute on partitions when applying the
fix.
In `@tests/unit/services/orchestrators/test_model_endpoint_service.py`:
- Around line 87-93: The fake delete_and_promote_default helper is rewriting
is_default across all stored rows instead of only the requested model type.
Update the logic in delete_and_promote_default so that the promotion loop only
touches rows whose model_type matches the model_type argument, keeping unrelated
defaults unchanged and aligned with the production contract.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2027c953-bc6f-47a9-a0aa-a49e3449d026
📒 Files selected for processing (8)
openrag/core/ports/model_endpoint_repo.pyopenrag/services/orchestrators/model_endpoint_service.pyopenrag/services/orchestrators/preset_service.pyopenrag/services/persistence/model_endpoint_repo.pyopenrag/services/persistence/preset_repo.pytests/unit/services/orchestrators/test_model_endpoint_service.pytests/unit/services/orchestrators/test_preset_service.pytests/unit/services/persistence/test_preset_repo.py
… rename race - delete_model_endpoint: the last-endpoint guard and survivor/default choice now happen inside the repo transaction, under SELECT ... FOR UPDATE on the model type. Concurrent deletes of the same type serialize, so they can't both pass a stale count check (removing the last row) or promote an already-deleted survivor — which would leave the type with no endpoint / no default. The repo returns (status, promoted) and the service maps it to 404/422 + cache eviction. - preset rename: if old_name vanishes between the service's existence check and the UPDATE (concurrent delete), raise NotFoundError instead of crashing in _row_to_dict(None).
The update path could create two defaults for one model_type. `is_default`
was in `_ALLOWED_UPDATE_FIELDS`, so `PUT /model-endpoints/{type}/{name}`
with `{"is_default": true}` ran a bare `UPDATE ... SET is_default = true`
that never cleared the previous default. `load_all()` then resolved the
`'default'` alias to whichever endpoint sorted last by name — not the one
the caller picked — and the new eviction (gated on `existing.is_default`)
skipped the stale `'default'` client cache.
Reproduced against the live admin API: one PUT of `{"is_default": true}`
on a non-default endpoint left two `is_default=true` rows for the type.
Fix: drop `is_default` from the repo's bare-update allowlist, and route a
truthy `is_default` in `update_model_endpoint` through `set_default`
(atomic clear-then-set, evicts the `'default'` alias). A false/None value
is a no-op — the default is switched by promoting another endpoint, never
by leaving the type with none. Adds unit coverage plus an HTTP-level
integration regression test asserting a single default after the update.
| async with self.pool.acquire() as conn: | ||
| async with conn.transaction(): | ||
| rec = await conn.fetchrow( | ||
| "UPDATE pipeline_presets SET name = $2, config = $3::jsonb, updated_at = now() " | ||
| "WHERE name = $1 AND preset_type = $4 RETURNING *", | ||
| old_name, | ||
| new_name, | ||
| config, | ||
| preset_type, | ||
| ) |
There was a problem hiding this comment.
The rename looks much safer now, but I think there's still one small consistency edge case.
This transaction updates existing partition references from old_name to new_name, but partition preset references are stored as plain strings. If another request creates or updates a partition while the rename is in progress, it could still save old_name, leaving behind a stale reference.
Would it make sense to serialize preset renames with partition writes, for example by using a shared advisory lock or another locking mechanism?
There was a problem hiding this comment.
Valid edge — a partition write committing old_name concurrently with the rename can still leave a stale reference. The robust fix (a shared advisory lock spanning preset rename AND the partition write path) is cross-cutting and touches the partition-write hot path, so we're deferring it to a focused follow-up after the pre-release rather than expanding this PR. Leaving this thread open to track it.
…delete/promote - Reorder cache eviction to run AFTER load_all() in update/delete/set_default, so a concurrent request that rebuilds a client during the reload window can't leave a stale client cached. - Add PgModelEndpointRepository tests for delete_and_promote_default (missing, last, non-default, default-promotes-survivor under FOR UPDATE).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/unit/services/persistence/test_model_endpoint_repo.py`:
- Around line 257-287: The tests around
PgModelEndpointRepository.delete_and_promote_default are only asserting the
promotion update and miss the non-promotion reset behavior. Tighten both
test_delete_and_promote_non_default_deletes_no_promotion and
test_delete_and_promote_default_promotes_survivor_under_lock by checking the
executed SQL for the expected is_default = false reset in the non-default delete
path and for both the reset and the is_default = true promotion in the
default-delete path, so the repository contract is fully covered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8c5f47b5-fd5a-47d7-8114-67bee4fce65f
📒 Files selected for processing (2)
openrag/services/orchestrators/model_endpoint_service.pytests/unit/services/persistence/test_model_endpoint_repo.py
🚧 Files skipped from review as they are similar to previous changes (1)
- openrag/services/orchestrators/model_endpoint_service.py
Tighten delete_and_promote_default coverage: the non-default delete path must emit no is_default UPDATE, and the default-delete path must run both the is_default=false reset and the is_default=true promotion.
|
I think one default-endpoint path is still worth closing or tracking explicitly. The PR fixes update/delete integrity, but create and set-default can still affect the same invariant: for each model type, we should have one valid default endpoint. A create request marked as default can still add another default without demoting the current one, and set-default checks the target before the transaction that clears/promotes defaults. If the target disappears in that gap, the model type can end up with no default. I know this may pre-exist, but because this PR is about registry integrity, it is easy to read it as covering the whole invariant. Could we either route these paths through the same transactional default-promotion logic, or explicitly track them as follow-up items separate from the preset rename / partition-write race? |
set_default cleared the old default then set the new one with no FOR UPDATE and no existence check, so a delete of the target between the caller's existence check and this transaction made the second UPDATE match 0 rows AFTER the first cleared the previous default — leaving the model type with no default at all (the 'default' alias then fails to resolve). This is the same invariant delete_and_promote_default already protects. set_default now SELECTs the type's rows FOR UPDATE, raises NotFoundError if the target is gone, then clears-then-sets inside the transaction, so a concurrent delete serializes and can never leave the type defaultless. Adds repo tests: lock + two updates on success; NotFoundError without clearing when the target is missing.
| @@ -119,8 +128,25 @@ async def delete(self, name: str, model_type: str) -> bool: | |||
| return result == "DELETE 1" | |||
|
|
|||
| async def set_default(self, model_type: str, name: str) -> None: | |||
There was a problem hiding this comment.
Bug (found in review, fixed here): set_default can leave a model type with NO default under a concurrent delete
Same "never leave a type defaultless" invariant that delete_and_promote_default was just hardened for — but set_default was left with two bare UPDATEs, no FOR UPDATE and no existence check:
UPDATE … SET is_default=false WHERE model_type=$1 -- clears the old default
UPDATE … SET is_default=true WHERE name=$1 … -- 0 rows if name was just deleted
Concretely — type llm with primary (default) + backup; two concurrent admin requests, POST …/backup/set-default and DELETE …/backup:
- set-default: service
get("backup")→ exists ✓ - delete:
delete_and_promote_default("backup")removes it (non-default, not last) and commits → onlyprimaryleft, still default - set-default txn:
UPDATE … is_default=false WHERE model_type='llm'→ clearsprimary - set-default txn:
UPDATE … is_default=true WHERE name='backup'→ 0 rows (gone) - commit →
llmhas a healthy endpoint but no default;load_all()registers no"default"alias, so the default LLM stops resolving until an admin re-runs set-default.
Low probability (admin-only path, ms-scale window) but high, silent impact.
Fix: set_default now SELECT … FOR UPDATEs the type's rows, raises NotFoundError if the target is gone, then clears-then-sets inside the transaction — so a concurrent delete serializes (either set-default wins, or it sees backup gone and 404s) and the type is never left defaultless. Mirrors delete_and_promote_default.
There was a problem hiding this comment.
Fixed in commit 7e693c1.
set_defaultnow selects the type's rowsFOR UPDATE, raisesNotFoundErrorif the target endpoint is absent, then clears-then-sets inside the same transaction.- Repo tests:
test_set_default_locks_rows_then_runs_two_updates(assertsFOR UPDATE+ bothis_defaultupdates) andtest_set_default_raises_not_found_without_clearing_when_target_missing(asserts it aborts before clearing the old default).
Validated locally: ruff clean, 44 unit tests pass (repo + service). Note this also covers the update_model_endpoint(is_default=true) path, which routes through set_default.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openrag/services/persistence/model_endpoint_repo.py (1)
142-159: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPrevent a second default from slipping in concurrently.
set_default()/delete_and_promote_default()only lock existing rows, socreate_model_endpoint()can still insert anotheris_default=truerow for the samemodel_type. There’s no partial unique constraint onmodel_endpointsto enforce the single-default invariant. Add a(model_type) WHERE is_defaultconstraint or route default creation through the same promotion path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openrag/services/persistence/model_endpoint_repo.py` around lines 142 - 159, `set_default()` and `delete_and_promote_default()` in `ModelEndpointRepo` only lock currently existing rows, so `create_model_endpoint()` can still insert a second `is_default=true` endpoint for the same `model_type`. Fix this by enforcing the single-default invariant at the database level with a partial unique constraint on `model_endpoints` for `(model_type) WHERE is_default`, or by making `create_model_endpoint()` reuse the same default-promotion flow used by the existing default-update methods. Ensure the change is applied consistently across the repo methods that create or promote defaults.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@openrag/services/persistence/model_endpoint_repo.py`:
- Around line 142-159: `set_default()` and `delete_and_promote_default()` in
`ModelEndpointRepo` only lock currently existing rows, so
`create_model_endpoint()` can still insert a second `is_default=true` endpoint
for the same `model_type`. Fix this by enforcing the single-default invariant at
the database level with a partial unique constraint on `model_endpoints` for
`(model_type) WHERE is_default`, or by making `create_model_endpoint()` reuse
the same default-promotion flow used by the existing default-update methods.
Ensure the change is applied consistently across the repo methods that create or
promote defaults.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b49f4182-ff8c-4c40-b64a-7d4cddba7ef3
📒 Files selected for processing (2)
openrag/services/persistence/model_endpoint_repo.pytests/unit/services/persistence/test_model_endpoint_repo.py
Closes the create-side half of Hedi's review: "A create request marked as
default can still add another default without demoting the current one." (The
set-default no-default race from the same comment is fixed in the preceding
commit, which hardens set_default under a row lock.)
repo.create ran a bare INSERT with is_default straight through, so
POST /model-endpoints/ {"is_default": true} added a second is_default=true row
when one already existed — the same hazard the update path already routes through
set_default, and confirmed against the live admin API. create now demotes any
existing default in the SAME transaction as the insert, so the new endpoint
becomes the sole default. ModelEndpointService.create_model_endpoint evicts the
stale 'default' client-cache alias when the created endpoint is the default.
Tests: unit coverage that a default-marked create clears the prior default before
inserting; HTTP integration regressions asserting a single default after a
default-marked create, and that a set-default on a missing target is a clean 404
that leaves exactly one default standing (never zero).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/integration/api/test_model_endpoints.py`:
- Around line 170-172: The cleanup in the test’s finally block ignores whether
restoring the original default LLM succeeded, which can leave later tests with
the wrong default state. In the test around the api_client.post call to
“/model-endpoints/llm/{original_default_llm}/set-default”, check the response
and fail loudly if it is not successful before proceeding to
_delete_ignore_errors for the temporary endpoint. Use the existing test flow and
symbols api_client, original_default_llm, and _delete_ignore_errors so the
temporary endpoint is only removed after a successful restore.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 182e7c6d-b2d3-4ee3-985f-f704efe1670e
📒 Files selected for processing (4)
openrag/services/orchestrators/model_endpoint_service.pyopenrag/services/persistence/model_endpoint_repo.pytests/integration/api/test_model_endpoints.pytests/unit/services/persistence/test_model_endpoint_repo.py
🚧 Files skipped from review as they are similar to previous changes (2)
- openrag/services/persistence/model_endpoint_repo.py
- openrag/services/orchestrators/model_endpoint_service.py
The create-default test's finally block ignored the /set-default restore result, so a failed restore would still delete the temp endpoint and could leave the integration DB with the wrong default for later tests. Assert the restore succeeds before removing the temporary endpoint.
4 participants
Backend integrity fixes surfaced while reviewing the admin UI: several admin registry mutations did not maintain referential / default-alias integrity. Each is server-side and independent of the UI.
Fixes
Preset rename orphaned partition references. Partitions reference a preset by its plain name string (
partitions.indexation_preset/retrieval_preset, no FK). A rename didDELETE old + INSERT new, leaving every referencing partition pointing at a name that no longer existed.PgPresetRepository.renamenow migrates references atomically (insert new → repoint partitions → drop old, one transaction), and the service rejects renaming onto an existing name.Reserved
defaultpreset. Thedefaultpreset is the system fallback (schemaserver_default,create_partition, andresolve_partition_rowall key off the literal name;seed_defaultsonly re-seeds when a type has zero rows). Renaming it away or deleting it would dangle that fallback for every new/unset partition. Both are now rejected with 422; editing its config stays allowed.Stale
defaultmodel-endpoint client on update. Updating the current default endpoint only evicted the named cache entry, not thedefaultalias — callers that resolveddefaultkept a stale client. It now evictsdefaulttoo (mirrorsset_default).Lost
defaultmodel-endpoint alias on delete. Deleting the current default promoted no replacement, soload_all()dropped thedefaultalias entirely and a laterfactory("default")raisedKeyError. Delete now promotes a surviving endpoint to default and evicts the staledefaultclient.Tests / verification
default-referencing partitions are untouched, and the old/new preset rows swap as expected.Summary by CodeRabbit
defaultpreset and made preset renames update references atomically.