peter
540ca244c8
Round 1 fixed the address pre-emption chain by letting a later registration replace the password on an unverified account and delete its outstanding verify tokens. Round 2 read that code and showed it was worse than what it fixed, so it goes back out. It closes nothing. The rule is symmetric and an attacker can simply register last — on a timer, inside the five-per-five-minutes budget — while the owner registers once. What it does add is two primitives an unauthenticated caller did not have: overwrite a real registrant's chosen password, and delete the confirmation link already sitting in their inbox (the approval mail's 7-day token is an ordinary verify token, so the blanket delete took that too). Worst of all it fired on the rows SEC-10 left stripped — password_hash NULL, password_revoked_at set, active — installing an attacker's password where the entire state means "there is none", and clearing the stamp that is the only reason password-reset still accepts them. That is a straight takeover which did not exist before, in the code written to prevent one. The owner's recourse needed no new primitive and already worked: password-reset accepts a pre-empted account, proves the mailbox, rewrites the hash and bumps the session epoch. What remains is the bounded window between a confirmation click and that reset, which is SEC-16 — logged with this attempt written down, so the next person does not reach for it again. Two smaller round-2 findings go with it. `upsert_pending_invite`'s new exact-spelling tie-break ran before any status preference, so it could deterministically pick a DENIED sibling over a pending one and reopen it — the very outcome its comment claimed to prevent; the status preference now comes first. And `send_invitation` activated the account before the send it might have to report as failed, so a request the UI called a failure had quietly let someone in; activation now follows a mail that actually left. The verify token minted to render that mail still commits either way — it is single-use, expires on its own and was mailed nowhere, and the note says so rather than leaving the next reader to wonder whether it was missed.
Backend conventions
The rules below are already followed across backend/app — they lived only in a few module
docstrings (demo.py, userscope.py, routes/_common.py) until this file collected them (E3 S3,
B-core-20). When you add code, match these so the next reader doesn't have to guess "which pattern do
I copy".
Module layering
Three layers, by who runs the code — not by feature:
-
routes/…— request-only. A route module holds HTTP concerns: auth dependencies, request/ response models, status-code mapping, serialization. It owns logic that runs only on a request. Business logic that a request happens to trigger does not live here if anything else runs it too. -
plex/,downloads/,sync/— domain logic shared by more than one caller. Anything the scheduler or the worker runs (not just a request) lives in one of these domain packages, so the route, the worker, and the scheduler share one definition. Examples from this codebase:downloads/service.pyowns enqueue + the job state transitions (routes call it);plex/sync.pyis run by both the admin route and the scheduler;downloads/pipeline.pyis the worker's child pipeline. -
Flat
app/*.pycore modules — cross-cutting primitives.auth,sysconfig,quota,audit,notifications,userscope,realtime,ratelimit,progress, … are single-file primitives many layers import. They depend on nothing above them (a core module must never import aroutes/…module).
Extraction trigger: the SECOND consumer. Keep new logic inline in its first caller. The moment a second caller needs it (a worker as well as a route; a scheduler as well as a route), extract it down a layer — into the relevant domain package, or a flat core module if it's cross-cutting. Don't pre-abstract for a single caller; don't leave shared logic duplicated once there are two.
Transaction ownership
This follows CLAUDE.md's canonical rule verbatim — "the handler/job-wrapper commits; helpers flush
(only quota owns its session)" — so read it that way, not as an exception to it. The commit belongs
to the operation owner; anything called inside another transaction only stages its change
(db.add / flush) and lets the owner commit, so the whole operation is one transaction.
audit.record() is the reference: it adds the audit row and the caller commits it together with the
mutation it records, so an audit entry can never exist without its change (or vice-versa).
- The operation owner is usually the route handler. A
service.*operation the handler delegates a whole transition to (e.g.service.cancel) is that owner and commits its one transition — it isn't a mid-operation helper. A helper that owns its OWN short-lived session (the worker'sjobdbwrites, the same carve-out CLAUDE.md grantsquota) commits that session. Everything else flushes. SessionLocalisexpire_on_commit=False: after a rawUPDATE+ commit, an ORM object you already loaded is stale. Read the fresh value from the row (RETURNING, or a re-SELECT), not the ORM attribute — seeservice.decr_ref_count.
Shared counters and status: do it atomically at the DB
- Never read-modify-write a shared counter in Python (
x.count = x.count + 1). Concurrent sessions lose an update. Use a DB-atomic statement —col = col + 1, orgreatest(col - 1, 0)withRETURNINGso the caller branches on the post-value under the row lock (service.incr_ref_count/decr_ref_count). - Download job status is a
JobStatusStrEnum (downloads/status.py), not bare strings. TheHOLDINGset defines which states hold aMediaAsset(so release accounting stays correct);STOPPED_BY_ROUTEis the route-initiated stop set. A worker child's terminal write goes throughservice.finalize_if_running(an atomicUPDATE … WHERE status='running') so a finishing child can't resurrect a job a route just stopped.
Config: one home per kind
app.config.settings— boot/env config (DB URL, download root, worker concurrency): read once at startup, not user-tunable at runtime.sysconfig.SPECS— the runtime admin knobs (DB-override-or-default). A new admin-tunable value goes inSPECS; don't invent a fourth store. (state.py'smaintenance_batchpredates this and is slated to fold intosysconfig— do not add more like it.)
The gate
Everything below must be green before a change is done (run via siftlode check):
ruff (backend) · tsc ×3 + eslint + prettier + knip (frontend) · vitest · pytest. The
pytest lane runs in backend/docker-compose.test.yml — an ephemeral Postgres + the backend tree
bind-mounted — so the DB-backed integration tests (the db fixture) actually run; SIFTLODE_TEST_ REQUIRE_DB=1 there turns a would-be silent skip into a hard failure. See docs/development.md.