fix(scopes): exclude scope memberships from the session observer limit
Scope memberships carry observe_others=true, so every scope counted against
SESSION_OBSERVERS_LIMIT (default 10) — capping scopes-per-session at the limit
minus the session's real observers, and reporting the failure as
`400 Cannot create session <name> with 11 observers. ... Observers are peers
with 'observe_others' set to true.` on a membership call. Wrong on three counts:
the ceiling is undocumented and contradicts RFC §5.1 ("sessions belong to any
number of scopes"), the message describes session creation, and it leaks the
word "observer" through a facade whose entire job is hiding observers (RFC
goal 5). The limit exists to bound per-observer deriver fan-out for real peers;
a scope costs document rows, not LLM calls (RFC §5.2), so it does not belong in
that budget.
Excluded from both halves of the check in `_get_or_add_peers_to_session`: the
incoming names via a flag-based lookup, existing memberships via a correlated
NOT EXISTS on `scope_peer_clause()` — the same pattern the replacement and
removal paths already use, so the exclusion holds regardless of concurrent
scope creation. The early `count_observers_in_config(session.peer_names)` check
in `get_or_create_session` is left alone: `peer_names` cannot contain a scope,
and `scopes` is a separate field.
`reject_scope_peers` is split into a `scope_peer_names()` query helper plus a
two-line raiser so the observer count reuses the authoritative name-AND-flag
predicate instead of growing a third copy of it. Still costs nothing on the
common path — no reserved-prefix name in the input means no query at all.
Also caps `SessionCreate.scopes` at 100, matching `ScopeSessionsAdd.session_ids`.
This belongs in the same commit: the observer limit was the only thing bounding
that list, so removing it turns an unbounded `scopes` array into a peer row and
a membership row per element, committed — the single-request path to the
cardinality anti-pattern RFC §8 warns about. Partly answers OQ6: no per-session
cap, 100 per request.
Tests: a session joins SESSION_OBSERVERS_LIMIT + 2 scopes through both the
facade and session creation; real observers over the limit still 400, so the
carve-out cannot quietly disable the limit; 101 scopes is a 422.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
14dd036e05
commit
331f6ff416
|
|
@ -193,6 +193,38 @@ async def reject_scope_observed(
|
|||
)
|
||||
|
||||
|
||||
async def scope_peer_names(
|
||||
db: AsyncSession,
|
||||
workspace_name: str,
|
||||
names: Iterable[str],
|
||||
) -> set[str]:
|
||||
"""Return the subset of ``names`` that are really scope peers (name AND flag).
|
||||
|
||||
Unlike a pure name check, a legacy peer that merely occupies the reserved
|
||||
namespace (names were length-only validated before migration
|
||||
``d429de0e5338``, so ``scope.production`` is a possible user name) is not
|
||||
reported, so it keeps its ordinary semantics instead of being locked out of
|
||||
its own data. A *missing* reserved name is likewise not reported.
|
||||
|
||||
Costs nothing on the common path: with no reserved-prefix name in ``names``
|
||||
there is no query at all.
|
||||
|
||||
Raises:
|
||||
ValidationException: On a NUL byte or an over-length name.
|
||||
"""
|
||||
candidates = await _reserved_name_candidates(names)
|
||||
if not candidates:
|
||||
return set()
|
||||
|
||||
result = await db.execute(
|
||||
select(models.Peer.name)
|
||||
.where(models.Peer.workspace_name == workspace_name)
|
||||
.where(models.Peer.name.in_(candidates))
|
||||
.where(scope_peer_clause())
|
||||
)
|
||||
return {row[0] for row in result.all()}
|
||||
|
||||
|
||||
async def reject_scope_peers(
|
||||
db: AsyncSession,
|
||||
workspace_name: str,
|
||||
|
|
@ -202,32 +234,15 @@ async def reject_scope_peers(
|
|||
) -> None:
|
||||
"""Reject peers that really are scopes, keyed off name AND flag.
|
||||
|
||||
Unlike a pure name check, a legacy peer that merely occupies the reserved
|
||||
namespace (names were length-only validated before migration
|
||||
``d429de0e5338``, so ``scope.production`` is a possible user name) keeps
|
||||
working normally instead of being locked out of its own data.
|
||||
|
||||
A *missing* reserved name passes here — the create paths this guards
|
||||
(`get_or_create_peers`) refuse it themselves. Positions where nothing creates
|
||||
the peer need ``reject_scope_observed`` instead.
|
||||
|
||||
Costs nothing on the common path: with no reserved-prefix name in ``names``
|
||||
there is no query at all.
|
||||
the peer need ``reject_scope_observed`` instead. See ``scope_peer_names`` for
|
||||
the name-vs-flag semantics.
|
||||
|
||||
Raises:
|
||||
ValidationException: If any name resolves to a real scope peer.
|
||||
"""
|
||||
candidates = await _reserved_name_candidates(names)
|
||||
if not candidates:
|
||||
return
|
||||
|
||||
result = await db.execute(
|
||||
select(models.Peer.name)
|
||||
.where(models.Peer.workspace_name == workspace_name)
|
||||
.where(models.Peer.name.in_(candidates))
|
||||
.where(scope_peer_clause())
|
||||
)
|
||||
offenders = sorted(row[0] for row in result.all())
|
||||
offenders = sorted(await scope_peer_names(db, workspace_name, names))
|
||||
if offenders:
|
||||
raise ValidationException(f"Peer name(s) {offenders} are scopes. {action}")
|
||||
|
||||
|
|
|
|||
|
|
@ -296,8 +296,6 @@ async def add_sessions_to_scope(
|
|||
Raises:
|
||||
ResourceNotFoundException: If the scope or any named session does not
|
||||
exist
|
||||
ObserverException: If a membership would exceed a session's observer
|
||||
limit
|
||||
"""
|
||||
# Imported lazily: crud.session imports this module for the session-create
|
||||
# `scopes` path, so a module-level import would be circular.
|
||||
|
|
|
|||
|
|
@ -50,6 +50,7 @@ from .peer import (
|
|||
get_peer,
|
||||
reject_scope_peers,
|
||||
scope_peer_clause,
|
||||
scope_peer_names,
|
||||
)
|
||||
from .scope import SCOPE_MEMBERSHIP_CONFIG, get_or_create_scopes
|
||||
from .workspace import get_or_create_workspace
|
||||
|
|
@ -1161,8 +1162,17 @@ async def _get_or_add_peers_to_session(
|
|||
result = await db.execute(select_stmt)
|
||||
return list(result.scalars().all())
|
||||
|
||||
# Only validate observer limit if we're adding peers with observe_others=True
|
||||
new_observer_count = count_observers_in_config(peer_names)
|
||||
# Scope memberships carry observe_others=True but do not count against the
|
||||
# limit. The limit bounds per-observer deriver fan-out for real peers; a scope
|
||||
# costs document rows, not LLM calls (Scopes RFC §5.2), and counting them would
|
||||
# cap scopes-per-session at SESSION_OBSERVERS_LIMIT and surface as an
|
||||
# observer-shaped 400 through a facade that hides observers entirely.
|
||||
scopes_being_added = await scope_peer_names(db, workspace_name, peer_names.keys())
|
||||
|
||||
# Only validate observer limit if we're adding non-scope peers with observe_others=True
|
||||
new_observer_count = count_observers_in_config(
|
||||
{n: c for n, c in peer_names.items() if n not in scopes_being_added}
|
||||
)
|
||||
|
||||
if new_observer_count > 0:
|
||||
# Use a single efficient query to count existing observers not being updated
|
||||
|
|
@ -1177,6 +1187,14 @@ async def _get_or_add_peers_to_session(
|
|||
models.SessionPeer.configuration["observe_others"].astext.cast(
|
||||
Boolean
|
||||
), # Only observers
|
||||
# Existing scope memberships are excluded for the same reason as above.
|
||||
~exists(
|
||||
select(models.Peer.id)
|
||||
.where(models.Peer.workspace_name == workspace_name)
|
||||
.where(models.Peer.name == models.SessionPeer.peer_name)
|
||||
.where(scope_peer_clause())
|
||||
.correlate(models.SessionPeer)
|
||||
),
|
||||
)
|
||||
result = await db.execute(existing_observers_stmt)
|
||||
existing_observer_count = result.scalar() or 0
|
||||
|
|
|
|||
|
|
@ -398,6 +398,7 @@ class SessionCreate(SessionBase):
|
|||
configuration: SessionConfiguration | None = None
|
||||
scopes: list[str] | None = Field(
|
||||
default=None,
|
||||
max_length=100,
|
||||
description=(
|
||||
"Optional list of (unprefixed) scope names to add this session to. "
|
||||
"Each scope is created if it does not exist yet. Note: scope "
|
||||
|
|
|
|||
|
|
@ -1332,3 +1332,93 @@ async def test_representation_rechecks_after_early_check(
|
|||
f"/v3/workspaces/{test_workspace.name}/peers/{squatter}/representation", json={}
|
||||
)
|
||||
assert response.status_code == 200, response.text
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Observer limit. Scope memberships carry observe_others=True but must not
|
||||
# consume SESSION_OBSERVERS_LIMIT: that would cap scopes-per-session at the
|
||||
# limit and report it as an observer-shaped 400 through a facade whose whole
|
||||
# job is hiding observers.
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_scopes_do_not_count_toward_observer_limit(
|
||||
client: TestClient, sample_data: tuple[Workspace, Peer]
|
||||
):
|
||||
"""A session can join more scopes than SESSION_OBSERVERS_LIMIT allows observers."""
|
||||
test_workspace, _ = sample_data
|
||||
session_name = _create_session(client, test_workspace.name)
|
||||
scope_names = [
|
||||
str(generate_nanoid()) for _ in range(settings.SESSION_OBSERVERS_LIMIT + 2)
|
||||
]
|
||||
|
||||
for scope_name in scope_names:
|
||||
assert _create_scope(client, test_workspace.name, scope_name).status_code == 201
|
||||
response = client.post(
|
||||
f"/v3/workspaces/{test_workspace.name}/scopes/{scope_name}/sessions",
|
||||
json={"session_ids": [session_name]},
|
||||
)
|
||||
assert response.status_code == 200, response.text
|
||||
|
||||
# Every membership is live, and the session-create path agrees.
|
||||
for scope_name in scope_names:
|
||||
response = client.get(
|
||||
f"/v3/workspaces/{test_workspace.name}/scopes/{scope_name}/sessions"
|
||||
)
|
||||
assert response.json()["session_ids"] == [session_name]
|
||||
|
||||
response = client.post(
|
||||
f"/v3/workspaces/{test_workspace.name}/sessions",
|
||||
json={"id": str(generate_nanoid()), "scopes": scope_names},
|
||||
)
|
||||
assert response.status_code == 201, response.text
|
||||
|
||||
|
||||
def test_observer_limit_still_applies_to_real_peers(
|
||||
client: TestClient, sample_data: tuple[Workspace, Peer]
|
||||
):
|
||||
"""The exclusion is scope-only: real observers are still capped.
|
||||
|
||||
Without this the scope carve-out could quietly disable the limit entirely.
|
||||
"""
|
||||
test_workspace, _ = sample_data
|
||||
session_name = _create_session(client, test_workspace.name)
|
||||
scope_name = str(generate_nanoid())
|
||||
assert _create_scope(client, test_workspace.name, scope_name).status_code == 201
|
||||
assert (
|
||||
client.post(
|
||||
f"/v3/workspaces/{test_workspace.name}/scopes/{scope_name}/sessions",
|
||||
json={"session_ids": [session_name]},
|
||||
).status_code
|
||||
== 200
|
||||
)
|
||||
|
||||
observers = {
|
||||
str(generate_nanoid()): {"observe_others": True}
|
||||
for _ in range(settings.SESSION_OBSERVERS_LIMIT + 1)
|
||||
}
|
||||
response = client.post(
|
||||
f"/v3/workspaces/{test_workspace.name}/sessions/{session_name}/peers",
|
||||
json=observers,
|
||||
)
|
||||
assert response.status_code == 400, response.text
|
||||
|
||||
|
||||
def test_session_create_scopes_list_is_capped(
|
||||
client: TestClient, sample_data: tuple[Workspace, Peer]
|
||||
):
|
||||
"""`scopes` is bounded like `session_ids` on the add-sessions route.
|
||||
|
||||
Nothing downstream bounds it now that scopes are outside the observer limit,
|
||||
so an unbounded list would mint a scope peer per element in one request.
|
||||
"""
|
||||
test_workspace, _ = sample_data
|
||||
|
||||
response = client.post(
|
||||
f"/v3/workspaces/{test_workspace.name}/sessions",
|
||||
json={
|
||||
"id": str(generate_nanoid()),
|
||||
"scopes": [str(generate_nanoid()) for _ in range(101)],
|
||||
},
|
||||
)
|
||||
assert response.status_code == 422, response.text
|
||||
|
|
|
|||
Loading…
Reference in New Issue