diff --git a/app/modules/model_sources/repository.py b/app/modules/model_sources/repository.py index 19d39febcd..6b4cfce8cb 100644 --- a/app/modules/model_sources/repository.py +++ b/app/modules/model_sources/repository.py @@ -1,12 +1,27 @@ from __future__ import annotations -from sqlalchemy import delete, select +from sqlalchemy import ColumnElement, and_, delete, or_, select from sqlalchemy.ext.asyncio import AsyncSession from sqlalchemy.orm import selectinload from app.db.models import ModelSource, ModelSourceModel +def _enablement_filter(only_disabled: bool) -> ColumnElement[bool]: + """Enabled-state predicate for a per-capability source lookup. + + ``only_disabled`` selects the exact complement of the routable set: rows + that match the model and the route shape but that an operator switched off, + at the source or at the individual model. Routing needs that complement to + tell "no source serves this model" apart from "the source that serves this + model is switched off" -- the latter must not fall through to a + subscription account, which rejects the model outright. + """ + if only_disabled: + return or_(ModelSource.is_enabled.is_(False), ModelSourceModel.is_enabled.is_(False)) + return and_(ModelSource.is_enabled.is_(True), ModelSourceModel.is_enabled.is_(True)) + + class ModelSourcesRepository: def __init__(self, session: AsyncSession) -> None: self._session = session @@ -38,16 +53,16 @@ async def find_chat_source_for_model( *, allowed_source_ids: set[str] | None = None, require_streaming: bool = False, + only_disabled: bool = False, ) -> ModelSource | None: stmt = ( select(ModelSource) .options(selectinload(ModelSource.models)) .join(ModelSourceModel, ModelSourceModel.source_id == ModelSource.id) .where(ModelSource.kind == "openai_compatible") - .where(ModelSource.is_enabled.is_(True)) .where(ModelSource.supports_chat_completions.is_(True)) .where(ModelSourceModel.model == model) - .where(ModelSourceModel.is_enabled.is_(True)) + .where(_enablement_filter(only_disabled)) .order_by(ModelSource.name, ModelSource.id) .limit(1) ) @@ -66,16 +81,16 @@ async def find_responses_source_for_model( *, allowed_source_ids: set[str] | None = None, require_streaming: bool = False, + only_disabled: bool = False, ) -> ModelSource | None: stmt = ( select(ModelSource) .options(selectinload(ModelSource.models)) .join(ModelSourceModel, ModelSourceModel.source_id == ModelSource.id) .where(ModelSource.kind == "openai_compatible") - .where(ModelSource.is_enabled.is_(True)) .where(ModelSource.supports_responses.is_(True)) .where(ModelSourceModel.model == model) - .where(ModelSourceModel.is_enabled.is_(True)) + .where(_enablement_filter(only_disabled)) .order_by(ModelSource.name, ModelSource.id) .limit(1) ) diff --git a/app/modules/model_sources/selection.py b/app/modules/model_sources/selection.py index 849c597599..9c0386d221 100644 --- a/app/modules/model_sources/selection.py +++ b/app/modules/model_sources/selection.py @@ -34,8 +34,19 @@ async def select_responses_model_source( *, raw_model: str | None = None, require_streaming: bool = False, + only_disabled: bool = False, ) -> tuple[ModelSource, str] | None: - """Resolve ``model`` to a Responses-capable model source, if any.""" + """Resolve ``model`` to a Responses-capable model source, if any. + + ``only_disabled`` inverts the enabled-state filter and leaves every other + rule -- candidate order, API key model allowlist, source assignment scope, + subscription-registry precedence, route shape, streaming -- untouched, so + the answer is exactly "the source this request would have used, had the + operator not switched it off". Callers use it after the ordinary lookup + misses, to refuse the request instead of handing a source-owned model to a + subscription account that cannot serve it. Sharing one function keeps the + two lookups from drifting apart the way the transports once did. + """ assigned_source_ids = allowed_source_ids_for_api_key(api_key) exact_allowed_models = set(api_key.allowed_models) if api_key and api_key.allowed_models else None candidates = [candidate for candidate in (raw_model, model) if candidate] @@ -55,6 +66,7 @@ async def select_responses_model_source( candidate, allowed_source_ids=assigned_source_ids, require_streaming=require_streaming, + only_disabled=only_disabled, ) if source is not None: break diff --git a/app/modules/proxy/_service/streaming/helpers.py b/app/modules/proxy/_service/streaming/helpers.py index c37a1d1498..0ddd2603a2 100644 --- a/app/modules/proxy/_service/streaming/helpers.py +++ b/app/modules/proxy/_service/streaming/helpers.py @@ -387,6 +387,7 @@ from app.modules.proxy.helpers import ( _normalize_error_code, classify_upstream_failure, + is_model_scoped_upstream_rejection, is_upstream_model_capacity_error, ) from app.modules.proxy.http_bridge_forwarding import ( @@ -867,6 +868,36 @@ def _is_account_neutral_request_rejection( return bool(_facade()._is_missing_tool_output_message(message)) +def _is_model_scoped_rejection( + *, + http_status: int | None, + message: str | None, +) -> bool: + """Return whether upstream rejected the requested model, not the account. + + The ChatGPT model-entitlement rejection is scoped to the model named in the + message. It proves nothing about the account's ability to serve the models + it *is* entitled to, so it must never move that account's ``error_count``: + otherwise one client looping on a model no account can serve drives every + serving account into ``ERROR_BACKOFF_THRESHOLD`` backoff and starves all + unrelated traffic on those accounts -- the exact failure mode when a model + reaches subscription selection because its OpenAI-compatible model source + is disabled or unreachable. + + Failover is deliberately untouched: the classified failure is still + returned, so the caller keeps trying other accounts, whose entitlements may + differ. + + The normalized code is not part of the match. Upstream delivers this + rejection with neither ``code`` nor ``type`` on the streaming path, which + normalizes to ``upstream_error``; on other paths it arrives as + ``invalid_request_error``. Only the exact message shape decides membership. + """ + if http_status is not None and http_status != 400: + return False + return is_model_scoped_upstream_rejection(message) + + async def _handle_stream_error( proxy: Any, account: Account, @@ -896,6 +927,17 @@ async def _handle_stream_error( code, ) return classified + if _is_model_scoped_rejection( + http_status=http_status, + message=error.get("message"), + ): + _facade().logger.info( + "Skipped account error penalty for model-scoped upstream rejection account_id=%s request_id=%s code=%s", + "" if privacy_policy.redacts_sensitive_details else account.id, + get_request_id(), + code, + ) + return classified if classified["failure_class"] == "rate_limit": await proxy._load_balancer.mark_rate_limit(account, error) elif classified["failure_class"] == "quota": diff --git a/app/modules/proxy/api.py b/app/modules/proxy/api.py index e5c0e1b4d4..9f36a8bcf0 100644 --- a/app/modules/proxy/api.py +++ b/app/modules/proxy/api.py @@ -1132,6 +1132,17 @@ async def responses( if source_selection is not None: source, selected_model = source_selection responses_payload.model = selected_model + else: + disabled_denial = await _disabled_model_source_denial( + request, + responses_payload.model, + api_key, + route="responses", + raw_model=raw_source_model, + require_streaming=True, + ) + if disabled_denial is not None: + return disabled_denial if source is not None: # Opportunistic admission gates subscription *account* capacity; # source-routed requests use no account, so a closed/empty pool must @@ -1272,9 +1283,10 @@ async def v1_responses( # File-referencing Responses requests pin to the subscription account that # registered the upload; that account-scoped invariant applies to /v1 # streams too, so such requests must not be source-routed. + file_pinned = bool(extract_input_file_ids(responses_payload.input)) source_selection = ( None - if extract_input_file_ids(responses_payload.input) + if file_pinned else await _select_responses_model_source( responses_payload.model, api_key, @@ -1285,6 +1297,17 @@ async def v1_responses( source = source_selection[0] if source_selection is not None else None if source_selection is not None: responses_payload.model = source_selection[1] + elif not file_pinned: + disabled_denial = await _disabled_model_source_denial( + request, + responses_payload.model, + api_key, + route="responses", + raw_model=raw_source_model, + require_streaming=responses_payload.stream is True, + ) + if disabled_denial is not None: + return disabled_denial if source is not None: # Opportunistic admission gates subscription *account* capacity; # source-routed requests use no account, so a closed/empty pool must @@ -4213,6 +4236,7 @@ async def v1_chat_completions( if prohibit_fast_mode and _is_fast_mode_model_alias(effective_model): effective_model = responses_payload.model validate_model_access(api_key, responses_payload.model) + source_route_attempted = not responses_shaped_payload and payload.messages is not None source_selection = ( await _select_chat_model_source( responses_payload.model, @@ -4220,11 +4244,24 @@ async def v1_chat_completions( raw_model=effective_model, require_streaming=payload.stream is True, ) - if not responses_shaped_payload and payload.messages is not None + if source_route_attempted else None ) source = source_selection[0] if source_selection is not None else None request_model = source_selection[1] if source_selection is not None else responses_payload.model + if source is None and source_route_attempted: + # Before any reservation is taken, so a refusal strands nothing. + disabled_denial = await _disabled_model_source_denial( + request, + responses_payload.model, + api_key, + route="chat", + raw_model=effective_model, + require_streaming=payload.stream is True, + headers=rate_limit_headers, + ) + if disabled_denial is not None: + return disabled_denial if source is None: apply_enforced_service_tier_model_fallback( responses_payload, @@ -4360,7 +4397,15 @@ async def _select_chat_model_source( *, raw_model: str | None = None, require_streaming: bool = False, + only_disabled: bool = False, ) -> tuple[ModelSource, str] | None: + """Resolve ``model`` to a Chat Completions-capable model source, if any. + + ``only_disabled`` mirrors :func:`select_responses_model_source`: every rule + stays the same except the enabled-state filter, which is inverted, so the + result names the source the request would have used had it not been + switched off. + """ assigned_source_ids = _allowed_source_ids_for_api_key(api_key) exact_allowed_models = set(api_key.allowed_models) if api_key and api_key.allowed_models else None candidates = [candidate for candidate in (raw_model, model) if candidate] @@ -4380,6 +4425,7 @@ async def _select_chat_model_source( candidate, allowed_source_ids=assigned_source_ids, require_streaming=require_streaming, + only_disabled=only_disabled, ) if source is not None: break @@ -4398,6 +4444,7 @@ async def _select_responses_model_source( *, raw_model: str | None = None, require_streaming: bool = False, + only_disabled: bool = False, ) -> tuple[ModelSource, str] | None: # Shared with the WebSocket path so both transports agree on which models # belong to a model source. @@ -4406,7 +4453,64 @@ async def _select_responses_model_source( api_key, raw_model=raw_model, require_streaming=require_streaming, + only_disabled=only_disabled, + ) + + +async def _disabled_model_source_denial( + request: Request, + model: str, + api_key: ApiKeyData | None, + *, + route: Literal["chat", "responses"], + raw_model: str | None = None, + require_streaming: bool = False, + headers: Mapping[str, str] | None = None, +) -> JSONResponse | None: + """Refuse a request whose model source exists but is switched off. + + Called only after the ordinary lookup missed. A miss has two very different + causes that the routing code cannot otherwise tell apart: the model belongs + to nobody, or it belongs to a model source (or a source model) an operator + disabled. Only the first may fall through to subscription selection -- + handing a source-owned slug to a ChatGPT account produces + ``The '' model is not supported when using Codex with a ChatGPT + account.``, which burns the account's health signal and tells the operator + nothing about the source they switched off. + + Returns ``None`` when no disabled source claims the model, leaving every + other request on its existing path. + """ + selection = ( + await _select_responses_model_source( + model, + api_key, + raw_model=raw_model, + require_streaming=require_streaming, + only_disabled=True, + ) + if route == "responses" + else await _select_chat_model_source( + model, + api_key, + raw_model=raw_model, + require_streaming=require_streaming, + only_disabled=True, + ) + ) + if selection is None: + return None + source, matched_model = selection + # The source name is operator-facing configuration, not a client-visible + # identifier, so the envelope names the model and the condition only. + reason = "is disabled" if not source.is_enabled else "has that model disabled" + error = openai_error( + "model_source_disabled", + f"The model '{matched_model}' is served by an OpenAI-compatible model source that {reason}. " + "Enable the source and its model in codex-lb, or request a different model.", + error_type="upstream_error", ) + return _logged_error_json_response(request, 503, error, headers=headers) async def _select_embeddings_model_source(model: str, api_key: ApiKeyData | None) -> ModelSource | None: diff --git a/app/modules/proxy/helpers.py b/app/modules/proxy/helpers.py index f47417b3f0..e8a1489398 100644 --- a/app/modules/proxy/helpers.py +++ b/app/modules/proxy/helpers.py @@ -1,5 +1,6 @@ from __future__ import annotations +import re from typing import Iterable from pydantic import ValidationError @@ -40,6 +41,29 @@ {"server_error", "upstream_error", "stream_incomplete", "overloaded_error", "server_is_overloaded"} ) _MODEL_CAPACITY_MESSAGE_MARKERS = ("selected model is at capacity",) +_MODEL_UNSUPPORTED_MESSAGE_RE = re.compile( + r"^The '.+' model is not supported when using Codex with a ChatGPT account\.$" +) + + +def is_model_scoped_upstream_rejection(message: str | None) -> bool: + """Match the ChatGPT model-entitlement rejection for *any* requested model. + + The rejection names the model, not the account: it reproduces on every + request for that model and says nothing about whether the serving account + can still stream the models it is entitled to. Callers use it to keep the + rejection out of account health while leaving failover alone -- a different + account may hold a different entitlement. + + Unlike ``_is_account_model_unsupported_error`` this does not require the + caller to know the requested model or the normalized error code. Upstream + delivers this rejection over the Codex WebSocket with neither ``code`` nor + ``type`` populated, which normalizes to the ``upstream_error`` fallback, so + a code-gated match misses it on the live stream path. + """ + if message is None: + return False + return _MODEL_UNSUPPORTED_MESSAGE_RE.fullmatch(" ".join(message.split())) is not None def _is_account_model_unsupported_error( diff --git a/openspec/changes/refuse-disabled-model-source-routing/proposal.md b/openspec/changes/refuse-disabled-model-source-routing/proposal.md new file mode 100644 index 0000000000..f1eedd87d9 --- /dev/null +++ b/openspec/changes/refuse-disabled-model-source-routing/proposal.md @@ -0,0 +1,43 @@ +## Why + +Source routing filters on `is_enabled` inside the lookup itself +(`ModelSourcesRepository.find_chat_source_for_model` / +`find_responses_source_for_model`), so a disabled source and a model nobody +configured produce the same answer: `None`. Both then fall through to +subscription account selection, and the subscription upstream rejects the +request: + +``` +The '' model is not supported when using Codex with a ChatGPT account. +``` + +On a live instance this repeated hundreds of times per hour for a model whose +only source had been switched off: every attempt selected a ChatGPT account, +spent its health signal on a request no account could serve, and told the +caller nothing about the source that was actually off. + +This is the same failure #1658/#1659 fixed for the WebSocket transport — a +source-owned model reaching a subscription account — reached through a +different door. The HTTP routes need the same guarantee, and it must be stated +in terms an operator can act on. + +## What Changes + +- Add `only_disabled` to the chat and Responses source lookups. It inverts the + enabled-state filter and leaves every other rule — candidate order, API key + model allowlist, source assignment scope, subscription-registry precedence, + route shape, streaming — untouched, so a hit is exactly "the source this + request would have used, had the operator not switched it off". +- Refuse such a request on `/v1/chat/completions`, `/v1/responses`, and + `/backend-api/codex/responses` with `503` and error code + `model_source_disabled`, instead of falling through to subscription routing. +- Leave every other miss on its existing path: a model no source claims, a + source scoped away from the API key, a route-shape mismatch, and a + subscription slug that an unscoped key never source-routes all behave exactly + as before. + +## Capabilities + +### Modified Capabilities + +- `responses-api-compat` diff --git a/openspec/changes/refuse-disabled-model-source-routing/specs/responses-api-compat/spec.md b/openspec/changes/refuse-disabled-model-source-routing/specs/responses-api-compat/spec.md new file mode 100644 index 0000000000..e90685e3fd --- /dev/null +++ b/openspec/changes/refuse-disabled-model-source-routing/specs/responses-api-compat/spec.md @@ -0,0 +1,65 @@ +## ADDED Requirements + +### Requirement: A disabled model source refuses its models instead of falling through + +The system SHALL NOT dispatch to a subscription account a request whose model +is served by an OpenAI-compatible model source that an operator has switched +off. It SHALL refuse such a request with HTTP status `503` and error code +`model_source_disabled`. + +"Switched off" covers both a disabled source row and a disabled model row on an +enabled source. The refusal SHALL apply on `/v1/chat/completions`, +`/v1/responses`, and `/backend-api/codex/responses`. + +The refusal SHALL be decided by the ordinary source-selection rules with the +enabled-state filter inverted and nothing else changed: same candidate list +(raw client alias and normalized model), same API key model allowlist, same +source assignment scope, same subscription-registry precedence, same route +shape, same streaming requirement. A request that the ordinary lookup would +have missed for any reason other than enabled state MUST keep its existing +behaviour, including a model no source exposes, a source the API key is not +assigned to, a chat-only source asked for a Responses route, and a +subscription-registry slug that an unscoped API key never source-routes. + +Requests excluded from source routing — a terminal `compaction_trigger`, and +Responses requests pinned to the subscription account that received an uploaded +file — MUST NOT be refused, and MUST proceed to subscription routing as before. + +The refusal MUST happen before any usage reservation is taken, so a refused +request strands no reservation, and MUST NOT create a request log entry for a +dispatch that never happened. + +#### Scenario: Chat request for a disabled source's model is refused + +- **GIVEN** an OpenAI-compatible model source exposes model `m` and is disabled +- **WHEN** a client calls `POST /v1/chat/completions` with model `m` +- **THEN** the response is `503` with error code `model_source_disabled` +- **AND** no subscription account is selected for the request +- **AND** no usage reservation is left held + +#### Scenario: Responses request for a disabled source's model is refused + +- **GIVEN** a Responses-capable OpenAI-compatible model source exposes model `m` and is disabled +- **WHEN** a client calls `POST /v1/responses` or `POST /backend-api/codex/responses` with model `m` +- **THEN** the response is `503` with error code `model_source_disabled` +- **AND** no subscription account is selected for the request + +#### Scenario: A disabled model on an enabled source is refused + +- **GIVEN** an enabled OpenAI-compatible model source whose model row for `m` is disabled +- **WHEN** a client calls `POST /v1/chat/completions` with model `m` +- **THEN** the response is `503` with error code `model_source_disabled` + +#### Scenario: A model no source exposes is unaffected + +- **GIVEN** no model source exposes model `m`, enabled or disabled +- **WHEN** a client calls `POST /v1/chat/completions` with model `m` +- **THEN** subscription routing proceeds exactly as it did before this requirement + +#### Scenario: A subscription slug shadowed by a disabled source is unaffected + +- **GIVEN** a disabled OpenAI-compatible model source lists a slug the subscription model registry already serves +- **AND** an API key without source assignment scoping +- **WHEN** the key requests that slug +- **THEN** the request is not refused with `model_source_disabled` +- **AND** subscription routing proceeds unchanged diff --git a/openspec/changes/refuse-disabled-model-source-routing/tasks.md b/openspec/changes/refuse-disabled-model-source-routing/tasks.md new file mode 100644 index 0000000000..ca9ba89395 --- /dev/null +++ b/openspec/changes/refuse-disabled-model-source-routing/tasks.md @@ -0,0 +1,20 @@ +## Tasks + +- [x] Add an `only_disabled` enabled-state filter to + `find_chat_source_for_model` / `find_responses_source_for_model`, sharing + one predicate so the routable set and its complement cannot drift. +- [x] Thread `only_disabled` through `select_responses_model_source` and + `_select_chat_model_source`, so the refusal lookup reuses the selection + rules rather than restating them. +- [x] Add `_disabled_model_source_denial` and call it from + `/v1/chat/completions`, `/v1/responses`, and + `/backend-api/codex/responses` after the ordinary lookup misses. +- [x] On the chat route, run the refusal before the usage reservation is taken + so a refusal strands nothing. +- [x] Keep the existing source-routing exclusions intact: file-pinned + Responses requests and terminal compaction triggers skip the refusal and + continue to subscription routing. +- [x] Add the spec delta for `responses-api-compat`. +- [x] Cover the refusal (disabled source, disabled source model, all three + routes) and the negative controls (unknown model, subscription slug + shadowed by a disabled source) with integration tests. diff --git a/openspec/changes/spare-account-health-from-model-rejections/proposal.md b/openspec/changes/spare-account-health-from-model-rejections/proposal.md new file mode 100644 index 0000000000..2f4d76c803 --- /dev/null +++ b/openspec/changes/spare-account-health-from-model-rejections/proposal.md @@ -0,0 +1,56 @@ +## Why + +A model registered only on an OpenAI-compatible model source stops resolving to +that source the moment the source is disabled or removed, because source lookup +filters on `is_enabled`. The request then falls through to ordinary subscription +account selection, and every ChatGPT account rejects it with HTTP 400 and the +message `The '' model is not supported when using Codex with a ChatGPT +account.` + +Today each of those rejections records a transient account error. A single +client polling one unroutable model therefore drives *every* serving account +past `ERROR_BACKOFF_THRESHOLD` and pins it at the 300-second error-backoff +ceiling. On a live deployment one such client produced 4,800 rejections in six +hours across three healthy Pro accounts — one client request fans out to all +three accounts, so all three were penalized by every poll. Unrelated foreground +traffic on those accounts was then denied: sticky selection passes +`allow_backoff_fallback=False`, so a session hard-pinned to a poisoned account +failed with `continuity_owner_unavailable` / `No available accounts` while the +account itself was active with 1–7% quota used. + +The rejection names the model, not the account. It says nothing about whether +the account can still serve the models it *is* entitled to, so it is not an +account-health signal. Excluding that account for the rest of the request — +which failover already does — is the correct and sufficient response. + +## What Changes + +- A model-entitlement rejection no longer records a transient account error, + rate-limit penalty, quota penalty, or permanent failure. +- Failure classification, the failover decision, and the client-visible status + and body are unchanged, so a differently entitled account is still tried. +- Membership is decided by the exact rejection message and a 400 status, not by + the normalized error code. Upstream delivers this rejection over the Codex + streaming path with neither `code` nor `type` set, which normalizes to the + `upstream_error` fallback; the previous code-gated matcher never saw it there. +- The skip is logged, matching the existing account-neutral skip log. + +## Capabilities + +### New Capabilities + +None. + +### Modified Capabilities + +- `account-routing`: model-entitlement rejections are health neutral while + remaining failover-eligible. + +## Impact + +- `app/modules/proxy/helpers.py` gains a code-agnostic matcher for the + rejection message. +- `app/modules/proxy/_service/streaming/helpers.py` skips the account-health + penalty for it in `_handle_stream_error`, the single funnel every transport + uses for stream-error account health. +- No schema, config, or API surface changes. diff --git a/openspec/changes/spare-account-health-from-model-rejections/specs/account-routing/spec.md b/openspec/changes/spare-account-health-from-model-rejections/specs/account-routing/spec.md new file mode 100644 index 0000000000..dd0232c33f --- /dev/null +++ b/openspec/changes/spare-account-health-from-model-rejections/specs/account-routing/spec.md @@ -0,0 +1,46 @@ +## MODIFIED Requirements + +### Requirement: Upstream rejections of the request payload are account neutral + +When upstream rejects a request because of the request payload itself, the proxy MUST NOT mutate the selected account's health: it MUST NOT record a transient account error, a rate-limit penalty, a quota penalty, or a permanent failure for that account. An upstream failure qualifies as a payload rejection only when it would reproduce identically on every account. The proxy MUST decide membership from the classified upstream message, never from the `invalid_request_error` code alone, and MUST require the upstream HTTP status to be 400 whenever a status is known. An upstream missing-tool-output rejection — the `invalid_request_error` whose message identifies a tool call with no matching tool output — MUST qualify. The proxy MUST also leave account health untouched for the model-entitlement rejection `The '' model is not supported when using Codex with a ChatGPT account.`: that rejection is scoped to the named model and is not evidence about the account's ability to serve the models it is entitled to. Because upstream delivers that rejection on the streaming path with neither an error `code` nor an error `type`, which normalizes to the `upstream_error` fallback, the proxy MUST decide it from the message and the 400 status alone and MUST NOT require a particular normalized error code. Skipping the penalty MUST be logged so the decision is observable, and MUST NOT change the failure classification, the failover decision, or the status and body returned to the client. + +#### Scenario: Missing-tool-output rejection leaves account health untouched + +- **GIVEN** account A is selected for a request whose input references a tool call with no matching tool output +- **WHEN** upstream returns HTTP 400 `invalid_request_error` with a missing-tool-output message +- **THEN** the proxy does not increment account A's transient error count and does not mark it rate-limited, quota-exceeded, or permanently failed +- **AND** the failure is still classified `non_retryable` and surfaced to the client unchanged + +#### Scenario: Repeated client payload rejection cannot starve unrelated sessions + +- **GIVEN** one client repeatedly re-sends the same payload that upstream rejects for a missing tool output +- **WHEN** those requests are served by accounts shared with other sessions +- **THEN** no serving account enters error backoff because of that payload +- **AND** a session hard-pinned to one of those accounts is not failed with a saturated-hard-affinity selection error caused by that payload + +#### Scenario: Model-entitlement rejection leaves account health untouched + +- **GIVEN** account A is selected for a model it is not entitled to use +- **WHEN** upstream returns HTTP 400 stating the model is not supported when using Codex with a ChatGPT account, with the error code normalized to `upstream_error` or to `invalid_request_error` +- **THEN** the proxy does not increment account A's transient error count and does not mark it rate-limited, quota-exceeded, or permanently failed +- **AND** the skip is logged + +#### Scenario: Model-entitlement rejection still fails over + +- **GIVEN** account A returned the model-entitlement rejection for the requested model +- **WHEN** the proxy classifies that failure +- **THEN** the classification and failover decision are unchanged, so an account with a different entitlement is still attempted +- **AND** the status and body returned to the client when every attempt is exhausted are unchanged + +#### Scenario: A model no source can serve cannot poison subscription accounts + +- **GIVEN** a model that resolves to no enabled model source and therefore reaches subscription account selection +- **WHEN** a client polls that model repeatedly and every subscription account returns the model-entitlement rejection +- **THEN** no serving account enters error backoff because of those rejections +- **AND** unrelated traffic hard-pinned to those accounts is not denied with a continuity-owner-unavailable or no-available-accounts selection error caused by them + +#### Scenario: A genuine upstream failure still penalizes the account + +- **GIVEN** account A is selected for a request +- **WHEN** upstream fails with an `upstream_error` whose message is not the model-entitlement rejection +- **THEN** the proxy records the account-health penalty for account A as before diff --git a/openspec/changes/spare-account-health-from-model-rejections/tasks.md b/openspec/changes/spare-account-health-from-model-rejections/tasks.md new file mode 100644 index 0000000000..51cc4b8063 --- /dev/null +++ b/openspec/changes/spare-account-health-from-model-rejections/tasks.md @@ -0,0 +1,15 @@ +## 1. Match the rejection independently of the error code + +- [x] 1.1 Add `is_model_scoped_upstream_rejection(message)` to `app/modules/proxy/helpers.py`, matching the exact entitlement message for any model after whitespace folding, and leave `_is_account_model_unsupported_error` (model- and code-scoped, used by the replay paths) unchanged. + +## 2. Keep the rejection out of account health + +- [x] 2.1 Add `_is_model_scoped_rejection` to `app/modules/proxy/_service/streaming/helpers.py`, requiring HTTP 400 whenever a status is known. +- [x] 2.2 Return the classified failure from `_handle_stream_error` before any health mutation when it matches, logging the skip. + +## 3. Verification + +- [x] 3.1 Assert no `record_error`/`record_errors`/`mark_rate_limit`/`mark_quota_exceeded`/`mark_permanent_failure` for both the `upstream_error` and `invalid_request_error` code shapes. +- [x] 3.2 Assert the failure is still classified `retryable_transient` so failover is unaffected. +- [x] 3.3 Negative controls: a genuine `upstream_error` with an unrelated message, the same message at a non-400 status, and a `rate_limit_exceeded` failure all keep their existing penalties. +- [x] 3.4 Run `uv run ruff check`, `uv run ty check`, and the unit suite. diff --git a/tests/integration/test_model_source_routing.py b/tests/integration/test_model_source_routing.py index 23e3aec80d..2efd8106a9 100644 --- a/tests/integration/test_model_source_routing.py +++ b/tests/integration/test_model_source_routing.py @@ -473,6 +473,198 @@ async def test_source_unreachable_returns_error_envelope_and_releases_reservatio assert result.scalars().all() == [] +async def _set_source_enabled(async_client, source_id: str, enabled: bool) -> None: + response = await async_client.patch(f"/api/model-sources/{source_id}", json={"isEnabled": enabled}) + assert response.status_code == 200 + assert response.json()["isEnabled"] is enabled + + +async def _disable_source_model(async_client, source_id: str, model: str) -> None: + response = await async_client.patch( + f"/api/model-sources/{source_id}", + json={ + "models": [ + { + "model": model, + "displayName": model, + "contextWindow": 8192, + "maxOutputTokens": 1024, + "supportsStreaming": True, + "supportsTools": True, + "isEnabled": False, + } + ] + }, + ) + assert response.status_code == 200 + assert response.json()["models"][0]["isEnabled"] is False + + +@pytest.mark.asyncio +async def test_disabled_source_chat_is_refused_instead_of_hitting_a_subscription(async_client): + """A disabled source's model must not be handed to a subscription account. + + Subscription upstreams answer such a request with "The '' model is + not supported when using Codex with a ChatGPT account.", which tells the + caller nothing and spends the account's health signal on a request no + account could ever serve. + """ + await _enable_api_key_auth(async_client) + model = "disabled-source-chat-model" + source_id = await _create_model_source( + async_client, + name="disabled-chat-source", + model=model, + base_url=f"http://127.0.0.1:{_free_port()}/v1", + ) + await _set_source_enabled(async_client, source_id, False) + created = await async_client.post( + "/api/api-keys/", + json={ + "name": "disabled-source-chat-key", + "limits": [ + {"limitType": "total_tokens", "limitWindow": "weekly", "maxValue": 1_000}, + ], + }, + ) + assert created.status_code == 200 + key = created.json()["key"] + + response = await async_client.post( + "/v1/chat/completions", + headers={"Authorization": f"Bearer {key}"}, + json={"model": model, "messages": [{"role": "user", "content": "hi"}]}, + ) + + assert response.status_code == 503 + error = response.json()["error"] + assert error["code"] == "model_source_disabled" + assert model in error["message"] + + # Refused before any account was selected, so nothing was dispatched and no + # reservation was taken. + async with SessionLocal() as session: + result = await session.execute(select(RequestLog).where(RequestLog.model == model)) + assert result.scalars().all() == [] + result = await session.execute( + select(ApiKeyUsageReservation).where(ApiKeyUsageReservation.status == "reserved") + ) + assert result.scalars().all() == [] + + +@pytest.mark.asyncio +async def test_disabled_source_model_row_chat_is_refused_instead_of_hitting_a_subscription(async_client): + await _enable_api_key_auth(async_client) + model = "disabled-row-chat-model" + source_id = await _create_model_source( + async_client, + name="disabled-row-chat-source", + model=model, + base_url=f"http://127.0.0.1:{_free_port()}/v1", + ) + await _disable_source_model(async_client, source_id, model) + created = await async_client.post("/api/api-keys/", json={"name": "disabled-row-chat-key"}) + assert created.status_code == 200 + key = created.json()["key"] + + response = await async_client.post( + "/v1/chat/completions", + headers={"Authorization": f"Bearer {key}"}, + json={"model": model, "messages": [{"role": "user", "content": "hi"}]}, + ) + + assert response.status_code == 503 + error = response.json()["error"] + assert error["code"] == "model_source_disabled" + assert "has that model disabled" in error["message"] + + +@pytest.mark.asyncio +@pytest.mark.parametrize("path", ["/v1/responses", "/backend-api/codex/responses"]) +async def test_disabled_source_responses_is_refused_instead_of_hitting_a_subscription(async_client, path): + await _enable_api_key_auth(async_client) + model = "disabled-source-responses-model" + source_id = await _create_model_source( + async_client, + name="disabled-responses-source", + model=model, + base_url=f"http://127.0.0.1:{_free_port()}/v1", + supports_responses=True, + ) + await _set_source_enabled(async_client, source_id, False) + created = await async_client.post("/api/api-keys/", json={"name": "disabled-source-responses-key"}) + assert created.status_code == 200 + key = created.json()["key"] + + response = await async_client.post( + path, + headers={"Authorization": f"Bearer {key}"}, + json={"model": model, "input": "hi", "stream": True}, + ) + + assert response.status_code == 503 + assert response.json()["error"]["code"] == "model_source_disabled" + + async with SessionLocal() as session: + result = await session.execute(select(RequestLog).where(RequestLog.model == model)) + assert result.scalars().all() == [] + + +@pytest.mark.asyncio +async def test_unknown_model_still_falls_through_to_subscription_routing(async_client): + """Negative control: a model no source ever claimed keeps today's path. + + The refusal above must key on "a model source owns this slug and is off", + not on "the slug is unfamiliar" -- custom subscription catalogs and alias + slugs legitimately reach the account pool. + """ + await _enable_api_key_auth(async_client) + created = await async_client.post("/api/api-keys/", json={"name": "unknown-model-key"}) + assert created.status_code == 200 + key = created.json()["key"] + + response = await async_client.post( + "/v1/chat/completions", + headers={"Authorization": f"Bearer {key}"}, + json={"model": "no-source-ever-claimed-this", "messages": [{"role": "user", "content": "hi"}]}, + ) + + # Unchanged behaviour: subscription selection runs and reports the account + # pool's own verdict. + assert response.status_code == 503 + assert response.json()["error"]["code"] == "no_accounts" + + +@pytest.mark.asyncio +async def test_disabled_source_does_not_capture_a_subscription_model_slug(async_client): + """Negative control: subscription slugs keep winning over source rows. + + An unscoped key never source-routes a slug the subscription registry + already serves, so a disabled source that happens to list that slug must + not start refusing subscription traffic. + """ + await _enable_api_key_auth(async_client) + model = "gpt-5.6-sol" + source_id = await _create_model_source( + async_client, + name="disabled-shadow-source", + model=model, + base_url=f"http://127.0.0.1:{_free_port()}/v1", + ) + await _set_source_enabled(async_client, source_id, False) + created = await async_client.post("/api/api-keys/", json={"name": "shadow-slug-key"}) + assert created.status_code == 200 + key = created.json()["key"] + + response = await async_client.post( + "/v1/chat/completions", + headers={"Authorization": f"Bearer {key}"}, + json={"model": model, "messages": [{"role": "user", "content": "hi"}]}, + ) + + assert response.json()["error"]["code"] == "no_accounts" + + @pytest.mark.asyncio async def test_patch_model_source_returns_updated_model_list(async_client): source_id = await _create_model_source( diff --git a/tests/unit/test_proxy_utils.py b/tests/unit/test_proxy_utils.py index 3984cf0939..828c8819f1 100644 --- a/tests/unit/test_proxy_utils.py +++ b/tests/unit/test_proxy_utils.py @@ -68,6 +68,7 @@ from app.modules.api_keys.service import ApiKeyData, ApiKeyUsageReservationData from app.modules.proxy import affinity as proxy_affinity from app.modules.proxy import api as proxy_api +from app.modules.proxy import helpers as proxy_helpers_module from app.modules.proxy import request_policy as proxy_request_policy from app.modules.proxy import service as proxy_service from app.modules.proxy._service import compact as proxy_compact_service @@ -278,8 +279,9 @@ async def test_process_network_failure_does_not_update_account_health() -> None: "No tool output found for function call call_abc.", True, ), - # A model-entitlement rejection is account scoped and must stay - # penalizing even though it shares the invalid_request_error code. + # A model-entitlement rejection is not a payload-shape rejection, so it + # is not a member of this narrow set. Its own health-neutrality is + # decided by ``_is_model_scoped_rejection`` instead. ( "invalid_request_error", 400, @@ -338,8 +340,101 @@ async def test_missing_tool_output_rejection_does_not_penalize_account() -> None load_balancer.mark_permanent_failure.assert_not_awaited() +@pytest.mark.parametrize( + ("message", "expected"), + [ + ( + "The 'gpt-5.3-codex' model is not supported when using Codex with a ChatGPT account.", + True, + ), + ( + "The 'qwen3-4b-instruct-2507-q8-notools:latest' model is not supported " + "when using Codex with a ChatGPT account.", + True, + ), + # Whitespace folding matches the existing entitlement matcher. + ( + "The 'gpt-5.3-codex' model is not supported\n when using Codex with a ChatGPT account.", + True, + ), + ("No tool output found for function call call_abc.", False), + ("The selected model is at capacity.", False), + ("", False), + (None, False), + ], +) +def test_is_model_scoped_upstream_rejection(message: str | None, expected: bool) -> None: + assert proxy_helpers_module.is_model_scoped_upstream_rejection(message) is expected + + +@pytest.mark.parametrize( + "code", + [ + # The live streaming path normalizes this rejection to the + # ``upstream_error`` fallback because upstream sends neither ``code`` + # nor ``type``; other paths surface ``invalid_request_error``. + "upstream_error", + "invalid_request_error", + ], +) +@pytest.mark.asyncio +async def test_model_scoped_rejection_does_not_penalize_account(code: str) -> None: + load_balancer = SimpleNamespace( + record_error=AsyncMock(), + record_errors=AsyncMock(), + mark_rate_limit=AsyncMock(), + mark_quota_exceeded=AsyncMock(), + mark_permanent_failure=AsyncMock(), + ) + proxy = SimpleNamespace(_load_balancer=load_balancer) + + classified = await streaming_helpers_module._handle_stream_error( + proxy, + cast(Account, SimpleNamespace(id="acc-1")), + { + "message": ( + "The 'qwen3-4b-instruct-2507-q8-notools:latest' model is not supported " + "when using Codex with a ChatGPT account." + ) + }, + code, + 400, + ) + + # Failover is untouched: another account may hold a different entitlement. + assert classified["error_code"] == code + load_balancer.record_error.assert_not_awaited() + load_balancer.record_errors.assert_not_awaited() + load_balancer.mark_rate_limit.assert_not_awaited() + load_balancer.mark_quota_exceeded.assert_not_awaited() + load_balancer.mark_permanent_failure.assert_not_awaited() + + +@pytest.mark.asyncio +async def test_model_scoped_rejection_classification_still_fails_over() -> None: + """The health skip must not turn the rejection into a terminal success.""" + load_balancer = SimpleNamespace( + record_error=AsyncMock(), + mark_rate_limit=AsyncMock(), + mark_quota_exceeded=AsyncMock(), + mark_permanent_failure=AsyncMock(), + ) + proxy = SimpleNamespace(_load_balancer=load_balancer) + + classified = await streaming_helpers_module._handle_stream_error( + proxy, + cast(Account, SimpleNamespace(id="acc-1")), + {"message": "The 'gpt-5.6-sol' model is not supported when using Codex with a ChatGPT account."}, + "upstream_error", + 400, + ) + + assert classified["failure_class"] == "retryable_transient" + + @pytest.mark.asyncio -async def test_account_scoped_invalid_request_error_still_penalizes_account() -> None: +async def test_genuine_upstream_error_still_penalizes_account() -> None: + """Negative control: a real ChatGPT upstream failure keeps penalizing.""" load_balancer = SimpleNamespace( record_error=AsyncMock(), mark_rate_limit=AsyncMock(), @@ -351,14 +446,59 @@ async def test_account_scoped_invalid_request_error_still_penalizes_account() -> await streaming_helpers_module._handle_stream_error( proxy, cast(Account, SimpleNamespace(id="acc-1")), - {"message": "The 'gpt-5.3-codex' model is not supported when using Codex with a ChatGPT account."}, - "invalid_request_error", + {"message": "Upstream request failed"}, + "upstream_error", 400, ) load_balancer.record_error.assert_awaited_once() +@pytest.mark.asyncio +async def test_non_400_model_rejection_message_still_penalizes_account() -> None: + """Negative control: only a genuine 400 is an entitlement rejection.""" + load_balancer = SimpleNamespace( + record_error=AsyncMock(), + mark_rate_limit=AsyncMock(), + mark_quota_exceeded=AsyncMock(), + mark_permanent_failure=AsyncMock(), + ) + proxy = SimpleNamespace(_load_balancer=load_balancer) + + await streaming_helpers_module._handle_stream_error( + proxy, + cast(Account, SimpleNamespace(id="acc-1")), + {"message": "The 'gpt-5.6-sol' model is not supported when using Codex with a ChatGPT account."}, + "server_error", + 500, + ) + + load_balancer.record_error.assert_awaited_once() + + +@pytest.mark.asyncio +async def test_rate_limit_still_marks_rate_limit() -> None: + """Negative control: quota/rate-limit accounting is unchanged.""" + load_balancer = SimpleNamespace( + record_error=AsyncMock(), + mark_rate_limit=AsyncMock(), + mark_quota_exceeded=AsyncMock(), + mark_permanent_failure=AsyncMock(), + ) + proxy = SimpleNamespace(_load_balancer=load_balancer) + + await streaming_helpers_module._handle_stream_error( + proxy, + cast(Account, SimpleNamespace(id="acc-1")), + {"message": "Rate limit reached"}, + "rate_limit_exceeded", + 429, + ) + + load_balancer.mark_rate_limit.assert_awaited_once() + load_balancer.record_error.assert_not_awaited() + + @pytest.mark.asyncio async def test_stream_idle_timeout_does_not_penalize_account() -> None: load_balancer = SimpleNamespace( diff --git a/tests/unit/test_proxy_websocket_model_source_guard.py b/tests/unit/test_proxy_websocket_model_source_guard.py index 8488f0a087..62fc04751d 100644 --- a/tests/unit/test_proxy_websocket_model_source_guard.py +++ b/tests/unit/test_proxy_websocket_model_source_guard.py @@ -424,8 +424,13 @@ async def find_responses_source_for_model( *, allowed_source_ids=None, # noqa: ANN001 require_streaming: bool = False, + only_disabled: bool = False, ): # noqa: ANN202 catalog.seen_candidates.append(candidate) + # Every source in this fake catalog is enabled, so the + # disabled-source lookup is always a miss. + if only_disabled: + return None if candidate in catalog.source_models: return SimpleNamespace(id="src_alias", name="alias-source", enabled=True) return None