From 331f6ff4166af0a54ca96f5a0912d9f5d46bcd09 Mon Sep 17 00:00:00 2001 From: Vineeth Voruganti <13438633+VVoruganti@users.noreply.github.com> Date: Wed, 29 Jul 2026 22:52:31 -0400 Subject: [PATCH] fix(scopes): exclude scope memberships from the session observer limit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 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) --- src/crud/peer.py | 55 ++++++++++++++--------- src/crud/scope.py | 2 - src/crud/session.py | 22 ++++++++- src/schemas/api.py | 1 + tests/routes/test_scopes.py | 90 +++++++++++++++++++++++++++++++++++++ 5 files changed, 146 insertions(+), 24 deletions(-) diff --git a/src/crud/peer.py b/src/crud/peer.py index 37c2caa2..4cccd263 100644 --- a/src/crud/peer.py +++ b/src/crud/peer.py @@ -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}") diff --git a/src/crud/scope.py b/src/crud/scope.py index 7bd554c9..c4950ef4 100644 --- a/src/crud/scope.py +++ b/src/crud/scope.py @@ -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. diff --git a/src/crud/session.py b/src/crud/session.py index 2f508b59..be1d1647 100644 --- a/src/crud/session.py +++ b/src/crud/session.py @@ -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 diff --git a/src/schemas/api.py b/src/schemas/api.py index 3040cee3..1d345ff8 100644 --- a/src/schemas/api.py +++ b/src/schemas/api.py @@ -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 " diff --git a/tests/routes/test_scopes.py b/tests/routes/test_scopes.py index 1f17d5d5..59149d55 100644 --- a/tests/routes/test_scopes.py +++ b/tests/routes/test_scopes.py @@ -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