fix(scopes): skip semantic retrieval when the embedding precompute failed
Addresses three open review comments. 1. Major — the representation read could embed inside its DB session. The route's precompute is suppressed, and both `RepresentationManager.get_working_representation` and `crud.query_documents` fall back to embedding when a query arrives without one, so a failed precompute meant an external call inside the read session this branch opens for the scope re-check — the connection-holding rule the route's own comment claimed to satisfy. The innermost fallback also only catches ValueError, so a provider outage surfaced as a 500. The semantic query is now passed only when an embedding exists, degrading to derived+recent retrieval. (`crud.query_documents` embedding inside a caller's session predates this branch and is left alone.) 2. Minor — `test_resolved_scope_peer_rejected_at_membership_upsert` described a race it does not perform. It creates an already-flagged scope and calls crud directly; the unflagged → flagged transition is not simulated. Docstring now says what the test actually pins. 3. Minor — `test_empty_replacement_preserves_scope_membership` asserted only half its docstring. It passed if the empty PUT left every ordinary membership intact; now asserts the ordinary peer's left_at is set. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
331f6ff416
commit
3359a3771b
|
|
@ -440,7 +440,17 @@ async def get_representation(
|
|||
session_allowlist=[options.session_id]
|
||||
if options.session_id is not None
|
||||
else session_allowlist,
|
||||
include_semantic_query=options.search_query,
|
||||
# Only ask for the semantic branch when we actually have an
|
||||
# embedding. The precompute above is suppressed, and both
|
||||
# `RepresentationManager.get_working_representation` and
|
||||
# `crud.query_documents` fall back to embedding internally when a
|
||||
# query arrives without one — which would run an external call
|
||||
# inside this session, and the innermost fallback is unsuppressed
|
||||
# (a provider outage would surface as a 500). Degrading to
|
||||
# derived+recent retrieval keeps the session DB-only.
|
||||
include_semantic_query=options.search_query
|
||||
if embedding is not None
|
||||
else None,
|
||||
embedding=embedding,
|
||||
semantic_search_top_k=options.search_top_k,
|
||||
semantic_search_max_distance=options.search_max_distance,
|
||||
|
|
|
|||
|
|
@ -945,9 +945,10 @@ async def test_resolved_scope_peer_rejected_at_membership_upsert(
|
|||
):
|
||||
"""The last-line guard runs on resolved rows, closing the check-then-upsert race.
|
||||
|
||||
Simulates the race by flagging the peer *after* the route-level name check
|
||||
would have passed: the peer exists and is unflagged when named, and is a real
|
||||
scope by the time membership is upserted.
|
||||
Calls crud directly, bypassing the route-level name check, so the only thing
|
||||
standing between the caller and a scope membership is the guard on the
|
||||
resolved peer row — the guard the racing caller would hit. The unflagged →
|
||||
flagged transition itself is not simulated here.
|
||||
"""
|
||||
test_workspace, _ = sample_data
|
||||
scope_name = str(generate_nanoid())
|
||||
|
|
@ -1116,8 +1117,10 @@ def test_generic_replacement_preserves_scope_membership(
|
|||
assert response.json()["session_ids"] == [session_name]
|
||||
|
||||
|
||||
def test_empty_replacement_preserves_scope_membership(
|
||||
client: TestClient, sample_data: tuple[Workspace, Peer]
|
||||
async def test_empty_replacement_preserves_scope_membership(
|
||||
client: TestClient,
|
||||
db_session: AsyncSession,
|
||||
sample_data: tuple[Workspace, Peer],
|
||||
):
|
||||
"""An empty replacement map clears ordinary peers but not scopes."""
|
||||
test_workspace, test_peer = sample_data
|
||||
|
|
@ -1146,6 +1149,14 @@ def test_empty_replacement_preserves_scope_membership(
|
|||
)
|
||||
assert response.json()["session_ids"] == [session_name]
|
||||
|
||||
# The other half of the docstring: without this the test passes even if the
|
||||
# empty replacement became a no-op for ordinary peers too.
|
||||
session_peer = await _get_session_peer(
|
||||
db_session, test_workspace.name, session_name, test_peer.name
|
||||
)
|
||||
assert session_peer is not None
|
||||
assert session_peer.left_at is not None
|
||||
|
||||
|
||||
async def test_replacement_still_removes_unflagged_squatter(
|
||||
client: TestClient,
|
||||
|
|
|
|||
Loading…
Reference in New Issue