fix: address PR #814 reviewer follow-ups
- gateway_client: guard token cache with a lazy anyio.Lock so concurrent embed calls share one M2M token request instead of racing - status subscriber: distinguish idle fetch timeouts from real broker errors (log + 5s backoff) instead of swallowing all and spinning - nats: warn when the bus URL uses unencrypted transport (non-tls://) - collection_metadata: accept an optional shared httpx client, make TLS verify explicit, document the unauthenticated control-plane contract - replace python -O-stripped asserts with explicit ValueError in the bus status builder and the api metadata source - document why the nil-UUID sentinel point can't collide with content ids 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
b5ed1e3b4d
commit
2d845cb70f
@@ -31,6 +31,9 @@ logger = logging.getLogger(__name__)
|
||||
|
||||
# Deterministic sentinel point id (design §10.1). Carries collection metadata
|
||||
# and never matches a search (no user_id/doc_id/doc_type payload to match).
|
||||
# The nil UUID is used deliberately: real chunk point ids are UUID5s derived
|
||||
# from (namespace, doc_id, chunk_index), so the all-zero id can never collide
|
||||
# with a content point — and it is trivially recognisable in Qdrant's UI.
|
||||
SENTINEL_POINT_ID = "00000000-0000-0000-0000-000000000000"
|
||||
|
||||
# Sentinel payload keys.
|
||||
@@ -112,29 +115,67 @@ async def _read_from_qdrant(
|
||||
}
|
||||
|
||||
|
||||
async def _read_from_api(api_url: str, collection_name: str) -> dict[str, Any] | None:
|
||||
async def _read_from_api(
|
||||
api_url: str,
|
||||
collection_name: str,
|
||||
*,
|
||||
client: httpx.AsyncClient | None = None,
|
||||
) -> dict[str, Any] | None:
|
||||
"""GET the control-plane metadata for a collection.
|
||||
|
||||
``client`` lets the caller pass a shared, lifespan-managed
|
||||
:class:`httpx.AsyncClient`; when omitted we create a short-lived one.
|
||||
|
||||
Auth note: the control-plane metadata endpoint is currently unauthenticated
|
||||
(read-only collection identity, no tenant secrets), matching the gateway's
|
||||
present state. When the control plane gains M2M auth this should reuse the
|
||||
same OIDC credentials as ``GatewayTokenProvider`` (design §10.2).
|
||||
"""
|
||||
url = f"{api_url.rstrip('/')}/v1/qdrant-collections/{collection_name}/metadata"
|
||||
async with httpx.AsyncClient(timeout=httpx.Timeout(10.0, connect=5.0)) as client:
|
||||
resp = await client.get(url)
|
||||
|
||||
async def _do(c: httpx.AsyncClient) -> dict[str, Any] | None:
|
||||
resp = await c.get(url)
|
||||
if resp.status_code == 404:
|
||||
return None
|
||||
resp.raise_for_status()
|
||||
return resp.json()
|
||||
|
||||
if client is not None:
|
||||
return await _do(client)
|
||||
# httpx verifies TLS by default (verify=True); stated here to be explicit.
|
||||
async with httpx.AsyncClient(
|
||||
timeout=httpx.Timeout(10.0, connect=5.0), verify=True
|
||||
) as owned:
|
||||
return await _do(owned)
|
||||
|
||||
|
||||
async def read_collection_metadata(
|
||||
client: AsyncQdrantClient,
|
||||
collection_name: str,
|
||||
settings: Settings | None = None,
|
||||
*,
|
||||
http_client: httpx.AsyncClient | None = None,
|
||||
) -> dict[str, Any]:
|
||||
"""Read collection metadata from the configured source, falling back to env
|
||||
defaults on any miss/error (preserves query availability — §10.1)."""
|
||||
defaults on any miss/error (preserves query availability — §10.1).
|
||||
|
||||
``http_client`` is forwarded to the ``api`` source so callers on the query
|
||||
path can share a lifespan-managed client instead of opening one per read.
|
||||
"""
|
||||
s = settings or get_settings()
|
||||
meta: dict[str, Any] | None = None
|
||||
try:
|
||||
if s.collection_metadata_source == "api":
|
||||
assert s.collection_metadata_api_url is not None
|
||||
meta = await _read_from_api(s.collection_metadata_api_url, collection_name)
|
||||
# Defence-in-depth (robust under ``python -O``): __post_init__
|
||||
# guarantees the URL when COLLECTION_METADATA_SOURCE=api.
|
||||
if s.collection_metadata_api_url is None:
|
||||
raise ValueError(
|
||||
"COLLECTION_METADATA_SOURCE=api requires "
|
||||
"COLLECTION_METADATA_API_URL"
|
||||
)
|
||||
meta = await _read_from_api(
|
||||
s.collection_metadata_api_url, collection_name, client=http_client
|
||||
)
|
||||
else:
|
||||
meta = await _read_from_qdrant(client, collection_name)
|
||||
except Exception:
|
||||
|
||||
@@ -29,6 +29,22 @@ STREAM_NAME = "mcp"
|
||||
INGEST_SUBJECT_PREFIX = "mcp.ingest.requested"
|
||||
|
||||
|
||||
def warn_if_insecure_nats_url(url: str) -> None:
|
||||
"""Log a warning when the bus URL is not TLS-encrypted.
|
||||
|
||||
``nats://`` (and ``ws://``) carry tenant document metadata in cleartext;
|
||||
production deployments should use ``tls://`` (or ``wss://``). We connect
|
||||
regardless — this is an operator alert, not a hard failure.
|
||||
"""
|
||||
scheme = url.split("://", 1)[0].lower()
|
||||
if scheme not in ("tls", "wss"):
|
||||
logger.warning(
|
||||
"NATS bus URL uses unencrypted transport (scheme=%s://); "
|
||||
"use tls:// in production to protect document metadata in transit",
|
||||
scheme,
|
||||
)
|
||||
|
||||
|
||||
def _modified_at_rfc3339(modified_at: int) -> str:
|
||||
"""DocumentTask.modified_at is an epoch int (0 for deletes)."""
|
||||
return datetime.fromtimestamp(int(modified_at), tz=timezone.utc).isoformat()
|
||||
@@ -74,6 +90,7 @@ class NatsTaskProducer:
|
||||
) -> NatsTaskProducer:
|
||||
import nats # noqa: PLC0415 (lazy: optional dependency for external mode)
|
||||
|
||||
warn_if_insecure_nats_url(url)
|
||||
nc = await nats.connect(url)
|
||||
js = nc.jetstream()
|
||||
await cls._ensure_stream(js, num_replicas)
|
||||
|
||||
@@ -126,6 +126,9 @@ class NatsStatusSubscriber:
|
||||
) -> NatsStatusSubscriber:
|
||||
import nats # noqa: PLC0415
|
||||
|
||||
from .nats import warn_if_insecure_nats_url # noqa: PLC0415
|
||||
|
||||
warn_if_insecure_nats_url(url)
|
||||
nc = await nats.connect(url)
|
||||
js = nc.jetstream()
|
||||
return cls(nc, js, tenant_id, store)
|
||||
@@ -138,6 +141,7 @@ class NatsStatusSubscriber:
|
||||
) -> None:
|
||||
"""Durable pull-consumer loop. Requires a live broker (integration)."""
|
||||
import anyio # noqa: PLC0415
|
||||
import nats.errors # noqa: PLC0415
|
||||
|
||||
subject = f"mcp.document.*.{self.tenant_id}"
|
||||
sub = await self._js.pull_subscribe(
|
||||
@@ -148,10 +152,17 @@ class NatsStatusSubscriber:
|
||||
while not shutdown_event.is_set():
|
||||
try:
|
||||
msgs = await sub.fetch(batch=16, timeout=5)
|
||||
except nats.errors.TimeoutError:
|
||||
# Expected when idle: no messages within the fetch window. Loop
|
||||
# straight back to re-check shutdown — no log, no extra sleep.
|
||||
continue
|
||||
except Exception:
|
||||
# fetch timeout when idle — brief pause before re-checking
|
||||
# shutdown, avoiding a tight check-and-sleep spin on quiet tenants.
|
||||
await anyio.sleep(0.1)
|
||||
# Real broker error (disconnect, auth failure, stream deleted).
|
||||
# Log it and back off so we don't hot-spin against a dead broker.
|
||||
logger.warning(
|
||||
"NATS status subscriber fetch failed; retrying", exc_info=True
|
||||
)
|
||||
await anyio.sleep(5)
|
||||
continue
|
||||
for msg in msgs:
|
||||
self.handle_message(msg.subject, msg.data)
|
||||
|
||||
Reference in New Issue
Block a user