fix(models): reject '/' in model-endpoint names - #769
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughModel endpoint names now use aligned backend and frontend allowlists. Endpoint renames cascade references, update aliases, evict caches, and reload dependent services. Retrieval reranker selection falls back from partition presets to the catalog default and then the legacy reranker. ChangesModel endpoint validation
Model endpoint rename consistency
Retrieval reranker fallback
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ModelEndpointService
participant PgModelEndpointRepository
participant PresetService
participant PartitionService
ModelEndpointService->>PgModelEndpointRepository: Rename endpoint and cascade references
PgModelEndpointRepository-->>ModelEndpointService: Return updated row
ModelEndpointService->>ModelEndpointService: Alias old and new names from updated row
ModelEndpointService->>ModelEndpointService: Evict old and new client caches
ModelEndpointService->>PresetService: Reload presets
ModelEndpointService->>PartitionService: Reload partitions
sequenceDiagram
participant RetrievalService
participant RerankerFactory
participant LegacyReranker
RetrievalService->>RerankerFactory: Resolve partition preset
RerankerFactory-->>RetrievalService: Return preset or report missing
RetrievalService->>RerankerFactory: Resolve catalog default
RerankerFactory-->>RetrievalService: Return default or report missing
RetrievalService->>LegacyReranker: Use legacy fallback
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
3 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
openrag/services/orchestrators/retrieval_service.py (1)
193-198: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBind warning context instead of passing formatting kwargs.
rerankerandpartitionare not attached as structured Loguru fields here. Bind them before emitting the warning.Proposed fix
- logger.warning( + logger.bind(reranker=reranker_name, partition=partition).warning( "Partition reranker preset not found in the model-endpoint catalog — " - "falling back to the default reranker", - reranker=reranker_name, - partition=partition, + "falling back to the default reranker" )As per coding guidelines, use Loguru structured logging through
get_logger(), binding relevant context such asfile_idandpartitionwhere applicable.🤖 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/orchestrators/retrieval_service.py` around lines 193 - 198, Update the warning in the retrieval-service partition reranker fallback to bind reranker_name and partition as Loguru context before logging, rather than passing them as formatting keyword arguments. Use the logger returned by get_logger() and preserve the existing warning message and fallback behavior.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@openrag/services/orchestrators/retrieval_service.py`:
- Around line 193-198: Update the warning in the retrieval-service partition
reranker fallback to bind reranker_name and partition as Loguru context before
logging, rather than passing them as formatting keyword arguments. Use the
logger returned by get_logger() and preserve the existing warning message and
fallback behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 01f2abb7-f623-4904-a4bb-fdb19c8e578b
📒 Files selected for processing (2)
openrag/services/orchestrators/retrieval_service.pytests/unit/services/orchestrators/test_retrieval_service.py
3 tasks
hedhoud
reviewed
Jul 29, 2026
Ahmath-Gadji
added a commit
that referenced
this pull request
Jul 30, 2026
The DB rename (and its partitions/presets cascade, #770) commits before PresetService.load_all() / PartitionService.load_partitions() run, and both are awaited — so a concurrent request can land in that window and resolve a partition/preset already repointed at new_name against a registry that (until the final load_all() call) still only answers to the old name, raising a bare KeyError. A failed reload made it worse: the rename stayed committed but the registry never got the new name at all, wedging every subsequent request until process restart. Alias new_name to the (unchanged) config synchronously right after the rename commits, before either reload call gets a chance to yield control. A pure rename doesn't touch endpoint/model_name/extra, so both names stay correctly resolvable throughout the transition; the next load_all() drops the old one on its own once the DB is the only source of truth again. Addresses review feedback from @hedhoud on #769.
Collaborator
Author
|
Addressed the two review items:
Full unit suite (2223 tests) and ruff both green locally after the changes. @coderabbitai review |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
openrag/services/orchestrators/model_endpoint_service.py (1)
581-583: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAnnotation doesn't admit
None.
getattr(..., None)can returnNone, which the very next line handles — but the declared type isdict[str, Any], so a type checker will flag the assignment.♻️ Tighten the annotation
- bucket: dict[str, Any] = getattr(self._config.models, model_type, None) + bucket: dict[str, Any] | None = getattr(self._config.models, model_type, None)🤖 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/orchestrators/model_endpoint_service.py` around lines 581 - 583, Update the bucket annotation in the model lookup flow to allow a None value returned by getattr, such as using an optional dictionary type. Keep the existing None check and return behavior unchanged.
🤖 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.
Nitpick comments:
In `@openrag/services/orchestrators/model_endpoint_service.py`:
- Around line 581-583: Update the bucket annotation in the model lookup flow to
allow a None value returned by getattr, such as using an optional dictionary
type. Keep the existing None check and return behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0b716366-9ca3-492b-be33-f8db33a21ac7
📒 Files selected for processing (6)
openrag/di/container.pyopenrag/services/orchestrators/model_endpoint_service.pyopenrag/services/orchestrators/retrieval_service.pyopenrag/services/persistence/model_endpoint_repo.pytests/unit/services/orchestrators/test_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/retrieval_service.py
Collaborator
Author
|
Fixed the annotation nitpick in e09d484. @coderabbitai review |
|
✅ Action performedReview finished.
|
hedhoud
reviewed
Jul 30, 2026
hedhoud
reviewed
Jul 30, 2026
Ahmath-Gadji
added a commit
that referenced
this pull request
Jul 30, 2026
Rejecting '/' alone (#768) missed that the exact values '.' and '..' are RFC 3986 dot-segments: browsers/HTTP clients normalize them out of the URL before the request is even sent, resolving to the collection route or dropping the model_type segment entirely — the same "row exists but every single-endpoint route 404s" failure mode as the slash case, just via a different mechanism. Denylisting each unsafe value as it's discovered doesn't close the class. Replace both checks with one allowlist: name must start and end with an alphanumeric character, with '.', '_', '-' allowed in between. That rules out '/', '.', '..', and any leading/trailing separator by construction, while still accepting realistic names like 'gpt-4.1' or 'jina_v3'. Also caps the name at 128 characters — the DB column has no length bound today. Mirrored in the admin UI's client-side guard, same rationale as before: FastAPI's validation `detail` is a list, so ApiError can't surface a readable message for a raw 422. Addresses review feedback from @hedhoud on #769.
Ahmath-Gadji
added a commit
that referenced
this pull request
Jul 30, 2026
_alias_renamed_name copied whatever the in-memory registry held under old_name before this call ran. A rename can land in the same request as a field update (e.g. a new endpoint URL), applied to the DB before the alias is built — so the alias silently pinned both names to the pre-update config. If a reload then failed, the final load_all() that would have corrected it never ran, leaving the registry serving the old endpoint under the DB-authoritative new name until process restart. Build the aliased config from the row this call just wrote (the update() result, or the pre-fetched row when no fields changed) instead of reading the bucket. Both old_name and new_name now always alias to the same, correct, up-to-date config. Addresses further review feedback from @hedhoud on #769.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openrag/services/orchestrators/model_endpoint_service.py (1)
451-455: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winEvict renamed client aliases before awaiting reloads.
A cached
old_nameclient can retain the pre-update endpoint. If either awaited reload raises, the evictions at Line 468 never run, so requests through the intentionally preservedold_namealias continue using stale settings. Evict both names immediately after aliasing, while retaining the post-reload eviction to close the normal reload race; add this case with a populated cache and failing reload.Proposed fix
renamed_from = name self._alias_renamed_name(model_type, name, new_name, updated or existing) + self._invalidate_client_cache(model_type, name) + self._invalidate_client_cache(model_type, new_name) if self._preset_service is not None: await self._preset_service.load_all()🤖 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/orchestrators/model_endpoint_service.py` around lines 451 - 455, The renamed client aliases are evicted too late, allowing stale cached clients when a reload fails. In the rename flow around _alias_renamed_name, evict both the original name and new_name immediately after aliasing and before either awaited reload; retain the existing post-reload eviction to cover the normal race, and add a test with populated cache and a failing reload.
🧹 Nitpick comments (1)
tests/unit/api/schemas/admin/test_phase14_schemas.py (1)
70-78: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep create and update invalid-name coverage in sync.
The create matrix covers whitespace, percent-encoded text, and the maximum-length boundary, but the update-schema test omits those cases. Since renames use a separate optional-name validator, a regression there could go undetected. Reuse the full invalid-name matrix or add equivalent boundary cases.
🤖 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 `@tests/unit/api/schemas/admin/test_phase14_schemas.py` around lines 70 - 78, The update-schema invalid-name test test_update_model_endpoint_rejects_unsafe_name should cover the same invalid-name matrix as the create-schema test, including whitespace, percent-encoded values, and maximum-length boundary cases. Reuse the existing create matrix where possible while preserving the UpdateModelEndpointRequest validation assertion.
🤖 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 `@ui/src/pages/admin/models.tsx`:
- Around line 327-334: Update the model name handling around the nameError
validation to compute name.trim() once and validate the normalized value for
length and pattern checks. Reuse that normalized name in create/update payloads
and rename comparisons, while preserving the existing empty-name behavior and
error messages.
---
Outside diff comments:
In `@openrag/services/orchestrators/model_endpoint_service.py`:
- Around line 451-455: The renamed client aliases are evicted too late, allowing
stale cached clients when a reload fails. In the rename flow around
_alias_renamed_name, evict both the original name and new_name immediately after
aliasing and before either awaited reload; retain the existing post-reload
eviction to cover the normal race, and add a test with populated cache and a
failing reload.
---
Nitpick comments:
In `@tests/unit/api/schemas/admin/test_phase14_schemas.py`:
- Around line 70-78: The update-schema invalid-name test
test_update_model_endpoint_rejects_unsafe_name should cover the same
invalid-name matrix as the create-schema test, including whitespace,
percent-encoded values, and maximum-length boundary cases. Reuse the existing
create matrix where possible while preserving the UpdateModelEndpointRequest
validation assertion.
🪄 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 Plus
Run ID: 383ff37b-a35b-4549-8f40-4fd770b39f7b
📒 Files selected for processing (5)
openrag/api/schemas/admin/model_endpoint_schemas.pyopenrag/services/orchestrators/model_endpoint_service.pytests/unit/api/schemas/admin/test_phase14_schemas.pytests/unit/services/orchestrators/test_model_endpoint_service.pyui/src/pages/admin/models.tsx
Collaborator
Author
|
Addressed CodeRabbit's latest pass:
@coderabbitai review |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/api/schemas/admin/test_phase14_schemas.py (1)
49-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd underscore boundary cases to the shared matrix.
The allowlist rejects every leading/trailing separator, but
_UNSAFE_NAMEScurrently covers only-and.at the boundaries. Add_leading_underscoreandtrailing_underscore_to protect the full validation contract.🤖 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 `@tests/unit/api/schemas/admin/test_phase14_schemas.py` around lines 49 - 60, Add the leading-underscore case "_leading_underscore" and trailing-underscore case "trailing_underscore_" to the shared _UNSAFE_NAMES test matrix, preserving the existing boundary validation coverage for dash and dot separators.
🤖 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.
Nitpick comments:
In `@tests/unit/api/schemas/admin/test_phase14_schemas.py`:
- Around line 49-60: Add the leading-underscore case "_leading_underscore" and
trailing-underscore case "trailing_underscore_" to the shared _UNSAFE_NAMES test
matrix, preserving the existing boundary validation coverage for dash and dot
separators.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e877ecf9-d17a-43a8-9faf-f901667a0802
📒 Files selected for processing (5)
openrag/api/schemas/admin/model_endpoint_schemas.pyopenrag/services/orchestrators/model_endpoint_service.pytests/unit/api/schemas/admin/test_phase14_schemas.pytests/unit/services/orchestrators/test_model_endpoint_service.pyui/src/pages/admin/models.tsx
Ahmath-Gadji
added a commit
that referenced
this pull request
Jul 30, 2026
…write A partition PATCH could validate chat_llm against the in-memory catalog, then block behind a concurrent PgModelEndpointRepository.rename()'s cascade UPDATE on the very row it was about to write, then resume and write the now-renamed-away name straight back once the rename commits. Once the service's temporary alias (#769) drops on the next reload, that partition points at an endpoint name that no longer exists — permanently, since nothing revalidates a stored chat_llm after assignment. rename() now LOCKs partitions IN SHARE MODE before touching anything, the same lock PgPresetRepository.delete already takes for the equivalent preset-delete-vs-assign race. PgPartitionRepository.update_partition gets a matching DB-authoritative re-check: when chat_llm is assigned, the write and a model_endpoints existence check run in one transaction that touches partitions before model_endpoints — the same order the rename's cascade uses — so the two can only block on each other, never deadlock, and a write that loses the race rolls back with MODEL_ENDPOINT_NOT_FOUND instead of silently persisting a stale reference. embedder carries no assignment-time validation today, so it isn't part of this guard — nothing to race against yet. Addresses further review feedback from @hedhoud on #769.
Collaborator
Author
|
Addressed the DB-level TOCTOU race in 7afc6aa. @coderabbitai review |
|
✅ Action performedReview finished.
|
Ahmath-Gadji
force-pushed
the
fix/768-model-endpoint-name-slash
branch
from
July 30, 2026 10:46
7afc6aa to
9902e3a
Compare
Ahmath-Gadji
added a commit
that referenced
this pull request
Jul 30, 2026
The DB rename (and its partitions/presets cascade, #770) commits before PresetService.load_all() / PartitionService.load_partitions() run, and both are awaited — so a concurrent request can land in that window and resolve a partition/preset already repointed at new_name against a registry that (until the final load_all() call) still only answers to the old name, raising a bare KeyError. A failed reload made it worse: the rename stayed committed but the registry never got the new name at all, wedging every subsequent request until process restart. Alias new_name to the (unchanged) config synchronously right after the rename commits, before either reload call gets a chance to yield control. A pure rename doesn't touch endpoint/model_name/extra, so both names stay correctly resolvable throughout the transition; the next load_all() drops the old one on its own once the DB is the only source of truth again. Addresses review feedback from @hedhoud on #769.
Ahmath-Gadji
added a commit
that referenced
this pull request
Jul 30, 2026
Rejecting '/' alone (#768) missed that the exact values '.' and '..' are RFC 3986 dot-segments: browsers/HTTP clients normalize them out of the URL before the request is even sent, resolving to the collection route or dropping the model_type segment entirely — the same "row exists but every single-endpoint route 404s" failure mode as the slash case, just via a different mechanism. Denylisting each unsafe value as it's discovered doesn't close the class. Replace both checks with one allowlist: name must start and end with an alphanumeric character, with '.', '_', '-' allowed in between. That rules out '/', '.', '..', and any leading/trailing separator by construction, while still accepting realistic names like 'gpt-4.1' or 'jina_v3'. Also caps the name at 128 characters — the DB column has no length bound today. Mirrored in the admin UI's client-side guard, same rationale as before: FastAPI's validation `detail` is a list, so ApiError can't surface a readable message for a raw 422. Addresses review feedback from @hedhoud on #769.
Ahmath-Gadji
added a commit
that referenced
this pull request
Jul 30, 2026
_alias_renamed_name copied whatever the in-memory registry held under old_name before this call ran. A rename can land in the same request as a field update (e.g. a new endpoint URL), applied to the DB before the alias is built — so the alias silently pinned both names to the pre-update config. If a reload then failed, the final load_all() that would have corrected it never ran, leaving the registry serving the old endpoint under the DB-authoritative new name until process restart. Build the aliased config from the row this call just wrote (the update() result, or the pre-fetched row when no fields changed) instead of reading the bucket. Both old_name and new_name now always alias to the same, correct, up-to-date config. Addresses further review feedback from @hedhoud on #769.
Ahmath-Gadji
added a commit
that referenced
this pull request
Jul 30, 2026
…write A partition PATCH could validate chat_llm against the in-memory catalog, then block behind a concurrent PgModelEndpointRepository.rename()'s cascade UPDATE on the very row it was about to write, then resume and write the now-renamed-away name straight back once the rename commits. Once the service's temporary alias (#769) drops on the next reload, that partition points at an endpoint name that no longer exists — permanently, since nothing revalidates a stored chat_llm after assignment. rename() now LOCKs partitions IN SHARE MODE before touching anything, the same lock PgPresetRepository.delete already takes for the equivalent preset-delete-vs-assign race. PgPartitionRepository.update_partition gets a matching DB-authoritative re-check: when chat_llm is assigned, the write and a model_endpoints existence check run in one transaction that touches partitions before model_endpoints — the same order the rename's cascade uses — so the two can only block on each other, never deadlock, and a write that loses the race rolls back with MODEL_ENDPOINT_NOT_FOUND instead of silently persisting a stale reference. embedder carries no assignment-time validation today, so it isn't part of this guard — nothing to race against yet. Addresses further review feedback from @hedhoud on #769.
Collaborator
Author
|
@hedhoud all three of your review threads are addressed (fix commits linked inline on each), CodeRabbit's findings are resolved too, and CI is green on the rebased branch. Could you take another look and approve if it looks good? |
Ahmath-Gadji
added a commit
that referenced
this pull request
Jul 30, 2026
…, safely PgModelEndpointRepository.rename() now cascades the new name to every stored reference — partitions.embedder / partitions.chat_llm, and the endpoint-name fields embedded in pipeline_presets.config (JSONB) — in the same transaction as the rename. Before this, a renamed endpoint silently stranded every partition/preset that pointed at the old name, since nothing else in the schema updates those when the referenced row's name changes (#770). Raises NotFoundError if the row vanished between the service's existence check and this transaction (a concurrent delete), mirroring PgPipelinePresetRepository.rename. ModelEndpointService.update_model_endpoint reloads PresetService then PartitionService's in-memory caches after a rename (the same order PresetService.update_preset uses), so the cascade's DB writes actually take effect, and finally puts the until-now-unused partition_service constructor arg to use. Three concurrency gaps in that reload sequence are closed: - The DB rename commits before either reload call runs, and both `await`, so a concurrent request could resolve a name the cascade already repointed at against a registry that still only knew the old one (bare KeyError), or — if a reload call raised — never learn the new name at all until process restart. `_alias_renamed_name` makes both the old and new name resolve immediately after the rename commits, no `await` in between, closing that window regardless of how the reload calls resolve. - A rename combined with a field change (e.g. a new endpoint URL) was aliasing the *pre-update* config, since the alias copied whatever the in-memory bucket held before this call ran rather than the row this call just wrote — a reload failure right after would leave the registry silently serving stale settings under the DB-authoritative new name. The alias is now built from the fresh row. - The client-instance cache (checked before the config registry) could keep serving a client built against the pre-rename/pre-update endpoint under either name, since it was only evicted at the very end of the method — skipped entirely if a reload call raised. Both names' cached clients are now evicted right after aliasing, before either reload `await`. Finally, a DB-level race: a partition PATCH could validate chat_llm in-memory, then block behind a concurrent rename's cascade UPDATE on the exact partitions row it was about to write, then resume and write the now-renamed-away name straight back once the rename commits — stranding that partition permanently once the temporary alias above drops. rename() now LOCKs partitions IN SHARE MODE before touching anything, the same lock PgPresetRepository.delete already takes for the equivalent preset-delete-vs-assign race, and PgPartitionRepository.update_partition gets a matching DB-authoritative re-check: assigning chat_llm now runs the write and a model_endpoints existence check in one transaction that touches partitions before model_endpoints — the same order the rename cascade uses, so the two can only block on each other, never deadlock, and a write that loses the race rolls back with MODEL_ENDPOINT_NOT_FOUND instead of silently persisting a stale reference. embedder isn't covered — it has no assignment-time validation at all today, so there's nothing yet for a rename to race against on that column. Closes #768. All of the above lands from review feedback on #769 (@hedhoud, CodeRabbit) — reproduced independently as #770.
Ahmath-Gadji
force-pushed
the
fix/768-model-endpoint-name-slash
branch
from
July 30, 2026 11:03
9902e3a to
c883462
Compare
hedhoud
approved these changes
Jul 30, 2026
hedhoud
left a comment
hedhoud
left a comment
Collaborator
There was a problem hiding this comment.
Rechecked the latest update and the earlier findings are addressed. The affected backend tests, lint, UI build, and CI are all clean. Approved from my side.
`name` is embedded as a single path segment in every single-endpoint
route (GET/PUT/DELETE /model-endpoints/{model_type}/{name},
.../set-default, .../reveal-api-key, .../validate). Denylisting unsafe
values one at a time doesn't close the class: a `/` splits across path
segments (#768), and the exact values `.`/`..` are RFC 3986 dot-segments
that browsers and HTTP clients normalize out of the URL before the
request is even sent — the same "row exists, every route 404s" failure,
via a different mechanism.
Replace the denylist with one allowlist: name must start and end with an
alphanumeric character, with '.', '_', '-' allowed in between. That rules
out '/', '.', '..', and any leading/trailing separator by construction,
while still accepting realistic names like 'gpt-4.1' or 'jina_v3'. Also
caps the name at 128 characters (the DB column has no length bound
today) and mirrors the same regex in the admin UI's client-side guard —
FastAPI's validation `detail` is a list, so ApiError can't surface a
readable message for a raw 422.
RetrievalService now resolves a partition's reranker the same way QueryService._resolve_llm/_default_llm (#755) resolves chat_llm: the partition's configured preset first, falling back to the catalog default, then the static startup reranker, with the resolved endpoint logged at debug — so "which reranker ran?" is answerable from logs the way chat_llm already is. This also closes the gap #755 fixed for chat_llm, on the reranker side: a partition's `reranker` preset has no create/PATCH-time validation, so a renamed/deleted endpoint reaching `_reranker_factory` raised an unhandled KeyError instead of falling back to the catalog default (and then the static reranker if that's missing too). The fallback warning binds reranker/partition as structured Loguru context via `.bind()` rather than passing them as message-format kwargs — passed directly to `logger.warning(msg, key=val)`, they were silently dropped since the message has no `{}` placeholders to substitute into, so the log never actually recorded which reranker/partition triggered the fallback.
…, safely PgModelEndpointRepository.rename() now cascades the new name to every stored reference — partitions.embedder / partitions.chat_llm, and the endpoint-name fields embedded in pipeline_presets.config (JSONB) — in the same transaction as the rename. Before this, a renamed endpoint silently stranded every partition/preset that pointed at the old name, since nothing else in the schema updates those when the referenced row's name changes (#770). Raises NotFoundError if the row vanished between the service's existence check and this transaction (a concurrent delete), mirroring PgPipelinePresetRepository.rename. ModelEndpointService.update_model_endpoint reloads PresetService then PartitionService's in-memory caches after a rename (the same order PresetService.update_preset uses), so the cascade's DB writes actually take effect, and finally puts the until-now-unused partition_service constructor arg to use. Three concurrency gaps in that reload sequence are closed: - The DB rename commits before either reload call runs, and both `await`, so a concurrent request could resolve a name the cascade already repointed at against a registry that still only knew the old one (bare KeyError), or — if a reload call raised — never learn the new name at all until process restart. `_alias_renamed_name` makes both the old and new name resolve immediately after the rename commits, no `await` in between, closing that window regardless of how the reload calls resolve. - A rename combined with a field change (e.g. a new endpoint URL) was aliasing the *pre-update* config, since the alias copied whatever the in-memory bucket held before this call ran rather than the row this call just wrote — a reload failure right after would leave the registry silently serving stale settings under the DB-authoritative new name. The alias is now built from the fresh row. - The client-instance cache (checked before the config registry) could keep serving a client built against the pre-rename/pre-update endpoint under either name, since it was only evicted at the very end of the method — skipped entirely if a reload call raised. Both names' cached clients are now evicted right after aliasing, before either reload `await`. Finally, a DB-level race: a partition PATCH could validate chat_llm in-memory, then block behind a concurrent rename's cascade UPDATE on the exact partitions row it was about to write, then resume and write the now-renamed-away name straight back once the rename commits — stranding that partition permanently once the temporary alias above drops. rename() now LOCKs partitions IN SHARE MODE before touching anything, the same lock PgPresetRepository.delete already takes for the equivalent preset-delete-vs-assign race, and PgPartitionRepository.update_partition gets a matching DB-authoritative re-check: assigning chat_llm now runs the write and a model_endpoints existence check in one transaction that touches partitions before model_endpoints — the same order the rename cascade uses, so the two can only block on each other, never deadlock, and a write that loses the race rolls back with MODEL_ENDPOINT_NOT_FOUND instead of silently persisting a stale reference. embedder isn't covered — it has no assignment-time validation at all today, so there's nothing yet for a rename to race against on that column. Closes #768. All of the above lands from review feedback on #769 (@hedhoud, CodeRabbit) — reproduced independently as #770.
The backend trims `name` before applying the length/allowlist checks (and before persisting it), but this form validated the raw input — " gpt-4.1 " was accepted by the backend but blocked here. Validate and submit `name.trim()` consistently: the inline error, the create/update payloads, and the rename comparison all use the same trimmed value now.
Ahmath-Gadji
force-pushed
the
fix/768-model-endpoint-name-slash
branch
from
July 30, 2026 12:22
46c8b3d to
4586475
Compare
Merged
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
2 participants
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.
Summary
/in model-endpointnameon create/rename (api/schemas/admin/model_endpoint_schemas.py) — a slash splits across the/{model_type}/{name}path segment used by every single-endpoint route, so the row stays visible in the list but every get/update/delete/set-default/reveal-api-key/validate call 404s, even URL-encoded.ui/src/pages/admin/models.tsx): inline error + disabled submit, instead of letting the mistake reach the backend and surface as an unreadable "HTTP 422" toast (ApiErroronly unwraps string-shapeddetail; FastAPI's validation errors are a list).RetrievalServicenow logs (debug) which reranker resolved for a partition — its own preset, the catalog default, or the static startup reranker — mirroringQueryService._resolve_llm/_default_llm(fix(chat): resolve the default chat LLM from the catalog default endpoint #755), so "which reranker ran?" is answerable from logs the way chat_llm already is.rerankerpreset has no create/PATCH-time validation, so a renamed/deleted endpoint reaching_reranker_factoryraised an unhandledKeyErrorinstead of falling back to the catalog default (and then the static reranker if that's missing too).PgModelEndpointRepository.rename()now cascades the new name to every stored reference —partitions.embedder/partitions.chat_llm, and the endpoint-name fields embedded inpipeline_presets.config(JSONB) — in the same transaction as the rename. This is the defect @andyne13 flagged on Model endpoint names containing '/' become permanently unreachable (404 on delete/set-default/update) #768 (originally filed independently as Model endpoint with a '/' in its name is unmanageable: every {name} route 404s #770): reproducible with any path-safe name, and a naive rescue rename of a stuck slash-named row would have hit it too.ModelEndpointService.update_model_endpointnow reloadsPresetServicethenPartitionService's in-memory caches after a rename, mirroringPresetService.update_preset's existing pattern — and finally puts the until-now-unusedpartition_serviceconstructor arg to use.Closes #768
Test plan
uv run pytest tests/unit/— 2220 passeduv run ruff check/ruff format— clean (pre-commit hook)npx tsc --noEmitonui/— cleanDELETE/set-defaulton arerankerendpoint namedLocalReranker/re→ 404 even with%2F), confirmed the row existed in Postgres, then verified the new validator rejects the same name at the API layer.Note: pre-existing rows created before this fix (with a
/already in their name) stay stuck and need a direct DB rename/delete — the API can never reach them.Summary by CodeRabbit
New Features
namevalidation to a single safe URL path segment (allowlist + max length), consistently enforced in the backend and admin UI.Bug Fixes
Tests