fix(uploads): make both caps answerable, and stop holding a connection
The epic-end deep review found three defects in P1 and four tests that did not hold what they claimed. They land together because they share the upload route and its test module; splitting by hunk would have produced commits that do not individually run green. * The edge ceiling and the route's own cap are the SAME number at the default configuration, so the edge always answers first: a declared Content-Length over the cap is refused before a byte is read, and an undeclared body trips the middleware on the same chunk the route would have tripped on. The route's numbered 422 -- and the drain written for it last week -- are reachable only once an admin LOWERS the knob, which is the one shape the browser UAT tested. Measured: headroom does not fix this, because the declared-length check fires first for any body big enough to matter and the drain pushes the count back over the edge for the rest. So the edge's own refusal is made readable instead: `errors.tooLarge` localizes `too_large` the way `upload_too_large` was already localized, and the dialog refuses an over-cap file BEFORE sending it, off the `upload_max_bytes` that GET /api/downloads/usage now reports. Measured in Chrome against the API directly: that 413 arrives as `onload` with a readable body in 43 ms. * Reading the body behind the auth dependency put the whole transfer inside the request's database session -- the auth SELECT autobegins a transaction, so one of the twenty pooled connections sat `idle in transaction`, with nothing to time it out, for as long as the client cared to keep sending. That is the invariant db.py sizes the pool on and plex/playback.py records an incident for. The read transaction now ends before the await. Measured on the dev server: an 1818 ms upload shows no idle-in-transaction sample while the bytes move, only afterwards, when the handler actually writes. * The display name moved into a request header, where nothing bounds its length, while the column stayed varchar(255) -- so a long enough name was a DataError 500 after the whole file had already transferred. One `display_text` helper now serves all three writers (the metadata PATCH already truncated, the upload ingest did not), and the dialog stops at the same number. * The gate invariant asked `route.dependant.body_params`, which holds only the route's OWN callable's parameters: a body declared in a SUB-DEPENDENCY -- the upload route's own idiom -- leaves it empty while FastAPI still reads the body before auth. It asks `route.body_field` now, which is what FastAPI itself consults. The AST registry of body-reading call sites had also never followed `capped_stream_to`, so the one route the whole invariant exists for was the single site the drift guard could not see. Four tests that passed over their own mutation are fixed: the cap boundary (a `>` loosened to `>=` survived the entire suite), the percent-decoding (every filename the suite sends is ASCII, where quote() is the identity function), the display-name header (no test at all), and the staging cleanup (deleting the unlink left the suite green while every refused upload leaked its partial bytes onto the volume). Every test added or changed here was mutation-checked.
This commit is contained in:
@@ -21,10 +21,12 @@ from fastapi import HTTPException
|
||||
|
||||
from app.config import settings
|
||||
from app.routes._common import capped_stream_to
|
||||
from app.downloads import mediakind, quota, storage
|
||||
from sqlalchemy import select
|
||||
|
||||
from app.downloads import mediakind, quota, service, storage
|
||||
from app.downloads import probe as probemod
|
||||
from app.downloads import upload as uploadmod
|
||||
from app.models import DownloadQuota, MediaAsset
|
||||
from app.models import DownloadJob, DownloadQuota, MediaAsset
|
||||
|
||||
|
||||
# --- pure: the kind invariant ---------------------------------------------------------------
|
||||
@@ -369,6 +371,149 @@ def test_upload_route_enforces_per_file_cap(
|
||||
assert r.json()["detail"]["code"] == "upload_too_large"
|
||||
|
||||
|
||||
def test_the_cap_admits_a_body_of_EXACTLY_the_cap_and_refuses_one_byte_more(
|
||||
as_user, make_user, db, _root, monkeypatch, upload_headers
|
||||
):
|
||||
"""The boundary, which the test above cannot see: it sends 16 bytes against a cap of 5, so a
|
||||
`>` silently loosened to `>=` refuses it just the same and the mutation survives the whole
|
||||
suite. The cap is `size > limit`, so a file of exactly the cap must be ACCEPTED.
|
||||
"""
|
||||
from app import sysconfig
|
||||
|
||||
_probe(monkeypatch, (None, None, None, None, None))
|
||||
payload = b"x" * 64
|
||||
sysconfig.set_value(db, "download_upload_max_bytes", len(payload))
|
||||
db.commit()
|
||||
c = as_user(make_user())
|
||||
|
||||
at_the_cap = c.post(
|
||||
"/api/downloads/upload", content=payload, headers=upload_headers("exact.bin")
|
||||
)
|
||||
assert at_the_cap.status_code == 200, at_the_cap.text
|
||||
|
||||
over = c.post(
|
||||
"/api/downloads/upload", content=payload + b"x", headers=upload_headers("over.bin")
|
||||
)
|
||||
assert over.status_code == 422
|
||||
assert over.json()["detail"] == {"code": "upload_too_large", "limit": len(payload)}
|
||||
|
||||
|
||||
def test_a_non_ascii_filename_survives_the_header_round_trip(
|
||||
as_user, make_user, db, _root, monkeypatch, upload_headers
|
||||
):
|
||||
"""The percent-decoding, which every other test in this file leaves vacuous.
|
||||
|
||||
Headers are latin-1 on the wire, so the client `encodeURIComponent`s the name and the route
|
||||
`unquote`s it. Every filename the suite sends is pure ASCII — and `quote()` is the IDENTITY
|
||||
function on ASCII — so deleting the `unquote` call changes nothing anywhere else. A Hungarian
|
||||
name is the only input that can tell the two apart: without the decode the sanitizer would be
|
||||
handed the literal `nyaral%C3%A1s.bin` and would keep the percent-escapes in the stored name.
|
||||
"""
|
||||
_probe(monkeypatch, (None, None, None, None, None))
|
||||
c = as_user(make_user())
|
||||
r = c.post(
|
||||
"/api/downloads/upload", content=b"data", headers=upload_headers("nyaralás ősszel.bin")
|
||||
)
|
||||
assert r.status_code == 200, r.text
|
||||
asset = db.execute(select(MediaAsset).order_by(MediaAsset.id.desc())).scalars().first()
|
||||
assert asset is not None
|
||||
stored = Path(asset.rel_path).name
|
||||
assert "%" not in stored and "c3" not in stored.lower(), stored
|
||||
assert stored.startswith("nyaralas_osszel"), stored
|
||||
|
||||
|
||||
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
|
||||
|
||||
|
||||
def test_the_display_name_header_reaches_the_job_and_is_bounded(
|
||||
as_user, make_user, _root, monkeypatch, upload_headers
|
||||
):
|
||||
"""The second header, which had no backend test at all — and the length nothing bounded.
|
||||
|
||||
Since P1 the display name arrives in a REQUEST HEADER rather than as a form field, so the only
|
||||
limit on it was the header size. The column is varchar(255), so a longer one reached Postgres as
|
||||
a DataError: a 500 AFTER the whole file had been transferred. It is truncated now, by the same
|
||||
helper the metadata PATCH uses.
|
||||
"""
|
||||
_probe(monkeypatch, (None, None, None, None, None))
|
||||
c = as_user(make_user())
|
||||
|
||||
ok = c.post(
|
||||
"/api/downloads/upload",
|
||||
content=b"data",
|
||||
headers=upload_headers("clip.bin", "Nyári vakáció"),
|
||||
)
|
||||
assert ok.status_code == 200, ok.text
|
||||
assert ok.json()["display_name"] == "Nyári vakáció"
|
||||
|
||||
long_name = "n" * 300
|
||||
bounded = c.post(
|
||||
"/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
|
||||
|
||||
|
||||
def test_the_upload_holds_no_db_transaction_while_the_body_streams(
|
||||
as_user, make_user, db, _root, monkeypatch, upload_headers
|
||||
):
|
||||
"""P1 moved the body read behind the auth dependency, which put the whole transfer INSIDE the
|
||||
request's database session: resolving the caller and reading the cap are SELECTs, SQLAlchemy 2.0
|
||||
autobegins on the first one, and the session then holds one of the twenty pooled connections —
|
||||
`idle in transaction` to Postgres, with no `idle_in_transaction_session_timeout` to end it — for
|
||||
as long as the client keeps sending. `db.py` sizes the pool on that not happening and
|
||||
`plex/playback.py` records the production incident behind the rule.
|
||||
|
||||
Asserted on the session rather than on `engine.pool.checkedout()`: the transaction is the cause,
|
||||
the checkout is its consequence (measured), and the pool counter is shared with whatever else
|
||||
the test process is holding.
|
||||
"""
|
||||
from app.routes import downloads as downloads_routes
|
||||
|
||||
_probe(monkeypatch, (None, None, None, None, None))
|
||||
seen: dict[str, bool] = {}
|
||||
real = downloads_routes.capped_stream_to
|
||||
|
||||
async def spy(request, out, limit, refusal):
|
||||
seen["in_transaction"] = bool(db.in_transaction())
|
||||
return await real(request, out, limit, refusal)
|
||||
|
||||
monkeypatch.setattr(downloads_routes, "capped_stream_to", spy)
|
||||
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, (
|
||||
"the request's session was inside a transaction when the transfer began, so a pooled "
|
||||
"connection is pinned for the whole upload"
|
||||
)
|
||||
|
||||
|
||||
def test_a_refused_upload_leaves_nothing_in_the_staging_directory(
|
||||
as_user, make_user, db, _root, monkeypatch, upload_headers
|
||||
):
|
||||
"""`spool_upload` writes the body to a staging temp file as it arrives, so a refusal happens
|
||||
with a partial file already on disk — its `finally` is what removes it. Nothing asserted that:
|
||||
deleting the unlink left the whole suite green while every refused upload leaked its partial
|
||||
bytes onto the volume, one file per attempt, with no job to ever clean them up.
|
||||
"""
|
||||
from app import sysconfig
|
||||
|
||||
sysconfig.set_value(db, "download_upload_max_bytes", 8)
|
||||
db.commit()
|
||||
staging = Path(settings.download_root) / ".staging" / "uploads"
|
||||
c = as_user(make_user())
|
||||
|
||||
r = c.post("/api/downloads/upload", content=b"x" * 4096, headers=upload_headers("leaky.bin"))
|
||||
assert r.status_code == 422
|
||||
assert staging.exists(), "the dependency did not get as far as creating the staging dir"
|
||||
assert list(staging.iterdir()) == [], "a refused upload left its partial file behind"
|
||||
|
||||
|
||||
def test_upload_cap_cannot_be_disabled_with_zero(db):
|
||||
"""download_upload_max_bytes floors at 1, and set_value must enforce it.
|
||||
|
||||
|
||||
Reference in New Issue
Block a user