peter
321776480e
Deep-review round 3, targeted at the surfaces round 2's fix commit changed. Five of the eight findings are the same shape: the guards written to stop half-done sweeps were themselves half-done. test_upload.py's display-field guard walked only `ast.Assign`, so the one writer with an unbounded REQUEST HEADER behind it — `spool_upload`'s `display_name=display_text(...)` keyword — was invisible to the guard meant to protect it. It also stopped at `app/routes/`, missing the four writers in `downloads/` that actually construct the row, and asserted `not offenders`, which enforces nothing once a refactor moves the writers out of its view. It now walks assignments AND keywords across `app/`, and holds exact set equality against a registry of all twelve write sites with each one's value. test_body_limit.py's release guard used `any(... ast.walk(fn))`, which accepts a `release_read_connection` that runs AFTER the await it exists to precede — satisfying the letter of the rule while pinning a connection for exactly the transfer it was supposed to free. Position is checked now, and a late release is its own, louder failure. The registry keyed sites by file basename while scanning recursively, and `admin.py`, `discover.py` and `playlists.py` each exist both at the top level and under `plex/` — the same entry-collapse the registry was widened to close. Keyed by relative path now. And all three guards recognised a body read only by the `capped_` naming convention, so a route spelling `await request.body()` inline declared no ceiling, released no connection, and was invisible to every one of them. `release_read_connection`'s precondition claimed the caller "must have NOTHING PENDING, and that is checked rather than described" while checking only UNFLUSHED ORM work. Measured: after `add`+`flush` all three collections are empty and the rollback loses the row, and Core DML never enters them at all — both of them this codebase's canonical write shapes. It now also asks the transaction directly via `pg_current_xact_id_if_assigned()`. `errors.tooLarge` was reworded to "try a smaller file", but that key answers every route's body cap: the issue composer's over-size JSON draft got advice about a file it does not have. Back to payload-neutral; the file-specific sentence keeps its own key. The upload dialog's new danger border outranked `dragOver`, so a user replacing an over-cap file got no sign the drop zone was live — and with a file picked the label shows its name rather than "drop it here", so the border was the only feedback left. Four further findings were refuted 3/3 and are not fixed: `aria-invalid` on the drop zone (measured in Chromium — the AX node does carry it on a native button), a bidi-override spoof through `display_text` (the served filename already strips those categories, and the proposed filter would have broken every ZWJ emoji), and `capped_body` not draining (the screenshot route's edge ceiling equals its own cap, so that refusal is unreachable, and the log sink is sent by beacon with nobody reading the answer).
84 lines
4.1 KiB
Python
84 lines
4.1 KiB
Python
from collections.abc import Iterator
|
|
|
|
from sqlalchemy import create_engine, text
|
|
from sqlalchemy.orm import DeclarativeBase, Session, sessionmaker
|
|
|
|
from app.config import settings
|
|
|
|
|
|
class Base(DeclarativeBase):
|
|
pass
|
|
|
|
|
|
# Pool sized above the sync-endpoint threadpool (~40) so a burst of short DB sessions can't starve on
|
|
# checkout. The image proxies deliberately DON'T hold a connection during their slow external fetch
|
|
# (see routes/plex/), so this headroom is only for the many brief sessions, not long ones.
|
|
engine = create_engine(
|
|
settings.database_url,
|
|
pool_pre_ping=True,
|
|
pool_size=20,
|
|
max_overflow=30,
|
|
pool_timeout=30,
|
|
future=True,
|
|
# A database error renders its bound parameters into its message — a title, an uploader, a
|
|
# file path, a URL — and that message is logged and stored as a job's error text. The SQL
|
|
# stays; the values are data, and the ids an operator needs are on the line already.
|
|
hide_parameters=True,
|
|
)
|
|
SessionLocal = sessionmaker(bind=engine, autoflush=False, expire_on_commit=False)
|
|
|
|
|
|
def get_db() -> Iterator[Session]:
|
|
db = SessionLocal()
|
|
try:
|
|
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.
|
|
|
|
TWO checks, because the obvious one is blind to the shapes this codebase actually writes in.
|
|
`db.new`/`deleted`/`dirty` hold only UNFLUSHED unit-of-work: the moment a helper calls
|
|
`db.flush()` — CLAUDE.md's canonical split is "the handler commits; helpers flush" — the object
|
|
is persistent with no unflushed history and all three sets are empty again, and Core DML
|
|
(`db.execute(update(...))`) never enters them at all. Measured: after `add`+`flush` the sets are
|
|
empty, the old check passed, and the rollback lost the row. So the transaction is asked
|
|
directly. Postgres assigns a transaction id on the first WRITE and never for a read, so
|
|
`pg_current_xact_id_if_assigned()` is NULL for exactly the state this function is safe in — and
|
|
it sees a raw `text("UPDATE …")` too, which no session-level bookkeeping would.
|
|
"""
|
|
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."
|
|
)
|
|
if (
|
|
db.in_transaction()
|
|
and db.execute(text("SELECT pg_current_xact_id_if_assigned()")).scalar() is not None
|
|
):
|
|
raise RuntimeError(
|
|
"release_read_connection() on a transaction that has already WRITTEN: something ahead "
|
|
"of this dependency flushed a row or ran an UPDATE/INSERT/DELETE, which ending the "
|
|
"transaction would discard. Commit it first, or do it after the await."
|
|
)
|
|
db.rollback()
|