diff --git a/backend/app/db.py b/backend/app/db.py index 51a06301..42f585bb 100644 --- a/backend/app/db.py +++ b/backend/app/db.py @@ -34,3 +34,31 @@ def get_db() -> Iterator[Session]: yield db finally: db.close() + + +def release_read_connection(db: Session) -> None: + """End a request's READ transaction so its pooled connection goes back BEFORE a long await. + + SQLAlchemy 2.0 autobegins on the first SELECT, so a request that has merely authenticated is + already holding one of the connections above — `idle in transaction` to Postgres, and nothing + times it out. A route that then awaits something slow (a request body still arriving, an + external fetch) pins it for the whole wait, which is precisely the invariant the pool is sized + on in the comment above. Measured: the next checkout after this call reuses the very backend + that was released, and a 1.8 s upload goes from one sustained `idle in transaction` to none. + + Every route that reads its own body from a dependency needs this, and `test_body_limit.py` + holds that — the three that do are the ones with a raised (or self-imposed) body ceiling, and + they got here one at a time, which is how the upload route ended up the only one fixed. + + The caller must have NOTHING PENDING, and that is checked rather than described. The session is + shared with every dependency that ran before, so a rollback here would silently discard a write + one of them staged — `service.enqueue` already learned that lesson the expensive way and uses a + SAVEPOINT for it. CLAUDE.md: a rule stated in a comment must also be enforced in code. + """ + if db.new or db.deleted or db.dirty: + raise RuntimeError( + "release_read_connection() with pending work: something ahead of this dependency " + "staged a write, which ending the transaction would discard. Commit it first, or do " + "it after the await." + ) + db.rollback() diff --git a/backend/app/downloads/pipeline.py b/backend/app/downloads/pipeline.py index d61e99b6..22c7c1ac 100644 --- a/backend/app/downloads/pipeline.py +++ b/backend/app/downloads/pipeline.py @@ -25,7 +25,7 @@ from app.downloads.jobdb import ( ) from app.downloads.stated import StatedFailure from app.downloads.status import JobStatus -from app.models import DownloadJob, MediaAsset +from app.models import DISPLAY_TEXT_MAX, DownloadJob, MediaAsset from app.notifications import create_notification from app.titles import normalize_title @@ -597,7 +597,7 @@ def _download( } if job and not job.display_name and meta.title: # display_name is VARCHAR(255) — clip a long title (see _fill_asset_meta_early). - fields["display_name"] = meta.title[:255] + fields["display_name"] = meta.title[:DISPLAY_TEXT_MAX] # Atomic terminal write: finalizes ONLY while the row is still running, so a child that # finishes just after a route cancel/pause can't resurrect it (see finalize_if_running). finalized = service.finalize_if_running(db, job_id, **fields) diff --git a/backend/app/downloads/service.py b/backend/app/downloads/service.py index c10676a9..f643d04d 100644 --- a/backend/app/downloads/service.py +++ b/backend/app/downloads/service.py @@ -20,7 +20,7 @@ from app.downloads import credentials from app.downloads import edit as editmod from app.downloads import formats, mediakind, quota, storage from app.downloads.status import HOLDING, JobStatus -from app.models import Channel, DownloadJob, MediaAsset, Video +from app.models import DISPLAY_TEXT_MAX, Channel, DownloadJob, MediaAsset, Video class EditError(Exception): @@ -31,26 +31,6 @@ class EditError(Exception): super().__init__(reason) -# The width of every user-supplied display string on a job (`DownloadJob.display_name`, -# `display_uploader`): `String(255)`. `test_upload.py` holds this against the columns -# themselves so the two cannot drift — a number in a comment is exactly how that drift happens. -DISPLAY_TEXT_MAX = 255 - - -def display_text(raw: str | None) -> str | None: - """A user-supplied display string on its way into one of those columns: stripped, truncated, - blank → None. - - One definition because there are three writers and they had drifted: the metadata PATCH - truncated, the upload ingest did not, and since P1 the upload's name arrives in a REQUEST HEADER - where nothing bounds its length — so a long enough one reached Postgres as a `DataError`, i.e. a - 500 after the whole file had already been transferred. Truncating is the right answer rather - than refusing: the name is a label on a file the user has already sent, and losing the tail of it - is not worth failing the upload over. - """ - return (raw or "").strip()[:DISPLAY_TEXT_MAX] or None - - def source_url(source_kind: str, source_ref: str) -> str: """The URL yt-dlp should fetch. YouTube stores the 11-char id (the adapter reconstructs the watch URL); ad-hoc sources store the URL verbatim.""" @@ -199,7 +179,7 @@ def enqueue( profile_id=profile_id, profile_snapshot=spec, display_name=display_name - or (asset.title[:255] if asset.status == "ready" and asset.title else None), + or (asset.title[:DISPLAY_TEXT_MAX] if asset.status == "ready" and asset.title else None), status="queued", queue_pos=next_queue_pos(db), ) @@ -218,10 +198,13 @@ def enqueue( def _clip_title(source_title: str | None, display_name: str | None) -> str: + # Clipped to DISPLAY_TEXT_MAX, not to a literal: this feeds `DownloadJob.display_name` through + # `enqueue_edit`, the same column `display_text` guards. A machine-made title, so it is clipped + # rather than run through `display_text` — blank here means "no title", not None. if display_name: - return display_name[:255] + return display_name[:DISPLAY_TEXT_MAX] base = source_title or "Clip" - return f"{base} (clip)"[:255] + return f"{base} (clip)"[:DISPLAY_TEXT_MAX] def enqueue_edit( @@ -275,7 +258,7 @@ def enqueue_edit( source_asset_id=src_asset.id, edit_spec=spec, profile_snapshot={}, - display_name=display_name or asset.title, + display_name=display_name or asset.title[:DISPLAY_TEXT_MAX], status="queued", queue_pos=next_queue_pos(db), ) diff --git a/backend/app/downloads/upload.py b/backend/app/downloads/upload.py index 70acf329..1c91421e 100644 --- a/backend/app/downloads/upload.py +++ b/backend/app/downloads/upload.py @@ -25,7 +25,7 @@ from sqlalchemy.orm import Session from app.config import settings from app.downloads import edit as editmod from app.downloads import mediakind, probe, quota, service, storage -from app.models import DownloadJob, MediaAsset +from app.models import DownloadJob, MediaAsset, display_text def _title_from_filename(filename: str) -> str: @@ -99,7 +99,7 @@ def ingest_upload( source_kind="upload", source_ref=ref, profile_snapshot={}, - display_name=service.display_text(display_name), + display_name=display_text(display_name), status="done", progress=100, queue_pos=service.next_queue_pos(db), diff --git a/backend/app/models.py b/backend/app/models.py index 3700f58a..5677c594 100644 --- a/backend/app/models.py +++ b/backend/app/models.py @@ -23,6 +23,34 @@ from sqlalchemy.dialects.postgresql import JSONB, TSVECTOR from sqlalchemy.orm import Mapped, mapped_column, relationship, validates from app.db import Base + +# Every user-supplied display string in this schema shares one width, and it lives HERE so the +# columns below are declared with it rather than with a literal: a name a person typed goes into +# `User.display_name` (their Google profile name), `DownloadJob.display_name` and +# `DownloadJob.display_uploader`. A value wider than the column is a psycopg DataError, which +# `main.py` re-raises for everything but the NUL case — i.e. a bare 500 for a name somebody pasted. +DISPLAY_TEXT_MAX = 255 + + +def display_text(raw: str | None) -> str | None: + """A user-supplied display string on its way into one of those columns: stripped, truncated, + blank -> None. + + One definition because there are SIX writers and they had drifted: `POST /api/downloads`, + `POST /api/downloads/edit`, `POST /api/downloads/upload`, the metadata PATCH's two fields, and + the Google sign-in callback. Only the PATCH truncated. Truncating is the right answer rather + than refusing: the name is a label on work the user has already done — a file already + transferred, an account already authenticated — and losing its tail is not worth failing the + request over. + + `test_upload.py` holds both halves: that every route assigning one of these fields goes through + this helper (parsed, not grepped), and that each of those routes truncates rather than 500s. A + sweep is exactly the kind of thing that gets done five-sixths of the way, which is how this + ended up being written twice. + """ + return (raw or "").strip()[:DISPLAY_TEXT_MAX] or None + + from app.utils import canonical_email @@ -114,7 +142,7 @@ class User(Base, TimestampMixin, CanonicalEmailMixin): # not cleared eagerly — it simply stops counting). diagnostics_level: Mapped[str | None] = mapped_column(String(8)) diagnostics_until: Mapped[datetime | None] = mapped_column(DateTime(timezone=True)) - display_name: Mapped[str | None] = mapped_column(String(255)) + display_name: Mapped[str | None] = mapped_column(String(DISPLAY_TEXT_MAX)) avatar_url: Mapped[str | None] = mapped_column(String(1024)) role: Mapped[str] = mapped_column(String(16), default="user", server_default="user") # The user's own YouTube channel id, resolved lazily (channels.list?mine=true) and @@ -953,11 +981,11 @@ class DownloadJob(Base, TimestampMixin, UpdatedAtMixin): ForeignKey("download_profiles.id", ondelete="SET NULL") ) profile_snapshot: Mapped[dict] = mapped_column(JSON) - display_name: Mapped[str | None] = mapped_column(String(255)) + display_name: Mapped[str | None] = mapped_column(String(DISPLAY_TEXT_MAX)) # Per-user display overrides + extras (like display_name — metadata only, never the file). # `display_uploader`/`display_uploader_url` override the asset's auto channel name/link; # `extra_links` is an optional list of extra reference URLs shown on the card + watch page. - display_uploader: Mapped[str | None] = mapped_column(String(255)) + display_uploader: Mapped[str | None] = mapped_column(String(DISPLAY_TEXT_MAX)) display_uploader_url: Mapped[str | None] = mapped_column(Text) extra_links: Mapped[list | None] = mapped_column(JSON) status: Mapped[str] = mapped_column( diff --git a/backend/app/routes/auth.py b/backend/app/routes/auth.py index 405acb02..2fbca25b 100644 --- a/backend/app/routes/auth.py +++ b/backend/app/routes/auth.py @@ -39,7 +39,7 @@ from app.auth import ( ) from app.config import settings from app.db import SessionLocal, get_db -from app.models import AuthToken, DemoWhitelist, Invite, OAuthToken, User +from app.models import AuthToken, DemoWhitelist, Invite, OAuthToken, User, display_text from app.ratelimit import RateLimiter, client_key from app.security import encrypt, hash_password, hash_token, verify_password from app.utils import canonical_email, valid_email @@ -472,7 +472,7 @@ async def callback( if email: user.google_email = email _sync_google_email(db, user, email) - user.display_name = userinfo.get("name") + user.display_name = display_text(userinfo.get("name")) user.avatar_url = userinfo.get("picture") # ADMIN_EMAILS (env) is the bootstrap admin list and always wins, so the configured admin can't # be locked out. Everyone else's role is managed in the admin UI and stored in the DB, so a @@ -594,7 +594,7 @@ def _complete_link( if linked_email: user.google_email = linked_email # see the sign-in callback: the other half of the identity if not user.display_name: - user.display_name = userinfo.get("name") + user.display_name = display_text(userinfo.get("name")) if not user.avatar_url: user.avatar_url = userinfo.get("picture") _store_token(db, user, token) diff --git a/backend/app/routes/debug_sessions.py b/backend/app/routes/debug_sessions.py index 3054364f..76345ea1 100644 --- a/backend/app/routes/debug_sessions.py +++ b/backend/app/routes/debug_sessions.py @@ -22,7 +22,7 @@ from sqlalchemy.orm import Session from app import audit, debugsession, logs, logsink from app.audit import AuditAction from app.auth import require_human -from app.db import get_db +from app.db import get_db, release_read_connection from app.issues import bundle, gitea from app.ratelimit import RateLimiter from app.refusal import refused @@ -335,7 +335,13 @@ def _upload_target( return owned_or_404(db, DebugSession, sid, user, what="debug_session") -async def _image_body(request: Request) -> bytes: +async def _image_body(request: Request, db: Session = Depends(get_db)) -> bytes: + # The owner check above has already run, and it was a SELECT — so this session is holding a + # pooled connection `idle in transaction` and would hold it for the whole 10 MiB upload, from a + # client that may be on a phone. Hand it back first; the handler's own work re-takes one. The + # `session` object the handler holds is expired by this and reloads on its next attribute read, + # which is the correct moment to touch the database again. + release_read_connection(db) return await capped_body(request, debugsession.SCREENSHOT_MAX_BYTES) diff --git a/backend/app/routes/diagnostics.py b/backend/app/routes/diagnostics.py index bdcfc9b9..8a7d4ddf 100644 --- a/backend/app/routes/diagnostics.py +++ b/backend/app/routes/diagnostics.py @@ -31,7 +31,7 @@ from sqlalchemy.orm import Session from app import debugsession, diagnostics, logs, logsink from app.auth import current_user -from app.db import get_db +from app.db import get_db, release_read_connection from app.models import DebugSession, LogEvent, User from app.ratelimit import RateLimiter from app.routes._common import capped_body @@ -73,7 +73,7 @@ class ClientEventsIn(BaseModel): events: list[ClientEvent] = Field(max_length=EVENTS_MAX) -async def _parsed_events(request: Request) -> ClientEventsIn: +async def _parsed_events(request: Request, db: Session = Depends(get_db)) -> ClientEventsIn: """The body as the model — parsed only after the size cap, which is why this is a dependency and not a plain body parameter (FastAPI reads a declared body in full before anything else runs). @@ -83,6 +83,10 @@ async def _parsed_events(request: Request) -> ClientEventsIn: media_type = request.headers.get("content-type", "").split(";")[0].strip().lower() if media_type != "application/json" and not media_type.endswith("+json"): raise HTTPException(status_code=415, detail={"code": "json_required"}) + # `current_user` resolved above on a SELECT, so this session is holding a pooled connection. + # Give it back before awaiting the body — the sink is a background beacon and its sender may be + # on a link far slower than the 512 KiB suggests. See `db.release_read_connection`. + release_read_connection(db) raw = await capped_body(request, BODY_MAX_BYTES) try: return ClientEventsIn.model_validate_json(raw) diff --git a/backend/app/routes/downloads.py b/backend/app/routes/downloads.py index dc50f71e..d96a95fd 100644 --- a/backend/app/routes/downloads.py +++ b/backend/app/routes/downloads.py @@ -24,7 +24,7 @@ from sqlalchemy.orm import Session from app import audit, sysconfig from app.auth import admin_user, require_human from app.config import settings -from app.db import get_db +from app.db import get_db, release_read_connection from app.downloads import credentials as dlcreds from app.downloads import edit as editmod from app.downloads import links as linksmod @@ -39,6 +39,7 @@ from app.models import ( DownloadShare, MediaAsset, User, + display_text, ) from app.routes._common import capped_stream_to, get_or_404, not_found, owned_or_404 from app.security import hash_password @@ -400,7 +401,7 @@ def enqueue_download( status_code=422, detail={"code": "url_blocked", "reason": e.reason} ) from e spec, profile_id = _resolve_spec(db, user, body.profile_id, body.spec) - display_name = (body.display_name or "").strip() or None + display_name = display_text(body.display_name) try: job = service.enqueue(db, user.id, source_kind, source_ref, spec, profile_id, display_name) except quota.QuotaExceeded as e: @@ -463,17 +464,11 @@ async def spool_upload( empty, quota, probe or move error, a client that vanished mid-upload — it is removed.""" cap = sysconfig.get_int(db, "download_upload_max_bytes") user_id = user.id - # Hand the pooled connection back BEFORE the transfer starts. Resolving `user` and reading the - # cap were both SELECTs, and SQLAlchemy 2.0 autobegins a transaction on the first one — so - # without this the session sits `idle in transaction` holding one of the twenty pooled - # connections for as long as the client cares to keep sending. That is the invariant `db.py` - # sizes the pool on, and `plex/playback.py` records the production incident that taught it; - # reading the body here instead of letting FastAPI spool it (P1) is what put this route at risk - # of breaking it. Auth only reads, so ending the read transaction loses nothing, and the - # handler's first query autobegins a fresh one — one ROLLBACK round-trip against a minute of - # held connection. Everything this dependency still needs is already an int in hand: touching - # an ORM attribute below would reload `user` and check a connection straight back out. - db.rollback() + # Hand the pooled connection back BEFORE the transfer starts — see `db.release_read_connection` + # for why, and for the precondition it enforces. Everything this dependency still needs is + # already an int or a header in hand: touching an ORM attribute below would reload `user` and + # check a connection straight back out. + release_read_connection(db) staging = Path(settings.download_root) / ".staging" / "uploads" staging.mkdir(parents=True, exist_ok=True) fd, tmp = tempfile.mkstemp(dir=str(staging)) @@ -491,7 +486,7 @@ async def spool_upload( size=size, user_id=user_id, filename=_header_text(request, UPLOAD_FILENAME_HEADER) or "upload", - display_name=service.display_text(_header_text(request, UPLOAD_DISPLAY_NAME_HEADER)), + display_name=display_text(_header_text(request, UPLOAD_DISPLAY_NAME_HEADER)), ) finally: tmp_path.unlink(missing_ok=True) @@ -546,7 +541,7 @@ def enqueue_edit( if source_job.status != "done" or source_job.asset_id is None: raise refused(409, "edit_unfinished") spec = body.edit_spec - display_name = (body.display_name or "").strip() or None + display_name = display_text(body.display_name) try: job = service.enqueue_edit(db, user.id, source_job, spec, display_name) except quota.QuotaExceeded as e: @@ -690,9 +685,9 @@ def update_download_meta( job = owned_or_404(db, DownloadJob, job_id, user, what="download") sent = body.model_fields_set if "display_name" in sent: - job.display_name = service.display_text(body.display_name) + job.display_name = display_text(body.display_name) if "display_uploader" in sent: - job.display_uploader = service.display_text(body.display_uploader) + job.display_uploader = display_text(body.display_uploader) if "display_uploader_url" in sent: job.display_uploader_url = _clean_url(body.display_uploader_url) if "extra_links" in sent: diff --git a/backend/app/worker.py b/backend/app/worker.py index 1aac2d5a..c5bd2ced 100644 --- a/backend/app/worker.py +++ b/backend/app/worker.py @@ -43,7 +43,7 @@ from app.downloads.jobdb import ( _staging_dir, ) from app.downloads.status import STOPPED_BY_ROUTE, JobStatus -from app.models import DownloadJob, MediaAsset +from app.models import DISPLAY_TEXT_MAX, DownloadJob, MediaAsset log = logs.get_logger(__name__) _stop = threading.Event() @@ -562,7 +562,7 @@ def _finish_from_cache(job_id: int, asset_id: int) -> None: fields = {"status": JobStatus.DONE, "progress": 100, "phase": None} if not job.display_name and asset.title: # display_name is VARCHAR(255); a title can be longer (e.g. a recipe in the title). - fields["display_name"] = asset.title[:255] + fields["display_name"] = asset.title[:DISPLAY_TEXT_MAX] asset.last_access_at = datetime.now(timezone.utc) # Atomic finalize: only marks the job done while it is still running, so a concurrent # cancel/pause is never resurrected (see service.finalize_if_running). diff --git a/backend/tests/test_body_limit.py b/backend/tests/test_body_limit.py index e6d4149a..315b583e 100644 --- a/backend/tests/test_body_limit.py +++ b/backend/tests/test_body_limit.py @@ -214,9 +214,9 @@ def test_the_default_ceiling_clears_the_largest_body_a_default_route_takes(): # site the drift guard could not see (deep-review round 1). The names come from `_common.py` itself # below, so a THIRD sibling cannot be added without this list noticing either. CAPPED_BODY_SITES = { - ("diagnostics.py", "capped_body", "BODY_MAX_BYTES"), - ("debug_sessions.py", "capped_body", "debugsession.SCREENSHOT_MAX_BYTES"), - ("downloads.py", "capped_stream_to", "cap"), + ("diagnostics.py", "_parsed_events", "capped_body", "BODY_MAX_BYTES"), + ("debug_sessions.py", "_image_body", "capped_body", "debugsession.SCREENSHOT_MAX_BYTES"), + ("downloads.py", "spool_upload", "capped_stream_to", "cap"), } @@ -228,43 +228,91 @@ def _body_readers() -> set[str]: return { node.name for node in tree.body - if isinstance(node, ast.AsyncFunctionDef) and node.name.startswith("capped_") + if isinstance(node, (ast.AsyncFunctionDef, ast.FunctionDef)) + and node.name.startswith("capped_") } def test_the_registry_covers_every_body_reading_helper(): - """The mutation guard for the test below: if a helper were renamed or added, the site list would - still match itself while the new one went uncovered.""" + """The mutation guard for the tests below: if a helper were renamed or added, the site list + would still match itself while the new one went uncovered.""" assert _body_readers() == {"capped_body", "capped_stream_to"}, ( "routes/_common.py grew or renamed a body-reading helper — add its call sites to " "CAPPED_BODY_SITES and this expectation." ) -def test_every_body_reading_site_is_known_and_fits_under_its_ceiling(): - """Parsed, not grepped. A regex over the call text has to guess how the arguments are spelled, - and the shapes it guesses wrong — a bare `4 * 1024 * 1024`, a parameter not named `request` — - are the natural ways the NEXT site gets written, so a miss reads as "no new sites" rather than - as a failure. `ast` sees every call whatever it looks like.""" +def _body_reading_functions() -> dict[tuple[str, str], dict]: + """Every function under `app/routes/` that reads a request body itself, keyed by + (module, function). + + Parsed, not grepped. A regex over the call text has to guess how the arguments are spelled, and + the shapes it guesses wrong — a bare `4 * 1024 * 1024`, a parameter not named `request` — are + the natural ways the NEXT site gets written, so a miss reads as "no new sites" rather than as a + failure. `ast` sees every call whatever it looks like. + + Keyed by the ENCLOSING FUNCTION, not just the module: a set keyed by module and limit + expression collapses two call sites that spell their limit the same way, which is exactly what + a second route copied from the first would do. + """ readers = _body_readers() routes_dir = pathlib.Path(__file__).resolve().parents[1] / "app" / "routes" - found: set[tuple[str, str, str]] = set() - for py in routes_dir.rglob("*.py"): + found: dict[tuple[str, str], dict] = {} + for py in sorted(routes_dir.rglob("*.py")): tree = ast.parse(py.read_text("utf-8"), filename=str(py)) - for node in ast.walk(tree): - if not isinstance(node, ast.Call): + for fn in ast.walk(tree): + if not isinstance(fn, (ast.AsyncFunctionDef, ast.FunctionDef)): continue - fn = node.func - name = fn.attr if isinstance(fn, ast.Attribute) else getattr(fn, "id", None) - if name not in readers: + calls = [ + node + for node in ast.walk(fn) + if isinstance(node, ast.Call) + and ( + node.func.attr + if isinstance(node.func, ast.Attribute) + else getattr(node.func, "id", None) + ) + in readers + ] + if not calls: continue - limit = next( - (kw.value for kw in node.keywords if kw.arg == "limit"), - node.args[2] if name == "capped_stream_to" and len(node.args) > 2 else None, + names = [ + node.func.attr + if isinstance(node.func, ast.Attribute) + else getattr(node.func, "id", None) + for node in calls + ] + limits = [] + for node, name in zip(calls, names): + limit = next((kw.value for kw in node.keywords if kw.arg == "limit"), None) + if limit is None: + pos = 2 if name == "capped_stream_to" else 1 + limit = node.args[pos] if len(node.args) > pos else None + limits.append(ast.unparse(limit) if limit is not None else "") + releases = any( + isinstance(node, ast.Call) + and ( + node.func.attr + if isinstance(node.func, ast.Attribute) + else getattr(node.func, "id", None) + ) + == "release_read_connection" + for node in ast.walk(fn) ) - if limit is None and name != "capped_stream_to": - limit = node.args[1] if len(node.args) > 1 else None - found.add((py.name, name, ast.unparse(limit) if limit is not None else "")) + found[(py.name, fn.name)] = { + "helpers": names, + "limits": limits, + "releases": releases, + } + return found + + +def test_every_body_reading_site_is_known_and_fits_under_its_ceiling(): + found = { + (module, fn, site["helpers"][i], site["limits"][i]) + for (module, fn), site in _body_reading_functions().items() + for i in range(len(site["helpers"])) + } assert found == CAPPED_BODY_SITES, ( f"a body-reading call site appeared, moved or changed shape: {found ^ CAPPED_BODY_SITES}. " "Each one declares a body size; check it against bodylimit.DEFAULT_MAX_BODY_BYTES and, if " @@ -279,6 +327,59 @@ def test_every_body_reading_site_is_known_and_fits_under_its_ceiling(): assert ("POST", "/api/downloads/upload") in bodylimit.ROUTE_MAX_BODY_BYTES +def test_release_read_connection_ends_the_transaction_it_was_called_for(db): + """The helper itself: a SELECT has autobegun, and afterwards it has not.""" + from sqlalchemy import select + + from app.db import release_read_connection + + db.execute(select(SystemConfig).limit(1)) + assert db.in_transaction(), "nothing autobegan, so this test would pass vacuously" + release_read_connection(db) + assert not db.in_transaction() + + +def test_release_read_connection_refuses_to_discard_pending_work(db): + """The precondition, ENFORCED rather than described. + + The session belongs to the whole request, not to the dependency calling this — so if something + ahead of it ever stages a write, ending the transaction would throw that write away with + nothing in the log. `service.enqueue` already learned that a whole-session rollback takes the + caller's pending work with it, which is why it uses a SAVEPOINT. A comment cannot stop the next + dependency from being added in the wrong place; this can. + """ + from app.db import release_read_connection + + db.add(SystemConfig(key="probe_pending_write", value="1")) + with pytest.raises(RuntimeError, match="pending work"): + release_read_connection(db) + db.rollback() # the staged row is this test's own + + +def test_every_body_reading_site_hands_its_pooled_connection_back_first(): + """Reading the body yourself is what makes a request SLOW inside its own database session. + + The auth (or ownership) dependency that must resolve first is a SELECT, so SQLAlchemy has + autobegun and the session is holding one of the pool's connections; awaiting the body then pins + it for as long as the client keeps sending — one connection per in-flight upload, `idle in + transaction`, with nothing to time it out. `db.release_read_connection` ends that transaction, + and every one of these sites needs it. + + Held generically because the three sites arrived one at a time and only the newest was fixed: + a fourth body-reading route gets the same requirement automatically. + """ + offenders = [ + f"{module}:{fn}" + for (module, fn), site in _body_reading_functions().items() + if not site["releases"] + ] + assert not offenders, ( + "a route reads its own request body while still inside its read transaction, pinning a " + f"pooled connection for the whole transfer: {offenders}. Call " + "db.release_read_connection(db) before the await — see routes/downloads.py:spool_upload." + ) + + def test_the_upload_ceiling_and_the_admin_knob_cannot_drift_apart(): """The upload route's in-handler cap is a `sysconfig` value an admin can change; the edge is a constant. If the knob could be set above the edge, an admin raising it would silently start diff --git a/backend/tests/test_upload.py b/backend/tests/test_upload.py index d3a230b2..6e137819 100644 --- a/backend/tests/test_upload.py +++ b/backend/tests/test_upload.py @@ -12,8 +12,10 @@ parsing (image → file, cover-art skip, dims, duration) is covered directly by below, which stubs one level down (`run_ffprobe_json`) and exercises the real `probe_media`. """ +import ast import asyncio import io +import pathlib from pathlib import Path import pytest @@ -23,10 +25,10 @@ from app.config import settings from app.routes._common import capped_stream_to from sqlalchemy import select -from app.downloads import mediakind, quota, service, storage +from app.downloads import mediakind, quota, storage from app.downloads import probe as probemod from app.downloads import upload as uploadmod -from app.models import DownloadJob, DownloadQuota, MediaAsset +from app.models import DISPLAY_TEXT_MAX, DownloadJob, DownloadQuota, MediaAsset # --- pure: the kind invariant --------------------------------------------------------------- @@ -422,12 +424,99 @@ def test_a_non_ascii_filename_survives_the_header_round_trip( assert stored.startswith("nyaralas_osszel"), stored +def test_a_long_display_name_is_truncated_on_EVERY_route_that_takes_one( + db, as_user, make_user, _root, monkeypatch, upload_headers +): + """The sweep, held end to end rather than at one site. + + `display_text` was introduced for the upload path and applied to three of the five writers of + this varchar(255) column; `POST /api/downloads` and `POST /api/downloads/edit` kept the old + `(x or "").strip() or None` and would still have answered a bare 500 (a psycopg DataError, + which main.py re-raises for everything but the NUL case) to a name a user simply pasted. Found + by four independent readers in deep-review round 2 — a sweep is exactly the kind of thing that + gets done four-fifths of the way, so this holds all of it at once. + """ + _probe(monkeypatch, ("h264", "aac", 640, 480, 60)) + monkeypatch.setattr(uploadmod.editmod, "ensure_poster", lambda _a, _root: None) + long_name = "n" * 300 + bound = "n" * DISPLAY_TEXT_MAX + c = as_user(make_user()) + + # 1. the upload route — the name arrives in a request header. + up = c.post( + "/api/downloads/upload", content=b"video", headers=upload_headers("a.mp4", long_name) + ) + assert up.status_code == 200, up.text + assert up.json()["display_name"] == bound + + # 2. the enqueue route — a plain JSON field, from the Add-download dialog. An inline `spec` + # rather than a profile id, so the test does not have to seed the builtin profile table. + enq = c.post( + "/api/downloads", + json={"source": "dQw4w9WgXcQ", "spec": {"kind": "video"}, "display_name": long_name}, + ) + assert enq.status_code == 200, enq.text + assert enq.json()["display_name"] == bound + + # 3. the edit route — the same field, from the editor. + edit = c.post( + "/api/downloads/edit", + json={ + "source_job_id": up.json()["id"], + "edit_spec": {"trim": {"start_s": 0, "end_s": 1}}, + "display_name": long_name, + }, + ) + assert edit.status_code == 200, edit.text + assert edit.json()["display_name"] == bound + + # 4 + 5. the metadata PATCH's two fields. + patched = c.patch( + f"/api/downloads/{up.json()['id']}", + json={"display_name": long_name, "display_uploader": long_name}, + ) + assert patched.status_code == 200, patched.text + assert patched.json()["display_name"] == bound + assert patched.json()["display_uploader"] == bound + + +def test_no_route_assigns_a_display_field_without_the_shared_helper(): + """The drift guard for the test above: it holds five routes, this holds the rule. + + Parsed, not grepped. Every `display_name = ...` / `display_uploader = ...` assignment under + `app/routes/` takes a string a user typed and puts it on its way to a varchar(255) column, so + every one of them must go through `display_text`. A sixth route added tomorrow fails here + instead of 500ing in production. + """ + routes_dir = pathlib.Path(__file__).resolve().parents[1] / "app" / "routes" + offenders: list[str] = [] + for py in sorted(routes_dir.rglob("*.py")): + tree = ast.parse(py.read_text("utf-8"), filename=str(py)) + for node in ast.walk(tree): + if not isinstance(node, ast.Assign): + continue + names = { + t.id if isinstance(t, ast.Name) else t.attr + for t in node.targets + if isinstance(t, (ast.Name, ast.Attribute)) + } + if not names & {"display_name", "display_uploader"}: + continue + value = ast.unparse(node.value) + if "display_text" not in value: + offenders.append(f"{py.name}:{node.lineno}: {value}") + assert not offenders, ( + "a display field is assigned without models.display_text, so a name longer " + f"than the column is a 500 rather than a truncation: {offenders}" + ) + + def test_the_display_text_bound_matches_the_columns_it_guards(): """A number living in two places is how the `cancelled`/`canceled` bug happened. `display_text` truncates to `DISPLAY_TEXT_MAX`; the columns it feeds are `String(255)`. Held against the model itself, so widening one without the other fails here rather than as a DataError in production.""" for name in ("display_name", "display_uploader"): - assert DownloadJob.__table__.c[name].type.length == service.DISPLAY_TEXT_MAX + assert DownloadJob.__table__.c[name].type.length == DISPLAY_TEXT_MAX def test_the_display_name_header_reaches_the_job_and_is_bounded( @@ -456,7 +545,7 @@ def test_the_display_name_header_reaches_the_job_and_is_bounded( "/api/downloads/upload", content=b"data2", headers=upload_headers("clip2.bin", long_name) ) assert bounded.status_code == 200, bounded.text - assert bounded.json()["display_name"] == "n" * service.DISPLAY_TEXT_MAX + assert bounded.json()["display_name"] == "n" * DISPLAY_TEXT_MAX def test_the_upload_holds_no_db_transaction_while_the_body_streams( @@ -477,22 +566,57 @@ def test_the_upload_holds_no_db_transaction_while_the_body_streams( _probe(monkeypatch, (None, None, None, None, None)) seen: dict[str, bool] = {} - real = downloads_routes.capped_stream_to + real_stream = downloads_routes.capped_stream_to + real_release = downloads_routes.release_read_connection - async def spy(request, out, limit, refusal): - seen["in_transaction"] = bool(db.in_transaction()) - return await real(request, out, limit, refusal) + def spy_release(session): + # The PRECONDITION, so this test cannot pass vacuously. Asserting only "no transaction + # while streaming" would stay green if the auth/cap reads stopped opening one at all — + # and under `as_user` the auth SELECT is already overridden away, so that is a live risk + # rather than a theoretical one. + seen["before"] = bool(session.in_transaction()) + real_release(session) + seen["after_release"] = bool(session.in_transaction()) - monkeypatch.setattr(downloads_routes, "capped_stream_to", spy) + async def spy_stream(request, out, limit, refusal): + seen["while_streaming"] = bool(db.in_transaction()) + return await real_stream(request, out, limit, refusal) + + monkeypatch.setattr(downloads_routes, "release_read_connection", spy_release) + monkeypatch.setattr(downloads_routes, "capped_stream_to", spy_stream) c = as_user(make_user()) r = c.post("/api/downloads/upload", content=b"data", headers=upload_headers("held.bin")) assert r.status_code == 200, r.text - assert seen["in_transaction"] is False, ( + assert seen["before"] is True, ( + "nothing had opened a transaction before the release, so this test proves nothing — the " + "reads it is meant to follow must have moved or been stubbed away" + ) + assert seen["after_release"] is False + assert seen["while_streaming"] is False, ( "the request's session was inside a transaction when the transfer began, so a pooled " "connection is pinned for the whole upload" ) +def test_the_usage_route_reports_the_per_file_upload_cap(as_user, make_user, db): + """The dialog refuses an over-cap file off this number, and it is the only layer that produces + a refusal carrying the limit on a default install — so the field is load-bearing, not + decorative. Nothing held it: the route had no test at all, and the frontend's `DownloadUsage` + is an unvalidated cast, so dropping the key would have been green everywhere (round 2). + """ + from app import sysconfig + + c = as_user(make_user()) + r = c.get("/api/downloads/usage") + assert r.status_code == 200, r.text + assert r.json()["upload_max_bytes"] == sysconfig.get_int(db, "download_upload_max_bytes") + + # And it is the KNOB, not a constant: a lowered override must show through. + sysconfig.set_value(db, "download_upload_max_bytes", 4096) + db.commit() + assert c.get("/api/downloads/usage").json()["upload_max_bytes"] == 4096 + + def test_a_refused_upload_leaves_nothing_in_the_staging_directory( as_user, make_user, db, _root, monkeypatch, upload_headers ): diff --git a/frontend/src/components/UploadDialog.dom.test.tsx b/frontend/src/components/UploadDialog.dom.test.tsx index 28e7a671..e8cdfd05 100644 --- a/frontend/src/components/UploadDialog.dom.test.tsx +++ b/frontend/src/components/UploadDialog.dom.test.tsx @@ -84,30 +84,79 @@ describe("UploadDialog", () => { expect(btn.disabled).toBe(true); }); - it("refuses an over-cap file here, naming the limit, instead of sending it", async () => { + // `file.size > cap`, on the boundary in both directions — the cap is inclusive server-side + // (backend test_upload.py pins that a body of EXACTLY the cap is accepted), so a dialog that + // refused it would make a legal file un-uploadable with no way round. Two sizes either side of + // one cap, because a single over-cap case passes just as happily with `>` loosened to `>=`. + const CAP = 8; + const pickFile = (bytes: number, name = "f.bin") => + fireEvent.change(fileInput(), { + target: { files: [new File(["x".repeat(bytes)], name)] }, + }); + + it("refuses a file one byte over the cap, naming the limit, instead of sending it", async () => { // Both server-side caps answer only after the transfer — and on a default install the one that - // answers is the edge ceiling, whose refusal carries no number at all. So the dialog checks the - // size itself: the user learns the limit before a byte moves. - h.downloadUsage.mockResolvedValue({ upload_max_bytes: 4 }); + // answers is the edge ceiling, whose refusal carries no number at all and may not even reach + // the browser. So the dialog checks the size itself: the user learns the limit before a byte + // moves. + h.downloadUsage.mockResolvedValue({ upload_max_bytes: CAP }); wrap(); await waitFor(() => expect(h.downloadUsage).toHaveBeenCalled()); - fireEvent.change(fileInput(), { - target: { files: [new File(["far too many bytes"], "big.bin")] }, - }); + pickFile(CAP + 1, "big.bin"); await waitFor(() => expect(screen.getByText("downloads.errors.upload.tooLarge")).toBeTruthy()); const btn = screen.getByText("downloads.upload.submit").closest("button")!; expect(btn.disabled).toBe(true); + // The drop zone carries the reason, because a natively disabled Submit is not in the tab order + // to be asked for it. + const zone = screen.getByText("big.bin").closest("button")!; + expect(zone.getAttribute("aria-invalid")).toBe("true"); + expect(zone.getAttribute("aria-describedby")).toBe(screen.getByRole("alert").id); + }); + + it("swallows a Submit press that arrives while the button still holds focus", async () => { + // The OTHER half of "never sent": the test above covers the natively disabled button (not + // focused), this one covers the soft-disabled one. `ui/Button` is focus-safe, so a button that + // turns unavailable while it HOLDS focus stays in the tab order and stays clickable, and it is + // the primitive's own click guard that refuses — measured: deleting the `tooBig` term from + // `submit()` changes nothing here, which is why that term is documented as belt and braces + // rather than as the mechanism. What this pins is the user-visible contract: pressing Upload + // after dropping a bigger file does not start a transfer the server will refuse. + h.downloadUsage.mockResolvedValue({ upload_max_bytes: CAP }); + wrap(); + await waitFor(() => expect(h.downloadUsage).toHaveBeenCalled()); + pickFile(CAP, "fits.bin"); + const btn = screen.getByText("downloads.upload.submit").closest("button")!; + btn.focus(); + fireEvent.focus(btn); + pickFile(CAP + 1, "toobig.bin"); + await waitFor(() => expect(screen.getByRole("alert")).toBeTruthy()); + expect(btn.disabled).toBe(false); // soft-disabled, still focusable — and still clickable fireEvent.click(btn); expect(h.uploadDownload).not.toHaveBeenCalled(); }); - it("sends a file that fits, so the check above is not simply refusing everything", async () => { - h.downloadUsage.mockResolvedValue({ upload_max_bytes: 1_000_000 }); + it("sends a file of EXACTLY the cap, which the server accepts", async () => { + h.downloadUsage.mockResolvedValue({ upload_max_bytes: CAP }); wrap(); await waitFor(() => expect(h.downloadUsage).toHaveBeenCalled()); - fireEvent.change(fileInput(), { target: { files: [new File(["small"], "ok.bin")] } }); + pickFile(CAP, "exact.bin"); + expect(screen.queryByRole("alert")).toBeNull(); fireEvent.click(screen.getByText("downloads.upload.submit")); await waitFor(() => expect(h.uploadDownload).toHaveBeenCalledTimes(1)); }); + + it("re-announces the refusal when a SECOND over-cap file is picked", async () => { + // Failing the same way again has to be announced again. The message names the CAP, not the + // file, so two over-cap picks leave the node and its text identical — and a live region that + // is never touched says nothing. See formAlerts.dom.test.tsx, which holds the same rule for + // the three other alerts in this app that can repeat. + h.downloadUsage.mockResolvedValue({ upload_max_bytes: CAP }); + wrap(); + await waitFor(() => expect(h.downloadUsage).toHaveBeenCalled()); + pickFile(CAP + 1, "a.bin"); + const first = await screen.findByRole("alert"); + pickFile(CAP + 2, "b.bin"); + await waitFor(() => expect(screen.getByRole("alert")).not.toBe(first)); + }); }); diff --git a/frontend/src/components/UploadDialog.tsx b/frontend/src/components/UploadDialog.tsx index 99f80fae..d66531ad 100644 --- a/frontend/src/components/UploadDialog.tsx +++ b/frontend/src/components/UploadDialog.tsx @@ -1,4 +1,4 @@ -import { useEffect, useRef, useState } from "react"; +import { useEffect, useId, useRef, useState } from "react"; import { useTranslation } from "react-i18next"; import { useQuery, useQueryClient } from "@tanstack/react-query"; import { UploadCloud, File as FileIcon } from "lucide-react"; @@ -12,6 +12,7 @@ import { invalidateDownloads } from "../lib/downloads"; import { notify } from "../lib/notifications"; import { navigateTo } from "../lib/nav"; import { formatBytes } from "../lib/format"; +import { DISPLAY_NAME_MAX } from "../lib/api/downloads"; import { inputCls } from "./ui/form"; import { FocusSafeButton } from "./ui/FocusSafeButton"; @@ -40,6 +41,18 @@ export default function UploadDialog({ onClose }: { onClose: () => void }) { const cap = useQuery({ queryKey: qk.downloadUsage(), queryFn: api.downloadUsage }).data ?.upload_max_bytes; const tooBig = !!file && !!cap && file.size > cap; + const refusalId = useId(); + // Every pick gets a new number, so the refusal below REMOUNTS rather than being reconciled. Two + // over-cap picks in a row leave the node and its text identical — the message names the cap, not + // the file — and a live region that is not touched is not announced, so the second refusal was + // silent to a screen reader. `KeyGate`, `SetupWizard` and `Welcome` all key their alerts for the + // same reason, and `formAlerts.dom.test.tsx` is the test that exists because of it. + const [pickSeq, setPickSeq] = useState(0); + const pick_ = (f: File) => { + setFile(f); + setName(""); + setPickSeq((n) => n + 1); + }; // Closing the dialog mid-upload (X / ESC / backdrop / browser Back all unmount us) must abort the // in-flight XHR — otherwise it keeps streaming in the background and still toasts success. Only the @@ -49,6 +62,10 @@ export default function UploadDialog({ onClose }: { onClose: () => void }) { const pick = () => inputRef.current?.click(); const submit = async () => { + // `tooBig` here is belt and braces, NOT the mechanism: `ui/Button` is focus-safe, so while the + // button holds focus it swallows the activation itself, and while it does not it is natively + // disabled. Measured — deleting this term breaks nothing, which is exactly why it must not be + // described as the guard. It is here for the next caller of `submit`, not for the only one. if (!file || busy || tooBig) return; const ac = new AbortController(); abortRef.current = ac; @@ -103,14 +120,20 @@ export default function UploadDialog({ onClose }: { onClose: () => void }) { dragDepth.current = 0; setDragOver(false); const f = e.dataTransfer.files?.[0]; - if (f) { - setFile(f); - setName(""); - } + if (f) pick_(f); }} disabled={busy} + // The refusal is tied to the control it invalidates, not left to a one-shot announcement: + // the drop zone reads as invalid and points at the sentence, so a reader who tabs back to + // it hears WHY, and the border says the same thing without colour being the only signal. + aria-invalid={tooBig} + aria-describedby={tooBig ? refusalId : undefined} className={`w-full flex flex-col items-center justify-center gap-2 rounded-xl border-2 border-dashed px-4 py-8 text-center transition disabled:opacity-60 ${ - dragOver ? "border-accent bg-accent/5" : "border-border hover:border-fg/40" + tooBig + ? "border-danger" + : dragOver + ? "border-accent bg-accent/5" + : "border-border hover:border-fg/40" }`} > {file ? ( @@ -134,31 +157,31 @@ export default function UploadDialog({ onClose }: { onClose: () => void }) { className="hidden" onChange={(e) => { const f = e.target.files?.[0]; - if (f) { - setFile(f); - setName(""); - } + if (f) pick_(f); e.target.value = ""; // let re-picking the same file fire onChange again }} /> {tooBig && ( -

+

)} -