peter
90fc86d437
Round 4 was targeted at the surfaces rounds 2 and 3 changed. 29 findings raised, 26 survived deduplication, 12 survived three adversarial readings each. `set_delivery` renewed the claim but never checked it was still ours, while its sibling `release_send` always did. `expire_on_commit` is off, so the token it re-stamped came from the caller's own memory, not the row: a send that outlived its 600 s lease came back, overwrote the take-over send's committed links with a full-column write, and put its own dead token back. It now takes the token, reads the row's claim with a targeted scalar (not a refresh, which would discard the `issue_url` assigned one line earlier) and raises `SendClaimLost` rather than writing. An issue created by a send that had already lost its claim is logged with its number — that record is the only one that will exist. The tracker's settings are read above the pre-call checkpoint instead of inside `_client`. Reading them there was a SELECT one line after the commit that was meant to end the transaction, and held it — idle, snapshot pinned — across the repository check and the create. The comment and the module docstring said otherwise; measuring it in pg_stat_activity is what settled it. Two more comments were saying things the code had stopped doing: the `**state` spread does not carry the claim (`set_delivery` strips and re-stamps it), and react-query does not debounce the preview — the editor being unmounted is what paces it. The composer stopped stranding the focus on `<body>`: a send that creates the issue and then fails on an attachment unmounts the button that was just pressed, and only the success path handed the focus on. Export no longer saves into a refusal it does not need — the zip is the way out of a stuck send, and its pre-save was the one step that could not survive one. The delivery banner announces through a role instead of an `aria-live` created with its own message. `historyState` returns a union, so `chipTone` matching on the same keys is checked by tsc rather than by hope. PlexPlayer was the only full-screen overlay in the tree with no focus containment at all — Tab walked out of an opaque player into a page nobody can see. It now carries the shared trap. Closing that hole exposed the one underneath it: `useDismiss` registered as a layer that traps the Tab while installing no trap, so the player stood its own down for its own menus. Measured in a real browser: 30 Tabs with the settings panel open, 9 escapes into the nav rail, and 0 after. Escape ordering is unchanged — the panel first, then the player. Three tests that did not hold their line: the advisory lock's call site (deleting it left the suite green), the trail assertion whose `all()` was vacuous over an empty filter, and a `layerStack` assertion the stand-in dialog already forced. All eleven executable fixes mutation-checked red for the right reason; the two comment corrections have nothing to pin.
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/,issues/— 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;issues/bundle.pyis the one definition of a bug report's files, for the zip export and the tracker. -
Flat
app/*.pycore modules — cross-cutting primitives.auth,sysconfig,quota,audit,notifications,userscope,realtime,ratelimit,progress,logs(the one logging configuration),logsink(itslog_eventstable),diagnostics(a user's consented log level) anddebugsession(a recording that tags its user's lines, E16), … 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.