fix(usage): drop redundant GatewayProvider.embed_batch override (round 3)
Round-3 claude-review finding: - 🟡 Double _ensure_bearer() on gateway.embed_batch(). Round 1 made OpenAIProvider.embed_batch() delegate to embed_batch_with_usage(); because GatewayProvider overrode both embed_batch() and embed_batch_with_usage() (each calling _ensure_bearer), gateway.embed_batch() refreshed the bearer twice (the second a cache-hit no-op). Remove the now-redundant embed_batch() override: OpenAI's embed_batch() routes through embed_batch_with_usage(), which the gateway still overrides, so the bearer refreshes exactly once on every path. The remaining two overrides (embed + embed_batch_with_usage) cover all four entrypoints; documented the topology. - 🟢 Added test_gateway_embed_batch_ensures_bearer_once locking in the single refresh. Cohere token-fallback (🟢 nit) is already covered by test_bedrock_with_usage_estimates_when_token_count_absent. Deck #67. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.8
parent
d15ce627ab
commit
9ac9e1ab09
@@ -217,22 +217,21 @@ class GatewayProvider(OpenAIProvider):
|
|||||||
exc,
|
exc,
|
||||||
)
|
)
|
||||||
|
|
||||||
|
# Bearer-refresh override topology. OpenAIProvider routes embed_batch(),
|
||||||
|
# embed_with_usage() and embed_batch_with_usage() all through
|
||||||
|
# embed_batch_with_usage(); only embed() (single) is self-contained. So we
|
||||||
|
# override exactly two methods to refresh the bearer exactly once on every
|
||||||
|
# path: embed() (its own entrypoint) and embed_batch_with_usage() (the
|
||||||
|
# shared funnel for the other three). Overriding embed_batch() as well would
|
||||||
|
# double-call _ensure_bearer() (override → super().embed_batch() →
|
||||||
|
# self.embed_batch_with_usage() → override again).
|
||||||
|
|
||||||
async def embed(self, text: str) -> list[float]:
|
async def embed(self, text: str) -> list[float]:
|
||||||
await self._ensure_bearer()
|
await self._ensure_bearer()
|
||||||
return await super().embed(text)
|
return await super().embed(text)
|
||||||
|
|
||||||
async def embed_batch(self, texts: list[str]) -> list[list[float]]:
|
|
||||||
await self._ensure_bearer()
|
|
||||||
return await super().embed_batch(texts)
|
|
||||||
|
|
||||||
async def embed_batch_with_usage(
|
async def embed_batch_with_usage(
|
||||||
self, texts: list[str]
|
self, texts: list[str]
|
||||||
) -> tuple[list[list[float]], int]:
|
) -> tuple[list[list[float]], int]:
|
||||||
# Only the batch usage-variant is overridden: OpenAIProvider's
|
|
||||||
# embed_with_usage() routes through embed_batch_with_usage(), so a
|
|
||||||
# single embed_with_usage() call already lands here and refreshes the
|
|
||||||
# bearer exactly once (overriding both would double-ensure). This
|
|
||||||
# differs from embed()/embed_batch() above, where the single embed() is
|
|
||||||
# self-contained and therefore needs its own override.
|
|
||||||
await self._ensure_bearer()
|
await self._ensure_bearer()
|
||||||
return await super().embed_batch_with_usage(texts)
|
return await super().embed_batch_with_usage(texts)
|
||||||
|
|||||||
@@ -393,6 +393,37 @@ async def test_gateway_embed_batch_with_usage_forwards_after_bearer(monkeypatch)
|
|||||||
assert ensured["n"] == 1
|
assert ensured["n"] == 1
|
||||||
|
|
||||||
|
|
||||||
|
async def test_gateway_embed_batch_ensures_bearer_once(monkeypatch):
|
||||||
|
"""embed_batch() has no override: it routes through the inherited OpenAI
|
||||||
|
embed_batch() → embed_batch_with_usage() (overridden), so the bearer is
|
||||||
|
refreshed exactly once — not twice."""
|
||||||
|
# https mock host (never contacted — the OpenAI client is patched below).
|
||||||
|
provider = GatewayProvider(
|
||||||
|
base_url="https://gw:8083/v1", embedding_model="mistral/mistral-embed"
|
||||||
|
)
|
||||||
|
ensured = {"n": 0}
|
||||||
|
|
||||||
|
async def _ensure_bearer():
|
||||||
|
ensured["n"] += 1
|
||||||
|
|
||||||
|
monkeypatch.setattr(provider, "_ensure_bearer", _ensure_bearer)
|
||||||
|
|
||||||
|
item = MagicMock()
|
||||||
|
item.embedding = [0.1, 0.2]
|
||||||
|
item.index = 0
|
||||||
|
response = MagicMock()
|
||||||
|
response.data = [item]
|
||||||
|
response.usage = MagicMock(total_tokens=4)
|
||||||
|
monkeypatch.setattr(
|
||||||
|
provider.client.embeddings, "create", AsyncMock(return_value=response)
|
||||||
|
)
|
||||||
|
|
||||||
|
embeddings = await provider.embed_batch(["x"])
|
||||||
|
|
||||||
|
assert embeddings == [[0.1, 0.2]]
|
||||||
|
assert ensured["n"] == 1 # not 2 — embed_batch() must not double-refresh
|
||||||
|
|
||||||
|
|
||||||
async def test_detect_dimension_with_bare_base_url_hits_v1_models(monkeypatch):
|
async def test_detect_dimension_with_bare_base_url_hits_v1_models(monkeypatch):
|
||||||
"""End-to-end of the fix: a bare-origin base_url still resolves the
|
"""End-to-end of the fix: a bare-origin base_url still resolves the
|
||||||
dimension because discovery lands on /v1/models."""
|
dimension because discovery lands on /v1/models."""
|
||||||
|
|||||||
Reference in New Issue
Block a user