diff --git a/backend/app/feedquery.py b/backend/app/feedquery.py index 5fec78f6..4138b9ea 100644 --- a/backend/app/feedquery.py +++ b/backend/app/feedquery.py @@ -719,8 +719,11 @@ _GOLDEN = 0.6180339887498949 def shuffle_start(seed: int) -> float: - """The point in [0, 1) where a seed's shuffle starts on the ring of `Video.shuffle_key`.""" - return (seed * _GOLDEN) % 1.0 + """The point in [0, 1) where a seed's shuffle starts on the ring of `Video.shuffle_key`. + + Folded to 32 bits first: an int past a float's range overflowed the product, and one past 2**53 + left the product no fraction to land on. A seed the client draws (under a billion) is unchanged.""" + return ((seed % 2**32) * _GOLDEN) % 1.0 def shuffle_walk( @@ -787,7 +790,7 @@ def feed_page( if vals is not None and cursor_id is not None: try: after = (int(vals[0]), float(vals[1]), str(cursor_id)) - except (TypeError, ValueError) as e: + except (TypeError, ValueError, OverflowError) as e: # Infinity, a 400-digit key raise BadCursor("Invalid feed cursor") from e rows = shuffle_walk(db, query, seed, limit + 1, after) else: diff --git a/backend/app/routes/_common.py b/backend/app/routes/_common.py index 6f904dad..32e1028f 100644 --- a/backend/app/routes/_common.py +++ b/backend/app/routes/_common.py @@ -6,9 +6,9 @@ and "fetch a row this user owns or 404" idioms have one definition, one status c one existence-leak policy. """ -from typing import Any, Protocol, TypeVar +from typing import Annotated, Any, Protocol, TypeVar -from fastapi import HTTPException, Request +from fastapi import HTTPException, Query, Request from pydantic import BaseModel, ConfigDict, Field from sqlalchemy.orm import Session @@ -21,6 +21,10 @@ from app.youtube.client import YouTubeError, refusal_reason M = TypeVar("M", bound=Base) +# A shuffle / surprise seed off the query string. The client draws one under a billion; bounded so a +# hand-typed 400-digit one is a 422, not an int no float or bigint can hold further down. +Seed = Annotated[int, Query(ge=-(2**53), le=2**53)] + # Resume-position thresholds of a player that only keeps a resume point (the YouTube player, the # library's): positions below this are "didn't really start" and within this of the end are # "basically finished" — neither is worth storing, so the position is cleared instead (no progress diff --git a/backend/app/routes/discover.py b/backend/app/routes/discover.py index a180254a..ab574095 100644 --- a/backend/app/routes/discover.py +++ b/backend/app/routes/discover.py @@ -23,7 +23,7 @@ from app.discover import pins, rails from app.discover.budget import Budget, DEFAULT_BUDGET from app.models import User from app.plex.cards import norm_lang -from app.routes._common import not_found +from app.routes._common import Seed, not_found router = APIRouter(prefix="/api/discover", tags=["discover"]) @@ -44,7 +44,7 @@ class ReorderIn(BaseModel): @router.get("") def get_discover( budget: Budget = DEFAULT_BUDGET, - seed: int = 0, + seed: Seed = 0, lang: str = "en", user: User = Depends(current_user), db: Session = Depends(get_db), @@ -79,7 +79,7 @@ MAX_EXCLUDE = 32 @router.get("/pick") def get_pick( budget: Budget = DEFAULT_BUDGET, - seed: int = 0, + seed: Seed = 0, exclude: str = "", start: int = 0, lang: str = "en", diff --git a/backend/app/routes/feed.py b/backend/app/routes/feed.py index 237cd8f7..260175f9 100644 --- a/backend/app/routes/feed.py +++ b/backend/app/routes/feed.py @@ -20,6 +20,7 @@ from app.models import User from app.reader import queries, service, takeout from app.routes._common import ( ProgressIn, + Seed, capped_body, drain_body, kept_position, @@ -89,7 +90,7 @@ def _feed_params( def get_feed( params: dict = Depends(_feed_params), sort: str = "newest", - seed: int = 0, + seed: Seed = 0, limit: int = Query(default=60, le=200), cursor: str | None = None, user: User = Depends(current_user), diff --git a/backend/tests/test_shuffle_walk.py b/backend/tests/test_shuffle_walk.py index 6c2b70a1..bb19ed01 100644 --- a/backend/tests/test_shuffle_walk.py +++ b/backend/tests/test_shuffle_walk.py @@ -6,10 +6,12 @@ a window of it and shuffles that window by the seed (`discover.queries.yt_surpri scope by md5(id || seed) cost ~350 ms per call on prod and grew with every Shuffle page. """ +import base64 +import random from contextlib import contextmanager from datetime import datetime, timedelta, timezone -from sqlalchemy import event +from sqlalchemy import event, insert, text from app import feedquery from app.catalog.models import Channel, Subscription, Video @@ -125,6 +127,51 @@ def test_neither_draw_orders_the_scope_by_a_hash(db, make_user): assert "ORDER BY videos.shuffle_key, videos.id" in s.replace("\n", " "), s +def test_the_walk_reads_the_ring_index_and_sorts_nothing(db, make_user): + # The text of the query is not the plan: each arc must be served by ix_videos_shuffle_key in key + # order (an index on another shape, or none, leaves a Sort of the whole scope). A scope of a few + # thousand videos, analysed, so the planner weighs it as it does a real one. + user = _seed(db, make_user) + rng = random.Random(7) + db.execute( + insert(Video), + [ + { + "id": f"bulk{i:05}", + "channel_id": "UCring", + "title": "b", + "duration_seconds": 600, + "published_at": NOW - timedelta(minutes=i), + "shuffle_key": rng.random(), + } + for i in range(4000) + ], + ) + db.commit() + db.execute(text("ANALYZE videos")) + query, _ = feedquery.filtered_query(db, user, **_FEED_BASE) + captured: list[tuple[str, object]] = [] + + def grab(conn, cursor, statement, params, *args): + if "FROM videos" in statement and "LIMIT" in statement: + captured.append((statement, params)) + + engine = db.get_bind() + event.listen(engine, "before_cursor_execute", grab) + try: + seed = _seed_landing_between(0.45, 0.55) + feedquery.shuffle_walk(db, query, seed, 6) # the first arc, from the seed's point + feedquery.shuffle_walk(db, query, seed, 6, (1, 0.3, "bulk00001")) # the wrap, resumed + finally: + event.remove(engine, "before_cursor_execute", grab) + assert len(captured) == 2 + conn = db.connection() + for statement, params in captured: + plan = "\n".join(r[0] for r in conn.exec_driver_sql("EXPLAIN " + statement, params)) + assert "ix_videos_shuffle_key" in plan, plan + assert "Sort Key: videos.shuffle_key" not in plan, plan + + def test_the_surprise_reads_a_window_of_the_ring(db, make_user, monkeypatch): user = _seed(db, make_user) monkeypatch.setattr(queries, "SURPRISE_WINDOW", 3) @@ -143,6 +190,31 @@ def test_a_wide_gap_on_the_ring_does_not_make_a_favourite(db, make_user): assert leads.count("lonely") < 15, leads.count("lonely") +def test_any_seed_lands_on_the_ring(): + # An int too large for a float overflowed the multiplication: a 500 for a hand-typed seed. + for seed in (10**400, -(10**400), 2**53 + 1, -7): + assert 0 <= feedquery.shuffle_start(seed) < 1, seed + + +def test_a_seed_out_of_range_is_refused_not_a_500(db, make_user, as_user): + client = as_user(_seed(db, make_user)) + huge = "9" * 400 + for url in ( + f"/api/feed?sort=shuffle&seed={huge}", + f"/api/discover?seed={huge}", + f"/api/discover/pick?seed={huge}", + ): + assert client.get(url).status_code == 422, url + + +def test_a_shuffle_cursor_past_a_float_is_a_bad_cursor(db, make_user, as_user): + client = as_user(_seed(db, make_user)) + for k in ("[Infinity, 0.5]", f"[0, {'9' * 400}]"): + raw = '{"k": %s, "id": "k20"}' % k + cursor = base64.urlsafe_b64encode(raw.encode()).decode() + assert client.get(f"/api/feed?sort=shuffle&seed=1&cursor={cursor}").status_code == 400, k + + def test_a_new_video_gets_its_own_key(db, make_user): _seed(db, make_user, {}) db.add_all([Video(id=f"n{i}", channel_id="UCring", title="n") for i in range(3)])